From 3b829fff9b90c42ac58fdab167a223074b84fac7 Mon Sep 17 00:00:00 2001 From: WEIREN FENG Date: Thu, 3 Sep 2026 13:10:53 -0700 Subject: [PATCH] Check the deletion tombstone in rules, not just in the profile callables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A deleted account could keep writing until its id token expired. The cascade deletes users/{uid} and admin/state, then deletes the Auth record last — deliberately, so a partial failure is still retryable ("Auth is the last bridge we burn"). But rules read the resulting absence as good news: isNotDeleting() `!exists(userDoc)` → not deleting isNotBanned() `!exists(adminDoc)` → not banned and rules validate only a JWT's signature and expiry, never that the Auth user still exists. So for the rest of that token's life the holder — the ex-owner, or anyone who took the token — could re-create the world-readable user doc through completeOnboarding (a setDoc merge, which is a CREATE once the doc is gone, and onboardingComplete is allowlisted), then like and bookmark other people's posts. Each like fired onLikeCreated and incremented likeCount for a uid that no longer exists, with no cleanup pass left to revisit it. users/{uid} create had neither isNotBanned() nor isNotDeleting() on it at all. The mechanism to fix it already existed and was only half-wired. userDeletionTombstones/{uid} is written BEFORE the user doc is removed, with a 24h TTL that comfortably outlives the 1h token it exists to outlive, and ensureUserProfileCallable and updateUserProfileCallable already check it. The paths governed by rules did not. isNotDeleting() now checks both signals: deletionPending covers the cascade while it runs, the tombstone covers after it finishes. Every gate already built on isNotDeleting() — likes, bookmarks, settings, blockedUsers — is fixed by that one change, and isNotDeleting() is added to users/{uid} create and to the owner branch of update. Rules-internal exists() is not governed by the tombstone collection's own `allow read: if false`. Deletes stay open, same as for banned and mid-deletion accounts: a client tearing itself down must still be able to remove rows the cascade missed. Four new tests fail on the old rules and pass on these. Three more pass on both, on purpose: two pin that a brand-new signup — no tombstone and no user doc, which is the same "absent" the bug relied on — can still create its profile and like a post, and one pins that a finished account can still delete its own leftovers. Co-Authored-By: Claude Opus 5 (1M context) --- firestore.rules | 59 +++++++++--- tests/rules/ban-and-engagement.test.ts | 119 +++++++++++++++++++++++++ 2 files changed, 166 insertions(+), 12 deletions(-) diff --git a/firestore.rules b/firestore.rules index ae6e1d2..96a09dc 100644 --- a/firestore.rules +++ b/firestore.rules @@ -69,17 +69,42 @@ service cloud.firestore { ); } - // Block direct client writes while the account is mid-deletion. The - // deleteUserAccount callable sets users/{uid}.deletionPending = true at - // the start of its cascade; without this check the user could keep - // creating new likes / bookmarks faster than the cleanup deletes them. + // Block direct client writes for an account that is being, or has been, + // deleted. Two signals, because one document cannot cover both halves: + // + // users/{uid}.deletionPending covers the cascade WHILE IT RUNS, so the + // user cannot create likes / bookmarks faster than cleanup removes + // them. + // userDeletionTombstones/{uid} covers AFTER it finishes. The cascade + // deletes the user doc itself (and admin/state) and only then deletes + // the Auth record, deliberately — Auth is the last bridge burned so a + // partial failure is still retryable. But that leaves a window where + // the deletionPending check reads `!exists(userDoc)` as "fine", and + // isNotBanned() likewise reads a missing admin/state as "not banned". + // Rules validate only a JWT's signature and expiry, not that the Auth + // user still exists, so a token issued before deletion kept working + // for the rest of its life: re-create the world-readable user doc via + // completeOnboarding, then like and bookmark other people's posts, + // incrementing likeCount for a uid that no longer exists and that no + // cleanup pass will ever revisit. + // + // The tombstone is written BEFORE the user doc is removed + // (functions/src/users.ts) and carries a 24h TTL, which comfortably + // outlives the 1h id token it exists to outlive. Rules-internal exists() + // is not governed by the collection's own `allow read: if false`. + // + // updateUserProfileCallable and ensureUserProfileCallable already check + // this tombstone; the gap was only ever the paths governed by rules. function isNotDeleting() { let userDoc = /databases/$(database)/documents/users/$(request.auth.uid); - return isAuthenticated() && ( - !exists(userDoc) || - !('deletionPending' in get(userDoc).data) || - get(userDoc).data.deletionPending != true - ); + let tombstone = /databases/$(database)/documents/userDeletionTombstones/$(request.auth.uid); + return isAuthenticated() && + !exists(tombstone) && + ( + !exists(userDoc) || + !('deletionPending' in get(userDoc).data) || + get(userDoc).data.deletionPending != true + ); } function isAllowedUserUpdate() { @@ -153,9 +178,19 @@ service cloud.firestore { // ========= users ========= match /users/{userId} { allow read: if true; - // Owner can only create with safe fields — no role, banned, email - allow create: if isOwner(userId) && isAllowedUserCreate(); - allow update: if (isOwner(userId) && isAllowedUserUpdate()) || isAdmin(); + // Owner can only create with safe fields — no role, banned, email. + // + // isNotDeleting() is what stops resurrection. The cascade removes this + // document, so completeOnboarding's setDoc(..., {merge:true}) becomes a + // CREATE, and onboardingComplete is an allowlisted field — a deleted + // account could rebuild its own world-readable profile on a token that + // had not expired yet. A brand-new signup is unaffected: no tombstone + // and no user doc both read as "not deleting". + allow create: if isOwner(userId) && + isNotDeleting() && + isAllowedUserCreate(); + allow update: if (isOwner(userId) && isNotDeleting() && isAllowedUserUpdate()) || + isAdmin(); // User deletion is handled exclusively by backend account-deletion logic. allow delete: if false; diff --git a/tests/rules/ban-and-engagement.test.ts b/tests/rules/ban-and-engagement.test.ts index c5d2588..7d18524 100644 --- a/tests/rules/ban-and-engagement.test.ts +++ b/tests/rules/ban-and-engagement.test.ts @@ -227,6 +227,125 @@ describe("bookmarks", () => { }); }); +describe("an account whose deletion cascade has already finished", () => { + // The mid-cascade state is covered above via DELETING's deletionPending + // flag. This is the state AFTER the cascade: user doc gone, admin/state + // gone, tombstone written, Auth record deleted last — and an id token issued + // before all that, still inside its hour. + // + // Rules check a JWT's signature and expiry, not whether the Auth user still + // exists, so the token keeps authenticating. Without the tombstone check, + // `!exists(userDoc)` read as "not deleting" and a missing admin/state read + // as "not banned", so every isNotBanned() && isNotDeleting() gate opened. + const GHOST = "ghost-user"; + + beforeEach(async () => { + await env.withSecurityRulesDisabled(async (ctx) => { + const db = ctx.firestore(); + // Exactly what deleteUserAccount leaves behind. + await setDoc(doc(db, `userDeletionTombstones/${GHOST}`), { + userId: GHOST, + reason: "account_deleted", + createdAt: new Date(), + expiresAt: new Date(Date.now() + 24 * 60 * 60 * 1000), + }); + }); + }); + + it("cannot rebuild its own user document", async () => { + // completeOnboarding is a setDoc(..., {merge:true}); with the doc deleted + // that is a create, and onboardingComplete is an allowlisted field. This + // was the resurrection path — and the rebuilt doc is world-readable. + const db = plainUser(env, GHOST).firestore(); + await assertFails( + setDoc(doc(db, "users", GHOST), { onboardingComplete: true }, { merge: true }) + ); + }); + + it("cannot like other people's posts", async () => { + // Each like would fire onLikeCreated and increment likeCount for a uid + // that no longer exists, with no cleanup pass left to undo it. + const db = plainUser(env, GHOST).firestore(); + await assertFails( + setDoc(doc(db, `posts/${POST}/likes/${GHOST}`), { + userId: GHOST, + postId: POST, + createdAt: serverTimestamp(), + counted: false, + }) + ); + }); + + it("cannot create bookmarks", async () => { + const db = plainUser(env, GHOST).firestore(); + await assertFails( + setDoc(doc(db, `users/${GHOST}/bookmarks/${POST}`), { + createdAt: serverTimestamp(), + }) + ); + }); + + it("cannot block anyone or write settings", async () => { + const db = plainUser(env, GHOST).firestore(); + await assertFails( + setDoc(doc(db, `users/${GHOST}/blockedUsers/${ALICE}`), { + blockedAt: serverTimestamp(), + }) + ); + await assertFails( + setDoc( + doc(db, `users/${GHOST}/settings/preferences`), + { language: "en" }, + { merge: true } + ) + ); + }); + + it("can still delete its own leftovers, so cleanup paths keep working", async () => { + // Same reasoning as the banned/mid-deletion cases: delete must stay open + // or a client tearing itself down strands rows the cascade already missed. + await env.withSecurityRulesDisabled(async (ctx) => { + const db = ctx.firestore(); + await setDoc(doc(db, `posts/${POST}/likes/${GHOST}`), { + userId: GHOST, + postId: POST, + createdAt: new Date(), + }); + await setDoc(doc(db, `users/${GHOST}/bookmarks/${POST}`), { + createdAt: new Date(), + }); + }); + const db = plainUser(env, GHOST).firestore(); + await assertSucceeds(deleteDoc(doc(db, `posts/${POST}/likes/${GHOST}`))); + await assertSucceeds(deleteDoc(doc(db, `users/${GHOST}/bookmarks/${POST}`))); + }); +}); + +describe("a brand-new signup is not mistaken for a deleted one", () => { + // The guard against fixing the above by breaking onboarding: a fresh uid has + // no tombstone AND no user doc, and both must read as "not deleting". + const FRESH = "fresh-user"; + + it("can create its own user document", async () => { + const db = plainUser(env, FRESH).firestore(); + await assertSucceeds( + setDoc(doc(db, "users", FRESH), { onboardingComplete: true }) + ); + }); + + it("can like a post", async () => { + const db = plainUser(env, FRESH).firestore(); + await assertSucceeds( + setDoc(doc(db, `posts/${POST}/likes/${FRESH}`), { + userId: FRESH, + postId: POST, + createdAt: serverTimestamp(), + counted: false, + }) + ); + }); +}); + describe("callable-only collections reject direct client writes", () => { it("refuses creating a post, comment, pet or meetup from a client", async () => { const db = plainUser(env, ALICE).firestore();