fix(consent,relay-eager): require a dashboard session, and inspect before abort

Two findings from the second audit round.

The star mutation required a GUI session only when isAgentDriven() was true, and
that function reads the SERVER's environment rather than the caller's. A proxy
running as a service — no agent markers, the normal remote setup — therefore
accepted a raw-admin-token star from anyone who could read the token, which is
every agent on the machine. Caller provenance is not knowable at this endpoint;
the credential is. The dashboard session is now required unconditionally, and the
refusal names the agent markers only when there are any. The former "hand-typed
run stars over HTTP" test encoded exactly the hole, so it is replaced by its
inverse plus a dashboard-click case on the same clean environment.

The eager producer honored the abort signal before examining a settled read. A
read can settle with a real chunk in the same tick the signal fires — the
post-cancel drain does exactly this: the terminal frame arrives, then the drain
deadline aborts upstream — so the terminal was discarded and the turn was
accounted as a cancel. The chunk is inspected first now, and abort is honored
immediately after. Driven red by restoring the old order.

Author: lidge-jun

src/server/management/sidebar-routes.ts

@@ -9,20 +9,22 @@
  * this surface only learns the yes/no answer. `gh` writes the authenticated account
  * name to stderr, so that output is discarded at the source rather than forwarded.
  *
- * The star POST additionally refuses agent-driven programmatic callers. Management
- * auth proves the caller reached the admin token, not that a person chose to star:
- * a coding agent runs on the user's machine and can read that token from disk, so
- * the CLI's "ask the user" deferral would be bypassable with one `curl` here.
+ * The star POST additionally requires a dashboard session. Management auth proves
+ * the caller reached the admin token, not that a person chose to star: a coding
+ * agent runs on the user's machine and can read that token from disk, so the CLI's
+ * "ask the user" deferral would be bypassable with one `curl` here.
  *
- * The dashboard button must keep working even when the proxy itself was started by
- * an agent, which is the common case — the person is at the browser, not at the
- * spawning shell. A GUI click is therefore distinguished by the CREDENTIAL that
- * authorized the request — a GUI session this process minted for a browser, which
- * the auth gate only accepts after matching origin and the per-session CSRF token —
- * rather than by the proxy's own env or by request headers.
+ * The requirement is unconditional, and that is the point. It used to apply only
+ * when `isAgentDriven()` was true — but that function reads the SERVER's
+ * environment, not the caller's, so a proxy already running as a service (no agent
+ * markers, the normal remote setup) accepted a raw-token star from anyone who could
+ * read the token, which includes every agent on the machine. The provenance of the
+ * HTTP caller is not knowable from the server's env; only the credential is. So the
+ * mutation asks for a GUI session this process minted for a browser, which the auth
+ * gate accepts only after matching origin and the per-session CSRF token.
  */
 import { jsonResponse } from "../auth-cors";
