Allow Range through realm-server CORS for authenticated media requests - #5798
Conversation
lukemelia
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Review lens: does the CORS surface actually let an authenticated, cross-origin, ranged media request through — the preflight approves the request headers the service worker sends, and the 206 exposes the headers a native player needs — and is every home of that contract updated, not just one.
Bottom line: no blocking issues. The change is correct and the mechanism reasoning in both the PR body and the code comments checks out against the middleware source. Two inline confirmations below document why it's right so the next editor doesn't undo it; both non-blocking.
What lands right (verified against @koa/cors@4.0.0 source, not just asserted):
- The "replace, not merge" claim is real. For an actual (non-OPTIONS) response,
@koa/corssetsAccess-Control-Expose-Headersfrom itsexposeHeadersbeforeawait next()(node_modules/.pnpm/@koa+cors@4.0.0/.../index.js:95-105); the handler then runs, andsetContextResponse(packages/realm-server/middleware/index.ts) copiescreateResponse's headers onto the Koa context withctxt.set(...), overwriting it. So on any realm-handler response,createResponse's list is final — which is exactly the list the media206carries. UpdatingcreateResponseis therefore the load-bearing half; updating the@koa/corsexposeHeaderscovers responses that never go throughcreateResponse. - The preflight list is final. For OPTIONS,
@koa/corssetsAccess-Control-Allow-Headersfrom the configuredallowHeadersand returns204without invoking the handler (index.js:155-163), so nothing downstream can dropRange/If-Range. - Twin check came back clean. The only other CORS header sites —
proxy-forward.ts(SSE /proxyAssetfor the auth-SW script), the host dev-CORS middleware,matrix-backend-authentication.ts— none serve realm file bytes, so none need the range headers. The realm server'scors()+createResponseare the two real homes, and both are updated.
Test coverage: the two tests pin the two winning lists — the preflight's Allow-Headers and the 206's Expose-Headers (which is createResponse's). Worth knowing that the @koa/cors exposeHeaders addition on server.ts is not exercised by either test, because every range response is produced by createResponse and so never surfaces the middleware's list; it's belt-and-suspenders for a hypothetical non-createResponse range response. Fine to keep, just uncovered.
Adjacent, out of scope (correctly deferred): end-to-end browser verification (service worker → preflight → authed 206s → native playback) is left to the FileDef-player follow-up, which is the right place for it — this PR's unit/integration level is the right altitude for the CORS contract itself.
Host Test Results 1 files 1 suites 13m 59s ⏱️ Results for commit dfc0ae5. Realm Server Test Results 1 files ±0 1 suites ±0 13m 17s ⏱️ - 2m 26s Results for commit dfc0ae5. ± Comparison against earlier commit c599254. |
052ed64 to
70cac48
Compare
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Review lens: the change itself is unchanged since the earlier pass — byte-for-byte — so this pass re-verified the CORS contract from scratch against the moved base (which now carries the byte-range file serving this depends on), and went after what the earlier pass didn't: whether the reason recorded in the comments matches the mechanism that actually consumes these headers, and whether the new tests fail when they should.
Bottom line: no blocking issues. The header lists are correct in both homes and the replace-not-merge reasoning in the description reproduces exactly. Four non-blocking comments: two confirmations that document why the code is right (including one asymmetry that looks like a bug and isn't), one comment-wording suggestion, and two test observations.
On the rebase. Comparing each tip against its own parent, the diff is identical to what was reviewed before — same three files, same 108/3. Only the base moved, onto a main where byte-range serving is merged. Re-checked the contract against that base rather than assuming it carried over: every 206 and the 416 in Realm's file-serving path is built with createResponse, so create-response.ts remains the list that reaches the wire on a media range response.
What lands right (verified, not inherited). I re-derived the middleware behavior with a standalone Koa app on @koa/cors@4.0.0 — cors() middleware, then a handler that builds a Response and copies it onto the context with the same per-header ctx.set() loop setContextResponse uses:
- Replace-not-merge is real, and it is per-header.
@koa/corssetsAccess-Control-Expose-Headersatindex.js:95-96and only reachesawait next()atindex.js:105/108, so the middleware's value is always in place before the handler runs.setContextResponsethen iterates only the headers present on the handler'sResponseandctxt.set()s each — so a response carrying its own expose list replaces the middleware's outright, and a response without one keeps it. UpdatingcreateResponsewas mandatory; updating the middleware's list covers the responses that never touch acreateResponsehandler. - The preflight list is final. For OPTIONS,
@koa/corsreturns204withAccess-Control-Allow-Headersechoing the configured list verbatim and never invokes the handler — nothing downstream can dropRange/If-Range. Confirmed in the repro alongsideAccess-Control-Max-Age: 86400. - Twin check came back clean, independently. The only other sites that set CORS headers are
proxy-forward.ts(setupSSEHeaders,text/event-streamonly),matrix-backend-authentication.ts(exposesAuthorizationalone), and the host dev-CORS middleware. None serves realm file bytes. There is no static-file middleware in the realm server that could emit a206outsidecreateResponse. The--cors=…,Range,…list on the icon static server is a separate surface serving built icon assets and needs nothing from this change. - No deploy-time lag from the 24 h preflight cache, which is a fair thing to wonder about given
maxAge: 86400. Per the Fetch spec the CORS-preflight cache is keyed per header name, and a request carrying a name with no cache entry forces a fresh preflight — so a newly allowedRangecan't be answered from a pre-existing entry. Spec-derived, not measured here.
CI. Realm-server tests are green (2,174 passing, 0 failing) including the two new ones. One host shard is red, and it emitted no junit at all — it died before test output rather than on an assertion, so it reads as infrastructure. Nothing in this diff can reach host tests: both changes are strictly additive CORS permissions, an allow list and an expose list, and neither can turn a request that previously succeeded into a failure. Worth a re-run rather than an investigation.
Recommendations, all non-blocking:
- Reword the
Content-Range/Accept-Rangesrationale increate-response.ts— the real consumer is the media element's own loading stack, not app JS, because the service worker returns the CORS-filteredResponseto it viarespondWith. Stronger reason, and the weak version is what invites a future trim. See the thread oncreate-response.ts. - Assert
Access-Control-Allow-Originin the206test — an expose list is inert without it, so today the test pins half its contract. Also assert the upload's204. See the thread on the ranged-response test. - Fold the two CORS tests into
range-request-test.ts, whose setup block is byte-identical and which already hasuploadSample/binaryParserand nosample.png. See the file-level thread oncors-range-test.ts. - Nothing to change for the
Locationasymmetry between the two expose lists — it reads like the silent drop the comments warn about but is safe, and the thread onserver.tsrecords why so it isn't "fixed" and the invariant isn't dismissed.
Adjacent, out of scope. packages/host/public/auth-service-worker.js still documents itself as being for <img> and CSS background-image, while the header comments added here correctly describe it as the path native <audio>/<video> depend on. Whoever next touches that file should widen its opening comment to match. Also agreed on deferring browser-level end-to-end verification of protected-realm playback to the player integration work — this PR's level is the right altitude for the CORS contract itself.
Generated by Claude Code
d718a69 to
c599254
Compare
Native audio/video elements cannot attach Authorization, so on protected realms the host's auth service worker re-issues media requests as mode:'cors' with the token injected. That rewrite turns the media element's Range header into an author header needing preflight approval, and the realm server's Access-Control-Allow-Headers did not include Range — the preflight failed and the native player errored before any bytes flowed. Range and If-Range join the CORS allow list, and Content-Range / Accept-Ranges / Content-Length join both expose lists — the @koa/cors exposeHeaders and the createResponse Access-Control-Expose-Headers, since the latter replaces rather than merges with the former on any response a realm handler produces. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The realm-server test entrypoint loads an explicit file list, and neither http-range-test nor range-request-test was on it, so none of the range or CORS coverage was actually running in CI. Both are registered now. The two CORS assertions move from their own module into range-request-test, which stands up a byte-identical realm fixture — picking up its asserted sample upload on the way. The exposure test now also pins Access-Control-Allow-Origin, without which no expose list is consulted, and the create-response comment states the real consumer of the exposed range headers: the CORS-filtered Response the auth service worker hands the media element via respondWith. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
c599254 to
dfc0ae5
Compare
What this does
Native
<audio>/<video>elements cannot attachAuthorization, so on protected realms the host's auth service worker re-issues media requests asmode: 'cors'with the token injected. That rewrite turns the media element'sRangeheader into an author header needing preflight approval — and the realm server'sAccess-Control-Allow-Headersdidn't includeRange, so the preflight failed and the native player errored before any bytes flowed.RangeandIf-Rangejoin the CORS allow list in the realm server's@koa/corsconfig.Content-Range,Accept-Ranges, andContent-Lengthjoin both expose lists: the@koa/corsexposeHeadersand theAccess-Control-Expose-HeadersthatcreateResponsestamps on every realm handler response — the latter replaces rather than merges with the former, so an addition to only one of them is silently dropped on real responses. Exposure matters to more than app JS: the service worker hands the CORS-filteredResponsefrom its ownfetch()straight to the media element viarespondWith, so an unexposedContent-Range/Accept-Rangesis invisible to the player's loading stack itself.Test plan
packages/realm-server/tests/range-request-test.ts— alongside the byte-range serving coverage (same realm fixture): a preflight carryingAccess-Control-Request-Headers: range, if-range, authorizationis approved with all three in the allow list, and a cross-origin206carriesAccess-Control-Allow-Origin(without which no expose list is consulted) and exposesContent-Range/Accept-Ranges/Content-Length.tests/index.ts;http-range-testandrange-request-testare registered there so the range and CORS coverage actually runs in CI.206s → native playback) is deliberately left to follow-up work with the FileDef player integration coverage.🤖 Generated with Claude Code