From 0bfe823302aa6ea1423da3fdbd88e5b0a93945d7 Mon Sep 17 00:00:00 2001 From: Yahya Fakhroji Date: Sat, 1 Aug 2026 20:16:22 +0700 Subject: [PATCH] fix(login): route every single-method account through the method chooser MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The identifier step picked a per-method screen from a static map, so an account whose only method was a linked IdP was sent to /sso — a session-gated management page that cannot start an intent. A Google-only user landed on a dead end instead of at Google. Route every account with at least one usable method to /login/method instead, which renders exactly what that account has and auto-starts what needs no form. The decision is pure over method KINDS, so it has no idpId and could only ever name a static path; resolving the actual provider has to happen where the links are known. - A sole linked IdP starts its intent in the LOADER, so the chooser never flashes. Being in a loader is also what makes it dangerous: a loader re-runs when the user presses Back at the provider, which would mint a new intent and bounce them forward forever. A one-shot marker cookie (signed, httpOnly, 10 min) makes the second arrival render the chooser instead, whose button restarts the ceremony deliberately as a POST. It is retired on each fresh identifier submit so the next sign-in auto-starts again. - A sole passkey begins its ceremony on mount. The auto-begun attempt has no transient user activation behind it, which WebKit requires, so its failure copy is suppressed for that attempt only rather than accusing a user who has done nothing but arrive. - Sole-password accounts take one extra click. Deliberate: collapsing every branch onto one destination is what keeps the unknown-identifier path indistinguishable. The loader is state-changing and its outcomes are distinguishable from outside, so it is gated on a live ceremony session for that loginName and throttled by two tiers — a tight ip|loginName budget bounding intents minted against one address, and a loose ip ceiling bounding enumeration breadth. The identifier POST that hands out that session is throttled too; it was the one unthrottled member of the auth surface. Anti-enumeration holds across the move. `ignoreUnknownUsernames` routes an unknown identifier through the same decision a password-only account gets, and the chooser serves it the same screen — same status, target, payload, cookies and audit line. Its policy org is resolved from the identifier's domain, so an unknown address is judged by the org that claims it rather than by the default org, whose policy can differ. `?policyOrg=` is now HMAC-authenticated. It rides an IdP return URL, so it is fully attacker-writable, and it is the one param there coupled to nothing else: it feeds the callback's allowRegister gate and auto-create org alone, while the same-email lookup stays instance-wide. A hand-written value would borrow a registration-open org's policy across that gap. Length validation cannot help, since decoupling those two orgs is the param's whole purpose — only provenance can. Also fixes the 429 page's back-link, which hardcoded /id instead of deriving it from APP_BASENAME and would 404 the user at the moment they are trying to recover. --- app/modules/auth/session/idp-autostart.ts | 64 ++++ app/modules/i18n/locales/en.po | 44 +-- app/resources/login/login-decision.ts | 15 +- app/resources/login/login.service.ts | 101 ++++-- app/resources/login/method-options.ts | 159 +++++++++ app/resources/shared/resolve-org.ts | 39 +++ app/resources/sso/idp-return-urls.ts | 36 ++ app/resources/sso/sso-callback.ts | 54 ++- app/routes/login/index.tsx | 18 +- app/routes/login/method.tsx | 301 ++++++++++++---- app/server.ts | 10 + app/server/middleware/rate-limit.ts | 77 ++++ app/server/net.ts | 64 +++- app/server/signed-param.ts | 52 +++ .../resources/login/login-decision.cy.ts | 52 ++- .../resolve-identifier-ignore-unknown.cy.ts | 28 ++ .../shared/org-first-settings-reads.cy.ts | 61 +++- .../resources/sso/idp-return-urls.cy.ts | 50 ++- .../resources/sso/sso-callback.cy.ts | 144 ++++++++ .../routes/login/enumeration-parity.cy.ts | 214 ++++++++++++ cypress/component/routes/login/index.cy.tsx | 30 +- .../routes/login/method-chooser.cy.ts | 328 +++++++++++++++++- cypress/component/routes/login/method.cy.tsx | 168 ++++++++- .../server/middleware/rate-limit.cy.ts | 96 +++++ cypress/component/server/net.cy.ts | 19 + .../component/support/to-request-url.cy.ts | 19 +- cypress/e2e/a11y-sweep.cy.ts | 20 ++ cypress/e2e/core-signin.cy.ts | 8 + cypress/e2e/forced-mfa-setup.cy.ts | 3 + cypress/e2e/login-hydrated-submit.cy.ts | 9 +- cypress/e2e/logout.cy.ts | 3 + cypress/e2e/mfa-totp.cy.ts | 4 + cypress/e2e/passkey-conditional.cy.ts | 15 +- cypress/e2e/passkey-discovery.cy.ts | 5 +- cypress/e2e/passkeys-manage.cy.ts | 7 +- cypress/e2e/setup-otp.cy.ts | 58 +--- cypress/e2e/setup-passkey-mfa.cy.ts | 7 +- cypress/e2e/verify-otp.cy.ts | 2 + cypress/support/node/harness.ts | 220 +++++++++++- cypress/support/node/scenario.ts | 39 +++ cypress/support/session.ts | 132 +++++-- cypress/support/vite-cypress-stubs.ts | 11 + 42 files changed, 2514 insertions(+), 272 deletions(-) create mode 100644 app/modules/auth/session/idp-autostart.ts create mode 100644 app/resources/login/method-options.ts create mode 100644 app/server/signed-param.ts create mode 100644 cypress/component/routes/login/enumeration-parity.cy.ts create mode 100644 cypress/component/server/net.cy.ts diff --git a/app/modules/auth/session/idp-autostart.ts b/app/modules/auth/session/idp-autostart.ts new file mode 100644 index 0000000000..354ad0801b --- /dev/null +++ b/app/modules/auth/session/idp-autostart.ts @@ -0,0 +1,64 @@ +import { env } from '@/server/infra/env.server'; +import { createCookie } from 'react-router'; + +/** + * ONE-SHOT marker for /login/method's sole-linked-IdP auto-start: the loginName whose IdP + * intent this browser has ALREADY been sent to the provider for. + * + * The auto-start lives in a LOADER (a server redirect, so the chooser never flashes), and a + * loader re-runs on every arrival at the URL — including the one the browser makes when the + * user presses Back at the provider. Without this marker that Back mints a brand-new Zitadel + * intent and bounces them straight forward again: the user can never return to the app. With + * it, the second arrival for the SAME loginName falls through and renders the chooser, whose + * IdP button re-starts the ceremony deliberately (a POST, where Back does not re-fire). + * + * NEVER an auth signal and never rendered — it only suppresses one automatic redirect. + * Same protection class as the `sessions` / `reauth-intent` cookies (which already store a + * loginName): httpOnly, sameSite lax, scoped to `/id`, signed with SESSION_SECRET. + * SHORT maxAge (10 min, matching reauth-intent): a sign-in completes promptly, and a stale + * marker must not keep suppressing the no-flash fast path for a later, unrelated visit. + */ +export const idpAutostartCookie = createCookie('idp-autostart', { + httpOnly: true, + sameSite: 'lax', + path: '/id', + secure: env.NODE_ENV === 'production', + secrets: [env.SESSION_SECRET], + maxAge: 60 * 10, // 10 minutes +}); + +/** Serialize the marker (the loginName just auto-started) to a Set-Cookie string. */ +export async function serializeIdpAutostart(loginName: string): Promise { + return idpAutostartCookie.serialize(loginName); +} + +/** + * Expire the marker — emitted by the IDENTIFIER submit, which is the moment a NEW sign-in + * ceremony begins. + * + * The marker only ever needs to survive one ceremony: it exists to stop the Back-from-the- + * provider arrival re-minting an intent, and Back does not re-POST the identifier. Left to its + * own 10-minute maxAge it outlived the ceremony that wrote it, so a user who signed in, signed + * out, and signed straight back in got the one-button chooser instead of the auto-start the + * whole feature exists to give them. Clearing it here scopes the one-shot to exactly one + * ceremony without weakening the Back guard at all. + */ +export async function clearIdpAutostart(): Promise { + return idpAutostartCookie.serialize('', { maxAge: 0 }); +} + +/** Read the marked loginName. Returns null when the cookie is absent, invalid, or empty. */ +export async function readIdpAutostart(request: Request): Promise { + const value = await idpAutostartCookie.parse(request.headers.get('cookie')); + return typeof value === 'string' && value.length > 0 ? value : null; +} + +/** + * True when this browser has already been auto-started into an IdP for `loginName`. + * Case-insensitive: the marker is written from the URL's loginName while the arrival that + * reads it may carry a different casing of the same account (IdPs / SAML round-trips and + * hand-typed identifiers both do this) — an exact compare would silently re-arm the trap. + */ +export function idpAutostartMatches(marker: string | null, loginName: string): boolean { + return marker !== null && marker.trim().toLowerCase() === loginName.trim().toLowerCase(); +} diff --git a/app/modules/i18n/locales/en.po b/app/modules/i18n/locales/en.po index 4bc049f5f2..34c67eb47b 100644 --- a/app/modules/i18n/locales/en.po +++ b/app/modules/i18n/locales/en.po @@ -78,7 +78,7 @@ msgstr "Additional verification is required to continue." msgid "Already have an account?" msgstr "Already have an account?" -#: app/routes/login/index.tsx:424 +#: app/routes/login/index.tsx:434 msgid "An account with this email already exists — sign in to continue." msgstr "An account with this email already exists — sign in to continue." @@ -172,7 +172,7 @@ msgstr "Choose a new password" msgid "Choose an account" msgstr "Choose an account" -#: app/routes/login/method.tsx:184 +#: app/routes/login/method.tsx:341 msgid "Choose how to sign in" msgstr "Choose how to sign in" @@ -180,7 +180,7 @@ msgstr "Choose how to sign in" msgid "Choose how you want to verify your identity." msgstr "Choose how you want to verify your identity." -#: app/routes/login/index.tsx:420 +#: app/routes/login/index.tsx:430 msgid "Choose your login method" msgstr "Choose your login method" @@ -211,7 +211,7 @@ msgid "Connected accounts" msgstr "Connected accounts" #: app/routes/device/index.tsx:73 -#: app/routes/login/index.tsx:546 +#: app/routes/login/index.tsx:556 #: app/routes/signup/index.tsx:291 msgid "Continue" msgstr "Continue" @@ -241,7 +241,7 @@ msgstr "Couldn't verify" msgid "Create a new account" msgstr "Create a new account" -#: app/routes/login/index.tsx:591 +#: app/routes/login/index.tsx:601 #: app/routes/signup/password.tsx:237 msgid "Create account" msgstr "Create account" @@ -268,8 +268,8 @@ msgstr "Device code" msgid "Device denied" msgstr "Device denied" -#: app/routes/login/index.tsx:377 -#: app/routes/login/index.tsx:392 +#: app/routes/login/index.tsx:387 +#: app/routes/login/index.tsx:402 #: app/routes/signup/index.tsx:250 #: app/routes/signup/index.tsx:275 msgid "Email" @@ -283,9 +283,9 @@ msgstr "Email code" msgid "Email me a code" msgstr "Email me a code" -#: app/routes/login/index.tsx:557 #: app/routes/login/index.tsx:567 -#: app/routes/login/method.tsx:238 +#: app/routes/login/index.tsx:577 +#: app/routes/login/method.tsx:411 #: app/routes/signup/method.tsx:339 msgid "Email me a sign-in link" msgstr "Email me a sign-in link" @@ -306,7 +306,7 @@ msgstr "Email OTP" msgid "Email sign-in isn't available — use your username." msgstr "Email sign-in isn't available — use your username." -#: app/routes/login/index.tsx:376 +#: app/routes/login/index.tsx:386 msgid "Email, phone, or username" msgstr "Email, phone, or username" @@ -446,13 +446,13 @@ msgstr "No signed-in accounts." msgid "Not now" msgstr "Not now" -#: app/routes/login/index.tsx:589 +#: app/routes/login/index.tsx:599 msgid "Not registered?" msgstr "Not registered?" #: app/components/identity-badge/identity-badge.tsx:30 #: app/routes/device/authorize.tsx:122 -#: app/routes/login/method.tsx:193 +#: app/routes/login/method.tsx:350 #: app/routes/passkeys.tsx:249 #: app/routes/reauth.tsx:280 #: app/routes/signed-in.tsx:49 @@ -472,8 +472,8 @@ msgstr "or" msgid "Or import this URI in your authenticator app" msgstr "Or import this URI in your authenticator app" -#: app/routes/login/index.tsx:487 -#: app/routes/login/method.tsx:221 +#: app/routes/login/index.tsx:497 +#: app/routes/login/method.tsx:394 #: app/routes/reauth.tsx:310 #: app/routes/setup/mfa.tsx:46 msgid "Passkey" @@ -511,7 +511,7 @@ msgstr "Passkeys" msgid "Passkeys let you sign in with your fingerprint, face, or device PIN." msgstr "Passkeys let you sign in with your fingerprint, face, or device PIN." -#: app/routes/login/method.tsx:255 +#: app/routes/login/method.tsx:428 #: app/routes/reauth.tsx:339 #: app/routes/reauth.tsx:377 #: app/routes/signup/password.tsx:229 @@ -543,8 +543,8 @@ msgstr "Password must contain an uppercase letter." msgid "Password sign-in isn't available for this account." msgstr "Password sign-in isn't available for this account." -#: app/routes/login/index.tsx:379 -#: app/routes/login/index.tsx:394 +#: app/routes/login/index.tsx:389 +#: app/routes/login/index.tsx:404 msgid "Phone" msgstr "Phone" @@ -702,7 +702,7 @@ msgstr "Sign out of" msgid "Sign out other sessions" msgstr "Sign out other sessions" -#: app/routes/login/index.tsx:579 +#: app/routes/login/index.tsx:589 msgid "Sign-in is currently unavailable for this account. Please contact your administrator." msgstr "Sign-in is currently unavailable for this account. Please contact your administrator." @@ -715,7 +715,7 @@ msgstr "Signed-in sessions on other devices can still be active. Sign out your o msgid "Signing in as" msgstr "Signing in as" -#: app/routes/login/method.tsx:187 +#: app/routes/login/method.tsx:344 msgid "Signing in as <0>{loginName}." msgstr "Signing in as <0>{loginName}." @@ -871,8 +871,8 @@ msgstr "Use your passkey to verify your identity." msgid "Use your security key to verify your identity." msgstr "Use your security key to verify your identity." -#: app/routes/login/index.tsx:380 -#: app/routes/login/index.tsx:395 +#: app/routes/login/index.tsx:390 +#: app/routes/login/index.tsx:405 #: app/routes/sso/ldap.tsx:76 msgid "Username" msgstr "Username" @@ -943,7 +943,7 @@ msgstr "We've sent a password reset link to <0>{0}" msgid "We've sent a verification link to <0>{0}" msgstr "We've sent a verification link to <0>{0}" -#: app/routes/login/index.tsx:417 +#: app/routes/login/index.tsx:427 msgid "Welcome" msgstr "Welcome" diff --git a/app/resources/login/login-decision.ts b/app/resources/login/login-decision.ts index 66b0fcea4f..18cfaf5b96 100644 --- a/app/resources/login/login-decision.ts +++ b/app/resources/login/login-decision.ts @@ -8,13 +8,6 @@ export type Decision = | { kind: 'redirect'; path: string; params?: Record } | { kind: 'error'; error: string }; -const SINGLE_TARGET: Record = { - passkey: '/login/passkey', - idp: '/sso', - password: '/login/password', - otp_email: '/login/verify/email', -}; - export function decideAfterIdentifier({ methods, settings, @@ -41,9 +34,11 @@ export function decideAfterIdentifier({ if (methods.includes('password') && settings.allowPassword) available.push('password'); if (methods.includes('otp_email') && emailDeliveryEnabled) available.push('otp_email'); - if (available.length >= 2) return { kind: 'redirect', path: '/login/method' }; - - if (available.length === 1) return { kind: 'redirect', path: SINGLE_TARGET[available[0]] }; + // Every account with at least one usable method goes to the chooser, which renders + // exactly the methods it has (and auto-starts the ones that need no form). Routing + // per-method from here is what sent sole-IdP users to a bare /sso page: this function + // is pure over method KINDS, so it has no idpId and could only name a static path. + if (available.length >= 1) return { kind: 'redirect', path: '/login/method' }; // available.length === 0: surface the most actionable error. if (methods.includes('password') && !settings.allowPassword) { diff --git a/app/resources/login/login.service.ts b/app/resources/login/login.service.ts index 4f45d02a97..42f70d0e7e 100644 --- a/app/resources/login/login.service.ts +++ b/app/resources/login/login.service.ts @@ -25,8 +25,13 @@ import type { LoginSettings, ProviderErrorCode } from '@/modules/auth/types'; import { ProviderError } from '@/modules/auth/types'; import { decideAfterIdentifier } from '@/resources/login/login-decision'; import { isEmailLike } from '@/resources/login/login.schema'; +import { + GHOST_METHODS, + emailDomain, + resolveGhostPolicyOrg, +} from '@/resources/login/method-options'; import { nextStepFromSession, threadParams } from '@/resources/shared/next-step-params'; -import { resolveOrg } from '@/resources/shared/resolve-org'; +import { resolveOrg, resolvePolicyOrg } from '@/resources/shared/resolve-org'; import { idpReturnUrls } from '@/resources/sso/idp-return-urls'; import { paths } from '@/routes/paths'; import { logAuthEvent, hashActor } from '@/server/observability'; @@ -69,6 +74,12 @@ export interface StartIdpInput { * is the real guard, this only improves the picker UX. */ reauthHint?: string; + /** + * The POLICY org this intent was decided under, when it differs from `organization`. + * Rides on the success URL so the callback gates `allowRegister` (and auto-creates) in the + * SAME org whose IdP list the caller picked from. See IdpReturnOpts.policyOrg. + */ + policyOrg?: string; /** MaxMind device-fingerprint token captured client-side; attached as session metadata. */ deviceTrackingToken?: string; } @@ -85,12 +96,21 @@ export type StartIdpResult = { ok: true; authUrl: string } | { ok: false; error: */ export async function startIdpIntent( provider: AuthProvider, - { idpId, origin, requestId, organization, reauthHint, deviceTrackingToken }: StartIdpInput + { + idpId, + origin, + requestId, + organization, + reauthHint, + policyOrg, + deviceTrackingToken, + }: StartIdpInput ): Promise { const slug = idpTypeToSlug(idpId) ?? idpId; const { success, failure } = idpReturnUrls(origin, slug, { requestId, organization, + policyOrg, deviceTrackingToken, }); // Guard the provider call: a ProviderError (Zitadel unavailable / IdP intent rejected) is the @@ -183,13 +203,6 @@ export type ResolveIdentifierResult = | ResolveIdentifierStartIdp | { ok: false; error: ResolveIdentifierError }; -/** Lowercased domain of an email-style identifier, or null when it is not an email. */ -function emailDomain(loginName: string): string | null { - const at = loginName.lastIndexOf('@'); - if (at <= 0 || at === loginName.length - 1) return null; - return loginName.slice(at + 1).toLowerCase(); -} - /** * Identifier step: resolve the user, create the ceremony session, decide the next * factor screen, and shape the ceremony-session entry + threaded query params. @@ -247,16 +260,30 @@ export async function resolveIdentifier( } } + // The caller's already-fetched settings describe `organization`, so they are reusable only + // while the resolved org still equals it (domain discovery above may have moved `org`). + // Shared by BOTH branches below: the known path saves an RPC with it, and the unknown path + // must save exactly the same one — an extra provider round-trip on one side only is a timing + // difference between "account exists" and "account does not", which is what this whole flag + // exists to erase. + const reusableSettings = + threadedSettings && org !== undefined && org === organization ? threadedSettings : undefined; + const user = await provider.findUser(loginName, org); if (!user) { // ignoreUnknownUsernames (settings-gated, DEFAULT-OFF): with the flag off this is - // byte-identical to before (USER_NOT_FOUND). With it on, proceed to the password - // step bound to the typed loginName but with NO user attached — the credential check - // then fails generically, identical to a wrong password for a real account. - // Org-first: resolve the org (explicit wins; else default org) before reading the policy that - // gates ignoreUnknownUsernames / disableLoginWithEmail. Reading raw `org` (often undefined - // here) returned INSTANCE settings, so these gates could diverge from the default org's policy. - const settings = await provider.getLoginSettings(await resolveOrg(provider, org)); + // byte-identical to before (USER_NOT_FOUND). With it on, proceed bound to the typed loginName + // but with NO user attached — the credential check then fails generically, identical to a + // wrong password for a real account. + // Resolve the policy org from the identifier's DOMAIN, not the default org — see + // resolveGhostPolicyOrg. Reading the default org here judged `nobody@acme.test` under a + // different policy than `mia@acme.test`, and when the two disagreed on `allowPassword` the + // redirect targets split (known → /login/method, ghost → /error): the exact account-existence + // oracle this flag exists to close. It also gates ignoreUnknownUsernames / + // disableLoginWithEmail, which now answer for the org that actually claims the address. + const settings = + reusableSettings ?? + (await provider.getLoginSettings(await resolveGhostPolicyOrg(provider, org, loginName))); if (settings.ignoreUnknownUsernames !== true) { // disableLoginWithEmail (settings-gated, DEFAULT-OFF): an email is never a valid loginname // under this policy, so the lookup already failed. Refine the generic not-found into a @@ -280,7 +307,28 @@ export async function resolveIdentifier( sessionEntryFromSession(ghostSession, { loginName, organization: org, requestId }) ); const ghostParams = new URLSearchParams(threadParams(loginName, requestId, org)); - return { ok: true, target: '/login/password', params: ghostParams, sessions: ghostSessions }; + // ROUTE THE GHOST THROUGH THE REAL DECISION, never a target of its own. + // + // This branch's ONLY job is to be indistinguishable from a real account, and a hardcoded + // '/login/password' stopped being that the moment every known account with >= 1 usable method + // started resolving to /login/method: known → /login/method, unknown → /login/password turns + // the redirect target into a perfect account-existence oracle and silently disables the very + // setting that put us here. The collision was the protection, so re-establish it structurally — + // ask decideAfterIdentifier the same question a PASSWORD-ONLY account asks (GHOST_METHODS), + // under the same settings, and take the same answer. An org that forbids passwords sends both + // to /error; every other org sends both to /login/method, whose loader serves the ghost the + // identical chooser (see routes/login/method.tsx's ghost-subject note). + const ghostDecision = decideAfterIdentifier({ + methods: [...GHOST_METHODS], + settings, + emailDeliveryEnabled, + context: { role: 'primary' }, + }); + if (ghostDecision.kind === 'redirect') { + Object.entries(ghostDecision.params ?? {}).forEach(([k, v]) => ghostParams.set(k, v)); + return { ok: true, target: ghostDecision.path, params: ghostParams, sessions: ghostSessions }; + } + return { ok: true, target: paths.error(), params: ghostParams, sessions: ghostSessions }; } logAuthEvent('identifier', 'success', { actor: hashActor(loginName) }); @@ -294,13 +342,18 @@ export async function resolveIdentifier( // pinned and domain-discovery didn't run (org === undefined), the user may live in a // different org than the default. Use user.orgId as the authoritative org so the // settings (allowPassword, passkeysType, …) match the user's actual policies. - // The caller's threadedSettings are only reused when both the caller pinned an explicit - // org AND the user's org matches it (org !== undefined && org === organization). - const userOrg = org ?? user.orgId; - const settings = - threadedSettings && org !== undefined && org === organization - ? threadedSettings - : await provider.getLoginSettings(userOrg); + // + // resolvePolicyOrg, NOT a bare `org ?? user.orgId`: `User.orgId` is OPTIONAL, and when it was + // absent this read fell through to INSTANCE settings while /login/method's loader — which + // re-computes the very availability decided here — fell through to the DEFAULT ORG's. Those + // two policies can disagree, and when they do this function approves a method the chooser then + // computes away, bouncing the user to /error. Both sides now share the one helper. + // `reusableSettings` (computed above, shared with the unknown-identifier branch) already + // encodes the only condition under which the caller's threaded settings describe this org: + // an explicit `organization` that survived discovery — in which case resolvePolicyOrg is the + // identity on `org` anyway. + const userOrg = await resolvePolicyOrg(provider, org, user.orgId); + const settings = reusableSettings ?? (await provider.getLoginSettings(userOrg)); const decision = decideAfterIdentifier({ methods, settings, diff --git a/app/resources/login/method-options.ts b/app/resources/login/method-options.ts new file mode 100644 index 0000000000..8773907b27 --- /dev/null +++ b/app/resources/login/method-options.ts @@ -0,0 +1,159 @@ +// app/resources/login/method-options.ts +// +// "What can this identified account actually sign in with RIGHT NOW?" — the resolution behind +// the /login/method chooser, extracted from the route loader so that loader stays a thin +// request/response shell (session gate → reads → decide → render). +// +// Applies the SAME policy gates decideAfterIdentifier (login-decision.ts) applies, plus the one +// thing that function structurally cannot know: whether the account's enrolled 'idp' method +// resolves to any actually-usable linked provider. decideAfterIdentifier is pure over method +// KINDS, so it counts 'idp' from `methods.includes('idp') && settings.allowExternalIdp` alone; +// only this resolution sees that the links may all be missing, deactivated, or LDAP-only. +import type { AuthProvider } from '@/modules/auth/auth-provider'; +import type { AuthMethod, IdProvider, LoginSettings } from '@/modules/auth/types'; +import { resolveOrg } from '@/resources/shared/resolve-org'; +import { getActiveIdPs } from '@/resources/sso/idp-providers'; +import { joinLinkedIdps, type LinkedIdpView } from '@/resources/sso/sso-management'; + +/** The four primary sign-in methods the chooser can offer. Closed set. */ +export type ChooserMethod = 'passkey' | 'password' | 'otp_email' | 'idp'; + +/** + * The enrolment the chooser SYNTHESIZES for a GHOST subject — an identifier that cleared + * /login/method's session gate but resolves to no account. + * + * `ignoreUnknownUsernames` is a tenant-facing anti-enumeration setting: with it on, /login's + * identifier step plants a real ceremony session for an UNKNOWN identifier and routes it to the + * SAME target a known account gets, so the response reveals nothing. That protection is ENTIRELY + * the indistinguishability, so the chooser must serve the ghost the very screen a real + * PASSWORD-ONLY account gets. + * + * Password, and only password, because it is the one method an unknown identifier can plausibly + * own end-to-end: the credential check at /login/password already fails generically for a ghost + * session (that is the whole design of the flag), whereas a passkey or IdP button would be an + * offer the ceremony cannot honour — and failing differently is exactly the leak we are closing. + * + * Fed through {@link resolveMethodOptions} like any other enrolment rather than short-circuiting + * the render, so an org that forbids passwords dead-ends the ghost at the same place it dead-ends + * a real password-only account instead of offering a button policy would refuse. + */ +export const GHOST_METHODS: readonly AuthMethod[] = Object.freeze(['password']); + +/** Lowercased domain of an email-style identifier, or null when it is not an email. */ +export function emailDomain(loginName: string): string | null { + const at = loginName.lastIndexOf('@'); + if (at <= 0 || at === loginName.length - 1) return null; + return loginName.slice(at + 1).toLowerCase(); +} + +/** + * The POLICY org a GHOST subject is judged under — the counterpart to {@link resolvePolicyOrg} + * for a subject that resolves to no account. + * + * WHY THIS EXISTS. A ghost has no `user.orgId`, so `resolvePolicyOrg`'s middle rung has nothing + * to bite on and the read fell through to the DEFAULT org — while a real account at the same + * address was judged by its OWN org. When those two policies disagreed the two branches took + * different routes (known → /login/method, ghost → /error for a default org with + * `allowPassword: false`), which is precisely the account-existence oracle + * `ignoreUnknownUsernames` exists to prevent. Both hops read this — the identifier decision + * (login.service.ts) and the chooser loader (routes/login/method.tsx) — so they cannot drift. + * + * Resolve from the identifier's DOMAIN instead: `nobody@acme.test` is then judged by the very + * org `mia@acme.test` belongs to. + * + * DELIBERATELY NOT GATED on `allowDomainDiscovery`. That flag governs whether a domain may ROUTE + * a ceremony (map an org and skip screens); this is a POLICY READ only — it never changes where + * anyone lands, only which org's settings decide it. Gating it would leave the oracle open for + * every tenant with discovery off, which is the default. + * + * ACCEPTED TRADE: an org whose domain is registered becomes distinguishable from one whose is + * not. That is ORG-existence, not ACCOUNT-existence — it says nothing about whether any + * particular person has an account, which is the property the flag protects. + * + * SIDE BENEFIT: the extra read balances the known branch's `listAuthMethods`, so the two + * branches now spend the same number of provider calls for an email-style identifier — closing + * the coarse timing channel they otherwise had. A NON-EMAIL ghost has no domain to look up and + * still spends one call fewer; that residual is unavoidable without a wasted round-trip. + * + * NOT error-guarded, on purpose: the known branch's `listAuthMethods` is not either, so a + * provider outage must surface identically on both paths rather than degrading one of them. + */ +export async function resolveGhostPolicyOrg( + provider: AuthProvider, + urlOrg: string | undefined, + loginName: string +): Promise { + // An explicit (or already domain-discovered) org wins outright — same precedence + // resolvePolicyOrg gives it, and the caller's threaded settings already describe it. + if (urlOrg !== undefined) return resolveOrg(provider, urlOrg); + const domain = emailDomain(loginName); + const hit = domain ? await provider.findOrgByDomain(domain) : null; + return resolveOrg(provider, hit?.orgId); +} + +export interface MethodOptions { + /** Policy-permitted, actually-usable primary methods, in chooser display order. */ + available: ChooserMethod[]; + /** This user's linked, active, non-LDAP providers — the buttons behind `available: ['idp']`. */ + idps: IdProvider[]; +} + +export interface MethodOptionsInput { + userId: string; + /** + * Enrolled methods (provider.listAuthMethods) — the caller already fetched them, or + * {@link GHOST_METHODS} for a subject that resolves to no account. + */ + methods: readonly AuthMethod[]; + /** Login settings read for the POLICY org (see resolvePolicyOrg). */ + settings: LoginSettings; + /** The POLICY org — the same one `settings` was read for. */ + policyOrg?: string; + /** env.AUTH_EMAIL_DELIVERY_ENABLED, threaded so this stays a pure-input decision. */ + emailDeliveryEnabled: boolean; +} + +/** + * Resolve the chooser's offer for one identified user. The only provider I/O is the linked-IdP + * join, and only when 'idp' is both enrolled and policy-allowed — a password-only account costs + * nothing extra. + */ +export async function resolveMethodOptions( + provider: AuthProvider, + { userId, methods, settings, policyOrg, emailDeliveryEnabled }: MethodOptionsInput +): Promise { + const available: ChooserMethod[] = []; + if (methods.includes('passkey') && settings.passkeysType !== 'not_allowed') { + available.push('passkey'); + } + + // Resolve the user's linked (redirect-based) IdPs directly into IdpButtonList's IdProvider + // shape — mirrors reauth.service.ts's loadReauth idp-resolution, including the LDAP exclusion + // (LDAP needs its own credential form, not an OAuth round-trip). A link whose provider is no + // longer active has no name/type to join — filtered out, since there is no sign-in button to + // offer for a dead provider. + let idps: IdProvider[] = []; + if (methods.includes('idp') && settings.allowExternalIdp) { + const [links, active] = await Promise.all([ + provider.listIdpLinks(userId), + getActiveIdPs(provider, policyOrg), + ]); + idps = joinLinkedIdps(links, active) + .filter( + (l): l is LinkedIdpView & { name: string; type: string } => + l.name !== undefined && l.type !== undefined && l.type !== 'LDAP' + ) + .map((l) => ({ id: l.idpId, name: l.name, type: l.type, logoUrl: l.logoUrl })); + if (idps.length > 0) available.push('idp'); + } + + if (methods.includes('password') && settings.allowPassword) available.push('password'); + if (methods.includes('otp_email') && emailDeliveryEnabled) available.push('otp_email'); + + return { available, idps }; +} + +/** True when the account's ONLY usable method is exactly one linked IdP (the auto-start case). */ +export function isSoleLinkedIdp({ available, idps }: MethodOptions): boolean { + return available.length === 1 && available[0] === 'idp' && idps.length === 1; +} diff --git a/app/resources/shared/resolve-org.ts b/app/resources/shared/resolve-org.ts index a0d2d00801..03f00bafce 100644 --- a/app/resources/shared/resolve-org.ts +++ b/app/resources/shared/resolve-org.ts @@ -63,3 +63,42 @@ export async function resolveOrg( ): Promise { return urlOrg ?? env.ZITADEL_DEFAULT_ORG_ID ?? (await getCachedDefaultOrg(provider)) ?? undefined; } + +/** + * The POLICY org for a POST-IDENTIFIER decision — i.e. every read (`getLoginSettings`, + * `getActiveIdPs`) that decides which sign-in methods an IDENTIFIED user may use. + * + * Precedence: the ceremony's explicit `?organization=` wins, else the org the FOUND USER + * actually belongs to, else {@link resolveOrg}'s default-org fallback. + * + * SHARED ON PURPOSE. `resolveIdentifier` (login.service.ts) decides a user has a usable method + * and routes them to /login/method; that route's loader then RE-computes the same availability. + * If the two resolve a different org they read different policies, and the loader can compute + * `available: []` for a user the decision just approved — a bounce to /error on the product's + * most travelled path. They previously agreed only for users WITH an `orgId`: `User.orgId` is + * optional, and when it was absent the decision fell through to INSTANCE settings while the + * loader fell through to the DEFAULT ORG's. Routing both through this one helper makes the + * agreement structural instead of coincidental. + * + * KNOWN, PRE-EXISTING: `urlOrg` is the raw `?organization=` query param and it WINS here, so + * whoever composes a URL — not only the ceremony that minted it — picks which org's policy + * decides an identified user's available methods. A permissive org named there can re-enable a + * method the user's own org disabled (`allowPassword`, `passkeysType`); a restrictive one can + * collapse their options to a single IdP. That is the trust level `?organization=` has always + * carried (every per-method screen was directly reachable with it long before this chooser + * existed) and the credential checks downstream are enforced by Zitadel, not by these settings — + * but note the chooser LOADER is a STATE-CHANGING consumer of it, since a collapsed option set + * can trigger the sole-IdP auto-start, which the per-method screens never did. Closing it means + * authenticating the param the way sso's `policyOrg` now is (see server/signed-param.ts); + * deliberately out of scope here because it predates and outlives this flow. + * + * A subject that resolves to NO account has no `userOrgId` for the middle rung and must not + * silently fall through to the default org — see `resolveGhostPolicyOrg` (login/method-options). + */ +export async function resolvePolicyOrg( + provider: AuthProvider, + urlOrg: string | undefined, + userOrgId: string | undefined +): Promise { + return resolveOrg(provider, urlOrg ?? userOrgId); +} diff --git a/app/resources/sso/idp-return-urls.ts b/app/resources/sso/idp-return-urls.ts index bd1160749a..6e9f8eb2f4 100644 --- a/app/resources/sso/idp-return-urls.ts +++ b/app/resources/sso/idp-return-urls.ts @@ -14,14 +14,34 @@ // APP_BASENAME is defined once in resources/shared; re-exported here for back-compat // (the sso barrel and this module's URL builder both consume it). import { APP_BASENAME } from '@/resources/shared/app-basename'; +import { signParam } from '@/server/signed-param'; export { APP_BASENAME }; +/** + * HMAC purpose tag for `policyOrg`. Shared with the callback that verifies it — a mismatch here + * is a fail-closed (the callback falls back to `organization`), never an accepted forgery. + */ +export const POLICY_ORG_PURPOSE = 'sso.policyOrg'; + export interface IdpReturnOpts { /** Original OIDC/SAML request to resume after the IdP round-trip (e.g. `oidc_…`). */ requestId?: string; /** Org scope to carry through the ceremony. */ organization?: string; + /** + * The POLICY org this intent was decided under, when it differs from the ceremony + * `organization` (which is the RAW `?organization=` the user arrived with, often absent). + * + * The callback resolves the org it reads `allowRegister` from — and auto-creates into — from + * `organization`, which on a bare flow falls back to the DEFAULT org. A caller that picked + * this IdP out of a list resolved for a DIFFERENT org (e.g. /login/method resolving the found + * user's own org) would otherwise have its start decided under one policy and its landing + * decided under another. Carrying it separately fixes that divergence WITHOUT widening + * `organization`, whose absence is what keeps the callback's `findUser` lookups instance-wide + * (see the note at sso-callback.ts's `callbackOrg`). + */ + policyOrg?: string; /** Account-linking flow (sets `?link=true` for the callback). */ link?: boolean; /** @@ -45,6 +65,22 @@ export function idpReturnUrls( if (opts?.link) query.set('link', 'true'); if (opts?.requestId) query.set('requestId', opts.requestId); if (opts?.organization) query.set('organization', opts.organization); + // Only when it actually differs — an identical value would be pure URL noise, and omitting it + // keeps every existing caller's success URL byte-identical. + // + // SIGNED, because this param is the one thing on the URL that is NOT already coupled to + // something else. `organization` drags the callback's same-email findUser scope, the created + // session's orgId and signInWithIdpIntent along with it, so naming a foreign org there costs an + // attacker the whole flow (an accidental fail-closed). `policyOrg` drags nothing: it feeds + // `callbackOrg` alone, which is what gates `allowRegister` and picks the auto-create org. A + // hand-written `?policyOrg=` would therefore borrow that org's + // registration policy while leaving the same-email lookup instance-wide — with + // ALLOW_IDP_AUTO_LINK on, a path into a victim's account in ANY org. Length validation cannot + // help: decoupling those two orgs is the param's whole purpose, so only provenance can. + if (opts?.policyOrg && opts.policyOrg !== opts.organization) { + query.set('policyOrg', opts.policyOrg); + query.set('policyOrgSig', signParam(POLICY_ORG_PURPOSE, opts.policyOrg)); + } if (opts?.deviceTrackingToken) query.set('deviceTrackingToken', opts.deviceTrackingToken); const qs = query.toString(); // The failure URL MUST carry requestId/organization too — the IdP broker redirects here diff --git a/app/resources/sso/sso-callback.ts b/app/resources/sso/sso-callback.ts index 8e3720b313..faa62f7be6 100644 --- a/app/resources/sso/sso-callback.ts +++ b/app/resources/sso/sso-callback.ts @@ -23,10 +23,12 @@ import { registerAndLinkIdp } from '@/resources/signup'; import { MAXMIND_TRACKING_TOKEN_METADATA_KEY } from '@/resources/signup/signup.service'; import { deriveIdpProfileName } from '@/resources/sso/derive-idp-name'; import { decideIdpCallback } from '@/resources/sso/idp-callback'; +import { POLICY_ORG_PURPOSE } from '@/resources/sso/idp-return-urls'; import { signInWithIdpIntent, requestScopedProviderReads } from '@/resources/sso/idp-session'; import type { SsoOutcome } from '@/resources/sso/sso-outcome'; import { env } from '@/server/infra/env.server'; import { logAuthEvent } from '@/server/observability'; +import { verifyParam } from '@/server/signed-param'; import { getOrCreateFingerprintId, userAgentFromRequest } from '@/server/user-agent'; import { providerErrorCode } from '@/utils/errors/auth-error'; import { z } from 'zod'; @@ -48,6 +50,19 @@ export const CallbackQuery = z.object({ // Attacker-controllable from the callback URL; POSTURE B2 is fail-closed against a wrong org, but // cap the length as defense-in-depth (Zitadel org ids are short numeric strings). organization: z.string().max(64).optional(), + // The POLICY org the START side decided this intent under (idp-return-urls.ts). Feeds + // `callbackOrg` ONLY — never the findUser lookups below, which stay on raw `organization`. + // + // That asymmetry is exactly why it must be AUTHENTICATED and not merely capped. `organization` + // is attacker-controllable too, but naming a foreign org there also narrows the same-email + // lookup and poisons createSession's orgId — it fails closed. `policyOrg` is coupled to none of + // that: it decides `allowRegister` and the auto-create org and nothing else, so an unsigned one + // is a way to borrow a registration-open org's policy while keeping the lookup instance-wide. + // Verified against `policyOrgSig` below; anything that fails falls back to `organization`. + policyOrg: z.string().max(64).optional(), + // Detached HMAC over `policyOrg`, minted by idpReturnUrls with SESSION_SECRET. base64url + // SHA-256 is 43 chars; the cap is slack, not a check — verifyParam is the check. + policyOrgSig: z.string().max(128).optional(), // MaxMind device-fingerprint token captured client-side and threaded through the OAuth // round-trip (see idp-return-urls.ts) so it can be attached to the resulting session's metadata. deviceTrackingToken: z.string().optional(), @@ -94,7 +109,28 @@ export async function processIdpCallback( }; } - const { id, token, link, requestId, organization, deviceTrackingToken } = parsed.data; + const { id, token, link, requestId, organization, policyOrg, policyOrgSig, deviceTrackingToken } = + parsed.data; + + // AUTHENTICATE the policy org before it is allowed to influence anything. An unsigned or + // mis-signed value is treated as absent — the callback falls back to `organization`, i.e. to + // exactly the behavior it had before the param existed. Logged (org ids are not PII) because a + // signature failure here is either tampering or a SESSION_SECRET rotation mid-flight, and both + // are worth seeing. + let verifiedPolicyOrg: string | undefined; + if (policyOrg) { + if (verifyParam(POLICY_ORG_PURPOSE, policyOrg, policyOrgSig)) { + verifiedPolicyOrg = policyOrg; + } else { + // snake_case per the P5+ naming convention, and the same `invalid_signature` reason the + // `sessions` cookie's tamper signal uses — one vocabulary for "we did not mint this". + logAuthEvent('idp_policy_org', 'failure', { + reason: 'invalid_signature', + requestId, + policyOrg, + }); + } + } // Ensure the fingerprintId cookie exists for this browser. The SAME minted id feeds // every createSession userAgent below (no first-session gap); fingerprintCookie is @@ -115,14 +151,22 @@ export async function processIdpCallback( let intent: IdpIntentResult; let entries: Awaited>; let decision: ReturnType; - // Resolve the effective org for this callback ceremony — org-first (URL ?organization=), - // then ZITADEL_DEFAULT_ORG_ID env pin, then the provider's instance Default Organization. + // Resolve the effective org for this callback ceremony — policy-org-first (the org the START + // side decided the intent under, see idp-return-urls.ts), then the URL `?organization=`, then + // the ZITADEL_DEFAULT_ORG_ID env pin, then the provider's instance Default Organization. // Hoisted before the try block so both the decision try and the auto-create switch case // can reference it. This resolved org feeds the allowRegister gate (getLoginSettings) and // the auto-create register call so a bare (no ?organization=) flow always has a concrete // org, preventing Zitadel's FAILED_PRECONDITION on addHumanUser with no org. - // NOTE: findUser calls below deliberately stay on raw `organization` (instance-wide lookup). - const callbackOrg = await resolveOrg(provider, organization); + // + // `policyOrg` participates HERE and nowhere else. Without it, a start that resolved its IdP + // list under the found user's own org (e.g. /login/method's sole-linked-IdP auto-start) landed + // here with `organization` still undefined and had allowRegister decided by the DEFAULT org — + // a different policy from the one that offered the provider in the first place. + // NOTE: findUser calls below deliberately stay on raw `organization` (instance-wide lookup), + // which is exactly why the policy org rides in its OWN param instead of widening this one — + // and why only the SIGNATURE-VERIFIED value may be used here. + const callbackOrg = await resolveOrg(provider, verifiedPolicyOrg ?? organization); try { intent = await doRetrieveIdpIntent(id, token); diff --git a/app/routes/login/index.tsx b/app/routes/login/index.tsx index 6760436b2a..14590ab742 100644 --- a/app/routes/login/index.tsx +++ b/app/routes/login/index.tsx @@ -15,6 +15,7 @@ import { idpTypeToSlug } from '@/modules/auth/idp-slug'; // The locked plan block incorrectly listed them as coming from @/modules/auth/session/session // (that module only has pure helpers, no cookie I/O). import { readSessions, serializeSessions } from '@/modules/auth/session/cookie'; +import { clearIdpAutostart } from '@/modules/auth/session/idp-autostart'; import { readLastUsedLogin } from '@/modules/auth/session/last-used-login'; import { readPasskeyHint, clearPasskeyHint } from '@/modules/auth/session/passkey-hint'; import { readReauthIntent } from '@/modules/auth/session/reauth-intent'; @@ -253,6 +254,13 @@ export async function action({ request }: ActionFunctionArgs) { const headers = new Headers(); headers.append('set-cookie', await serializeSessions(result.sessions)); if (fpCookie) headers.append('set-cookie', fpCookie); + // A fresh ceremony starts here, so retire any one-shot IdP auto-start marker the previous one + // left behind — otherwise a sign-out/sign-in inside its 10-minute window suppressed the very + // auto-start the marker exists to protect. Unconditional, and identical on both the known and + // ghost branches: expiring an already-absent cookie is a no-op for the browser, and branching + // on its presence would both cost a read and make the two branches' headers differ. See + // clearIdpAutostart. + headers.append('set-cookie', await clearIdpAutostart()); // Domain-discovery single-IdP org: start the IdP intent directly. A plain redirect to /sso // (the old behavior) dead-ends session-less users in a /login ↔ /sso bounce loop. if ('startIdp' in result) { @@ -266,10 +274,12 @@ export async function action({ request }: ActionFunctionArgs) { if (!idpResult.ok) return data({ error: idpResult.error }, { status: 502 }); return redirect(idpResult.authUrl, { headers }); } - // Sole-passkey: redirect to /login/passkey like every other post-identifier path - // (password, OTP) — - // /login renders the chooser and nothing else; no route inlines an identity-bound, - // single-method screen except /login/method and /reauth, which are out of scope here. + // Every account with >= 1 usable method now resolves to the same target — /login/method — + // which renders exactly what that account has and auto-starts what needs no form (a sole + // linked IdP server-side in its loader, a sole passkey client-side on mount). This route + // deliberately picks no per-method screen: it is pure over method KINDS, so it has no idpId + // and could only ever name a static path (that is what sent sole-IdP users to a bare /sso). + // The zero-method legs (/verify, /error) are unchanged. return redirect(`${result.target}?${result.params}`, { headers }); } diff --git a/app/routes/login/method.tsx b/app/routes/login/method.tsx index ed93291411..710bb9aacd 100644 --- a/app/routes/login/method.tsx +++ b/app/routes/login/method.tsx @@ -5,26 +5,36 @@ import { useAuthActionError } from '@/hooks/use-auth-action-error'; import { useLoginContext } from '@/hooks/use-login-context'; import { usePasskeyLoginCeremony } from '@/hooks/use-passkey-login-ceremony'; import SplitLayout from '@/layouts/split.layout'; -import type { IdProvider } from '@/modules/auth/types'; +import { listSessions, readSessions } from '@/modules/auth/session/cookie'; +import { + idpAutostartMatches, + readIdpAutostart, + serializeIdpAutostart, +} from '@/modules/auth/session/idp-autostart'; import { startIdpIntent } from '@/resources/login'; -import { decideAfterIdentifier } from '@/resources/login/login-decision'; import { loginIdpSchema } from '@/resources/login/login.schema'; +import { + GHOST_METHODS, + isSoleLinkedIdp, + resolveGhostPolicyOrg, + resolveMethodOptions, +} from '@/resources/login/method-options'; import { readCeremonyParams } from '@/resources/shared/ceremony-params'; -import { resolveOrg } from '@/resources/shared/resolve-org'; +import { resolveOrg, resolvePolicyOrg } from '@/resources/shared/resolve-org'; import { joinLinkedIdps } from '@/resources/sso'; import { getActiveIdPs } from '@/resources/sso/idp-providers'; -import type { LinkedIdpView } from '@/resources/sso/sso-management'; import { redirectToLogin } from '@/routes/login-bounce'; import { paths } from '@/routes/paths'; import { providerForRequest } from '@/server/auth-context.server'; import { loaderCsrf, assertCsrf } from '@/server/csrf'; import { trustedAppOrigin } from '@/server/infra/app-origin.server'; import { env } from '@/server/infra/env.server'; -import { Button, LinkButton } from '@datum-cloud/datum-ui/button'; +import { LinkButton } from '@datum-cloud/datum-ui/button'; import { Icon } from '@datum-cloud/datum-ui/icons'; import { cn } from '@datum-cloud/datum-ui/utils'; import { Trans } from '@lingui/react/macro'; import { Key, Lock, Mail } from 'lucide-react'; +import { useEffect, useRef, useState } from 'react'; import { data, redirect, @@ -41,69 +51,164 @@ export const meta: MetaFunction = () => [{ title: 'Choose how to sign in' }]; export async function loader({ request }: LoaderFunctionArgs) { const provider = providerForRequest(request); const { loginName, requestId, organization } = readCeremonyParams(new URL(request.url)); + const bounce = () => redirect(redirectToLogin(requestId, organization)); - if (!loginName) return redirect(redirectToLogin(requestId, organization)); + if (!loginName) return bounce(); + + // ── SESSION GATE ──────────────────────────────────────────────────────────────────────── + // This loader is state-changing (it mints a real Zitadel IdP intent for a sole-linked-IdP + // account and 302s to the provider) and its outcomes are distinguishable from outside: + // 302-to-a-named-provider (sole IdP) vs 200 (several methods) vs 302-to-/verify|/error (none). + // Before the gate an unknown identifier added a fourth, 302-to-/login. Reachable by URL alone + // that made + // `GET /id/login/method?loginName=X` an account-existence AND identity-provider oracle, a + // login-CSRF vector (no token on a GET), and a bypass of ignoreUnknownUsernames — which is + // honoured only in resolveIdentifier (login.service.ts). + // + // Requiring the ceremony session the identifier step ALREADY planted for this loginName costs + // legitimate arrivals nothing: /login's identifier action serializes `result.sessions` into the + // cookie on the same response that redirects here, and every other path to this URL (the e2e + // helper's chooser hop, a Back from a later step, a reload) carries that same cookie. + // + // WHAT THIS GATE IS AND IS NOT. It is NOT an authorization boundary: the cookie it demands is + // SELF-ISSUABLE — any party can POST /id/login (CSRF-protected, so not forceable cross-site, + // but self-driven probing only needs to fetch the token first) and be handed one. So the gate + // raises the cost of reaching this loader by exactly one request; what actually bounds + // enumeration BREADTH is the two-tier GET limiter in server/middleware/rate-limit.ts, now + // backed by an ip ceiling on the identifier POST itself. What the gate genuinely buys is that + // `GET /id/login/method?loginName=X` alone — with no cookie at all — mints nothing and tells + // nobody anything, which is what makes the URL safe to hand out, bookmark and reload. + // + // NOT-KNOWN-EXPIRED, not "live": listSessions is the codebase's expiry-aware filter (the same + // idiom arm-login-passkey.ts uses) but it deliberately KEEPS entries whose expirationTs is + // empty or unparseable (session.ts) — and Zitadel Session-API sessions minted without an + // explicit lifetime, which is exactly what the ceremony createSession produces, carry no + // expirationDate. So a known-expired entry cannot pass, but an unknown-expiry one can. + // Case-insensitive: the URL carries the provider's canonical loginName while a hand-typed or + // IdP-returned identifier may differ in case. + const sessions = listSessions(await readSessions(request), Date.now()); + if (!sessions.some((s) => s.loginName.toLowerCase() === loginName.toLowerCase())) { + return bounce(); + } + // ── GHOST SUBJECT (anti-enumeration) ──────────────────────────────────────────────────── + // A findUser MISS here does NOT mean "no such account, bounce". `ignoreUnknownUsernames` makes + // /login's identifier step plant a real ceremony session for an UNKNOWN identifier and route it + // to THIS url — the same one a real password-only account is routed to — precisely so the two + // are indistinguishable. Bouncing on the miss would hand that difference straight back: a + // 302-to-/login for the unknown vs a 200 chooser for the known is a perfect existence oracle, + // and it would silently disable a security setting the tenant switched on. + // + // So a miss that CLEARED THE SESSION GATE above is served the password-only chooser (see + // GHOST_METHODS): same status, same single control, same branding resolution, same rotated CSRF + // token, same headers. Its Password link leads to /login/password, where the ghost session's + // credential check fails generically — exactly as it did before this route existed. + // + // The gate is what makes this safe to serve at all: the ONLY way to reach it is a live ceremony + // session this browser was handed for this loginName, which /login mints only under the flag. const user = await provider.findUser(loginName, organization); - if (!user) return redirect(redirectToLogin(requestId, organization)); // The method chooser is a branded screen — thread getBranding through so SplitLayout // renders the org logo, mirroring /login and /signup. Fetched in parallel with the rest. // Org-first: an explicit org wins, else the default org (matches the old app's // `organization ?? getDefaultOrg()`). findUser above stays instance-wide by design. - const settingsOrg = await resolveOrg(provider, organization); + const brandingOrg = await resolveOrg(provider, organization); + // POLICY reads (login settings + the org's active IdPs) must resolve to the SAME org + // resolveIdentifier used when it routed the user here. Both sides now call resolvePolicyOrg, + // so that agreement is structural rather than two look-alike expressions kept in sync by hand + // (they previously diverged for a user with NO orgId: instance settings there, default-org + // settings here). Branding is deliberately left org-first — the logo follows the ceremony's + // org, not the user's. Sequenced after brandingOrg so the memoized default-org lookup is + // already warm and this costs no second provider round-trip. + // A GHOST has no orgId for resolvePolicyOrg's middle rung to bite on, so this used to fall + // through to the DEFAULT org while a real account at the same address was judged by its own. + // That split `available` — and with it the render-vs-/error outcome — between the two branches, + // re-opening the existence oracle one hop after resolveIdentifier closed it. Both hops now + // resolve a ghost's policy org from the identifier's domain (see resolveGhostPolicyOrg), so the + // decision that routed the subject here and the one recomputed here read the same org either way. + const settingsOrg = user + ? await resolvePolicyOrg(provider, organization, user.orgId) + : await resolveGhostPolicyOrg(provider, organization, loginName); const [methods, settings, branding, { csrfToken, headers }] = await Promise.all([ - provider.listAuthMethods(user.id), + // A ghost has no id to enumerate methods for; it carries the synthesized password-only + // enrolment instead, and every read below is the one a real account makes. + user ? provider.listAuthMethods(user.id) : Promise.resolve([...GHOST_METHODS]), provider.getLoginSettings(settingsOrg), - provider.getBranding(settingsOrg), + provider.getBranding(brandingOrg), loaderCsrf(request), ]); - // Compute available primary sign-in methods using the same policy gates as - // decideAfterIdentifier (login-decision.ts). This screen only appears when - // available.length >= 2; we surface exactly those available methods. - const available: Array<'passkey' | 'password' | 'otp_email' | 'idp'> = []; - if (methods.includes('passkey') && settings.passkeysType !== 'not_allowed') - available.push('passkey'); + // Which methods this account can actually use right now (same policy gates as + // decideAfterIdentifier, plus the linked-IdP resolution that function cannot see). + // This screen is the destination for EVERY account with >= 1 available method. + const { available, idps } = await resolveMethodOptions(provider, { + // Never read for a ghost: GHOST_METHODS carries no 'idp', so the linked-provider join + // (the only place userId is used) does not run. + userId: user?.id ?? '', + methods, + settings, + policyOrg: settingsOrg, + emailDeliveryEnabled: env.AUTH_EMAIL_DELIVERY_ENABLED, + }); - // Resolve the user's linked (redirect-based) IdPs directly into IdpButtonList's - // IdProvider shape — mirrors reauth.service.ts's loadReauth idp-resolution, including - // the LDAP exclusion (LDAP needs its own credential form, not an OAuth round-trip). A - // link whose provider is no longer active has no name/type to join — filtered out, - // since there's no sign-in button to offer for a dead provider. - let idps: IdProvider[] = []; - if (methods.includes('idp') && settings.allowExternalIdp) { - const [links, active] = await Promise.all([ - provider.listIdpLinks(user.id), - getActiveIdPs(provider, settingsOrg), - ]); - idps = joinLinkedIdps(links, active) - .filter( - (l): l is LinkedIdpView & { name: string; type: string } => - l.name !== undefined && l.type !== undefined && l.type !== 'LDAP' - ) - .map((l) => ({ id: l.idpId, name: l.name, type: l.type, logoUrl: l.logoUrl })); - if (idps.length > 0) available.push('idp'); + // Sole linked IdP: start the intent here rather than rendering a one-button screen. + // Server-side, so the chooser never renders and the user sees no intermediate page. + // + // ONE-SHOT. Being in a LOADER is what buys the no-flash redirect, and it is also what makes + // this dangerous: a loader re-runs on every arrival at the URL, including the one the browser + // makes when the user presses Back at the provider. Unguarded, that Back mints a NEW Zitadel + // intent and bounces them forward again — they can never get back into the app. The marker + // cookie makes the second arrival for the same loginName fall through and RENDER the chooser, + // whose IdP button restarts the ceremony deliberately (a POST to this route's action, where + // Back does not re-fire — which is also why /login's domain-discovery branch, an action, never + // needed a guard like this one). + if (isSoleLinkedIdp({ available, idps })) { + const alreadyAutoStarted = idpAutostartMatches(await readIdpAutostart(request), loginName); + if (!alreadyAutoStarted) { + const idpResult = await startIdpIntent(provider, { + idpId: idps[0].id, + origin: trustedAppOrigin(request), + requestId, + organization, + // The IdP was picked out of a list resolved for settingsOrg; the callback must gate + // allowRegister in that same org rather than in whatever `organization` resolves to. + policyOrg: settingsOrg, + reauthHint: loginName, + }); + // On failure fall through and render the button, so the user keeps a way forward. + // The marker is written only on the redirect that actually mints — a failed start has + // nothing to come Back from. + if (idpResult.ok) { + return redirect(idpResult.authUrl, { + headers: { 'set-cookie': await serializeIdpAutostart(loginName) }, + }); + } + } } - if (methods.includes('password') && settings.allowPassword) available.push('password'); - if (methods.includes('otp_email') && env.AUTH_EMAIL_DELIVERY_ENABLED) available.push('otp_email'); - - // Defensive: if for some reason < 2 methods are available, run decideAfterIdentifier - // and redirect to the single target (or /error). Consume the Decision union by - // `kind` — 'redirect' → its path, 'error' → /error. - if (available.length < 2) { - const decision = decideAfterIdentifier({ - methods, - settings, - emailDeliveryEnabled: env.AUTH_EMAIL_DELIVERY_ENABLED, - context: { role: 'primary' }, // post-identifier decision is the primary flow - }); - const params = new URLSearchParams({ loginName }); - if (requestId) params.set('requestId', requestId); - if (organization) params.set('organization', organization); - const target = decision.kind === 'redirect' ? decision.path : paths.error(); - return redirect(`${target}?${params.toString()}`); + // Nothing usable. Exactly two outcomes are reachable from here, so name them directly + // rather than asking decideAfterIdentifier and filtering its answer: no enrolled methods + // at all is the invite path (/verify); anything else is a policy dead end (/error — the + // PASSWORD_NOT_ALLOWED / NO_SUPPORTED_METHOD legs, which carry no params of their own). + // + // A GHOST reaches this only where a real password-only account would: its synthesized + // enrolment is non-empty, so an org that forbids passwords sends both to /error, and neither + // can reach the /verify leg. (In practice resolveIdentifier already sent both there — this is + // the direct-URL arrival with a still-live session.) + // + // Deciding it here is what makes a self-redirect loop UNREPRESENTABLE. decideAfterIdentifier + // counts 'idp' as available from methods.includes('idp') && settings.allowExternalIdp alone + // — it has no visibility into the resolved `idps` list above, which is the ONLY source of + // truth this route has for actually-usable linked IdPs. A user whose sole enrolled method is + // 'idp' but whose links are all missing, inactive, or LDAP-only lands here (our own + // `available` is []) while that function still names THIS route. Following it would 302 into + // ourselves forever (same user, same inputs, same empty `available` every time); neither + // destination below can name /login/method, so there is nothing left to guard against. + if (available.length === 0) { + // Same query the manual URLSearchParams built (loginName, then requestId, then + // organization; undefined values skipped) — paths.* threads the ceremony for us. + const query = { loginName, requestId, organization }; + return redirect(methods.length === 0 ? paths.verify.index(query) : paths.error(query)); } return data( @@ -136,7 +241,11 @@ export async function action({ request }: ActionFunctionArgs) { // resolves to one of THIS identified user's own linked, active, non-LDAP providers // before starting the round-trip. Mirrors the same check reauth.tsx's idp-reauth // action uses for its own linked-IdP chooser. - const settingsOrg = await resolveOrg(provider, organization); + // + // Resolved through the SAME helper the loader uses, for the same reason: this check must + // accept precisely the set of buttons the loader rendered. Reading a different org's active + // IdPs here would 400 a legitimate click on a provider the loader itself just offered. + const settingsOrg = await resolvePolicyOrg(provider, organization, user.orgId); const [links, active] = await Promise.all([ provider.listIdpLinks(user.id), getActiveIdPs(provider, settingsOrg), @@ -151,12 +260,32 @@ export async function action({ request }: ActionFunctionArgs) { origin: trustedAppOrigin(request), requestId, organization, + // Same divergence the loader's auto-start closes: the button was offered from settingsOrg's + // IdP list, so the callback must gate allowRegister in settingsOrg too. + policyOrg: settingsOrg, reauthHint: loginName, }); if (!result.ok) return data({ error: result.error }, { status: 502 }); return redirect(result.authUrl); } +// This loader MINTS CEREMONY STATE — a Zitadel IdP intent for a sole-linked-IdP account, a +// rotated loaderCsrf token, the one-shot auto-start marker — and costs ~5 provider reads plus a +// rate-limit unit per run. The in-place passkey ceremony below submits its assertion to +// /login/passkey while THIS route stays mounted, and RR's default post-submit revalidation would +// re-run all of that on every ceremony step. Same guard, same marker, and the same WEBAU-3M9si +// class of hazard as login/index.tsx:284 and /login/passkey:40. Anything else revalidates normally. +export function shouldRevalidate({ + formData, + defaultShouldRevalidate, +}: { + formData?: FormData; + defaultShouldRevalidate: boolean; +}) { + if (formData?.get('passkeyCeremony') === '1') return false; + return defaultShouldRevalidate; +} + export default function LoginMethod() { const { methods, branding, idps, csrfToken } = useLoaderData(); const { loginName, requestId, organization } = useLoginContext(); @@ -169,6 +298,34 @@ export default function LoginMethod() { const serverError = useAuthActionError(ceremony.actionData); const passkeyBusy = ceremony.phase !== 'idle'; + // Sole passkey: the identifier step already captured intent, so run the ceremony on + // arrival instead of rendering a one-button screen. One-shot — a cancelled ceremony + // must leave the button usable rather than re-prompting in a loop. + const solePasskey = methods.length === 1 && methods[0] === 'passkey'; + const autoBegunRef = useRef(false); + // The auto-begun attempt has no transient user activation behind it (no click preceded the + // mount), and WebKit REQUIRES activation for navigator.credentials.get() — so on Safari it + // reliably rejects with NotAllowedError, which the ceremony classifies as 'not-allowed'. + // That reason's copy tells the user their passkey sign-in "was cancelled", accusing someone + // who has done nothing but arrive. Suppress the copy for THIS attempt only; the Passkey + // button below stays as the way forward, and a ceremony the user starts by clicking reports + // its failure exactly as it does on every other surface. + const [suppressAutoBeginReason, setSuppressAutoBeginReason] = useState(false); + useEffect(() => { + if (!solePasskey || autoBegunRef.current) return; + autoBegunRef.current = true; + setSuppressAutoBeginReason(true); + ceremony.begin(); + }, [solePasskey, ceremony]); + + // Any user-initiated ceremony re-arms the copy — begin() clears the previous reason, so the + // next one to land belongs to an attempt the user asked for. + const beginFromClick = () => { + setSuppressAutoBeginReason(false); + ceremony.begin(); + }; + const ceremonyReason = suppressAutoBeginReason ? null : ceremony.reason; + // Mirrors login/index.tsx's own submittingIdpId computation — drives IdpButtonList's // per-row loading state while its POST to this route's own action is in flight. const navigation = useNavigation(); @@ -193,9 +350,9 @@ export default function LoginMethod() { Not you?

- {ceremony.reason ? ( + {ceremonyReason ? ( - + ) : serverError ? ( {serverError} @@ -204,22 +361,38 @@ export default function LoginMethod() {
{methods.includes('passkey') ? ( - // Button (not LinkButton) — fires the ceremony IN PLACE (lazy challenge; - // a password pick never spends a session) instead of navigating to - // /login/passkey. The list stays visible as the fallback on failure. - + ) : null} {methods.includes('otp_email') ? ( diff --git a/app/server.ts b/app/server.ts index a66940305c..ca37afb13f 100644 --- a/app/server.ts +++ b/app/server.ts @@ -13,6 +13,9 @@ import { reauthRateLimit, passkeysRateLimit, loginMethodRateLimit, + loginMethodIntentRateLimit, + loginMethodIntentIpRateLimit, + loginIdentifierRateLimit, } from '@/server/middleware/rate-limit'; import { requestContext, type RequestContextEnv } from '@/server/middleware/request-context'; import { appSecureHeaders, resolveFrameAncestors } from '@/server/middleware/secure-headers'; @@ -108,6 +111,9 @@ export default await createHonoServer({ // normalized to lowercase by `normalizedPathname` in net.ts (toLowerCase + trailing-slash // strip + .data strip). Requests that do not match call `next()` immediately with no // meaningful overhead beyond a pathname parse. + // The identifier submit is what hands out the ceremony session /id/login/method's gate + // demands, so it needs its own ceiling — see the rationale in rate-limit.ts. + app.use('*', loginIdentifierRateLimit); app.use('*', loginPasswordRateLimit); app.use('*', signupRateLimit); app.use('*', passwordResetRateLimit); @@ -120,6 +126,10 @@ export default await createHonoServer({ app.use('*', reauthRateLimit); app.use('*', passkeysRateLimit); app.use('*', loginMethodRateLimit); + // The chooser GET is counted by BOTH tiers (tight ip|loginName, loose ip ceiling) — + // see the two-tier rationale in rate-limit.ts. + app.use('*', loginMethodIntentRateLimit); + app.use('*', loginMethodIntentIpRateLimit); app.get('/healthz', (c) => c.json({ status: 'ok' })); app.get('/readyz', (c) => c.json({ status: 'ready' })); app.get('/security', (c) => diff --git a/app/server/middleware/rate-limit.ts b/app/server/middleware/rate-limit.ts index 84e9517ea8..dd4de62fa8 100644 --- a/app/server/middleware/rate-limit.ts +++ b/app/server/middleware/rate-limit.ts @@ -83,6 +83,37 @@ export const verifyEmailSendRateLimit: MiddlewareHandler = createRateLimit({ key: (_c, ip) => ip, }); +// One shared limiter for the IDENTIFIER submit, POST /id/login: 120 attempts / 5 min per ip. +// +// This endpoint was the one unthrottled member of the auth surface, and the chooser work made +// that load-bearing rather than incidental: /id/login/method's session gate demands a ceremony +// session for the probed loginName, and THIS is the endpoint that hands one out. Unthrottled, a +// prober mints cookies for as many addresses as they like and the gate costs them a single extra +// request each. It is also the endpoint that reveals existence directly whenever +// ignoreUnknownUsernames is off (302 vs an inline USER_NOT_FOUND). +// +// IP-ONLY, deliberately. The loginName is in the POST body (body-stream hazard — the RR7 action +// downstream calls `await request.formData()`, and a Hono request body reads once), so a tight +// ip|loginName tier would need a `c.req.raw.clone()` and an async key, which createRateLimit's +// synchronous `key` contract does not take. An ip-only ceiling is what bounds enumeration +// BREADTH anyway; per-account brute force is not the threat at this step (no credential is +// submitted here) and Zitadel's failedAttempts policy backstops the password step that follows. +// +// 120/5min MIRRORS loginMethodIntentIpRateLimit on purpose — same endpoint class (one hit per +// sign-in, on the most travelled path in the product), so the two halves of the same ceremony +// cannot disagree about what a busy NAT'd office looks like. ~24 identifier submits/minute from +// a single egress IP. +const loginIdentifierLimiter = new RateLimiter({ limit: 120, windowMs: 5 * 60_000 }); + +// Mounted on '*' in server.ts. Self-guards on POST + the normalized path (which strips the RR7 +// single-fetch `.data` suffix, so the hydrated submit is counted too). +export const loginIdentifierRateLimit: MiddlewareHandler = createRateLimit({ + limiter: loginIdentifierLimiter, + match: (c, pathname) => c.req.method === 'POST' && pathname === '/id/login', + key: (_c, ip) => ip, + logFields: (c, ip, pathname) => ({ ip, path: pathname }), +}); + // One shared limiter for /login/password: 5 attempts / 5 min per (ip + loginName). const passwordLimiter = new RateLimiter({ limit: 5, windowMs: 5 * 60_000 }); @@ -292,3 +323,49 @@ export const loginMethodRateLimit: MiddlewareHandler = createRateLimit({ match: (c, pathname) => c.req.method === 'POST' && pathname === '/id/login/method', key: (_c, ip) => ip, }); + +// The /id/login/method LOADER is state-changing too, and the limiter above never saw it. +// When the identified user's only usable method is a single linked IdP, the loader mints a real +// Zitadel IdP intent and 302s to the provider (routes/login/method.tsx) — so +// `GET /id/login/method?loginName=X` changes provider-side state AND its redirect names the +// exact IdP that address uses. That is the same linked-IdP identity oracle the POST comment +// above describes, reachable by URL alone. (The loader now also requires a live ceremony session +// for that loginName, which closes the oracle to an unauthenticated prober; these limiters stay +// as the defence-in-depth layer underneath it, exactly like the POST's.) +// +// Matching requires a NON-EMPTY `loginName`: a bare GET (and a `?loginName=` with nothing in it) +// only bounces to /login and touches nothing, so it must not spend anyone's budget. +// +// SEPARATE BUDGET from the POST, deliberately. The POST is a chooser click; this GET is an +// ordinary page load on what is now the destination of EVERY post-identifier sign-in. +// +// TWO TIERS, because one key cannot serve both goals: +// • ip|loginName (10/5min) — the TIGHT one. Bounds how many intents can be minted against ONE +// address, which is what repeated probing of a known account (and the Back-button intent +// storm) looks like. A human signing in spends 1; a reload or two still fits. +// • ip (120/5min) — the LOOSE CEILING. Keying only by (ip, loginName) would hand an +// enumerator a fresh bucket per probed address, so breadth still needs an ip-only cap. It is +// deliberately generous: this path is now on every single sign-in, and a whole office behind +// one NAT egress IP must not 429 during a busy morning. 120/5min leaves ~24 sign-ins/minute +// from a single IP while still capping scripted enumeration breadth. +// Both are mounted; a request is counted by each, and whichever trips first answers 429. +const loginMethodIntentLimiter = new RateLimiter({ limit: 10, windowMs: 5 * 60_000 }); +const loginMethodIntentIpLimiter = new RateLimiter({ limit: 120, windowMs: 5 * 60_000 }); + +/** Shared self-guard: only an identified GET on the chooser is state-changing. */ +const isIdentifiedChooserGet = (c: Context, pathname: string): boolean => + c.req.method === 'GET' && pathname === '/id/login/method' && normalizedLoginName(c) !== ''; + +export const loginMethodIntentRateLimit: MiddlewareHandler = createRateLimit({ + limiter: loginMethodIntentLimiter, + match: isIdentifiedChooserGet, + key: (c, ip) => `${ip}|${normalizedLoginName(c)}`, + logFields: (c, ip) => ({ ip, actor: hashActor(normalizedLoginName(c)), tier: 'ip+loginName' }), +}); + +export const loginMethodIntentIpRateLimit: MiddlewareHandler = createRateLimit({ + limiter: loginMethodIntentIpLimiter, + match: isIdentifiedChooserGet, + key: (_c, ip) => ip, + logFields: (c, ip, pathname) => ({ ip, path: pathname, tier: 'ip' }), +}); diff --git a/app/server/net.ts b/app/server/net.ts index c4cc99c86f..e9069d53d7 100644 --- a/app/server/net.ts +++ b/app/server/net.ts @@ -3,6 +3,7 @@ // Shared network primitives for server middleware: client-IP extraction from the // X-Forwarded-For header, and a factory that owns the common rate-limit middleware // shape (pathname-normalize → IP-extract → check → audit-on-429 → 429 response). +import { APP_BASENAME } from '@/resources/shared/app-basename'; import type { RateLimiter } from '@/server/middleware/rate-limit'; import { logAuthEvent } from '@/server/observability'; import type { Context, MiddlewareHandler } from 'hono'; @@ -44,6 +45,61 @@ const RATE_LIMITED_BODY = { message: 'Too many attempts. Please try again later.', } as const; +/** + * True when this request is a TOP-LEVEL BROWSER NAVIGATION rather than a data fetch. + * + * Rate-limited POSTs and React-Router single-fetch `.data` requests are consumed by code (an + * action result, a fetcher) and must keep the JSON body every other limiter in this file + * returns. A rate-limited GET document request, by contrast, is rendered by the browser AS THE + * PAGE — a raw `{"error":"RATE_LIMITED"}` blob is what the user would see. Only that case gets + * HTML. + * + * `pathname` is already normalized (lowercased, `.data` stripped), so the raw URL is re-read here + * to tell `/id/login/method` from `/id/login/method.data`. + */ +function isTopLevelNavigation(c: Context): boolean { + if (c.req.method !== 'GET') return false; + if (new URL(c.req.url).pathname.toLowerCase().endsWith('.data')) return false; + return (c.req.header('accept') ?? '').includes('text/html'); +} + +/** + * Minimal, self-contained 429 page for a rate-limited top-level navigation. + * + * Hono middleware runs OUTSIDE the React/Lingui tree (same constraint as + * server/routes/saml-post.ts), so the copy is plain English here rather than a ``. + * No script (the prod CSP is nonce-based + strict-dynamic and this response carries no nonce); + * the inline + +
+

Too many attempts

+

You have made too many requests. Please try again ${wait}.

+

Back to sign in

+
+ `; +} + export interface CreateRateLimitOpts { /** The shared limiter instance (window + limit + store live here). */ limiter: RateLimiter; @@ -84,9 +140,11 @@ export function createRateLimit(opts: CreateRateLimitOpts): MiddlewareHandler { if (!allowed) { const fields = logFields ? logFields(c, ip, pathname) : { ip, path: pathname }; logAuthEvent('rate_limit', 'failure', fields); - return c.json(RATE_LIMITED_BODY, 429, { - 'Retry-After': String(Math.ceil(retryAfterMs / 1000)), - }); + const retryAfterSeconds = Math.ceil(retryAfterMs / 1000); + const headers = { 'Retry-After': String(retryAfterSeconds) }; + return isTopLevelNavigation(c) + ? c.html(renderRateLimitedPage(retryAfterSeconds), 429, headers) + : c.json(RATE_LIMITED_BODY, 429, headers); } await next(); }; diff --git a/app/server/signed-param.ts b/app/server/signed-param.ts new file mode 100644 index 0000000000..03febe8b97 --- /dev/null +++ b/app/server/signed-param.ts @@ -0,0 +1,52 @@ +// app/server/signed-param.ts +// +// Detached HMAC-SHA256 signatures for values that must survive a round-trip through an +// ATTACKER-CONTROLLED carrier and still be trusted on the way back. +// +// Every other value we hand to a browser and read again rides a signed cookie +// (`sessions`, `reauth-intent`, `idp-autostart`, …) — react-router's `createCookie({ secrets })` +// does the signing there. A QUERY PARAM on an IdP return URL cannot use that machinery: the +// URL is handed to the IdP broker, which redirects the browser to it directly, so the value +// travels in the open and anyone who can craft a callback URL can put whatever they like in it. +// A `.max(64)` on the way back proves nothing about WHO wrote it. +// +// Same secret and the same HMAC-SHA256 construction the cookies use (SESSION_SECRET, validated +// >= 32 chars at boot), so there is one key to rotate and one primitive to reason about. +import { env } from '@/server/infra/env.server'; +import { createHmac, timingSafeEqual } from 'node:crypto'; + +/** + * Domain separation. A signature minted for one param must never validate as another — without + * a purpose in the MAC input, a signed `policyOrg=` and any future signed param carrying the + * same string would be interchangeable, and the narrower capability could be swapped in for the + * wider one. + */ +function digest(purpose: string, value: string): Buffer { + return createHmac('sha256', env.SESSION_SECRET).update(`${purpose}:${value}`).digest(); +} + +/** base64url HMAC over `purpose:value`. URL-safe, so it needs no extra escaping in a query. */ +export function signParam(purpose: string, value: string): string { + return digest(purpose, value).toString('base64url'); +} + +/** + * True only for a signature THIS deployment minted for exactly this purpose + value. + * Absent, malformed, truncated, and forged signatures are all plain `false` — callers are + * expected to fall back to whatever they would have used with no value at all, never to + * trust the unverified value. + */ +export function verifyParam( + purpose: string, + value: string, + signature: string | null | undefined +): boolean { + if (!signature) return false; + const expected = digest(purpose, value); + // Buffer.from does not throw on stray base64url characters — it decodes what it can — so the + // length check is what rejects a malformed signature before timingSafeEqual (which throws on + // mismatched lengths). Comparison itself stays constant-time. + const provided = Buffer.from(signature, 'base64url'); + if (provided.length !== expected.length) return false; + return timingSafeEqual(provided, expected); +} diff --git a/cypress/component/resources/login/login-decision.cy.ts b/cypress/component/resources/login/login-decision.cy.ts index 1e5c40d888..33a36c2ec3 100644 --- a/cypress/component/resources/login/login-decision.cy.ts +++ b/cypress/component/resources/login/login-decision.cy.ts @@ -8,7 +8,7 @@ import { decideAfterIdentifier } from '@/resources/login/login-decision'; const PRIMARY = { role: 'primary' } as const; describe('decideAfterIdentifier → discriminated Decision union', () => { - it('does NOT route to /sso when allowExternalIdp is false (policy gate)', () => { + it('does NOT count idp when allowExternalIdp is false (policy gate)', () => { const d = decideAfterIdentifier({ methods: ['idp', 'password'], settings: { @@ -19,10 +19,12 @@ describe('decideAfterIdentifier → discriminated Decision union', () => { emailDeliveryEnabled: true, context: PRIMARY, }); - expect(d).to.deep.equal({ kind: 'redirect', path: '/login/password' }); + // Only password survives the gate — but a single method now still routes to + // the chooser, which renders exactly that one method. + expect(d).to.deep.equal({ kind: 'redirect', path: '/login/method' }); }); - it('routes to /sso when allowExternalIdp is true and idp is enrolled', () => { + it('a sole idp routes to /login/method, never to /sso', () => { const d = decideAfterIdentifier({ methods: ['idp'], settings: { @@ -33,6 +35,48 @@ describe('decideAfterIdentifier → discriminated Decision union', () => { emailDeliveryEnabled: true, context: PRIMARY, }); - expect(d).to.deep.equal({ kind: 'redirect', path: '/sso' }); + expect(d).to.deep.equal({ kind: 'redirect', path: '/login/method' }); + }); + + it('two methods still route to /login/method', () => { + const d = decideAfterIdentifier({ + methods: ['idp', 'password'], + settings: { + allowPassword: true, + allowExternalIdp: true, + passkeysType: 'not_allowed', + } as LoginSettings, + emailDeliveryEnabled: true, + context: PRIMARY, + }); + expect(d).to.deep.equal({ kind: 'redirect', path: '/login/method' }); + }); + + it('zero enrolled methods still routes to /verify (invite path)', () => { + const d = decideAfterIdentifier({ + methods: [], + settings: { + allowPassword: true, + allowExternalIdp: true, + passkeysType: 'not_allowed', + } as LoginSettings, + emailDeliveryEnabled: true, + context: PRIMARY, + }); + expect(d).to.deep.equal({ kind: 'redirect', path: '/verify' }); + }); + + it('enrolled password but policy forbids it → PASSWORD_NOT_ALLOWED', () => { + const d = decideAfterIdentifier({ + methods: ['password'], + settings: { + allowPassword: false, + allowExternalIdp: false, + passkeysType: 'not_allowed', + } as LoginSettings, + emailDeliveryEnabled: true, + context: PRIMARY, + }); + expect(d).to.deep.equal({ kind: 'error', error: 'PASSWORD_NOT_ALLOWED' }); }); }); diff --git a/cypress/component/resources/login/resolve-identifier-ignore-unknown.cy.ts b/cypress/component/resources/login/resolve-identifier-ignore-unknown.cy.ts index 9bbbed972f..bbe9f82195 100644 --- a/cypress/component/resources/login/resolve-identifier-ignore-unknown.cy.ts +++ b/cypress/component/resources/login/resolve-identifier-ignore-unknown.cy.ts @@ -13,4 +13,32 @@ describe('resolveIdentifier — ignoreUnknownUsernames', () => { }); expect(r).to.deep.equal({ ok: false, error: 'USER_NOT_FOUND' }); }); + + it('ON: the ghost target is whatever a real PASSWORD-ONLY account resolves to', () => { + // Pinned as a comparison, never as a literal. The ghost has no destination of its own — its + // only job is to collide with the password-only account's, and a literal '/login/method' here + // would pass just as happily on the day one of the two moves and the other does not. + const seed = { + users: [{ id: 'u1', loginName: 'alice@acme.test' }], + authMethods: { u1: ['password'] as const }, + settingsByOrg: { 'org-default-fake': { ignoreUnknownUsernames: true } }, + }; + cy.wrap(null).then(async () => { + const p = new FakeAuthProvider(seed as ConstructorParameters[0]); + const known = await resolveIdentifier(p, [], { + loginName: 'alice@acme.test', + emailDeliveryEnabled: true, + }); + const ghost = await resolveIdentifier(p, [], { + loginName: 'ghost@acme.test', + emailDeliveryEnabled: true, + }); + expect(known.ok && 'target' in known && known.target).to.be.a('string'); + expect(ghost.ok && 'target' in ghost && ghost.target).to.equal( + known.ok && 'target' in known && known.target + ); + // And it really did plant a ceremony session, or /login/method's gate would bounce it. + expect(ghost.ok && 'sessions' in ghost && ghost.sessions).to.have.length(1); + }); + }); }); diff --git a/cypress/component/resources/shared/org-first-settings-reads.cy.ts b/cypress/component/resources/shared/org-first-settings-reads.cy.ts index 2261983ed6..06ba7c429c 100644 --- a/cypress/component/resources/shared/org-first-settings-reads.cy.ts +++ b/cypress/component/resources/shared/org-first-settings-reads.cy.ts @@ -19,7 +19,13 @@ describe('login/method loader — org-first getLoginSettings + getBranding', () callService({ fn: 'loginMethodLoader', provider: 'singleton', - request: { url: `http://localhost/id/login/method?loginName=${ALICE}` }, + request: { + url: `http://localhost/id/login/method?loginName=${ALICE}`, + // The chooser loader is session-gated: it only identifies a user the ceremony has + // already planted a LIVE session for (the identifier step writes it on the same + // response that redirects here). + sessions: [{ id: 's1', token: 'tok-s1', loginName: ALICE }], + }, recordCalls: ['getDefaultOrg', 'getLoginSettings', 'getBranding'], }).then((v) => { expect(v.calls?.getDefaultOrg).to.have.length(1); @@ -34,6 +40,7 @@ describe('login/method loader — org-first getLoginSettings + getBranding', () provider: 'singleton', request: { url: `http://localhost/id/login/method?loginName=${ALICE}&organization=org-explicit`, + sessions: [{ id: 's1', token: 'tok-s1', loginName: ALICE }], }, recordCalls: ['getDefaultOrg', 'getLoginSettings', 'getBranding'], }).then((v) => { @@ -44,6 +51,58 @@ describe('login/method loader — org-first getLoginSettings + getBranding', () }); }); +describe('resolveIdentifier — post-identifier policy org agrees with the chooser', () => { + // The identifier step decides a user HAS a usable method and routes them to /login/method, + // whose loader then RE-computes that availability. If the two read a different org's policy the + // loader can compute `available: []` for a user the decision just approved and bounce them to + // /error. They previously agreed only for users WITH an orgId: `User.orgId` is OPTIONAL, and + // when it was absent this side fell through to INSTANCE settings (getLoginSettings(undefined)) + // while the chooser fell through to the DEFAULT ORG's. alice has no orgId, so she is exactly + // that case. + it('with NO ?organization AND no user.orgId, reads the DEFAULT org (not instance settings)', () => { + callService({ + fn: 'loginAction', + provider: 'singleton', + request: { + url: 'http://localhost/id/login', + form: { loginName: ALICE }, + csrf: true, + }, + recordCalls: ['getLoginSettings'], + }).then((v) => { + // LAST call, not a fixed index: resolveIdentifier's domain-discovery probe reads the + // BASE/instance settings first by design (the org isn't known yet), so the post-identifier + // read is the final one. + const orgArgs = (v.calls?.getLoginSettings ?? []).map((args) => args[0]); + expect(orgArgs.length, 'the post-identifier settings read ran').to.be.greaterThan(1); + expect(orgArgs.at(-1), 'must not fall through to INSTANCE settings').to.equal( + 'org-default-fake' + ); + // And the decision still routes to the chooser rather than a policy dead end. + expect(v.response?.location ?? '').to.contain('/login/method'); + }); + }); + + it('a user WITH an orgId still gets their OWN org, never the default', () => { + callService({ + fn: 'loginAction', + seed: { + users: [{ id: 'u1', loginName: 'mia@acme.test', orgId: 'org-mia' }], + authMethods: { u1: ['password'] }, + }, + request: { + url: 'http://localhost/id/login', + form: { loginName: 'mia@acme.test' }, + csrf: true, + }, + recordCalls: ['getLoginSettings'], + }).then((v) => { + const orgArgs = (v.calls?.getLoginSettings ?? []).map((args) => args[0]); + expect(orgArgs.at(-1)).to.equal('org-mia'); + }); + }); +}); + describe('session.service listAccounts — session-derived org-first getLoginSettings', () => { it('a cookie session WITH an organization → getLoginSettings gets it; provider default never consulted', () => { callService({ diff --git a/cypress/component/resources/sso/idp-return-urls.cy.ts b/cypress/component/resources/sso/idp-return-urls.cy.ts index 38b8bae87b..7c15947f8f 100644 --- a/cypress/component/resources/sso/idp-return-urls.cy.ts +++ b/cypress/component/resources/sso/idp-return-urls.cy.ts @@ -2,7 +2,8 @@ // // Component (no-mount) port of app/resources/sso/__tests__/idp-return-urls.test.ts. // idpReturnUrls is pure URL building (basename + requestId/link threading) → browser-side Chai. -import { idpReturnUrls, APP_BASENAME } from '@/resources/sso/idp-return-urls'; +import { idpReturnUrls, APP_BASENAME, POLICY_ORG_PURPOSE } from '@/resources/sso/idp-return-urls'; +import { signParam } from '@/server/signed-param'; describe('idpReturnUrls', () => { it('includes the /id basename so the IdP broker redirect hits the real route (regression: 404)', () => { @@ -46,9 +47,54 @@ describe('idpReturnUrls', () => { expect(failure).to.equal('http://localhost:3000/id/sso/google/error'); }); - it('failure url never carries link/deviceTrackingToken (success-path-only concerns)', () => { + it('carries a policyOrg that DIFFERS from organization, so the callback gates allowRegister in the deciding org', () => { + // /login/method picks a sole linked IdP out of a list resolved for the FOUND USER's org while + // the ceremony `organization` is still absent. Without this param the callback resolved + // allowRegister (and the auto-create org) from the DEFAULT org instead — a different policy + // from the one that offered the provider. + const { success } = idpReturnUrls('http://localhost:3000', 'google', { + policyOrg: 'org-mia', + }); + // SIGNED, and signed here at the START. This param is the only one on the URL that is not + // already coupled to something the attacker would also have to get right (see the note in + // idp-return-urls.ts), so the callback trusts provenance rather than shape — which means the + // producer must always emit the pair. Asserted through signParam rather than a literal so the + // spec pins the PAIRING, not one particular digest. + const sig = signParam(POLICY_ORG_PURPOSE, 'org-mia'); + expect(sig, 'a signature is actually produced').to.be.a('string').and.not.be.empty; + expect(success).to.equal( + `http://localhost:3000/id/sso/google/callback?policyOrg=org-mia&policyOrgSig=${encodeURIComponent(sig)}` + ); + }); + + it('never emits a bare policyOrg — an unsigned one is exactly what the callback refuses', () => { + // Regression guard on the producer half: dropping the signature here would not break any + // sign-in (the callback silently falls back to `organization`), it would just quietly restore + // the default-org policy divergence policyOrg exists to fix. Fail loudly instead. + const { success } = idpReturnUrls('http://localhost:3000', 'google', { + organization: 'org-1', + policyOrg: 'org-mia', + requestId: 'oidc_1', + }); + const params = new URL(success).searchParams; + expect(params.get('policyOrg')).to.equal('org-mia'); + expect(params.get('policyOrgSig'), 'signature rides with the value').to.equal( + signParam(POLICY_ORG_PURPOSE, 'org-mia') + ); + }); + + it('omits policyOrg when it is identical to organization (no redundant param)', () => { + const { success } = idpReturnUrls('http://localhost:3000', 'google', { + organization: 'org-1', + policyOrg: 'org-1', + }); + expect(success).to.equal('http://localhost:3000/id/sso/google/callback?organization=org-1'); + }); + + it('failure url never carries policyOrg/link/deviceTrackingToken (success-path-only concerns)', () => { const { failure } = idpReturnUrls('http://localhost:3000', 'google', { link: true, + policyOrg: 'org-mia', deviceTrackingToken: 'mm-token-abc', }); expect(failure).to.equal('http://localhost:3000/id/sso/google/error'); diff --git a/cypress/component/resources/sso/sso-callback.cy.ts b/cypress/component/resources/sso/sso-callback.cy.ts index 56fcd8bb9e..846456f215 100644 --- a/cypress/component/resources/sso/sso-callback.cy.ts +++ b/cypress/component/resources/sso/sso-callback.cy.ts @@ -310,6 +310,150 @@ describe('processIdpCallback — default-org resolution for IdP auto-create (bar }); }); +describe('processIdpCallback — ?policyOrg gates the allowRegister/auto-create org', () => { + // The START side (login/method.tsx's sole-linked-IdP auto-start, and its chooser action) picks + // an IdP out of a list resolved for the FOUND USER's org while the ceremony `organization` is + // still absent. `policyOrg` carries that deciding org across the round-trip so the callback + // gates allowRegister — and auto-creates — under the SAME policy that offered the provider, + // instead of falling back to the default org. + it('reads allowRegister from policyOrg and registers into it, ignoring the default-org fallback', () => { + callService({ + fn: 'processIdpCallback', + slug: 'google', + seed: {}, + idpIntent: REGISTER_INTENT_VERIFIED, + // signPolicyOrg mints the value + signature through the REAL idpReturnUrls, i.e. exactly + // what a legitimate start emits. Only that pair is honoured (see the forgery specs below). + request: { url: CB('google', 'id=intent-1&token=tok-1'), signPolicyOrg: 'org-mia' }, + recordCalls: ['getLoginSettings', 'register'], + }).then((v) => { + const settingsCalls = (v.calls?.['getLoginSettings'] ?? []) as Array<[string | undefined]>; + expect(settingsCalls[0]?.[0], 'allowRegister is read from the deciding org').to.equal( + 'org-mia' + ); + const registerCalls = (v.calls?.['register'] ?? []) as Array<[Record]>; + expect(registerCalls[0]?.[0]?.orgId, 'auto-create lands in the deciding org').to.equal( + 'org-mia' + ); + }); + }); + + it('does NOT org-scope the same-email findUser — that lookup stays instance-wide', () => { + // The whole reason policyOrg is its own param: widening `organization` instead would have + // silently narrowed these findUser lookups (see the note at sso-callback.ts's callbackOrg), + // so an existing account outside the named org would stop being found and the flow would + // register a duplicate. The seeded account has NO orgId, so an org-scoped lookup misses it. + callService({ + fn: 'processIdpCallback', + slug: 'google', + env: { ALLOW_IDP_AUTO_LINK: 'true' }, + seed: { users: [{ id: 'u1', loginName: 'you@gmail.com', displayName: 'You User' }] }, + idpIntent: REGISTER_INTENT_VERIFIED, + request: { url: CB('google', 'id=intent-1&token=tok-1'), signPolicyOrg: 'org-mia' }, + recordCalls: ['findUser', 'register'], + }).then((v) => { + const findUserCalls = (v.calls?.['findUser'] ?? []) as Array<[string, string | undefined]>; + expect(findUserCalls.length, 'the same-email pre-check ran').to.be.greaterThan(0); + // `undefined` crosses the cy.task JSON boundary as null — either way it is NOT 'org-mia'. + expect( + findUserCalls[0][1] ?? null, + 'findUser stays instance-wide (org arg untouched)' + ).to.equal(null); + // …and the existing account was actually found: it auto-linked instead of registering. + expect(v.calls?.['register'] ?? []).to.have.length(0); + }); + }); +}); + +describe('processIdpCallback — an unsigned/forged ?policyOrg is ignored', () => { + // The callback URL is handed to the IdP broker and then to the browser, so `?policyOrg=` is + // fully attacker-writable. It is also the ONE param on that URL coupled to nothing else: + // `organization` drags the same-email findUser scope, createSession's orgId and + // signInWithIdpIntent along with it, so naming a foreign org there fails closed. policyOrg feeds + // `callbackOrg` alone — which gates allowRegister and picks the auto-create org — so a crafted + // one borrows a registration-open org's policy WHILE the same-email lookup stays instance-wide. + // With ALLOW_IDP_AUTO_LINK on that is a path into a victim's account in any org. A `.max(64)` + // cannot help: decoupling those two orgs is the param's entire purpose, so only provenance can. + const FORGED = 'org-attacker-registration-open'; + + it('a hand-written policyOrg with NO signature falls back to `organization`', () => { + callService({ + fn: 'processIdpCallback', + slug: 'google', + seed: {}, + idpIntent: REGISTER_INTENT_VERIFIED, + request: { url: CB('google', `id=intent-1&token=tok-1&policyOrg=${FORGED}`) }, + recordCalls: ['getLoginSettings', 'register'], + }).then((v) => { + const settingsCalls = (v.calls?.['getLoginSettings'] ?? []) as Array<[string | undefined]>; + expect(settingsCalls[0]?.[0], 'allowRegister must NOT come from the forged org').to.equal( + 'org-default-fake' + ); + const registerCalls = (v.calls?.['register'] ?? []) as Array<[Record]>; + expect(registerCalls[0]?.[0]?.orgId, 'auto-create must NOT land in the forged org').to.equal( + 'org-default-fake' + ); + }); + }); + + it('a MIS-signed policyOrg (valid signature, swapped value) is ignored too', () => { + // The realistic forgery is not a random digest — it is a signature lifted from a legitimate + // start and pasted next to a different org. Domain separation + signing the VALUE is what + // makes that fail; a signature is a claim about one exact string, not a bearer token. + callService({ + fn: 'processIdpCallback', + slug: 'google', + seed: {}, + idpIntent: REGISTER_INTENT_VERIFIED, + // Mint a real pair for 'org-mia', then overwrite the VALUE while keeping the signature. + request: { + url: CB('google', 'id=intent-1&token=tok-1'), + signPolicyOrg: 'org-mia', + tamperPolicyOrg: FORGED, + }, + recordCalls: ['getLoginSettings'], + }).then((v) => { + const settingsCalls = (v.calls?.['getLoginSettings'] ?? []) as Array<[string | undefined]>; + expect(settingsCalls[0]?.[0], 'a mismatched value voids the signature').to.equal( + 'org-default-fake' + ); + }); + }); + + it('logs the rejection so tampering (or a mid-flight secret rotation) is visible', () => { + callService({ + fn: 'processIdpCallback', + slug: 'google', + seed: {}, + idpIntent: REGISTER_INTENT_VERIFIED, + request: { url: CB('google', `id=intent-1&token=tok-1&policyOrg=${FORGED}`) }, + }).then((v) => { + const line = v.audit.find((e) => e.event === 'idp_policy_org'); + expect(line, 'an audit line names the rejected param').to.not.equal(undefined); + expect(line?.outcome).to.equal('failure'); + expect((line as { reason?: string } | undefined)?.reason).to.equal('invalid_signature'); + }); + }); + + it('no policyOrg at all still resolves the default org (unchanged baseline)', () => { + callService({ + fn: 'processIdpCallback', + slug: 'google', + seed: {}, + idpIntent: REGISTER_INTENT_VERIFIED, + request: { url: CB('google', 'id=intent-1&token=tok-1') }, + recordCalls: ['getLoginSettings'], + }).then((v) => { + const settingsCalls = (v.calls?.['getLoginSettings'] ?? []) as Array<[string | undefined]>; + expect(settingsCalls[0]?.[0]).to.equal('org-default-fake'); + expect( + v.audit.some((e) => e.event === 'idp_policy_org'), + 'nothing to reject, nothing logged' + ).to.equal(false); + }); + }); +}); + describe('processIdpCallback — fresh-identity link ceremony (Req 2)', () => { // A FRESH external identity (intent.userId == null) attached to the ACTIVE session user via // ?link=true — the Req-2 wiring (sso-callback.ts:144-153) that the existing already-MAPPED link diff --git a/cypress/component/routes/login/enumeration-parity.cy.ts b/cypress/component/routes/login/enumeration-parity.cy.ts new file mode 100644 index 0000000000..276bb78295 --- /dev/null +++ b/cypress/component/routes/login/enumeration-parity.cy.ts @@ -0,0 +1,214 @@ +// cypress/component/routes/login/enumeration-parity.cy.ts +// +// `ignoreUnknownUsernames` is a tenant-facing ANTI-ENUMERATION setting, and its entire protection +// is that an UNKNOWN identifier and a real PASSWORD-ONLY account are indistinguishable to an +// unauthenticated observer. Routing every account with >= 1 usable method to /login/method broke +// exactly that: the known side moved and the ghost side did not, turning the identifier step's +// redirect target into a perfect account-existence oracle. +// +// These specs pin the parity itself rather than either side's destination, so a future re-route of +// one branch fails here instead of silently reopening the oracle. Four observable channels: +// status, redirect target, rendered chooser, Set-Cookie shape — plus the audit line. +import { callService } from '../../../support/node/call-service'; + +/** The default org the fake resolves to; where the tenant flips the flag. */ +const DEFAULT_ORG = 'org-default-fake'; + +/** One real password-only account, in an org that has anti-enumeration ON. */ +const ANTI_ENUM_SEED = { + users: [{ id: 'u1', loginName: 'alice@acme.test' }], + authMethods: { u1: ['password'] }, + settingsByOrg: { [DEFAULT_ORG]: { ignoreUnknownUsernames: true } }, +}; + +const KNOWN = 'alice@acme.test'; +const GHOST = 'nobody-here@acme.test'; + +/** The identifier submit, as an unauthenticated visitor makes it. */ +function submitIdentifier(loginName: string, seed: Record = ANTI_ENUM_SEED) { + return callService({ + fn: 'loginAction', + seed, + env: { AUTH_EMAIL_DELIVERY_ENABLED: 'true' }, + request: { + url: 'http://localhost/id/login', + form: { loginName }, + csrf: true, + }, + }); +} + +/** The chooser GET the identifier submit redirects to, carrying the session it just planted. */ +function loadChooser(loginName: string, seed: Record = ANTI_ENUM_SEED) { + return callService({ + fn: 'loginMethodLoader', + seed, + env: { AUTH_EMAIL_DELIVERY_ENABLED: 'true' }, + request: { + url: `http://localhost/id/login/method?loginName=${encodeURIComponent(loginName)}`, + sessions: [{ id: 's1', token: 'tok-s1', loginName }], + }, + }); +} + +/** Cookie NAMES set by a response, sorted — the shape an observer can compare. */ +function cookieNames(setCookies: string[] | undefined): string[] { + return (setCookies ?? []).map((c) => c.split('=')[0]).sort(); +} + +describe('ignoreUnknownUsernames — identifier step is not an existence oracle', () => { + it('unknown and password-only land on the SAME target with the same status', () => { + submitIdentifier(KNOWN).then((known) => { + submitIdentifier(GHOST).then((ghost) => { + expect(known.response?.status, 'known status').to.equal(302); + expect(ghost.response?.status, 'ghost status').to.equal(known.response?.status); + + // Compare the whole Location minus the one field that legitimately differs: the + // identifier the visitor typed. Anything else diverging IS the oracle. + const strip = (loc: string) => loc.replace(/loginName=[^&]*/, 'loginName='); + const knownLoc = strip(known.response?.location ?? ''); + const ghostLoc = strip(ghost.response?.location ?? ''); + expect(knownLoc, 'known goes to the chooser').to.contain('/login/method'); + expect(ghostLoc, 'ghost redirect target must be byte-identical').to.equal(knownLoc); + }); + }); + }); + + it('both set the same Set-Cookie shape (a real ceremony session either way)', () => { + submitIdentifier(KNOWN).then((known) => { + submitIdentifier(GHOST).then((ghost) => { + const knownNames = cookieNames(known.response?.setCookies); + expect(knownNames, 'the known path persists a ceremony session').to.include('sessions'); + expect(cookieNames(ghost.response?.setCookies), 'same cookies, same order').to.deep.equal( + knownNames + ); + // Not merely present — the ghost cookie must carry a real entry, or the chooser's + // session gate would bounce it and re-open the oracle one hop later. + expect( + ghost.response?.cookieEntries ?? [], + 'ghost session entry is planted' + ).to.have.length(known.response?.cookieEntries?.length ?? 0); + }); + }); + }); + + it('emits the SAME audit event for both (no "not_found" tell in the logs)', () => { + submitIdentifier(KNOWN).then((known) => { + submitIdentifier(GHOST).then((ghost) => { + const shape = (v: typeof known) => + v.audit + .filter((e) => e.event === 'identifier') + .map((e) => `${e.event}:${e.outcome}:${(e as { reason?: string }).reason ?? ''}`); + expect(shape(known)).to.deep.equal(['identifier:success:']); + expect(shape(ghost), 'a failure/reason line here names the branch').to.deep.equal( + shape(known) + ); + }); + }); + }); + + it('OFF (default): the unknown identifier is still rejected, no ghost session', () => { + // The parity above must not have turned the flag into a no-op — with it off, an unknown + // identifier is still USER_NOT_FOUND and plants nothing. + submitIdentifier(GHOST, { + users: [{ id: 'u1', loginName: KNOWN }], + authMethods: { u1: ['password'] }, + }).then((v) => { + expect(v.response?.dataBody).to.deep.equal({ error: 'USER_NOT_FOUND' }); + expect(cookieNames(v.response?.dataSetCookies), 'no ceremony session').to.not.include( + 'sessions' + ); + }); + }); +}); + +describe('ignoreUnknownUsernames — the chooser renders the same screen for both', () => { + it('a ghost that clears the session gate RENDERS, it does not bounce', () => { + // The gate is the authorization (a live ceremony session for this loginName, which /login + // mints only under the flag). Bouncing on the findUser miss would move the oracle here. + loadChooser(GHOST).then((v) => { + expect(v.response?.status, 'must not redirect').to.not.equal(302); + expect((v.response?.dataBody as { methods: string[] }).methods).to.deep.equal(['password']); + }); + }); + + it('identical chooser payload: same controls, same branding, same CSRF handling', () => { + loadChooser(KNOWN).then((known) => { + loadChooser(GHOST).then((ghost) => { + type Body = { + loginName: string; + methods: string[]; + idps: unknown[]; + branding: unknown; + csrfToken: string; + }; + const k = known.response?.dataBody as Body; + const g = ghost.response?.dataBody as Body; + + expect(known.response?.status).to.equal(ghost.response?.status); + expect(g.methods, 'same single control').to.deep.equal(k.methods); + expect(g.idps, 'same (empty) IdP list').to.deep.equal(k.idps); + expect(g.branding, 'branding resolves identically').to.deep.equal(k.branding); + // A rotated per-request token either way — its VALUE must differ, its presence must not. + expect(g.csrfToken, 'ghost gets a real CSRF token').to.be.a('string').and.not.be.empty; + expect(typeof g.csrfToken).to.equal(typeof k.csrfToken); + expect(cookieNames(ghost.response?.dataSetCookies)).to.deep.equal( + cookieNames(known.response?.dataSetCookies) + ); + }); + }); + }); + + it('parity SURVIVES a user whose org is NOT the default org', () => { + // The regression this pins. A ghost has no user.orgId, so its policy read fell through to the + // DEFAULT org while a real account was judged by its OWN. Seed the two to DISAGREE — the + // default org forbids passwords, mia's org does not — and the old code split the pair + // (known -> /login/method, ghost -> /error): a perfect existence oracle, in the one + // configuration the rest of this file cannot reach because its users carry no orgId. + // Both branches now resolve a ghost's policy org from the identifier's DOMAIN + // (resolveGhostPolicyOrg), so nobody-here@acme.test is judged by the very org + // mia@acme.test belongs to. + const seed = { + users: [{ id: 'u1', loginName: 'mia@acme.test', orgId: 'org-mia' }], + authMethods: { u1: ['password'] }, + orgDomains: { 'acme.test': 'org-mia' }, + settingsByOrg: { + [DEFAULT_ORG]: { ignoreUnknownUsernames: true, allowPassword: false }, + 'org-mia': { ignoreUnknownUsernames: true }, + }, + }; + submitIdentifier('mia@acme.test', seed).then((known) => { + submitIdentifier('nobody-here@acme.test', seed).then((ghost) => { + const strip = (loc: string) => loc.replace(/loginName=[^&]*/, 'loginName='); + expect(known.response?.location ?? '', 'known reaches the chooser').to.contain( + '/login/method' + ); + expect(ghost.response?.status, 'same status').to.equal(known.response?.status); + expect( + strip(ghost.response?.location ?? ''), + 'the ghost must not be dead-ended by a policy the real account never sees' + ).to.equal(strip(known.response?.location ?? '')); + }); + }); + }); + + it('an org that forbids passwords dead-ends BOTH at /error, never one of them', () => { + // The ghost is decided by the same decideAfterIdentifier call a password-only account gets, + // so a policy that removes the only method removes it for both. Hardcoding a ghost target + // would send it to a chooser the real account is not allowed to reach. + const seed = { + users: [{ id: 'u1', loginName: KNOWN }], + authMethods: { u1: ['password'] }, + settingsByOrg: { + [DEFAULT_ORG]: { ignoreUnknownUsernames: true, allowPassword: false }, + }, + }; + submitIdentifier(KNOWN, seed).then((known) => { + submitIdentifier(GHOST, seed).then((ghost) => { + expect(known.response?.location ?? '', 'known: policy dead end').to.contain('/error'); + expect(ghost.response?.status).to.equal(known.response?.status); + expect(ghost.response?.location ?? '').to.contain('/error'); + }); + }); + }); +}); diff --git a/cypress/component/routes/login/index.cy.tsx b/cypress/component/routes/login/index.cy.tsx index 2105697f7b..3cb0952e3e 100644 --- a/cypress/component/routes/login/index.cy.tsx +++ b/cypress/component/routes/login/index.cy.tsx @@ -2,11 +2,15 @@ // // UI contract for /login: the chooser // renders unconditionally. There is no identity-bound inline passkey ceremony state on -// this route anymore — a sole-passkey identifier now REDIRECTS to /login/passkey (see -// the node-bound action test below) instead of the action returning inline challenge -// data. The gated Passkey SHORTCUT still drives the shared ceremony in place -// on this page, and its own failure surface is still covered here. Mirrors -// method.cy.tsx's stub /login/passkey route + capturedPosts convention (Task 2). +// this route anymore — every identifier with >= 1 usable method (sole-passkey included) +// now REDIRECTS to /login/method, the method chooser, instead of the action returning +// inline challenge data or redirecting straight to a per-method target (see the +// node-bound action test below). /login/method itself now owns auto-starting a sole +// passkey client-side (Task 3) and a sole linked IdP server-side (Task 2) — this route's +// own action no longer picks a per-method destination. The gated Passkey SHORTCUT still +// drives the shared ceremony in place on this page, and its own failure surface is still +// covered here. Mirrors method.cy.tsx's stub /login/passkey route + capturedPosts +// convention (Task 2). import { callService } from '../../../support/node/call-service'; import Login from '@/routes/login/index'; import { ConformAdapter } from '@datum-cloud/datum-ui/form/adapters/conform'; @@ -161,7 +165,12 @@ describe('/login — chooser (no inline ceremony)', () => { }); describe('/login action — sole-passkey identifier', () => { - it('a sole-passkey identifier now REDIRECTS to /login/passkey instead of returning inline data', () => { + it('a sole-passkey identifier REDIRECTS to /login/method (the chooser), not a per-method target', () => { + // Task 1 collapsed decideAfterIdentifier's per-method single-target branch: every + // account with >= 1 usable method — sole-passkey included — now redirects to the + // chooser. /login/method itself auto-begins a sole passkey ceremony client-side + // (Task 3), so the user still sees no intermediate chooser screen; this route's own + // action just hands off to /login/method rather than picking /login/passkey directly. callService({ fn: 'loginAction', provider: 'singleton', @@ -173,15 +182,18 @@ describe('/login action — sole-passkey identifier', () => { }).then((v) => { expect(v.response?.isResponse).to.equal(true); expect(v.response?.status).to.equal(302); - expect(v.response?.location ?? '').to.contain('/login/passkey'); + expect(v.response?.location ?? '').to.contain('/login/method'); + // Regression guard: must NOT fall back to the old per-method redirect target. + expect(v.response?.location ?? '').to.not.contain('/login/passkey'); expect(v.response?.location ?? '').to.contain('loginName='); }); }); }); // ── Identifier-form view logic ─────────────────────────────────────────────── -// Sole-passkey identifiers REDIRECT to /login/passkey (asserted above); these two -// cover the identifier-form visibility logic under restricted org policies. +// Sole-passkey (and every other >= 1-usable-method) identifier REDIRECTS to /login/method +// (asserted above); these two cover the identifier-form visibility logic under restricted +// org policies. describe('/login — identifier-form view logic', () => { // REGRESSION: with allowPassword=false the identifier form used to be hidden entirely, // while signInUnavailable stayed false (suppressed by showPasskeyPrompt) — so a fresh diff --git a/cypress/component/routes/login/method-chooser.cy.ts b/cypress/component/routes/login/method-chooser.cy.ts index 385fb97820..95c8d64758 100644 --- a/cypress/component/routes/login/method-chooser.cy.ts +++ b/cypress/component/routes/login/method-chooser.cy.ts @@ -5,6 +5,21 @@ // (B) /login/method loader → methods array, branding, redirect guards. import { callService } from '../../../support/node/call-service'; +// The chooser loader is SESSION-GATED: it acts only for a loginName this browser already holds a +// LIVE ceremony session for. /login's identifier action plants exactly this entry on the same +// response that redirects here, so supplying it is what makes these scenarios legitimate arrivals +// rather than the drive-by GETs the gate exists to refuse. +const ALICE_SESSION = { id: 's1', token: 'tok-s1', loginName: 'alice@acme.test' }; +const MIA_SESSION = { id: 's1', token: 'tok-s1', loginName: 'mia@acme.test' }; + +/** The sole-linked-IdP fixture: one enrolled method ('idp'), one active+linked provider. */ +const SOLE_IDP_SEED = { + users: [{ id: 'u1', loginName: 'mia@acme.test' }], + authMethods: { u1: ['idp'] }, + idps: [{ id: 'idp-google', name: 'Google', type: 'GOOGLE' }], + idpLinks: { u1: [{ idpId: 'idp-google', idpUserId: 'g-1' }] }, +}; + // ── (A) intent=email-link ────────────────────────────────────────────────────── describe('login action — intent=email-link', () => { @@ -31,16 +46,318 @@ describe('login action — intent=email-link', () => { // ── (B) /login/method loader ─────────────────────────────────────────────────── describe('/login/method loader', () => { - it('alice@acme.test (password-only) → redirects away (< 2 methods)', () => { + it('alice@acme.test (password-only) → RENDERS the chooser, never redirects', () => { + // Regression guard: this used to 302 to /login/password. Now that the decision + // returns /login/method for a single method, redirecting here is a self-redirect loop. callService({ fn: 'loginMethodLoader', provider: 'singleton', env: { AUTH_EMAIL_DELIVERY_ENABLED: 'true' }, - request: { url: 'http://localhost/id/login/method?loginName=alice%40acme.test' }, + request: { + url: 'http://localhost/id/login/method?loginName=alice%40acme.test', + sessions: [ALICE_SESSION], + }, + }).then((v) => { + expect(v.response?.status, 'must not redirect').to.not.equal(302); + const body = v.response?.dataBody as { methods: string[] }; + expect(body.methods).to.deep.equal(['password']); + }); + }); + + it('a sole linked IdP auto-starts the intent instead of rendering one button', () => { + callService({ + fn: 'loginMethodLoader', + seed: { + users: [{ id: 'u1', loginName: 'mia@acme.test' }], + authMethods: { u1: ['idp'] }, + idps: [{ id: 'idp-google', name: 'Google', type: 'GOOGLE' }], + idpLinks: { u1: [{ idpId: 'idp-google', idpUserId: 'g-1' }] }, + }, + env: { AUTH_EMAIL_DELIVERY_ENABLED: 'true' }, + request: { + url: 'http://localhost/id/login/method?loginName=mia%40acme.test', + sessions: [MIA_SESSION], + }, }).then((v) => { expect(v.response?.status).to.equal(302); const loc = v.response?.location ?? ''; - expect(loc).to.contain('/login/password'); + expect(loc, 'goes to the provider, not /sso').to.not.contain('/sso'); + expect(loc).to.contain('idp-google'); + }); + }); + + it('two linked IdPs render a picker rather than guessing one', () => { + callService({ + fn: 'loginMethodLoader', + seed: { + users: [{ id: 'u1', loginName: 'mia@acme.test' }], + authMethods: { u1: ['idp'] }, + idps: [ + { id: 'idp-google', name: 'Google', type: 'GOOGLE' }, + { id: 'idp-github', name: 'GitHub', type: 'GITHUB' }, + ], + idpLinks: { + u1: [ + { idpId: 'idp-google', idpUserId: 'g-1' }, + { idpId: 'idp-github', idpUserId: 'h-1' }, + ], + }, + }, + env: { AUTH_EMAIL_DELIVERY_ENABLED: 'true' }, + request: { + url: 'http://localhost/id/login/method?loginName=mia%40acme.test', + sessions: [MIA_SESSION], + }, + }).then((v) => { + expect(v.response?.status).to.not.equal(302); + const body = v.response?.dataBody as { idps: Array<{ id: string }> }; + expect(body.idps).to.have.length(2); + }); + }); + + it('idp enrolled with zero usable links never self-redirects to /login/method', () => { + // Regression guard for the loop the previous fix round missed: the loader's OWN + // `available` computation only counts 'idp' when a real, active, non-LDAP link + // resolves (method.tsx:75-87) — here there are none. decideAfterIdentifier, however, + // recomputes availability blind to that resolution (methods.includes('idp') && + // settings.allowExternalIdp alone), so it still names THIS route. Falling into the + // `available.length === 0` branch and following that decision verbatim would 302 back + // to /login/method with the exact same inputs — an infinite loop. + callService({ + fn: 'loginMethodLoader', + seed: { + users: [{ id: 'u1', loginName: 'mia@acme.test' }], + authMethods: { u1: ['idp'] }, + // No idpLinks entry for u1 at all: methods says 'idp' is enrolled, but nothing + // resolves to a usable, linked, active, non-LDAP provider. + }, + env: { AUTH_EMAIL_DELIVERY_ENABLED: 'true' }, + request: { + url: 'http://localhost/id/login/method?loginName=mia%40acme.test', + sessions: [MIA_SESSION], + }, + }).then((v) => { + // Name the destination, don't merely rule one out: `not.contain('/login/method')` also + // passes on a 200 that renders an EMPTY chooser, which is the other way this can go wrong. + // 'idp' enrolled means methods.length !== 0, so the policy-dead-end leg (/error) is the + // one and only correct answer here. + expect(v.response?.status, 'must redirect, not render an empty chooser').to.equal(302); + const loc = v.response?.location ?? ''; + expect(loc, 'a policy dead end goes to /error').to.contain('/error'); + expect(loc, 'must never redirect back to /login/method').to.not.contain('/login/method'); + }); + }); + + it('falls through and RENDERS the button when the sole-IdP intent fails to start', () => { + // A provider that accepts the call but returns no authUrl → IDP_UNAVAILABLE. Redirecting or + // erroring here would strand the user on a screen with nothing to press; the chooser must + // render its one Google button so they keep a way forward (and can retry via the action). + callService({ + fn: 'loginMethodLoader', + seed: SOLE_IDP_SEED, + failStartIdpIntent: true, + env: { AUTH_EMAIL_DELIVERY_ENABLED: 'true' }, + request: { + url: 'http://localhost/id/login/method?loginName=mia%40acme.test', + sessions: [MIA_SESSION], + }, + }).then((v) => { + expect(v.response?.status, 'no redirect — render the fallback').to.not.equal(302); + const body = v.response?.dataBody as { methods: string[]; idps: Array<{ id: string }> }; + expect(body.methods).to.deep.equal(['idp']); + expect(body.idps).to.have.length(1); + }); + }); +}); + +describe('/login/method loader — session gate', () => { + // GET /id/login/method?loginName=X is CSRF-token-free and state-changing (a sole-linked-IdP + // account makes it mint a real Zitadel intent and 302 to the provider, naming that provider). + // Ungated it was an account-existence AND identity-provider oracle reachable by URL alone, a + // login-CSRF vector, and a bypass of ignoreUnknownUsernames — which is honoured only in + // resolveIdentifier. The ceremony session the identifier step already planted is the gate. + it('bounces to /login with NO session cookie at all', () => { + callService({ + fn: 'loginMethodLoader', + seed: SOLE_IDP_SEED, + request: { url: 'http://localhost/id/login/method?loginName=mia%40acme.test' }, + }).then((v) => { + expect(v.response?.status).to.equal(302); + const loc = v.response?.location ?? ''; + expect(loc).to.match(/\/login(\?|$)/); + expect(loc, 'must not name the provider').to.not.contain('idp-google'); + }); + }); + + it('bounces when the session belongs to a DIFFERENT account (no borrowing someone else)', () => { + callService({ + fn: 'loginMethodLoader', + seed: SOLE_IDP_SEED, + request: { + url: 'http://localhost/id/login/method?loginName=mia%40acme.test', + sessions: [ALICE_SESSION], + }, + }).then((v) => { + expect(v.response?.location ?? '').to.match(/\/login(\?|$)/); + }); + }); + + it('bounces when the matching session has EXPIRED (present is not live)', () => { + callService({ + fn: 'loginMethodLoader', + seed: SOLE_IDP_SEED, + request: { + url: 'http://localhost/id/login/method?loginName=mia%40acme.test', + sessions: [{ ...MIA_SESSION, expirationTs: '2000-01-01T00:00:00.000Z' }], + }, + }).then((v) => { + expect(v.response?.location ?? '').to.match(/\/login(\?|$)/); + }); + }); + + it('accepts a session whose loginName differs only in CASE', () => { + // The URL carries the provider's canonical loginName; a hand-typed or IdP-returned + // identifier may differ in case. An exact compare would lock out a legitimate arrival. + callService({ + fn: 'loginMethodLoader', + seed: { + users: [{ id: 'u1', loginName: 'mia@acme.test' }], + authMethods: { u1: ['password'] }, + }, + request: { + url: 'http://localhost/id/login/method?loginName=mia%40acme.test', + sessions: [{ ...MIA_SESSION, loginName: 'MIA@Acme.Test' }], + }, + }).then((v) => { + expect(v.response?.status).to.not.equal(302); + expect((v.response?.dataBody as { methods: string[] }).methods).to.deep.equal(['password']); + }); + }); + + it('threads requestId + organization onto the bounce so the ceremony survives', () => { + callService({ + fn: 'loginMethodLoader', + seed: SOLE_IDP_SEED, + request: { + url: 'http://localhost/id/login/method?loginName=mia%40acme.test&requestId=oidc_1&organization=org-1', + }, + }).then((v) => { + const loc = v.response?.location ?? ''; + expect(loc).to.contain('requestId=oidc_1'); + expect(loc).to.contain('organization=org-1'); + }); + }); +}); + +describe('login action — retires the one-shot auto-start marker', () => { + it('expires idp-autostart so the NEXT ceremony can auto-start again', () => { + // The marker suppresses a second auto-start for the same loginName, which is what stops the + // Back-from-the-provider arrival re-minting an intent. Its 10-minute maxAge outlived the + // ceremony that wrote it, so a sign-out/sign-in inside that window got the one-button chooser + // instead of the auto-start. The identifier submit IS a new ceremony, so it clears the marker + // — Back never re-POSTs here, so the guard the marker provides is untouched. + callService({ + fn: 'loginAction', + seed: { + users: [{ id: 'u1', loginName: 'alice@acme.test' }], + authMethods: { u1: ['password'] }, + }, + env: { AUTH_EMAIL_DELIVERY_ENABLED: 'true' }, + request: { + url: 'http://localhost/id/login', + form: { loginName: 'alice@acme.test' }, + csrf: true, + idpAutostart: 'alice@acme.test', + }, + }).then((v) => { + const cleared = (v.response?.setCookies ?? []).find((c) => c.startsWith('idp-autostart=')); + expect(cleared, 'the action emits an idp-autostart cookie').to.be.a('string'); + // Expiry, not a rewrite: Max-Age=0 is what actually retires it in the browser. + expect(cleared ?? '', 'expired, not re-armed').to.contain('Max-Age=0'); + }); + }); +}); + +describe('/login/method loader — ONE-SHOT sole-IdP auto-start', () => { + // The auto-start lives in a LOADER, which re-runs on every arrival at the URL — including the + // one the browser makes when the user presses Back at the provider. Unguarded that Back mints + // a NEW intent and bounces them straight forward again: they can never return to the app. + it('FIRST arrival still auto-starts, and marks the browser', () => { + callService({ + fn: 'loginMethodLoader', + seed: SOLE_IDP_SEED, + env: { AUTH_EMAIL_DELIVERY_ENABLED: 'true' }, + request: { + url: 'http://localhost/id/login/method?loginName=mia%40acme.test', + sessions: [MIA_SESSION], + }, + }).then((v) => { + expect(v.response?.status).to.equal(302); + expect(v.response?.location ?? '').to.contain('idp-google'); + expect(v.response?.setCookie ?? '', 'writes the one-shot marker').to.contain('idp-autostart'); + }); + }); + + it('SECOND arrival for the same account renders the chooser instead of re-minting', () => { + callService({ + fn: 'loginMethodLoader', + seed: SOLE_IDP_SEED, + env: { AUTH_EMAIL_DELIVERY_ENABLED: 'true' }, + request: { + url: 'http://localhost/id/login/method?loginName=mia%40acme.test', + sessions: [MIA_SESSION], + idpAutostart: 'mia@acme.test', + }, + recordCalls: ['startIdpIntent'], + }).then((v) => { + expect(v.response?.status, 'must not redirect to the provider again').to.not.equal(302); + expect(v.calls?.startIdpIntent, 'no new Zitadel intent minted').to.have.length(0); + const body = v.response?.dataBody as { methods: string[]; idps: Array<{ id: string }> }; + expect(body.methods).to.deep.equal(['idp']); + expect(body.idps, 'the chooser offers the provider as a button').to.have.length(1); + }); + }); + + it('a marker for a DIFFERENT account does not suppress this one', () => { + callService({ + fn: 'loginMethodLoader', + seed: SOLE_IDP_SEED, + env: { AUTH_EMAIL_DELIVERY_ENABLED: 'true' }, + request: { + url: 'http://localhost/id/login/method?loginName=mia%40acme.test', + sessions: [MIA_SESSION], + idpAutostart: 'someone-else@acme.test', + }, + }).then((v) => { + expect(v.response?.status).to.equal(302); + expect(v.response?.location ?? '').to.contain('idp-google'); + }); + }); + + it("gates on the USER's own org policy, not the default org's, with no ?organization", () => { + // resolveIdentifier decides with `org ?? user.orgId` (login.service.ts) — the org the found + // user actually belongs to — and it is what routed the user here. This loader must gate on + // the same policy. When it resolved settings default-org-first instead, any user outside the + // default org signing in without an explicit ?organization got their method approved by one + // policy and then computed away by another: available=[] and a bounce to /error, on the most + // travelled path in the product. Here the default org forbids passwords and mia's org does + // not, so reading the wrong one is a redirect instead of a render. + callService({ + fn: 'loginMethodLoader', + seed: { + users: [{ id: 'u1', loginName: 'mia@acme.test', orgId: 'org-mia' }], + authMethods: { u1: ['password'] }, + settingsByOrg: { 'org-default-fake': { allowPassword: false } }, + }, + env: { AUTH_EMAIL_DELIVERY_ENABLED: 'true' }, + request: { + url: 'http://localhost/id/login/method?loginName=mia%40acme.test', + sessions: [MIA_SESSION], + }, + }).then((v) => { + expect(v.response?.status, 'must not bounce to /error').to.not.equal(302); + const body = v.response?.dataBody as { methods: string[] }; + expect(body.methods).to.deep.equal(['password']); }); }); @@ -58,7 +375,10 @@ describe('/login/method loader', () => { idpLinks: { u1: [{ idpId: 'idp-google', idpUserId: 'g-1' }] }, }, env: { AUTH_EMAIL_DELIVERY_ENABLED: 'true' }, - request: { url: 'http://localhost/id/login/method?loginName=mia%40acme.test' }, + request: { + url: 'http://localhost/id/login/method?loginName=mia%40acme.test', + sessions: [MIA_SESSION], + }, }).then((v) => { const body = v.response?.dataBody as { methods: string[]; idps: Array<{ id: string }> }; expect(body.methods).to.include('idp'); diff --git a/cypress/component/routes/login/method.cy.tsx b/cypress/component/routes/login/method.cy.tsx index 1786d9c541..8eb7a4bc73 100644 --- a/cypress/component/routes/login/method.cy.tsx +++ b/cypress/component/routes/login/method.cy.tsx @@ -1,10 +1,11 @@ // cypress/component/routes/login/method.cy.tsx // // UI contract for /login/method (A-P10): identity header ("Signing in as " / -// "Not you?") and the Passkey entry firing usePasskeyLoginCeremony IN PLACE (a Button -// that lazily loads the /login/passkey challenge and submits the pre-baked Cypress -// credential) instead of navigating there. The other method entries stay plain links. -import LoginMethod from '@/routes/login/method'; +// "Not you?") and the Passkey entry firing usePasskeyLoginCeremony IN PLACE — a real to +// /login/passkey whose click JS intercepts to lazily load the challenge and submit the pre-baked +// Cypress credential, so an unhydrated visitor simply follows the href instead. Every method +// entry is a link; only the IdP rows are submit buttons (starting an intent needs an action). +import LoginMethod, { shouldRevalidate } from '@/routes/login/method'; import { ConformAdapter } from '@datum-cloud/datum-ui/form/adapters/conform'; import { setupI18n } from '@lingui/core'; import { I18nProvider } from '@lingui/react'; @@ -24,6 +25,10 @@ const METHOD_LOADER_DATA = { }; const capturedPosts: Array> = []; +// Every challenge fetch the ceremony makes. Used as the sync point for the "did the +// auto-begun attempt actually run?" assertions below — a ceremony that fails at the +// challenge produces no POST, so capturedPosts cannot serve as the observable there. +const challengeLoads: string[] = []; function withI18n(node: React.ReactNode) { const i18n = setupI18n({ locale: 'en', messages: { en: {} } }); @@ -34,19 +39,35 @@ function withI18n(node: React.ReactNode) { ); } -function mountMethod() { +// Optional overrides let sole-passkey tests exercise a different `methods` array and +// `loginName` without duplicating the router wiring — same override-and-spread idiom +// passkey-button-visibility.cy.tsx's mountLogin() already uses for this same route family. +function mountMethod(opts?: { + methods?: Array<'passkey' | 'password' | 'otp_email' | 'idp'>; + loginName?: string; + // A null challenge is the deterministic failure seam: the hook treats a missing + // publicKeyCredentialRequestOptions as a non-fatal mint failure and sets reason='unknown' + // WITHOUT touching navigator.credentials, so a spec can exercise the failure copy without + // depending on the browser's real WebAuthn behavior. + challenge?: unknown; +}) { + const loginContext = { ...LOGIN_CONTEXT, loginName: opts?.loginName ?? LOGIN_CONTEXT.loginName }; + const methodLoaderData = { + ...METHOD_LOADER_DATA, + methods: opts?.methods ?? METHOD_LOADER_DATA.methods, + }; const router = createMemoryRouter( [ { id: 'login', path: '/login', - loader: () => LOGIN_CONTEXT, + loader: () => loginContext, children: [ { id: 'method', path: 'method', element: , - loader: async () => METHOD_LOADER_DATA, + loader: async () => methodLoaderData, }, // Stub /login/passkey — same route the shared ceremony hook lazily loads // (challenge) then posts to (credential). Mirrors passkeys-ui.cy.tsx's @@ -54,13 +75,17 @@ function mountMethod() { { id: 'passkey', path: 'passkey', - loader: async () => ({ - csrfToken: 'tok-1', - loginName: 'mia@acme.test', - requestId: undefined, - organization: undefined, - publicKeyCredentialRequestOptions: { publicKey: { challenge: 'x' } }, - }), + loader: async () => { + challengeLoads.push(loginContext.loginName); + return { + csrfToken: 'tok-1', + loginName: loginContext.loginName, + requestId: undefined, + organization: undefined, + publicKeyCredentialRequestOptions: + opts && 'challenge' in opts ? opts.challenge : { publicKey: { challenge: 'x' } }, + }; + }, action: async ({ request }: { request: Request }) => { capturedPosts.push(Object.fromEntries(await request.formData())); // Truthy (not null) — mirrors a real completed action so the ceremony's @@ -73,15 +98,25 @@ function mountMethod() { }, ], { - initialEntries: ['/login/method?loginName=mia%40acme.test'], + initialEntries: [`/login/method?loginName=${encodeURIComponent(loginContext.loginName)}`], hydrationData: { - loaderData: { login: LOGIN_CONTEXT, method: METHOD_LOADER_DATA }, + loaderData: { login: loginContext, method: methodLoaderData }, }, } ); return mount(withI18n()); } +// Same observable the neighbouring "Passkey fires the ceremony in place" test above uses to +// prove a ceremony ran: the captured POST to the /login/passkey stub action. Asserting on +// this (not on ceremony.phase / button-disabled) avoids racing the Cypress fake-credential +// seam, which resolves the ceremony without ever showing a real dialog. +function expectCeremonyStarted(times: number) { + cy.wrap(null).should(() => { + expect(capturedPosts).to.have.length(times); + }); +} + describe('/login/method — identity header + in-place passkey ceremony', () => { beforeEach(() => { capturedPosts.length = 0; @@ -97,9 +132,21 @@ describe('/login/method — identity header + in-place passkey ceremony', () => .and('not.contain', 'loginName'); }); + it('Passkey is a REAL link to /login/passkey, so it works without hydration', () => { + // It was a