fix: preserve OAuth popup session state

[sethcarlton]

Summary

OAuth sessions persist a raw random state token. When a self-hosted flow starts from the organization context, the authorization request wraps that token with the organization slug before sending it to the provider. The provider echoes the wrapped value back on callback, but the shared callback handler passed it directly to oauth.complete, which looks up sessions by the raw token.

Cloud already unwraps this state at its request boundary because it must use orgSlug to select the right tenant before constructing the executor. Self-host is single-tenant, so it does not need org routing on callback, but it still needs the raw token for session lookup.

This decodes wrapped callback state in the shared popup callback path and uses the raw token for both OAuth completion and popup result correlation. Raw callback state still passes through unchanged.

Testing

• bun run --cwd packages/core/api test -- oauth-popup.test.ts
• bun run --cwd e2e test:selfhost -- selfhost/oauth-callback-unauthenticated.test.ts

The self-host e2e test was failing before this fix with OAuth session expired or not found on the callback page, and now passes.

[greptile-apps]

Greptile Summary

Fixes a self-host OAuth callback bug where the popup handler was forwarding the provider-echoed org-slug-wrapped state token (base64url({"state":"…","orgSlug":"…"})) directly to oauth.complete instead of the raw session token. Cloud unwraps this at its own routing boundary; self-host was missing the unwrap entirely, causing every self-hosted org-context OAuth flow to fail with "OAuth session expired or not found".

• runOAuthCallback now calls decodeOAuthCallbackState once at the top of the function and falls back to the raw urlParams.state when no wrapping is present; the resolved sessionId is used consistently in the complete() call and all three result branches (success, provider error, defect error).
• A unit test is added verifying that a wrapped-state callback passes the raw token to complete() and embeds it in the popup HTML sessionId; the success path is now fully exercised, while the provider-error and defect-error paths with a wrapped state remain untested.

Confidence Score: 4/5

Safe to merge — the fix is minimal and targeted, touching only the state-unwrapping step in runOAuthCallback, and the fallback preserves existing behavior for raw (non-wrapped) state values.

The core logic is correct and the implementation is clean: sessionId is computed once and used consistently across the success, provider-error, and defect-error branches. The new test locks in the success path. The only gap is that the provider-error and complete()-failure branches with a wrapped state are not explicitly tested — a future regression there would be silent.

oauth-popup.test.ts — provider-error + wrapped-state scenario is not covered by the new test.

Important Files Changed

Filename	Overview
packages/core/api/src/oauth-popup.ts	Adds decodeOAuthCallbackState to unwrap org-slug-prefixed state tokens before passing state to complete() and embedding sessionId in all three result branches (success, provider error, defect). Logic is correct and consistent across all paths.
packages/core/api/src/oauth-popup.test.ts	Adds a test for wrapped-state success path, verifying both complete() receives the raw token and the popup HTML uses it as sessionId. Provider-error and complete()-failure paths with a wrapped state are not explicitly exercised.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant UI as Opener UI
    participant SH as Self-Host /callback
    participant SDK as decodeOAuthCallbackState
    participant OA as oauth.complete()

    UI->>SH: "GET /callback?state=base64({state:"raw123",orgSlug:"acme"})&code=…"

    Note over SH: Before fix: passes wrapped state to complete()
    SH--xOA: "complete({ state: "base64(…)" }) ❌ session not found"

    Note over SH: After fix
    SH->>SDK: decodeOAuthCallbackState("base64(…)")
    SDK-->>SH: "{ state: "raw123", orgSlug: "acme" }"
    SH->>OA: "complete({ state: "raw123", code: "…" }) ✅"
    OA-->>SH: Connection

    SH-->>UI: "popup HTML { sessionId: "raw123", ok: true }"
    Note over UI: Opener correlates on "raw123" ✅

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant UI as Opener UI
    participant SH as Self-Host /callback
    participant SDK as decodeOAuthCallbackState
    participant OA as oauth.complete()

    UI->>SH: "GET /callback?state=base64({state:"raw123",orgSlug:"acme"})&code=…"

    Note over SH: Before fix: passes wrapped state to complete()
    SH--xOA: "complete({ state: "base64(…)" }) ❌ session not found"

    Note over SH: After fix
    SH->>SDK: decodeOAuthCallbackState("base64(…)")
    SDK-->>SH: "{ state: "raw123", orgSlug: "acme" }"
    SH->>OA: "complete({ state: "raw123", code: "…" }) ✅"
    OA-->>SH: Connection

    SH-->>UI: "popup HTML { sessionId: "raw123", ok: true }"
    Note over UI: Opener correlates on "raw123" ✅

Loading

_{Reviews (1): Last reviewed commit: "fix: preserve OAuth popup session state" | Re-trigger Greptile}

[greptile-apps]

Missing test coverage for error paths with wrapped state

The new test validates success + wrapped state, but two other branches go untested: (1) provider error (urlParams.error present) with a wrapped state, and (2) complete() failing with a wrapped state. In both cases sessionId should still be the unwrapped raw token so the opener can correlate the failure popup. The code is correct because sessionId is computed once at the top of runOAuthCallback and shared across all three result paths, but a test for the provider-error + wrapped-state scenario would lock that invariant in explicitly — e.g. urlParams: { state: providerState, error: "access_denied" } with an assertion that html contains "sessionId":"session-raw" and not the wrapped value.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[RhysSullivan]

thanks, hooking up the e2e tests to catch this moving forward as well