From 8793bbf229d81d85f0d62197dd125d416eaaed69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Fri, 7 Aug 2026 03:56:39 +0200 Subject: [PATCH] fix: close four small gaps around the managed-profile surface - bound profile creation at ten an hour per account, the ceiling its guardian sibling has carried since it shipped - distinguish a failed roster read from a slow one, with the way back - cover the three caller states the roster read has to refuse: an unaccepted invitation, an expired grant, and MANAGE held elsewhere - cover what deletion does to a guardian standing inside the record --- e2e/setup/managed-profile-fixture.ts | 11 +- src/app/api/managed-profiles/route.ts | 30 +++- .../settings/access/managed-profile-card.tsx | 15 ++ .../managed-profile-surface.test.ts | 162 +++++++++++++++++- 4 files changed, 213 insertions(+), 5 deletions(-) diff --git a/e2e/setup/managed-profile-fixture.ts b/e2e/setup/managed-profile-fixture.ts index fa13cf344..f010017db 100644 --- a/e2e/setup/managed-profile-fixture.ts +++ b/e2e/setup/managed-profile-fixture.ts @@ -64,15 +64,20 @@ export async function refreshGuardianStepUp(): Promise { ); // The guardian-invitation bucket, for the same reason `global-setup.ts` // clears the auth bucket between its own logins: the ceiling is ten - // invitations an hour per caller, and this journey spends one every time it + // invitations an hour per caller (and ten creations), and this journey spends + // one of each every time it // runs. On CI that never matters; locally, the eleventh run inside an hour // is answered by the product's own 429 rather than by what the test is // about, and the failure reads as a broken invitation form. Only this one // fixture account's bucket is touched. await pool.query( `DELETE FROM rate_limits - WHERE key = 'managed-profile:guardian-invite:' || - (SELECT id FROM users WHERE username = $1)`, + WHERE key IN ( + 'managed-profile:guardian-invite:' || + (SELECT id FROM users WHERE username = $1), + 'managed-profile:create:' || + (SELECT id FROM users WHERE username = $1) + )`, [E2E_GUARDIAN.username], ); } finally { diff --git a/src/app/api/managed-profiles/route.ts b/src/app/api/managed-profiles/route.ts index 964a6e0bf..d65868ebf 100644 --- a/src/app/api/managed-profiles/route.ts +++ b/src/app/api/managed-profiles/route.ts @@ -6,12 +6,30 @@ import { MFA_STEP_UP_MAX_AGE_SECONDS, requireFreshMfa, } from "@/lib/api-handler"; -import { apiSuccess, returnAllZodIssues, safeJson } from "@/lib/api-response"; +import { + apiError, + apiSuccess, + returnAllZodIssues, + safeJson, +} from "@/lib/api-response"; import { auditLog } from "@/lib/auth/audit"; import { createManagedProfile } from "@/lib/managed-profiles/create"; import { annotate } from "@/lib/logging/context"; +import { checkRateLimit } from "@/lib/rate-limit"; import { isValidTimezone } from "@/lib/tz/format"; +/** + * The same ceiling its guardian sibling carries, and for the same reason. + * + * `POST /api/managed-profiles/{id}/guardians` has been rate-limited at ten an + * hour since it shipped; this route creates a real account row on every call + * and had none. A person cannot mistype their way into a hundred accounts, but + * a script can, and the two endpoints of one family answering differently to + * the same abuse is the kind of asymmetry nobody notices until it is used. + */ +const CREATE_LIMIT = 10; +const CREATE_WINDOW_MS = 60 * 60 * 1000; + const managedProfileSchema = z .object({ displayName: z.string().trim().min(1).max(80), @@ -32,6 +50,16 @@ const managedProfileSchema = z */ export const POST = apiHandler(async (request: NextRequest) => { const { user } = await requireFreshMfa(MFA_STEP_UP_MAX_AGE_SECONDS); + // Before the body is read, exactly as the guardian route does it: the cheap + // refusal comes first so a flood costs a bucket lookup rather than a parse. + const rateLimit = await checkRateLimit( + `managed-profile:create:${user.id}`, + CREATE_LIMIT, + CREATE_WINDOW_MS, + ); + if (!rateLimit.allowed) { + return apiError("Too many profiles created, try again later", 429); + } const { data: body, error: jsonError } = await safeJson(request, { maxBytes: 64 * 1024, }); diff --git a/src/components/settings/access/managed-profile-card.tsx b/src/components/settings/access/managed-profile-card.tsx index 6fc75c404..8d75699fa 100644 --- a/src/components/settings/access/managed-profile-card.tsx +++ b/src/components/settings/access/managed-profile-card.tsx @@ -11,6 +11,7 @@ import { ManagedProfileGuardians } from "@/components/settings/access/managed-pr import { SettingsCard } from "@/components/settings/settings-card"; import { SettingsCardHeader } from "@/components/settings/_card-header"; import { ConfirmButton } from "@/components/ui/confirm-button"; +import { QueryErrorCard } from "@/components/ui/query-error-card"; import { useAuth } from "@/hooks/use-auth"; import { useTranslations } from "@/lib/i18n/context"; import { @@ -171,6 +172,20 @@ function ManagedProfileRow({ onRetry={() => remove.mutate(profile.accountId)} /> )} + {/* A failed roster read is not an absent roster. + Without this branch a 500 renders exactly as a still-loading one — + the row with no guardian panel and no delete control — and the person + waits for something that is never coming. UI-STANDARDS §6: a read + that failed says so and offers the way back. It is deliberately NOT + the same as the 404 arm: a 404 means this account is no longer a + Guardian of that profile, which is an answer, and the row correctly + shows no destructive control for it. */} + {guardians.isError && ( + void guardians.refetch()} + /> + )} {guardians.data && ( { ); }); }); + +describe("the three caller states the roster read has to refuse", () => { + it("refuses a Guardian whose invitation has not been accepted", async () => { + // PENDING confers nothing. `activeGuardianWhere` requires `acceptedAt`, so + // an invitee who can see the invitation on their own panel still cannot + // read the roster of the record it is for — and must not be able to, or + // an invitation would disclose the household before it was accepted. + const { invitee, profile } = await profileWithPendingSecondGuardian(); + signIn(invitee); + + const response = await listGuardians(profile.id); + expect(response.status).toBe(404); + expect(await errorCodeOf(response)).toBe("managed_profile.not_found"); + }); + + it("refuses a Guardian whose grant has expired", async () => { + // The other half of the same predicate, and the one a `revokedAt IS NULL` + // check alone would miss: nobody ended this grant, the clock did. + const { invitee, profile, invitation } = + await profileWithPendingSecondGuardian(); + await acceptGrant({ grantId: invitation.id, granteeId: invitee.id }); + await getPrismaClient().accountGrant.update({ + where: { id: invitation.id }, + data: { expiresAt: new Date(Date.now() - 60_000) }, + }); + signIn(invitee); + + const response = await listGuardians(profile.id); + expect(response.status).toBe(404); + expect(await errorCodeOf(response)).toBe("managed_profile.not_found"); + }); + + it("refuses a holder of MANAGE on an ordinary record, against a real managed profile", async () => { + // Distinct from the leg above that points a delegate at THEIR OWN grantor: + // this caller holds a live MANAGE grant somewhere, and aims it at somebody + // else's managed profile. Holding the level is not holding this record. + const { profile } = await profileWithPendingSecondGuardian(); + const owner = await person("owner"); + const manager = await person("manager"); + const invitation = await inviteGrant({ + grantorId: owner.id, + granteeId: manager.id, + access: "MANAGE", + scope: null, + }); + await acceptGrant({ grantId: invitation.id, granteeId: manager.id }); + signIn(manager); + + const response = await listGuardians(profile.id); + expect(response.status).toBe(404); + expect(await errorCodeOf(response)).toBe("managed_profile.not_found"); + }); +}); + +describe("what deletion does to a Guardian who is standing inside the record", () => { + it("puts their session back in their own account rather than nowhere", async () => { + // The composition worth an integration leg: `Session.actingAsUserId` is a + // foreign key with ON DELETE SET NULL, and the epoch is bumped by a + // database trigger on that column. Deleting the profile therefore has to + // leave the other Guardian's browser in a state it can prove — back in + // their own record, on a NEW epoch — rather than pointed at a row that is + // gone. A session left holding a dangling selector is the shape that + // strands somebody behind the fence with no way out. + const { creator, invitee, profile, invitation } = + await profileWithPendingSecondGuardian(); + await acceptGrant({ grantId: invitation.id, granteeId: invitee.id }); + await switchSessionTo(invitee.sessionId, profile.id); + + const before = await getPrismaClient().session.findUniqueOrThrow({ + where: { id: invitee.sessionId }, + }); + // Non-zero proof: they really were inside the record before the delete. + expect(before.actingAsUserId).toBe(profile.id); + + signIn(creator); + expect((await deleteProfile(profile.id)).status).toBe(200); + + const after = await getPrismaClient().session.findUniqueOrThrow({ + where: { id: invitee.sessionId }, + }); + // The session survives — deleting a record must not sign anybody out — + // and it is back in its own account. + expect(after.actingAsUserId).toBeNull(); + // And the context moved, so a request formed inside the deleted record is + // refused as stale rather than served against nothing. + expect(after.recordEpoch).toBeGreaterThan(before.recordEpoch); + }); +}); + +describe("how many profiles one account may create in an hour", () => { + it("refuses the eleventh, with the same ceiling the guardian route carries", async () => { + // Ten an hour. The guardian invitation has been bounded since it shipped; + // this route creates a real account row per call and was not, which is the + // kind of asymmetry between two endpoints of one family that nobody + // notices until it is used. + const creator = await person("creator"); + signIn(creator); + + for (let i = 0; i < 10; i += 1) { + const allowed = await createProfile({ + displayName: `Managed record ${i}`, + locale: "en", + timezone: "UTC", + }); + expect(allowed.status, `creation ${i}`).toBe(201); + } + + const refused = await createProfile({ + displayName: "One too many", + locale: "en", + timezone: "UTC", + }); + expect(refused.status).toBe(429); + // No error code, like every other rate-limit refusal in this family — the + // client branches on status, which is recorded in the findings. + expect(await errorCodeOf(refused)).toBeUndefined(); + // And nothing was created by the refused call. + expect( + await getPrismaClient().user.count({ + where: { managedProfileAt: { not: null } }, + }), + ).toBe(10); + }); + + it("bounds the caller and not the instance", async () => { + // A per-user bucket, so one busy account cannot stop another from creating + // a record. `checkRateLimit` is keyed by caller id; this proves the key + // really carries it rather than being a single global bucket. + const first = await person("creator"); + signIn(first); + for (let i = 0; i < 10; i += 1) { + await createProfile({ + displayName: `Managed record ${i}`, + locale: "en", + timezone: "UTC", + }); + } + expect( + ( + await createProfile({ + displayName: "One too many", + locale: "en", + timezone: "UTC", + }) + ).status, + ).toBe(429); + + const second = await person("creator"); + signIn(second); + expect( + ( + await createProfile({ + displayName: "Somebody else's first", + locale: "en", + timezone: "UTC", + }) + ).status, + ).toBe(201); + }); +});