feat: require confirmation token for managePercyBuildApproval

[SavioBS629]

No description provided.

[SavioBS629]

Claude Code PR Review

PR: #336 • Head: d43c13d • Reviewers: stack:code-review

Summary

Adds an HMAC-based confirmation-token gate to managePercyBuildApproval: the first call (no/invalid confirmToken) returns a freshly minted token and does not hit Percy; a second call with a matching token performs the destructive approve/reject. Also stops leaking the upstream Percy error body.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	HMAC keyed on auth string, output is a one-way truncated digest — no creds emitted
High	Security	Authentication/authorization checks present	Pass	Adds a two-step confirmation gate before the irreversible action
High	Security	Input validation and sanitization	Pass	Zod schema validates buildId/action/confirmToken; token compared against a derived set
High	Security	No IDOR — resource ownership validated	N/A	buildId ownership enforced by Percy Basic-auth, unchanged
High	Security	No SQL injection (parameterized queries)	N/A	No DB access
High	Correctness	Logic is correct, handles edge cases	Pass	Token bound to buildId+action+minute; tests cover wrong-build/wrong-action/invalid/valid
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Util throws on API failure; wrapped by handler try/catch → handleMCPError
High	Correctness	No race conditions or concurrency issues	Pass	Token derivation is fully request-scoped; no shared mutable state
Medium	Testing	New code has corresponding tests	Pass	New percyApproveReject.test.ts, 6 cases
Medium	Testing	Error paths and edge cases tested	Pass	Invalid/mismatched token + API-error-without-leak all covered
Medium	Testing	Existing tests still pass (no regressions)	Pass	Behavior change is additive to the tool's contract
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Bounded 6-iteration token loop
Medium	Performance	Long-running tasks use background jobs	N/A	—
Medium	Quality	Follows existing codebase patterns	Pass	Multi-tenant-safe (no globals); auth via getBrowserStackAuth(config)
Medium	Quality	Changes are focused (single concern)	Pass	Confirmation gate + error-leak fix + tests
Low	Quality	Meaningful names, no dead code	Pass	—
Low	Quality	Comments explain why, not what	Pass	Bucket-window comment is helpful
Low	Quality	No unnecessary dependencies added	Pass	node:crypto only

Findings

No blocking issues. Three low-severity / non-gating observations:

• src/tools/review-agent-utils/percy-approve-reject.ts:39-49 — Severity: Low (design). On a missing/invalid token the tool returns a valid token in plaintext, so an autonomous agent can mint-then-use it across two calls with no human in the loop. This is the intended "re-call to confirm" speed-bump (guards against a single accidental destructive call), not an authorization control — worth stating explicitly so it isn't mistaken for the latter. No change required.
• src/tools/review-agent-utils/percy-approve-reject.ts:34 — Severity: Low (cosmetic). The validation loop runs i <= CONFIRM_TOKEN_TTL_MINUTES (6 buckets), giving up to ~6 minutes of tolerance while the user-facing message says "5 minutes". Harmless; tighten to i < CONFIRM_TOKEN_TTL_MINUTES if exact 5-min parity matters. A token is also reusable multiple times within the window (no single-use tracking) — acceptable for a confirmation gate on same-user, near-idempotent actions.
• src/tools/review-agent-utils/percy-approve-reject.ts:65 — Severity: Low (pre-existing). The util calls Percy via global fetch rather than the repo's axios/apiClient convention. Not introduced by this PR (the fetch call predates it) and Percy is an external, non-BrowserStack API, so it's out of scope here — noting only for a possible follow-up. The removal of the upstream error body from the thrown message is a genuine improvement.

---

Verdict: PASS — well-scoped security hardening with strong test coverage; credential-safe token derivation and a good error-leak fix. Only low-severity, non-blocking observations.