Stop a legacy location map from blocking every write to a user document

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) &&

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