security: scope CORS to loopback + block cross-origin config/stop (PER-8600/8601/8602/8603)

[Shivanshu-07]

Summary

Fifth (final contained) percy-cli security PR — the local-server origin findings (deadline 2026-06-16) + chain PER-8626.

Ticket	CWE	Finding
PER-8602	CWE-942	CORS wildcard enables cross-origin state mutation
PER-8600	CWE-306	Missing auth on the process-terminating endpoint
PER-8601	CWE-306	Unauthenticated live config mutation via POST /percy/config
PER-8603	CWE-915	Live config mass assignment

The constraint

The local API is unauthenticated by design — every SDK posts to it without credentials. The real exploit path in these findings is a malicious website the user is visiting reaching http://localhost:5338 from the browser. The fix defends that vector without breaking the SDK contract.

Changes

server.js (PER-8602): the CORS middleware no longer returns Access-Control-Allow-Origin: *. It reflects the request Origin only when it's loopback (isLoopbackOrigin — localhost/127.0.0.1/::1/*.localhost, any port). Arbitrary sites get no ACAO and can't read local-server responses (build data, config). Node SDK clients don't use CORS; loopback browser tooling (e.g. the Storybook manager on localhost:6006) still works.

api.js (PER-8600 / PER-8601): assertNotCrossOrigin() rejects (403) any POST /percy/config or /percy/stop request carrying a non-loopback Origin header. Node SDK clients send no Origin → unaffected; a malicious site's cross-origin fetch → blocked.

PER-8603: already mitigated by the existing stripBlockedConfigFields (httpReadOnly) control; the cross-origin guard adds defense against browser-driven mutation.

Why not Host-validation

The server binds to all interfaces by default (PERCY_SERVER_HOST, default ::), so Dockerized SDKs legitimately reach it via a non-loopback Host. Origin-based checks defend the browser vector without breaking those setups.

Verification

