security: redact CLI logs, bound regex matching (ReDoS) (PER-8609/8615)

[Shivanshu-07]

Summary

Second focused percy-cli security PR — two contained runtime findings in @percy/core.

Ticket	CWE	Finding
PER-8609	CWE-532	CLI logs transmitted to Percy API without secret redaction
PER-8615	CWE-1333	ReDoS via user-controlled regex in snapshot include/exclude

PER-8616 (SSRF via unvalidated PERCY_CHROMIUM_BASE_URL) was originally part of this PR but has been removed — the ticket is closed as won't-fix (machine-access: exploiting it requires attacker control of the process environment). install.js is restored to its original download behavior.

Changes

percy.js (PER-8609): sendBuildLogs() sent clilogs raw while cilogs were already passed through redactSecrets(). Wrap clilogs in the same redactSecrets() so tokens / credential-bearing URLs are stripped before egress. A Percy-token pattern is added to secretPatterns.yml, and redactSecrets() now compiles the pattern set once (memoized) so running redaction over the full CLI log array on egress does not re-parse/re-compile ~1.7k regexes per string.

snapshot.js (PER-8615): snapshotMatches() runs user-controllable regex/glob patterns against snapshot.name. A crafted long name reaching the matcher (e.g. via the local API — chain PER-8627) plus a backtracking-prone pattern could hang the process. Added MAX_MATCH_INPUT_LENGTH = 2048: glob/RegExp matching is skipped for over-long names (exact-string matching is unaffected, and real snapshot names are short).

Verification (against real source)

• redactSecrets on the clilogs array shape: GitHub token + bearer + Percy token redacted to [REDACTED]; utils.test.js covers all Percy-token prefixes. ✅
• ReDoS guard: catastrophic (a+)+$ against a 50k-char input returns in 0 ms (would otherwise hang); normal patterns still match. ✅
• Existing sendBuildLogs tests use secret-free messages (redaction is a no-op → content matches). ✅

Closes PER-8609, PER-8615. Mitigates the ReDoS leg of PER-8627.

🤖 Generated with Claude Code

[Shivanshu-07]

Claude Code PR Review

PR: #2279 • Head: bda111a • Reviewers: stack:code-reviewer

Summary

Three runtime-hardening measures in @percy/core:

1. PER-8609 (CWE-532): redact secrets from CLI logs before they are sent to the Percy API — clilogs and cilogs are both wrapped with redactSecrets() in percy.js:sendBuildLogs.
2. PER-8615 (CWE-1333 / ReDoS): bound snapshot-name pattern matching to a max input length in snapshot.js.
3. PER-8616 (CWE-918 / SSRF): validate PERCY_CHROMIUM_BASE_URL via resolveChromiumBaseUrl() — require a well-formed HTTPS URL, else warn and fall back to the trusted default host.

Review Table

Priority	Category	Check	Status	Notes
High	Security	Secrets redacted before egress	Pass	Both clilogs (percy.js:869) and cilogs wrapped with redactSecrets()
High	Security	SSRF / integrity-downgrade closed	Pass	resolveChromiumBaseUrl enforces parseable + HTTPS, else trusted-default fallback
High	Security	ReDoS input bound	Pass	Match input length-capped
High	Correctness	No test regression	Pass	Verified: redactSecrets is byte-identical on non-secret logs, so the sendBuildLogs assertions encode identical content
Medium	Testing	New security paths tested	Fail	Rejection branches of resolveChromiumBaseUrl and the ReDoS length guard lack unit tests
Medium	Quality	Focused change	Pass	—

Findings

• File: packages/core/src/install.js (resolveChromiumBaseUrl) + test/unit/install.test.js

• Severity: Medium

• Issue: The invalid-URL and non-HTTPS rejection branches (the security-critical paths) have no test coverage; only a valid HTTPS URL is exercised. A future refactor could silently drop the protocol check.

• Suggestion: Add tests for an unparseable value and an http:// value, asserting fallback to the default.

• File: packages/core/src/snapshot.js (snapshotMatches length guard)

• Severity: Medium

• Issue: Names exceeding the max length silently fall through to the fallback (effectively "no match"), with no log.warn and no test. A legitimate long name could be unexpectedly excluded with no signal.

• Suggestion: Emit a log.warn when the guard trips and add a boundary test.

• File: packages/core/src/install.js:162

• Severity: Low

• Issue: Returns the raw value (plus trailing slash) rather than reconstructing from parsed — a query string/fragment on the operator-supplied URL survives. Minor; the value is operator-set, not attacker-controlled.

