mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-13 22:32:04 +00:00
fix(auth): link single sign-on to an existing account only after its own sign-in
The SSO callback bound a verified provider email to the local account with the same address on the spot. The local address is not verified: anyone could register with it, or type it into their profile, and the SSO user then landed in an account the other person still held the password to. The callback now links nothing. It records the link it would make in a sealed, ten-minute, HttpOnly cookie and sends the browser to the sign-in page, and the link is made only when that browser then signs in to that same account with its password (and second factor) or passkey. Under an SSO-only policy the password form opens for exactly that account. The native sign-in answers oidc_link_required; the link is confirmed once on the web. Moving an account to a new email address now also needs a fresh proof: a sign-in or second factor in the last five minutes, or the current password sent alongside the change. Without it the address is left as it was and reported in rejectedFields; the rest of the save still lands.
This commit is contained in:
1 parent
7b5bc130d8
commit
8da7583ec6
27 files changed
+878
-76
No files matched your search
@@ -33,7 +33,18 @@ vi.mock("@/lib/rate-limit", () => ({
|
||||
checkProfileEmailRateLimit: vi.fn(),
|
||||
}));
|
||||
|
||||
import { applyProfileUpdate } from "../profile-update";
|
||||
import { applyProfileUpdate as applyWithProof } from "../profile-update";
|
||||
|
||||
/** These suites are about validation and the email budget, not the proof:
|
||||
* every call is made as an owner who has just proved themselves. */
|
||||
const applyProfileUpdate = (
|
||||
userId: string,
|
||||
body: unknown,
|
||||
ipAddress?: string | null,
|
||||
) =>
|
||||
applyWithProof(userId, body, ipAddress, {
|
||||
authorizeEmailChange: async () => "ok",
|
||||
});
|
||||
import { prisma } from "@/lib/db";
|
||||
import { auditLog } from "@/lib/auth/audit";
|
||||
import { checkProfileEmailRateLimit } from "@/lib/rate-limit";
|
||||
@@ -238,3 +249,72 @@ describe("applyProfileUpdate — the email conflict is metered", () => {
|
||||
expect(checkProfileEmailRateLimit).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
});
|
||||
|
||||
describe("applyProfileUpdate — a new address needs a fresh proof", () => {
|
||||
it("refuses an address-only change without asking about the address", async () => {
|
||||
const authorizeEmailChange = vi.fn(async () => "required" as const);
|
||||
const result = await applyWithProof(
|
||||
USER_ID,
|
||||
{ email: "someone@example.test" },
|
||||
null,
|
||||
{ authorizeEmailChange },
|
||||
);
|
||||
|
||||
expect(result.ok).toBe(false);
|
||||
if (!result.ok) {
|
||||
expect(result.status).toBe(401);
|
||||
expect(result.errorCode).toBe("auth.reproof.required");
|
||||
}
|
||||
expect(emailProbes()).toHaveLength(0);
|
||||
expect(checkProfileEmailRateLimit).not.toHaveBeenCalled();
|
||||
expect(prisma.user.update).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("drops only the address when the save carries other fields", async () => {
|
||||
const result = await applyWithProof(
|
||||
USER_ID,
|
||||
{ email: "someone@example.test", heightCm: 180 },
|
||||
null,
|
||||
{ authorizeEmailChange: async () => "failed" },
|
||||
);
|
||||
|
||||
expect(result.ok).toBe(true);
|
||||
if (result.ok) {
|
||||
expect(result.rejectedFields).toEqual([
|
||||
expect.objectContaining({ path: "email", code: "reproof_failed" }),
|
||||
]);
|
||||
}
|
||||
const written = vi.mocked(prisma.user.update).mock.calls[0]?.[0] as {
|
||||
data: Record<string, unknown>;
|
||||
};
|
||||
expect(written.data).not.toHaveProperty("email");
|
||||
expect(written.data).toHaveProperty("heightCm", 180);
|
||||
});
|
||||
|
||||
it("hands the body's current password to the proof and never writes it", async () => {
|
||||
const authorizeEmailChange = vi.fn(async () => "ok" as const);
|
||||
await applyWithProof(
|
||||
USER_ID,
|
||||
{ email: "someone@example.test", currentPassword: "hunter2hunter2" },
|
||||
null,
|
||||
{ authorizeEmailChange },
|
||||
);
|
||||
expect(authorizeEmailChange).toHaveBeenCalledWith("hunter2hunter2");
|
||||
const written = vi.mocked(prisma.user.update).mock.calls[0]?.[0] as {
|
||||
data: Record<string, unknown>;
|
||||
};
|
||||
expect(JSON.stringify(written.data)).not.toContain("hunter2");
|
||||
});
|
||||
|
||||
it("does not ask when the address on file is re-posted", async () => {
|
||||
const authorizeEmailChange = vi.fn(async () => "required" as const);
|
||||
const result = await applyWithProof(
|
||||
USER_ID,
|
||||
{ email: ON_FILE, heightCm: 170 },
|
||||
null,
|
||||
{ authorizeEmailChange },
|
||||
);
|
||||
expect(result.ok).toBe(true);
|
||||
expect(authorizeEmailChange).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
@@ -16,7 +16,18 @@ vi.mock("@/lib/auth/audit", () => ({
|
||||
auditLog: vi.fn().mockResolvedValue(undefined),
|
||||
}));
|
||||
|
||||
import { applyProfileUpdate } from "../profile-update";
|
||||
import { applyProfileUpdate as applyWithProof } from "../profile-update";
|
||||
|
||||
/** These suites are about validation and the email budget, not the proof:
|
||||
* every call is made as an owner who has just proved themselves. */
|
||||
const applyProfileUpdate = (
|
||||
userId: string,
|
||||
body: unknown,
|
||||
ipAddress?: string | null,
|
||||
) =>
|
||||
applyWithProof(userId, body, ipAddress, {
|
||||
authorizeEmailChange: async () => "ok",
|
||||
});
|
||||
import { prisma } from "@/lib/db";
|
||||
|
||||
const USER_ID = "user-1";
|
||||
|
||||
@@ -282,3 +282,76 @@ async function readProof(
|
||||
const parsed = stepUpMintSchema.safeParse(body);
|
||||
return parsed.success ? parsed.data : "invalid";
|
||||
}
|
||||
|
||||
export type SensitiveChangeProof = "ok" | "required" | "failed" | "rate_limited";
|
||||
|
||||
/**
|
||||
* The proof in front of changing the account's email address.
|
||||
*
|
||||
* The address is what single sign-on matches an existing account by, so it is
|
||||
* part of how the account is reached and a stolen session must not rewrite it.
|
||||
* Accepted: on a cookie session, a sign-in or second factor inside the last
|
||||
* five minutes; on either transport, the current password alongside the
|
||||
* change. The password draws on the shared re-proof budget and a wrong one is
|
||||
* audited like every other refused proof.
|
||||
*/
|
||||
export async function authorizeSensitiveChange(args: {
|
||||
user: Pick<
|
||||
User,
|
||||
| "id"
|
||||
| "passwordHash"
|
||||
| "totpSecretEncrypted"
|
||||
| "totpLastStep"
|
||||
| "totpConfirmedAt"
|
||||
>;
|
||||
/** The session row id on a cookie request; null on a Bearer request. */
|
||||
cookieSessionId: string | null;
|
||||
currentPassword: string | null;
|
||||
ipAddress: string | null;
|
||||
stage: string;
|
||||
}): Promise<SensitiveChangeProof> {
|
||||
const { user, cookieSessionId, currentPassword, ipAddress, stage } = args;
|
||||
|
||||
if (cookieSessionId) {
|
||||
const row = await prisma.session.findUnique({
|
||||
where: { id: cookieSessionId },
|
||||
select: { mfaVerifiedAt: true, createdAt: true },
|
||||
});
|
||||
const now = Date.now();
|
||||
const recent = (d: Date | null | undefined) =>
|
||||
d != null && now - d.getTime() <= RECENT_PROOF_MAX_AGE_MS;
|
||||
if (recent(row?.mfaVerifiedAt) || recent(row?.createdAt)) return "ok";
|
||||
}
|
||||
|
||||
if (!currentPassword) return "required";
|
||||
|
||||
const rl = await checkRateLimit(
|
||||
reproofBucket(user.id),
|
||||
REPROOF_LIMIT,
|
||||
REPROOF_WINDOW_MS,
|
||||
);
|
||||
if (!rl.allowed) {
|
||||
await auditLog("auth.reproof.rate_limited", {
|
||||
userId: user.id,
|
||||
ipAddress,
|
||||
details: { stage },
|
||||
});
|
||||
return "rate_limited";
|
||||
}
|
||||
|
||||
const outcome = await verifyExistingFactorProof(user, {
|
||||
method: "password",
|
||||
password: currentPassword,
|
||||
});
|
||||
if (!outcome.ok) {
|
||||
await recordReproofFailure(
|
||||
user.id,
|
||||
ipAddress,
|
||||
stage,
|
||||
"password",
|
||||
outcome.reason,
|
||||
);
|
||||
return "failed";
|
||||
}
|
||||
return "ok";
|
||||
}
|
||||
@@ -29,6 +29,7 @@ import { issueAccessAndRefresh } from "@/lib/auth/refresh-token";
|
||||
import { recordSignInDevice } from "@/lib/auth/login-alert";
|
||||
import { detectInsecureCookieTransport } from "@/lib/auth/secure-cookie";
|
||||
import { annotate } from "@/lib/logging/context";
|
||||
import { completePendingOidcLink } from "@/lib/auth/oidc-pending-link";
|
||||
import type { User } from "@/generated/prisma/client";
|
||||
|
||||
export interface FinishLoginParams {
|
||||
@@ -130,6 +131,13 @@ export async function finishLogin(
|
||||
annotate({ meta: { session_cookie_secure_transport_mismatch: true } });
|
||||
}
|
||||
|
||||
// A browser sign-in that proved the account's own credential confirms a
|
||||
// single sign-on link the callback left pending for this very account. An
|
||||
// SSO sign-in is not such a proof, so it never completes one.
|
||||
if (!source.startsWith("login.oidc") && source !== "login.web_handoff") {
|
||||
await completePendingOidcLink(user.id, ip);
|
||||
}
|
||||
|
||||
// `createSession` anchors the onboarding cookie itself; thread the user's
|
||||
// onboarding state through.
|
||||
await createSession(
|
||||
|
||||
@@ -0,0 +1,143 @@
|
||||
/**
|
||||
* Linking a single sign-on identity to an account that already exists here.
|
||||
*
|
||||
* The SSO callback used to bind a verified IdP email to the local account with
|
||||
* the same address on the spot. The local address is NOT verified — anyone can
|
||||
* register with any address, and an account holder can type any address into
|
||||
* their profile — so whoever set that address first owned the account the
|
||||
* other person's SSO login landed in: they kept the password, and the SSO user
|
||||
* filled their health record into an account the other person could read.
|
||||
*
|
||||
* So the callback no longer links. It records what it WOULD link in a sealed,
|
||||
* short-lived, HttpOnly cookie and sends the browser to the sign-in page, and
|
||||
* the link is made only when that same browser then signs in to that same
|
||||
* account with the account's own credential — password (plus its second
|
||||
* factor, if it has one) or passkey. The IdP proved who the person is; the
|
||||
* local sign-in proves the account is theirs. Neither alone links anything.
|
||||
*
|
||||
* The cookie is AES-256-GCM sealed with the at-rest key (`encrypt`), so it can
|
||||
* be neither read nor forged, and it carries its own expiry. It is scoped to
|
||||
* `/api/auth`, the only place that reads it.
|
||||
*/
|
||||
import type { NextResponse } from "next/server";
|
||||
import { cookies } from "next/headers";
|
||||
import { prisma } from "@/lib/db";
|
||||
import { decrypt, encrypt } from "@/lib/crypto";
|
||||
import { auditLog } from "@/lib/auth/audit";
|
||||
import { annotate } from "@/lib/logging/context";
|
||||
import { shouldEmitSecureCookie } from "@/lib/auth/secure-cookie";
|
||||
|
||||
export const OIDC_LINK_COOKIE = "hl_oidc_link";
|
||||
export const OIDC_LINK_COOKIE_PATH = "/api/auth";
|
||||
export const OIDC_LINK_TTL_MS = 10 * 60 * 1000;
|
||||
|
||||
export interface PendingOidcLink {
|
||||
userId: string;
|
||||
issuer: string;
|
||||
sub: string;
|
||||
}
|
||||
|
||||
/** Attach the sealed pending link to a callback redirect. */
|
||||
export function setPendingLinkCookie(
|
||||
response: NextResponse,
|
||||
link: PendingOidcLink,
|
||||
): void {
|
||||
const sealed = encrypt(
|
||||
JSON.stringify({
|
||||
u: link.userId,
|
||||
i: link.issuer,
|
||||
s: link.sub,
|
||||
e: Date.now() + OIDC_LINK_TTL_MS,
|
||||
}),
|
||||
);
|
||||
response.cookies.set(OIDC_LINK_COOKIE, sealed, {
|
||||
httpOnly: true,
|
||||
secure: shouldEmitSecureCookie(),
|
||||
// Lax, not Strict: the browser arrives here from the IdP's redirect and
|
||||
// must carry it on the sign-in that follows. A cross-site POST cannot
|
||||
// carry it, and every reader is a JSON POST besides.
|
||||
sameSite: "lax",
|
||||
maxAge: Math.floor(OIDC_LINK_TTL_MS / 1000),
|
||||
path: OIDC_LINK_COOKIE_PATH,
|
||||
});
|
||||
}
|
||||
|
||||
/** The pending link this browser carries, or null when absent, forged or expired. */
|
||||
export async function readPendingLink(): Promise<PendingOidcLink | null> {
|
||||
let raw: string | undefined;
|
||||
try {
|
||||
raw = (await cookies()).get(OIDC_LINK_COOKIE)?.value;
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
if (!raw) return null;
|
||||
try {
|
||||
const parsed = JSON.parse(decrypt(raw)) as Record<string, unknown>;
|
||||
if (
|
||||
typeof parsed.u !== "string" ||
|
||||
typeof parsed.i !== "string" ||
|
||||
typeof parsed.s !== "string" ||
|
||||
typeof parsed.e !== "number" ||
|
||||
parsed.e <= Date.now()
|
||||
) {
|
||||
return null;
|
||||
}
|
||||
return { userId: parsed.u, issuer: parsed.i, sub: parsed.s };
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
||||
async function clearPendingLinkCookie(): Promise<void> {
|
||||
try {
|
||||
(await cookies()).set(OIDC_LINK_COOKIE, "", {
|
||||
httpOnly: true,
|
||||
secure: shouldEmitSecureCookie(),
|
||||
sameSite: "lax",
|
||||
maxAge: 0,
|
||||
path: OIDC_LINK_COOKIE_PATH,
|
||||
});
|
||||
} catch {
|
||||
// Outside a request scope there is no cookie to clear.
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Called once a browser sign-in has proved the account's OWN credential.
|
||||
* Binds the pending SSO identity when the cookie names this very account and
|
||||
* the account is still unlinked. Never throws into the sign-in: a link that
|
||||
* cannot be made leaves the login intact and the account as it was.
|
||||
*/
|
||||
export async function completePendingOidcLink(
|
||||
userId: string,
|
||||
ipAddress: string | null,
|
||||
): Promise<boolean> {
|
||||
const link = await readPendingLink();
|
||||
if (!link) return false;
|
||||
await clearPendingLinkCookie();
|
||||
if (link.userId !== userId) {
|
||||
annotate({ meta: { oidc_link: "other_account" } });
|
||||
return false;
|
||||
}
|
||||
try {
|
||||
const linked = await prisma.user.updateMany({
|
||||
where: { id: userId, oidcIssuer: null, oidcSub: null },
|
||||
data: { oidcIssuer: link.issuer, oidcSub: link.sub },
|
||||
});
|
||||
if (linked.count !== 1) {
|
||||
annotate({ meta: { oidc_link: "already_linked" } });
|
||||
return false;
|
||||
}
|
||||
} catch {
|
||||
// The (issuer, sub) pair became bound to another account in between.
|
||||
annotate({ meta: { oidc_link: "identity_taken" } });
|
||||
return false;
|
||||
}
|
||||
await auditLog("auth.oidc.linked", {
|
||||
userId,
|
||||
ipAddress,
|
||||
details: { confirmedBy: "local_sign_in" },
|
||||
});
|
||||
annotate({ action: { name: "auth.oidc.linked" } });
|
||||
return true;
|
||||
}
|
||||
@@ -16,6 +16,11 @@ import { sanitiseZodIssues, type SanitisedZodIssue } from "@/lib/api-response";
|
||||
import { checkProfileEmailRateLimit } from "@/lib/rate-limit";
|
||||
import { annotate } from "@/lib/logging/context";
|
||||
import { z } from "zod/v4";
|
||||
import {
|
||||
REPROOF_FAILED_CODE,
|
||||
REPROOF_REQUIRED_CODE,
|
||||
type SensitiveChangeProof,
|
||||
} from "@/lib/auth/existing-factor-proof";
|
||||
|
||||
const extendedProfileSchema = profileSchema.extend({
|
||||
displayName: z.string().min(1).max(80).nullable().optional(),
|
||||
@@ -185,7 +190,19 @@ export interface ApplyProfileError {
|
||||
export async function applyProfileUpdate(
|
||||
userId: string,
|
||||
body: unknown,
|
||||
ipAddress?: string | null,
|
||||
ipAddress: string | null | undefined,
|
||||
opts: {
|
||||
/**
|
||||
* Decides whether this request may move the account to a NEW email
|
||||
* address. The address is what single sign-on links an existing account
|
||||
* by, so changing it asks for a fresh proof (`authorizeSensitiveChange`).
|
||||
* Receives the `currentPassword` the body carried, if any; the password is
|
||||
* never written anywhere.
|
||||
*/
|
||||
authorizeEmailChange: (
|
||||
currentPassword: string | null,
|
||||
) => Promise<SensitiveChangeProof>;
|
||||
},
|
||||
): Promise<ApplyProfileResult | ApplyProfileError> {
|
||||
const parsed = extendedProfileSchema.safeParse(body);
|
||||
|
||||
@@ -258,6 +275,65 @@ export async function applyProfileUpdate(
|
||||
const addressOnFile = current?.email?.trim().toLowerCase() ?? null;
|
||||
|
||||
if (addressOnFile !== normalizedEmail) {
|
||||
// Asked before the uniqueness probe and before the budget: a caller who
|
||||
// cannot show it is the account owner learns nothing about which
|
||||
// addresses exist here and spends nothing.
|
||||
const rawPassword =
|
||||
typeof body === "object" && body !== null && !Array.isArray(body)
|
||||
? (body as Record<string, unknown>).currentPassword
|
||||
: undefined;
|
||||
const currentPassword =
|
||||
typeof rawPassword === "string" &&
|
||||
rawPassword.length > 0 &&
|
||||
rawPassword.length <= 512
|
||||
? rawPassword
|
||||
: null;
|
||||
const proof = await opts.authorizeEmailChange(currentPassword);
|
||||
if (proof !== "ok") {
|
||||
annotate({
|
||||
action: { name: "profile.email.reproof_refused" },
|
||||
meta: { proof },
|
||||
});
|
||||
const code =
|
||||
proof === "rate_limited"
|
||||
? "rate_limited"
|
||||
: proof === "failed"
|
||||
? "reproof_failed"
|
||||
: "reproof_required";
|
||||
// Same narrowing as the email budget below: the address is dropped,
|
||||
// the rest of the save lands, and `rejectedFields` says why.
|
||||
const { email: _unproven, ...keepable } = data;
|
||||
if (Object.keys(keepable).length === 0) {
|
||||
return {
|
||||
ok: false,
|
||||
status: proof === "rate_limited" ? 429 : 401,
|
||||
message:
|
||||
proof === "failed"
|
||||
? "Verification failed"
|
||||
: proof === "rate_limited"
|
||||
? "Too many attempts. Please wait 15 minutes."
|
||||
: "Confirm your current password to change the email address.",
|
||||
errorCode:
|
||||
proof === "failed"
|
||||
? REPROOF_FAILED_CODE
|
||||
: proof === "rate_limited"
|
||||
? "profile.update.emailRateLimited"
|
||||
: REPROOF_REQUIRED_CODE,
|
||||
};
|
||||
}
|
||||
data = keepable;
|
||||
rejectedFields = [
|
||||
...rejectedFields,
|
||||
{
|
||||
path: "email",
|
||||
code,
|
||||
message: "Confirm your current password to change the email address.",
|
||||
},
|
||||
];
|
||||
}
|
||||
}
|
||||
|
||||
if (data.email !== undefined && addressOnFile !== normalizedEmail) {
|
||||
// Charged BEFORE the probe, not after: a refused request must not
|
||||
// learn the answer it was refused for. The 409 below tells any
|
||||
// signed-in caller whether an address is registered here, and until
|
||||
|
||||
Reference in new issue
Block a user