fix(annotate): scope the framed 404 to paths that name a file, so a VS Code session renders the app

AGENTS.md

@@ -984,7 +984,7 @@ These surfaces are **comment-only**. `redline` (auto-DELETION) and `quickLabel`
 
 *Security.* Embedded pages are untrusted author HTML served from the Plannotator server's origin. Inside the annotate surface the primary document's `sandbox="allow-scripts"` iframe already protects them: sandboxing flags are inherited and intersected by every nested browsing context, so an embed runs at an opaque origin with no `allow-same-origin` and its `fetch('/api/plan')` fails as cross-origin (verified in the browser, before AND after this change — the pre-fix nested Plannotator was already origin-`null`, which is exactly why the owner saw "no cookies in it"). Defense in depth for the case with no parent sandbox — a reviewer pasting an asset URL into a top-level tab — is on the response: every HTML response from the asset route carries `Content-Security-Policy: sandbox allow-scripts` (`HTML_ASSET_DOCUMENT_CSP`, never `allow-same-origin`) plus `X-Content-Type-Options: nosniff`, and no cookies are involved. Nothing widens what is readable: the same per-directory token, the same `..` refusal, the same `isWithinDirectory` symlink check; HTML documents additionally honour the 2MB `MAX_ANNOTATABLE_FILE_BYTES` annotate cap rather than the 50MB asset cap. The decision is single-sourced in `resolveHtmlAssetRoute` (`packages/shared/html-assets.ts`, vendored to Pi) so the Bun route and the Pi mirror cannot drift.
 
-*Never the app in a frame.* A request whose `Sec-Fetch-Dest` is `iframe`/`frame`/`embed`/`object` — or any `.html` path under the assets prefix — gets a small plain 404 document naming the missing file instead of the catch-all app or a JSON blob, in both runtimes. Ordinary asset misses keep their JSON shape.
+*Never the app in a frame — but only for a path that could BE a file.* Under the assets prefix, a request whose `Sec-Fetch-Dest` is `iframe`/`frame`/`embed`/`object`, or any `.html` path however it was made, gets a small plain 404 document naming the missing file instead of a JSON blob; ordinary asset misses keep their JSON shape. The annotate **catch-all** applies the same 404 document, but only when BOTH conditions hold (`isFramedEmbeddedDocumentRequest` = `isFramedFetchDest` && `pathNamesEmbeddedDocument`, `packages/shared/html-assets.ts`, vendored to Pi and used by both runtimes): the destination is framed, AND the path is not `/` and either its last segment carries an extension (`/prototype-slash.html`) or it sits under a directory segment (`/assets/frame`). `/` and a bare single-segment word (`/settings`) are served the app as always, so a framed session URL and any future SPA route cannot 404. The rule keys on the **shape of the path, not `Sec-Fetch-Site`**: an annotated page is a sandboxed srcdoc with an opaque origin, so its nested-document requests are `cross-site` — the same value the VS Code webview wrapper produces, and `none` on both sides for a pasted URL — so site can never separate the two, while the path can, because the app only ever loads at `/` and a relative embed is anchored at `/api/html-assets/<token>/` by the `<base href>` (with its own 404); only a root-relative embed reaches the catch-all at all. Scoping it this way is the #1561 regression fix: the VS Code extension renders the session URL inside an `<iframe>` behind its cookie proxy (`apps/vscode-extension/src/panel-manager.ts`, `cookie-proxy.ts`, which forwards headers verbatim) and every subcommand launched from a VS Code terminal is routed there by `PLANNOTATOR_BROWSER`, so the unscoped guard made an annotate session opened from the editor show "404 Not found" and its one auto-reload 404 again. A NON-framed request for a missing path is untouched and still gets the app, exactly as before #1561. Plan and review servers carry no such guard.
 
 *Pinpoint on an embed.* The bridge is **never** injected into a nested frame, so an embed is one pinpointable element from the outer page's perspective. While pinpoint is armed, frames are `pointer-events: none` (`body[data-plannotator-frame-inert]`, set by `updatePinpointCursor` for srcdoc sessions only — live-app sessions are untouched, since their nested frames belong to the user's app), which is what lets the click reach the outer document at all; hit-testing would then pass through to the container behind, so `preferInertFrameAt` resolves a point inside a frame's own rect back to the frame. Interact (`Esc`, the header pen, `Mod+Shift+A`) restores native interaction inside the embed — which is also the only state a link inside an embed can be followed from, exactly like the rest of this surface. An embed carries no text, so it takes the pre-existing text-less fail-closed anchor rule: the comment and its element context (tag, ancestor path, the frame's `title` as the accessible name) are recorded and exported, but the placed marker does not restore across a reload unless the author gave the frame an `id` or a `data-testid`. Text-carrying pins on the page around it restore normally.
 

