Fix infinite sign-in redirect loop under base-path deploys#1692
Fix infinite sign-in redirect loop under base-path deploys#1692aryeh-stark wants to merge 3 commits into
Conversation
RequireSession had no guard against redirecting to the sign-in page from the sign-in page. When the app shell is served at the sign-in path (e.g. a base-path deploy where the SPA answers /<app>/_agent-native/sign-in with no client- resolvable session), it redirected sign-in -> sign-in, nesting and re-encoding the current URL as a fresh ?return= on every hop (the runaway .../sign-in?return=%252F...sign-in URL). - RequireSession: new isOnSignInPage() guard — never redirect to sign-in when already on it. - safeReturnPath: collapse any return that resolves back to .../_agent-native/sign-in to "/" (server-side defense in depth). Adds tests for both paths and a changeset.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Visual recap — skippedThe visual recap job did not run for this pull request. This is informational only and does not block the PR. Recap skipped for |
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
There was a problem hiding this comment.
Builder reviewed your changes and found 1 potential issue 🔴
Review Details
This PR fixes an auth redirect loop in two layers: RequireSession now avoids redirecting when the browser is already on the framework sign-in entrypoint, and safeReturnPath() collapses sign-in self-returns to prevent post-auth re-entry into the same loop. The overall approach is sound and the added tests are targeted and useful. I classified this as high risk because it changes authentication/session redirect behavior in shared core code that affects mounted/base-path deployments.
Key finding:
- 🔴 HIGH — the new server-side collapse-to-
"/"behavior loses the app base path in the/_agent-native/sign-insuccess redirect, so base-path deployments can send authenticated users to the host root instead of the mounted app.
Good patterns here: the fix adds defense in depth across client and server, preserves normal return-path sanitization, and documents the production failure mode clearly in tests/changelog. However, the remaining base-path regression means the auth flow is not yet safe to merge as-is.
🧪 Browser testing: Will run after this review (PR touches UI code)
| // Never return to the sign-in entry point itself. A `return` that resolves | ||
| // back to `…/_agent-native/sign-in` re-enters the redirect loop — each hop | ||
| // nests the prior sign-in URL as a fresh `?return=`. Collapse it to "/" so | ||
| // the post-sign-in 302 lands on the app, not another sign-in page. Matches | ||
| // with or without an app base-path prefix (`/<app>/_agent-native/sign-in`). | ||
| if (parsed.pathname.endsWith("/_agent-native/sign-in")) return "/"; |
There was a problem hiding this comment.
🔴 Loop guard drops base-path deployments onto site root
When safeReturnPath() collapses a looping sign-in return to /, the /_agent-native/sign-in handler later redirects to that raw path instead of remapping it to getAppBasePath() || "/". On base-path deploys, a successful sign-in can therefore land on the host root (or 404) instead of the mounted app.
Additional Info
Found by 2/4 review passes; validated against auth.ts lines 1798-1808 (`Location: safeReturn`) and 1819-1827 (existing base-path-aware `/login`/`/signup` handling).
|
thanks @aryeh-stark ! one comment above looks useful to fix |
OAuth client credentials (GOOGLE_CLIENT_ID/SECRET, GOOGLE_SIGN_IN_CLIENT_*, GITHUB_CLIENT_*) are deployment-wide identity, not per-tenant secrets: every user authorizes the same app for their own account and gets their own tokens. But resolveSecret refused to fall back to the deploy env for signed-in users in a hosted runtime, so a deployment that set GOOGLE_CLIENT_* still showed each logged-in user the "bring your own OAuth app" setup wizard and /google/auth-url returned missing_credentials. Let these keys use the deploy env fallback (same as Builder credential keys); a per-tenant BYO client still wins (resolved first). Also fix GoogleConnectBanner computing "configured" across every env key (Slack/Resend/LLMs) instead of just the Google client keys.
There was a problem hiding this comment.
Builder reviewed your changes and found 1 potential issue 🟡
Review Details
This incremental update expands the PR beyond the original redirect-loop fix: it now also changes hosted secret resolution for OAuth client credentials and adjusts Mail’s Google connect readiness UI. I reviewed the updated auth/session, credential-resolution, and Mail UI paths at high risk because they affect shared authentication behavior, Google OAuth configuration, and cross-tenant secret handling in core code.
Current state: the previously reported blocking auth issue in packages/core/src/server/auth.ts still appears unresolved and remains visible on the PR, so the branch is still not ready to merge. In the new changes, I found one additional substantive regression in OAuth credential resolution and one lower-severity Mail UI state issue.
Key findings:
- 🔴 HIGH (existing unresolved) — base-path sign-in loop guard still redirects authenticated users to host root.
- 🟡 MEDIUM (new) — deploy-env fallback now resolves OAuth client keys independently, which can mix tenant and deploy credentials into an invalid pair.
- ⚪ LOW (new) — Mail’s banner still uses all env keys for most readiness UI, so unrelated optional secrets can keep Google setup looking incomplete.
🧪 Browser testing: Will run after this review (PR touches UI code)
| const allowDeployFallback = isBuilderCredentialKey(key) | ||
| ? canUseBuilderDeployCredentialFallbackForRequest() | ||
| : isDeploymentWideOAuthClientKey(key) | ||
| ? // A shared OAuth client is deployment-wide identity, not a per-tenant | ||
| // secret — safe to use for authenticated users (BYO per-tenant client | ||
| // is still resolved first, above). See DEPLOYMENT_WIDE_OAUTH_CLIENT_KEYS. | ||
| true | ||
| : canUseDeployCredentialFallbackForRequest(); | ||
| const envFallback = allowDeployFallback ? process.env[key] || null : null; |
There was a problem hiding this comment.
🟡 OAuth client env fallback can silently mix tenant and deploy credentials
resolveSecret() now allows deploy-level fallback for Google OAuth client keys independently, so a tenant-scoped GOOGLE_CLIENT_ID can be paired with a deploy-scoped GOOGLE_CLIENT_SECRET (or vice versa). Mail and Calendar resolve those keys separately, which turns partial overrides into broken invalid_client/unauthorized_client OAuth flows instead of a clean missing-credentials state.
Additional Info
Found by 1/3 review passes; validated against templates/mail/server/lib/google-auth.ts:59-62 and templates/calendar/server/handlers/google-auth.ts:48-53, both of which resolve the pair via separate resolveSecret calls.
defaultOrgAppLinks hardcoded each template's *.agent-native.com prod URL, so a path-prefixed deployment (served at <origin>/<app>/, e.g. apps.example.com/mail/) linked every sibling app to the first-party hosted site instead of the current deployment. Build <origin>/<app>/ links when the client is path-prefixed (VITE_APP_BASE_PATH baked at build); keep the prod URLs for the subdomain layout.
There was a problem hiding this comment.
Builder reviewed your changes and found 1 potential issue 🟡
Review Details
This incremental update adds a path-prefixed org-switcher link change in workspace-app-links.ts on top of the earlier auth redirect-loop and OAuth credential fallback work, and it also attempts to tighten Mail’s Google setup readiness logic. I reviewed the updated branch at high risk because it still touches shared authentication flow, hosted credential resolution, and cross-app navigation/UI state in shared core code.
Current status: the two previously reported issues are still present and I left those existing comments in place rather than reposting them. The auth-side base-path redirect bug in packages/core/src/server/auth.ts remains the blocking item. I also found one additional UI-state regression in the new Mail banner logic.
Key findings:
- 🔴 HIGH (existing unresolved) — authenticated
/_agent-native/sign-inrequests can still redirect base-path deployments to/instead of the mounted app. - 🟡 MEDIUM (existing unresolved) — OAuth client env fallback can still mix tenant and deploy credentials.
- 🟡 MEDIUM (new) — Mail’s Google setup flow now uses a Google-only readiness check in
fetchStatus(), but most render branches still key offenvStatus.every(...), so unrelated optional secrets can keep the banner in the “not configured” state.
Good changes: the org-switcher path-prefix tests are targeted, and the sign-in loop guard / Google-credential helper remain easy to follow. However, with the unresolved auth bug and the new Mail UI inconsistency, this branch is not ready to merge.
🧪 Browser testing: Attempted, but the browser executor still lacks Chrome automation tools in this environment; I could only gather server-side evidence, not visual confirmation.
| setEnvStatus(data); | ||
| const allConfigured = data.every((k) => k.configured); | ||
| if (allConfigured && data.length > 0) { | ||
| if (googleCredsConfigured(data)) { |
There was a problem hiding this comment.
🟡 Google setup UI still depends on unrelated env keys
fetchStatus() now treats only the Google client keys as the readiness signal, but the rest of the component still branches on envStatus.every((k) => k.configured). That means missing optional keys like Slack/Anthropic/A2A can still leave the banner in the “not configured” state even when Google is ready.
Additional Info
Validated manually against GoogleConnectBanner.tsx: `googleCredsConfigured(data)` is used at line 199, while render state still uses `allConfigured = envStatus.length > 0 && envStatus.every(...)` for banner copy, CTA, and wizard visibility.
2 participants
Problem
RequireSessionhas no guard against redirecting to the sign-in page from the sign-in page. When the authenticated app shell (wrapped inRequireSession) is served at the sign-in path — e.g. a base-path deploy where the SPA answers/<app>/_agent-native/sign-inwith no client-resolvable session — it redirects sign-in → sign-in, nesting and re-encoding the current URL as a fresh?return=on every hop:…growing without bound until the browser/server gives up. Hit in production behind a
/<app>path prefix once a session cookie existed but the client session check couldn't resolve it.Fix
RequireSession: new exportedisOnSignInPage()helper; the gate never redirects whenwindow.location.pathnamealready equalsagentNativePath("/_agent-native/sign-in")(matches with or without an app base path). Falls through to the loading fallback instead.safeReturnPath(server): collapse anyreturnthat resolves back to…/_agent-native/sign-into/, so the post-sign-in 302 can't re-enter the loop either (defense in depth).Tests
require-session.spec.tsx: new case — never redirects when already on the sign-in page.auth.spec.ts:safeReturnPathcollapses sign-in returns (root + base-path + nested-encoded), leaves normal app paths intact.Includes a changeset (patch,
@agent-native/core).