fix(gateway): fail closed when CORS header generation throws (#3705)#3778
Merged
Conversation
The gateway previously caught any exception from getCorsHeaders() and fell back to `Access-Control-Allow-Origin: *`, converting the allowlist into permissive wildcard CORS on the error path. A regression in origin handling could therefore silently broaden cross-origin readability for premium / user / intelligence-adjacent endpoints. Now the catch path is fail-closed: log via captureSilentError + return a 500 with no Access-Control-Allow-Origin header so the browser blocks any cross-origin read. Adds `cors_error` to RequestReason for telemetry distinct from auth/origin rejections. Regression test asserts (a) gateway source contains no wildcard ACAO literal and (b) the cors catch block passes the original error to captureSilentError with a `step: 'cors_headers'` tag.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Greptile SummaryThis PR fixes a security regression (#3705) where any exception thrown by
Confidence Score: 4/5The core security fix is correct and safe to merge; the only gap is in the new test file. The gateway change is well-scoped and correct. The new tests guard against the specific old regression but do not exercise the gateway handler at runtime, leaving the 500 response contract untested. tests/cors-fail-closed.test.mts — relies on source-level regex assertions rather than runtime handler invocation Important Files Changed
Sequence DiagramsequenceDiagram
participant Browser
participant Gateway
participant getCorsHeaders
participant Sentry
Browser->>Gateway: HTTP Request (with Origin header)
Gateway->>Gateway: isDisallowedOrigin(request)?
alt Origin disallowed
Gateway-->>Browser: 403 Origin not allowed
else Origin allowed
Gateway->>getCorsHeaders: getCorsHeaders(request)
alt throws exception (new fail-closed path)
getCorsHeaders-->>Gateway: throws Error
Gateway->>Sentry: "captureSilentError(err, { step: cors_headers })"
Gateway->>Gateway: emitRequest(500, cors_error, null)
Gateway-->>Browser: 500 Internal Server Error (no ACAO header)
else returns headers
getCorsHeaders-->>Gateway: corsHeaders
alt OPTIONS preflight
Gateway-->>Browser: 204 + corsHeaders
else Normal request
Gateway->>Gateway: auth to rate-limit to route to handler
Gateway-->>Browser: Response + corsHeaders
end
end
end
Reviews (1): Last reviewed commit: "fix(gateway): fail closed when CORS head..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+31
to
+62
| describe('gateway CORS error path (issue #3705)', () => { | ||
| it('does not contain a wildcard ACAO fallback in source', async () => { | ||
| const source = await readFile( | ||
| new URL('../server/gateway.ts', import.meta.url), | ||
| 'utf8', | ||
| ); | ||
| // No literal that pairs Access-Control-Allow-Origin with `*` should | ||
| // appear in gateway.ts. The pre-#3705 fallback was: | ||
| // corsHeaders = { 'Access-Control-Allow-Origin': '*' }; | ||
| const widening = /Access-Control-Allow-Origin['"]?\s*:\s*['"]\*['"]/i; | ||
| assert.ok( | ||
| !widening.test(source), | ||
| 'gateway.ts must not emit wildcard ACAO — see issue #3705', | ||
| ); | ||
| }); | ||
|
|
||
| it('routes CORS exceptions through captureSilentError + 500 (no wildcard)', async () => { | ||
| const source = await readFile( | ||
| new URL('../server/gateway.ts', import.meta.url), | ||
| 'utf8', | ||
| ); | ||
| // The fail-closed branch must log the original error to Sentry AND | ||
| // return a 5xx instead of a permissive CORS response. | ||
| assert.ok( | ||
| /catch \(err\)[\s\S]{0,200}captureSilentError\(err/.test(source), | ||
| 'gateway.ts cors catch must pass the original error to captureSilentError', | ||
| ); | ||
| assert.ok( | ||
| /step:\s*['"]cors_headers['"]/.test(source), | ||
| 'gateway.ts cors catch must tag Sentry events with step="cors_headers"', | ||
| ); | ||
| }); |
Contributor
There was a problem hiding this comment.
The two gateway CORS error path tests read gateway.ts as a string and match patterns with regexes. They confirm the old wildcard literal is absent and that captureSilentError(err appears near the catch, but they never actually run the gateway handler with a throwing getCorsHeaders. That means a regression where, say, the catch block logs the error but returns a 200 with no CORS header (rather than a 500) would pass both tests. To truly guard the contract, a test should instantiate the gateway (or a minimal stub) with a mock getCorsHeaders that throws, invoke the handler, and assert response.status === 500 and response.headers.has('Access-Control-Allow-Origin') === false.
Review-pass polish on the fail-closed CORS catch: - Cache-Control: no-store on the 500. Without this header, a CDN/edge could pin a transient gateway error for downstream callers across the cache TTL. - Pass the captureSilentError promise through ctx.waitUntil so the Vercel Edge isolate survives long enough to flush the Sentry event. The helper uses keepalive:true as a transport fallback, but an explicit waitUntil is the documented best practice. - Strip JS comments before applying the WILDCARD_ACAO_LITERAL guard in the regression test so a future PR description quoted in JSDoc cannot false-positive. Extract the regex into a named constant for self-documenting failure messages. - Add a third regression test that asserts Cache-Control: no-store appears inside the cors catch block. 12/12 tests pass; typecheck clean.
koala73
added a commit
that referenced
this pull request
May 19, 2026
…ocal-token default (#3804) (#3829) * fix(docker): require explicit REDIS_PASSWORD + REDIS_TOKEN, drop wm-local-token default (#3804) Closes #3804. Self-hosted Docker stack shipped two interlocking security defaults that made it one override away from "publicly authenticated by a known string": 1. Redis ran without --requirepass — any container added to the worldmonitor docker network could read/write all cache entries (session tokens, rate-limit counters, AIS snapshots) without credentials. The redis-rest proxy held the only credential gate. 2. SRH_TOKEN and UPSTASH_REDIS_REST_TOKEN both defaulted to the publicly documented literal "wm-local-token" via ${REDIS_TOKEN:-wm-local-token}. SELF_HOSTING.md and scripts/run-seeders.sh both reinforced the default — a fresh clone that flipped the redis-rest binding from 127.0.0.1 to 0.0.0.0 instantly exposed an authenticated REST interface with a token anyone reading the docs already knew. Fix (one focused change): - docker-compose.yml: switch both vars to fail-closed parameter expansion so the affected container refuses to start with a helpful error pointing at openssl rand -hex 32. Add --requirepass to redis using REDIS_PASSWORD; plumb the password into SRH_CONNECTION_STRING so the REST proxy still reaches Redis. - .env.example: new "Self-hosted Redis (Docker Compose only)" section documenting both vars + how to generate, with a note distinguishing them from the Vercel-Upstash hosted block above. - SELF_HOSTING.md: quick-start now generates all three secrets in step 2; required-env table covers all three with a migration note for existing clones; manual-seeder snippet uses fail-closed expansion instead of the literal. - scripts/run-seeders.sh: auto-source REDIS_TOKEN from .env, fail-loud if neither REDIS_TOKEN nor UPSTASH_REDIS_REST_TOKEN is set instead of silently using "wm-local-token". Regression test (tests/docker-compose-no-default-secrets.test.mts): literal-absence + structural-presence source-greps over all four files, following the static-grep recipe from PR #3778 (cors fail-closed). The shipped-default regex set is paired with a meta-test exercising 7 representative inputs (4 historical bad shapes + 3 acceptable shapes including prose mentions of the literal in a deprecation note) so a future "simplification" of the pattern can't silently drop coverage. Reverse-check: temporarily reintroducing the bad default in any of the three files fails the matching test with the #3804 reference. Test plan - 5/5 new test cases pass; reverse-test confirms each fires when the bad default is reintroduced. Out of scope: doesn't touch the Vercel Upstash credential path (UPSTASH_REDIS_REST_URL / UPSTASH_REDIS_REST_TOKEN sourced from Upstash itself, unaffected by this change). * fix(docker): address Greptile review of #3829 (P1 token precedence + P2 fail-closed expansion) Two findings from Greptile review of PR #3829. P1 — scripts/run-seeders.sh precedence inverted. My guard read "copy REDIS_TOKEN only if UPSTASH_REDIS_REST_TOKEN is absent", but the set -a / source .env / set +a block five lines above already populates UPSTASH_REDIS_REST_TOKEN from .env if it is there. A developer who also works on the Vercel/Upstash side and keeps the production token in the same .env therefore ended up sending the Vercel-Upstash bearer to localhost:8079 and getting a 401 from the local proxy with no hint why. This script is Docker-only, so REDIS_TOKEN must win unconditionally if set. Fix: replace the AND-conditional with a simple "if REDIS_TOKEN set, use it" plus a comment pointing at the .env-coexistence case. P2 — docker-compose.yml redis-rest SRH_CONNECTION_STRING used a bare REDIS_PASSWORD expansion instead of the fail-closed form. Today the redis service's fail-closed expansion validates REDIS_PASSWORD before any container starts, so the bare form is functionally safe — but a future PR that removes --requirepass or copies the connection string into a new service would silently expand to an empty password. Cheap defense-in-depth: use the same fail-closed form everywhere REDIS_PASSWORD or REDIS_TOKEN is expanded. Regression test extended: - Added "no bare REDIS_TOKEN/REDIS_PASSWORD expansion anywhere in docker-compose.yml" assertion using a negative-lookahead regex, so any future bare form anywhere in the file trips the test. - Added scripts/run-seeders.sh assertions that codify the inverted precedence: "must NOT gate REDIS_TOKEN copy on UPSTASH_REDIS_REST_TOKEN being absent" + "must unconditionally prefer REDIS_TOKEN when set" — both reference PR #3829 in their messages. Reverse-checks performed: - Reintroducing the bare REDIS_PASSWORD in SRH_CONNECTION_STRING fails the docker-compose assertion with the bare-count diff. - Reintroducing the original AND-conditional precedence guard fails the seeder assertion with the #3829 P1 reference. Restoring both turns the suite green again (5/5). * docs(self-hosting): source .env in manual seeder snippet so REDIS_TOKEN resolves (#3829 follow-up) Reviewer doc-nit on PR #3829: the manual seeder snippet uses ${REDIS_TOKEN:?...} but quick-start puts REDIS_TOKEN in .env, not in the user's shell. A user copy-pasting the snippet verbatim hits "REDIS_TOKEN: parameter null or not set" before the first seeder runs. Add the same `set -a; . ./.env; set +a` pattern the wrapper script uses, with a one-line comment naming the failure mode so a future reader doesn't strip the line as redundant. Doc-only; regression test suite (5/5) unaffected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The gateway previously caught any exception from
getCorsHeaders()and fell back toAccess-Control-Allow-Origin: *, converting the allowlist into permissive wildcard CORS on the error path. Per issue #3705, a regression in origin handling could therefore silently broaden cross-origin readability for premium / user / intelligence-adjacent endpoints.Change
Test plan
The new test file asserts (a) the gateway source contains no wildcard ACAO literal anywhere, and (b) the cors catch block routes the original error into `captureSilentError` with a `step: 'cors_headers'` tag. Together this guards against future regressions to either property.
Closes #3705