fix(core): allowlist the /percy/log level so request input never drives dynamic dispatch (PER-8625)

[ninadbstack]

Summary

• Security fix for PER-8625 (chain-breaker finding PER-8606, CWE-1321). POST /percy/log used the request-supplied level as a property key on the logger group object: log[level](message, meta).
• On the unauthenticated local API server this let any caller invoke arbitrary logger methods. Confirmed side effects before the fix: {"level":"loglevel","message":"debug"} returned 200 and flipped the process-wide log level; {"level":"deprecated",...} emitted arbitrary deprecation warnings. __proto__/constructor produced a 500 rather than pollution, but the primitive is the same one the ticket asks us to close.
• level is now validated against debug|info|warn|error and rejected with 400 { error: 'Invalid log level' } otherwise. Dispatch is an explicit switch, so no request input reaches a dynamic property lookup.
• SDK behaviour is unchanged: @percy/sdk-utils only ever sends the four valid levels.

Test plan

• New spec: every invalid level (__proto__, constructor, loglevel, deprecated, stdout, '', null, 42) gets a 400, the global loglevel is untouched, nothing is logged, Object.prototype is clean. Verified this spec fails against the unpatched handler.
• New spec: all four valid levels dispatch and are recorded with the right level (keeps the switch at 100% coverage).
• Existing /log spec and the full API Server describe block pass locally (39 specs); eslint clean.
• CI green (full suite + coverage threshold)

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Central YAML (base), Workspace UI (inherited)

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: d81f63af-569b-41fa-a6ec-9b63e506e172

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

_{Comment @coderabbitai help to get the list of available commands.}

[ninadbstack]

Claude Code Review (automated) — 2 inline finding(s). Full report in the PR comment below. Verdict: Passed.

[ninadbstack]

[Low] Level allowlist duplicates the logger's own level list

This set repeats the levels already defined as LOG_LEVELS in packages/logger/src/logger.js, which is not exported. If a level is ever added or renamed there without updating this copy, the two drift silently and this endpoint starts rejecting a valid level.

Suggestion: Export the level list from @percy/logger and import it here rather than keeping a second hardcoded copy. Fine as a follow-up.

Reviewer: stack-code-reviewer

[ninadbstack]

[Low] Switch is redundant once the guard has run

After SDK_LOG_LEVELS.has(level) passes, level is provably one of four safe literals, so log[level](message, meta) would be equally safe and shorter. The comment above reads as though the guard alone is the mitigation, which leaves this switch looking redundant to a later reader.

Suggestion: Keep the switch if it is deliberate defense in depth against any dynamic property access, and say so in the comment. Otherwise collapse it back to the guarded dynamic call.

Reviewer: stack-code-reviewer

[ninadbstack]

Claude Code PR Review

PR: #2426 • Head: fec8de0 • Reviewers: stack-code-reviewer

Summary

Allowlists the four real log levels on the unauthenticated POST /percy/log route and replaces the dynamic log[level](...) dispatch with an explicit switch, so a request-supplied level can no longer invoke arbitrary logger or Object.prototype members (CWE-1321); adds regression tests for rejected levels and for all four allowed levels.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	No secrets or credentials in the diff.
High	Security	Authentication/authorization checks present	Pass	Route stays unauthenticated by design (local SDK API); the change tightens what unauthenticated input can reach. Existing cross-origin assertion is untouched.
High	Security	Input validation and sanitization	Pass	level is now validated against a four-value allowlist before use; invalid input returns 400.
High	Security	No IDOR — resource ownership validated	N/A	No resource lookup by identifier in this change.
High	Security	No SQL injection (parameterized queries)	N/A	No database access in this change.
High	Correctness	Logic is correct, handles edge cases	Pass	Set membership rejects non-strings, empty string, null and numbers; the switch covers all four allowlisted values, with error as the default arm.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Invalid level returns an explicit 400 with an error body; nothing is swallowed.
High	Correctness	No race conditions or concurrency issues	Pass	Module-level Set is immutable in practice and read-only per request; no shared mutable state added.
Medium	Testing	New code has corresponding tests	Pass	Both new branches (reject, dispatch) have dedicated tests.
Medium	Testing	Error paths and edge cases tested	Pass	Rejection test covers __proto__, constructor, loglevel, deprecated, stdout, empty string, null and a number, and asserts no side effects.
Medium	Testing	Existing tests still pass (no regressions)	Pass	The pre-existing /percy/log test uses allowlisted levels and is unaffected by the stricter validation.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	No data fetching in this change.
Medium	Performance	Long-running tasks use background jobs	N/A	Handler remains a constant-time dispatch.
Medium	Quality	Follows existing codebase patterns	Pass	Matches the file's existing res.json(<status>, { error }) convention for rejected input.
Medium	Quality	Changes are focused (single concern)	Pass	Two files, one concern: log-level dispatch hardening plus its tests.
Low	Quality	Meaningful names, no dead code	Pass	SDK_LOG_LEVELS is clear; no dead code introduced.
Low	Quality	Comments explain why, not what	Pass	The comment states the threat and cites the tickets. See the finding below on making the defense-in-depth intent explicit.
Low	Quality	No unnecessary dependencies added	Pass	No dependency changes.

Findings

• File: packages/core/src/api.js:101

• Severity: Low

• Reviewer: stack-code-reviewer

• Issue: The level allowlist duplicates the level set already defined as LOG_LEVELS in packages/logger/src/logger.js, which is not exported. If a level is added or renamed in the logger without updating this copy, the two lists drift silently and the endpoint rejects a valid level.

• Suggestion: Export the level list from @percy/logger and import it here instead of maintaining a second hardcoded copy. Reasonable as a follow-up rather than a change in this PR.

• File: packages/core/src/api.js:333

• Severity: Low

• Reviewer: stack-code-reviewer

• Issue: Once the Set guard has run, level is provably one of four safe literals, so the switch is not required for correctness or safety. The comment reads as though the guard alone is the mitigation, which leaves the switch looking redundant to a future reader.

• Suggestion: Keep the switch if it is intended as defense in depth against all dynamic property access, and say so in the comment. Otherwise the guarded log[level](message, meta) is equivalent and shorter.

• File: packages/core/src/api.js:322

• Severity: Low

• Reviewer: stack-code-reviewer

• Issue: Pre-existing and outside this PR's scope. message and meta from the same unauthenticated body are still passed through unvalidated, and the logger calls .toString() on message, so a null message can throw inside the handler.

• Suggestion: File a follow-up to validate the message and meta shapes on this route, since they cross the same trust boundary.

Note: the review-threads lookup returned no data this run, so thread-resolution state was unavailable. Reconciliation used REST comment data only.

The other human reviewer approved this PR without inline comments, so there were no human concerns to confirm or carry forward.

---

Verdict: PASS — the security fix is correct and well covered by tests; all three findings are Low and non-gating.