Serve no CORS headers from the brain - #476
Merged
Merged
Conversation
withCORS wrapped the whole handler chain on both profiles: it reflected whatever Origin arrived, set Access-Control-Allow-Credentials: true, and answered every OPTIONS with 204. On hosted that is a live hole. Apps are <slug>.<box-id>.malmo.network and the dashboard is <box-id>.malmo.network, and malmo.network is not on the Public Suffix List, so the two are same-site. The owner's SameSite=Lax session cookie therefore rides a fetch from any app to the dashboard host, and the reflected header let the app read the reply -- including the reply to POST /auth/elevate/challenge. Two places already asserted the opposite: AUTH.md # Re-authentication says a cross-origin page cannot make an authenticated JSON POST here, and confirm.go says the JSON content type forces a preflight the brain does not answer. The hosted confirm step's cross-site argument rested on that refusal, which was in fact a 204. Removed rather than narrowed to an allowlist, because nothing calls this API cross-origin in any lane. The dashboard fetches relative paths, Caddy serves it and the brain on one host in production, and the Vite dev server proxies /api to the brain -- which had already made the middleware's own stated reason for existing untrue. Closes #475
|
9 tasks
Review pointed out that both the spec sentence and the confirm.go comment described the wrong mechanism, and that the new AUTH.md guarantee repeated it. elevateChallenge takes no body and requires no JSON content type, so the POST is a simple request in CORS terms. A same-site app page can send it with the owner's cookie and a challenge is minted. The protection was never that the request cannot be sent -- it is that the reply cannot be read, and a challenge nobody reads is inert: single-use, bound to its user, and expiring unspent. The preflight still matters, for the requests that do trigger one: every JSON PUT and PATCH on this API. The test comment now says which half covers which.
This was referenced Sep 11, 2026
Merged
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
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.
What & why
internal/api.withCORSwrapped the whole handler chain on both profiles. It reflected whateverOriginarrived, setAccess-Control-Allow-Credentials: true, and answered everyOPTIONSwith 204. It is removed.On hosted that is a live hole. Apps are served at
<slug>.<box-id>.malmo.networkand the dashboard at<box-id>.malmo.network.malmo.networkis not on the Public Suffix List — the same factENVIRONMENT.md# Certs uses to explain the shared Let's Encrypt budget — so those hosts are same-site. The owner'smalmo_sessioncookie beingSameSite=Laxtherefore does not stop a fetch from an app origin to the dashboard host, and the reflected header let the app read the answer. Any app on the box could act as the owner against the dashboard API,POST /api/v1/auth/elevate/challengeincluded.Two places in the tree already asserted this was impossible, which is what makes it a defect rather than a tradeoff:
AUTH.md# Re-authentication: "minting one needs an authenticated JSON POST to the box's own API, which a cross-origin page cannot make."internal/api/confirm.go: "the JSON content type forces a preflight the brain does not answer."The brain answered it. The hosted confirm step's whole cross-site argument rested on a refusal that was returning 204.
Removed rather than narrowed to an allowlist. Nothing calls this API cross-origin in any lane:
web-ui/src/api.tsfetches relative paths, Caddy serves the dashboard and the brain on one host in production, andweb-ui/vite.config.tsproxies/apito the brain withchangeOriginin dev. The middleware's comment said it existed for the Vite dev server; the proxy had already made that untrue. An allowlist keeps a knob that is only ever set wrong, for a caller that does not exist.Found by Greptile on the v0.12.0 release PR (#474), in code already merged to
dev. This is the first of three stacked fixes from that review; #474 stays open until all three land.Spec(s) touched
docs/specs/AUTH.md# Re-authentication for destructive actions. The sentence that assumed a cross-origin page cannot POST here now states it as a guarantee the brain keeps, with the same-site reasoning and the PSL fact named, so a future CORS layer contradicts a spec line rather than slipping in. NoDECISIONS.mdentry: nothing locked flipped, the code was simply not doing what the spec said.What was tested
internal/api/cors_test.go, two tests. A foreignOriginon a real 200 gets none of the fourAccess-Control-*headers back; a preflightOPTIONSon the challenge route is not answered 204.https://photos.box-1.malmo.networkback verbatim inAccess-Control-Allow-OriginwithAccess-Control-Allow-Credentials: true. They are regression tests, not assertions that happen to hold.make checkgreen, exit 0.Known gaps & deviations
OPTIONS204 goes with it. Intended, and no malmo code sent one. Same-origin requests never preflight, somake devis unaffected — but a developer calling the brain directly from a browser page instead of through the Vite proxy would now see a preflight fail. Called out because it is the one behaviour change someone could trip over.SPEC.md# Origins.Platform gaps
None.
Definition of done
make dev), and integration-tested against the real system if it touches one.make checkgreen.docs/progress/cors-origin-reflection.md); progress index updated.AUTH.md); noDECISIONS.mdentry needed.log/slogonly, conventions perCLAUDE.md.dev, PR intodev.Closes #475