Skip to content

Commit 12ed1e8

Browse files
yahyafakhrojiclaude
andcommitted
fix(auth): resolve confirmed findings from the Fable-5 security audit
Fixes 31 of 32 confirmed findings from the adversarially-verified ultracode audit of the auth-ui rebuild; #16 is partially fixed with a documented residual. All changes typecheck clean and pass the mirrored cypress component specs. See docs/AUDIT-FINDINGS.md for the full per-finding detail and status. Highlights by subsystem: - session/device: device-authorization grants no longer auto-complete from the forgeable GET /signed-in; they route to the CSRF-protected /device/authorize consent screen (explicit Approve showing app + scope). (#10) - rate-limit: limiters mount on '*' and self-guard on a normalized (lowercased, .data-stripped) path, closing the case-variant and single-fetch bypasses. (#1, #11) - verify: email-code resend is gated on server-verified session ownership and never 500s / enumerates; requestId validated + encoded into the /authorize hand-back. (#9, #25, #30) - signup: registration policy (allowRegister/allowPassword/passkeysType) enforced in the actions; duplicate-email responses emit a Set-Cookie (presence oracle closed, content/size residual documented — #16); fingerprintId used instead of the MaxMind token; requestId resumed on email-link complete. (#3, #4, #14, #16, #17) - sso: guarded getSession/startIdpIntent (graceful branded errors, not 500s); LDAP reauth cookie cleared; ProviderError on sign-in mapped to the branded page; deviceTrackingToken threaded on auto-link. (#2, #5, #6, #13, #19, #20, #21) - providers/mappers: U2F factor derived from the webAuthN proto factor (fixes the U2F login loop); failedAttempts extracted with the proper schema; forceMfaLocalOnly kept distinct from forceMfa. (#0, #7, #22) - session cookie: dual-format (ISO/epoch) timestamp parsing in the eviction pre-pass so the 2KB budget is respected. (#8) - webauthn: real browsers without WebAuthn see the unsupported message instead of submitting the Cypress fake credential. (#27) - fraud: MaxMind token falls back to the cookie when it lands after the poll budget; default-org cache keyed per service URL. (#28, #29) - audit logging: raw loginName replaced with hashActor across OTP/webauthn/password reset events. (#23, #24, #26) - setup: safeParse on tampered skip params; TOTP register guarded. (#12, #31) Verification (2026-07-04): the 12 findings applied in the prior batch were independently re-verified (verifier + adversary per finding). 9 closed cleanly; #10 (device-auth) was incomplete and #30 (resend test) was a regression — both re-fixed here; #16 (signup enumeration) is partial, with the residual documented and full closure (sessions-cookie encryption) deferred by owner decision. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017UhyUkaSmoNFax5wu2aHfi
1 parent 990dbcf commit 12ed1e8

38 files changed

Lines changed: 1016 additions & 379 deletions