• Suggestion: Return parsed.origin + parsed.pathname (slash-normalised).

Verdict is PASS: no Critical/High issue is introduced or worsened. The two High concerns raised in an earlier review pass (clilogs redaction "not wired"; sendBuildLogs tests breaking) were verified false positives — the redaction is present at percy.js:869, and redactSecrets produces byte-identical output for the non-secret logs the tests use. The Medium test-coverage gaps above are recommended as a fast follow-up.

---

Verdict: PASS

[Shivanshu-07]

Resolved the Semgrep detect-non-literal-regexp finding on packages/core/src/utils.js. The RegExp source strings come only from the first-party, bundled secretPatterns.yml that ships in the package (resolved relative to import.meta.url) - never from remote or attacker-controlled input. This is the same regex construction that already existed on master, just memoized. Added a targeted // nosemgrep on the flagged line with a justification comment rather than changing behavior.

[Shivanshu-07]

Claude Code PR Review

PR: #2279 • Head: d31dfe5 • Reviewers: Claude Code (inline review — no in-repo reviewer skills in scope)

Summary

Hardens the CLI runtime against two security issues: redacts secrets from clilogs before they egress to the Percy API in sendBuildLogs (adds a Percy-token pattern to secretPatterns.yml, CWE-532), and bounds user-controllable snapshot-name regex/glob matching to a 2048-char input to prevent catastrophic backtracking / ReDoS in snapshotMatches (CWE-1333). Also memoizes the ~1.7k compiled secret patterns so the expanded redaction path stays within timeouts. The originally-scoped PERCY_CHROMIUM_BASE_URL validation (8616) was reverted; the net diff cleanly leaves no orphaned references.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	No secrets added; change removes secrets from egressing logs.
High	Security	Authentication/authorization checks present	N/A	No auth surface touched.
High	Security	Input validation and sanitization	Pass	ReDoS input bound (2048) added; redaction runs on egress payload.
High	Security	No IDOR — resource ownership validated	N/A	No resource access changes.
High	Security	No SQL injection (parameterized queries)	N/A	No SQL.
High	Correctness	Logic is correct, handles edge cases	Pass	Exact match preserved when input over bound; non-string names now fail safe. Verified redaction + memoized-regex reuse has no lastIndex state leak.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	sendBuildLogs retains try/catch; string-regexp parse still guarded.
High	Correctness	No race conditions or concurrency issues	Pass	Lazy singleton _compiledSecretPatterns; single-threaded, idempotent init.
Medium	Testing	New code has corresponding tests	Fail	Percy-token redaction is tested (6 prefixes, verified green). The new MAX_MATCH_INPUT_LENGTH ReDoS bound in snapshot.js has no test.
Medium	Testing	Error paths and edge cases tested	Partial	No test asserts an over-2048 name skips glob/regex matching.
Medium	Testing	Existing tests still pass (no regressions)	Pass	utils.test.js 25 specs 0 failures; lint clean on changed files.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Memoization removes per-call YAML parse + recompile (real improvement).
Medium	Performance	Long-running tasks use background jobs	N/A	N/A.
Medium	Quality	Follows existing codebase patterns	Pass	Mirrors existing cilogs redaction and matcher structure.
Medium	Quality	Changes are focused (single concern)	Pass	Scoped to redaction + ReDoS bound; 8616 cleanly reverted.
Low	Quality	Meaningful names, no dead code	Pass	Clear names; no dead code.
Low	Quality	Comments explain why, not what	Pass	Comments give rationale; no ticket ids embedded in source.
Low	Quality	No unnecessary dependencies added	Pass	No new dependencies.

Findings

• File: packages/core/src/snapshot.js:53-84

• Severity: Medium

• Reviewer: Claude Code

• Issue: The new MAX_MATCH_INPUT_LENGTH (2048) ReDoS guard — the primary security behavior of PER-8615 — has no test. Nothing pins that an over-length name skips glob/regex matching while a normal name still matches, so the guard could silently regress.

• Suggestion: Add a snapshot-matching test: a >2048-char name is not matched by a glob/regex include, an exact-string include still matches, and a normal short name matches as before.

• File: packages/core/src/utils.js:637-651 (used by percy.js:869)

• Severity: Medium

• Reviewer: Claude Code