-import { agentDrivenMarkers, isAgentDriven } from "../../cli/agent-driven";
+import { agentDrivenMarkers } from "../../cli/agent-driven";
 import type { ManagementContext } from "./context";
 
 /**
@@ -40,12 +42,11 @@ function hasBrowserSessionEvidence(ctx: ManagementContext): boolean {
   return ctx.principal === "gui-session";
 }
 
-// Known edge, deliberately fail-closed: a non-loopback operator dashboard signs in
-// with the raw admin token instead of a minted GUI session, so its clicks carry no
-// CSRF header. If that proxy was *also* started from an agent shell, the button is
-// refused and the response names the one-line `gh` command to run by hand. A
-// service-run proxy (`OCX_SERVICE`, the usual remote setup) is not agent-driven and
-// is unaffected.
+// Known edge, deliberately fail-closed: a non-loopback operator dashboard that signs
+// in with the raw admin token instead of a minted GUI session gets its click refused,
+// and the response names the one-line `gh` command to run by hand. That is the
+// correct trade — an endpoint reachable with a readable token cannot establish that
+// a human chose to spend their own GitHub identity.
 
 export async function handleSidebarRoutes(ctx: ManagementContext): Promise<Response | null> {
   const { req, url } = ctx;
@@ -58,19 +59,21 @@ export async function handleSidebarRoutes(ctx: ManagementContext): Promise<Respo
   if (url.pathname === "/api/github/star" && req.method === "POST") {
     const { STAR_REPO, STAR_REPO_URL, starRepository } = await import("../../github/star-state");
     // Starring uses the user's GitHub identity, so consent must come from the
-    // account owner. An agent-driven caller without browser-session evidence
-    // cannot have obtained it, and must relay the question instead of answering
-    // it with an HTTP call.
-    if (isAgentDriven() && !hasBrowserSessionEvidence(ctx)) {
+    // account owner. Only a minted dashboard session evidences that; a caller
+    // holding the admin token must relay the question instead of answering it
+    // with an HTTP call.
+    if (!hasBrowserSessionEvidence(ctx)) {
+      const markers = agentDrivenMarkers().slice(0, 3);
       return jsonResponse({
         ok: false,
         state: "not-starred",
         repo: STAR_REPO,
         url: STAR_REPO_URL,
         code: "agent_consent_required",
         message:
-          `Refused: agent session detected (${agentDrivenMarkers().slice(0, 3).join(", ")}) and this request `
-          + `carries no dashboard session. Starring writes to the user's own GitHub account, so ask the user `
+          `Refused: this request carries no dashboard session`
+          + `${markers.length ? ` (agent session detected: ${markers.join(", ")})` : ""}. `
+          + `Starring writes to the user's own GitHub account, so ask the user `
           + `directly as a required Yes/No choice — not a soft aside, and no answer is not a No, so re-ask `
           + `it unchanged while it is open — and only if they say yes run: `
           + `gh api -X PUT /user/starred/${STAR_REPO}`,

src/server/relay-eager.ts

@@ -206,8 +206,14 @@ export function relaySseEagerBounded(
     try {
       for (;;) {
         const result = await reader.read();
-        if (upstream.signal.aborted) break;
         const { done: upstreamDone, value } = result;
+        // A chunk that already settled is INSPECTED before abort is honored. A read
+        // can settle with a real chunk in the same tick the signal fires (post-cancel
+        // drain: the terminal frame arrives, then the drain timer aborts upstream).
+        // Checking the signal first discarded that frame, so the terminal was never
+        // recorded and the turn was accounted as a plain cancel.
+        if (!upstreamDone && value !== undefined) hooks.inspectChunk(value);
+        if (upstream.signal.aborted) break;
         if (upstreamDone) {
           hooks.finishInspection();
           if (rewrite) {
@@ -222,7 +228,6 @@ export function relaySseEagerBounded(
           }
           break;
         }
-        hooks.inspectChunk(value);
         if (cancelled) {
           // Discard-drain: inspection only, nothing queued. Stop at terminal
           // or when the bounded window expires.

tests/relay-eager.test.ts

@@ -111,6 +111,28 @@ async function readAll(stream: ReadableStream<Uint8Array>): Promise<string> {
 }
 
 describe("relaySseEagerBounded — inline payload rewrite (#864)", () => {
+  test("a terminal frame settling in the same tick as abort is still recorded", async () => {
+    // Post-cancel drain: the terminal arrives, and the drain deadline aborts
+    // upstream in the same tick. Honoring the signal before examining the settled
+    // read discarded that frame, so the turn was accounted as a cancel instead of
+    // the completion it actually reached.
+    const up = controlledUpstream();
+    const { hooks, rec } = makeHooks();
+    const upstream = new AbortController();
+    const relayed = relaySseEagerBounded(up.stream, upstream, hooks);
+    const reading = readAll(relayed);
+
+    up.push(sse(DELTA));
+    await settle();
+    // Enqueue the terminal and abort without yielding in between.
+    up.push(enc.encode(`event: response.completed\ndata: ${COMPLETED}\n\n`));
+    upstream.abort(new Error("drain window expired"));
+    up.close();
+    await reading;
+
+    expect(rec.terminals.map(t => t.status)).toContain("completed");
+  });
+
   test("rewrites complete blocks across fragmented chunks and flushes the tail at EOF", async () => {
     const up = controlledUpstream();
     const { hooks } = makeHooks();

tests/sidebar-routes.test.ts

@@ -240,14 +240,34 @@ describe("route surface", () => {
     expect(calls).toEqual([]);
   });
 
-  test("a hand-typed run is not blocked by the agent guard", async () => {
+  test("a clean-environment server still refuses a raw-token star", async () => {
+    // The guard used to fire only when isAgentDriven() was true — but that reads
+    // the SERVER's environment, not the caller's. A proxy running as a service has
+    // no agent markers, so this exact request (raw admin token, no dashboard
+    // session) starred the repository for anyone who could read the token, which
+    // is every agent on the machine. Caller provenance is not knowable here; the
+    // credential is, so the dashboard session is required unconditionally.
     const calls: string[][] = [];
     await withEnv(NO_AGENT_ENV, () => withStarDeps({
       nowMs: () => 0,
       async runGh(args) { calls.push(args); return { status: 0 }; },
     }, async () => {
       invalidateStarStatusCache();
-      const { status } = await call("POST", "/api/github/star");
+      const { status, body } = await call("POST", "/api/github/star", {}, "admin-token");
+      expect(status).toBe(403);
+      expect((body as Record<string, unknown>).code).toBe("agent_consent_required");
+    }));
+    expect(calls).toEqual([]);
+  });
+
+  test("a dashboard click on a clean-environment server still stars", async () => {
+    const calls: string[][] = [];
+    await withEnv(NO_AGENT_ENV, () => withStarDeps({
+      nowMs: () => 0,
+      async runGh(args) { calls.push(args); return { status: 0 }; },
+    }, async () => {
+      invalidateStarStatusCache();
+      const { status } = await call("POST", "/api/github/star", {}, "gui-session");
       expect(status).toBe(200);
     }));
     expect(calls.some(args => args.includes("PUT"))).toBe(true);

tests/startup-prompt.test.ts

@@ -115,12 +115,13 @@ describe("startup star prompt", () => {
 
     // The CLI deferral is worthless if an agent can reach the same write over
     // HTTP: it runs on the user's machine and can read the admin token from disk.
-    expect(routes).toContain("isAgentDriven()");
     expect(routes).toContain("agent_consent_required");
-    // A dashboard click must still work when an agent started the proxy, so the
-    // refusal is conditioned on the absence of browser-session evidence.
     expect(routes).toContain("hasBrowserSessionEvidence");
-    expect(routes).toMatch(/isAgentDriven\(\)\s*&&\s*!hasBrowserSessionEvidence\(ctx\)/);
+    // The requirement is UNCONDITIONAL. Gating it on isAgentDriven() reads the
+    // server's environment, not the caller's, so a service-run proxy (no agent
+    // markers) accepted a raw-token star from any agent on the machine.
+    expect(routes).toMatch(/if\s*\(!hasBrowserSessionEvidence\(ctx\)\)/);
+    expect(routes).not.toMatch(/isAgentDriven\(\)\s*&&/);
     // And that evidence must be the authenticating CREDENTIAL, never a request
     // header: the admin token is readable by anything running as the user, so a
     // header-shaped check is forgeable by the exact caller this guard refuses.