Skip to content

test(core): cover clilogs secret redaction in sendBuildLogs #2412

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ninadbstack merged 4 commits into from
Sep 7, 2026
Merged

test(core): cover clilogs secret redaction in sendBuildLogs #2412

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1952bc8
Redact secrets from CLI logs, not just CI logs
ninadbstack Sep 1, 2026
cc770be
Merge master into PER-9354-redact-clilogs
ninadbstack Sep 1, 2026
06aeddd
test(core): suppress semgrep on the fake AWS key, make cilogs asserti…
ninadbstack Sep 4, 2026
32af7aa
test(core): match a keyword pattern instead of an AWS key literal
ninadbstack Sep 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions packages/core/test/percy.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1540,6 +1540,55 @@ describe('Percy', () => {
]));
});

it('redacts secrets from CLI logs, not just CI logs', async () => {
process.env.PERCY_TOKEN = 'PERCY_TOKEN';
delete process.env.PERCY_CLIENT_ERROR_LOGS;
percy = new Percy({
token: 'PERCY_TOKEN',
snapshot: { widths: [1000] },
discovery: { concurrency: 1 },
clientInfo: 'client-info',
environmentInfo: 'env-info'
});
percy.build = { id: 1 };

// Matches the "AWS cred file info" rule in secretPatterns.yml, which keys
// on the bare `aws_secret_access_key` keyword rather than on a key value.
// That exercises redaction without committing anything credential-shaped:
// a real key literal trips Semgrep's hardcoded-credential rule, and an
// inline `// nosemgrep` only zeroes semgrep's own blocking count -- the
// finding still reaches the SARIF and surfaces as a code-scanning alert.
// The unit under test here is sendBuildLogs applying redactSecrets, not
// any particular regex in the pattern file, so any matching string works.
Comment on lines +1555 to +1562

@ninadbstack ninadbstack Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Low] Inline commentary is unusually long for a single spec

Roughly 14 comment lines across two blocks. The content is accurate and the two non-obvious parts genuinely earn their place — why a keyword pattern rather than a key literal, and why a CI-tagged entry has to go through logger.instance. The Semgrep/nosemgrep-SARIF paragraph, though, is repo-process history rather than something a reader needs in order to understand the test.

Suggestion: Optionally condense the Semgrep rationale to a single line (the PR description already records the full story for posterity), keeping the pattern-choice and logger.instance notes. Not blocking.

Reviewer: stack-code-reviewer

const secretish = 'aws_secret_access_key';

// A CLI-side log entry can carry upstream response text, so it needs the
// same redaction cilogs already gets. `percy.log` is a namespace group
// bound as log(name, level, ...), so a CI-tagged entry has to be written
// through the instance directly -- passing a third arg to
// percy.log.info() does nothing, since log() only takes (message, meta).
percy.log.info(`leaked from upstream: ${secretish}`);
logger.instance.log('ci', 'info', `ci side: ${secretish}`);

await expectAsync(percy.sendBuildLogs()).toBeResolved();
expect(api.requests['/logs']).toBeDefined();

const sent = JSON.parse(Pako.ungzip(
Buffer.from(api.requests['/logs'][0].body.data.content, 'base64'),
{ to: 'string' }
));

const clilogs = JSON.stringify(sent.clilogs);
const cilogs = JSON.stringify(sent.cilogs);
// Both halves must be present, or an assertion below passes vacuously.
expect(sent.clilogs.length).toBeGreaterThan(0);
expect(sent.cilogs.length).toBeGreaterThan(0);
expect(clilogs).not.toContain(secretish);
expect(clilogs).toContain('[REDACTED]');
expect(cilogs).not.toContain(secretish);
expect(cilogs).toContain('[REDACTED]');

@ninadbstack ninadbstack Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Low] No case pairs PERCY_CLIENT_ERROR_LOGS=false with a secret-bearing clilogs entry

This spec always sends both halves, so it does not confirm that clilogs redaction is independent of the CI-log toggle. The existing should not add CI logs if PERCY_CLIENT_ERROR_LOGS is false spec (:1474) carries no secret fixture, so that combination is currently uncovered.

Suggestion: Optionally add a short case setting PERCY_CLIENT_ERROR_LOGS=false with a secret in a CLI-side entry, asserting clilogs is still redacted and cilogs is absent. Not required for this regression guard to be effective.

Reviewer: stack-code-reviewer

});

it('should catch the error in sending build logs', async () => {
process.env.PERCY_TOKEN = 'PERCY_TOKEN';
delete process.env.PERCY_CLIENT_ERROR_LOGS;
Expand Down
Loading