Clear the OAuth popup result from localStorage after handover (#1570)

* Clear the OAuth popup result from localStorage after handing it over

The callback page writes its result to localStorage as the reliable same-origin
completion channel, and the opener removes it on pickup. When nothing is
listening — an abandoned flow, a reloaded opener, any caller not using the React
helper — nobody ever removes it, and the payload carries the identity label (an
email) and, on failure, the error preview. It sat in the user's browser profile
indefinitely.

The page now clears its own entry: on success just before the existing
auto-close, and on failure after a delay, since a failed flow deliberately keeps
the window up. This cannot cost a listener the result — a `storage` event
captures `newValue` at dispatch, so an opener that was notified already holds
it, and the only other reader polls `popup.closed`, not storage.

The tests RUN the generated script against stub globals rather than matching its
source, because a string assertion passes just as well on a script that never
executes, and what is at stake here is what the browser is left holding.

* Add a changeset for the popup storage-residue fix

* fix(api): don't strand the OAuth error payload when the user closes the failure popup

The failure page never auto-closes so the user can read the error, and
its localStorage clear rode solely on a 5s timer. Closing the window
earlier killed the pending timer, and with no opener listening (COOP
severs window.opener; a reloaded app tab has no storage listener)
nothing else removed the key — the error payload sat in the browser
profile forever, exactly the residue this branch exists to eliminate.

A pagehide listener now clears the entry when the document dies; the
timers stay for the page merely sitting open. Clearing on pagehide
cannot cost a listener the result: a storage event captures newValue at
dispatch, and the opener ignores the null-newValue removal event.

---------

Co-authored-by: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com>

Author: GeiserX

.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>`;
 };