feat(agentex-ui): OIDC login with server-side access token - #351
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
bb896c7 to
c53787d
Compare
|
Addressed the Greptile P1s in
Rebased onto the updated base branch so the stack stays clean; the footer now renders Give Feedback → Account Picker → Log out (account picker placement lands in #350). |
6e749ef to
db8bec0
Compare
db8bec0 to
00d2195
Compare
8eb5b47 to
5f243a0
Compare
054fad9 to
5cd4b21
Compare
|
I went through this myself and it looks good. I also had Claude do a review pass focused on regressions and open-source (auth-disabled) safety, and it surfaced 6 items worth a look. Default path is clean — verified with a real A few things to address on the auth-enabled path, roughly by severity:
|
declan-scale
left a comment
There was a problem hiding this comment.
Forgot to hit approve, just double check that first claude point and I think we should be good
|
Thanks Declan — all 6 addressed in 1. 2. Transient refresh no longer terminal. The 3. Discovery validates before caching. 4. Stale 5. 6. auto-signin already-authenticated check. Cosmetic — |
## Summary First of a **2-PR stack** bringing account scoping + OIDC login to agentex-ui. This base PR routes the SDK through a same-origin BFF and adds an account switcher. Chart + deploy side lives in scaleapi/sgp#3961. - Routes the agentex SDK through a same-origin **BFF** (`/api/agentex`) instead of calling the API directly from the browser — the upstream URL and credentials never reach client JS. - Shared `applyBffCredentials` forwards `x-selected-account-id`, drops any client-sent `Authorization`, and strips the account-scoped `_jwt` cookie so SGP honors the **selected** account rather than the one you linked in with (identity stays via `_identityJwt`). The proxy also strips `Location` on upstream 3xx so internal redirect targets don't leak. - Account selection is driven by the `account_id` query param (no cookie), injected on every SDK request via a synchronous ref. Switching accounts resets the open task + selected agent to the account's home grid, and `resetQueries` drops the previous account's cached data so it never briefly renders the old agents. - `/api/user-info` fetches the caller's accounts; the picker bootstraps to the first when the param is missing/stale, shows a disabled/empty loading state (not a skeleton), renders single-account as static context, and lives in the sidebar footer (collapsed → icon-only). - Env-gated on the platform API being configured (`SGP_API_URL` / `NEXT_PUBLIC_SGP_APP_URL`); no-op otherwise. **Env:** `NEXT_PUBLIC_AGENTEX_API_BASE_URL` → server-only `AGENTEX_API_URL`. In this PR the BFF runs in default (cookie) mode — no auth dependency. The token-hiding Bearer path arrives in the stacked PR. ## Stack 1. **This PR** — account picker + same-origin BFF proxy → `main` 2. #351 — OIDC login with server-side access token → stacked on this branch 3. scaleapi/sgp#3961 — Helm chart + system-manager pack (deploy side) ## Test plan - [x] `npm run typecheck` / `npm run lint` — clean - [x] Runtime: switching accounts re-scopes agents/tasks (and SGP calls via the `_jwt` strip); single / multi / no-account render; loading state; no stale-account flash 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Add opt-in OIDC login on top of the BFF, keeping the access token out of the browser entirely. - NextAuth (generic OIDC provider) selected + enabled by a single env var, `AGENTEX_UI_AUTH_PROVIDER_ID`; disabled by default so non-auth deployments are unaffected (no SessionProvider mounts, no /api/auth/session calls). - The BFF attaches the access token as a Bearer server-side via `getToken` and drops the UI session cookie — the token never reaches client JS or the NextAuth session (which exposes only `error`). - Middleware auto-redirects unauthenticated requests to sign-in; a client guard re-auths on refresh failure; RP-initiated logout clears the IdP session. - Supports `client_secret_post` (dev) and `private_key_jwt` (prod). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address Greptile P1s. The refresh path and RP-initiated logout hardcoded Ory's `/oauth2/token` and `/oauth2/sessions/logout` paths, which 404 on non-Ory IdPs — silently forcing a re-auth loop (refresh) or a transparent re-login (logout). Resolve `token_endpoint` and `end_session_endpoint` from the issuer's `.well-known/openid-configuration` (cached per-process; failures not cached). No Ory-specific fallback: a missing token_endpoint fails the refresh cleanly (→ re-auth), and a missing end_session_endpoint means local-only logout rather than a guessed path. OneAuth (Ory Hydra) advertises both in discovery, and NextAuth already relies on the same document for sign-in. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A GET /api/auth/logout is reachable by a crafted cross-site navigation (SameSite=Lax sends the session cookie on top-level GET), letting an attacker end the user's IdP SSO session. Switch to POST (Lax cookies aren't sent on cross-site POST); the client POSTs and follows the returned RP-initiated logout URL. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t mode
Trim verbose/redundant comments across auth.ts, middleware, session-guard, layout and the
auth routes; drop provider-specific ("Ory") wording (endpoints come from discovery); rename
the non-auth "legacy mode" to "default mode". No behavior change.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- getToken passes secureCookie (from AUTH_URL) so the BFF finds the `__Secure-` session cookie over HTTPS; without it the Bearer was dropped (401s) and logout skipped end-session. - Refresh: only invalid_grant is terminal; transient failures keep the token and retry. - OIDC discovery: don't cache a 200 that's missing token_endpoint. - private_key_jwt: add `kid` to the login client_assertion (parity with refresh). - auto-signin: short-circuit when already authenticated; sanitize the redirect target. - /api/auth/*: 404 in default (no-auth) mode instead of the provider-less handler's 500. - SDK fetch: on a 401, refresh the session (deduped) and retry once. - Expose the non-secret `sub` claim on the session (access/id/refresh tokens stay server-side). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…invalid_grant Any 4xx (except 429) means the grant/client/request is bad and won't recover by retrying → terminal (force re-auth); 429/5xx and network errors stay transient (keep the token, retry next cycle). Broadens the terminal set beyond invalid_grant so a persistent invalid_client (e.g. rotated secret) surfaces a real error instead of silently forwarding an expired token, while still not logging users out on a transient IdP blip. Drops the now-redundant response-body parse. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
56998b0 to
4f6c53a
Compare
…t-switch races (#355) ## Problem Switching accounts intermittently corrupted the URL state: `task_id` would survive the switch (leaving a stuck "phantom task" under the new account), `agent_name` would clear, and sometimes `account_id` even reverted to the old value **while the sidebar already showed the new account's tasks**. ## Root cause `updateParams` rebuilt the entire query string from the `useSearchParams()` **snapshot**, which lags a render behind `router.push`. An account switch fires **two** navigations from two components: - the provider's `setSelectedAccountId` clears `task_id` - the agent-validation effect in `agentex-ui-root` clears `agent_name` Whichever call read the stale snapshot and landed **last** clobbered the other's change. The data layer switches synchronously (`selectedAccountIdRef`), so tasks refetched for the new account even when the URL reverted — the smoking-gun symptom. It was intermittent because it depended on whether `useSearchParams` had propagated between the two writes. ## Fix - **`use-safe-search-params.ts`** — `updateParams` now merges each update against the **live URL** (`window.location.search`), which is always current, so concurrent writes compose instead of overwrite. This is the systemic fix and addresses all three symptoms. - **`agentex-provider.tsx`** — an explicit account switch now clears `task_id` **and** `agent_name` in one atomic navigation (no dependence on a downstream effect, no flash of stale params). - **`use-safe-search-params.test.tsx`** — regression test proving a write built from a stale snapshot no longer resurrects `task_id`/`account_id`. ## Test plan - `npm run typecheck` ✓ - `npx vitest run` — 50/50 ✓ - `npm run lint` ✓ Independent of #351 (OIDC); can merge in any order. <!-- greptile_comment --> <h3>Greptile Summary</h3> This PR fixes an intermittent URL-state corruption that occurred during account switches. The root cause was `updateParams` building its query string from the `useSearchParams()` snapshot, which lags behind `router.push()` calls — so two rapid navigations from different components would race and the last writer would clobber the first's changes. - **`use-safe-search-params.ts`**: `updateParams` now reads `window.location.search` (always current, updated synchronously by the History API) instead of the React snapshot, so concurrent writes compose rather than overwrite. Also fixes a minor trailing-`?` edge case for empty param sets. - **`agentex-provider.tsx`**: The explicit account-switch now clears both `task_id` and `agent_name` atomically in one navigation, eliminating the need for a downstream effect to clean up `agent_name`. - **`use-safe-search-params.test.tsx`**: Regression tests proving the stale-snapshot clobber is gone, covering set/delete/preserve semantics and `replace` vs `push` routing. <details><summary><h3>Confidence Score: 5/5</h3></summary> Safe to merge — the fix is narrowly scoped to URL merge behavior and is directly validated by new regression tests. The change replaces a React snapshot read with a synchronous window.location.search read inside updateParams, which is always safe in browser environments and has a correct SSR fallback. The provider change and the new tests are consistent with the fix, and no existing behavior outside the race window is altered. No files require special attention. </details> <details><summary><h3>Important Files Changed</h3></summary> | Filename | Overview | |----------|----------| | agentex-ui/hooks/use-safe-search-params.ts | Core fix: reads from `window.location.search` instead of the stale `useSearchParams()` snapshot so concurrent writes compose correctly. Also adds a clean fallback for SSR and fixes the trailing-`?` edge case for empty param sets. | | agentex-ui/components/providers/agentex-provider.tsx | Adds `agent_name: null` alongside `task_id: null` in the explicit-switch branch, so both account-scoped params are cleared atomically in one navigation rather than relying on a downstream effect. | | agentex-ui/hooks/use-safe-search-params.test.tsx | New regression test suite with three cases: stale-snapshot clobber prevention, combined set/delete/preserve in one update, and `replace` vs `push` routing. Correctly uses `vi.hoisted` to share mock state with the factory closure. | </details> <details><summary><h3>Sequence Diagram</h3></summary> <a href="#gh-light-mode-only"> ```mermaid %%{init: {'theme': 'neutral'}}%% sequenceDiagram participant P as AgentexProvider participant E as agentex-ui-root effect participant H as useSafeSearchParams participant W as window.location participant R as Next.js Router Note over P,R: Before fix - stale snapshot clobber P->>H: "updateParams({account_id:new, task_id:null})" H->>W: reads stale snapshot (account_id:old, task_id:T) H->>R: "push(?account_id=new&task_id=null)" R->>W: window.location.search updated Note over E: snapshot not yet updated E->>H: "updateParams({agent_name:null})" H->>W: reads stale snapshot (account_id:old, task_id:T) H->>R: "push(?account_id=old&task_id=T) clobbers first write" Note over P,R: After fix - live URL merge P->>H: "updateParams({account_id:new, task_id:null, agent_name:null})" H->>W: reads window.location.search (live) H->>R: "push(?account_id=new) atomic, one navigation" R->>W: window.location.search updated Note over E: effect fires but agent_name already cleared ``` </a> <a href="#gh-dark-mode-only"> ```mermaid %%{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 P as AgentexProvider participant E as agentex-ui-root effect participant H as useSafeSearchParams participant W as window.location participant R as Next.js Router Note over P,R: Before fix - stale snapshot clobber P->>H: "updateParams({account_id:new, task_id:null})" H->>W: reads stale snapshot (account_id:old, task_id:T) H->>R: "push(?account_id=new&task_id=null)" R->>W: window.location.search updated Note over E: snapshot not yet updated E->>H: "updateParams({agent_name:null})" H->>W: reads stale snapshot (account_id:old, task_id:T) H->>R: "push(?account_id=old&task_id=T) clobbers first write" Note over P,R: After fix - live URL merge P->>H: "updateParams({account_id:new, task_id:null, agent_name:null})" H->>W: reads window.location.search (live) H->>R: "push(?account_id=new) atomic, one navigation" R->>W: window.location.search updated Note over E: effect fires but agent_name already cleared ``` </a> </details> <sub>Reviews (2): Last reviewed commit: ["fix(agentex-ui): merge URL updates again..."](dc3529b) | [Re-trigger Greptile](https://app.greptile.com/api/retrigger?id=43214802)</sub> <!-- /greptile_comment --> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Second of a 2-PR stack. Stacked on #350 — base is
feat/agentex-ui-account-picker, so this diff is auth-only (GitHub auto-retargets tomainonce #350 merges). The chart + deploy wiring lives in scaleapi/sgp#3961.Adds opt-in OIDC login on top of the BFF, keeping the access token out of the browser entirely.
AGENTEX_UI_AUTH_PROVIDER_ID; disabled by default so non-auth deployments are unaffected (noSessionProvider, no/api/auth/sessioncalls).Bearerserver-side viagetTokenand drops the UI session cookie — the token never reaches client JS or the NextAuth session (which exposes onlyerror).client_secret_post(dev) andprivate_key_jwt(prod).Why token-hidden (BFF) over exposing it on the session
Exposing the access token to client JS is contrary to the OAuth 2.0 for Browser-Based Apps BCP and would flag in security review. Keeping it server-side (the BFF attaches the Bearer) is the recommended pattern; the client only ever sees the same-origin proxy.
Stack
AGENTEX_UI_AUTH_PROVIDER_ID,OIDC_ISSUER_URL, the client Secret viaenvFrom)Test plan
npm run typecheck/npm run lint— clean🤖 Generated with Claude Code
Greptile Summary
Adds opt-in OIDC login to agentex-ui via NextAuth v5, keeping the access token entirely server-side (BFF pattern): the middleware redirects unauthenticated requests to a server-side auto-signin handler, the BFF attaches the Bearer from the encrypted session JWT, and a POST-only RP-initiated logout endpoint ends the IdP session.
auth.ts— generic OIDC provider built from env vars; token/end-session endpoints resolved from OIDC discovery (cached per-process, failures not cached for retry); customprivate_key_jwtsigner for prod andclient_secret_postfor dev; refresh logic correctly treats 4xx (excluding 429) as terminal and 429/5xx/network as transient.bff.ts— in auth mode, strips all cookies and attachesBearer <accessToken>from the server-side session JWT; falls back to cookie forwarding in non-auth mode; the entire feature is gated onAGENTEX_UI_AUTH_PROVIDER_IDso non-auth deployments are unaffected.agentex-provider.tsx/session-guard.tsx— client-side 401 handling retries once after a deduplicated session refresh;SessionGuardtriggers re-auth onRefreshAccessTokenErrorfor sessions that are purely making API calls without navigation.Confidence Score: 5/5
Safe to merge — the BFF pattern is correctly implemented, the access token never reaches client JS, and all auth edge cases are handled correctly.
All the previously flagged issues have been addressed in the current diff. The discovery-based endpoint resolution caches correctly, the 4xx/429/5xx split on refresh errors is sound, and the middleware correctly gates only non-API routes. No new logic bugs were found.
No files require special attention; auth.ts is the most complex file but its token refresh and OIDC discovery logic are well-guarded.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant B as Browser participant MW as Middleware participant AS as auto-signin participant NA as NextAuth participant BFF as BFF participant UP as Upstream API participant IDP as IdP (OIDC) B->>MW: GET / MW->>MW: auth() no session MW-->>B: redirect /api/auth/auto-signin B->>AS: GET /api/auth/auto-signin AS->>IDP: signIn (PKCE + state) IDP-->>B: redirect /api/auth/callback B->>NA: GET /api/auth/callback NA->>IDP: exchange code for tokens NA->>NA: jwt callback stores tokens in encrypted cookie NA-->>B: redirect / (session cookie set) B->>BFF: fetch /api/agentex BFF->>BFF: getSessionToken extracts accessToken BFF->>UP: GET with Authorization Bearer token UP-->>B: 200 Note over B,NA: Token refresh (refetchInterval=240s) B->>NA: GET /api/auth/session NA->>IDP: POST token_endpoint refresh_token IDP-->>NA: new access_token NA-->>B: Set-Cookie updated session Note over B,BFF: Logout B->>BFF: POST /api/auth/logout BFF->>BFF: signOut + discover end_session_endpoint BFF-->>B: url to IdP end_session B->>IDP: navigate to end_session IDP-->>B: redirect to origin%%{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 B as Browser participant MW as Middleware participant AS as auto-signin participant NA as NextAuth participant BFF as BFF participant UP as Upstream API participant IDP as IdP (OIDC) B->>MW: GET / MW->>MW: auth() no session MW-->>B: redirect /api/auth/auto-signin B->>AS: GET /api/auth/auto-signin AS->>IDP: signIn (PKCE + state) IDP-->>B: redirect /api/auth/callback B->>NA: GET /api/auth/callback NA->>IDP: exchange code for tokens NA->>NA: jwt callback stores tokens in encrypted cookie NA-->>B: redirect / (session cookie set) B->>BFF: fetch /api/agentex BFF->>BFF: getSessionToken extracts accessToken BFF->>UP: GET with Authorization Bearer token UP-->>B: 200 Note over B,NA: Token refresh (refetchInterval=240s) B->>NA: GET /api/auth/session NA->>IDP: POST token_endpoint refresh_token IDP-->>NA: new access_token NA-->>B: Set-Cookie updated session Note over B,BFF: Logout B->>BFF: POST /api/auth/logout BFF->>BFF: signOut + discover end_session_endpoint BFF-->>B: url to IdP end_session B->>IDP: navigate to end_session IDP-->>B: redirect to originReviews (11): Last reviewed commit: "fix(agentex-ui): escalate refresh failur..." | Re-trigger Greptile