From cd9852f7a947bc3847c49bd4732c045b39bd619f Mon Sep 17 00:00:00 2001 From: Yahya Fakhroji Date: Mon, 20 Jul 2026 19:06:15 +0700 Subject: [PATCH 1/5] fix(authorize): route session-less select_account to /login MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit prompt=select_account returned /accounts without consulting hasSessions, so a first-time datumctl login landed on an empty account picker. Every other decideAuthorize branch already considers session state; the SAML entry path was hardened the same way in authorize.service.ts:391-396. Adds the first tests for this branch — it had none, despite three source comments warning about select_account loops. Refs #99 --- app/resources/authorize/authorize-decision.ts | 9 ++++++- .../authorize/authorize-decision.cy.ts | 26 +++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/app/resources/authorize/authorize-decision.ts b/app/resources/authorize/authorize-decision.ts index e25628eed0..3f67ecd63a 100644 --- a/app/resources/authorize/authorize-decision.ts +++ b/app/resources/authorize/authorize-decision.ts @@ -45,8 +45,15 @@ export function decideAuthorize({ const baseParams = org ? { organization: org } : undefined; if (authRequest.prompt.includes('create')) return { target: '/signup', params: baseParams }; + // An account picker with nothing to pick is a dead end — its only control is "Add an + // account". With no sessions, bootstrap straight into the identifier screen instead. + // Mirrors the SAML no-session branch (authorize.service.ts:391-396), which was already + // hardened against exactly this. Issue #99: datumctl sends prompt=select_account + // unconditionally, so a first-time login hit the empty picker. if (authRequest.prompt.includes('select_account')) - return { target: '/accounts', params: baseParams }; + return hasSessions + ? { target: '/accounts', params: baseParams } + : { target: '/login', params: baseParams }; if (authRequest.prompt.includes('login')) { const params: Record = { ...(baseParams ?? {}) }; diff --git a/cypress/component/resources/authorize/authorize-decision.cy.ts b/cypress/component/resources/authorize/authorize-decision.cy.ts index a160ab2c28..6b06de1162 100644 --- a/cypress/component/resources/authorize/authorize-decision.cy.ts +++ b/cypress/component/resources/authorize/authorize-decision.cy.ts @@ -14,4 +14,30 @@ describe('decideAuthorize', () => { expect(r.target).to.equal('error'); expect(r.error).to.equal('NO_ACTIVE_SESSION'); }); + + it('SELECT_ACCOUNT with no sessions → /login (issue #99: never an empty picker)', () => { + const r = decideAuthorize({ + authRequest: { ...base, prompt: ['select_account'] }, + hasSessions: false, + }); + expect(r.target).to.equal('/login'); + }); + + it('SELECT_ACCOUNT with sessions → /accounts (the picker is still the right screen)', () => { + const r = decideAuthorize({ + authRequest: { ...base, prompt: ['select_account'] }, + hasSessions: true, + }); + expect(r.target).to.equal('/accounts'); + }); + + it('SELECT_ACCOUNT threads organization onto the session-less /login bootstrap', () => { + const r = decideAuthorize({ + authRequest: { ...base, prompt: ['select_account'] }, + hasSessions: false, + organization: 'org-1', + }); + expect(r.target).to.equal('/login'); + expect(r.params).to.deep.equal({ organization: 'org-1' }); + }); }); From 333d413b25c6c22e7f0598a9d3938dced4795b77 Mon Sep 17 00:00:00 2001 From: Yahya Fakhroji Date: Mon, 20 Jul 2026 19:20:16 +0700 Subject: [PATCH 2/5] refactor(schemas): extract shared userCode schema switchSchema and removeSchema each carried an inline copy of the same max(64) + charset bound. The /accounts loader is about to become a third consumer, so hoist it to one source with boundary tests. No behavior change. Refs #99 --- app/resources/schemas/user-code.ts | 14 +++++++++++ app/resources/session/session.service.ts | 13 +++------- .../resources/schemas/user-code.cy.ts | 25 +++++++++++++++++++ 3 files changed, 42 insertions(+), 10 deletions(-) create mode 100644 app/resources/schemas/user-code.ts create mode 100644 cypress/component/resources/schemas/user-code.cy.ts diff --git a/app/resources/schemas/user-code.ts b/app/resources/schemas/user-code.ts new file mode 100644 index 0000000000..04a74fb00a --- /dev/null +++ b/app/resources/schemas/user-code.ts @@ -0,0 +1,14 @@ +import { z } from 'zod'; + +/** + * The OAuth device-grant `user_code` shape (RFC 8628 §6.1 permits any printable + * charset; Zitadel issues alphanumeric plus separators). + * + * Bounded so a malformed value can never pollute the `device_` requestId or a + * redirect target. Three consumers share this: switchSchema and removeSchema + * (session.service.ts) reflect it onto the post-action redirect, and the /accounts + * loader (routes/accounts.tsx) feeds it into an automatic 302. + */ +const USER_CODE_PATTERN = /^[A-Za-z0-9_-]+$/; + +export const userCodeSchema = z.string().max(64).regex(USER_CODE_PATTERN).optional(); diff --git a/app/resources/session/session.service.ts b/app/resources/session/session.service.ts index 3331b3003f..9caa821aaf 100644 --- a/app/resources/session/session.service.ts +++ b/app/resources/session/session.service.ts @@ -31,6 +31,7 @@ import type { Session, AuthMethod, LoginSettings, ProviderErrorCode } from '@/mo import { ProviderError } from '@/modules/auth/types'; import { isAllowedRequestId } from '@/resources/authorize'; import { postLoginDestinationWithSource } from '@/resources/login/post-login-destination'; +import { userCodeSchema } from '@/resources/schemas/user-code'; import { nextStepWithParams } from '@/resources/shared/next-step-params'; import { resolveOrg } from '@/resources/shared/resolve-org'; import { paths } from '@/routes/paths'; @@ -377,11 +378,7 @@ export const switchSchema = z.object({ // user reviews and authorizes — instead of the normal post-login destination. Bounded to the // OAuth device user_code shape (alphanumeric + - / _) so a malformed value can't pollute the // device_ requestId or the redirect. - userCode: z - .string() - .max(64) - .regex(/^[A-Za-z0-9_-]+$/) - .optional(), + userCode: userCodeSchema, }); export const removeSchema = z.object({ @@ -399,11 +396,7 @@ export const removeSchema = z.object({ // to /accounts (mirrors requestId for OIDC/SAML) so removing an account mid-device-grant keeps // the device context — a subsequent switch/add still returns to /device/authorize. Bounded to // the OAuth device user_code shape so a malformed value can't be reflected onto the redirect. - userCode: z - .string() - .max(64) - .regex(/^[A-Za-z0-9_-]+$/) - .optional(), + userCode: userCodeSchema, }); export type AccountActionError = diff --git a/cypress/component/resources/schemas/user-code.cy.ts b/cypress/component/resources/schemas/user-code.cy.ts new file mode 100644 index 0000000000..50475a3162 --- /dev/null +++ b/cypress/component/resources/schemas/user-code.cy.ts @@ -0,0 +1,25 @@ +import { userCodeSchema } from '@/resources/schemas/user-code'; + +describe('userCodeSchema', () => { + it('accepts an OAuth device user_code shape', () => { + expect(userCodeSchema.safeParse('WDJB-MJHT').success).to.be.true; + }); + + it('accepts underscores', () => { + expect(userCodeSchema.safeParse('WDJB_MJHT').success).to.be.true; + }); + + it('rejects query-injection characters — SECURITY', () => { + expect(userCodeSchema.safeParse('X&loginName=admin').success).to.be.false; + }); + + it('rejects a value over 64 characters', () => { + expect(userCodeSchema.safeParse('A'.repeat(65)).success).to.be.false; + }); + + it('treats absence as valid (optional)', () => { + const r = userCodeSchema.safeParse(undefined); + expect(r.success).to.be.true; + expect(r.success && r.data).to.equal(undefined); + }); +}); From 53612035c2bd2c2e2ab21d6dce7d1364d1b2b679 Mon Sep 17 00:00:00 2001 From: Yahya Fakhroji Date: Mon, 20 Jul 2026 19:40:07 +0700 Subject: [PATCH 3/5] refactor(accounts): extract addAccountHref as a shared pure function The loader is about to need the exact target the component links to. Sharing one function keeps the link and the redirect from drifting. No behavior change. Refs #99 --- app/modules/i18n/locales/en.po | 16 ++++----- app/routes/accounts.tsx | 37 ++++++++++++++------ cypress/component/routes/accounts-row.cy.tsx | 22 +++++++++++- 3 files changed, 56 insertions(+), 19 deletions(-) diff --git a/app/modules/i18n/locales/en.po b/app/modules/i18n/locales/en.po index 2a9bb5e396..37faa1aeeb 100644 --- a/app/modules/i18n/locales/en.po +++ b/app/modules/i18n/locales/en.po @@ -36,7 +36,7 @@ msgstr "A new code has been sent to your email." msgid "Activate your device" msgstr "Activate your device" -#: app/routes/accounts.tsx:126 +#: app/routes/accounts.tsx:143 msgid "Add an account" msgstr "Add an account" @@ -44,7 +44,7 @@ msgstr "Add an account" msgid "Add an extra layer of security to your account by setting up a second factor." msgstr "Add an extra layer of security to your account by setting up a second factor." -#: app/routes/accounts.tsx:218 +#: app/routes/accounts.tsx:235 msgid "Add another account" msgstr "Add another account" @@ -152,7 +152,7 @@ msgstr "Check your email" msgid "Choose a new password" msgstr "Choose a new password" -#: app/routes/accounts.tsx:100 +#: app/routes/accounts.tsx:117 msgid "Choose an account" msgstr "Choose an account" @@ -355,7 +355,7 @@ msgstr "Linked accounts" msgid "Manual setup key" msgstr "Manual setup key" -#: app/routes/accounts.tsx:177 +#: app/routes/accounts.tsx:194 msgid "Needs re-authentication" msgstr "Needs re-authentication" @@ -372,7 +372,7 @@ msgstr "No account was found and sign-up is not available." msgid "No sign-in method is available for this account." msgstr "No sign-in method is available for this account." -#: app/routes/accounts.tsx:118 +#: app/routes/accounts.tsx:135 msgid "No signed-in accounts." msgstr "No signed-in accounts." @@ -481,7 +481,7 @@ msgstr "Scan the QR code below with your authenticator app, then enter the 6-dig msgid "Security key" msgstr "Security key" -#: app/routes/accounts.tsx:101 +#: app/routes/accounts.tsx:118 msgid "Select an account to continue or add a new one." msgstr "Select an account to continue or add a new one." @@ -497,7 +497,7 @@ msgstr "Send reset link" msgid "Service temporarily unavailable. Please try again." msgstr "Service temporarily unavailable. Please try again." -#: app/routes/accounts.tsx:175 +#: app/routes/accounts.tsx:192 msgid "Session active" msgstr "Session active" @@ -813,7 +813,7 @@ msgstr "You may return to your device." msgid "You must be signed in to link an external account." msgstr "You must be signed in to link an external account." -#: app/routes/accounts.tsx:108 +#: app/routes/accounts.tsx:125 msgid "You signed in as a different account than the one you were re-authenticating. Both are kept — choose an account to continue." msgstr "You signed in as a different account than the one you were re-authenticating. Both are kept — choose an account to continue." diff --git a/app/routes/accounts.tsx b/app/routes/accounts.tsx index bacbddfd84..015b4f37af 100644 --- a/app/routes/accounts.tsx +++ b/app/routes/accounts.tsx @@ -75,6 +75,30 @@ export async function action({ request }: ActionFunctionArgs) { return accountActionOutcomeToResponse(outcome); } +/** + * The "add another account" target, shared by the component's link and the loader's + * empty-picker redirect so the two can never drift apart. + * + * In the device-grant "change account" sub-flow (userCode set) log in for that device grant + * (device_) so the fresh account auto-authorizes the device; otherwise carry the + * current ceremony requestId/organization (or nothing for a standalone add). + * paths.login.index skips undefined values, so passing organization unconditionally is safe + * even when it's absent. + */ +export function addAccountHref({ + requestId, + organization, + userCode, +}: { + requestId: string | null; + organization: string | undefined; + userCode: string | null; +}): string { + return userCode + ? paths.login.index({ requestId: `device_${userCode}`, organization }) + : paths.login.index({ requestId: requestId ?? undefined, organization }); +} + // ─── Component ─────────────────────────────────────────────────────────────── export default function AccountPicker() { @@ -86,14 +110,7 @@ export default function AccountPicker() { // banner near the top of the accounts content — no toast. const errorMessage = useAuthActionError(actionData); - // "Add another account" target. In the device-grant "change account" sub-flow (userCode set) - // log in for that device grant (device_) so the fresh account auto-authorizes the - // device; otherwise carry the current ceremony requestId/organization (or nothing for a - // standalone add). paths.login.index skips undefined values, so passing organization - // unconditionally is safe even when it's absent. - const addAccountHref = userCode - ? paths.login.index({ requestId: `device_${userCode}`, organization }) - : paths.login.index({ requestId: requestId ?? undefined, organization }); + const addAccountTarget = addAccountHref({ requestId, organization, userCode }); return ( + Add an account @@ -214,7 +231,7 @@ export default function AccountPicker() { type="quaternary" className="text-muted-foreground text-sm" as={Link} - href={addAccountHref}> + href={addAccountTarget}> Add another account diff --git a/cypress/component/routes/accounts-row.cy.tsx b/cypress/component/routes/accounts-row.cy.tsx index 0ded34d099..84b82d63be 100644 --- a/cypress/component/routes/accounts-row.cy.tsx +++ b/cypress/component/routes/accounts-row.cy.tsx @@ -8,7 +8,7 @@ // are covered by inline-action-error.cy.tsx which uses the same approach. Second-pass // trim: keeps the CSRF-bearing switch form, the separate-remove-form safety property, // and the IdP badge branch; requestId-threading and plain-login-link permutations cut. -import AccountPicker from '@/routes/accounts'; +import AccountPicker, { addAccountHref } from '@/routes/accounts'; import { ConformAdapter } from '@datum-cloud/datum-ui/form/adapters/conform'; import { setupI18n } from '@lingui/core'; import { I18nProvider } from '@lingui/react'; @@ -95,3 +95,23 @@ describe('accounts row — switch form structure', () => { ); }); }); + +describe('addAccountHref', () => { + it('carries an OIDC ceremony requestId and organization', () => { + expect( + addAccountHref({ requestId: 'oidc_abc', organization: 'org-1', userCode: null }) + ).to.equal('/login?requestId=oidc_abc&organization=org-1'); + }); + + it('prefers the device user_code, rewriting it as a device_ requestId', () => { + expect( + addAccountHref({ requestId: 'oidc_abc', organization: undefined, userCode: 'WDJB-MJHT' }) + ).to.equal('/login?requestId=device_WDJB-MJHT'); + }); + + it('omits absent values rather than emitting empty params', () => { + expect(addAccountHref({ requestId: null, organization: undefined, userCode: null })).to.equal( + '/login' + ); + }); +}); From 31e2edce7a560a16f0e85d624606e02392497f33 Mon Sep 17 00:00:00 2001 From: Yahya Fakhroji Date: Mon, 20 Jul 2026 19:51:59 +0700 Subject: [PATCH 4/5] fix(accounts): skip the empty picker during an active ceremony MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An empty account picker holding a requestId is a dead end — nothing to select, and the only control is "Add an account". Redirect straight to the identifier screen, carrying the ceremony so login resumes the OIDC/SAML/device callback. Scoped to ceremony-present: a bare empty /accounts still renders the empty state, which is the correct answer for a logout probe, a bookmark, or the legacy /ui/v2/login/accounts 301. Also validates user_code in the loader — it now feeds an automatic 302 rather than a link a human clicks. Closes #99 --- app/modules/i18n/locales/en.po | 16 +++--- app/routes/accounts.tsx | 25 +++++++++- .../routes/accounts-loader-action.cy.ts | 50 +++++++++++++++++++ 3 files changed, 82 insertions(+), 9 deletions(-) diff --git a/app/modules/i18n/locales/en.po b/app/modules/i18n/locales/en.po index 37faa1aeeb..04849f44c1 100644 --- a/app/modules/i18n/locales/en.po +++ b/app/modules/i18n/locales/en.po @@ -36,7 +36,7 @@ msgstr "A new code has been sent to your email." msgid "Activate your device" msgstr "Activate your device" -#: app/routes/accounts.tsx:143 +#: app/routes/accounts.tsx:166 msgid "Add an account" msgstr "Add an account" @@ -44,7 +44,7 @@ msgstr "Add an account" msgid "Add an extra layer of security to your account by setting up a second factor." msgstr "Add an extra layer of security to your account by setting up a second factor." -#: app/routes/accounts.tsx:235 +#: app/routes/accounts.tsx:258 msgid "Add another account" msgstr "Add another account" @@ -152,7 +152,7 @@ msgstr "Check your email" msgid "Choose a new password" msgstr "Choose a new password" -#: app/routes/accounts.tsx:117 +#: app/routes/accounts.tsx:140 msgid "Choose an account" msgstr "Choose an account" @@ -355,7 +355,7 @@ msgstr "Linked accounts" msgid "Manual setup key" msgstr "Manual setup key" -#: app/routes/accounts.tsx:194 +#: app/routes/accounts.tsx:217 msgid "Needs re-authentication" msgstr "Needs re-authentication" @@ -372,7 +372,7 @@ msgstr "No account was found and sign-up is not available." msgid "No sign-in method is available for this account." msgstr "No sign-in method is available for this account." -#: app/routes/accounts.tsx:135 +#: app/routes/accounts.tsx:158 msgid "No signed-in accounts." msgstr "No signed-in accounts." @@ -481,7 +481,7 @@ msgstr "Scan the QR code below with your authenticator app, then enter the 6-dig msgid "Security key" msgstr "Security key" -#: app/routes/accounts.tsx:118 +#: app/routes/accounts.tsx:141 msgid "Select an account to continue or add a new one." msgstr "Select an account to continue or add a new one." @@ -497,7 +497,7 @@ msgstr "Send reset link" msgid "Service temporarily unavailable. Please try again." msgstr "Service temporarily unavailable. Please try again." -#: app/routes/accounts.tsx:192 +#: app/routes/accounts.tsx:215 msgid "Session active" msgstr "Session active" @@ -813,7 +813,7 @@ msgstr "You may return to your device." msgid "You must be signed in to link an external account." msgstr "You must be signed in to link an external account." -#: app/routes/accounts.tsx:125 +#: app/routes/accounts.tsx:148 msgid "You signed in as a different account than the one you were re-authenticating. Both are kept — choose an account to continue." msgstr "You signed in as a different account than the one you were re-authenticating. Both are kept — choose an account to continue." diff --git a/app/routes/accounts.tsx b/app/routes/accounts.tsx index 015b4f37af..942934f830 100644 --- a/app/routes/accounts.tsx +++ b/app/routes/accounts.tsx @@ -5,6 +5,7 @@ import { IdpIcon } from '@/components/idp-icon/idp-icon'; import { useAuthActionError } from '@/hooks/use-auth-action-error'; import { inferIdpType } from '@/modules/auth/idp-detect'; import { isAllowedRequestId } from '@/resources/authorize'; +import { userCodeSchema } from '@/resources/schemas/user-code'; import { listAccounts, resolveAccountAction, @@ -20,6 +21,7 @@ import { Trans } from '@lingui/react/macro'; import { Trash2 } from 'lucide-react'; import { data, + redirect, useLoaderData, useActionData, type ActionFunctionArgs, @@ -50,12 +52,33 @@ export async function loader({ request }: LoaderFunctionArgs) { // Device-grant "change account": the stable user_code carries the device context so a switch // returns to /device/authorize (consent) with the now-active account, and "add account" logs // in for that device grant. - const userCode = url.searchParams.get('user_code') ?? null; + // Validate against the OAuth device user_code shape BEFORE it can reach a redirect target. + // switchSchema/removeSchema already bound it for the action path; the loader is the third + // consumer and the only one that now feeds an automatic 302 rather than a link a human clicks. + const parsedUserCode = userCodeSchema.safeParse(url.searchParams.get('user_code') ?? undefined); + const userCode = parsedUserCode.success ? (parsedUserCode.data ?? null) : null; // Re-auth landed here because the account that authenticated differs from the one being // re-authenticated (see reauthRedirect / the login + IdP identity guards). Surface a banner so // the user knows both accounts are kept and they should pick how to continue. const reauthMismatch = url.searchParams.get('reauthMismatch') === '1'; + // An empty picker mid-ceremony is a dead end: there is nothing to select and the only + // control is "Add an account". Bounce straight to the identifier screen, carrying the + // ceremony so login resumes the OIDC/SAML/device callback instead of falling through to + // the default post-login redirect. + // + // Scoped to ceremony-present on purpose. A BARE empty /accounts still renders the empty + // state — that is the correct answer for a logout probe, a bookmark, or the legacy + // /ui/v2/login/accounts 301, and acceptance/logout.acceptance.cy.ts depends on it as a + // security assertion (a signed-out session must not resolve to /signed-in). + // + // Placed BEFORE loaderCsrf so a discarded token is never minted. reauthMismatch cannot + // co-occur with an empty list (both accounts are deliberately kept), and if it somehow + // did, its "choose an account" banner is meaningless with zero accounts. + if (accounts.length === 0 && (requestId || userCode)) { + return redirect(addAccountHref({ requestId, organization, userCode })); + } + const { csrfToken, headers } = await loaderCsrf(request); return data( diff --git a/cypress/component/routes/accounts-loader-action.cy.ts b/cypress/component/routes/accounts-loader-action.cy.ts index d3333036ea..dfaf792d7c 100644 --- a/cypress/component/routes/accounts-loader-action.cy.ts +++ b/cypress/component/routes/accounts-loader-action.cy.ts @@ -57,6 +57,56 @@ describe('accounts loader', () => { expect(body.organization).to.equal(undefined); }); }); + + // ── issue #99: an empty picker mid-ceremony is a dead end ─────────────────────────── + // The harness has no sessions cookie, so listAccounts() returns [] for every case below. + + it('empty + ceremony requestId → 302 to /login carrying the ceremony', () => { + callService({ + fn: 'accountsLoader', + request: { url: `${BASE}?requestId=oidc_abc&organization=org-1` }, + }).then((v) => { + expect(v.response!.isResponse).to.be.true; + expect(v.response!.status).to.equal(302); + expect(v.response!.location).to.equal('/login?requestId=oidc_abc&organization=org-1'); + }); + }); + + it('empty + device user_code → 302 to /login?requestId=device_', () => { + callService({ + fn: 'accountsLoader', + request: { url: `${BASE}?user_code=WDJB-MJHT` }, + }).then((v) => { + expect(v.response!.isResponse).to.be.true; + expect(v.response!.location).to.equal('/login?requestId=device_WDJB-MJHT'); + }); + }); + + it('empty + malformed user_code → no redirect, no polluted target — SECURITY', () => { + callService({ + fn: 'accountsLoader', + request: { url: `${BASE}?user_code=${encodeURIComponent('X&loginName=admin')}` }, + }).then((v) => { + expect(v.response!.isResponse).to.be.false; + const body = v.response!.dataBody as Record; + expect(body.userCode).to.equal(null); + }); + }); + + it('empty + BARE url → renders the empty state (logout probe must stay put)', () => { + callService({ fn: 'accountsLoader', request: { url: BASE } }).then((v) => { + expect(v.response!.isResponse).to.be.false; + }); + }); + + it('empty + non-allowlisted requestId → no redirect — SECURITY', () => { + callService({ + fn: 'accountsLoader', + request: { url: `${BASE}?requestId=evil_payload` }, + }).then((v) => { + expect(v.response!.isResponse).to.be.false; + }); + }); }); describe('accounts action', () => { From f669325ae8edb0be62c679af16d684ccced1b38c Mon Sep 17 00:00:00 2001 From: Yahya Fakhroji Date: Mon, 20 Jul 2026 21:26:41 +0700 Subject: [PATCH 5/5] fix(accounts): detect a ceremony arriving in raw authRequest/samlRequest form MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The loader's ceremony test read only ?requestId= and ?user_code=, but a ceremony legitimately arrives in Zitadel's raw shape too — normalizeRequestId folds ?authRequest= into oidc_ and ?samlRequest= into saml_. The legacy /ui/v2/login/accounts 301 preserves the query verbatim and 'accounts' is in KNOWN_SEGMENTS, so /ui/v2/login/accounts?authRequest=abc reached the picker with an undetected ceremony: it rendered the empty state and "Add an account" resolved to a bare /login, dropping the ceremony. Reuses normalizeRequestId rather than re-deriving the mapping, so the raw shapes stay in sync with /authorize. A threaded requestId still wins. Refs #99 --- app/modules/i18n/locales/en.po | 16 ++++----- app/resources/authorize/index.ts | 7 +++- app/routes/accounts.tsx | 10 ++++-- .../routes/accounts-loader-action.cy.ts | 34 +++++++++++++++++++ 4 files changed, 56 insertions(+), 11 deletions(-) diff --git a/app/modules/i18n/locales/en.po b/app/modules/i18n/locales/en.po index 04849f44c1..8c14b4657f 100644 --- a/app/modules/i18n/locales/en.po +++ b/app/modules/i18n/locales/en.po @@ -36,7 +36,7 @@ msgstr "A new code has been sent to your email." msgid "Activate your device" msgstr "Activate your device" -#: app/routes/accounts.tsx:166 +#: app/routes/accounts.tsx:172 msgid "Add an account" msgstr "Add an account" @@ -44,7 +44,7 @@ msgstr "Add an account" msgid "Add an extra layer of security to your account by setting up a second factor." msgstr "Add an extra layer of security to your account by setting up a second factor." -#: app/routes/accounts.tsx:258 +#: app/routes/accounts.tsx:264 msgid "Add another account" msgstr "Add another account" @@ -152,7 +152,7 @@ msgstr "Check your email" msgid "Choose a new password" msgstr "Choose a new password" -#: app/routes/accounts.tsx:140 +#: app/routes/accounts.tsx:146 msgid "Choose an account" msgstr "Choose an account" @@ -355,7 +355,7 @@ msgstr "Linked accounts" msgid "Manual setup key" msgstr "Manual setup key" -#: app/routes/accounts.tsx:217 +#: app/routes/accounts.tsx:223 msgid "Needs re-authentication" msgstr "Needs re-authentication" @@ -372,7 +372,7 @@ msgstr "No account was found and sign-up is not available." msgid "No sign-in method is available for this account." msgstr "No sign-in method is available for this account." -#: app/routes/accounts.tsx:158 +#: app/routes/accounts.tsx:164 msgid "No signed-in accounts." msgstr "No signed-in accounts." @@ -481,7 +481,7 @@ msgstr "Scan the QR code below with your authenticator app, then enter the 6-dig msgid "Security key" msgstr "Security key" -#: app/routes/accounts.tsx:141 +#: app/routes/accounts.tsx:147 msgid "Select an account to continue or add a new one." msgstr "Select an account to continue or add a new one." @@ -497,7 +497,7 @@ msgstr "Send reset link" msgid "Service temporarily unavailable. Please try again." msgstr "Service temporarily unavailable. Please try again." -#: app/routes/accounts.tsx:215 +#: app/routes/accounts.tsx:221 msgid "Session active" msgstr "Session active" @@ -813,7 +813,7 @@ msgstr "You may return to your device." msgid "You must be signed in to link an external account." msgstr "You must be signed in to link an external account." -#: app/routes/accounts.tsx:148 +#: app/routes/accounts.tsx:154 msgid "You signed in as a different account than the one you were re-authenticating. Both are kept — choose an account to continue." msgstr "You signed in as a different account than the one you were re-authenticating. Both are kept — choose an account to continue." diff --git a/app/resources/authorize/index.ts b/app/resources/authorize/index.ts index 120aef1ce4..32f56d0a2e 100644 --- a/app/resources/authorize/index.ts +++ b/app/resources/authorize/index.ts @@ -1,2 +1,7 @@ // Barrel for the authorize domain. Routes and tests import from here. -export { resolveAuthorize, outcomeToResponse, isAllowedRequestId } from './authorize.service'; +export { + resolveAuthorize, + outcomeToResponse, + isAllowedRequestId, + normalizeRequestId, +} from './authorize.service'; diff --git a/app/routes/accounts.tsx b/app/routes/accounts.tsx index 942934f830..e34c12f6ea 100644 --- a/app/routes/accounts.tsx +++ b/app/routes/accounts.tsx @@ -4,7 +4,7 @@ import { FormError } from '@/components/form-error/form-error'; import { IdpIcon } from '@/components/idp-icon/idp-icon'; import { useAuthActionError } from '@/hooks/use-auth-action-error'; import { inferIdpType } from '@/modules/auth/idp-detect'; -import { isAllowedRequestId } from '@/resources/authorize'; +import { isAllowedRequestId, normalizeRequestId } from '@/resources/authorize'; import { userCodeSchema } from '@/resources/schemas/user-code'; import { listAccounts, @@ -42,8 +42,14 @@ export async function loader({ request }: LoaderFunctionArgs) { // Thread the CURRENT ceremony requestId (a mid-OIDC/SAML/device account switch reaches the // picker at /accounts?requestId=…). Only an allowlisted (oidc_/saml_/device_) id is carried; // anything else is treated as absent so a switch/remove never reflects an arbitrary value. + // + // normalizeRequestId also folds Zitadel's RAW param shapes (?authRequest= → oidc_, + // ?samlRequest= → saml_) into the same value, preferring an already-threaded requestId. + // Without it a ceremony arriving raw — e.g. the legacy /ui/v2/login/accounts?authRequest=… + // 301, which preserves the query verbatim — went undetected, so the picker rendered its + // empty state and "Add an account" dropped the ceremony. const url = new URL(request.url); - const candidate = url.searchParams.get('requestId') ?? undefined; + const candidate = normalizeRequestId(url); const requestId = isAllowedRequestId(candidate) ? candidate : null; // Thread the CURRENT ceremony organization alongside requestId — the loader previously never // read it, so a mid-ceremony org scope silently dropped off "Add an account" and the diff --git a/cypress/component/routes/accounts-loader-action.cy.ts b/cypress/component/routes/accounts-loader-action.cy.ts index dfaf792d7c..7b34e7c796 100644 --- a/cypress/component/routes/accounts-loader-action.cy.ts +++ b/cypress/component/routes/accounts-loader-action.cy.ts @@ -61,6 +61,40 @@ describe('accounts loader', () => { // ── issue #99: an empty picker mid-ceremony is a dead end ─────────────────────────── // The harness has no sessions cookie, so listAccounts() returns [] for every case below. + // A ceremony legitimately arrives in Zitadel's RAW param shape (?authRequest=/?samlRequest=) + // as well as the threaded ?requestId=. normalizeRequestId folds all three into one value, so + // the raw shapes must be detected as a ceremony too — otherwise the legacy + // /ui/v2/login/accounts?authRequest=… 301 lands on an empty picker whose "Add an account" + // link drops the ceremony entirely. + it('empty + raw authRequest → 302 to /login?requestId=oidc_', () => { + callService({ + fn: 'accountsLoader', + request: { url: `${BASE}?authRequest=abc` }, + }).then((v) => { + expect(v.response!.isResponse).to.be.true; + expect(v.response!.location).to.equal('/login?requestId=oidc_abc'); + }); + }); + + it('empty + raw samlRequest → 302 to /login?requestId=saml_', () => { + callService({ + fn: 'accountsLoader', + request: { url: `${BASE}?samlRequest=xyz` }, + }).then((v) => { + expect(v.response!.isResponse).to.be.true; + expect(v.response!.location).to.equal('/login?requestId=saml_xyz'); + }); + }); + + it('a threaded requestId wins over a raw authRequest', () => { + callService({ + fn: 'accountsLoader', + request: { url: `${BASE}?requestId=oidc_threaded&authRequest=raw` }, + }).then((v) => { + expect(v.response!.location).to.equal('/login?requestId=oidc_threaded'); + }); + }); + it('empty + ceremony requestId → 302 to /login carrying the ceremony', () => { callService({ fn: 'accountsLoader',