Repository navigation
test(core): cover clilogs secret redaction in sendBuildLogs - #2412
Conversation
sendBuildLogs applied redactSecrets to cilogs but sent clilogs through untouched. CLI log entries can carry upstream response text — SDK errors interpolate remote response fields into their messages — and /logs content is retrievable via GET /api/v1/logs by anyone with build read access, so the half that most often holds foreign response data was the unredacted half. Also memoize the pattern file. redactSecrets recurses per log entry and re-read + re-parsed the ~1750-rule YAML and recompiled every regex on each call; that was tolerable for cilogs alone but not once clilogs (the larger set) goes through it. Patterns are now parsed and compiled once. Reusing the global regexes across replace() calls is safe — replace() resets lastIndex on a global pattern. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Master landed the same fix in #2279 (security: redact CLI logs, bound regex matching). Both conflicting hunks resolved in favour of master's version, which is a superset of this branch's: same redactSecrets call on clilogs in sendBuildLogs, same compile-patterns-once memoization, plus in-place entry mutation, the ReDoS bound and a semgrep annotation this branch didn't have. What remains of this branch is the integration-level sendBuildLogs redaction test; master's coverage for #2279 is unit-level in test/unit/utils.test.js. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…on real
Two fixes to the clilogs redaction spec:
Semgrep. `semgrep ci` runs diff-aware, so the spec's four literal copies of
ASIAY34FZKBOKMUTVV7A were the only thing scanned and produced 4 blocking
findings from one hardcoded-AWS-credential Code rule (exit 1). The string is
the published AWS documentation sample for an STS temporary access key ID, not
a live credential. Hoist it to a single `fakeAwsKey` const so there is one
literal, and suppress that one line with `// nosemgrep` -- the preceding-line
form already used in packages/dom/test, honored because the workflow pins
semgrep 1.164.0 for exactly this reason.
Vacuous assertion. `percy.log` is a namespace group bound as
log(name, level, ...), and Logger#log only takes (debug, level, message, meta),
so the third `true` arg in percy.log.info(msg, {}, true) was silently dropped.
That entry landed in clilogs with debug === 'percy', leaving cilogs empty, so
`expect(cilogs).not.toContain(key)` was asserting against '[]'. Write the
CI-tagged entry through logger.instance.log('ci', 'info', ...) -- matching how
cli-exec/src/exec.js:140 does it via logger('ci') -- and assert both arrays are
non-empty plus that both carry [REDACTED].
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ninadbstack
changed the title
`// nosemgrep` took semgrep/ci from 4 blocking findings to 0, but the suppressed finding still lands in the SARIF, so GitHub code scanning raised it anyway and the "Semgrep OSS" check reported "1 new alert including 1 error". Suppressing semgrep's own exit code is not enough. Drop the credential-shaped literal entirely and match secretPatterns.yml's "AWS cred file info" rule instead, which keys on the bare `aws_secret_access_key` keyword rather than on a key value. Semgrep's hardcoded-credential rules need an actual secret value, so there is nothing left to flag and no suppression is needed. This does not weaken the spec: the unit under test is sendBuildLogs applying redactSecrets to both log halves, not any particular regex in the pattern file, so any string the file matches serves equally well. It also leaves no credential-shaped literal in the repo, which is better hygiene than a suppressed one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ninadbstack
left a comment
ninadbstack
left a comment
There was a problem hiding this comment.
Claude Code Review (automated) — 2 inline finding(s). Full report in the PR comment below. Verdict: Passed.
| // 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. |
There was a problem hiding this comment.
[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
| expect(clilogs).not.toContain(secretish); | ||
| expect(clilogs).toContain('[REDACTED]'); | ||
| expect(cilogs).not.toContain(secretish); | ||
| expect(cilogs).toContain('[REDACTED]'); |
There was a problem hiding this comment.
[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
Claude Code PR ReviewPR: #2412 • Head: 32af7aa • Reviewers: stack-code-reviewer SummaryTest-only PR adding a regression spec — Review Table
Test isolation was checked specifically: the env-var mutations ( FindingsAll three findings are Low and non-gating.
Notes on other signals (non-gating)
Verdict: PASS — a focused, genuinely-effective regression guard; the three findings are Low nits and optional follow-ups. |
3 participants
What
Adds regression coverage for
clilogssecret redaction insendBuildLogs. Test-only — the fix itself is already onmaster.Why this is test-only
I wrote this branch to fix
sendBuildLogsapplyingredactSecretstocilogsbut notclilogs. That fix had already landed in #2279 (3f329840, "security: redact CLI logs, bound regex matching (ReDoS)") as its PER-8609 hunk, along with thesecretPatterns.ymlmemoization, for the same reason I'd hit:redactSecretsre-read and re-parsed ~1.7k regexes per recursive call, which becomes real once the full CLI log array goes through it.Merging
masterin resolved the overlap in master's favour, sopackages/core/src/percy.jsandpackages/core/src/utils.json this branch are now byte-identical tomaster. The only remaining delta is the spec.What #2279 did not add is a test —
grep -n 'REDACTED\|redact' packages/core/test/percy.test.jsonmasterreturns nothing. So the redaction on theclilogspath is currently unguarded against regression. This adds that guard.The test
redacts secrets from CLI logs, not just CI logsinpackages/core/test/percy.test.jsdecodes the actual gzipped/logspayload and asserts, for both halves, that the credential-shaped string is gone and[REDACTED]is present.Two details worth calling out for review:
logger.instance.log('ci', 'info', …).percy.logis a namespace group bound aslog(name, level, …), andLogger#logonly takes(debug, level, message, meta)— so thepercy.log.info(msg, {}, true)idiom used by the neighbouring specs silently drops the third arg, and the entry lands inclilogswithdebug === 'percy'. This mirrors howpackages/cli-exec/src/exec.js:140produces CI entries withlogger('ci'). (loggerin this test file is the helpers object, not callable, hence going through.instance.)expect(...).not.toContain(key)can pass vacuously against an empty[].Semgrep
Proving redaction needs a string the pattern file matches, and the obvious choice — a literal AWS access key ID — trips Semgrep's hardcoded-credential rule.
semgrep ciis diff-aware, so this file was the only thing scanned and it produced 4 blocking findings, one per copy of the literal.Worth recording for the next person, since the repo has hit this before: an inline
// nosemgrepis not sufficient. It takessemgrep cifrom 4 blocking findings to 0 and the job exits 0, but the suppressed finding still lands insemgrep.sarif, which the workflow uploads to GitHub code scanning — so theSemgrep OSScheck independently reported "1 new alert including 1 error" and stayed red. That partially reconciles the two conflicting notes in the tree:.semgrepignoresays twice that inline// nosemgrep"is not honored by the CI semgrep version", whilesemgrep.ymlpins image 1.164.0 to fix exactly that. Both are half right — it is honored for the exit code, not for the SARIF.So the spec matches
secretPatterns.yml's "AWS cred file info" rule instead, which keys on the bareaws_secret_access_keykeyword rather than on a key value:Semgrep's hardcoded-credential rules need an actual secret value, so there is nothing left to flag and no suppression is needed anywhere. This doesn't weaken the spec — the unit under test is
sendBuildLogsapplyingredactSecretsto both halves, not any particular regex in the pattern file — and it leaves no credential-shaped literal in the repo, which is better than a suppressed one.Rejected alternatives: assembling the key at runtime (
['ASIA', …].join('')) keeps the AWS shape but relies on semgrep not constant-folding it; addingpackages/core/test/percy.test.jsto.semgrepignorewould drop scanning on a 2000-line file to silence one line.Testing
Linux
Test @percy/coreis green: the spec passes on the first attempt with no retries consumed,Executed 1240 of 1240 specs SUCCESS. Both Semgrep checks are green.Windows went green on the previous push only via the spec-level retry: the first attempt failed on
Discovery captures favicon when the server provides one(packages/core/test/discovery.test.js:407), a pre-existing flake in a different spec file, annotated by CI as "@percy/core flaked on the first attempt and was recovered by a spec-level retry (tracked in PER-9011)". This spec itself passed on Windows on the first attempt.Note that
semgrep/ciis also red onmaster(8cb91228) independently of this branch, since the non-diff-aware push scan reports pre-existing findings.I could not run the suite or ESLint locally (no
node_modulesin my environment), so CI is the verification for both.Context
Found while assessing PER-9354. That chain finding does not reproduce (details in the ticket); this coverage gap is an independent thing surfaced along the way.
🤖 Generated with Claude Code