• Issue: Redaction now runs ~1.7k regexes over the full, length-unbounded clilogs content on egress. Unlike snapshotMatches, this path has no input-length bound, so it retains a residual ReDoS surface if any bundled pattern backtracks on attacker-influenced log content (e.g. a crafted URL/DOM string captured into a debug log). Pre-existing for cilogs; this PR expands it to the larger clilogs stream.

• Suggestion: Consider truncating or length-bounding each log string before redaction (consistent with the 2048 bound just added), or verify the bundled pattern set is backtracking-free on long inputs.

• File: packages/core/src/utils.js:643-644

• Severity: Low

• Reviewer: Claude Code

• Issue: Redaction only rewrites each log object's .message field. A secret surfacing in another field (e.g. meta, a nested error) would not be redacted. Pre-existing design, consistent with the prior cilogs behavior — noted for completeness.

• Suggestion: If in scope later, redact string values recursively across all fields rather than only message.

• File: packages/core/src/secretPatterns.yml:7024-7028

• Severity: Low

• Reviewer: Claude Code

• Issue: The Percy Token pattern requires {20,} chars after a fixed prefix set (web|app|auto|ss|vmw|res). A token shorter than 20 chars or using an unlisted prefix would not be redacted. Verified real-length tokens redact; short synthetic tokens (web_short123) do not.

• Suggestion: Confirm the prefix list is exhaustive for current/future token classes and that 20 is a safe lower bound for the shortest real token.

---

Verdict: PASS

[Shivanshu-07]

Claude Code PR Review

PR: #2279 • Head: d31dfe5 • Reviewers: inline security+correctness review

Summary

