Skip to content

Commit c126989

Browse files
committed
fix(auth): address code-review findings (HIGH→LOW) on the re-auth guard + infra
Re-auth identity guard — close the gaps the review found, via a shared helper so every login factor agrees (checkReauthIntent, case-insensitive): - H-1 passkey & security-key: add the guard to the shared webauthn-verify factory action (was bypassed → a passkey re-auth could complete as the wrong identity / never clear the intent). Now match-checks + clears + bounces to the picker on mismatch. - H-2/H-3 SSO link / auto-link / link-needs-auth / auto-create: clear the reauth-intent cookie so a stale marker can't false-mismatch a later login (notably link-needs-auth → /login → password). - Case-insensitive comparison (was exact) so IdP/SAML casing differences don't false-mismatch. - password + IdP paths refactored onto the shared checkReauthIntent. Correctness / quality: - H-5 removeAccount: build the redirect via paths.accounts() (no hand-rolled URL string). - H-6 RateLimiter: type the store as the RateLimitStore INTERFACE + narrow the union return (remove the unsafe cast) so the Redis seam is real. - userCode: bound to the OAuth device user_code shape in switch/remove schemas. - listAccounts: stop threading the stale cookie-baked requestId into display-only nextPath. - /authorize prompt=login freshness window now honors a stricter org passwordCheckLifetimeMs (only ever tightens); explicit sessionId guard instead of byId(''). - rate-limit-store: record windowMs per bucket so the sweep is correct under a shared store. - Extracted /logout into session-logout.service.ts (session.service 793->657 lines), re-exported; registered both modules in the auth-event coverage registry. Deps / docs: re-pin remix-utils to 9.3.1 (stop floating) + refresh csrf comment; document the CSP unsafe-inline accepted risk; de-TODO the LDAP-link + split-layout design notes. NOTE: js-yaml is KEPT — it satisfies @datum-cloud/datum-ui's optional peer (removing it repeats the @tanstack/react-virtual clean-install break); the review's 'unused dep' was a false positive. Tests: reauth-intent cookie + shared checkReauthIntent (match/mismatch/case-insensitive/clear); IdP guard case-insensitive case.
1 parent 231183e commit c126989

20 files changed

Lines changed: 446 additions & 228 deletions

File tree

