From 1d6f81aefeae6d65d08b26ddd5e7bcf5669473ae Mon Sep 17 00:00:00 2001 From: WEIREN FENG Date: Sun, 6 Sep 2026 15:13:30 -0700 Subject: [PATCH] Scope the checkins collection group, and serve pet history from a callable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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) --- firestore.rules | 30 +++- functions/src/__tests__/pet-checkins.test.ts | 156 +++++++++++++++++++ functions/src/index.ts | 1 + functions/src/places.ts | 82 ++++++++++ src/pages/PetProfile.tsx | 4 +- src/services/checkins.ts | 90 ++++++----- tests/rules/ban-and-engagement.test.ts | 83 ++++++++++ 7 files changed, 408 insertions(+), 38 deletions(-) create mode 100644 functions/src/__tests__/pet-checkins.test.ts diff --git a/firestore.rules b/firestore.rules index 008cb40..c91efd3 100644 --- a/firestore.rules +++ b/firestore.rules @@ -570,8 +570,36 @@ service cloud.firestore { } // ========= Collection Group queries ========= + // Scoped to the requesting user's own check-ins. + // + // Open, this collection group answered "every check-in matching a filter, + // across all locations", and the (userId, createdAt) and (petId, createdAt) + // collection-group indexes both exist, so it was servable. That turned any + // uid into a movement timeline — every place a person has physically been, + // with timestamps — for an unauthenticated caller, with no rate limit. + // + // Scoping by userId closes BOTH harvesting paths, not just the obvious one. + // A petId-filtered collection-group query can return documents belonging to + // other people, so Firestore refuses it under this rule — which matters, + // because pets/{petId} is world-readable and carries ownerId, putting the + // same timeline one hop away. The pet profile now reads its history through + // getPetCheckinsCallable instead, which stays public but is capped and rate + // limited. + // + // Reading your OWN history stays a direct query: Profile's check-ins tab + // does exactly that (src/services/checkins.ts getUserCheckins, called with + // the signed-in uid), and there is no reason to route a user's own data + // through a callable. + // + // A location's check-ins are still world-readable through + // locations/{locationId}/checkins above. That is the per-place view, not a + // per-person one, and it is unchanged. + // + // The exposure was never that check-ins are visible — the user published + // them and the pet page has always shown them. It is that they could be + // harvested in bulk, by anyone, with no login and no ceiling. match /{path=**}/checkins/{checkinId} { - allow read: if true; + allow read: if isAuthenticated() && resource.data.userId == request.auth.uid; } // Scoped to the requesting user, unlike the per-post rule above. diff --git a/functions/src/__tests__/pet-checkins.test.ts b/functions/src/__tests__/pet-checkins.test.ts new file mode 100644 index 0000000..36263d4 --- /dev/null +++ b/functions/src/__tests__/pet-checkins.test.ts @@ -0,0 +1,156 @@ +import "./setup"; +import { afterAll, beforeEach, describe, expect, it } from "vitest"; +import { admin, db } from "../platform"; +import { getPetCheckinsCallable } from "../places"; +import { callAs, clearRateLimits, errorCodeOf } from "./helpers"; + +// The checkins collection group is now scoped to the requesting user, which +// closed the petId route along with the userId one — a petId-filtered +// collection-group query can return other people's documents, so Firestore +// refuses it. The pet profile reads its history through this callable instead. +// +// It stays PUBLIC: check-ins are content the user chose to publish and the pet +// page has always shown them. What changed is that harvesting now costs a +// capped, rate-limited call per pet rather than one unbounded query, and the +// response carries no user identity at all. + +const OWNER = "checkin-owner"; +const PET = "checkin-pet"; +const OTHER_PET = "other-pet"; +const PLACE = "checkin-place"; + +async function wipe() { + for (const c of ["users", "pets", "locations", "callableRateLimits"]) { + const snap = await db.collection(c).get(); + for (const d of snap.docs) await db.recursiveDelete(d.ref).catch(() => undefined); + } + const users = await admin.auth().listUsers(1000); + await Promise.all( + users.users.map((u) => admin.auth().deleteUser(u.uid).catch(() => undefined)) + ); +} + +/** Seeds `count` check-ins for a pet, oldest first so ordering is testable. */ +async function seedCheckins(petId: string, count: number) { + for (let i = 0; i < count; i += 1) { + await db.doc(`locations/${PLACE}/checkins/${petId}_${i}`).set({ + counted: true, + userId: OWNER, + userName: "Owner Name", + userAvatar: "https://api.dicebear.com/7.x/thumbs/svg?seed=owner", + petId, + petName: "Rex", + photoUrl: `https://res.cloudinary.com/c/image/upload/petnote/${petId}-${i}.jpg`, + caption: `visit ${i}`, + locationId: PLACE, + createdAt: admin.firestore.Timestamp.fromMillis( + Date.UTC(2026, 0, 1) + i * 86_400_000 + ), + }); + } +} + +beforeEach(async () => { + await wipe(); + await clearRateLimits(); + await db.doc(`users/${OWNER}`).set({ displayName: OWNER }); + await db.doc(`pets/${PET}`).set({ name: "Rex", ownerId: OWNER }); + await db.doc(`locations/${PLACE}`).set({ name: "Dog Park" }); +}); +afterAll(wipe); + +type Res = { + checkins: Array>; +}; + +describe("getPetCheckinsCallable", () => { + it("returns a pet's check-ins without being logged in", async () => { + // The whole point of the callable: the pet page is public and stays public. + await seedCheckins(PET, 3); + const res = await callAs(getPetCheckinsCallable, null, { petId: PET }); + expect(res.checkins).toHaveLength(3); + }); + + it("returns them newest first", async () => { + await seedCheckins(PET, 3); + const res = await callAs(getPetCheckinsCallable, null, { petId: PET }); + const times = res.checkins.map((c) => c.createdAtMillis as number); + expect(times).toEqual([...times].sort((a, b) => b - a)); + }); + + it("carries no user identity at all", async () => { + // The pet page never rendered userId / userName / userAvatar, so a public + // endpoint has no reason to hand them out. Omitting them means this route + // cannot be turned back into a per-person lookup even in aggregate. + await seedCheckins(PET, 1); + const res = await callAs(getPetCheckinsCallable, null, { petId: PET }); + const [row] = res.checkins; + expect(row).not.toHaveProperty("userId"); + expect(row).not.toHaveProperty("userName"); + expect(row).not.toHaveProperty("userAvatar"); + expect(JSON.stringify(res)).not.toContain(OWNER); + expect(JSON.stringify(res)).not.toContain("Owner Name"); + // And it still carries what the page does render. + expect(row.photoUrl).toContain("res.cloudinary.com"); + expect(row.locationId).toBe(PLACE); + expect(row.caption).toBe("visit 0"); + }); + + it("caps the row count however many are asked for", async () => { + // Without a ceiling, one rate-limited call becomes an unbounded dump. + await seedCheckins(PET, 12); + const res = await callAs(getPetCheckinsCallable, null, { + petId: PET, + limitCount: 100000, + }); + expect(res.checkins.length).toBeLessThanOrEqual(100); + expect(res.checkins).toHaveLength(12); + }); + + it("honours a smaller limit, and refuses a nonsense one gracefully", async () => { + await seedCheckins(PET, 5); + const few = await callAs(getPetCheckinsCallable, null, { + petId: PET, + limitCount: 2, + }); + expect(few.checkins).toHaveLength(2); + + const zero = await callAs(getPetCheckinsCallable, null, { + petId: PET, + limitCount: 0, + }); + expect(zero.checkins).toHaveLength(1); + + const nan = await callAs(getPetCheckinsCallable, null, { + petId: PET, + limitCount: Number.NaN, + }); + expect(nan.checkins.length).toBeGreaterThan(0); + }); + + it("does not leak another pet's check-ins", async () => { + await seedCheckins(PET, 2); + await seedCheckins(OTHER_PET, 2); + const res = await callAs(getPetCheckinsCallable, null, { petId: PET }); + expect(res.checkins).toHaveLength(2); + for (const row of res.checkins) expect(row.petId).toBe(PET); + }); + + it("rejects a petId that is not a document id", async () => { + // requiredDocId: a value with slashes would otherwise be interpolated + // into a path and reach a different collection. + expect( + await errorCodeOf(() => + callAs(getPetCheckinsCallable, null, { petId: "pets/x/family/y" }) + ) + ).toBe("invalid-argument"); + expect( + await errorCodeOf(() => callAs(getPetCheckinsCallable, null, {})) + ).toBe("invalid-argument"); + }); + + it("returns an empty list for a pet with no check-ins", async () => { + const res = await callAs(getPetCheckinsCallable, null, { petId: PET }); + expect(res.checkins).toEqual([]); + }); +}); diff --git a/functions/src/index.ts b/functions/src/index.ts index 7f6e549..b0e4315 100644 --- a/functions/src/index.ts +++ b/functions/src/index.ts @@ -37,6 +37,7 @@ export { addLocationPhotosCallable, addPlaceCallable, checkInCallable, + getPetCheckinsCallable, onCheckinCreated, onCheckinDeleted, onLocationDeleted, diff --git a/functions/src/places.ts b/functions/src/places.ts index 45398ae..44502cb 100644 --- a/functions/src/places.ts +++ b/functions/src/places.ts @@ -39,6 +39,11 @@ const allowedPlaceCategories = new Set([ "other", ]); +// Pet-profile check-in history. The page asks for 100; the ceiling is what +// stops a caller turning one rate-limited call into an unbounded dump. +const PET_CHECKIN_PAGE_SIZE = 50; +const PET_CHECKIN_MAX_PAGE_SIZE = 100; + const allowedPlaceFeatures = new Set([ "off_leash", "fenced", @@ -844,6 +849,83 @@ export const checkInCallable = onCall( return { id: checkinId }; }); +// Public read of one pet's check-in history, for the pet profile. +// +// This exists because the checkins collection group had to be closed. Left +// open it answered "every check-in matching a filter, across all locations", +// which turned any uid into a movement timeline — every place a person has +// physically been, with timestamps — to an unauthenticated caller with no +// rate limit. The (userId, createdAt) and (petId, createdAt) collection-group +// indexes both exist, so the query was servable. +// +// Closing only the userId path would have been theatre: pets/{petId} is +// world-readable and carries ownerId, so the same timeline was one hop away +// through petId. Both are closed; this callable is the sanctioned way back in. +// +// Still public — no login. Check-ins are content the user chose to publish, +// and the pet profile has always shown them. What changes is that harvesting +// now costs a rate-limited callable per pet instead of one unbounded query, +// and the response carries no user identity at all: the pet page never +// rendered userId / userName / userAvatar, so they are not returned. +export const getPetCheckinsCallable = onCall(async (request) => { + const data = requestData(request.data) as { + petId?: string; + limitCount?: number; + }; + const petId = requiredDocId(data.petId, "petId"); + + // Hard cap regardless of what the client asks for. + const requested = + typeof data.limitCount === "number" && Number.isFinite(data.limitCount) + ? Math.floor(data.limitCount) + : PET_CHECKIN_PAGE_SIZE; + const limitCount = Math.min( + PET_CHECKIN_MAX_PAGE_SIZE, + Math.max(1, requested) + ); + + // Signed-in callers are bucketed by uid; anonymous ones by client IP, so an + // unauthenticated scraper cannot get an unlimited budget just by not + // logging in. assertRateLimit only needs a stable string for the bucket. + const callerUid = request.auth?.uid; + const forwardedFor = request.rawRequest?.headers?.["x-forwarded-for"]; + const rawIp = + (Array.isArray(forwardedFor) ? forwardedFor[0] : forwardedFor) + ?.split(",")[0] + ?.trim() || + request.rawRequest?.ip || + "unknown"; + const bucket = callerUid ?? `anon_${rawIp.replace(/[^A-Za-z0-9_.:-]/g, "_")}`; + await assertRateLimit(bucket, "getPetCheckins", RATE_LIMITS.read); + + const snap = await db + .collectionGroup("checkins") + .where("petId", "==", petId) + .orderBy("createdAt", "desc") + .limit(limitCount) + .get(); + + return { + checkins: snap.docs.map((docSnap) => { + const checkin = docSnap.data() ?? {}; + const createdAt = checkin.createdAt; + return { + id: docSnap.id, + locationId: docSnap.ref.parent.parent?.id ?? "", + petId: typeof checkin.petId === "string" ? checkin.petId : "", + petName: typeof checkin.petName === "string" ? checkin.petName : "", + photoUrl: typeof checkin.photoUrl === "string" ? checkin.photoUrl : "", + caption: typeof checkin.caption === "string" ? checkin.caption : "", + // Timestamps do not survive the callable boundary as Timestamps. + createdAtMillis: + createdAt && typeof createdAt.toMillis === "function" + ? createdAt.toMillis() + : null, + }; + }), + }; +}); + // Admin-only backfill: rebuilds petFriendlySum / petFriendlyAvg / // tagCounts / topTags from the location's review subcollection. Used to // initialise these aggregates on locations whose reviews predate the diff --git a/src/pages/PetProfile.tsx b/src/pages/PetProfile.tsx index e6cc680..8125dac 100644 --- a/src/pages/PetProfile.tsx +++ b/src/pages/PetProfile.tsx @@ -8,7 +8,7 @@ import { useAuth } from "../hooks/useAuth"; import { useToast } from "../contexts/ToastContext"; import { useFollowPet } from "../hooks/useFollow"; import { getPetFollowers, type PetFollower } from "../services/follow"; -import { getCheckinsByPet, type Checkin } from "../services/checkins"; +import { getCheckinsByPet, type PetCheckin } from "../services/checkins"; import { batchGetLocations, type Location } from "../services/locations"; import { deletePet, @@ -42,7 +42,7 @@ export function PetProfile() { const [familyMembers, setFamilyMembers] = useState([]); const [viewerIsFamilyMember, setViewerIsFamilyMember] = useState(false); - const [checkins, setCheckins] = useState([]); + const [checkins, setCheckins] = useState([]); const [checkinLocations, setCheckinLocations] = useState>( {} ); diff --git a/src/services/checkins.ts b/src/services/checkins.ts index e7b2b11..6847cf5 100644 --- a/src/services/checkins.ts +++ b/src/services/checkins.ts @@ -124,43 +124,63 @@ export async function getUserCheckins( }; } +/** What getPetCheckinsCallable returns. Deliberately no user identity. */ +export type PetCheckin = { + id: string; + locationId: string; + petId: string; + petName: string; + photoUrl: string; + caption: string; + createdAt: Date | null; +}; + +/** + * A pet's check-in history, through a callable rather than a direct query. + * + * This used to be a collection-group query filtered by petId. That query is no + * longer permitted: the checkins collection group is scoped to the requesting + * user's own check-ins, because left open it let anyone harvest a person's + * whole movement timeline — by userId directly, or by petId via the + * world-readable pets/{petId}.ownerId, which is why closing only the userId + * path would have been pointless. + * + * Still public, no login required. The callable caps the row count and applies + * the usual rate limit, and returns no userId / userName / userAvatar, none of + * which this page ever rendered. + */ export async function getCheckinsByPet( petId: string, - options?: { limitCount?: number; lastDoc?: QueryDocumentSnapshot } -): Promise<{ - checkins: Checkin[]; - lastDoc: QueryDocumentSnapshot | null; - hasMore: boolean; -}> { - if (!petId) return { checkins: [], lastDoc: null, hasMore: false }; - const limitCount = options?.limitCount ?? 50; - // Order on the server with a (petId ASC, createdAt DESC) collection-group - // index so we get the *latest* N rather than an unordered slice that we - // then sort locally. Without orderBy, `.limit(50)` could return any 50 - // matching docs and miss recent check-ins for prolific pets. - const constraints: QueryConstraint[] = [ - where("petId", "==", petId), - orderBy("createdAt", "desc"), - limit(limitCount), - ]; - if (options?.lastDoc) { - constraints.push(startAfter(options.lastDoc)); - } - const snapshot = await getDocs( - query(collectionGroup(db, "checkins"), ...constraints) - ); - const checkins = snapshot.docs.map((docSnap) => ({ - id: docSnap.id, - locationId: docSnap.ref.parent.parent?.id || "", - ...(docSnap.data() as Omit), - })); - const nextLast = - (snapshot.docs[snapshot.docs.length - 1] as - | QueryDocumentSnapshot - | undefined) ?? null; + options?: { limitCount?: number } +): Promise<{ checkins: PetCheckin[] }> { + if (!petId) return { checkins: [] }; + + const getPetCheckins = httpsCallable< + { petId: string; limitCount?: number }, + { + checkins: Array<{ + id: string; + locationId: string; + petId: string; + petName: string; + photoUrl: string; + caption: string; + createdAtMillis: number | null; + }>; + } + >(functions, "getPetCheckinsCallable"); + + const { data } = await getPetCheckins({ + petId, + ...(options?.limitCount ? { limitCount: options.limitCount } : {}), + }); + return { - checkins, - lastDoc: nextLast, - hasMore: snapshot.docs.length === limitCount, + checkins: (data.checkins ?? []).map((item) => ({ + ...item, + // Timestamps do not survive the callable boundary, so the server sends + // millis and the Date is rebuilt here. + createdAt: item.createdAtMillis ? new Date(item.createdAtMillis) : null, + })), }; } diff --git a/tests/rules/ban-and-engagement.test.ts b/tests/rules/ban-and-engagement.test.ts index c2b6e92..f17dba3 100644 --- a/tests/rules/ban-and-engagement.test.ts +++ b/tests/rules/ban-and-engagement.test.ts @@ -468,3 +468,86 @@ describe("the likes collection group is scoped to the requesting user", () => { ); }); }); + +describe("the checkins collection group is scoped to the requesting user", () => { + // Open, this collection group answered "every check-in matching a filter, + // across all locations" — a movement timeline for any uid, to anyone, with + // no rate limit. The (userId, createdAt) and (petId, createdAt) indexes both + // exist, so it was servable, not theoretical. + const PLACE = "location-1"; + const OTHER = "other-user"; + + beforeEach(async () => { + await env.withSecurityRulesDisabled(async (ctx) => { + const db = ctx.firestore(); + await setDoc(doc(db, "locations", PLACE), { name: "Dog Park" }); + await setDoc(doc(db, `locations/${PLACE}/checkins/${ALICE}_2026-09-06`), { + userId: ALICE, + petId: "alice-pet", + photoUrl: "https://res.cloudinary.com/c/image/upload/petnote/x.jpg", + createdAt: new Date(), + }); + await setDoc(doc(db, `locations/${PLACE}/checkins/${OTHER}_2026-09-06`), { + userId: OTHER, + petId: "other-pet", + photoUrl: "https://res.cloudinary.com/c/image/upload/petnote/y.jpg", + createdAt: new Date(), + }); + }); + }); + + it("still lets anyone read one location's check-ins", async () => { + // The per-place view, unchanged. This is what the location page shows. + const db = env.unauthenticatedContext().firestore(); + await assertSucceeds(getDocs(collection(db, `locations/${PLACE}/checkins`))); + }); + + it("lets a signed-in user query their own history", async () => { + // Profile's check-ins tab does exactly this, with the signed-in uid. + // Routing a user's own data through a callable would be pointless. + const db = plainUser(env, ALICE).firestore(); + await assertSucceeds( + getDocs( + query(collectionGroup(db, "checkins"), where("userId", "==", ALICE)) + ) + ); + }); + + it("refuses a query for someone else's history", async () => { + const db = plainUser(env, ALICE).firestore(); + await assertFails( + getDocs( + query(collectionGroup(db, "checkins"), where("userId", "==", OTHER)) + ) + ); + }); + + it("refuses an unauthenticated query for anyone's history", async () => { + const db = env.unauthenticatedContext().firestore(); + await assertFails( + getDocs( + query(collectionGroup(db, "checkins"), where("userId", "==", ALICE)) + ) + ); + }); + + it("refuses the petId route, which is the same leak one hop away", async () => { + // pets/{petId} is world-readable and carries ownerId, so filtering by + // petId reconstructs a person's timeline just as well. Scoping by userId + // closes this too: a petId-filtered collection-group query can return + // documents belonging to other people, so Firestore refuses it outright. + // Even the pet's own owner is refused — the sanctioned route is + // getPetCheckinsCallable, which is capped and rate limited. + const db = plainUser(env, ALICE).firestore(); + await assertFails( + getDocs( + query(collectionGroup(db, "checkins"), where("petId", "==", "alice-pet")) + ) + ); + }); + + it("refuses an unfiltered collection-group scan", async () => { + const db = plainUser(env, ALICE).firestore(); + await assertFails(getDocs(collectionGroup(db, "checkins"))); + }); +});