fix: return no origin instead of throwing for disallowed CORS origins - #134
Conversation
A request from an origin outside the whitelist gets HTTP 500, not a CORS
denial. @tinyhttp/cors passes the origin() return value straight to
res.setHeader and never wraps the call, so the thrown Error escapes the
middleware and becomes a 500 for the whole request, including the OPTIONS
preflight.
That is wrong in three ways. It reports a client-side policy decision as a
server fault, so it pages as an outage and reads like one in logs. It hides
the real cause: the browser shows a generic 500 rather than a CORS message,
and curl against the endpoint looks fine because a plain request sends no
Origin at all. And enforcement does not belong on the server here anyway;
omitting the header and letting the browser block the read is what the
no-origin branch a few lines above already does.
Returning "" for a disallowed origin makes the two paths consistent. It is
also the only workable value: origin() feeds res.setHeader directly, so
returning undefined throws ERR_HTTP_INVALID_HEADER_VALUE and returning false
is not a valid header value. Credentials are enabled, so a wildcard is not an
option and none is proposed. Nothing about which origins are allowed changes.
Verified against @tinyhttp/cors 2.0.0, the pinned version, with a local
harness running the current and proposed origin() side by side:
current no Origin GET -> 200 acao=""
allowed Origin GET -> 200 acao="https://meshtastic.org"
disallowed Origin GET -> 500 acao=undefined
disallowed Origin OPTIONS -> 500 acao=undefined
proposed no Origin GET -> 200 acao=""
allowed Origin GET -> 200 acao="https://meshtastic.org"
disallowed Origin GET -> 200 acao=""
disallowed Origin OPTIONS -> 204 acao=""
The 500s match production today: api.meshtastic.org returns 500 with body
"Origin not allowed by CORS" for https://client.meshtastic.org, and 200 with a
reflected header for https://meshtastic.org and http://localhost:3000. A
plain request with no Origin already receives an empty
access-control-allow-origin in production, so the proposed value is behaviour
this service is known to serve correctly.
biome ci over src/ and scripts/ is unchanged at 0 errors and the same 2
pre-existing warnings, in files this commit does not touch.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe CORS origin callback now returns an empty string for non-whitelisted origins instead of throwing an error. ChangesCORS origin handling
Estimated code review effort: 2 (Simple) | ~5 minutes Merge Risk: 🔵 Low · up to This change prevents disallowed origins from producing erroneous 500 responses while keeping the existing allowlist unchanged. The PR is mergeable with owner awareness that state-changing, cookie-authenticated routes should independently enforce CSRF or equivalent request authorization because CORS denial alone does not prevent server-side effects. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Closing the loop on the one caveat above: CI confirmed the part I could not run locally. The |
2 participants
Why
A request from an origin outside the whitelist gets HTTP 500, not a CORS denial.
@tinyhttp/corspasses theorigin()return value straight intores.setHeaderand never wraps the call, so thethrow new Error("Origin not allowed by CORS")atsrc/index.ts:56escapes the middleware and becomes a 500 for the whole request, preflight included.That is wrong in three ways:
curlagainst the same endpoint looks perfectly healthy because a plain request sends noOriginat all. This cost me a while to pin down.Returning
""for a disallowed origin makes the two paths consistent.credentials: trueis set, so a wildcard is not an option and none is proposed.On why
""and not something more explicit, sinceorigin()feedsres.setHeaderdirectly (measured, same harness as below):origin()returns""access-control-allow-origin: ""falseaccess-control-allow-origin: falsenullaccess-control-allow-origin: nullundefinedERR_HTTP_INVALID_HEADER_VALUEfalseandnulldo deny correctly, they just put a junk token on the wire that reads like a bug to the next person.undefinedswaps one 500 for another.""is the value this service already emits for no-Origin requests, so it is the one with production evidence behind it.Nothing about which origins are allowed changes in this PR.
Testing Performed
Verified against
@tinyhttp/cors2.0.0 (the pinned version) with a local harness running the current and proposedorigin()side by side:Originacao: ""acao: ""acao: https://meshtastic.orgacao: https://meshtastic.orgacao: ""acao: ""The 500s match production today:
A plain request with no
Originalready receives an emptyaccess-control-allow-originin production, so the proposed value is behaviour this service is known to serve correctly.biome cioversrc/andscripts/is unchanged: 0 errors, and the same 2 pre-existing warnings insrc/lib/mqtt.tsandsrc/services/gateway.ts, neither touched here. I could not runpnpm installlocally (4@buf/*lockfile entries have nointegrityfield, which pnpm 11 rejects; CI's pnpm 9 does not enforce this), sopnpm buildis unverified locally. The change is one return statement inside an existingstring-returning function, so I would expecttscto be indifferent, but worth a CI confirmation.Two related findings, not fixed here
Both turned up while tracking down the above. Neither is actionable in this repo, flagging them rather than guessing at a fix.
client.meshtastic.orgis not on its own API's allowlist. It is live and serving, but gets the 500 above.map.meshtastic.organdflasher.meshtastic.orgare on the list. Adding an origin is your call, not a bug fix, so I deliberately left the whitelist untouched. Happy to add it in a follow-up if you want it.apiv2 serves
iconUrlvalues pointing atapi.meshtastic.org.src/routes/eventFirmware.tsre-origins hosted icon URLs offX-Forwarded-Host, which is exactly right, but apiv2 responds withserver: cloudflareand nox-powered-by: tinyhttpand ignores cache-busting query strings, so it is serving R2 objects without running this route. The result is that on apiv2 the event icon URLs still point at v1, where a browser request for them hits the 500 this PR fixes. That lives in the apiv2 Worker/R2 sync rather than here, so there is nothing to change in this repo, but you will want it on the list.Context
Found while pointing the Meshtastic Android/desktop/web app at
apiv2.meshtastic.org. The wasmJs web target cannot use v1 at all because of this 500; apiv2 servesaccess-control-allow-origin: *and a 204 preflight. Payload parity between the two hosts checked out across all six endpoints the app consumes plus the maintenance UF2 binaries (byte-identical, digests match)./cc @thebentern