app/layouts/split.layout.tsx

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ export default function SplitLayout({
7272
{/* Intrinsic 232×290 raster line-art. width/height pin the aspect ratio so
7373
the absolutely-positioned panel reserves space and never shifts layout (CLS).
7474
object-contain keeps the line-art crisp/uncropped while it scales down into the
75-
115px box. TODO(design): replace with a vector (SVG) or AVIF source — a designer
75+
115px box. Design note (asset pending): replace with a vector (SVG) or AVIF source — a designer
7676
asset is still needed; this raster is the only source that currently exists. */}
7777
<img
7878
src={assetUrl('/images/illustration-2.svg')}
@@ -132,7 +132,7 @@ export default function SplitLayout({
132132
</div>
133133

134134
{/* 95×38 signature raster at ~1× in a 96px-wide slot. width/height pin the box
135-
(CLS) and object-contain keeps the strokes crisp without stretching. TODO(design): a
135+
(CLS) and object-contain keeps the strokes crisp without stretching. Design note (asset pending): a
136136
vector (SVG) signature is still needed — only this low-res raster currently exists. */}
137137
<img
138138
src={assetUrl('/images/zac-sign.png')}
@@ -148,7 +148,7 @@ export default function SplitLayout({
148148
<div className="absolute right-0 bottom-0 z-0 max-w-[500px] md:max-w-[800px]">
149149
{/* Intrinsic 707×155 raster line-art stretched into a fluid box up to 800px wide,
150150
so it currently upscales (soft). width/height pin the aspect ratio for CLS; object-contain
151-
avoids the crop/extra blur object-cover introduced. TODO(design): replace with a vector
151+
avoids the crop/extra blur object-cover introduced. Design note (asset pending): replace with a vector
152152
(SVG) or higher-res AVIF source — a designer asset is still needed to render crisply at
153153
the 800px display width; the 707px raster is the only source that exists today. */}
154154
<img
Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
// @vitest-environment node
2+
//
3+
// The reauth-intent cookie + the SHARED re-auth identity guard used by every login factor
4+
// (password, IdP callback, passkey/security-key). checkReauthIntent is the single source of the
5+
// match/mismatch decision, so testing it here covers the guard logic for all factors at once.
6+
import {
7+
serializeReauthIntent,
8+
readReauthIntent,
9+
clearReauthIntent,
10+
checkReauthIntent,
11+
} from '../reauth-intent';
12+
import { describe, it, expect } from 'vitest';
13+
14+
/** A request carrying ONLY the reauth-intent cookie (or none when intent is null). */
15+
async function req(intent: string | null): Promise<Request> {
16+
const headers: Record<string, string> = {};
17+
if (intent !== null) headers.cookie = (await serializeReauthIntent(intent)).split(';')[0];
18+
return new Request('http://localhost/id/login/password', { headers });
19+
}
20+
21+
describe('reauth-intent cookie round-trip', () => {
22+
it('serialize → read returns the stored loginName', async () => {
23+
expect(await readReauthIntent(await req('alice@acme.test'))).toBe('alice@acme.test');
24+
});
25+
26+
it('read returns null when the cookie is absent', async () => {
27+
expect(await readReauthIntent(await req(null))).toBeNull();
28+
});
29+
30+
it('clear produces an immediately-expiring Set-Cookie', async () => {
31+
const cleared = await clearReauthIntent();
32+
expect(cleared).toContain('reauth-intent=');
33+
expect(cleared).toMatch(/Max-Age=0/i);
34+
});
35+
});
36+
37+
describe('checkReauthIntent (shared identity guard)', () => {
38+
it('no intent in flight → not a re-auth, no mismatch, no clear cookie', async () => {
39+
const r = await checkReauthIntent(await req(null), 'whoever@acme.test');
40+
expect(r).toEqual({ intent: null, mismatch: false });
41+
});
42+
43+
it('matching identity → no mismatch, intent echoed, clear cookie present', async () => {
44+
const r = await checkReauthIntent(await req('alice@acme.test'), 'alice@acme.test');
45+
expect(r.intent).toBe('alice@acme.test');
46+
expect(r.mismatch).toBe(false);
47+
expect(r.clearCookie).toContain('reauth-intent=');
48+
});
49+
50+
it('matches case-insensitively (IdP/SAML may return different casing)', async () => {
51+
const r = await checkReauthIntent(await req('Alice@ACME.test'), 'alice@acme.test');
52+
expect(r.mismatch).toBe(false);
53+
});
54+
55+
it('different identity → mismatch true, clear cookie still present', async () => {
56+
const r = await checkReauthIntent(await req('alice@acme.test'), 'bob@acme.test');
57+
expect(r.intent).toBe('alice@acme.test');
58+
expect(r.mismatch).toBe(true);
59+
expect(r.clearCookie).toContain('reauth-intent=');
60+
});
61+
});

app/modules/auth/session/reauth-intent.ts

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,3 +39,35 @@ export async function readReauthIntent(request: Request): Promise<string | null>
3939
export async function clearReauthIntent(): Promise<string> {
4040
return reauthIntentCookie.serialize('', { maxAge: 0 });
4141
}
42+
43+
/** Result of the shared re-auth identity guard (see {@link checkReauthIntent}). */
44+
export interface ReauthCheck {
45+
/** The identity being re-authenticated, or null when this is not a re-auth flow. */
46+
intent: string | null;
47+
/** True only when a re-auth IS in flight AND the authenticated identity differs from it. */
48+
mismatch: boolean;
49+
/**
50+
* A Set-Cookie that clears the intent marker — present whenever a re-auth resolved (intent set),
51+
* undefined otherwise. Callers MUST append it on both the match and mismatch paths so a stale
52+
* marker can never gate a later, unrelated login.
53+
*/
54+
clearCookie?: string;
55+
}
56+
57+
/**
58+
* Shared re-auth identity guard for a just-completed authentication, used by every login factor
59+
* (password, IdP callback, passkey/security-key) so they agree. Compares the authenticated
60+
* loginName against the re-auth intent CASE-INSENSITIVELY (IdPs / SAML may return a different
61+
* casing than what was stored — an exact compare would false-mismatch case-insensitive accounts).
62+
* A non-null intent always yields a clearCookie; on mismatch the caller keeps both accounts and
63+
* routes to the picker instead of completing the ceremony as the unintended identity.
64+
*/
65+
export async function checkReauthIntent(
66+
request: Request,
67+
authedLoginName: string
68+
): Promise<ReauthCheck> {
69+
const intent = await readReauthIntent(request);
70+
if (intent === null) return { intent: null, mismatch: false };
71+
const mismatch = authedLoginName.trim().toLowerCase() !== intent.trim().toLowerCase();
72+
return { intent, mismatch, clearCookie: await clearReauthIntent() };
73+
}

app/modules/i18n/locales/en.po

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ msgstr ""
1313
"Plural-Forms: \n"
1414

1515
#. placeholder {0}: attempts.count
16-
#: app/routes/login/password.tsx:158
16+
#: app/routes/login/password.tsx:157
1717
msgid "{0, plural, one {# attempt remaining.} other {# attempts remaining.}}"
1818
msgstr "{0, plural, one {# attempt remaining.} other {# attempts remaining.}}"
1919

@@ -191,7 +191,7 @@ msgstr "Continue"
191191
msgid "Continue with your provider"
192192
msgstr "Continue with your provider"
193193

194-
#: app/routes/login/password.tsx:106
194+
#: app/routes/login/password.tsx:105
195195
msgid "Could not verify password"
196196
msgstr "Could not verify password"
197197

@@ -302,8 +302,8 @@ msgstr "Enter your authenticator code"
302302
msgid "Enter your email code"
303303
msgstr "Enter your email code"
304304

305-
#: app/routes/login/password.tsx:126
306-
#: app/routes/login/password.tsx:143
305+
#: app/routes/login/password.tsx:125
306+
#: app/routes/login/password.tsx:142
307307
msgid "Enter your password"
308308
msgstr "Enter your password"
309309

@@ -315,7 +315,7 @@ msgstr "Enter your SMS code"
315315
msgid "Finish creating your account"
316316
msgstr "Finish creating your account"
317317

318-
#: app/routes/login/password.tsx:189
318+
#: app/routes/login/password.tsx:188
319319
msgid "Forgot password?"
320320
msgstr "Forgot password?"
321321

@@ -432,7 +432,7 @@ msgstr "Phone"
432432
msgid "Phone sign-in isn't available — use your email or username."
433433
msgstr "Phone sign-in isn't available — use your email or username."
434434

435-
#: app/routes/login/password.tsx:177
435+
#: app/routes/login/password.tsx:176
436436
#: app/utils/errors/auth-error-messages.tsx:27
437437
msgid "Please check your input and try again."
438438
msgstr "Please check your input and try again."
@@ -530,13 +530,13 @@ msgstr "Set up security key"
530530
msgid "Set up SMS one-time code"
531531
msgstr "Set up SMS one-time code"
532532

533-
#: app/routes/login/password.tsx:182
533+
#: app/routes/login/password.tsx:181
534534
#: app/routes/signup/index.tsx:286
535535
#: app/routes/sso/ldap.tsx:82
536536
msgid "Sign in"
537537
msgstr "Sign in"
538538

539-
#: app/routes/login/password.tsx:171
539+
#: app/routes/login/password.tsx:170
540540
#: app/routes/logout/success.tsx:22
541541
#: app/utils/errors/auth-error-recovery.tsx:47
542542
msgid "Sign in again"
@@ -815,7 +815,7 @@ msgstr "You're almost done! <0>{loginName}</0>"
815815
msgid "You've been signed out"
816816
msgstr "You've been signed out"
817817

818-
#: app/routes/login/password.tsx:153
818+
#: app/routes/login/password.tsx:152
819819
msgid "Your account is temporarily locked after too many attempts."
820820
msgstr "Your account is temporarily locked after too many attempts."
821821

@@ -839,7 +839,7 @@ msgstr "Your password has expired. Please reset it to continue."
839839
msgid "Your session has ended and you've been securely signed out of Datum. You can safely close this tab, or sign back in any time to pick up where you left off."
840840
msgstr "Your session has ended and you've been securely signed out of Datum. You can safely close this tab, or sign back in any time to pick up where you left off."
841841

842-
#: app/routes/login/password.tsx:169
842+
#: app/routes/login/password.tsx:168
843843
#: app/utils/errors/auth-error-messages.tsx:56
844844
msgid "Your session has expired."
845845
msgstr "Your session has expired."

app/resources/authorize/authorize.service.ts

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,24 @@ const DEAD_SESSION_CODES: ReadonlySet<ProviderError['code']> = new Set([
4949
// can no longer satisfy prompt=login.
5050
const FRESH_LOGIN_WINDOW_MS = 2 * 60 * 1000; // 2 minutes
5151

52+
/**
53+
* The effective anti-forgery freshness window for a prompt=login hand-back. Defaults to the fixed
54+
* FRESH_LOGIN_WINDOW_MS (redirect-latency allowance), but an org configured with a STRICTER
55+
* `passwordCheckLifetimeMs` tightens it — so a strict org's policy is honored even on the gate's
56+
* pass path (which finalizes without re-running nextStep). It only ever tightens; an unset or
57+
* longer policy keeps the 2-minute window. Settings-read failure degrades to the fixed window.
58+
*/
59+
async function freshLoginWindowMs(
60+
provider: AuthProvider,
61+
entry: { organization?: string }
62+
): Promise<number> {
63+
const settings = await provider.getLoginSettings(entry.organization).catch(() => undefined);
64+
const lifetime = settings?.passwordCheckLifetimeMs;
65+
return typeof lifetime === 'number' && lifetime > 0
66+
? Math.min(FRESH_LOGIN_WINDOW_MS, lifetime)
67+
: FRESH_LOGIN_WINDOW_MS;
68+
}
69+
5270
/**
5371
* A typed description of what the /authorize loader resolved to. The route (and the
5472
* `outcomeToResponse` translator below) turn this into a redirect/JSON Response. Keeping
@@ -313,7 +331,7 @@ async function resolveOidc(
313331
// harmless (picking your own live session), so the gate is scoped to prompt=login.
314332
const mustReauth =
315333
authRequest.prompt.includes('login') &&
316-
!primaryFresh(gate.session.factors, nowMs, FRESH_LOGIN_WINDOW_MS);
334+
!primaryFresh(gate.session.factors, nowMs, await freshLoginWindowMs(provider, entry));
317335
if (!mustReauth) return runCallback(provider, rawId, entry);
318336
// else: stale prompt=login → do NOT finalize; fall through to decideAuthorize below.
319337
}
@@ -327,7 +345,8 @@ async function resolveOidc(
327345
});
328346

329347
if (decision.target === 'callback') {
330-
const entry = byId(list, decision.params?.sessionId ?? '');
348+
if (!decision.params?.sessionId) return { kind: 'error-redirect', code: 'no_session' };
349+
const entry = byId(list, decision.params.sessionId);
331350
if (!entry) return { kind: 'error-redirect', code: 'no_session' };
332351
// Validate liveness BEFORE reuse (same self-heal as the explicit-sessionId path above). No
333352
// freshness gate here: for prompt=login decideAuthorize returns target '/login', never

0 commit comments

Comments
 (0)