Skip to content

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

Merged
renrenmimi merged 1 commit into
mainfrom
fix/legacy-location-blocks-writes
Sep 4, 2026
Merged

Stop a legacy location map from blocking every write to a user document#186
renrenmimi merged 1 commit into
mainfrom
fix/legacy-location-blocks-writes

Conversation

@renrenmimi

Copy link
Copy Markdown
Owner

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.

Why that broke real accounts

Documents written before the private-location split still carry precise coordinates in the world-readable user doc. saveUserLocation writes with setDoc(..., {merge:true}), and merge deep-merges nested maps — so the old lat/lng survive the write, fail the allowlist, and the update is denied. getUserLocation swallows that with .catch(() => undefined), and AuthContext retries on every sign-in. Silent, and forever.

The damage was not limited to location. Because the check applied to every update, it blocked every allowed write to those documents:

× can still be written to at all              ← {onboardingComplete: true}, denied
× accepts the city/state write Settings sends ← the actual saveUserLocation shape

Both fail on the current rules and pass on this branch. For affected users, "Update location" in Settings failed with a generic toast and onboarding could not complete. This is live functional breakage, not only a privacy leak.

What the rule says now

A client may never introduce or change lat/lng. It may drop them, and it may leave inherited ones untouched — that last part 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.

What this does NOT fix

The residual coordinates are still there and still readable by an unauthenticated stranger. There's a test pinning that exposure rather than letting the PR imply it's closed:

it("is still world-readable, which is what the migration has to fix", ...)

Removing them needs production writes. Not in this PR — waiting on your go-ahead. scripts/migrate-user-locations-private.ts already uses batch.update with a whole map, which replaces rather than merges, so it does drop them; it has no record of ever having been run.

Also in here: a comment you asked for

settingsTypesOk 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. Same trap, one helper away. It now says so in the code and points at this fix, so the next person doesn't "finish" it.

Tests

tests/rules/users.test.ts, 20 → 26.

Two fail on the old rules, pass on these (above). Four pass on both, as guardrails:

  • a write that changes inherited coordinates is still refused
  • a whole-map write that drops them still succeeds — removal must not be mistaken for modification
  • a document with no legacy coordinates still cannot have them introduced
  • the legacy document is still world-readable (pins the residual exposure)

Local run: rules 63/63, functions test:emulator 73/73, lint / typecheck:tests clean.

Deploy

firebase deploy --only firestore:rules

🤖 Generated with Claude Code

Copilot AI lite review requested due to automatic review settings September 4, 2026 05:29

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@vercel

vercel Bot commented Sep 4, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
pet-note Ready Ready Preview Sep 4, 2026 7:17am UTC

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) <noreply@anthropic.com>
@renrenmimi
renrenmimi force-pushed the fix/legacy-location-blocks-writes branch from 54e4f54 to 08feeee Compare September 4, 2026 07:16
@renrenmimi
renrenmimi merged commit c96edb9 into main Sep 4, 2026
6 checks passed
@renrenmimi
renrenmimi deleted the fix/legacy-location-blocks-writes branch September 4, 2026 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants