diff --git a/app/modules/i18n/locales/en.po b/app/modules/i18n/locales/en.po index 2a9bb5e396..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:126 +#: 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:218 +#: 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:100 +#: 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:177 +#: 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:118 +#: 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:101 +#: 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:175 +#: 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:108 +#: 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/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/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/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/app/routes/accounts.tsx b/app/routes/accounts.tsx index bacbddfd84..e34c12f6ea 100644 --- a/app/routes/accounts.tsx +++ b/app/routes/accounts.tsx @@ -4,7 +4,8 @@ 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, resolveAccountAction, @@ -20,6 +21,7 @@ import { Trans } from '@lingui/react/macro'; import { Trash2 } from 'lucide-react'; import { data, + redirect, useLoaderData, useActionData, type ActionFunctionArgs, @@ -40,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 @@ -50,12 +58,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( @@ -75,6 +104,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 +139,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 +260,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/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' }); + }); }); 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); + }); +}); diff --git a/cypress/component/routes/accounts-loader-action.cy.ts b/cypress/component/routes/accounts-loader-action.cy.ts index d3333036ea..7b34e7c796 100644 --- a/cypress/component/routes/accounts-loader-action.cy.ts +++ b/cypress/component/routes/accounts-loader-action.cy.ts @@ -57,6 +57,90 @@ 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. + + // 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', + 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', () => { 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' + ); + }); +});