From b9cb24584fd4eefe58aba0f129ea99fda2d9e478 Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Thu, 5 Nov 2026 23:37:11 -0500 Subject: [PATCH 1/4] fix(oauth): record and log why an OAuth start failed The catch dropped the error, so a refused redirect and a dead provider left the same bare start_failed event and nothing in the log. --- .changeset/oauth-start-failure-detail.md | 5 +++++ src/controllers/oauth.ts | 13 +++++++++++-- src/services/oauthService.ts | 20 +++++++++++++++++++- tests/integration/oauth/oauth.spec.ts | 23 ++++++++++++++++++++++- 4 files changed, 57 insertions(+), 4 deletions(-) create mode 100644 .changeset/oauth-start-failure-detail.md diff --git a/.changeset/oauth-start-failure-detail.md b/.changeset/oauth-start-failure-detail.md new file mode 100644 index 00000000..99f7a0af --- /dev/null +++ b/.changeset/oauth-start-failure-detail.md @@ -0,0 +1,5 @@ +--- +'seamless-auth-api': patch +--- + +A failed OAuth start now says why. The `oauth_login_failed` event with reason `start_failed` carries a `detail` of `redirect_not_allowed` or `provider_unusable`, and the error is logged with the provider id. A refused redirect is logged by scheme and host only. The answer to the client is unchanged. diff --git a/src/controllers/oauth.ts b/src/controllers/oauth.ts index 35a3e219..48e38c2e 100644 --- a/src/controllers/oauth.ts +++ b/src/controllers/oauth.ts @@ -25,6 +25,7 @@ import { OAuthProfileError, oauthProfileFromIdToken, OAuthProviderRetiredError, + OAuthRedirectNotAllowedError, resolveOAuthProvider, resolveOAuthRedirectUri, resolveOAuthUser, @@ -98,11 +99,19 @@ export async function startOAuthLogin(req: RouteRequest, res: Response) { ...(codeChallenge ? { codeChallenge } : {}), }), }); - } catch { + } catch (error) { + // The client only ever sees a generic answer, so this is the one place an operator + // can learn why. A redirect refusal is a client or allowlist problem and anything + // else is the provider's configuration or discovery, and those are fixed differently. + const detail = + error instanceof OAuthRedirectNotAllowedError ? 'redirect_not_allowed' : 'provider_unusable'; + + logger.warn(`OAuth start failed for provider ${provider.id}: ${String(error)}`); + await AuthEventService.log({ type: 'oauth_login_failed', req, - metadata: { providerId: provider.id, reason: 'start_failed' }, + metadata: { providerId: provider.id, reason: 'start_failed', detail }, }); return res.status(400).json({ diff --git a/src/services/oauthService.ts b/src/services/oauthService.ts index 3a08f635..a47e27f0 100644 --- a/src/services/oauthService.ts +++ b/src/services/oauthService.ts @@ -87,6 +87,24 @@ export class OAuthProfileError extends Error { } } +/** + * The client asked for a redirect the provider config does not allow. The message names + * the scheme and host only: the rest of the value is whatever the caller sent. + */ +export class OAuthRedirectNotAllowedError extends Error { + constructor(requestedRedirectUri: string) { + let target = 'an unparsable value'; + try { + const url = new URL(requestedRedirectUri); + target = `${url.protocol}//${url.host}`; + } catch { + // Named as unparsable above. + } + super(`OAuth redirect URI is not allowed: ${target}`); + this.name = 'OAuthRedirectNotAllowedError'; + } +} + export class OAuthProviderRetiredError extends Error { readonly code = 'oauth_provider_retired'; readonly userId: string; @@ -259,7 +277,7 @@ export async function resolveOAuthRedirectUri( if (requestedRedirectUri) { if (!allowedRedirect(requestedRedirectUri, providerAllowlist, config.origins)) { - throw new Error('OAuth redirect URI is not allowed'); + throw new OAuthRedirectNotAllowedError(requestedRedirectUri); } return requestedRedirectUri; diff --git a/tests/integration/oauth/oauth.spec.ts b/tests/integration/oauth/oauth.spec.ts index 62273c90..1c7f6002 100644 --- a/tests/integration/oauth/oauth.spec.ts +++ b/tests/integration/oauth/oauth.spec.ts @@ -570,6 +570,12 @@ describe('OAuth routes', () => { vi.unstubAllGlobals(); expect(res.status).toBe(400); expect(res.body.error).toBe('OAuth start failed'); + expect(AuthEventService.log).toHaveBeenCalledWith( + expect.objectContaining({ + type: 'oauth_login_failed', + metadata: { providerId: 'idp', reason: 'start_failed', detail: 'provider_unusable' }, + }), + ); }); it('refuses an ID token minted for a different sign-in', async () => { @@ -621,7 +627,22 @@ describe('OAuth routes', () => { expect(AuthEventService.log).toHaveBeenCalledWith( expect.objectContaining({ type: 'oauth_login_failed', - metadata: { providerId: 'google', reason: 'start_failed' }, + metadata: { providerId: 'google', reason: 'start_failed', detail: 'redirect_not_allowed' }, + }), + ); + }); + + it('refuses an app scheme the provider does not allow, and says that is why', async () => { + const res = await request(app).post('/oauth/google/start').send({ + redirectUri: 'myapp://oauth/callback?code=secret', + }); + + expect(res.status).toBe(400); + expect(res.body.error).toBe('OAuth start failed'); + expect(AuthEventService.log).toHaveBeenCalledWith( + expect.objectContaining({ + type: 'oauth_login_failed', + metadata: { providerId: 'google', reason: 'start_failed', detail: 'redirect_not_allowed' }, }), ); }); From 88b5e7d8ec83f248723f5741bce25402f3577853 Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Fri, 6 Nov 2026 00:26:09 -0500 Subject: [PATCH 2/4] fix(oauth): log the requested provider id, not the config's CodeQL treats a value read from the provider config as sensitive. The route parameter is the same id. --- src/controllers/oauth.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/controllers/oauth.ts b/src/controllers/oauth.ts index 48e38c2e..d22466ea 100644 --- a/src/controllers/oauth.ts +++ b/src/controllers/oauth.ts @@ -106,7 +106,7 @@ export async function startOAuthLogin(req: RouteRequest, res: Response) { const detail = error instanceof OAuthRedirectNotAllowedError ? 'redirect_not_allowed' : 'provider_unusable'; - logger.warn(`OAuth start failed for provider ${provider.id}: ${String(error)}`); + logger.warn(`OAuth start failed for provider ${providerId}: ${String(error)}`); await AuthEventService.log({ type: 'oauth_login_failed', From 2679d11d3b8534fd9221dcd6e19c5fd3d1e72037 Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Fri, 6 Nov 2026 00:28:51 -0500 Subject: [PATCH 3/4] fix(oauth): keep a failed start's log entry on one line CodeQL flagged the route parameter in the log entry. Line breaks are the part of it that could forge another entry, so they are removed from the whole message, including the error text. --- src/controllers/oauth.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/controllers/oauth.ts b/src/controllers/oauth.ts index d22466ea..3ad1ca70 100644 --- a/src/controllers/oauth.ts +++ b/src/controllers/oauth.ts @@ -106,7 +106,9 @@ export async function startOAuthLogin(req: RouteRequest, res: Response) { const detail = error instanceof OAuthRedirectNotAllowedError ? 'redirect_not_allowed' : 'provider_unusable'; - logger.warn(`OAuth start failed for provider ${providerId}: ${String(error)}`); + // On one line whatever the route parameter or the error holds. + const failure = `OAuth start failed for provider ${providerId}: ${String(error)}`; + logger.warn(failure.replace(/[\r\n]+/g, ' ')); await AuthEventService.log({ type: 'oauth_login_failed', From 5e08d9ddfa8ac5869ecaa795004a92bea4b74a20 Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Fri, 6 Nov 2026 00:31:52 -0500 Subject: [PATCH 4/4] fix(oauth): leave the route parameter out of a failed start's log entry The event already records the provider, so the log entry does not need a value from the request. --- .changeset/oauth-start-failure-detail.md | 2 +- src/controllers/oauth.ts | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.changeset/oauth-start-failure-detail.md b/.changeset/oauth-start-failure-detail.md index 99f7a0af..47098fe6 100644 --- a/.changeset/oauth-start-failure-detail.md +++ b/.changeset/oauth-start-failure-detail.md @@ -2,4 +2,4 @@ 'seamless-auth-api': patch --- -A failed OAuth start now says why. The `oauth_login_failed` event with reason `start_failed` carries a `detail` of `redirect_not_allowed` or `provider_unusable`, and the error is logged with the provider id. A refused redirect is logged by scheme and host only. The answer to the client is unchanged. +A failed OAuth start now says why. The `oauth_login_failed` event with reason `start_failed` carries a `detail` of `redirect_not_allowed` or `provider_unusable`, and the error is logged. A refused redirect is logged by scheme and host only. The answer to the client is unchanged. diff --git a/src/controllers/oauth.ts b/src/controllers/oauth.ts index 3ad1ca70..4c0568de 100644 --- a/src/controllers/oauth.ts +++ b/src/controllers/oauth.ts @@ -106,9 +106,9 @@ export async function startOAuthLogin(req: RouteRequest, res: Response) { const detail = error instanceof OAuthRedirectNotAllowedError ? 'redirect_not_allowed' : 'provider_unusable'; - // On one line whatever the route parameter or the error holds. - const failure = `OAuth start failed for provider ${providerId}: ${String(error)}`; - logger.warn(failure.replace(/[\r\n]+/g, ' ')); + // The provider is on the event below. The error names a refused redirect by its parsed + // scheme and host only, so nothing the caller sent reaches the log as written. + logger.warn(`OAuth start failed: ${String(error)}`); await AuthEventService.log({ type: 'oauth_login_failed',