From ee4757ca753414f7498247fd5244ea51c169c05b Mon Sep 17 00:00:00 2001 From: James Lawton Date: Thu, 16 Jul 2026 16:38:13 +0100 Subject: [PATCH] fix: use a static oms return uri and carry the login session in sessionstorage --- .../agentconnect-ui/src/login/LoginPage.tsx | 46 +++++++++++++++++-- .../src/login/returnUrl.test.ts | 22 ++++----- .../agentconnect-ui/src/login/returnUrl.ts | 32 +++++++------ .../src/lib/browser-login.test.ts | 2 +- .../src/lib/browser-login.ts | 14 ++++-- 5 files changed, 80 insertions(+), 36 deletions(-) diff --git a/packages/agentconnect-ui/src/login/LoginPage.tsx b/packages/agentconnect-ui/src/login/LoginPage.tsx index 2c6890c..fe9141d 100644 --- a/packages/agentconnect-ui/src/login/LoginPage.tsx +++ b/packages/agentconnect-ui/src/login/LoginPage.tsx @@ -60,9 +60,36 @@ async function fetchStatus(session: string): Promise { const TERMINAL: MachineState['kind'][] = ['success', 'expired', 'failed']; +// The OMS relay return URI is now the bare, static `/login` (required to +// match its allowlist exactly), so it can no longer carry the pairing +// session as a query param. Instead, the session is stashed here before the +// redirect to Google and recovered from here on the bounce back, since a +// sessionStorage entry survives that round trip on the same origin. +const SESSION_STORE_KEY = 'oms_login_session'; + export function LoginPage() { - const session = getSessionId(window.location.search, window.location.hash); const relayReturn = isRelayReturn(window.location.search); + // Resolve the session from the url first (fresh CLI announce, or the + // legacy `?s=` shape); fall back to whatever was stashed before the + // redirect to Google. Runs once per mount via the lazy state initializer, + // so the sessionStorage write on a fresh url load never thrashes. + const [session] = useState(() => { + const fromUrl = getSessionId(window.location.search, window.location.hash); + if (fromUrl) { + try { + window.sessionStorage.setItem(SESSION_STORE_KEY, fromUrl); + } catch { + // Best-effort: storage may be unavailable (private browsing, etc). + // This load still has the session; it just won't survive a redirect. + } + return fromUrl; + } + try { + return window.sessionStorage.getItem(SESSION_STORE_KEY) ?? ''; + } catch { + return ''; + } + }); const [state, setState] = useState(() => relayReturn ? reduce(initialState, { type: 'relay-return' }) : initialState ); @@ -78,10 +105,23 @@ export function LoginPage() { // A failed post (a network error, or a 410 for a session that already // expired) must not leave the user stuck on the "finishing sign in" // spinner until the ~10-minute poll timeout, so a false result dispatches - // the terminal failed state right away. + // the terminal failed state right away. If the session could not be + // recovered at all (sessionStorage empty, e.g. a different browser or a + // cleared session), there is nothing to post against, so it fails fast + // instead of posting an empty session to the relay. useEffect(() => { - if (postedRelayCallback.current || !session || !relayReturn) return; + if (postedRelayCallback.current || !relayReturn) return; postedRelayCallback.current = true; + if (!session) { + dispatch({ + type: 'status', + status: { + status: 'error', + message: 'Could not recover the login session. Re-run wallet login in your terminal.' + } + }); + return; + } void postAction(session, { type: 'oidc-callback', callbackUrl: window.location.href }).then( (ok) => { if (ok) return; diff --git a/packages/agentconnect-ui/src/login/returnUrl.test.ts b/packages/agentconnect-ui/src/login/returnUrl.test.ts index 942c5bb..a7e0670 100644 --- a/packages/agentconnect-ui/src/login/returnUrl.test.ts +++ b/packages/agentconnect-ui/src/login/returnUrl.test.ts @@ -25,32 +25,28 @@ describe('isRelayReturn', () => { expect(isRelayReturn('#abc')).toBe(false); }); - it('is false for a bare `s` with no other query keys', () => { - expect(isRelayReturn('?s=abc')).toBe(false); + it('is true for a bare oauth callback param, no `s` needed', () => { + expect(isRelayReturn('?code=xyz')).toBe(true); }); - it('is true once a relay callback param joins `s`', () => { + it('is true when `s` and an oauth callback param are both present', () => { expect(isRelayReturn('?s=abc&code=xyz')).toBe(true); }); - it('is true with multiple relay callback params alongside `s`', () => { - expect(isRelayReturn('?s=abc&state=1&code=2')).toBe(true); + it('is true for a bare `state` param', () => { + expect(isRelayReturn('?state=1')).toBe(true); }); it('is true when the relay reports an oauth error', () => { - expect(isRelayReturn('?s=abc&error=access_denied')).toBe(true); + expect(isRelayReturn('?error=access_denied')).toBe(true); }); - it('is false when a link wrapper appends utm params to a pasted link', () => { + it('is false for `s` alongside a non-oauth query param', () => { expect(isRelayReturn('?s=abc&utm_source=x')).toBe(false); }); - it('is false when a link wrapper appends a gclid param to a pasted link', () => { - expect(isRelayReturn('?s=abc&gclid=x')).toBe(false); - }); - - it('is false when `s` is missing, even with other query keys', () => { - expect(isRelayReturn('?code=xyz')).toBe(false); + it('is false for a non-oauth query param with no `s`', () => { + expect(isRelayReturn('?utm_source=x')).toBe(false); }); it('is false for an empty query string', () => { diff --git a/packages/agentconnect-ui/src/login/returnUrl.ts b/packages/agentconnect-ui/src/login/returnUrl.ts index 4369747..d89359e 100644 --- a/packages/agentconnect-ui/src/login/returnUrl.ts +++ b/packages/agentconnect-ui/src/login/returnUrl.ts @@ -7,15 +7,19 @@ // - CLI announce: `/login#` (fragment). The CLI opens this in the // user's browser with nothing to lose by putting the session in the // fragment, since there is no round trip through a third party. -// - Relay return: `/login?s=&` (query). Once Google -// sign-in bounces through the OMS relay and back, the session has to -// survive as a query param, since a fragment is never sent to (or -// preserved by) a server in the middle of that redirect chain, and the -// relay appends its own callback params (code, state, etc.) alongside it. +// - Relay return: `/login?` (query only, e.g. `code`, `state`). +// The return URI registered with the OMS relay must be the bare, static +// `/login` to satisfy its allowlist (an exact-string match), so it can no +// longer carry `?s=`. The pairing session is instead stashed in +// sessionStorage by the component before the redirect to Google, and +// recovered from there when the browser bounces back; these helpers only +// see the query/fragment, not sessionStorage. -// Session id lives in the `?s=` query param (used for the OMS relay return -// URI, since fragments may be consumed by the relay); the `#` fragment is -// kept as a fallback for older announce links. +// Session id lives in the `?s=` query param (used for the legacy relay +// return shape, and still supported); the `#` fragment is the primary +// carrier for the CLI announce link. On an OMS relay return neither is +// present, since the return URI is now the bare, static `/login` — the +// component falls back to sessionStorage in that case. export function getSessionId(search: string, hash: string): string { const fromQuery = new URLSearchParams(search).get('s'); if (fromQuery) return fromQuery; @@ -30,13 +34,13 @@ const OAUTH_CALLBACK_PARAMS = ['code', 'state', 'error']; // True when this load is the browser bouncing back from the OMS relay after // Google sign-in, not a fresh open. We key specifically on the OAuth callback -// params (`code`, `state`, `error`) the relay appends alongside `s`, rather -// than on "any other query key present": a link wrapper or ad click can -// append tracking params (`utm_*`, `gclid`, etc.) to a pasted `/login?s=...` -// link, and that must not be mistaken for a relay return that posts a bogus -// callback to the relay. +// params (`code`, `state`, `error`) the relay appends to the static `/login` +// return URI, rather than on "any other query key present": a link wrapper or +// ad click can append tracking params (`utm_*`, `gclid`, etc.) to a pasted +// `/login?...` link, and that must not be mistaken for a relay return that +// posts a bogus callback to the relay. This no longer requires `s` alongside +// them, since the return URI is static and never carries it. export function isRelayReturn(search: string): boolean { const params = new URLSearchParams(search); - if (!params.get('s')) return false; return OAUTH_CALLBACK_PARAMS.some((key) => params.has(key)); } diff --git a/packages/polygon-agent-cli/src/lib/browser-login.test.ts b/packages/polygon-agent-cli/src/lib/browser-login.test.ts index 6149beb..b0d5c49 100644 --- a/packages/polygon-agent-cli/src/lib/browser-login.test.ts +++ b/packages/polygon-agent-cli/src/lib/browser-login.test.ts @@ -76,7 +76,7 @@ describe('runBrowserLogin', () => { expect(result).toEqual({ walletAddress: '0xW', loginMethod: 'google' }); expect(calls).toContain('announce:https://ui.test/login#sessionid12345678'); - expect(calls).toContain('startOidc:true:https://ui.test/login?s=sessionid12345678'); + expect(calls).toContain('startOidc:true:https://ui.test/login'); expect(calls).toContain('completeOidc:https://ui.test/login?s=sessionid12345678'); expect(statuses).toEqual([ { status: 'auth-url', url: 'https://accounts.google.com/auth' }, diff --git a/packages/polygon-agent-cli/src/lib/browser-login.ts b/packages/polygon-agent-cli/src/lib/browser-login.ts index 930f818..2e1bc6e 100644 --- a/packages/polygon-agent-cli/src/lib/browser-login.ts +++ b/packages/polygon-agent-cli/src/lib/browser-login.ts @@ -88,13 +88,17 @@ export async function runBrowserLogin( } if (action.type === 'google') { - // The relay's return page carries this pairing session (`s`) so it - // survives the OAuth round-trip; the OMS relay owns the Google - // callback and bounces the browser back to that page, which posts - // its full URL back to us as an `oidc-callback` action below. + // The OMS relay validates this return URI against the project's + // allowlist with an exact string match, so it must stay the bare, + // static `/login` (no query): a per-login `?s=` would never match a + // static registration. The pairing session instead survives the + // OAuth round trip via sessionStorage on the page itself, which the + // relay bounces back to with its own callback params appended; the + // page then posts the full return URL back to us as an + // `oidc-callback` action below. const { authorizationUrl } = await wallet.startOidcRedirectAuth({ provider: deps.oidcProviderGoogle, - omsRelayReturnUri: `${opts.uiBase}/login?s=${session}` + omsRelayReturnUri: `${opts.uiBase}/login` }); await relay.setStatus(session, { status: 'auth-url', url: authorizationUrl }); continue;