• isLoopbackOrigin verified (localhost/127.0.0.1/*.localhost allowed; evil.com/empty/undefined rejected); both files node --check clean.
• Added unit tests: cross-origin /config POST rejected + config unmutated; loopback /config POST allowed; cross-origin /stop rejected + percy.stop not called.
• Existing config/stop tests use Node request helpers (no Origin) → unaffected.

Closes PER-8602; closes the browser-origin vector for PER-8600/8601/8603 and chain PER-8626.

Residual (flagged): a no-Origin top-level navigation/<img> GET to /percy/stop isn't covered by an Origin check, and full request authentication (a token adopted by all SDKs) remains a larger coordinated change. This PR closes the cross-origin (CORS/fetch) vector the findings exploit; full auth tracked as follow-up.

🤖 Generated with Claude Code

[Shivanshu-07]

Claude Code PR Review

PR: #2282 • Head: 8d04218 • Reviewers: stack:code-reviewer

Summary

Scopes the local Percy CLI API server's CORS to loopback origins and blocks cross-origin state changes (PER-8600/8601/8602/8603, CWE-942/CWE-352). isLoopbackOrigin() gates Access-Control-Allow-Origin so the server only echoes a loopback origin (never *) and omits CORS headers for missing/non-loopback origins; assertNotCrossOrigin guards /percy/config POST and /percy/stop. Node SDK clients (no Origin header) are unaffected.

Review Table

Priority	Category	Check	Status	Notes
High	Security	Cross-origin reads blocked (CORS)	Pass	Access-Control-Allow-Origin only echoed for loopback; verified: loopback→echo, no-origin/evil→absent
High	Security	Cross-origin state changes blocked	Pass	assertNotCrossOrigin on /percy/config POST + /percy/stop
High	Correctness	No test regression	Fixed in this cycle	The existing preflight spec still asserted '*' and would break CI — updated (commit 8d04218)
High	Correctness	Legitimate SDK traffic preserved	Pass	No-Origin requests still succeed (204), unaffected by the guard
Medium	Testing	New behavior tested	Pass	api.test.js cross-origin tests + rewritten server.test.js preflight + negative case
Low	Quality	No dead code	Fixed in this cycle	Removed unreachable host === '[::1]' branch (URL.hostname strips brackets)

Findings (all resolved or non-blocking)

• File: packages/core/test/unit/server.test.js:244 — Severity: High — RESOLVED

• Issue: handles CORS preflight requests asserted access-control-allow-origin: '*', which the patch no longer emits → CI failure. The PR updated api.test.js but not this spec.

• Resolution: Rewrote the spec to send a loopback Origin and assert the echoed value + methods/headers, and added a negative spec asserting CORS headers are absent for missing and cross-origin requests. Verified against the real server (commit 8d04218).

• File: packages/core/src/server.js (isLoopbackOrigin) — Severity: Low — RESOLVED

• Issue: host === '[::1]' is dead — new URL('http://[::1]').hostname returns ::1 (brackets stripped).

• Resolution: Removed the bracketed branch, kept ::1, with a clarifying comment.

Non-blocking follow-ups (Low/pre-existing, out of scope): consider a Host-header allowlist to fully close DNS-rebinding (server binds 0.0.0.0/::); set Vary: Origin unconditionally; call assertNotCrossOrigin for all /percy/config POSTs rather than only when a body is present.

---

Verdict: PASS

[Shivanshu-07]

Resolved the Semgrep cors-misconfiguration finding on packages/core/src/server.js. This is a false positive: the reflected Origin is only echoed into Access-Control-Allow-Origin inside the if (originAllowed) branch, and originAllowed = isLoopbackOrigin(origin) validates it against a loopback-only allowlist (localhost / 127.0.0.1 / ::1 / *.localhost) first — which is the entire point of this hardening change. Added a targeted // nosemgrep suppression with an inline justification rather than weakening the check. Semgrep CI is now green.

[Shivanshu-07]

Claude Code PR Review

PR: #2282 • Head: b2668d7 • Reviewers: fallback inline checklist

Summary

Scopes the local Percy server's CORS to loopback origins (echoes a validated loopback Origin instead of *) and adds an assertNotCrossOrigin gate to the state-changing /percy/config (POST with body) and /percy/stop endpoints, to block a malicious website from driving the unauthenticated localhost server via the browser (CWE-942 / CWE-352 / CWE-306, PER-8600/8601/8602/8603).

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	None introduced.
High	Security	Authentication/authorization checks present	Fail	Origin gate on /percy/stop is bypassable via a cross-origin GET (<img>/navigation) which carries no Origin header — see Finding 1.
High	Security	Input validation and sanitization	Pass	isLoopbackOrigin parses via URL, unwraps [::1], fails closed on malformed/null/non-string origins. IPv6/userinfo/port tricks resolve to hostname and fail closed.
High	Security	No IDOR — resource ownership validated	N/A	No per-resource ownership model on the local server.
High	Security	No SQL injection (parameterized queries)	N/A	No SQL.
High	Correctness	Logic is correct, handles edge cases	Fail	/percy/stop route registered with no method matches ALL verbs; the GET path defeats the gate (Finding 1).
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	ServerError(403,...) thrown and rendered by #handleRequest; try/catch in isLoopbackOrigin deliberately fails closed.
High	Correctness	No race conditions or concurrency issues	Pass	No new shared state.
Medium	Testing	New code has corresponding tests	Pass (partial)	Good coverage for CORS loopback vs blocked, isLoopbackOrigin, [::1], malformed, and POST cross-origin stop/config. Missing: GET-vector stop test (Finding 1).
Medium	Testing	Error paths and edge cases tested	Pass	Malformed/missing origin and non-loopback covered.
Medium	Testing	Existing tests still pass (no regressions)	Pass	Preflight spec updated to send Origin; no behavioral regression for legitimate loopback tooling.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	—
Medium	Performance	Long-running tasks use background jobs	N/A	—
Medium	Quality	Follows existing codebase patterns	Pass	Reuses ServerError, route/middleware style, exported helper for reuse in api.js.
Medium	Quality	Changes are focused (single concern)	Pass	Scoped to CORS + origin gate.
Low	Quality	Meaningful names, no dead code	Pass	isLoopbackOrigin/assertNotCrossOrigin clear; dead [::1] branch removed.
Low	Quality	Comments explain why, not what	Pass	Comments cite the threat/CWE rationale; no ticket ids embedded in code.
Low	Quality	No unnecessary dependencies added	Pass	Uses built-in URL.

Findings

• File: packages/core/src/api.js:1000 (route) — interacts with packages/core/src/server.js (!methods || methods.includes(req.method))

• Severity: High

• Reviewer: fallback inline checklist

• Issue: /percy/stop is registered as .route('/percy/stop', handler) with no HTTP method. In #handleRequest, let result = !methods || methods.includes(req.method) means a method-less route matches every verb, including GET. The new assertNotCrossOrigin(req) only rejects when an Origin header is present and non-loopback. Browsers do not send an Origin header on cross-origin no-cors GET subresource loads or top-level navigations. So a malicious page the user is visiting can stop the running build with a simple <img src="http://localhost:5338/percy/stop"> (default port 5338 is guessable) — no Origin header is sent, the gate is a no-op, and percy.stop() fires. This defeats the exact CSRF protection PER-8600 exists to add. The POST-only tests pass while the real attack vector (GET) remains open and untested.

• Suggestion: Restrict the endpoint to POST: .route('post', '/percy/stop', ...). Every legitimate caller already uses POST (packages/cli-exec/src/stop.js → request(stop, { method: 'POST' }); @percy/sdk-utils request defaults to POST; core/README.md documents "POST /percy/stop"), so this is non-breaking. With the endpoint POST-only, the browser always attaches an Origin header on cross-origin POSTs, so assertNotCrossOrigin reliably blocks the attack. Add a test asserting a GET to /percy/stop does not stop Percy and stop is not called.

• File: packages/core/src/api.js:359 and other POST endpoints (/percy/snapshot, /percy/flush, /percy/events, /percy/comparison, ...)

• Severity: Low (note, non-blocking)

• Reviewer: fallback inline checklist

• Issue: The origin gate is intentionally scoped to /percy/config and /percy/stop. Other state-changing POST endpoints are not gated. In practice they are protected: cross-origin JSON POSTs trigger a preflight that now returns no CORS headers (browser blocks the send), and a preflight-free "simple" POST is not parsed as JSON (IncomingMessage only JSON.parses when content-type includes json), so those handlers receive a raw Buffer and do not perform their intended mutation.

• Suggestion: No change required for this PR's scope; consider a follow-up to apply assertNotCrossOrigin uniformly to all mutating POST routes for defense-in-depth and consistency.

---

Verdict: FAIL

[Shivanshu-07]

Claude Code PR Review

PR: #2282 • Head: 2ffd545 • Reviewers: fallback inline checklist

Re-review after fix commit 2ffd545 addressed the High finding from the previous review.

Summary

Scopes the local Percy server's CORS to loopback origins (echoes a validated loopback Origin instead of *) and adds an assertNotCrossOrigin gate to the state-changing /percy/config (POST with body) and /percy/stop endpoints, blocking a malicious website from driving the unauthenticated localhost server via the browser (CWE-942 / CWE-352 / CWE-306, PER-8600/8601/8602/8603). /percy/stop is now POST-only, closing the no-Origin GET vector.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	None introduced.
High	Security	Authentication/authorization checks present	Pass	/percy/stop is now POST-only; cross-origin browser POSTs always carry an Origin header, so the loopback gate reliably blocks them. GET vector closed.
High	Security	Input validation and sanitization	Pass	isLoopbackOrigin parses via URL, unwraps [::1], fails closed on malformed/null/non-string origins; IPv6/userinfo/port tricks fail closed.
High	Security	No IDOR — resource ownership validated	N/A	No per-resource ownership model on the local server.
High	Security	No SQL injection (parameterized queries)	N/A	No SQL.
High	Correctness	Logic is correct, handles edge cases	Pass	Method-less route removed for /percy/stop; GET now 404s and does not stop Percy (regression test added).
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	ServerError(403,...) thrown and rendered; isLoopbackOrigin try/catch deliberately fails closed.
High	Correctness	No race conditions or concurrency issues	Pass	No new shared state.
Medium	Testing	New code has corresponding tests	Pass	Added GET-vector stop test; existing CORS/loopback/isLoopbackOrigin/[::1]/POST cross-origin tests retained.
Medium	Testing	Error paths and edge cases tested	Pass	Malformed/missing/non-loopback origin and GET vector covered.
Medium	Testing	Existing tests still pass (no regressions)	Pass	unit/server.test.js 39/39 green; api.test.js CORS/config/stop specs green.
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	—
Medium	Performance	Long-running tasks use background jobs	N/A	—
Medium	Quality	Follows existing codebase patterns	Pass	POST route matches how all callers invoke stop and the README contract.
Medium	Quality	Changes are focused (single concern)	Pass	Scoped to CORS + origin gate + method hardening.
Low	Quality	Meaningful names, no dead code	Pass	Clear helpers; dead [::1] branch removed earlier.
Low	Quality	Comments explain why, not what	Pass	Rationale/CWE comments; no ticket ids in source comments.
Low	Quality	No unnecessary dependencies added	Pass	Uses built-in URL.

Findings

• File: packages/core/src/api.js:1000 — RESOLVED

• Severity: High (fixed in 2ffd545)

• Reviewer: fallback inline checklist

• Issue (resolved): /percy/stop was registered with no HTTP method, so it matched GET; a cross-origin no-Origin GET (<img src="http://localhost:5338/percy/stop">) bypassed assertNotCrossOrigin and stopped the build.

• Resolution: Route restricted to POST (.route('post', '/percy/stop', ...)). All legitimate callers already POST (cli-exec/src/stop.js, @percy/sdk-utils default, README). A regression test asserts a GET to /percy/stop does not call percy.stop().

• File: packages/core/src/api.js:359 and other POST endpoints — Low (note, non-blocking, unchanged)

• Severity: Low

• Reviewer: fallback inline checklist

• Issue: Origin gate is intentionally scoped to /percy/config and /percy/stop. Other mutating POST endpoints are protected in practice (JSON preflight now blocked; non-JSON simple POSTs are not parsed as JSON so handlers no-op). Optional follow-up: apply assertNotCrossOrigin uniformly for defense-in-depth.

---

Verdict: PASS