From fe70c2205c21d0696d5304854a7ae7bcb0690fe9 Mon Sep 17 00:00:00 2001 From: WEIREN FENG Date: Thu, 3 Sep 2026 11:46:44 -0700 Subject: [PATCH] Decide admin status from Firestore, not from the token claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A demoted admin kept full admin power for up to an hour, and could use it to re-promote themselves permanently. isAdmin() returned true on a positive `admin` custom claim alone, as a cheap path with no Firestore read. Nothing revokes an already-issued id token when onAdminStateWritten clears that claim on demotion, and rules verify only a JWT's signature and expiry — not whether the claim inside it is still true, nor even that the Auth user still exists. So for the remaining life of their token, an ex-admin still satisfied isAdmin(). That alone was an hour of banning users, reading every feedback submitter's email, and writing arbitrary fields to any user document (the isAdmin() branch on users/{uid} update has no field allowlist). `role` being writable with it made it permanent: write {role:'admin'} back to your own admin/state on the stale claim, and the trigger re-issues the claim for real. The direction of trust is what was wrong, not the caching. isNotBanned() trusts a positive `banned` claim too, but a stale ban claim keeps someone banned slightly too long — it fails safe. A stale admin claim fails open. isAdmin() now reads admin/state, which is what the callables have always done via getNotificationActor, so the two layers agree. exists() stays first so the helper returns false rather than raising for a user with no such document — an error would propagate through `isAdmin() || (own record)` and lock ordinary users out of their own reports and feedback, which is #150. Also: no client write may introduce or change `role`, not even a real admin's. Promotion is an owner operation through the console or the Admin SDK, both of which bypass rules, and the app has never written it — blockUserByAdmin, the only caller, writes the three ban fields. One compromised admin session can no longer mint more admins. `role` deliberately STAYS in isAllowedAdminStateWrite's key allowlist. That list says which keys may exist, and request.resource.data on an update is the whole post-write document, not the changed keys — so dropping it would not have stopped a client writing role, it would have stopped admins banning anyone the Admin SDK had already given one. That is the same trap hasSafePublicLocation is currently stuck in. The restriction is expressed per-operation instead: absent on create, unaffected on update. Revoking refresh tokens was considered and is not the fix: revocation stops the client obtaining a NEW id token, but the current one keeps satisfying rules until it expires. Three new tests fail on the old rules and pass on these. Two more pass on both, on purpose: one pins that a freshly promoted admin whose token carries no claim is still recognised (the propagation-lag case the claim shortcut existed for), and one pins that banning a user whose document already carries a role still works — the trap above. Co-Authored-By: Claude Opus 5 (1M context) --- firestore.rules | 77 +++++++++++++++++++++++--------- functions/src/users.ts | 31 +++++++++---- tests/rules/users.test.ts | 94 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 172 insertions(+), 30 deletions(-) diff --git a/firestore.rules b/firestore.rules index 8a8d0c6..ae6e1d2 100644 --- a/firestore.rules +++ b/firestore.rules @@ -12,28 +12,41 @@ service cloud.firestore { return isAuthenticated() && request.auth.uid == userId; } - // Admin and banned status are mirrored to Auth custom claims by the - // onAdminStateWritten trigger (functions/src/users.ts). Rules trust a - // POSITIVE token claim (cheap, no Firestore read) and fall back to - // the admin/state doc when the token doesn't carry the claim — covers - // freshly issued tokens that haven't picked up a recent change yet. + // Admin status is decided by the admin/state document, never by the token. // - // CALLABLES still read admin/state directly (via getNotificationActor) - // because custom claims have up-to-1h propagation lag, and we don't - // want a banned user to keep mutating until their token refreshes. + // This used to trust a POSITIVE `admin` custom claim on its own, as a cheap + // path with no Firestore read, falling back to the document only when the + // token didn't carry the claim. That was unsound in one direction, and the + // direction matters: + // + // isNotBanned() trusts a positive `banned` claim → a STALE claim keeps + // someone banned slightly too long. Fails safe. + // isAdmin() trusting a positive `admin` claim → a STALE claim keeps + // someone an admin for the rest of that token's life. Fails open. + // + // onAdminStateWritten clears the claim on demotion but nothing revokes the + // already-issued ID token, and rules only verify a JWT's signature and + // expiry — not that the claim inside it is still true, nor even that the + // Auth user still exists. So a demoted admin kept full admin power for up + // to an hour, and `role` being writable with it made that permanent: write + // {role:'admin'} back to your own admin/state on the stale claim, and the + // trigger re-issues the claim for real. + // + // Reading the document is what the callables have always done + // (getNotificationActor), so this makes the two layers agree. Cost is one + // document read wherever isAdmin() is actually reached; it sits behind a + // short-circuit on every hot path (see the `|| isAdmin()` shapes below). + // + // exists() must come first so this returns FALSE rather than raising an + // evaluation error for a user with no admin/state doc — an error would + // propagate through `isAdmin() || (own record)` and lock ordinary users out + // of their own reports and feedback. That is #150, pinned by a test. function isAdmin() { let adminDoc = /databases/$(database)/documents/users/$(request.auth.uid)/admin/state; - return isAuthenticated() && ( - // Guard the claim access: a token without the `admin` claim (i.e. every - // non-admin) must yield false here, not a failed expression — same - // missing-custom-claim footgun fixed for isNotBanned in #149. - ('admin' in request.auth.token && request.auth.token.admin == true) || - ( - exists(adminDoc) && - 'role' in get(adminDoc).data && - get(adminDoc).data.role == 'admin' - ) - ); + return isAuthenticated() && + exists(adminDoc) && + 'role' in get(adminDoc).data && + get(adminDoc).data.role == 'admin'; } function isNotBanned() { @@ -85,6 +98,16 @@ service cloud.firestore { hasSafePublicLocation(request.resource.data); } + // Which keys may EXIST on an admin/state document. `role` stays in this + // list deliberately: it is written by the Admin SDK (console, or + // functions/scripts/migrate-admin-state.js), which bypasses rules, so a + // real document legitimately carries it. Dropping it here would not stop + // a client writing role — it would stop a client banning anyone whose + // document already has one, because request.resource.data on an update is + // the whole POST-WRITE document, not the changed keys. That is the same + // trap hasSafePublicLocation fell into; see the audit note on it. + // + // Whether a client may write `role` is enforced per-operation below. function isAllowedAdminStateWrite() { let allowed = ['role', 'banned', 'bannedReason', 'bannedAt']; return request.resource.data.keys().hasOnly(allowed); @@ -198,8 +221,20 @@ service cloud.firestore { match /admin/state { allow read: if isOwner(userId) || isAdmin(); - allow create: if isAdmin() && isAllowedAdminStateWrite(); - allow update: if isAdmin() && isAllowedAdminStateWrite(); + // No client write may introduce or change `role`, not even an admin's. + // Promotion is an owner operation through the console or the Admin SDK, + // which bypass these rules; the app has never written role (the only + // caller, blockUserByAdmin in src/services/admin.ts, writes the three + // ban fields). Keeping it out of reach means one compromised admin + // session cannot mint more admins, and — with isAdmin() now reading + // this document rather than the token — closes the loop where a demoted + // admin re-promoted themselves on a stale claim. + allow create: if isAdmin() && + isAllowedAdminStateWrite() && + !('role' in request.resource.data); + allow update: if isAdmin() && + isAllowedAdminStateWrite() && + !request.resource.data.diff(resource.data).affectedKeys().hasAny(['role']); allow delete: if false; } } diff --git a/functions/src/users.ts b/functions/src/users.ts index e1832bd..608cb78 100644 --- a/functions/src/users.ts +++ b/functions/src/users.ts @@ -218,14 +218,29 @@ export const onUserUpdated = onDocumentWritten( } ); -// Sync admin/banned to Auth custom claims so firestore.rules can short- -// circuit cheap token-claim checks (admin == true, banned == true) ahead -// of the existing get(/admin/state) reads. Custom claims are eventually -// consistent — they only land on the user's NEXT id token, up to 1h -// later — so callables continue to read Firestore directly via -// getNotificationActor for time-critical authorization. Rules combine -// both: positive token claim trusted, negative/missing falls back to -// Firestore. +// Sync admin/banned to Auth custom claims. Custom claims are eventually +// consistent — they only land on the user's NEXT id token, up to 1h later — +// so callables read Firestore directly via getNotificationActor for +// time-critical authorization. +// +// What rules do with each claim is NOT symmetric, and the asymmetry is the +// point: +// +// `banned` is trusted positively by isNotBanned(), as a cheap path ahead +// of the get(/admin/state) fallback. A stale banned claim keeps +// someone banned slightly too long, which fails safe. +// `admin` is NOT trusted by isAdmin() any more. Nothing revokes an +// already-issued id token when this trigger clears the claim on +// demotion, and rules cannot tell a stale claim from a live one, +// so trusting it positively meant a demoted admin kept admin +// power for the rest of that token's life. isAdmin() reads this +// document instead. The claim is still set here — it costs +// nothing and other backends may want it — but do not +// reintroduce a rule that grants anything on the claim alone. +// +// Revoking refresh tokens here would not have fixed it: revocation stops the +// client getting a NEW id token, but Firestore rules validate only a JWT's +// signature and expiry, so the current one keeps working until it expires. export const onAdminStateWritten = onDocumentWritten( "users/{userId}/admin/state", async (event) => { diff --git a/tests/rules/users.test.ts b/tests/rules/users.test.ts index cc7e054..38b1a0a 100644 --- a/tests/rules/users.test.ts +++ b/tests/rules/users.test.ts @@ -133,7 +133,6 @@ describe("users/{uid}/admin/state", () => { const db = adminUser(env, ADMIN).firestore(); await assertSucceeds( setDoc(doc(db, `users/${BOB}/admin/state`), { - role: "user", banned: true, bannedReason: "spam", bannedAt: new Date(), @@ -147,6 +146,42 @@ describe("users/{uid}/admin/state", () => { ); }); + it("refuses to let even a real admin write role from the client", async () => { + // Promotion is an owner operation via the console or the Admin SDK, both + // of which bypass rules. No client path has ever needed it, so keeping it + // unreachable costs nothing and means one compromised admin session cannot + // mint more admins. + const db = adminUser(env, ADMIN).firestore(); + await assertFails( + setDoc(doc(db, `users/${BOB}/admin/state`), { role: "admin" }) + ); + await assertFails( + setDoc(doc(db, `users/${BOB}/admin/state`), { + role: "user", + banned: true, + }) + ); + }); + + it("still lets an admin ban a user whose doc already carries a role", async () => { + // The reason `role` stays in isAllowedAdminStateWrite's key allowlist. + // request.resource.data on an update is the whole post-write document, so + // removing role from hasOnly would not have blocked writing role — it + // would have blocked banning anyone the Admin SDK had given one. + await env.withSecurityRulesDisabled(async (ctx) => { + await setDoc(doc(ctx.firestore(), `users/${BOB}/admin/state`), { + role: "user", + }); + }); + const db = adminUser(env, ADMIN).firestore(); + await assertSucceeds( + updateDoc(doc(db, `users/${BOB}/admin/state`), { + banned: true, + bannedReason: "spam", + }) + ); + }); + it("lets a user read their own admin state but not someone else's", async () => { await assertSucceeds( getDoc(doc(plainUser(env, ALICE).firestore(), `users/${ALICE}/admin/state`)) @@ -189,3 +224,60 @@ describe("users/{uid}/admin/state", () => { ); }); }); + +describe("an admin custom claim is not sufficient on its own", () => { + // isAdmin() used to return true on a positive `admin` claim alone. Nothing + // revokes an already-issued ID token on demotion, and rules only check a + // JWT's signature and expiry — not whether the claim inside it is still + // true. These tests pin that the admin/state document is now the authority. + + it("grants nothing to a token claiming admin with no admin/state doc", async () => { + // ALICE is seeded with a user doc but no admin/state — the shape of a + // demoted admin whose doc was deleted, or a forged/stale token. + const db = adminUser(env, ALICE).firestore(); + await assertFails( + setDoc(doc(db, `users/${BOB}/admin/state`), { banned: true }) + ); + await assertFails(getDoc(doc(db, `users/${BOB}/admin/state`))); + }); + + it("does not let a demoted admin re-promote themselves on the stale claim", async () => { + // The exploit, end to end. The owner demotes ALICE in the console; her ID + // token keeps saying admin:true for up to an hour. Previously she could + // write role:'admin' back to her own admin/state, and + // onAdminStateWritten would re-issue the claim for real — making the + // demotion permanently reversible by the person demoted. + await env.withSecurityRulesDisabled(async (ctx) => { + await setDoc(doc(ctx.firestore(), `users/${ALICE}/admin/state`), { + role: "user", + }); + }); + const db = adminUser(env, ALICE).firestore(); + await assertFails( + updateDoc(doc(db, `users/${ALICE}/admin/state`), { role: "admin" }) + ); + await assertFails( + setDoc( + doc(db, `users/${ALICE}/admin/state`), + { role: "admin" }, + { merge: true } + ) + ); + // And the rest of what the stale claim used to buy is gone too. + await assertFails( + setDoc(doc(db, `users/${BOB}/admin/state`), { banned: true }) + ); + }); + + it("recognises a real admin whose token carries no claim at all", async () => { + // The other direction, which is why the document is read rather than + // ANDed with the claim: a freshly promoted admin must not have to wait + // out their old token. ADMIN holds role:'admin' in Firestore (seeded in + // beforeEach) and plainUser gives a token with no custom claims. + const db = plainUser(env, ADMIN).firestore(); + await assertSucceeds( + setDoc(doc(db, `users/${BOB}/admin/state`), { banned: true }) + ); + await assertSucceeds(getDoc(doc(db, `users/${BOB}/admin/state`))); + }); +});