fix(auth): surface OAuth errors on /auth/callback instead of silent timeout - #1258
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe OAuth callback page now parses OAuth errors from URL search and hash params (hash wins) and immediately renders a sanitized fatal-error UI when present; otherwise it exchanges or waits for a Supabase session (10s unsubscribe timeout) and redirects on sign-in. Stable DOM IDs and spinner/error UI were added. BLOCKING: no stubs/TODOs detected. ChangesAuth callback + E2E coverage
Sequence DiagramsequenceDiagram
participant Browser as Browser (page)
participant Callback as CallbackScript
participant Supabase as Supabase SDK
participant Redirect as Redirect Target
Browser->>Callback: load /auth/callback (with search/hash)
Callback->>Callback: parseAuthError()
alt URL-embedded error present
Callback-->>Browser: render fatal error UI (showFatalError)
else no URL error
Callback->>Supabase: supabase.auth.getSession()
alt session returned
Callback->>Redirect: location.replace(redirect)
else no session
Callback->>Supabase: onAuthStateChange listen for SIGNED_IN
Supabase->>Callback: auth state = SIGNED_IN
Callback->>Redirect: location.replace(redirect)
Note right of Callback: 10s timeout -> unsubscribe, console.error, show fatal timeout UI
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
3 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@website/src/pages/auth/callback.astro`:
- Around line 157-177: The setTimeout started for the 10s fallback isn’t cleared
when the SIGNED_IN handler in supabase.auth.onAuthStateChange runs, so
showFatalError can still fire after navigation; store the timeout ID (e.g.,
const timeoutId = setTimeout(...)) and call clearTimeout(timeoutId) inside the
SIGNED_IN branch (before subscription.unsubscribe() and before setting
window.location.href) to prevent the error callback from running after a
successful sign-in.
- Around line 111-116: The code currently calls decodeURIComponent directly when
building detailLines (symbol: detailLines) which can throw on malformed
percent-encodings and also uses the raw urlError.errorDescription later for hint
checks; create a small safe decode helper (e.g., tryDecodeURIComponent) that
tries decodeURIComponent(errorDescription) in a try/catch and on failure returns
the original raw string (after replacing '+' with space) or a safe placeholder,
then compute a decodedDescription variable from urlError.errorDescription using
that helper and use decodedDescription both in the detailLines array (replace
the inline decode) and in the later hint check (the check currently at line
~129) so the UI won't crash and the hint triggers consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: efa78051-d738-4ac1-874c-4fa3d84c2dd6
📒 Files selected for processing (1)
website/src/pages/auth/callback.astro
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the website’s OAuth callback page so authentication failures from Supabase/GitHub are surfaced directly in the UI instead of degrading into a generic timeout. It fits into the website auth flow by improving diagnostics on the /auth/callback handoff between the external provider and the existing Supabase session logic.
Changes:
- Adds parsing of OAuth failures from both query-string and hash parameters on
/auth/callback. - Replaces the old generic error rendering with a structured failure state, including raw provider details and tailored hints for common OAuth misconfiguration cases.
- Keeps the existing session-based success redirect flow and timeout fallback, while adding logging for each failure path.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…imeout oauth providers can fail at three different layers, each surfacing the error in a different place: 1. pkce / code-flow error → url search params: ?error=... 2. implicit-flow error → url hash params: #error=... 3. session-exchange error → returned from getsession() the previous version of /auth/callback only checked (3), so any error routed through (1) or (2) — which is what every supabase-side provider config failure produces — landed on a 10-second silent timeout that printed only "Authentication timed out". that made the github oauth bug (#1257) effectively undebuggable from the client: a customer who clicks Continue with GitHub and ends up back here with ?error=server_error&error_description=... in the url would just see a generic timeout message and have no idea what actually failed. now we read all three layers, surface whichever fires first, render the raw error_description in a monospace block for support, and include a best-effort hint for the most common failure modes (server_error, access_denied, unsupported_provider). regression-safe: if no error is in the url and the session materializes normally, behavior is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ooples
force-pushed
the
fix/oauth-callback-error-visibility
branch
from
May 4, 2026 18:16
5eb64a1 to
db12c58
Compare
|
Deployment failed with the following error: Learn More: https://vercel.com/franklins-projects-02a0b5a0?upgradeToPro=build-rate-limit |
#1+#3 (coderabbitai+copilot, callback.astro:114): URLSearchParams.get() already percent-decodes its return value AND maps '+' to space, so the prior `decodeURIComponent(...).replace(/\+/g, ' ')` was double-decoding. That threw URIError on legitimate descriptions containing literal '%' (e.g. an IdP message with '%25' that became '%' after the first decode and then crashed the second), dropping the user into the generic "An unexpected error occurred" branch and burying the real OAuth failure. Removed the second decode and the '+' replacement entirely; error_description now renders as the IdP sent it. #2 (coderabbitai, callback.astro:177): the 10s setTimeout was never cleared on SIGNED_IN — a near-deadline success would still fire showFatalError after the redirect started, briefly flashing "did not complete in time" before navigation. Captured the timeout id and clearTimeout() it inside the SIGNED_IN handler. #4 (copilot, callback.astro:129): the disabled-provider hint only checked errorCode === 'unsupported_provider', missing callbacks like ?error=unsupported_provider&error_description=... where Supabase puts the marker on `error` instead. Added a parallel check on the `error` field so the actionable hint fires either way. #5 (copilot): no Playwright e2e for the new branches. Added tests/e2e/auth/callback.spec.ts (5 specs, listed cleanly) covering: - search-param server_error → provider-misconfig hint - search-param access_denied → user-cancelled hint - error=unsupported_provider on `error` (not error_code) → disabled hint (pins #4) - hash-param error precedence over search-param error - error_description with literal '%' renders intact (pins #1+#3) playwright.config.ts: new auth-anon project that runs auth/*.spec.ts without storageState (the existing auth/*.setup.ts files are excluded via testIgnore). Keeps the unauthenticated error-path specs separate from the user.setup / admin.setup login fixtures. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@website/src/pages/auth/callback.astro`:
- Around line 65-76: The current pick() merges hash and search per-key causing
mixed-layer auth errors; change logic so once an error is present in the hash
fragment all error fields (error, error_code, error_description) are read
exclusively from hash, otherwise read them from search. Concretely, detect const
error = hash.get('error') ?? search.get('error'); then if hash.has('error') use
hash.get(...) for errorCode and errorDescription (and for error), else use
search.get(...); update uses of pick, hash, and search and the returned object
(error, errorCode, errorDescription) accordingly so fields come from the same
layer.
- Around line 188-198: The timeout fallback branch for the 10s window inside the
callback handler does not log an error, leaving the third-party-cookie / missing
SIGNED_IN path invisible; update the timeout handler (the setTimeout callback
that calls subscription.unsubscribe() and showFatalError) to also call
console.error() with a descriptive message and any relevant state (e.g., that
getSession() did not emit SIGNED_IN within 10s and suspecting third-party cookie
blocking), so developers can see the failure in devtools when this branch
executes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cd3bbd5a-2be8-482d-9513-5c39a0064bb2
📒 Files selected for processing (3)
website/playwright.config.tswebsite/src/pages/auth/callback.astrowebsite/tests/e2e/auth/callback.spec.ts
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address the reviewer's accessibility, observability, and error-rendering concerns on /auth/callback: - Error container now has `role="alert"`, `aria-live="assertive"`, `aria-atomic="true"` so screen readers announce the dynamically- injected error message. Without these the hidden div's content flipping from empty to populated was silent for AT users. - The 10-second timeout fallback now console.errors before showing the fatal-error UI, matching the PR description's claim that every failure path leaves a stack trace in devtools. Previously the timeout was the one exception. - The `catch (err)` branch no longer renders `String(err)`, which prints `[object Object]` for non-Error throws (some Supabase SDK paths reject with plain objects). Render `err.message` for Error instances, fall back to `JSON.stringify` for plain objects, and fall back further to `String(err)` only as a last resort. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@website/src/pages/auth/callback.astro`:
- Around line 191-197: The redirect query param is used directly in two places
(params.get('redirect') → window.location.href in the immediate-session branch
and inside supabase.auth.onAuthStateChange handler), causing an open-redirect;
replace direct assignment with a validation step that clamps the redirect to
same-origin paths only (use or add a helper like sanitizeRedirect/clampRedirect
that returns a safe path or fallback to `${base}account/`), and call that helper
in both locations before assigning window.location.href (keep existing
timeoutId/ subscription cleanup logic intact).
- Around line 224-229: The JSON.stringify fallback can return undefined for
undefined, functions, or symbols which leaves detail undefined and causes
safe(detail).replace(...) to throw; update the error handling in the callback to
check the result of JSON.stringify(err) and if it is undefined, null, or an
empty string, set detail to a stable string (e.g., String(err) or a fixed
placeholder like "<unserializable value>"); specifically change the branch that
assigns detail after the try/catch so that the variable detail (used later with
safe(detail).replace(...)) is always a defined string even when JSON.stringify
returns undefined.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b8c4bee9-2798-475d-9213-758968b7e965
📒 Files selected for processing (1)
website/src/pages/auth/callback.astro
Address the per-field-merge concern + missing-failure-mode coverage:
callback.astro parseAuthError():
- Switch from per-FIELD precedence (`hash.get(key) ?? search.get(key)`)
to per-SOURCE precedence: if the hash carries any `error`, use ALL
hash fields together; otherwise use ALL search fields together. The
previous merge could splice an `error` from one source with an
`error_description` from the OTHER, producing a synthetic error that
didn't actually arrive from any single OAuth callback layer. Hash =
implicit-flow returns; search = code-flow returns; they're different
protocol layers and their fields shouldn't mix.
callback.spec.ts:
- New `per-source precedence` test: hash with `error` only + search
with `error_description` must NOT splice the search description into
the hash error.
- New `non-URL failure paths` describe block with two specs:
* getSession() error path — routes the supabase module to a shim
that returns an error tuple, asserts "Session exchange failed."
is rendered AND the failure was console.error'd.
* 10-second timeout path — uses page.clock.fastForward(10_500) to
skip the wall-clock wait, asserts the timeout fatal-error UI
surfaces AND the documented console.error trace fired (PR #1258
promised every failure path would log; the timeout path was the
one previously-uncovered branch).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
website/src/pages/auth/callback.astro (2)
241-243:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize the
JSON.stringifyfallback to a definite string.This branch can still leave
detailnon-string.JSON.stringify()returnsundefinedfor top-level pure values likeundefined, functions, and symbols, soshowFatalError(..., detail)can fail again whensafe(details)calls.replace(...). (developer.mozilla.org)Proposed fix
} else { - try { detail = JSON.stringify(err); } + try { + detail = JSON.stringify(err) ?? String(err); + } catch { detail = String(err); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@website/src/pages/auth/callback.astro` around lines 241 - 243, The fallback for serializing the error in the else branch can leave detail non-string (JSON.stringify may return undefined); update the block that sets detail (the try/catch around JSON.stringify(err)) so that after attempting JSON.stringify you coerce undefined/non-string results into a definite string (e.g., use the JSON result if truthy else String(err) or a fixed fallback like "<unserializable error>") before passing to showFatalError and safe(detail).
205-211:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winClamp
redirectto a same-origin path before navigating.Line 210 still assigns
params.get('redirect')directly intowindow.location.href. A crafted query value can bounce the user off-origin after login, and the same unresolved sink still exists in the existing-session branch on Line 191. This needs a shared redirect sanitizer before either assignment.Proposed fix
+ function getSafeRedirect(raw: string | null, fallback: string): string { + if (!raw) return fallback; + try { + const url = new URL(raw, window.location.origin); + if (url.origin !== window.location.origin) return fallback; + return `${url.pathname}${url.search}${url.hash}`; + } catch { + return fallback; + } + } + async function handleCallback() { @@ if (data.session) { const params = new URLSearchParams(window.location.search); - const redirectTo = params.get('redirect') || `${base}account/`; + const redirectTo = getSafeRedirect(params.get('redirect'), `${base}account/`); window.location.href = redirectTo; return; } @@ const { data: { subscription } } = supabase.auth.onAuthStateChange((event, session) => { if (event === 'SIGNED_IN' && session) { subscription.unsubscribe(); if (timeoutId !== undefined) clearTimeout(timeoutId); const params = new URLSearchParams(window.location.search); - const redirectTo = params.get('redirect') || `${base}account/`; + const redirectTo = getSafeRedirect(params.get('redirect'), `${base}account/`); window.location.href = redirectTo; } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@website/src/pages/auth/callback.astro` around lines 205 - 211, The redirect value obtained from params.get('redirect') is used directly in window.location.href in the supabase.auth.onAuthStateChange handler (inside the SIGNED_IN branch) and must be sanitized to a same-origin path first; implement a shared sanitizer function (e.g., sanitizeRedirectPath) and call it from both the onAuthStateChange handler and the existing-session branch before assigning window.location.href, ensuring it rejects or normalizes external origins, strips protocol/host, enforces a leading slash, and falls back to `${base}account/` when invalid.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@website/src/pages/auth/callback.astro`:
- Around line 205-208: The auth state change handler currently only checks for
event === 'SIGNED_IN', causing a race where an 'INITIAL_SESSION' is ignored;
update the onAuthStateChange handler (the subscription created via
supabase.auth.onAuthStateChange referenced as subscription) to treat both
'SIGNED_IN' and 'INITIAL_SESSION' as successful session events (i.e., if (event
=== 'SIGNED_IN' || event === 'INITIAL_SESSION') { ... }), keep the same logic to
unsubscribe the subscription and clear timeoutId, and ensure getSession() usage
remains unchanged so valid sessions loaded from storage won't trigger the
timeout.
---
Duplicate comments:
In `@website/src/pages/auth/callback.astro`:
- Around line 241-243: The fallback for serializing the error in the else branch
can leave detail non-string (JSON.stringify may return undefined); update the
block that sets detail (the try/catch around JSON.stringify(err)) so that after
attempting JSON.stringify you coerce undefined/non-string results into a
definite string (e.g., use the JSON result if truthy else String(err) or a fixed
fallback like "<unserializable error>") before passing to showFatalError and
safe(detail).
- Around line 205-211: The redirect value obtained from params.get('redirect')
is used directly in window.location.href in the supabase.auth.onAuthStateChange
handler (inside the SIGNED_IN branch) and must be sanitized to a same-origin
path first; implement a shared sanitizer function (e.g., sanitizeRedirectPath)
and call it from both the onAuthStateChange handler and the existing-session
branch before assigning window.location.href, ensuring it rejects or normalizes
external origins, strips protocol/host, enforces a leading slash, and falls back
to `${base}account/` when invalid.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0bee4c85-cf8d-4194-9405-41051fb951d6
📒 Files selected for processing (2)
website/src/pages/auth/callback.astrowebsite/tests/e2e/auth/callback.spec.ts
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ON, phishing disclosure, errorCode-only parser, success-path tests Resolves 8 unresolved review threads from PR #1258 second-pass review: #1 sanitizeRedirect: redirect query param now rejects non-same-origin paths (//evil.example, https://attacker.com) — open-redirect/phishing guard. Falls back to base + account/ for null/empty/disallowed input. #2 JSON.stringify undefined: catch block coalesces JSON.stringify(err) to String(err) when stringify returns undefined. #3 INITIAL_SESSION race: onAuthStateChange now accepts both SIGNED_IN and INITIAL_SESSION events. #4 errorCode-only parser: parseUrlError treats any of error, error_code, error_description as a failure marker. #5 phishing details disclosure: raw IdP-supplied error text now lives behind a Show-technical-details disclosure. #6 success-path tests: new describe block covers immediate getSession success, ?redirect=/settings/api-keys/ honored, ?redirect=//evil.example rejected. #7 HTML-escape regression test: spec asserts script tag is escaped. #8 errorCode === unsupported_provider variant test. Spec file count: 9 → 14 tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 of 3 tasks
ooples
added a commit
that referenced
this pull request
May 10, 2026
* fix(website): unblock auth/callback e2e smoke tests in production deploys 5 callback.spec.ts tests were failing on every post-deploy run since PR #1258 merged. Two independent root causes: 1. Astro/Vite production-bundles the supabase import into a hashed `_astro/<chunk>.js` file. The page.route('lib/supabase') glob used by 4 tests never matched the bundled URL, so the real supabase client always loaded and the tests fell into the SIGNED_IN-event-never-fires timeout instead of the branch the spec wanted to cover. Replaced with a tiny page-side test seam: callback.astro now reads `window.__authTestStub ?? defaultSupabase`, tests plant the stub via `page.addInitScript` BEFORE navigation, which works regardless of bundling. 2. The 'literal % does not crash' test asserted on text inside the <details> 'Show technical details' disclosure that PR #1258 added as a phishing mitigation. <details> content is hidden until the summary is clicked. Test now opens the disclosure first. Also: the success-path tests asserted on URL ending in /account/, but /account/ runs its own auth-state check and redirects to /login/ when the session isn't real. Added stubRedirectTargets() to fulfill /account/ and /settings/ HTML with no-op markup so the navigation lands and stays where the callback page sent it. Verified locally against `astro preview`: all 14 tests pass (vs. 5 failed pre-fix). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(vercel): extract ignoreCommand to scripts (256-char limit) Vercel preview deploys for both `aidotnet-playground-api` and `aidotnet_website` started failing on every PR with: The `vercel.json` schema validation failed with the following message: `ignoreCommand` should NOT be longer than 256 characters The previous inline commands were 324 and 267 chars respectively. Move the same logic into shell scripts and reference them from vercel.json (33 / 29 chars): - scripts/vercel-ignore-api.sh — root project (rootDirectory=null); builds when api/ or vercel.json changes, always builds master. - website/scripts/vercel-ignore.sh — website project (rootDirectory=website); builds when anything inside website/ changes, always builds master. Same exit-code contract preserved (0 = ignore, 1 = build) so the Vercel project config doesn't need adjustment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Diagnostic complement to #1257 (GitHub OAuth broken on aidotnet.dev). The redirect to GitHub itself works — the failure is downstream, in the post-GitHub callback. The current `/auth/callback` page silently swallows URL-parameter errors, so any Supabase-side provider failure surfaces as a generic 10-second timeout with no actionable detail.
OAuth providers can fail at three different layers, each surfacing the error in a different place:
The old code only checked layer 3 — that's why hitting `Continue with GitHub` could land here with `?error=server_error&error_description=...` and the user would still just see "Authentication timed out" after 10 seconds.
This PR:
Diagnostic data captured during this investigation
Headless probe of `/login` clicking `Continue with GitHub` from incognito:
So the first half of the OAuth flow is correct. The bug is in the post-GitHub callback. Most likely causes (cannot verify without dashboard access):
After this PR ships, the user will SEE which of those three is firing on their next sign-in attempt — the page will display the actual `error_description` from Supabase instead of the generic timeout.
Test plan
Out of scope
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
UI/UX Improvements
Tests
Chores