mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-10 05:12:14 +00:00
fix: reauthorize delegated replay
- Recheck the live grant before returning cached delegated responses - Refuse cached replay when the acting record cannot be resolved safely
This commit is contained in:
1 parent
f8b05d541e
commit
60b842f614
5 files changed
+123
-54
No files matched your search
@@ -2848,6 +2848,31 @@ describe("(h) the guardian route set is frozen", () => {
|
||||
});
|
||||
});
|
||||
|
||||
/* -------------------------------------------------------------------------- */
|
||||
/* (i) the delegated replay boundary */
|
||||
/* -------------------------------------------------------------------------- */
|
||||
|
||||
describe("(i) delegated replay is re-authorized", () => {
|
||||
it("checks the current grant before returning a cached delegated body", () => {
|
||||
const replay = functionSource(
|
||||
"lib/idempotency.ts",
|
||||
"canReplayDelegatedResponse",
|
||||
);
|
||||
expect(replay.length).toBeGreaterThan(0);
|
||||
expect(replay).toContain("findActiveGrant");
|
||||
expect(replay).toContain("grantorId: recordUserId");
|
||||
expect(replay).toContain("granteeId: actorUserId");
|
||||
|
||||
const wrapper = functionSource("lib/idempotency.ts", "withIdempotency");
|
||||
expect(wrapper.length).toBeGreaterThan(0);
|
||||
expect(wrapper).toContain("claimedRecord === undefined");
|
||||
const authorization = wrapper.indexOf("canReplayDelegatedResponse");
|
||||
const replayReturn = wrapper.indexOf("return cached.response");
|
||||
expect(authorization).toBeGreaterThan(-1);
|
||||
expect(replayReturn).toBeGreaterThan(authorization);
|
||||
});
|
||||
});
|
||||
|
||||
/* -------------------------------------------------------------------------- */
|
||||
/* The counter-test: quiet on the innocent tree */
|
||||
/* -------------------------------------------------------------------------- */
|
||||
|
||||
@@ -184,10 +184,16 @@ export async function getUnswitchedSession(): Promise<Awaited<
|
||||
* session through that seam would make every custom resolver responsible for a
|
||||
* question none of them asks.
|
||||
*
|
||||
* Returns the CLAIMED account, unverified and unchecked. The grant check
|
||||
* happens later, inside the handler, on every request.
|
||||
* Returns the CLAIMED account, unverified and unchecked. `undefined` means
|
||||
* the carrier was unsafe to resolve: today, that is a selector header on a
|
||||
* cookie request. The idempotency wrapper must then skip replay and let the
|
||||
* handler issue its normal refusal instead of treating ambiguity as an
|
||||
* unswitched own-record request. The grant check happens later, inside the
|
||||
* handler, on every request.
|
||||
*/
|
||||
export async function readClaimedActingAccount(): Promise<string | null> {
|
||||
export async function readClaimedActingAccount(): Promise<
|
||||
string | null | undefined
|
||||
> {
|
||||
const header = await readSelectorHeader();
|
||||
let session: Awaited<ReturnType<typeof getSession>> = null;
|
||||
try {
|
||||
@@ -198,11 +204,11 @@ export async function readClaimedActingAccount(): Promise<string | null> {
|
||||
// unauthenticated request claims nothing.
|
||||
session = null;
|
||||
}
|
||||
return carrierTarget(
|
||||
decideActingCarrier({
|
||||
transport: session ? "cookie" : "bearer",
|
||||
stamped: session?.session.actingAsUserId ?? null,
|
||||
header,
|
||||
}),
|
||||
);
|
||||
const carrier = decideActingCarrier({
|
||||
transport: session ? "cookie" : "bearer",
|
||||
stamped: session?.session.actingAsUserId ?? null,
|
||||
header,
|
||||
});
|
||||
if (carrier.kind === "misplaced-header") return undefined;
|
||||
return carrierTarget(carrier);
|
||||
}
|
||||
+50
-6
@@ -24,6 +24,7 @@ import {
|
||||
import { hashToken } from "@/lib/auth/hmac";
|
||||
import { annotate } from "@/lib/logging/context";
|
||||
import { isP2002 } from "@/lib/prisma-errors";
|
||||
import { findActiveGrant } from "@/lib/sharing/grants";
|
||||
import { decrypt, encrypt } from "@/lib/crypto";
|
||||
|
||||
const TTL_MS = 24 * 60 * 60 * 1000;
|
||||
@@ -396,6 +397,34 @@ function inflightConflictResponse(): Response {
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether a cached delegated response may still be returned.
|
||||
*
|
||||
* The wrapper runs before a route can call `requireRecordAuth`, but it must
|
||||
* not become an alternate way around that fresh grant check. A completed cell
|
||||
* carries the caller and claimed record; the grant row is the only authority
|
||||
* that decides whether that pair is live now. A missing, expired, revoked, or
|
||||
* unreadable grant deliberately falls through to the handler, whose normal
|
||||
* refusal is the only response the caller receives.
|
||||
*/
|
||||
async function canReplayDelegatedResponse(
|
||||
actorUserId: string,
|
||||
recordUserId: string,
|
||||
): Promise<boolean> {
|
||||
try {
|
||||
return (
|
||||
(await findActiveGrant({
|
||||
grantorId: recordUserId,
|
||||
granteeId: actorUserId,
|
||||
})) !== null
|
||||
);
|
||||
} catch {
|
||||
// The cache is an accelerator. A failed lookup must never return a body
|
||||
// that the route would refuse once the database is available again.
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
export function withIdempotency<
|
||||
Args extends [Request | NextRequest, ...unknown[]],
|
||||
>(
|
||||
@@ -424,12 +453,17 @@ export function withIdempotency<
|
||||
// person's record, and reports success. Nothing errors and nothing logs a
|
||||
// conflict, which is why it is folded here rather than left to call sites.
|
||||
//
|
||||
// The consequence of using the CLAIM: inside the 24h window a delegate
|
||||
// whose grant has since ended can still replay a key they already
|
||||
// completed, without the grant check running. They receive bytes they
|
||||
// already had and nothing new is written — a fresh key from them is
|
||||
// refused, because that request reaches the handler.
|
||||
// A claim only chooses the cache cell. It never carries authority: a
|
||||
// completed delegated cell is checked against the current grant again
|
||||
// immediately before its body can be returned below.
|
||||
const claimedRecord = await readClaimedActingAccount();
|
||||
if (claimedRecord === undefined) {
|
||||
// The carrier cannot safely name a record. In particular, a selector on
|
||||
// a cookie request is a misplaced claim, not an own-record request.
|
||||
// Let the route return its normal refusal instead of exposing a cached
|
||||
// body from the own-record cell.
|
||||
return handler(...args);
|
||||
}
|
||||
if (claimedRecord !== null && !selectorNamesAnAccount(claimedRecord)) {
|
||||
// A claim that names no account that could exist. The request is refused
|
||||
// downstream; skipping the cache keeps a caller from writing an
|
||||
@@ -447,7 +481,17 @@ export function withIdempotency<
|
||||
};
|
||||
|
||||
const cached = await findCached(ctx);
|
||||
if (cached?.kind === "replay") return cached.response;
|
||||
if (cached?.kind === "replay") {
|
||||
if (
|
||||
claimedRecord !== null &&
|
||||
!(await canReplayDelegatedResponse(userId, claimedRecord))
|
||||
) {
|
||||
// Do not replace the route's refusal with a cache-specific response:
|
||||
// it owns the stable 403 envelope and the associated audit trail.
|
||||
return handler(...args);
|
||||
}
|
||||
return cached.response;
|
||||
}
|
||||
if (cached?.kind === "pending") {
|
||||
// A concurrent request is mid-flight on this exact key. Refuse
|
||||
// rather than run the side-effect a second time. The client should
|
||||
|
||||
@@ -227,41 +227,4 @@ describe("the replay cell is per record", () => {
|
||||
expect(cells[0]).toMatchObject({ userId: caller.id, key: KEY });
|
||||
expect(await measurementsOf(caller.id)).toHaveLength(1);
|
||||
});
|
||||
|
||||
it("replays inside the TTL after the grant is revoked, and writes nothing", async () => {
|
||||
// The accepted consequence of folding the CLAIMED account rather than a
|
||||
// verified one: within the 24h window a revoked delegate's retry is
|
||||
// answered from the cache without the grant check running again. Pinned
|
||||
// here so it stays a decision. What matters is the second half — the
|
||||
// response is one they already received, and no new row appears.
|
||||
const delegate = await makeUser("delegate");
|
||||
const owner = await makeUser("owner");
|
||||
const grant = await writeGrant(owner.id, delegate.id);
|
||||
headerJar.set("authorization", `Bearer ${await mintToken(delegate.id)}`);
|
||||
actAs(owner.id);
|
||||
|
||||
const first = await post();
|
||||
const firstBody = await first.json();
|
||||
expect(first.status).toBe(201);
|
||||
|
||||
// The owner withdraws access. Both revocation columns move together —
|
||||
// the database refuses one without the other.
|
||||
await getPrismaClient().accountGrant.update({
|
||||
where: { id: grant.id },
|
||||
data: { revokedAt: new Date(), revokedBy: "GRANTOR" },
|
||||
});
|
||||
|
||||
const afterRevoke = await post();
|
||||
expect(afterRevoke.status).toBe(201);
|
||||
expect(afterRevoke.headers.get("X-Idempotent-Replay")).toBe("true");
|
||||
expect(await afterRevoke.json()).toEqual(firstBody);
|
||||
expect(handlerRuns).toBe(1);
|
||||
expect(await measurementsOf(owner.id)).toHaveLength(1);
|
||||
|
||||
// A request the cache does NOT answer is refused, which is what keeps the
|
||||
// window above bounded to keys the delegate had already completed.
|
||||
const fresh = await post("retry-11119999");
|
||||
expect(fresh.status).toBe(403);
|
||||
expect(await measurementsOf(owner.id)).toHaveLength(1);
|
||||
});
|
||||
});
|
||||
@@ -75,6 +75,16 @@ async function mintToken(userId: string): Promise<string> {
|
||||
return raw;
|
||||
}
|
||||
|
||||
async function mintSession(userId: string): Promise<void> {
|
||||
const session = await getPrismaClient().session.create({
|
||||
data: {
|
||||
userId,
|
||||
expiresAt: new Date(Date.now() + 60 * 60 * 1000),
|
||||
},
|
||||
});
|
||||
cookieJar.set("healthlog_session", session.id);
|
||||
}
|
||||
|
||||
async function createWriteGrant(input: {
|
||||
id: string;
|
||||
grantorId: string;
|
||||
@@ -93,8 +103,8 @@ async function createWriteGrant(input: {
|
||||
|
||||
const delegatedWrite: (request: NextRequest) => Promise<Response> = apiHandler(
|
||||
withIdempotency<[NextRequest]>(async () => {
|
||||
handlerRuns += 1;
|
||||
const { user } = await requireRecordAuth("write", "measurements");
|
||||
handlerRuns += 1;
|
||||
const row = await prisma.measurement.create({
|
||||
data: {
|
||||
userId: user.id,
|
||||
@@ -230,4 +240,25 @@ describe("delegated idempotency replay", () => {
|
||||
}),
|
||||
).toBe(1);
|
||||
});
|
||||
|
||||
it("does not replay when the cookie transport carries a misplaced selector", async () => {
|
||||
const principal = SECURITY_PRINCIPALS.ownerIdempotencyPositiveControl;
|
||||
await createUser(principal.identities.actorUserId);
|
||||
await mintSession(principal.identities.actorUserId);
|
||||
|
||||
const first = await post();
|
||||
expect(first.status).toBe(201);
|
||||
|
||||
headerJar.set(ACCOUNT_SELECTOR_HEADER, "untrusted-record-selector");
|
||||
const replay = await post();
|
||||
|
||||
expect(replay.status).toBe(403);
|
||||
expect(replay.headers.get("X-Idempotent-Replay")).not.toBe("true");
|
||||
expect(handlerRuns).toBe(1);
|
||||
expect(
|
||||
await getPrismaClient().measurement.count({
|
||||
where: { userId: principal.identities.recordUserId },
|
||||
}),
|
||||
).toBe(1);
|
||||
});
|
||||
});
|
||||
Reference in new issue
Block a user