apps/pi-extension/server/serverAnnotate-embeds.test.ts

@@ -103,6 +103,42 @@ describe("pi annotate server: embedded local documents", () => {
 		});
 	});
 
+	// The #1561 regression: the guard keyed on Sec-Fetch-Dest alone, so the app
+	// document 404'd too — and the VS Code extension frames the session URL.
+	test("a framed request for the app document still gets the app shell", async () => {
+		await withSession("framed-root", async ({ url }) => {
+			for (const path of ["/", "/?x=1"]) {
+				const response = await fetch(`${url}${path}`, {
+					headers: { "sec-fetch-dest": "iframe" },
+				});
+				expect(response.status).toBe(200);
+				expect(await response.text()).toContain("PLANNOTATOR_APP_SHELL");
+			}
+		});
+	});
+
+	test("a framed path under a directory is a file reference; a bare word is not", async () => {
+		await withSession("framed-shape", async ({ url }) => {
+			const nested = await fetch(`${url}/assets/frame`, {
+				headers: { "sec-fetch-dest": "iframe" },
+			});
+			expect(nested.status).toBe(404);
+			const bare = await fetch(`${url}/settings`, {
+				headers: { "sec-fetch-dest": "iframe" },
+			});
+			expect(bare.status).toBe(200);
+			expect(await bare.text()).toContain("PLANNOTATOR_APP_SHELL");
+		});
+	});
+
+	test("a plain request for a missing path still gets the app, as before #1561", async () => {
+		await withSession("plain-miss", async ({ url }) => {
+			const response = await fetch(`${url}/prototype-slash.html`);
+			expect(response.status).toBe(200);
+			expect(await response.text()).toContain("PLANNOTATOR_APP_SHELL");
+		});
+	});
+
 	test("an ordinary top-level navigation still gets the app shell", async () => {
 		await withSession("spa", async ({ url }) => {
 			const response = await fetch(`${url}/some/spa/route`, {

apps/pi-extension/server/serverAnnotate.ts

@@ -72,7 +72,7 @@ import {
 	encodeHtmlAssetPath,
 	htmlAssetBaseHref,
 	htmlAssetDocumentHeaders,
-	isFramedFetchDest,
+	isFramedEmbeddedDocumentRequest,
 	resolveHtmlAssetRoute,
 	rewriteHtmlAssetReferences,
 } from "../generated/html-assets.ts";
@@ -1164,11 +1164,14 @@ export async function startAnnotateServer(options: {
 			await handleSaveNotesRequest(req, res);
 		} else if (url.pathname.startsWith("/api/")) {
 			handleApiNotFound(res, url.pathname);
-		} else if (isFramedFetchDest(firstHeader(req.headers["sec-fetch-dest"]))) {
+		} else if (isFramedEmbeddedDocumentRequest(firstHeader(req.headers["sec-fetch-dest"]), url.pathname)) {
 			// Nested-document guard: a request the browser will render inside a
-			// frame must never receive the editor app. Relative embeds are
-			// anchored at their own directory by the asset-route <base href>, so
-			// anything reaching here names a file that genuinely is not there.
+			// frame AND whose path names a file must never receive the editor
+			// app. Relative embeds are anchored at their own directory by the
+			// asset-route <base href>, so anything reaching here names a file
+			// that genuinely is not there. The path condition keeps the app
+			// document itself (`/`, which is how the VS Code extension frames a
+			// session) out of the guard — see pathNamesEmbeddedDocument.
 			const name = url.pathname.split("/").filter(Boolean).pop();
 			res.writeHead(404, htmlAssetDocumentHeaders(HTML_ASSET_ERROR_CSP));
 			res.end(buildHtmlAssetErrorDocument(404, "Not found", name));

packages/server/annotate-html-assets.test.ts

@@ -1,8 +1,9 @@
-import { describe, expect, test } from "bun:test";
+import { afterEach, beforeEach, describe, expect, test } from "bun:test";
 import { mkdirSync, mkdtempSync, realpathSync, symlinkSync, writeFileSync } from "node:fs";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
 import { createHtmlAssetRegistry, framedDocumentNotFound, inlineHtmlLocalAssets } from "./html-assets";
+import { startAnnotateServer } from "./annotate";
 
 describe("annotate raw HTML assets", () => {
   test("rewrites raw HTML support assets and serves them from the source directory", async () => {
@@ -216,6 +217,12 @@ describe("annotate embedded local documents", () => {
     expect(framed?.status).toBe(404);
     expect(framed?.headers.get("content-type")).toContain("text/html");
     expect(framedDocumentNotFound(new Request(String(url)), url)).toBeNull();
+    // ...but never for the app document itself, which is how the VS Code
+    // extension loads a session (#1561 regression).
+    const root = new URL("http://localhost/");
+    expect(
+      framedDocumentNotFound(new Request(String(root), { headers: { "sec-fetch-dest": "iframe" } }), root),
+    ).toBeNull();
     expect(
       framedDocumentNotFound(new Request(String(url), { headers: { "sec-fetch-dest": "document" } }), url),
     ).toBeNull();
@@ -235,3 +242,95 @@ describe("annotate embedded local documents", () => {
     expect(shared).not.toContain("EMBEDDED_SIBLING");
   });
 });
+
+/**
+ * The catch-all's framed guard, over a real server.
+ *
+ * #1561 scoped the guard to `Sec-Fetch-Dest` alone, so it also answered 404 for
+ * the APP document — and the VS Code extension frames the session URL
+ * (`panel-manager.ts` puts it in an `<iframe src>`, and every subcommand
+ * launched from a VS Code terminal is routed there by `PLANNOTATOR_BROWSER`),
+ * so an annotate session opened from the editor showed "404 Not found".
+ */
+describe("annotate catch-all: framed requests", () => {
+  const APP_SHELL = "<html><body>PLANNOTATOR_APP_SHELL</body></html>";
+  let savedPort: string | undefined;
+  let savedRemote: string | undefined;
+
+  beforeEach(() => {
+    savedPort = process.env.PLANNOTATOR_PORT;
+    savedRemote = process.env.PLANNOTATOR_REMOTE;
+    delete process.env.PLANNOTATOR_PORT;
+    process.env.PLANNOTATOR_REMOTE = "0";
+  });
+
+  afterEach(() => {
+    if (savedPort === undefined) delete process.env.PLANNOTATOR_PORT;
+    else process.env.PLANNOTATOR_PORT = savedPort;
+    if (savedRemote === undefined) delete process.env.PLANNOTATOR_REMOTE;
+    else process.env.PLANNOTATOR_REMOTE = savedRemote;
+  });
+
+  async function withServer(run: (url: string) => Promise<void>): Promise<void> {
+    const dir = realpathSync(mkdtempSync(join(tmpdir(), "plannotator-framed-catchall-")));
+    const filePath = join(dir, "notes.md");
+    writeFileSync(filePath, "# Notes", "utf-8");
+    const server = await startAnnotateServer({
+      markdown: "# Notes",
+      filePath,
+      htmlContent: APP_SHELL,
+    });
+    try {
+      await run(server.url);
+    } finally {
+      server.stop();
+    }
+  }
+
+  const framed = (url: string, path: string) =>
+    fetch(`${url}${path}`, { headers: { "sec-fetch-dest": "iframe" } });
+
+  test("serves the app to a framed request for the app document", async () => {
+    await withServer(async (url) => {
+      for (const path of ["/", "/?x=1"]) {
+        const response = await framed(url, path);
+        expect(response.status).toBe(200);
+        expect(response.headers.get("content-type")).toContain("text/html");
+        expect(await response.text()).toContain("PLANNOTATOR_APP_SHELL");
+      }
+    });
+  });
+
+  test("answers a framed file reference with the 404 document", async () => {
+    await withServer(async (url) => {
+      const response = await framed(url, "/prototype-slash.html");
+      expect(response.status).toBe(404);
+      const body = await response.text();
+      expect(body).toContain("prototype-slash.html");
+      expect(body).not.toContain("PLANNOTATOR_APP_SHELL");
+    });
+  });
+
+  test("a framed path under a directory is a file reference; a bare word is not", async () => {
+    await withServer(async (url) => {
+      // Only a root-relative embed is spelled with a directory segment; the app
+      // has no nested routes, so this is a miss worth naming.
+      expect((await framed(url, "/assets/frame")).status).toBe(404);
+      // One bare segment stays with the app, so a future SPA route cannot 404
+      // inside a frame.
+      const bare = await framed(url, "/settings");
+      expect(bare.status).toBe(200);
+      expect(await bare.text()).toContain("PLANNOTATOR_APP_SHELL");
+    });
+  });
+
+  test("a plain request for a missing path still gets the app, as before #1561", async () => {
+    await withServer(async (url) => {
+      for (const path of ["/prototype-slash.html", "/assets/frame", "/"]) {
+        const response = await fetch(`${url}${path}`);
+        expect(response.status).toBe(200);
+        expect(await response.text()).toContain("PLANNOTATOR_APP_SHELL");
+      }
+    });
+  });
+});

packages/server/html-assets.ts

@@ -7,7 +7,7 @@ import {
   encodeHtmlAssetPath,
   htmlAssetBaseHref,
   htmlAssetDocumentHeaders,
-  isFramedFetchDest,
+  isFramedEmbeddedDocumentRequest,
   resolveHtmlAssetRoute,
   rewriteHtmlAssetReferences,
 } from "@plannotator/shared/html-assets";
@@ -39,13 +39,18 @@ function assetError(
 
 /**
  * The catch-all's guard: a request the browser will render as a nested
- * document must never receive the editor app. That is the bug this whole
- * change is about — Plannotator rendering inside an annotated page's embed —
- * and the `<base href>` fix removes the usual way of getting here, so anything
- * still arriving is a genuinely missing file and deserves to say so.
+ * document, AND whose path names a file, must never receive the editor app.
+ * That is the bug this whole change is about — Plannotator rendering inside an
+ * annotated page's embed — and the `<base href>` fix removes the usual way of
+ * getting here, so anything still arriving is a genuinely missing file and
+ * deserves to say so. The path condition is what keeps the app document itself
+ * (`/`) out of it: see `pathNamesEmbeddedDocument` for why the shape of the
+ * path, and not `Sec-Fetch-Site`, is the signal.
  */
 export function framedDocumentNotFound(req: Request, url: URL): Response | null {
-  if (!isFramedFetchDest(req.headers.get("sec-fetch-dest"))) return null;
+  if (!isFramedEmbeddedDocumentRequest(req.headers.get("sec-fetch-dest"), url.pathname)) {
+    return null;
+  }
   const name = url.pathname.split("/").filter(Boolean).pop();
   return new Response(buildHtmlAssetErrorDocument(404, "Not found", name), {
     status: 404,

packages/shared/html-assets.test.ts

@@ -3,7 +3,9 @@ import {
   buildHtmlAssetErrorDocument,
   encodeHtmlAssetPath,
   htmlAssetContentType,
+  isFramedEmbeddedDocumentRequest,
   isFramedFetchDest,
+  pathNamesEmbeddedDocument,
   rewriteCssAssetReferences,
   normalizeHtmlAssetRoutePath,
   resolveHtmlAssetRoute,
@@ -233,3 +235,38 @@ describe("isFramedFetchDest", () => {
     }
   });
 });
+
+describe("pathNamesEmbeddedDocument", () => {
+  // The #1561 regression: the guard fired on the app document too, so the VS
+  // Code panel (which frames the session URL) rendered "404 Not found".
+  test("the app document is never a missing embed, whatever the query string", () => {
+    for (const pathname of ["/", "//"]) {
+      expect(pathNamesEmbeddedDocument(pathname)).toBe(false);
+    }
+    // A query string is not part of the pathname, so `/?x=1` reads as `/`.
+    expect(pathNamesEmbeddedDocument(new URL("http://localhost/?x=1").pathname)).toBe(false);
+  });
+
+  test("a file reference is a missing embed", () => {
+    for (const pathname of ["/prototype-slash.html", "/chart.svg", "/app.js", "/assets/frame"]) {
+      expect(pathNamesEmbeddedDocument(pathname)).toBe(true);
+    }
+  });
+
+  test("a bare single-segment word stays with the app", () => {
+    // Nothing routes these today; leaving them to the catch-all is what stops a
+    // future SPA route from 404ing inside a frame.
+    for (const pathname of ["/settings", "/review"]) {
+      expect(pathNamesEmbeddedDocument(pathname)).toBe(false);
+    }
+  });
+});
+
+describe("isFramedEmbeddedDocumentRequest", () => {
+  test("needs both a framed destination and a file-shaped path", () => {
+    expect(isFramedEmbeddedDocumentRequest("iframe", "/gone.html")).toBe(true);
+    expect(isFramedEmbeddedDocumentRequest("iframe", "/")).toBe(false);
+    expect(isFramedEmbeddedDocumentRequest("document", "/gone.html")).toBe(false);
+    expect(isFramedEmbeddedDocumentRequest(null, "/gone.html")).toBe(false);
+  });
+});

packages/shared/html-assets.ts

@@ -48,6 +48,57 @@ export function isFramedFetchDest(secFetchDest: string | null | undefined): bool
   return typeof secFetchDest === "string" && FRAMED_FETCH_DESTS.has(secFetchDest.trim().toLowerCase());
 }
 
+/**
+ * A single path segment that names a file: `page.html`, `chart.svg`, `app.js`.
+ * Bounded on purpose — a long trailing dot-run in a slug (`v1.2.3-release`) is
+ * still a file shape, but an arbitrary tail is not worth treating as one.
+ */
+const FILE_NAME_SEGMENT = /\.[A-Za-z0-9][A-Za-z0-9_-]{0,9}$/;
+
+/**
+ * Could this path name a MISSING embedded document, as opposed to the app
+ * document itself?
+ *
+ * The annotate catch-all serves the editor app for every non-`/api` path, so
+ * without this the framed-404 guard answers 404 for the app root too — which is
+ * exactly how the VS Code extension loads a session (`panel-manager.ts` puts the
+ * session URL in an `<iframe src>`, and every subcommand launched from a VS Code
+ * terminal is routed there), so the panel rendered "404 Not found" instead of
+ * the app (#1561 regression).
+ *
+ * The rule is the SHAPE OF THE PATH, not `Sec-Fetch-Site`: an annotated page is
+ * a sandboxed srcdoc with an opaque origin, so its nested-document requests are
+ * `cross-site` — the same value the VS Code webview wrapper produces, and a
+ * pasted URL is `none` on both sides. Site can never separate the two; the path
+ * can, because the app is only ever loaded at `/` while an embed that reaches
+ * the catch-all was written as a root-relative file reference (relative ones are
+ * anchored at `/api/html-assets/<token>/` by #1561's `<base href>`, which has
+ * its own 404).
+ */
+export function pathNamesEmbeddedDocument(pathname: string): boolean {
+  const segments = pathname.split("/").filter(Boolean);
+  // `/` (and `//`): the app document. Never a missing embed.
+  if (segments.length === 0) return false;
+  // Under a directory segment (`/assets/frame`): only a file reference is
+  // spelled that way; the app has no nested routes.
+  if (segments.length > 1) return true;
+  // One segment: a file name (`/prototype-slash.html`), not a bare word, which
+  // stays with the app so a future SPA route cannot 404 inside a frame.
+  return FILE_NAME_SEGMENT.test(segments[0]);
+}
+
+/**
+ * The catch-all's guard, shared by both runtimes: a request the browser will
+ * render as a nested document AND whose path names a file gets the small 404
+ * document instead of the editor app.
+ */
+export function isFramedEmbeddedDocumentRequest(
+  secFetchDest: string | null | undefined,
+  pathname: string,
+): boolean {
+  return isFramedFetchDest(secFetchDest) && pathNamesEmbeddedDocument(pathname);
+}
+
 /** `<base href>` value that anchors a document's relative URLs at its own directory. */
 export function htmlAssetBaseHref(token: string): string {
   return `${HTML_ASSET_ROUTE_PREFIX}/${encodeURIComponent(token)}/`;