Scope the checkins collection group, and serve pet history from a callable - #190
Open
renrenmimi wants to merge 1 commit into
Open
Scope the checkins collection group, and serve pet history from a callable#190renrenmimi wants to merge 1 commit into
renrenmimi wants to merge 1 commit into
Conversation
…lable
`collectionGroup('checkins').where('userId','==',victim).orderBy('createdAt')`
returned a person's movement timeline — every place they have physically been,
with timestamps — to an unauthenticated caller with no rate limit. Both the
(userId, createdAt) and (petId, createdAt) collection-group indexes exist, so
this was servable, not theoretical.
Closing only the userId path would have been theatre, and the brief said so:
pets/{petId} is world-readable and carries ownerId, so the same timeline was
one hop away through petId. Scoping the rule to the requesting user closes
both, because a petId-filtered collection-group query can return documents
belonging to other people and Firestore refuses it outright.
THE BRIEF WAS WRONG ABOUT ONE THING, and following it would have broken a live
feature. It said to delete getUserCheckins as dead code with no callers. The
function is actually getUserCheckins, and src/pages/Profile.tsx:130 calls it
with the signed-in user's own uid to render their own check-ins tab. It is
kept, and the rule is scoped rather than closed outright so that query keeps
working — routing a user's own data through a callable would buy nothing.
The pet profile moves to getPetCheckinsCallable, which stays public with no
login, because check-ins are content the user chose to publish and that page
has always shown them. The exposure was never visibility, it was bulk
harvesting with no ceiling. So the callable caps rows at 100 and runs through
assertRateLimit, bucketed by uid when signed in and by client IP when not, so
an anonymous scraper cannot get an unlimited budget by staying logged out.
The response deliberately carries no userId / userName / userAvatar. The pet
page never rendered them, and leaving them out means this route cannot be
reassembled into a per-person lookup either.
The per-location view (locations/{id}/checkins) is untouched and still
world-readable. That is a per-place question, not a per-person one.
Six new rules tests, four of which fail on the old rule; the two that pass on
both are the guardrails — an unauthenticated read of one location's check-ins,
and a user querying their own history. Eight new callable tests covering the
public path, ordering, the row cap, the absent identity fields, cross-pet
isolation and docId validation.
Rules 74/74, functions 88/88.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e538be5d71
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+891
to
+895
| const forwardedFor = request.rawRequest?.headers?.["x-forwarded-for"]; | ||
| const rawIp = | ||
| (Array.isArray(forwardedFor) ? forwardedFor[0] : forwardedFor) | ||
| ?.split(",")[0] | ||
| ?.trim() || |
There was a problem hiding this comment.
An unauthenticated caller invoking the callable directly can supply an arbitrary leading X-Forwarded-For value; Google’s proxy may append forwarding information, but this code always selects the first entry. Rotating that value creates a fresh Firestore rate-limit document for every request, bypassing the intended 120-request ceiling and allowing the pet-history scraping this change is meant to prevent. Use a platform-derived address or parse only the trusted proxy hop instead.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
collectionGroup('checkins').where('userId','==',victim).orderBy('createdAt')returned a person's movement timeline — every place they have physically been, with timestamps — to an unauthenticated caller with no rate limit. Both the(userId, createdAt)and(petId, createdAt)collection-group indexes exist, so this was servable today.The function is
getUserCheckins, and it is not dead: Profile.tsx:130 callsgetUserCheckins(user.uid)to render the signed-in user's own check-ins tab.Deleting it, or closing the rule outright with
allow read: if falseas the brief implied, would have removed a user's ability to see their own check-in history.So the rule is scoped to the requesting user rather than closed:
This still closes both harvesting paths. The brief's core point was right — tightening only
userIdis theatre, sincepets/{petId}is world-readable and carriesownerId— but scoping achieves it anyway: apetId-filtered collection-group query can return documents belonging to other people, so Firestore refuses it outright. There's a test pinning exactly that, including that even the pet's own owner is refused.The pet profile
Moves to
getPetCheckinsCallable. Still public, no login — check-ins are content the user chose to publish and that page has always shown them. The exposure was never visibility, it was bulk harvesting with no ceiling. So:assertRateLimit, bucketed by uid when signed in and by client IP when not, so an anonymous scraper can't get an unlimited budget by staying logged outuserId/userName/userAvatarin the response. The page never rendered them, and leaving them out means this route can't be reassembled into a per-person lookup eitherlocations/{id}/checkinsis untouched and still world-readable — that's a per-place question, not a per-person one.Tests
Rules, 6 new — 4 fail on the old rule:
Two pass on both, as guardrails: an unauthenticated read of one location's check-ins, and a user querying their own history — the Profile tab the brief would have broken.
Callable, 8 new: the public path, newest-first ordering, the row cap under an absurd
limitCount, the absent identity fields (asserted both by key and by scanning the serialised response for the uid), cross-pet isolation,requiredDocIdvalidation, and the empty case.Local: rules 74/74, functions
test:emulator88/88,typecheck:test/typecheck:tests/ both builds clean.Deploy
Rules first, then functions — the client change depends on the callable existing.
Noted separately, not bundled: the deletion cascade does not clean up
locationsa user added.🤖 Generated with Claude Code