diff --git a/packages/core/src/analytics/oauth-callback.analytics.test.ts b/packages/core/src/analytics/oauth-callback.analytics.test.ts index e05f1a6303..9b64738b6f 100644 --- a/packages/core/src/analytics/oauth-callback.analytics.test.ts +++ b/packages/core/src/analytics/oauth-callback.analytics.test.ts @@ -55,4 +55,63 @@ describe("oauth-callback.analytics", () => { provider_error: "exchangeFailed", }); }); + + it("omits provider_error when an exchange failure carries no reason", () => { + expect( + oauthCallbackFailureReason({ + outcome: "error", + state: "signed-state", + code: "auth-code", + oauthError: undefined, + exchangeError: Object.assign(new Error("exchange failed"), { + name: "ProviderAuthError", + }), + }), + ).toEqual({ reason: "token_exchange_error" }); + }); + + it("ignores a non-provider error on the exchange path", () => { + expect( + oauthCallbackFailureReason({ + outcome: "error", + state: "signed-state", + code: "auth-code", + oauthError: undefined, + exchangeError: new TypeError("unrelated"), + }), + ).toEqual({ reason: "other" }); + }); + + it("treats a blank state or code as absent", () => { + expect( + oauthCallbackFailureReason({ + outcome: "error", + state: "", + code: "auth-code", + oauthError: undefined, + }), + ).toEqual({ reason: "invalid_state" }); + expect( + oauthCallbackFailureReason({ + outcome: "error", + state: "signed-state", + code: "", + oauthError: undefined, + }), + ).toEqual({ reason: "missing_code" }); + }); + + it("maps a state verification failure to invalid_state", () => { + for (const stateVerifyReason of ["invalidSignature", "expired"] as const) { + expect( + oauthCallbackFailureReason({ + outcome: "error", + state: "signed-state", + code: "auth-code", + oauthError: undefined, + stateVerifyReason, + }), + ).toEqual({ reason: "invalid_state" }); + } + }); }); diff --git a/packages/core/src/analytics/oauth-callback.analytics.ts b/packages/core/src/analytics/oauth-callback.analytics.ts index edb02ce570..c1c2cc774c 100644 --- a/packages/core/src/analytics/oauth-callback.analytics.ts +++ b/packages/core/src/analytics/oauth-callback.analytics.ts @@ -2,6 +2,7 @@ import { type OAuthFailedReason, oauthFailedReasonFromCallbackQuery, } from "@core/analytics/oauth-signup.events"; +import { type OAuthStateFailure } from "@core/types/sync/connection.contracts"; export type SyncOAuthCallbackOutcome = | "connected" @@ -42,32 +43,28 @@ export function oauthCallbackFailureReason(input: { code: unknown; oauthError: unknown; oauthErrorDescription?: unknown; - stateVerifyReason?: - | "malformed" - | "invalidSignature" - | "expired" - | "stateMismatch" - | undefined; + stateVerifyReason?: OAuthStateFailure | undefined; exchangeError?: unknown; }): { reason: OAuthFailedReason; provider_error?: string } | undefined { if (input.outcome === "connected") return undefined; - const providerErrorRaw = - typeof input.oauthError === "string" ? input.oauthError : undefined; + // Callback query values reach here as `unknown` straight off the request, so + // each one is narrowed once up front rather than at every branch that reads it. + const state = asNonEmptyString(input.state); + const code = asNonEmptyString(input.code); + const providerErrorRaw = asNonEmptyString(input.oauthError); const providerError = sanitizeOAuthProviderError(providerErrorRaw); const providerDescription = sanitizeOAuthProviderError( - typeof input.oauthErrorDescription === "string" - ? input.oauthErrorDescription - : undefined, + asNonEmptyString(input.oauthErrorDescription), ); const mergedProviderError = providerError ?? providerDescription; if (input.outcome === "declined") { const reason = oauthFailedReasonFromCallbackQuery({ - state: typeof input.state === "string" ? input.state : null, - code: typeof input.code === "string" ? input.code : null, - providerError: providerErrorRaw ?? null, + state, + code, + providerError: providerErrorRaw, }) ?? "other"; return withProviderError(reason, providerError); } @@ -77,13 +74,10 @@ export function oauthCallbackFailureReason(input: { } if (input.outcome === "error") { - const hasState = typeof input.state === "string" && input.state.length > 0; - const hasCode = typeof input.code === "string" && input.code.length > 0; - - if (!hasState || input.stateVerifyReason === "malformed") { + if (!state || input.stateVerifyReason === "malformed") { return withProviderError("invalid_state", providerError); } - if (!hasCode) { + if (!code) { return withProviderError("missing_code", providerError); } if ( @@ -92,11 +86,9 @@ export function oauthCallbackFailureReason(input: { ) { return { reason: "invalid_state" }; } - if (isTokenExchangeFailure(input.exchangeError)) { - return { - reason: "token_exchange_error", - provider_error: providerErrorFromExchange(input.exchangeError), - }; + const exchange = tokenExchangeFailure(input.exchangeError); + if (exchange) { + return withProviderError("token_exchange_error", exchange.providerError); } return withProviderError("other", mergedProviderError); } @@ -114,17 +106,22 @@ function withProviderError( }; } -function isTokenExchangeFailure(error: unknown): boolean { - if (!error || typeof error !== "object") return false; - const name = (error as { name?: unknown }).name; - return name === "ProviderAuthError"; +/** Blank query values mean "absent", the same way a missing param does. */ +function asNonEmptyString(value: unknown): string | undefined { + return typeof value === "string" && value.length > 0 ? value : undefined; } -function providerErrorFromExchange(error: unknown): string | undefined { - if (!error || typeof error !== "object") return undefined; - const reason = (error as { reason?: unknown }).reason; - if (typeof reason === "string" && reason.length > 0) { - return sanitizeOAuthProviderError(reason); - } - return undefined; +/** + * A provider token-exchange failure, matched structurally because core cannot + * import sync's `ProviderAuthError` without inverting the package dependency. + * Returns its sanitized reason when it matches, else null. + */ +function tokenExchangeFailure( + error: unknown, +): { providerError?: string } | null { + if (!error || typeof error !== "object") return null; + const { name, reason } = error as { name?: unknown; reason?: unknown }; + if (name !== "ProviderAuthError") return null; + const providerError = sanitizeOAuthProviderError(asNonEmptyString(reason)); + return { ...(providerError ? { providerError } : {}) }; } diff --git a/packages/core/src/types/sync/connection.contracts.ts b/packages/core/src/types/sync/connection.contracts.ts index 1c444dce45..3b6248f179 100644 --- a/packages/core/src/types/sync/connection.contracts.ts +++ b/packages/core/src/types/sync/connection.contracts.ts @@ -71,6 +71,18 @@ export type ConnectionBeginFeatures = z.infer< typeof ConnectionBeginFeaturesSchema >; +// Why a signed OAuth state token failed verification on the public callback. +// Sync mints the state at `begin` and verifies the echo; callback analytics map +// these onto funnel reasons. Shared here so the verifier and the analytics +// mapper cannot drift apart. +export const OAuthStateFailureSchema = z.enum([ + "malformed", + "invalidSignature", + "expired", + "stateMismatch", +]); +export type OAuthStateFailure = z.infer; + // Opaque authenticated-encryption envelope for credentials sent between the // Compass API and Sync services. The browser never sees this transport type. export const EncryptedCredentialEnvelopeSchema = z.strictObject({ diff --git a/packages/sync/src/oauth/oauth-state.ts b/packages/sync/src/oauth/oauth-state.ts index fff2027951..2b5707647f 100644 --- a/packages/sync/src/oauth/oauth-state.ts +++ b/packages/sync/src/oauth/oauth-state.ts @@ -1,3 +1,4 @@ +import { type OAuthStateFailure } from "@core/types/sync/connection.contracts"; import { type ConnectionId, ConnectionIdSchema, @@ -56,12 +57,6 @@ export function signOAuthState( return `${encoded}.${sign(secret, encoded)}`; } -export type OAuthStateFailure = - | "malformed" - | "invalidSignature" - | "expired" - | "stateMismatch"; - export type OAuthStateResult = | { readonly ok: true; readonly payload: OAuthStatePayload } | { readonly ok: false; readonly reason: OAuthStateFailure }; diff --git a/packages/sync/src/server/connection.routes.ts b/packages/sync/src/server/connection.routes.ts index ade4a8590c..c3114db418 100644 --- a/packages/sync/src/server/connection.routes.ts +++ b/packages/sync/src/server/connection.routes.ts @@ -32,6 +32,7 @@ import { ConnectionRefreshResponseSchema, ForegroundRefreshRequestSchema, GoogleConnectionAdoptionRequestSchema, + type OAuthStateFailure, type ProviderCalendar, ProviderCalendarSchema, type ProviderConnection, @@ -72,7 +73,6 @@ import { HORIZON_PAST_MONTHS, } from "@sync/domain/horizon"; import { - type OAuthStateFailure, peekOAuthStatePayload, signOAuthState, verifyOAuthState,