Security hardening for @percy/core: redact Percy tokens/credentials from clilogs before egress (CWE-532), add a memoized Percy-token secret pattern, and bound user-controllable snapshot-name matching to mitigate ReDoS (CWE-1333). The SSRF leg (PERCY_CHROMIUM_BASE_URL) was reverted (won't-fix).

Review Table

Priority	Category	Check	Status	Notes
High	Security	Secrets redacted on every log egress path (clilogs + cilogs)	Pass	clilogs now wrapped in redactSecrets matching cilogs; both paths covered in sendBuildLogs.
High	Security	New Percy-token pattern matches real token formats	Pass	`(web
High	Security	Added regex free of catastrophic backtracking	Pass	No nested quantifiers; linear on 200k input (~1ms).
High	Security	ReDoS guard covers all user-controllable regex/glob code paths	Pass	glob, string-regexp, and RegExp predicate branches all gated on patternSafe; exact + function branches intentionally exempt.
High	Security	No new SSRF / injection surface introduced	Pass	SSRF leg reverted cleanly; install.js restored to original behavior.
High	Correctness	Memoized global-flag regexes safe to reuse	Pass	Reused only via String.replace(re,…), which is stateless for /g; no lastIndex hazard.
High	Correctness	Exact-string and function matching unaffected by guard	Pass	snapshot.name === predicate and function predicates run regardless of length.
High	Correctness	Reverted Chromium validation leaves no dangling refs/tests	Pass	Net diff contains no orphaned validation code or tests.
Medium	Perf	redactSecrets no longer re-parses YAML / recompiles ~1.7k regexes per call	Pass	Lazy compile-once cache; correct given static bundled patterns.
Medium	Perf	ReDoS input bound actually neutralizes worst-case matching	Fail	See Finding 1 — 2048 cap is far above where exponential backtracking manifests.
Medium	Testing	New tests cover all Percy-token prefixes	Pass	utils.test.js parametrizes all 6 prefixes with positive + negative assertions.
Medium	Testing	Regression coverage for the new snapshot-match guard	Fail	See Finding 2 — no test exercises the over-long-name path in snapshotMatches.
Medium	Testing	Existing sendBuildLogs tests remain valid	Pass	Secret-free fixtures → redaction is a no-op; assertions still hold.
Medium	Quality	Intent/CWE rationale documented in comments	Pass	Clear comments on redaction, memoization, and the bound.
Low	Quality	secretPatterns.yml well-formed (trailing newline, lint)	Pass	Trailing newline added; valid YAML.
Low	Quality	nosemgrep suppression scoped and justified	Pass	Applies only to first-party bundled patterns; rationale in comment.
Low	Quality	Unanchored token pattern over-redaction risk	Pass	Minor over-match possible (e.g. …class_<20+ alnum>); harmless in logs, fails safe.
Low	Quality	Commit history clean / conventional	Pass	Conventional commits; revert isolated to its own commit.

Findings

File: packages/core/src/snapshot.js:53
Severity: Medium
Issue: MAX_MATCH_INPUT_LENGTH = 2048 bounds input length, which defends against polynomial/large-N blowup, but it is set well above the input size at which true exponential (catastrophic) backtracking already manifests. A genuinely backtracking-prone pattern remains expensive on inputs comfortably under the cap, so the guard does not fully achieve its stated CWE-1333 objective. Note the realistic path is compound (a backtracking-prone pattern must originate from the user's own config, plus a route to inject a long name), so this is a hardening gap, not a regression — the change is still a net improvement over the prior unbounded behavior.
Suggestion: Prefer a matching strategy that cannot backtrack pathologically (e.g. a time-bounded/RE2-style matcher or timeout around user-pattern evaluation) rather than relying on a length cap; if keeping a cap, choose a much tighter value aligned to realistic snapshot-name lengths.

File: packages/core/test/unit/utils.test.js:231
Severity: Medium
Issue: The only new tests cover token-prefix redaction. There is no regression test asserting that snapshotMatches skips glob/RegExp evaluation for an over-long snapshot.name (and still honors exact + function matching). The behavioral guard added in snapshot.js is therefore uncovered and could silently regress.
Suggestion: Add a unit test that feeds a name longer than the bound and asserts glob/regexp predicates are not evaluated while exact-string and function predicates still match.

---

Verdict: PASS

[pranavz28]

Automated multi-agent security review. Directionally correct — the sendBuildLogs redaction and the ReDoS bound are sound and the memoization is a real fix (not gold-plating). Two follow-ups worth doing so the CWE-532 mitigation isn't assumed complete: the redaction is applied at one egress but a sibling egress path and non-message fields remain uncovered. Inline comments below.

[Shivanshu-07]

Claude Code PR Review

PR: #2279 • Head: f1ae531 • Reviewers: fallback inline checklist

Summary

Redacts secrets from CLI logs before egress to the Percy API (both the sendBuildLogs clilogs path and the per-snapshot log resource in discovery.js), anchors the Percy-token secret pattern, and bounds user-controllable snapshot-name pattern matching to defeat ReDoS (PER-8609 / PER-8615).

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	Token fixture de-literalized (78f8303) to clear secret-scanning; no live secrets in diff
High	Security	Authentication/authorization checks present	N/A	No auth surface changed
High	Security	Input validation and sanitization	Pass	Snapshot-name length bounded (MAX_MATCH_INPUT_LENGTH=2048) before glob/regexp matching (ReDoS, CWE-1333)
High	Security	No IDOR — resource ownership validated	N/A	No resource ownership logic
High	Security	No SQL injection (parameterized queries)	N/A	No SQL
High	Correctness	Logic is correct, handles edge cases	Pass	redactSecrets mutates message in place, returns same ref; array/primitive branches covered
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	No new catch-swallow; egress wrapped as before
High	Correctness	No race conditions or concurrency issues	Pass	Pattern compile memoized once; no shared mutable state introduced
Medium	Testing	New code has corresponding tests	Pass	77 new lines in utils.test.js: redaction, in-place, meta-untouched, no-false-positive, token prefixes
Medium	Testing	Error paths and edge cases tested	Pass	null/undefined/number/bool pass-through covered
Medium	Testing	Existing tests still pass (no regressions)	Pass	cli-exec (Windows) re-run green; discovery too-large instrumentation preserved
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Patterns compiled once (a3388a3) — avoids O(patterns) re-read per call
Medium	Performance	Long-running tasks use background jobs	N/A	—
Medium	Quality	Follows existing codebase patterns	Pass	nosemgrep justification matches repo convention; comments explain intent
Medium	Quality	Changes are focused (single concern)	Pass	Redaction + ReDoS bound; PERCY_CHROMIUM_BASE_URL leg reverted out (794d0e0)
Low	Quality	Meaningful names, no dead code	Pass	—
Low	Quality	Comments explain why, not what	Pass	Redaction contract + ReDoS rationale documented inline
Low	Quality	No unnecessary dependencies added	Pass	No dependency changes

Findings

No Critical or High findings.

Note (design characteristic, non-blocking): redaction is egress-scoped — it scrubs the copy sent to the Percy API. The local per-pid temp log file and terminal/CI console output are intentionally not rewritten. This matches the fix's stated scope (CWE-532 network egress); worth a one-line note in the ticket so the boundary is explicit.

---

Verdict: PASS

[pranavz28]

LGTM