app/components/webauthn-button/webauthn-button.tsx

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,13 @@ export function WebAuthnButton({
9090
let credential: Record<string, unknown>;
9191

9292
const isCypress = typeof window !== 'undefined' && 'Cypress' in window;
93-
if (isCypress || !isWebAuthnSupported()) {
93+
if (!isCypress && !isWebAuthnSupported()) {
94+
// Real browser without WebAuthn — surface the unsupported message immediately
95+
// instead of submitting the fake Cypress credential to the production backend.
96+
setError('webauthn-unsupported');
97+
return;
98+
}
99+
if (isCypress) {
94100
// Cypress fake-credential path: no publicKey needed.
95101
// The pre-baked credential works for both assertion and attestation because
96102
// the fake provider accepts any payload for both verifyPasskey and verifyU2F.

app/modules/auth/providers/zitadel/index.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,9 +59,12 @@ export class ZitadelAuthProvider implements AuthProvider {
5959
oidc: true,
6060
registration: true,
6161
};
62+
/** Exposed so resolve-org can key the default-org memo per Zitadel instance URL. */
63+
readonly serviceUrl: string;
6264
private readonly ctx: ZitadelCtx;
6365
constructor(opts: ZitadelOpts) {
6466
this.ctx = createZitadelCtx(opts);
67+
this.serviceUrl = opts.serviceUrl;
6568
}
6669

6770
// settings

app/modules/auth/providers/zitadel/mappers.ts

Lines changed: 27 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import {
2121
import type { JsonObject } from '@bufbuild/protobuf';
2222
import { timestampDate } from '@bufbuild/protobuf/wkt';
2323
import type { Timestamp } from '@bufbuild/protobuf/wkt';
24+
import { CredentialsCheckErrorSchema } from '@zitadel/proto/zitadel/message_pb';
2425
import { UserVerificationRequirement } from '@zitadel/proto/zitadel/session/v2/challenge_pb';
2526

2627
// gRPC status code → ProviderError code
@@ -82,7 +83,11 @@ export function normalizeError(error: unknown): ProviderError {
8283

8384
function extractFailedAttempts(e: ConnectErrorLike): number | undefined {
8485
try {
85-
const details = e.findDetails?.() ?? [];
86+
// ConnectError.findDetails REQUIRES a message schema (or registry): it dereferences
87+
// `arg.kind` immediately, so calling it with no argument throws — the catch would then
88+
// swallow it and failedAttempts would never surface. Zitadel returns the count in a
89+
// zitadel.v1.CredentialsCheckError detail, so pass its schema explicitly.
90+
const details = e.findDetails?.(CredentialsCheckErrorSchema) ?? [];
8691
for (const d of details) {
8792
if (d && typeof d === 'object' && 'failedAttempts' in d) {
8893
const v = (d as { failedAttempts: unknown }).failedAttempts;
@@ -177,7 +182,6 @@ export function toSession(
177182
totp?: { verifiedAt?: unknown };
178183
otpSms?: { verifiedAt?: unknown };
179184
otpEmail?: { verifiedAt?: unknown };
180-
u2f?: { verifiedAt?: unknown; userVerified?: unknown };
181185
};
182186
expirationDate?: unknown;
183187
changeDate?: unknown;
@@ -201,6 +205,16 @@ export function toSession(
201205
};
202206
const v = (x?: { verifiedAt?: unknown }): Date | null =>
203207
x?.verifiedAt ? tsToDate(x.verifiedAt) : null;
208+
// Real Zitadel reports BOTH passwordless passkeys and U2F security keys through the single
209+
// `webAuthN` proto factor — the session Factors message has no `u2f` field. `userVerified` is
210+
// the discriminator: passkeys verify the user (true); U2F 2nd-factor keys do not (the app
211+
// requests 'discouraged' user verification for U2F). Split that one proto factor into the two
212+
// neutral factors so passwordlessPasskeyFresh (passkey) and secondFactorFresh (u2f) each see
213+
// their own signal — without the split a completed U2F check satisfies neither rule and the
214+
// security-key login loops forever.
215+
const webAuthN = proto.factors?.webAuthN;
216+
const webAuthNVerifiedAt = v(webAuthN);
217+
const webAuthNUserVerified = Boolean(webAuthN?.userVerified);
204218
return {
205219
id: proto.id ?? '',
206220
token,
@@ -213,20 +227,22 @@ export function toSession(
213227
: undefined,
214228
factors: {
215229
password: { verifiedAt: v(proto.factors?.password) },
216-
// passkey factor carries userVerified, which drives the passwordless-passkey MFA-satisfied rule
230+
// passkey factor carries userVerified, which drives the passwordless-passkey MFA-satisfied
231+
// rule. Only a user-verified webAuthN check is a passkey; a non-verified one is a U2F key.
217232
passkey: {
218-
verifiedAt: v(proto.factors?.webAuthN),
219-
userVerified: Boolean(proto.factors?.webAuthN?.userVerified),
233+
verifiedAt: webAuthNUserVerified ? webAuthNVerifiedAt : null,
234+
userVerified: webAuthNUserVerified,
220235
},
221236
idpIntent: { verifiedAt: v(proto.factors?.intent) },
222-
// Real Zitadel populates these after an OTP/TOTP/U2F check; without
237+
// Real Zitadel populates these after an OTP/TOTP check; without
223238
// mapping them, mfa-routing.secondFactorFresh never sees a completed second factor.
224239
totp: { verifiedAt: v(proto.factors?.totp) },
225240
otpSms: { verifiedAt: v(proto.factors?.otpSms) },
226241
otpEmail: { verifiedAt: v(proto.factors?.otpEmail) },
242+
// A U2F security key surfaces as a non-user-verified webAuthN check (see split above).
227243
u2f: {
228-
verifiedAt: v(proto.factors?.u2f),
229-
userVerified: Boolean(proto.factors?.u2f?.userVerified),
244+
verifiedAt: webAuthN && !webAuthNUserVerified ? webAuthNVerifiedAt : null,
245+
userVerified: false,
230246
},
231247
},
232248
expiresAt: proto.expirationDate ? tsToIso(proto.expirationDate) : '',
@@ -340,7 +356,9 @@ export function toLoginSettings(proto: Record<string, unknown>): LoginSettings {
340356
allowRegister: Boolean(proto.allowRegister),
341357
allowExternalIdp: Boolean(proto.allowExternalIdp),
342358
passkeysType: proto.passkeysType === 1 ? 'allowed' : 'not_allowed',
343-
forceMfa: Boolean(proto.forceMfa) || Boolean(proto.forceMfaLocalOnly),
359+
forceMfa: Boolean(proto.forceMfa),
360+
// Kept distinct from forceMfa: local-only must NOT force MFA on IdP-authenticated sessions.
361+
forceMfaLocalOnly: Boolean(proto.forceMfaLocalOnly),
344362
hidePasswordReset: Boolean(proto.hidePasswordReset),
345363
ignoreUnknownUsernames: Boolean(proto.ignoreUnknownUsernames),
346364
disableLoginWithEmail: Boolean(proto.disableLoginWithEmail),

app/modules/auth/session/cookie.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import {
66
listSessions,
77
byId,
88
byLoginName,
9+
tsMs,
910
type SessionEntry,
1011
} from './session';
1112
import type { Session } from '@/modules/auth/types';
@@ -89,12 +90,14 @@ export async function readSessions(request: Request): Promise<SessionEntry[]> {
8990
export async function serializeSessions(list: SessionEntry[]): Promise<string> {
9091
// Async pre-pass: measure the REAL signed payload for each newest-k candidate list,
9192
// then let the pure capSessions pick the survivors via a lookup-based sizeOf.
92-
const byNewest = [...list].sort((a, b) => Number(b.changeTs) - Number(a.changeTs));
93+
// changeTs is a string (numeric-epoch OR ISO-8601). Number(ISO) is NaN, which made this sort
94+
// a no-op — the "newest-k" pre-pass then measured an arbitrary subset, so the eviction size
95+
// table was computed over the wrong sessions and the emitted cookie could exceed the byte
96+
// budget or over-evict. tsMs parses both formats (shared with mostRecent/listSessions).
97+
const byNewest = [...list].sort((a, b) => tsMs(b.changeTs) - tsMs(a.changeTs));
9398
const sizeByLength = new Map<number, number>();
9499
for (let k = list.length; k >= 0; k--) {
95-
const candidate = [...byNewest.slice(0, k)].sort(
96-
(a, b) => Number(a.changeTs) - Number(b.changeTs)
97-
);
100+
const candidate = [...byNewest.slice(0, k)].sort((a, b) => tsMs(a.changeTs) - tsMs(b.changeTs));
98101
const bytes = new TextEncoder().encode(await sessionsCookie.serialize(candidate)).byteLength;
99102
sizeByLength.set(k, bytes);
100103
if (bytes <= MAX_COOKIE_BYTES) break; // smaller suffixes can only be smaller; no need to measure them

app/modules/auth/session/session.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ export function removeSession(list: SessionEntry[], id: string): SessionEntry[]
2222
* which silently broke expiry filtering and recency sorting — parse both.
2323
* Unparseable values map to NaN → treated as expired / oldest.
2424
*/
25-
function tsMs(value: string): number {
25+
export function tsMs(value: string): number {
2626
const n = Number(value);
2727
return Number.isNaN(n) ? Date.parse(value) : n;
2828
}

app/modules/auth/types.ts

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -68,9 +68,7 @@ export interface U2FCreationOptions {
6868
* `SessionChallenges.otpEmailCode` so callers can complete the factor in-band.
6969
*/
7070
export type OtpEmailChallenge =
71-
| { kind: 'send' }
72-
| { kind: 'send-template'; urlTemplate: string }
73-
| { kind: 'return-code' };
71+
{ kind: 'send' } | { kind: 'send-template'; urlTemplate: string } | { kind: 'return-code' };
7472

7573
// OIDC (Phase 1) — SAML fields added in Phase 5.
7674
export interface AuthRequest {
@@ -134,7 +132,15 @@ export interface LoginSettings {
134132
allowRegister: boolean;
135133
allowExternalIdp: boolean;
136134
passkeysType: 'not_allowed' | 'allowed';
135+
/** Login policy: MFA is mandatory for all sessions (local and IdP). Proto `forceMfa`. */
137136
forceMfa: boolean;
137+
/**
138+
* Login policy: MFA is mandatory only for local (username/password) logins; sessions
139+
* authenticated via an external IdP are exempt. Proto `forceMfaLocalOnly`. Optional,
140+
* default-off. Kept distinct from `forceMfa` so IdP sessions the policy exempts are not
141+
* wrongly forced into MFA setup.
142+
*/
143+
forceMfaLocalOnly?: boolean;
138144
/** Login policy: hide the "Forgot password?" entry point. Proto `hidePasswordReset`. Optional (undefined ⇒ show). */
139145
hidePasswordReset?: boolean;
140146
/**

app/modules/fraud/maxmind-tracker.tsx

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -69,12 +69,30 @@ export function MaxMindTracker({ accountId }: MaxMindTrackerProps) {
6969
return null;
7070
}
7171

72-
/** Reads the mirrored device-tracking token at form-submit time. */
72+
/**
73+
* Reads the device-tracking token at form-submit time.
74+
*
75+
* Checks sessionStorage first (the fast path — the tracker polls the cookie and mirrors it
76+
* there on success). If the mirror is absent (e.g. the MaxMind cookie landed after the poll
77+
* budget expired), falls back to reading the cookie directly and mirrors it so future reads
78+
* are fast. This prevents the token from being permanently lost on slow connections.
79+
*/
7380
export function readMaxMindTrackingToken(): string | undefined {
7481
if (typeof window === 'undefined') return undefined;
7582
try {
76-
return window.sessionStorage.getItem(MAXMIND_TOKEN_STORAGE_KEY) || undefined;
83+
const mirrored = window.sessionStorage.getItem(MAXMIND_TOKEN_STORAGE_KEY);
84+
if (mirrored) return mirrored;
85+
} catch {
86+
// sessionStorage unavailable — fall through to cookie read
87+
}
88+
// Fallback: cookie may have arrived after the poll budget expired.
89+
const fromCookie = readMaxMindCookie();
90+
if (!fromCookie) return undefined;
91+
// Mirror it so subsequent reads (e.g. the signup interval) find it immediately.
92+
try {
93+
window.sessionStorage.setItem(MAXMIND_TOKEN_STORAGE_KEY, fromCookie);
7794
} catch {
78-
return undefined;
95+
// sessionStorage unavailable; the cookie read still returns the token this call.
7996
}
97+
return fromCookie;
8098
}

0 commit comments

Comments
 (0)