From 08feeeea59bbf7708e01ed9bedf848645c5f8669 Mon Sep 17 00:00:00 2001 From: WEIREN FENG Date: Thu, 3 Sep 2026 22:28:54 -0700 Subject: [PATCH] Stop a legacy location map from blocking every write to a user document MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit hasSafePublicLocation was written for the create path and used on the update path too. On an update `request.resource.data` is the whole POST-WRITE document, not the changed keys, so its hasOnly(['city','state','updatedAt']) ran against a `location` map the write may never have touched. Documents written before the private-location split still carry precise coordinates there. saveUserLocation writes with setDoc(..., {merge:true}), which DEEP-merges nested maps, so the old lat/lng survived the write, failed the allowlist, and the update was denied. getUserLocation swallows that with .catch(() => undefined) and AuthContext retries it on every sign-in, so it failed silently and forever. The damage was not limited to location. Because the check applied to every update, it blocked EVERY allowed write to those documents — even {onboardingComplete: true}. For affected users "Update location" in Settings failed with a generic toast and onboarding could not complete. That is live functional breakage, not only a privacy leak. Both were confirmed against the emulator before this change; the tests here are that check, kept. The rule now expresses what was actually meant: a client may never introduce or change lat/lng. It may drop them, and it may leave inherited ones untouched — which is what lets these documents be written at all again. The create path keeps the strict whole-document check, where it is correct: on a create the post-write document IS the write. This is the rules half only. The residual coordinates are still in those world-readable documents and still readable by an unauthenticated stranger; there is a test pinning that rather than implying otherwise. Removing them needs production writes and is not in this PR. Also documents settingsTypesOk, which receives the whole post-write document and deliberately has no hasOnly. That looks like an oversight and is the opposite: an allowlist there would constrain fields the write never touched and retro-lock old settings documents — the same trap, one helper away. It now says so, and points at this fix. Co-Authored-By: Claude Opus 5 (1M context) --- firestore.rules | 61 ++++++++++++++++++++++++- tests/rules/users.test.ts | 95 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 153 insertions(+), 3 deletions(-) diff --git a/firestore.rules b/firestore.rules index 782c172..008cb40 100644 --- a/firestore.rules +++ b/firestore.rules @@ -112,7 +112,7 @@ service cloud.firestore { // so displayName uniqueness is enforced through /usernames reservations. let allowed = ['location', 'onboardingComplete']; return request.resource.data.diff(resource.data).affectedKeys().hasOnly(allowed) && - hasSafePublicLocation(request.resource.data); + hasSafeLocationChange(); } // Safe fields allowed during user document creation. @@ -138,6 +138,9 @@ service cloud.firestore { return request.resource.data.keys().hasOnly(allowed); } + // CREATE path only. On a create the post-write document IS the write, so + // checking the whole thing is exactly right: a new user document may not + // carry coordinates at all. function hasSafePublicLocation(userData) { return !('location' in userData) || ( userData.location is map && @@ -147,6 +150,49 @@ service cloud.firestore { ); } + // UPDATE path. The create-shaped check above was being used here too, and + // on an update `request.resource.data` is the whole POST-WRITE document, + // not the changed keys — so a `location` map that already contained + // lat/lng failed `hasOnly(['city','state','updatedAt'])` no matter what + // the write actually touched. + // + // That was live breakage, not just a stale guard. Documents written before + // the private-location split still carry precise coordinates, and + // saveUserLocation writes with setDoc(..., {merge:true}), which DEEP-merges + // nested maps — so the old lat/lng survived the write, failed the check, + // and the update was denied. `.catch(() => undefined)` in getUserLocation + // swallowed it, and AuthContext retried it on every sign-in, forever. + // Worse, it blocked EVERY allowed write to those documents: even + // {onboardingComplete: true} was rejected, so "Update location" in Settings + // failed silently for those users and onboarding could not complete. + // Confirmed against the emulator before this change. + // + // The rule this expresses: a client may never introduce or change lat/lng. + // It may drop them, and it may leave inherited ones untouched — which is + // what lets these documents be written again at all. The residual + // coordinates are a privacy problem that only a migration can fix; this is + // the half that stops the bleeding. + function hasSafeLocationChange() { + let after = request.resource.data; + let before = resource.data; + return !('location' in after) || ( + after.location is map && + after.location.keys().hasOnly(['city', 'state', 'updatedAt', 'lat', 'lng']) && + (!('city' in after.location) || after.location.city is string) && + (!('state' in after.location) || after.location.state is string) && + (!('lat' in after.location) || ( + 'location' in before && + 'lat' in before.location && + after.location.lat == before.location.lat + )) && + (!('lng' in after.location) || ( + 'location' in before && + 'lng' in before.location && + after.location.lng == before.location.lng + )) + ); + } + // ----- private per-user settings docs ----- // Two known documents live under users/{uid}/settings, both written from // the client with setDoc(..., { merge: true }): @@ -163,6 +209,19 @@ service cloud.firestore { affectedKeys.hasOnly(['lat', 'lng', 'city', 'state', 'updatedAt'])); } + // Type checks only, each guarded by `in`. There is DELIBERATELY no + // keys().hasOnly(...) here, and it is not an oversight to tidy up. + // + // This receives the whole post-write document, so an allowlist would + // constrain fields the write never touched — and a legacy key on an old + // settings document would then block every future write to it. That is + // exactly the trap hasSafePublicLocation fell into on users/{uid}, where + // it broke Settings for real users until it was fixed above. The key + // allowlist belongs on settingsKeysOk, which is handed + // diff(...).affectedKeys() on update and the full key set on create. + // + // If you are here to "finish" this function with a hasOnly, read + // hasSafeLocationChange first. function settingsTypesOk(data) { return (!('likeNotifications' in data) || data.likeNotifications is bool) && (!('commentNotifications' in data) || data.commentNotifications is bool) && diff --git a/tests/rules/users.test.ts b/tests/rules/users.test.ts index 38b1a0a..d9f67f5 100644 --- a/tests/rules/users.test.ts +++ b/tests/rules/users.test.ts @@ -1,10 +1,17 @@ -import { afterAll, beforeAll, beforeEach, describe, it } from "vitest"; +import { afterAll, beforeAll, beforeEach, describe, expect, it } from "vitest"; import { assertFails, assertSucceeds, type RulesTestEnvironment, } from "@firebase/rules-unit-testing"; -import { doc, getDoc, setDoc, updateDoc, deleteDoc } from "firebase/firestore"; +import { + doc, + getDoc, + setDoc, + updateDoc, + deleteDoc, + serverTimestamp, +} from "firebase/firestore"; import { adminUser, bannedUser, makeTestEnv, plainUser } from "./env"; // The rules are the last line of defence on user documents: everything that @@ -281,3 +288,87 @@ describe("an admin custom claim is not sufficient on its own", () => { await assertSucceeds(getDoc(doc(db, `users/${BOB}/admin/state`))); }); }); + +describe("a user document carrying legacy precise coordinates", () => { + // Documents written before the private-location split still hold + // location.lat/lng in the world-readable user doc. saveUserLocation writes + // with setDoc(..., {merge:true}), which DEEP-merges nested maps, so those + // coordinates survive the write. The old check ran hasOnly(city/state/ + // updatedAt) against the whole post-write document, so it failed — and + // because it was applied to every update, it blocked writes that had + // nothing to do with location at all. + const LEGACY = "legacy-coords-user"; + const COORDS = { city: "Boston", state: "MA", lat: 42.3601, lng: -71.0589 }; + + beforeEach(async () => { + await env.withSecurityRulesDisabled(async (ctx) => { + await setDoc(doc(ctx.firestore(), "users", LEGACY), { + displayName: LEGACY, + location: { ...COORDS, updatedAt: new Date() }, + }); + }); + }); + + it("can still be written to at all", async () => { + // The live breakage. onboardingComplete has nothing to do with location, + // and was rejected purely because the stale map failed the allowlist. + const db = plainUser(env, LEGACY).firestore(); + await assertSucceeds( + setDoc(doc(db, "users", LEGACY), { onboardingComplete: true }, { merge: true }) + ); + }); + + it("accepts the city/state write Settings actually sends", async () => { + // Exactly src/services/location.ts saveUserLocation: a merge that carries + // only the safe keys. The inherited lat/lng survive the merge; that is the + // privacy problem the migration fixes, not a reason to deny the write. + const db = plainUser(env, LEGACY).firestore(); + await assertSucceeds( + setDoc( + doc(db, "users", LEGACY), + { location: { city: "Cambridge", state: "MA", updatedAt: serverTimestamp() } }, + { merge: true } + ) + ); + }); + + it("still refuses a write that CHANGES the inherited coordinates", async () => { + const db = plainUser(env, LEGACY).firestore(); + await assertFails( + updateDoc(doc(db, "users", LEGACY), { + location: { ...COORDS, lat: 1.234 }, + }) + ); + }); + + it("lets a whole-map write drop the coordinates", async () => { + // The shape the migration script uses, and the only way a client can + // clean itself up. Removing lat/lng must not be mistaken for changing it. + const db = plainUser(env, LEGACY).firestore(); + await assertSucceeds( + updateDoc(doc(db, "users", LEGACY), { + location: { city: "Boston", state: "MA" }, + }) + ); + }); + + it("is still world-readable, which is what the migration has to fix", async () => { + // Pinning the exposure rather than implying this change closed it. The + // rules half stops the breakage; the coordinates are still there. + const db = env.unauthenticatedContext().firestore(); + const snap = await assertSucceeds(getDoc(doc(db, "users", LEGACY))); + expect(snap.data()?.location.lat).toBe(COORDS.lat); + }); +}); + +describe("a user document with no legacy coordinates", () => { + it("still cannot have coordinates introduced", async () => { + // The guard that must not be lost while unblocking the legacy documents. + const db = plainUser(env, ALICE).firestore(); + await assertFails( + updateDoc(doc(db, "users", ALICE), { + location: { city: "Boston", state: "MA", lat: 42.36, lng: -71.06 }, + }) + ); + }); +});