Clear the OAuth popup result from localStorage after handover

.changeset/oauth-popup-localstorage-residue.md

@@ -0,0 +1,9 @@
+---
+"executor": patch
+---
+
+**The OAuth popup clears its result out of `localStorage` after handing it over**
+
+The popup writes its result to `localStorage` as the fallback completion channel, because `postMessage` is severed when a provider's consent page sets COOP and `BroadcastChannel` can be partitioned or raced by the auto-close. Nothing removed that entry afterwards, so the payload — which carries the identity label, an email, and on failure the error preview — stayed parked in the user's browser profile.
+
+The entry is now cleared once the handover has had time to land, and on `pagehide` as a backstop — the failure page never auto-closes so the user can read the error, and closing it by hand would otherwise cancel the pending timer and strand the entry. This cannot cost a listener the result: a `storage` event captures `newValue` at dispatch, so an opener that has been notified already holds it.

packages/core/api/src/oauth-popup.test.ts

@@ -149,6 +149,106 @@ describe("popupDocument", () => {
     expect(script).toContain("channel\\u003c/script\\u003e");
   });
 
+  // The assertions below RUN the generated script against stub globals rather
+  // than matching its source text. A string check would pass on a script that
+  // never executes — and the property at stake here is what the browser is left
+  // holding, which only running it can show.
+  const runPopupScript = (html: string) => {
+    const script = /<script>\n?([\s\S]*?)<\/script>/.exec(html)?.[1];
+    expect(script).toBeDefined();
+    const store = new Map<string, string>();
+    const timers: { readonly fn: () => void; readonly ms: number }[] = [];
+    const pagehideListeners: (() => void)[] = [];
+    let closed = false;
+    const win = {
+      opener: null,
+      location: { origin: "https://app.example" },
+      close: () => {
+        closed = true;
+      },
+      addEventListener: (type: string, listener: () => void) => {
+        if (type === "pagehide") pagehideListeners.push(listener);
+      },
+    };
+    const fn = new Function(
+      "window",
+      "localStorage",
+      "setTimeout",
+      "BroadcastChannel",
+      script ?? "",
+    ) as (w: unknown, ls: unknown, st: unknown, bc: unknown) => void;
+    fn(
+      win,
+      {
+        setItem: (k: string, v: string) => store.set(k, v),
+        removeItem: (k: string) => store.delete(k),
+      },
+      (cb: () => void, ms: number) => {
+        timers.push({ fn: cb, ms });
+        return timers.length;
+      },
+      undefined,
+    );
+    return {
+      store,
+      isClosed: () => closed,
+      runTimers: () => {
+        for (const t of [...timers]) t.fn();
+      },
+      // Simulates the document dying (user closes the window): pagehide fires,
+      // pending timers never do.
+      firePagehide: () => {
+        for (const listener of [...pagehideListeners]) listener();
+      },
+    };
+  };
+
+  it("clears the stored result after handing it over on success", () => {
+    const html = popupDocument(successPayload, "chan-1");
+    const run = runPopupScript(html);
+    // Written first, so a listening opener gets its `storage` event.
+    expect(run.store.get("chan-1")).toContain("session-abc");
+
+    run.runTimers();
+
+    // ...and not left in the profile afterwards. The payload carries an identity
+    // label; nobody listening must not mean it sits there forever.
+    expect(run.store.has("chan-1")).toBe(false);
+    expect(run.isClosed()).toBe(true);
+  });
+
+  it("clears the stored result on failure too, without closing the window", () => {
+    const html = popupDocument(
+      { type: OAUTH_POPUP_MESSAGE_TYPE, ok: false, sessionId: null, error: "nope" },
+      "chan-2",
+    );
+    const run = runPopupScript(html);
+    expect(run.store.get("chan-2")).toContain("nope");
+
+    run.runTimers();
+
+    expect(run.store.has("chan-2")).toBe(false);
+    // A failed flow keeps the window up so the user can read the error.
+    expect(run.isClosed()).toBe(false);
+  });
+
+  it("clears the stored result when the user closes the failure window before the timer", () => {
+    const html = popupDocument(
+      { type: OAUTH_POPUP_MESSAGE_TYPE, ok: false, sessionId: null, error: "nope" },
+      "chan-3",
+    );
+    const run = runPopupScript(html);
+    expect(run.store.get("chan-3")).toContain("nope");
+
+    // The failure page never auto-closes; the user reads the error and closes
+    // the window before the 5s timer fires. The document dies — pending timers
+    // never run — so pagehide is the only thing standing between the payload
+    // and living in the browser profile forever.
+    run.firePagehide();
+
+    expect(run.store.has("chan-3")).toBe(false);
+  });
+
   it("posts to window.opener AND falls back to BroadcastChannel with the given channel name", () => {
     const html = popupDocument(successPayload, "executor:openapi-oauth-result");
     expect(html).toContain("window.opener.postMessage(p,window.location.origin)");

packages/core/api/src/oauth-popup.ts

@@ -119,7 +119,18 @@ ${detailsHtml}
 try{if(window.opener)window.opener.postMessage(p,window.location.origin)}catch(e){}
 try{if("BroadcastChannel"in window){const c=new BroadcastChannel(${serializedChannel});c.postMessage(p);setTimeout(()=>c.close(),100)}}catch(e){}
 try{localStorage.setItem(${serializedChannel},JSON.stringify(p))}catch(e){}
-if(p.ok)setTimeout(()=>window.close(),400);})();
+// The payload carries the identity label — an email — and, on failure, the
+// error preview, so it must not outlive the handover. Clearing it cannot cost a
+// listener the result: a 'storage' event captures newValue at dispatch, so an
+// opener that has been notified already holds it. Leaving it would park that
+// data in the user's browser profile indefinitely whenever nobody is listening,
+// which is every abandoned or opener-less flow. pagehide backs the timers up:
+// the failure page never auto-closes (the user must be able to read the error),
+// and closing it kills any pending timer — without pagehide the entry would
+// outlive the document after all.
+const clear=()=>{try{localStorage.removeItem(${serializedChannel})}catch(e){}};
+window.addEventListener("pagehide",clear);
+if(p.ok)setTimeout(()=>{clear();window.close()},400);else setTimeout(clear,5000);})();
 </script>
 </body></html>`;
 };