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`))); + }); +});