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();