Serve document bytes, not the app shell, to <object>/<embed> loads - #5816
Serve document bytes, not the app shell, to <object>/<embed> loads#5816lukemelia wants to merge 1 commit into
Conversation
lukemelia
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] This review focused on verifying the fix's transport-layer premise end-to-end — does the pinned Accept header actually reach the wire on an <object>/<embed> load? — because the whole change hangs on the service worker seeing those requests.
Bottom line: blocking. Service workers never receive fetch events for <object>/<embed> loads — the ServiceWorker specification makes them fall back to the network without dispatching an event, and I reproduced exactly that in Chrome 151 — so acceptOverrideFor never fires, the pinned Accept never leaves the browser, and the recursive app boot is still reachable. Full evidence (spec text, live-browser repro, captured request headers) is in the inline thread on acceptOverrideFor in packages/host/public/auth-service-worker.js.
What lands right. The diagnosis of the recursion is verified correct on both halves. The captured <embed> request really does go out frame-style with accept: text/html,application/xhtml+xml,... and sec-fetch-mode: navigate. And on the realm side, serveIndex in packages/realm-server/handlers/serve-index.ts serves the app shell for any text/html-accepting request without a vnd. type regardless of file extension, while with Accept: */* an extension-carrying path takes the hasExtension branch to next(), reaching the realm's fallbackHandle → serveLocalFile, which returns the document bytes. If the pinned header reached the wire, the realm would behave exactly as this description says. The interception point is the only wrong link in the chain.
Recommendations
- Refix at the realm's content negotiation:
serveIndexshould decline to serve the app shell when the request carriesSec-Fetch-Dest: embedorobject— these loads self-identify on the wire, address-bar navigations carrySec-Fetch-Dest: documentand keep working, and no client change is needed. Detail in theacceptOverrideForthread. - Pin whichever fix lands with a wire-level realm-server integration test (frame-style request for a file URL asserting bytes, not shell) — detail in the thread on the test scaffold in
packages/host/tests/unit/auth-service-worker-test.ts. - Follow-up worth filing: private-realm documents cannot authenticate through native embeds at all — the same bypass blocks
Authorizationinjection, so the native-viewer approach needs an auth story (authed fetch +blob:, delegated/signed URLs, or cookie-scoped sessions).
Adjacent, out of scope. The comment in packages/base/file-formats/pdf-viewer.gts stating the auth service worker injects the realm token on the native <object> request describes a path that does not exist (same bypass). Pre-existing and not this change's doing, but worth correcting when the real fix lands so the next reader doesn't build on it again.
| function acceptOverrideFor(destination) { | ||
| return destination === 'object' || destination === 'embed' | ||
| ? '*/*' | ||
| : undefined; | ||
| } |
There was a problem hiding this comment.
[Claude Code 🤖] This override can never take effect: browsers do not dispatch service worker fetch events for <object>/<embed> loads. request.destination is never 'object' or 'embed' inside this fetch handler, so the pinned Accept never reaches the wire and the recursive app boot is still reachable.
The mechanism. The ServiceWorker specification carves these two elements out of interception deliberately, because plugin content derives its security origin from its own URL and a controlling service worker can't mediate that. From the spec's Implementer Concerns section (w3c.github.io/ServiceWorker, §6.7):
Plug-ins should not load via service workers. As plug-ins may get their security origins from their own urls, the embedding service worker cannot handle it. For this reason, the Handle Fetch algorithm makes
<embed>and<object>requests immediately fallback to the network without dispatching fetch event.
The Fetch specification correspondingly classifies destinations "embed" and "object" as non-subresource requests — navigation-class, not subresources of the controlled page.
Verified empirically (Chrome 151). A page controlled by a service worker whose fetch listener logs every event's destination, mounting — same-origin, all inside the SW's scope — <object data="/test.pdf" type="application/pdf">, <embed src="/test2.pdf" type="application/pdf">, and <img src="/probe.png">:
- The image request dispatched a fetch event (destination
"image"). Neither PDF load dispatched one; the test HTTP server's log shows the browser fetched both PDFs directly from the network. - The
<embed>request left the browser with its frame-style headers untouched by the SW:
accept: text/html,application/xhtml+xml,application/xml;q=0.9,...,*/*;q=0.8
sec-fetch-dest: embed
sec-fetch-mode: navigate
The same bypass applies to the Authorization injection this file exists for: the SW cannot add a bearer token to an <object> load either, so a private realm's document can't authenticate through the native viewer at all (today such a load receives the app shell, which is served unauthenticated).
The way out. The realm already owns this negotiation decision, and these loads self-identify on the wire: as captured above, an embed load carries Sec-Fetch-Dest: embed (object for <object>), while a pasted-URL navigation carries Sec-Fetch-Dest: document. serveIndex in packages/realm-server/handlers/serve-index.ts already branches on the Accept header to choose app shell vs next(); additionally declining to serve the shell when Sec-Fetch-Dest is embed or object fixes the recursion at the layer that actually sees the request, keeps address-bar navigation opening the app, needs no client change, and works for prerendered markup carrying real file URLs — the property this change was designed to preserve. Browsers stamp Sec-Fetch-* on trustworthy origins (HTTPS and localhost), which covers dev, staging, and production; a browser that omits it just keeps today's behavior.
Private-realm document auth through native embeds remains a separate gap under any variant of that fix (these loads can carry neither Authorization nor a SW rewrite) — that wants its own design (authed fetch + blob: in the viewer, delegated/signed URLs, or cookie-scoped sessions) as a follow-up.
Scope. Blocking for this change's goal: the new code is unreachable in conformant browsers, so the bug it targets is unfixed. It is inert rather than harmful, but shipping it would cement a wrong assumption — that the service worker mediates embed loads — into the transport layer's contract.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 10e90a9. The Accept-pinning override is removed along with its unit-test mirror; the negotiation now lives where the request is actually visible — the realm server. serveIndex and serveHostApp in packages/realm-server/handlers/serve-index.ts decline to serve the app shell when the request carries Sec-Fetch-Dest: embed or object and fall through to the realm, which serves the file's own bytes with the file's own content type. Address-bar navigations (Sec-Fetch-Dest: document, or no header) keep opening the app, and the cacheable (ETagged) shell responses add Sec-Fetch-Dest to Vary so a cached shell can never satisfy an embed load. The private-realm auth gap for native embeds (these loads can't carry Authorization either) remains open as a follow-up, and is now stated on resourceUrl in packages/base/file-formats/pdf-viewer.gts.
| // Mirrors auth-service-worker.js's acceptOverrideFor. A synthetic Request | ||
| // can't carry a `destination`, so the scaffold threads it explicitly where | ||
| // the real SW reads `request.destination`. | ||
| function acceptOverrideFor(destination?: string): string | undefined { | ||
| return destination === 'object' || destination === 'embed' | ||
| ? '*/*' | ||
| : undefined; | ||
| } |
There was a problem hiding this comment.
[Claude Code 🤖] The scaffold's explicit destination parameter is the tell for why these tests stay green while the feature is unreachable: a synthetic Request can't carry a destination because only the browser assigns one — and for 'object'/'embed' the browser assigns them to requests it never shows a service worker (see the thread on acceptOverrideFor in packages/host/public/auth-service-worker.js). So the new module pins the mirror's behavior faithfully, but the behavior it pins cannot occur on the wire.
This isn't a flaw the mirror pattern could have avoided — no test at this layer can observe "the browser never dispatches the event." It's a reason the executable check for whichever fix lands should live where the wire is real: a realm-server integration test that issues a file GET with Sec-Fetch-Dest: embed / a frame-style Accept and asserts document bytes come back rather than the app shell would fail today and pin the actual contract.
Scope. Non-blocking on its own; folds into the main finding — these three tests should move or be retired along with the Accept-pinning logic they mirror.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 10e90a9. The mirror tests are removed together with the logic they pinned; the executable check now lives at the wire: the document embed negotiation module in packages/realm-server/tests/server-endpoints/index-responses-test.ts issues real GETs for a .pdf with a frame-style Accept plus Sec-Fetch-Dest: embed / object / document / absent, asserting document bytes for the embed destinations and the app shell for the navigation cases, and the published-realm ETag test asserts Vary includes Sec-Fetch-Dest.
442b914 to
10e90a9
Compare
An <object>/<embed> load is issued as a frame-style navigation whose Accept header includes text/html, and the realm serves the host app for any HTML-accepting request — so an embedded PDF viewer received the app shell and booted the app recursively inside the preview. No client-side layer can correct this: the ServiceWorker spec makes <embed>/<object> requests fall back to the network without dispatching a fetch event. Browsers do stamp Sec-Fetch-Dest on these loads (on trustworthy origins), so serveIndex/serveHostApp now decline to serve the shell when it is embed or object and fall through to the realm, which serves the file's own bytes. Address-bar navigations (Sec-Fetch-Dest: document, or absent) keep opening the app, and cacheable shell responses vary on Sec-Fetch-Dest so a cached shell can never satisfy an embed load. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
10e90a9 to
4b46c8d
Compare
What this does
Opening a PDF FileDef in the embedded or isolated view rendered the entire Boxel app recursively nested inside the preview. The chain: the PDF viewer mounts a native
<object data="…/file.pdf" type="application/pdf">; the browser issues<object>/<embed>loads as frame-style navigations whoseAcceptheader includestext/html; and the realm serves the host app for any request that accepts HTML (the same negotiation that lets a pasted file URL open in operator mode) — so the<object>receives the app shell, boots it, deep-links to the PDF card, mounts another<object>, and recurses.The negotiation is corrected on the server, because no client-side layer can do it: service workers are spec-required to pass
<object>/<embed>loads straight to the network without dispatching a fetch event (ServiceWorker spec §6.7; confirmed empirically in Chrome — a controlling SW sees<img>loads but never object/embed loads). These loads do self-identify on the wire, though: browsers stampSec-Fetch-Dest: embed/objecton them (on trustworthy origins — HTTPS and localhost), while an address-bar navigation carriesSec-Fetch-Dest: document.serveIndex/serveHostAppin the realm server now decline to serve the app shell when the request'sSec-Fetch-Destisembedorobject, falling through to the realm, which serves the file's own bytes with the file's own content type. Consequences of fixing at this layer:Sec-Fetch-Dest: document), and requests with noSec-Fetch-Destkeep their existing behavior.<object data="…">stays valid in any browsing context, where a component-fetchedblob:URL would be dead outside the context that created it.Sec-Fetch-DesttoVary, so a cached shell can never satisfy an embed load.Known limitation, unchanged by this PR:
<object>/<embed>loads can't carryAuthorization(the same service-worker bypass blocks token injection), so documents render in the native viewer only from publicly readable realms. Private-realm document auth through native embeds needs its own design — follow-up.Test plan
packages/realm-server/tests/server-endpoints/index-responses-test.ts(document embed negotiationmodule): a.pdfGET with a frame-styleAcceptplusSec-Fetch-Dest: embedorobjectreturnsapplication/pdfbytes, not the shell; the same URL withSec-Fetch-Dest: documentor noSec-Fetch-Deststill returns the app shell; the published-realm ETag response'sVaryincludesSec-Fetch-Dest. All touched modules green locally (the full file exhausts the local test-pg tmpfs in unrelated later modules, so they were run per-module).Fixes CS-12557.
🤖 Generated with Claude Code