OAuth provider failures were not handled properly in the frontend, causing unclear errors and broken login behavior.#415
Conversation
…auth-failures [WIP] Add error handling for OAuth provider failures in frontend
…OAuth provider failures. - Enhance getOIDCUserData and triggerOauthFlow functions to handle network and provider errors gracefully. - Update login callback to log and respond to OAuth provider errors with appropriate messages. - Introduce SignInButton component for user sign-in with error handling via query parameters. - Create integration tests for OAuth provider failures to ensure correct error propagation and responses. - Add mongodb-memory-server dependency for testing purposes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR improves resilience and UX around OAuth provider outages by introducing structured provider-failure errors (502 + {code,message}), mapping those errors to user-facing toasts, and adding automated test coverage for failure paths.
Changes:
- Add typed
OAuthProviderError/OAuthErrorCodeand wrap OIDC discovery/callback flows to emit structured 502 JSON errors on provider failures. - Redirect HTML navigations that hit provider failures to
/?error=<CODE>so the UI can show a toast. - Add Vitest coverage for provider-down scenarios in both the auth helper and the login callback route.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/lib/server/auth.ts | Adds OAuthProviderError + wraps provider calls to classify/log failures and return structured errors. |
| frontend/src/hooks.server.ts | Intercepts 502 from triggerOauthFlow and redirects HTML requests to /?error=<CODE>. |
| frontend/src/routes/login/callback/+server.ts | Catches OAuthProviderError during callback token exchange and returns a structured 502 JSON response. |
| frontend/src/routes/+layout.svelte | Reads ?error= and maps OAuth error codes to toast messages. |
| frontend/src/lib/components/SignInButton.svelte | Adds a sign-in button component that also reads ?error= and sets the error store. |
| frontend/src/lib/server/auth.spec.ts | Adds unit tests for OAuthProviderError, triggerOauthFlow, and getOIDCUserData failure handling. |
| frontend/src/routes/login/callback/auth-provider-failure.spec.ts | Adds integration-style tests for provider downtime behavior. |
| frontend/src/routes/login/callback/provider-error-response.spec.ts | Adds tests asserting the callback route returns structured 502 JSON payloads. |
| frontend/package.json | Adds mongodb-memory-server to support server Vitest environment setup. |
| frontend/package-lock.json | Updates lockfile for the added dev dependency and its transitive deps. |
Files not reviewed (1)
- frontend/package-lock.json: Language not supported
| if (!oauthErrorCode || oauthErrorCode === lastOAuthErrorCode) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
lastOAuthErrorCode prevents showing the toast again for the same ?error= code. If a user retries login and hits the same provider failure, they may not get any feedback on subsequent redirects/navigations. Consider clearing the error query param after consuming it (e.g., via goto with replaceState) or resetting lastOAuthErrorCode when the URL no longer has error, so repeated failures still surface a toast.
| if (!oauthErrorCode || oauthErrorCode === lastOAuthErrorCode) { | |
| return; | |
| } | |
| if (!oauthErrorCode) { | |
| lastOAuthErrorCode = null; | |
| return; | |
| } | |
| if (oauthErrorCode === lastOAuthErrorCode) { | |
| return; | |
| } |
| let { classNames = "", label = "Sign in" }: Props = $props(); | ||
|
|
||
| const OAUTH_ERROR_MESSAGES: Record<string, string> = { | ||
| OAUTH_PROVIDER_UNAVAILABLE: | ||
| "Sign-in service is temporarily unavailable. Please try again later.", | ||
| OAUTH_PROVIDER_ERROR: "An error occurred during sign-in. Please try again.", | ||
| }; | ||
|
|
||
| onMount(() => { | ||
| const errorCode = page.url.searchParams.get("error"); | ||
| if (!errorCode) return; | ||
|
|
||
| errorStore.set(OAUTH_ERROR_MESSAGES[errorCode] ?? ERROR_MESSAGES.default); | ||
| }); |
There was a problem hiding this comment.
This new SignInButton component isn't referenced anywhere under frontend/src (no imports/usages found), so it may be dead code and won’t affect the login UX. Either wire it into the relevant page(s) or remove it; if it is intended to handle ?error= toasts, that logic is now duplicated with the root +layout.svelte handler and should be centralized in one place to avoid double-setting the error store.
| const oauthResponse = await triggerOauthFlow(event); | ||
| if (oauthResponse.status === 502) { | ||
| const accept = event.request.headers.get("accept") || ""; | ||
| const wantsHtml = accept.includes("text/html"); | ||
|
|
||
| if (wantsHtml) { | ||
| try { | ||
| const body = await oauthResponse.clone().json(); | ||
| if (typeof body?.code === "string") { | ||
| return new Response(null, { | ||
| status: 302, | ||
| headers: { | ||
| Location: `${base}/?error=${encodeURIComponent(body.code)}`, | ||
| "Cache-Control": "no-store", | ||
| }, | ||
| }); | ||
| } | ||
| } catch { | ||
| // fall through and return original structured 502 response | ||
| } | ||
| } | ||
| } | ||
| return oauthResponse; |
There was a problem hiding this comment.
The 502-to-302 HTML redirect handling is duplicated in two branches in this file, which makes it easy for the behavior to drift over time (e.g., headers/message parsing). Consider extracting this into a small helper function (e.g., redirectOauthErrorIfHtml(event, oauthResponse)) and reusing it in both call sites.
| const { mockIssuerDiscover } = vi.hoisted(() => ({ | ||
| mockIssuerDiscover: vi.fn(), | ||
| })); | ||
|
|
There was a problem hiding this comment.
These newly added tests use spaces for indentation, but the repo's Prettier config sets useTabs: true (frontend/.prettierrc). This will likely fail npm run lint (prettier --check .) unless reformatted. Please run Prettier (or adjust indentation to tabs) on this file.
| const { mockIssuerDiscover } = vi.hoisted(() => ({ | ||
| mockIssuerDiscover: vi.fn(), | ||
| })); | ||
|
|
There was a problem hiding this comment.
These newly added tests use spaces for indentation, but the repo's Prettier config sets useTabs: true (frontend/.prettierrc). This will likely fail npm run lint (prettier --check .) unless reformatted. Please run Prettier (or adjust indentation to tabs) on this file.
| // Re-wrap as a typed error so callers (e.g. callback +server.ts) can distinguish | ||
| // provider unavailability from other failures without logging sensitive token data. | ||
| const isNetworkError = | ||
| err instanceof TypeError || | ||
| (err instanceof Error && (err.message.includes("ECONNREFUSED") || err.message.includes("ENOTFOUND") || err.message.includes("fetch failed"))); | ||
| const code: OAuthErrorCode = isNetworkError | ||
| ? "OAUTH_PROVIDER_UNAVAILABLE" | ||
| : "OAUTH_PROVIDER_ERROR"; | ||
| logger.error({ code, msg: err instanceof Error ? err.message : String(err) }, "OAuth provider error in getOIDCUserData"); | ||
| throw new OAuthProviderError( | ||
| isNetworkError ? "OAuth provider is unreachable" : "OAuth provider returned an error", | ||
| code | ||
| ); |
There was a problem hiding this comment.
Both getOIDCUserData and triggerOauthFlow implement similar network-error classification logic via string matching on err.message. Consider extracting this into a shared helper (e.g., classifyOAuthProviderError(err): OAuthErrorCode) so the mapping stays consistent and is easier to update when new error shapes/messages appear.
| } catch (err) { | ||
| if (err instanceof OAuthProviderError) { | ||
| const message = | ||
| err.code === "OAUTH_PROVIDER_UNAVAILABLE" | ||
| ? "The sign-in service is temporarily unavailable. Please try again later." | ||
| : "An error occurred during sign-in. Please try again."; | ||
|
|
||
| logger.error( | ||
| { code: err.code, msg: err.message }, | ||
| "OAuth provider error during callback token exchange" | ||
| ); | ||
|
|
||
| return new Response( | ||
| JSON.stringify({ code: err.code, message }), | ||
| { | ||
| status: 502, | ||
| headers: { "Content-Type": "application/json" }, | ||
| } | ||
| ); |
There was a problem hiding this comment.
The callback route returns a JSON 502 response for OAuthProviderError regardless of request type. In the browser OAuth callback flow this is typically an HTML navigation, so users may land on a raw JSON error page instead of being redirected to /?error=<CODE> for the toast handling described in the PR. Consider branching on the request's Accept header (similar to hooks.server.ts): redirect for text/html and keep the structured JSON 502 for API callers (also add Cache-Control: no-store on the redirect/error responses).
Change Type (select all that apply)
Scope (select all touched areas)
Linked Issue/PR
User-Visible / Behavior Changes
?error=<CODE>for UI handling.Security Impact (required)
Yes, explain risk + mitigation: NoneVerification
Environment
Steps to Test
Expected Behavior
Actual Behavior
Evidence (attach at least one)
Human Verification (required)
What you personally verified (not just CI):
Compatibility / Migration
Failure Recovery (if this breaks)
auth.ts,hooks.server.ts,+server.ts,+layout.svelte,SignInButtonRisks and Mitigations
Checklist
uv run pytest)uv run pre-commit run --all-files)