From 468f672b26b2aabf2b029b6c606241400caae23f Mon Sep 17 00:00:00 2001 From: Duygu <84078855+duuyguyiilmaz@users.noreply.github.com> Date: Tue, 1 Sep 2026 19:43:58 +0300 Subject: [PATCH 1/3] fix: require a vote's totals to be written with it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The link between a vote and a note's score was only enforced in one direction. The note's rule said "if the totals move, show me the vote that justifies them". Nothing said the reverse — "if a vote is written, the totals must follow" — so a rating document could be written entirely on its own. The test suite asserted that as correct behaviour, which is why CI stayed green over it: the suite described the hole rather than catching it. It is not a tidiness problem. It is unbounded score inflation from a single voter, four points a lap: 1. write the vote alone as 1 (totals untouched) 2. raise it to 5 with the totals (sum += 4, count unchanged — the rule correctly sees a changed vote) 3. write it alone as 1 again (totals untouched) 4. go to 2 Step 1 is what was permitted, and refusing it breaks the loop at its first move. The ratings rule now mirrors the note's arithmetic from its own side: read the note before and after the commit with `get()`/`getAfter()`, recompute the expected `ratingSum` and `ratingCount` from the vote being written and the one it replaces, and require the note to actually carry them. Both sides now compute the same numbers as `RatingCalculator`, floors included, and neither write is accepted without the other. Four tests replace the one that asserted the gap: a vote cannot be written without its totals, an existing vote cannot be changed without them, the inflation loop is refused at step one, and totals that do not match the vote are rejected from this side too. The existing ratings tests were rewritten to go through the batch helper — several of them wrote a lone rating document and passed for the wrong reason, including the range check, which now carries the totals the rule would compute if the rating were valid so that only the range can reject it. Verified by mutation, not just by a green run: deleting the single new clause from firestore.rules turns exactly those three tests red and leaves the other 86 green. Rules suite 65 -> 69, 89 green in firestore-tests, 126 on the JVM side, lint clean. Deployed to the live project. Co-Authored-By: Claude Opus 5 --- README.md | 25 +++++--- firestore-tests/rules.test.js | 111 +++++++++++++++++++++++++++------- firestore.rules | 43 ++++++++++++- 3 files changed, 150 insertions(+), 29 deletions(-) diff --git a/README.md b/README.md index efa6ff3..c8d4530 100644 --- a/README.md +++ b/README.md @@ -272,11 +272,22 @@ and the two score fields; everything else was open to a client that skipped the UI, which an attacker was never obliged to use. They also verify the scoring rather than trusting it. A vote writes the note's totals and -the rating document in one atomic commit, and the rule for the note uses `getAfter()` to -read the rating being written in that same commit, recompute the expected `ratingSum` and -`ratingCount` from it, and reject anything that does not match. So the totals cannot move -without a real vote behind them, a changed vote cannot be counted twice, and rating your -own note is refused by the server rather than only by the client. +the rating document in one atomic commit, and each side of that commit checks the other +with `getAfter()`: the note's rule recomputes the expected `ratingSum` and `ratingCount` +from the rating being written beside it, and the rating's rule recomputes the same two +numbers and requires the note to actually carry them. So the totals cannot move without a +real vote behind them, a vote cannot be written without the totals that follow from it, a +changed vote cannot be counted twice, and rating your own note is refused by the server +rather than only by the client. + +The second direction was missing until recently, and its absence was the worst hole in +this file. Only "totals moved, so show me the vote" was enforced; "a vote was written, so +the totals must follow" was not, which left a rating document writable entirely on its +own — and the test suite asserted that as correct behaviour, so CI stayed green over it. +It was not untidiness but unbounded inflation, four points a lap from one voter: write the +vote alone as 1, raise it to 5 with the totals (`sum += 4`, count unchanged because the +rule sees a changed vote), write it alone as 1 again, repeat. The lone write is now +refused, which breaks the loop at step one. The expected totals are the same arithmetic as [`RatingCalculator`](app/src/main/java/duygu/yilmaz/campusnote/data/model/RatingCalculation.kt), @@ -488,7 +499,7 @@ enforces — who may delete a note, whether a score can move without a vote behi invisible to them. That is the layer an attacker actually meets: the Android client can be replaced, the rules cannot. -65 tests in [`firestore-tests/rules.test.js`](firestore-tests/rules.test.js) run +69 tests in [`firestore-tests/rules.test.js`](firestore-tests/rules.test.js) run [`firestore.rules`](firestore.rules) against the local Firestore emulator through `@firebase/rules-unit-testing`. `npm test` starts the emulator, runs the suite and shuts it down again; nothing touches the real project, and no billing account is involved. @@ -501,7 +512,7 @@ it down again; nothing touches the real project, and no billing account is invol | `notes` | `uploaderUid` cannot be forged and a note cannot be born with a score; the uploader may edit metadata but not the totals, the department, or the identity fields; a vote and the totals it implies are accepted only together, must match the arithmetic exactly, cannot be counted twice, and cannot be cast on your own note; delete is owner-only. | | `note creation is pinned to an exact shape` | One test per way a hand-rolled request used to get through: an unexpected field, a missing one, a resurrected `avgRating`, another department, a forged `uploaderName`, an email address written onto a note, a client-chosen `createdAt`, a wrong type, and an empty or oversized title. Each asserts the refusal, so relaxing any single check turns exactly one test red. | | `note content` | The batched note-plus-file upload, owner-only create, replace and delete, that a file on another department's note cannot be read — otherwise the boundary is walked around through the file path — that a content document carrying anything but `fileData` is refused, and that a batch cannot pair a legitimate note of its own with a file smuggled onto someone else's. | -| `ratings` | The `_` document id, which is what stops one user voting as another, plus the 1–5 integer range. Deletion has four: a vote cannot be removed on its own, not even by the note's owner; the owner may remove it in the same commit that deletes the note; a stranger cannot reach it by deleting a note they do not own; and a vote whose note is already gone stays refused, which is the gap the prune script covers. | +| `ratings` | The `_` document id, which is what stops one user voting as another, plus the 1–5 integer range. Four cover the link back to the note: a vote cannot be written or changed without the totals that follow from it, the totals must match the vote from this side too, and the inflation loop above is refused at its first move. Removing that one clause turns exactly those three red and leaves the other 86 green. Deletion has four: a vote cannot be removed on its own, not even by the note's owner; the owner may remove it in the same commit that deletes the note; a stranger cannot reach it by deleting a note they do not own; and a vote whose note is already gone stays refused, which is the gap the prune script covers. | | `unmatched paths` | A path no `match` block covers is denied, so adding a collection to the app without adding a rule fails closed rather than open. | Fixtures are seeded with `withSecurityRulesDisabled`, so a broken rule surfaces as a diff --git a/firestore-tests/rules.test.js b/firestore-tests/rules.test.js index 1b42c20..990ac1c 100644 --- a/firestore-tests/rules.test.js +++ b/firestore-tests/rules.test.js @@ -125,17 +125,24 @@ function noteFields(overrides = {}) { * @param uid oy dokümanının sahibi olarak yazılacak uid — kurala göre çağıranınki olmalı * @param totals notun yazılacak yeni değerleri; `extra` ile fazladan alan sızdırılır */ -function vote(db, uid, noteId, rating, { sum, count, extra = {} }) { +function vote( + db, + uid, + noteId, + rating, + { sum, count, extra = {}, id = `${uid}_${noteId}`, voteOverrides = {} }, +) { const batch = writeBatch(db); batch.update(doc(db, "notes", noteId), { ratingSum: sum, ratingCount: count, ...extra, }); - batch.set(doc(db, "ratings", `${uid}_${noteId}`), { + batch.set(doc(db, "ratings", id), { uid, noteId, rating, + ...voteOverrides, }); return batch.commit(); } @@ -612,52 +619,114 @@ describe("ratings", () => { const validVote = { uid: RATER, noteId: NOTE, rating: 4 }; test("a vote is written under _", async () => { - await assertSucceeds( - setDoc(doc(rater, "ratings", `${RATER}_${NOTE}`), validVote), - ); + await assertSucceeds(vote(rater, RATER, NOTE, 4, { sum: 4, count: 1 })); }); test("re-voting replaces the existing document", async () => { - await assertSucceeds( + await assertSucceeds(vote(rater, RATER, NOTE, 4, { sum: 4, count: 1 })); + // Changing 4 to 2 moves the sum by the difference and leaves the count. + await assertSucceeds(vote(rater, RATER, NOTE, 2, { sum: 2, count: 1 })); + }); + + /** + * The other half of the link, and the one that was missing. + * + * The note's own rule says "if the totals move, a vote must justify them". It + * never said the reverse — "if a vote is written, the totals must move with it" — + * so a rating document could be written entirely on its own, and the test here + * asserted that as correct behaviour. + * + * That was not a tidiness problem. It was unbounded score inflation from a single + * voter, four points a lap: + * + * 1. write the vote alone as 1 (totals untouched) + * 2. raise it to 5 with the totals (sum += 4, count unchanged) + * 3. write it alone as 1 again (totals untouched) + * 4. go to 2 + * + * Step 1 is what is now refused, which breaks the loop at its first move. + */ + test("a vote cannot be written without the totals that follow from it", async () => { + await assertFails( setDoc(doc(rater, "ratings", `${RATER}_${NOTE}`), validVote), ); - await assertSucceeds( + }); + + test("an existing vote cannot be changed without the totals following", async () => { + await assertSucceeds(vote(rater, RATER, NOTE, 5, { sum: 5, count: 1 })); + + // The lone downgrade that made the loop above possible. + await assertFails( setDoc(doc(rater, "ratings", `${RATER}_${NOTE}`), { ...validVote, - rating: 2, + rating: 1, }), ); }); + test("the inflation loop does not survive its first lap", async () => { + await assertSucceeds(vote(rater, RATER, NOTE, 5, { sum: 5, count: 1 })); + + // Every remaining step of the loop, each refused on its own: dropping the vote + // silently, and then claiming the difference as if it had been dropped. + await assertFails( + setDoc(doc(rater, "ratings", `${RATER}_${NOTE}`), { + ...validVote, + rating: 1, + }), + ); + await assertFails(vote(rater, RATER, NOTE, 5, { sum: 9, count: 1 })); + }); + + test("the totals must match the vote from this side too", async () => { + // Written as a batch, so the note rule is satisfied by the vote beside it and + // the arithmetic is the only thing left to reject. + await assertFails(vote(rater, RATER, NOTE, 1, { sum: 5, count: 1 })); + await assertFails(vote(rater, RATER, NOTE, 4, { sum: 4, count: 2 })); + }); + test("a user cannot vote in someone else's name", async () => { // The uid field and the document id are checked separately, so both // halves of the forgery have to be rejected on their own. await assertFails( - setDoc(doc(rater, "ratings", `${OWNER}_${NOTE}`), { - ...validVote, - uid: OWNER, + vote(rater, RATER, NOTE, 4, { + sum: 4, + count: 1, + id: `${OWNER}_${NOTE}`, + voteOverrides: { uid: OWNER }, }), ); await assertFails( - setDoc(doc(rater, "ratings", `${OWNER}_${NOTE}`), validVote), + vote(rater, RATER, NOTE, 4, { sum: 4, count: 1, id: `${OWNER}_${NOTE}` }), ); }); test("the document id must match the uid and note it claims", async () => { await assertFails( - setDoc(doc(rater, "ratings", `${RATER}_other-note`), validVote), + vote(rater, RATER, NOTE, 4, { + sum: 4, + count: 1, + id: `${RATER}_other-note`, + }), + ); + await assertFails( + vote(rater, RATER, NOTE, 4, { sum: 4, count: 1, id: "arbitrary-id" }), ); - await assertFails(setDoc(doc(rater, "ratings", "arbitrary-id"), validVote)); }); test("the rating must be an integer between 1 and 5", async () => { - for (const rating of [0, 6, -1, 3.5, "4"]) { - await assertFails( - setDoc(doc(rater, "ratings", `${RATER}_${NOTE}`), { - ...validVote, - rating, - }), - ); + // Each case carries the totals the rule would compute *if* the rating were + // valid, so the range check is the only thing left that can reject it. The + // string case is the exception: it fails on the arithmetic as well, since a + // string cannot be added to a sum. + for (const [rating, sum] of [ + [0, 0], + [6, 6], + [-1, 0], + [3.5, 3.5], + ["4", 4], + ]) { + await assertFails(vote(rater, RATER, NOTE, rating, { sum, count: 1 })); } }); diff --git a/firestore.rules b/firestore.rules index 570656d..45b3c45 100644 --- a/firestore.rules +++ b/firestore.rules @@ -301,7 +301,48 @@ service cloud.firestore { // notun toplamını değiştirmese de liderlik tablosunun // oy sayısını şişirirdi. && get(/databases/$(database)/documents/notes/$(request.resource.data.noteId)) - .data.uploaderUid != request.auth.uid; + .data.uploaderUid != request.auth.uid + && totalsFollowTheVote(request.resource.data.noteId); + + function notePath(noteId) { + return /databases/$(database)/documents/notes/$(noteId); + } + + /** + * Oyun karşılığı toplam aynı commit'te yazılmış olmalı. + * + * Bağ eskiden tek yönlüydü: notun kuralı "toplam değişiyorsa yanında oy + * bulunsun" diyordu, ama oyun kuralı "oy yazılıyorsa toplam da doğru + * değişsin" demiyordu. Tek başına oy belgesi yazmak serbestti ve bu, + * sınırsız puan şişirmenin yoluydu: + * + * 1. oy belgesini tek başına 1 yap (toplam değişmez) + * 2. oyu 5'e çıkar + toplamı güncelle (fark 4 eklenir, sayı artmaz) + * 3. oy belgesini tek başına 1 yap (toplam değişmez) + * 4. 2. adıma dön + * + * Her turda not 4 puan kazanıyor ve tek bir oy verenden geliyordu. + * + * `getAfter` notun commit sonrası hâlini okuyor; beklenen değerler notun + * kendi kuralındaki aritmetiğin aynısı — iki taraf da RatingCalculator ile + * birebir aynı olmak zorunda, tabanları dahil. + */ + function totalsFollowTheVote(noteId) { + let before = get(notePath(noteId)).data; + let after = getAfter(notePath(noteId)).data; + let votedBefore = exists(/databases/$(database)/documents/ratings/$(ratingId)); + let previous = votedBefore + ? get(/databases/$(database)/documents/ratings/$(ratingId)).data.rating + : 0; + let rawSum = before.ratingSum + request.resource.data.rating - previous; + + return after.ratingSum == (rawSum < 0 ? 0 : rawSum) + && after.ratingCount == ( + votedBefore + ? (before.ratingCount < 1 ? 1 : before.ratingCount) + : before.ratingCount + 1 + ); + } /** * Bir oy yalnızca ait olduğu notla birlikte silinebilir. From ba94350b5942434f8e239b848230073264f022e8 Mon Sep 17 00:00:00 2001 From: Duygu <84078855+duuyguyiilmaz@users.noreply.github.com> Date: Tue, 1 Sep 2026 20:02:40 +0300 Subject: [PATCH 2/3] fix: enforce the university address on the server, not on the form MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The profile create rule checked that you were writing your own document and that its `id` field agreed with the document id. Nothing else. So `email`, `department`, `createdAt` and any field at all were whatever the request said, and the `@ogr.akdeniz.edu.tr` requirement lived only in `RegisterActivity` — client-side, on a screen nobody is obliged to use. Firebase Auth accepts any address, so signing up with a personal one and writing the profile directly was enough to become a student. That got sharper when reads were scoped by department, because `ownDepartment()` reads the department from this very document. An unverified profile was not a data-hygiene problem by then; it was read access to a department's notes. The rule now pins the field set, takes the address from `request.auth.token.email` instead of the request body, checks the domain itself, requires a non-empty department, and requires `createdAt` to be `request.time`. The department is not checked against the list of 144 — embedding those in a rule would break it on every curriculum change — so the bound is "a non-empty string of sane length", which is what stops it being an empty or absurd read scope. `createdAt` moving to the server clock meant `saveUser` had to write fields explicitly rather than `set(user)`, since a server timestamp does not fit a `Long` on the data class. **A bug this introduced, caught by running the app rather than by the suite.** `getTimestamp()` does not return null when the field holds a number — it throws — so reading any profile written before this change crashed the profile screen with "Field 'createdAt' is not a com.google.firebase.Timestamp". `toPost()` already had the try/catch for exactly this and I did not reuse it. Now it does, with a comment on each side pointing at the other. This is the gap the README documents under Known limitations: no test executes repository code, so the only thing that covers it is opening the app. Tests: rules 69 -> 76. Five cover the profile rule — a personal address cannot register, the profile's email must be the session's own, a blank department, a client-chosen `createdAt`, and any unexpected or missing field are all refused — and removing the two email clauses turns exactly the first two red. Two more close a gap left by the previous commit: the app votes through `runTransaction`, not `writeBatch`, and nothing exercised that. Both directions of the two-way rule are now tested against the transaction the repository actually issues. 96 green in firestore-tests, 126 on the JVM side, lint clean. Deployed, and registration verified end to end on the device against the deployed rules. Co-Authored-By: Claude Opus 5 --- README.md | 24 +++- .../data/repository/FirebaseUserRepository.kt | 34 ++++- .../campusnote/ui/auth/RegisterViewModel.kt | 4 +- firestore-tests/rules.test.js | 125 ++++++++++++++++-- firestore.rules | 50 ++++++- 5 files changed, 215 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index c8d4530..b0be11d 100644 --- a/README.md +++ b/README.md @@ -183,9 +183,9 @@ Cloud Firestore, four collections: | Field | Type | Notes | |---|---|---| | `id` | string | matches the Firebase Auth UID | -| `email` | string | | -| `department` | string | | -| `createdAt` | number | | +| `email` | string | must equal the session's own address, and end in `@ogr.akdeniz.edu.tr` | +| `department` | string | non-empty; `ownDepartment()` reads it to decide what the account may read | +| `createdAt` | timestamp | `request.time`; profiles written before this carry epoch millis and are still read | Older user documents may still carry `hasUploadedNote` and `points`. Neither is read or written any more: the contribution gate is derived from the user's notes, and so is the @@ -218,6 +218,18 @@ Firebase Console, so they can be reviewed and versioned. Deploy with: firebase deploy --only firestore:rules ``` +**Registration is restricted on the server, not only on the form.** The create rule used +to check that you were writing your own document and that its `id` field agreed with the +document id, and nothing else — so the `@ogr.akdeniz.edu.tr` requirement lived only in +`RegisterActivity`. Firebase Auth accepts any address, so anyone could sign up with a +personal one, skip the Android screen entirely and write themselves a profile carrying any +`email`, any `department`, any `createdAt` and any extra fields they liked. Since reads are +now scoped by `department` and `ownDepartment()` reads it from this very document, that +forged profile opened a department's notes. The rule now pins the field set, takes the +address from `request.auth.token.email` rather than from the request body, checks the +domain itself, requires a non-empty department, and requires `createdAt` to be +`request.time`. + The rules require authentication everywhere, restrict note edits and deletes to the uploader, and pin each rating document to its author's UID. The delete rule is the *only* ownership check on deletion — the app code does not verify it. @@ -499,7 +511,7 @@ enforces — who may delete a note, whether a score can move without a vote behi invisible to them. That is the layer an attacker actually meets: the Android client can be replaced, the rules cannot. -69 tests in [`firestore-tests/rules.test.js`](firestore-tests/rules.test.js) run +76 tests in [`firestore-tests/rules.test.js`](firestore-tests/rules.test.js) run [`firestore.rules`](firestore.rules) against the local Firestore emulator through `@firebase/rules-unit-testing`. `npm test` starts the emulator, runs the suite and shuts it down again; nothing touches the real project, and no billing account is involved. @@ -507,12 +519,12 @@ it down again; nothing touches the real project, and no billing account is invol | Group | What it pins down | |---|---| | `signed-out access` | Every collection is closed to an unauthenticated client — the one guard shared by all four rule blocks. | -| `users` | Self-registration only, the document id matching the `id` field, that nobody updates a profile afterwards — including the two shapes of the old hole, awarding yourself points and writing points onto someone else — and that a profile is readable only by its owner. | +| `users` | Self-registration only, the document id matching the `id` field, that a personal address cannot register at all and the profile's `email` must be the session's own — removing those two clauses turns exactly those two tests red — that a blank department, a client-chosen `createdAt` and any unexpected or missing field are refused, that nobody updates a profile afterwards — including the two shapes of the old hole, awarding yourself points and writing points onto someone else — and that a profile is readable only by its owner. | | `notes` (reads) | A note in the reader's own department is readable and another department's note is not; an unfiltered query is refused, one filtered to the reader's own department is allowed, and one filtered to another department is refused. Two more cover the query the app actually builds for a profile: `where uploaderUid == me` is allowed without a department filter, and the same query for someone else's uid is refused. | | `notes` | `uploaderUid` cannot be forged and a note cannot be born with a score; the uploader may edit metadata but not the totals, the department, or the identity fields; a vote and the totals it implies are accepted only together, must match the arithmetic exactly, cannot be counted twice, and cannot be cast on your own note; delete is owner-only. | | `note creation is pinned to an exact shape` | One test per way a hand-rolled request used to get through: an unexpected field, a missing one, a resurrected `avgRating`, another department, a forged `uploaderName`, an email address written onto a note, a client-chosen `createdAt`, a wrong type, and an empty or oversized title. Each asserts the refusal, so relaxing any single check turns exactly one test red. | | `note content` | The batched note-plus-file upload, owner-only create, replace and delete, that a file on another department's note cannot be read — otherwise the boundary is walked around through the file path — that a content document carrying anything but `fileData` is refused, and that a batch cannot pair a legitimate note of its own with a file smuggled onto someone else's. | -| `ratings` | The `_` document id, which is what stops one user voting as another, plus the 1–5 integer range. Four cover the link back to the note: a vote cannot be written or changed without the totals that follow from it, the totals must match the vote from this side too, and the inflation loop above is refused at its first move. Removing that one clause turns exactly those three red and leaves the other 86 green. Deletion has four: a vote cannot be removed on its own, not even by the note's owner; the owner may remove it in the same commit that deletes the note; a stranger cannot reach it by deleting a note they do not own; and a vote whose note is already gone stays refused, which is the gap the prune script covers. | +| `ratings` | The `_` document id, which is what stops one user voting as another, plus the 1–5 integer range. Six cover the link back to the note: a vote cannot be written or changed without the totals that follow from it, the totals must match the vote from this side too, the inflation loop above is refused at its first move, and — because the app writes through `runTransaction` rather than a batch — the transaction the repository actually issues is exercised in both directions. Removing that one clause turns exactly those three red and leaves the other 86 green. Deletion has four: a vote cannot be removed on its own, not even by the note's owner; the owner may remove it in the same commit that deletes the note; a stranger cannot reach it by deleting a note they do not own; and a vote whose note is already gone stays refused, which is the gap the prune script covers. | | `unmatched paths` | A path no `match` block covers is denied, so adding a collection to the app without adding a rule fails closed rather than open. | Fixtures are seeded with `withSecurityRulesDisabled`, so a broken rule surfaces as a diff --git a/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseUserRepository.kt b/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseUserRepository.kt index 3f350ea..f512627 100644 --- a/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseUserRepository.kt +++ b/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseUserRepository.kt @@ -1,5 +1,7 @@ package duygu.yilmaz.campusnote.data.repository +import com.google.firebase.firestore.DocumentSnapshot +import com.google.firebase.firestore.FieldValue import com.google.firebase.firestore.FirebaseFirestore import duygu.yilmaz.campusnote.data.model.UserProfile import kotlinx.coroutines.tasks.await @@ -8,10 +10,23 @@ class FirebaseUserRepository( private val firestore: FirebaseFirestore = FirebaseFirestore.getInstance() ) : UserRepository { + /** + * Profili alan alan yazıyor, veri sınıfını olduğu gibi değil: `createdAt` sunucu + * saatinden geliyor ve [FieldValue.serverTimestamp] bir `Long` alanına sığmıyor. + * Güvenlik kuralı da `request.time` bekliyor — kayıt tarihi istemcinin + * söylediği şey olmamalı. + */ override suspend fun saveUser(user: UserProfile) { firestore.collection(USERS_COLLECTION) .document(user.id) - .set(user) + .set( + mapOf( + "id" to user.id, + "email" to user.email, + "department" to user.department, + "createdAt" to FieldValue.serverTimestamp() + ) + ) .await() } @@ -27,10 +42,25 @@ class FirebaseUserRepository( id = document.getString("id") ?: document.id, email = document.getString("email") ?: "", department = document.getString("department") ?: "", - createdAt = document.getLong("createdAt") ?: 0L + createdAt = document.createdAtMillis() ) } + /** + * Kayıt tarihi artık sunucu saatinden gelen bir `Timestamp`; sunucu saatine + * geçmeden önce yazılmış profiller hâlâ epoch millis taşıyor. + * + * `getTimestamp()` alan sayı olduğunda null dönmüyor, [RuntimeException] + * fırlatıyor — yani `?:` ile yedeklemek yetmiyor, çağrının kendisi sarılmak + * zorunda. Aynı tuzak `FirebaseNoteRepository.toPost()` içinde de var ve orada + * da böyle çözülmüş; ikisi ayrışırsa biri eski veride çöker. + */ + private fun DocumentSnapshot.createdAtMillis(): Long = try { + getTimestamp("createdAt")?.toDate()?.time ?: getLong("createdAt") ?: 0L + } catch (_: Exception) { + getLong("createdAt") ?: 0L + } + private companion object { const val USERS_COLLECTION = "users" } diff --git a/app/src/main/java/duygu/yilmaz/campusnote/ui/auth/RegisterViewModel.kt b/app/src/main/java/duygu/yilmaz/campusnote/ui/auth/RegisterViewModel.kt index 3a4c922..3662ba4 100644 --- a/app/src/main/java/duygu/yilmaz/campusnote/ui/auth/RegisterViewModel.kt +++ b/app/src/main/java/duygu/yilmaz/campusnote/ui/auth/RegisterViewModel.kt @@ -41,8 +41,8 @@ class RegisterViewModel( UserProfile( id = authenticatedUser.uid, email = email, - department = department, - createdAt = System.currentTimeMillis() + department = department + // createdAt sunucuda yazılıyor; bkz. FirebaseUserRepository. ) ) AuthUiState.Success diff --git a/firestore-tests/rules.test.js b/firestore-tests/rules.test.js index 990ac1c..42eac83 100644 --- a/firestore-tests/rules.test.js +++ b/firestore-tests/rules.test.js @@ -13,6 +13,7 @@ import { query, serverTimestamp, setDoc, + runTransaction, updateDoc, where, writeBatch, @@ -179,26 +180,91 @@ describe("users", () => { await assertFails(getDoc(doc(rater, "users", OWNER))); }); + /** A profile in exactly the shape the create rule now demands. */ + const profileFields = (overrides = {}) => ({ + id: RATER, + email: RATER_EMAIL, + department: DEPARTMENT, + createdAt: serverTimestamp(), + ...overrides, + }); + test("registration writes a profile whose id matches the caller", async () => { await testEnv.clearFirestore(); - await assertSucceeds( - setDoc(doc(rater, "users", RATER), { - id: RATER, - email: "rater@ogr.akdeniz.edu.tr", - department: "Bilgisayar Mühendisliği", - createdAt: 1, - }), - ); + await assertSucceeds(setDoc(doc(rater, "users", RATER), profileFields())); }); test("a user cannot create a profile under someone else's uid", async () => { await testEnv.clearFirestore(); - await assertFails(setDoc(doc(rater, "users", OWNER), { id: OWNER })); + await assertFails( + setDoc(doc(rater, "users", OWNER), profileFields({ id: OWNER })), + ); }); test("a create whose id field disagrees with the document id is rejected", async () => { await testEnv.clearFirestore(); - await assertFails(setDoc(doc(rater, "users", RATER), { id: OWNER })); + await assertFails( + setDoc(doc(rater, "users", RATER), profileFields({ id: OWNER })), + ); + }); + + /** + * The university restriction used to live only in RegisterActivity. Firebase Auth + * accepts any address, so anyone could sign up with a personal one, skip the + * Android screen and write themselves a profile — and since reads are scoped by + * `department`, which is read from this very document, that profile opened a + * department's notes. + */ + test("a profile cannot be created from a non-university account", async () => { + await testEnv.clearFirestore(); + const outsider = testEnv + .authenticatedContext("outsider-uid", { email: "someone@gmail.com" }) + .firestore(); + + await assertFails( + setDoc( + doc(outsider, "users", "outsider-uid"), + profileFields({ id: "outsider-uid", email: "someone@gmail.com" }), + ), + ); + }); + + test("the profile's email must be the session's own", async () => { + await testEnv.clearFirestore(); + // Claiming a university address while signed in as someone else was the other + // half of the same forgery. + await assertFails( + setDoc( + doc(rater, "users", RATER), + profileFields({ email: "rektor@ogr.akdeniz.edu.tr" }), + ), + ); + }); + + test("a profile carrying an unexpected or missing field is rejected", async () => { + await testEnv.clearFirestore(); + await assertFails( + setDoc(doc(rater, "users", RATER), profileFields({ points: 999999 })), + ); + + const { department, ...withoutDepartment } = profileFields(); + await assertFails(setDoc(doc(rater, "users", RATER), withoutDepartment)); + }); + + test("a blank department is rejected", async () => { + await testEnv.clearFirestore(); + // `ownDepartment()` reads this field to decide what the account may read, so an + // empty one is not a cosmetic problem. + await assertFails( + setDoc(doc(rater, "users", RATER), profileFields({ department: "" })), + ); + }); + + test("createdAt must be the server's clock", async () => { + await testEnv.clearFirestore(); + await assertFails( + setDoc(doc(rater, "users", RATER), profileFields({ createdAt: 1 })), + ); }); test("nobody may update a profile, not even its owner", async () => { @@ -678,6 +744,45 @@ describe("ratings", () => { await assertFails(vote(rater, RATER, NOTE, 5, { sum: 9, count: 1 })); }); + /** + * The app does not use a batch here — `FirebaseRatingRepository.submitRating` + * wraps the two writes in `runTransaction`, so it can re-read the note and + * compute the new totals from what it finds. Rules treat a transaction's writes + * as one commit the same way they treat a batch's, and `getAfter()` sees across + * it, but that is the mechanism the app actually ships and nothing here exercised + * it. This does, with the arithmetic the repository performs. + */ + test("the vote the app writes, through a transaction, is accepted", async () => { + await assertSucceeds( + runTransaction(rater, async (transaction) => { + const noteRef = doc(rater, "notes", NOTE); + const note = await transaction.get(noteRef); + + transaction.update(noteRef, { + ratingSum: note.data().ratingSum + 4, + ratingCount: note.data().ratingCount + 1, + }); + transaction.set(doc(rater, "ratings", `${RATER}_${NOTE}`), { + uid: RATER, + noteId: NOTE, + rating: 4, + }); + }), + ); + }); + + test("a transaction cannot write the vote without the totals either", async () => { + await assertFails( + runTransaction(rater, async (transaction) => { + transaction.set(doc(rater, "ratings", `${RATER}_${NOTE}`), { + uid: RATER, + noteId: NOTE, + rating: 4, + }); + }), + ); + }); + test("the totals must match the vote from this side too", async () => { // Written as a batch, so the note rule is satisfied by the vote beside it and // the arithmetic is the only thing left to reject. diff --git a/firestore.rules b/firestore.rules index 45b3c45..590a0ab 100644 --- a/firestore.rules +++ b/firestore.rules @@ -50,9 +50,55 @@ service cloud.firestore { */ allow read: if isOwner(userId); - // Kayıt sırasında kullanıcı yalnızca kendi dokümanını oluşturabilir. + /** + * Kayıt. + * + * Buradaki tek kontrol "kendi belgeni yazıyorsun ve `id` alanı belge kimliğiyle + * aynı" idi. Geri kalan her şey serbestti, ve bunun en ağır sonucu şuydu: + * `@ogr.akdeniz.edu.tr` kısıtı yalnızca `RegisterActivity`'de, yani istemcide + * yaşıyordu. Firebase Auth'ta e-posta/parola açık olduğu için herhangi biri + * gmail adresiyle hesap açıp Firestore'a doğrudan bağlanarak kendine profil + * yazabiliyordu — üstelik profildeki `email` alanına oturumdakinden başka bir + * adres, `department` alanına da listede olmayan bir bölüm koyarak. + * + * Okuma kapsamı `department`'a bağlandığından beri bu bir erişim sorunu: + * `ownDepartment()` tam da bu belgeden okuyor, yani doğrulanmamış bir profil + * istediği bölümün bütün notlarını açıyordu. + * + * Adres oturumun kendisinden alınıyor — istemcinin yazdığı değerden değil — + * ve alan adı burada, sunucuda kontrol ediliyor. + */ allow create: if isOwner(userId) - && request.resource.data.id == userId; + && hasExactProfileFields() + && request.resource.data.id == userId + && request.resource.data.email == request.auth.token.email + && isUniversityAddress(request.auth.token.email) + // Bölüm listesinin 144 girdisini kurala gömmek onu her müfredat + // değişikliğinde bozardı; buradaki iş boş ya da devasa bir değerin + // `ownDepartment()` üzerinden erişim kapsamı olmasını engellemek. + && request.resource.data.department is string + && request.resource.data.department.size() > 0 + && request.resource.data.department.size() <= 120 + // Sunucu saati; kayıt tarihi istemciden gelmiyor. + && request.resource.data.createdAt == request.time; + + function profileFields() { + return ['id', 'email', 'department', 'createdAt']; + } + + function hasExactProfileFields() { + return request.resource.data.keys().hasOnly(profileFields()) + && request.resource.data.keys().hasAll(profileFields()); + } + + /** + * `matches` bütün dizeyi eşleştiriyor. Nokta karakter sınıfıyla yazıldı + * (`[.]`), kaçış yerine: kural dilinde ters bölü iki kez kaçmak zorunda ve + * sessizce yanlış yazılması kolay. + */ + function isUniversityAddress(email) { + return email is string && email.matches('.*@ogr[.]akdeniz[.]edu[.]tr'); + } // Uygulama bu dokümanı yalnızca oluşturuyor, hiç güncellemiyor // (`saveUser` tek çağrısı kayıt akışında). Açık bırakmanın bedeli vardı: From 36c1994642222d9b736c9446505afe3ebb5b7896 Mon Sep 17 00:00:00 2001 From: Duygu <84078855+duuyguyiilmaz@users.noreply.github.com> Date: Tue, 1 Sep 2026 20:14:30 +0300 Subject: [PATCH 3/3] fix: whitelist what an edit may write, and scope who may read a vote MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four rule findings, and the two documentation lines that had gone stale under them. **Editing a note was a blacklist.** The rule named the fields that must not move and let everything else through, so a field nobody had thought of — including one added next year — was writable by default, and the metadata that *was* allowed had no type or size bound at all: a number in `title`, two megabytes in `description`. It is now a whitelist. `changedFields().hasOnly` fixes the set to the five the edit screen writes, each is checked the way creation checks it, and `updatedAt` must be `request.time`. **Replacing a file skipped the schema.** Creating a content document pinned it to a single `fileData` string; updating one checked only ownership. Writing the document twice — a valid one, then anything at all — walked around the check entirely. Update now carries the same shape requirement. **Every vote was readable by every signed-in user.** Each row says who rated which note and how, and the app never needed the collection: the voter reads their own document inside the rating transaction, and a note's owner reads the votes on their own note when deleting it. Reads are now scoped to exactly those two, which Firestore enforces through the query — `where uid == me` or `where noteId ==` a note you own passes, and pulling the collection does not. **The bulk delete was only ever tested with one vote.** That left the interesting question untested: rules cap the document access calls a request may make, and each rating delete evaluates `existsAfter()` and `get()` on the note. If those counted per rating rather than per path, a note with a dozen votes would have become undeletable — precisely the note that would have them. A test now deletes a note with twelve votes in one commit; it passes, so the calls are cached per path, and the only ceiling left is the 500-write batch limit already documented. **README:** `RatingCalculatorTest` no longer claims to check "average recalculation" — there has been no average since the field was removed — and the Data migrations section no longer says the feed downloads every note in the department, which stopped being true when it was paginated. Its `createdAt` material is now a subsection of its own, which also fixes `Continuous integration` sitting under Data migrations as if CI were a migration. The CI section now states what it cannot check: it runs the rules against the emulator, which says the file is correct and nothing about the live project running it. Rules suite 76 -> 87, 107 green in firestore-tests, 126 on the JVM side, lint clean, deployed. Not re-verified on the device this round: note editing and deletion. `updateNote` writes exactly the five whitelisted fields with a server timestamp — checked against the rule by reading both — and the emulator covers the positive case, but the account on the emulator is the throwaway one from the previous commit and has no note to edit. Co-Authored-By: Claude Opus 5 --- README.md | 39 +++++---- firestore-tests/rules.test.js | 154 ++++++++++++++++++++++++++++++++-- firestore.rules | 67 ++++++++++++--- 3 files changed, 227 insertions(+), 33 deletions(-) diff --git a/README.md b/README.md index b0be11d..15458b0 100644 --- a/README.md +++ b/README.md @@ -235,8 +235,8 @@ uploader, and pin each rating document to its author's UID. The delete rule is t *only* ownership check on deletion — the app code does not verify it. **Reads are scoped, not merely filtered.** Every read used to be `if isSignedIn()`, so any -signed-in student could pull every profile, every note and every attached file in the -university. That the app only ever asked for its own department was a property of the app, +signed-in student could pull every profile, every note, every attached file and every vote +in the university — the votes being a record of who rated what, and how. That the app only ever asked for its own department was a property of the app, and the app is not the only client anyone can write. Notes and their file subcollections are now readable only when their `department` matches the department on the caller's own profile, or the note is the caller's own. Firestore evaluates a list against the *query* @@ -248,7 +248,8 @@ single-branch rule rejected them outright — the feed opened empty in productio how the branch was found. Profiles went further and are readable only by their owner: all four `getUser` calls in the app pass the caller's own uid, and the leaderboard takes the uploader's name from the note, so the open rule was -granting an access nobody needed. +granting an access nobody needed. Votes are the same shape of fix — readable by the voter, +and by the owner of the note being rated, which are exactly the two reads the app makes. **A vote can only be deleted with its note.** `ratings` is a top-level collection rather than a subcollection, so deleting a note never touched the votes cast on it and @@ -446,7 +447,7 @@ the app is built around — what the screen actually shows. | Suite | What it pins down | |---|---| -| `RatingCalculatorTest` | First-time votes, changed votes, unchanged votes, average recalculation, and two inconsistent-data guards — the total flooring at zero, and the vote count flooring at one so the average never divides by zero. | +| `RatingCalculatorTest` | First-time votes, changed votes, unchanged votes, and two inconsistent-data guards: the total flooring at zero, and the vote count flooring at one. Both floors are mirrored in `firestore.rules`, so the client and the server have to agree on them exactly. | | `FeedViewModelTest` | The contribution gate: locked without an upload, unlocked with one, and locked for a missing profile or a blank department — and that the department query is never even built while locked. Also the paging: the window each page asks for, when there is more to fetch, that the last page stops asking, and that the list stays on screen while the next page loads. | | `UploadViewModelTest` | The uid, email and department written onto a note, including the `UNKNOWN_DEPARTMENT` fallback that keeps a blank department out of `whereEqualTo` queries. | | `NoteDetailViewModelTest` | Metadata and file content loading as separate states, no file read for a note without one, and the rating rules (own note, missing session, deleted note). | @@ -511,7 +512,7 @@ enforces — who may delete a note, whether a score can move without a vote behi invisible to them. That is the layer an attacker actually meets: the Android client can be replaced, the rules cannot. -76 tests in [`firestore-tests/rules.test.js`](firestore-tests/rules.test.js) run +87 tests in [`firestore-tests/rules.test.js`](firestore-tests/rules.test.js) run [`firestore.rules`](firestore.rules) against the local Firestore emulator through `@firebase/rules-unit-testing`. `npm test` starts the emulator, runs the suite and shuts it down again; nothing touches the real project, and no billing account is involved. @@ -521,10 +522,10 @@ it down again; nothing touches the real project, and no billing account is invol | `signed-out access` | Every collection is closed to an unauthenticated client — the one guard shared by all four rule blocks. | | `users` | Self-registration only, the document id matching the `id` field, that a personal address cannot register at all and the profile's `email` must be the session's own — removing those two clauses turns exactly those two tests red — that a blank department, a client-chosen `createdAt` and any unexpected or missing field are refused, that nobody updates a profile afterwards — including the two shapes of the old hole, awarding yourself points and writing points onto someone else — and that a profile is readable only by its owner. | | `notes` (reads) | A note in the reader's own department is readable and another department's note is not; an unfiltered query is refused, one filtered to the reader's own department is allowed, and one filtered to another department is refused. Two more cover the query the app actually builds for a profile: `where uploaderUid == me` is allowed without a department filter, and the same query for someone else's uid is refused. | -| `notes` | `uploaderUid` cannot be forged and a note cannot be born with a score; the uploader may edit metadata but not the totals, the department, or the identity fields; a vote and the totals it implies are accepted only together, must match the arithmetic exactly, cannot be counted twice, and cannot be cast on your own note; delete is owner-only. | +| `notes` | `uploaderUid` cannot be forged and a note cannot be born with a score; an edit may touch only the metadata fields and each is type- and size-checked, so a field nobody expects cannot be introduced and `updatedAt` cannot be backdated; a vote and the totals it implies are accepted only together, must match the arithmetic exactly, cannot be counted twice, and cannot be cast on your own note; delete is owner-only. | | `note creation is pinned to an exact shape` | One test per way a hand-rolled request used to get through: an unexpected field, a missing one, a resurrected `avgRating`, another department, a forged `uploaderName`, an email address written onto a note, a client-chosen `createdAt`, a wrong type, and an empty or oversized title. Each asserts the refusal, so relaxing any single check turns exactly one test red. | -| `note content` | The batched note-plus-file upload, owner-only create, replace and delete, that a file on another department's note cannot be read — otherwise the boundary is walked around through the file path — that a content document carrying anything but `fileData` is refused, and that a batch cannot pair a legitimate note of its own with a file smuggled onto someone else's. | -| `ratings` | The `_` document id, which is what stops one user voting as another, plus the 1–5 integer range. Six cover the link back to the note: a vote cannot be written or changed without the totals that follow from it, the totals must match the vote from this side too, the inflation loop above is refused at its first move, and — because the app writes through `runTransaction` rather than a batch — the transaction the repository actually issues is exercised in both directions. Removing that one clause turns exactly those three red and leaves the other 86 green. Deletion has four: a vote cannot be removed on its own, not even by the note's owner; the owner may remove it in the same commit that deletes the note; a stranger cannot reach it by deleting a note they do not own; and a vote whose note is already gone stays refused, which is the gap the prune script covers. | +| `note content` | The batched note-plus-file upload, owner-only create, replace and delete, that replacing a file must keep the same shape — create pinned it and update did not, so writing the document twice skipped the check — that a file on another department's note cannot be read — otherwise the boundary is walked around through the file path — that a content document carrying anything but `fileData` is refused, and that a batch cannot pair a legitimate note of its own with a file smuggled onto someone else's. | +| `ratings` | The `_` document id, which is what stops one user voting as another, plus the 1–5 integer range. Six cover who may read a vote: your own is readable, someone else's is not, the whole collection cannot be pulled, and of the two queries the app makes — your own votes, and the votes on your own note when deleting it — each is allowed for the person entitled to it and refused for anyone else. Six cover the link back to the note: a vote cannot be written or changed without the totals that follow from it, the totals must match the vote from this side too, the inflation loop above is refused at its first move, and — because the app writes through `runTransaction` rather than a batch — the transaction the repository actually issues is exercised in both directions. Removing that one clause turns exactly those three red and leaves the other 86 green. Deletion has four: a vote cannot be removed on its own, not even by the note's owner; the owner may remove it in the same commit that deletes the note; a stranger cannot reach it by deleting a note they do not own; and a vote whose note is already gone stays refused, which is the gap the prune script covers. | | `unmatched paths` | A path no `match` block covers is denied, so adding a collection to the app without adding a rule fails closed rather than open. | Fixtures are seeded with `withSecurityRulesDisabled`, so a broken rule surfaces as a @@ -536,9 +537,11 @@ everything would otherwise pass a suite made only of `assertFails`. ## Data migrations -The feed and the leaderboard currently download every note in the department and sort in -the client. Paginating them means ordering in the query instead — and Firestore's -ordering has two behaviours that turn old data into damage a user can see: +### Normalising `createdAt` + +The feed and the leaderboard used to download every note in the department and sort in the +client. Paginating them meant ordering in the query instead — and Firestore's ordering has +two behaviours that turn old data into damage a user can see: - **A document that lacks the ordered field is excluded from the query.** Not sorted last: absent. A note written before `createdAt` existed would vanish from the feed @@ -546,8 +549,8 @@ ordering has two behaviours that turn old data into damage a user can see: - **Values sort by type before value, and numbers come before timestamps.** A note whose `createdAt` is epoch millis would land below every timestamped note whatever its date. -`toPost()` still reads both shapes, which is the evidence that both exist. So the data is -normalised before the ordering changes, not after: +`toPost()` still reads both shapes, which is the evidence that both exist. The data was +normalised before the ordering changed, not after: ```bash cd firestore-tests @@ -609,7 +612,9 @@ against the emulator: the orphan goes, the live vote stays, and the note id is r the field rather than split out of the document id, which would be ambiguous whenever a uid contains an underscore. -### Continuous integration +--- + +## Continuous integration [`.github/workflows/android.yml`](.github/workflows/android.yml) runs on every push to `main` and on every pull request. Two jobs run in parallel: unit tests, a debug build and @@ -620,6 +625,12 @@ Because `google-services.json` is not in the repository, CI copies the example f place before building. The placeholder credentials are enough — the Google Services plugin only parses the file at build time and never contacts Firebase. +That is also the limit of what CI can tell you about the security rules: it runs +`firestore.rules` against the emulator, which proves the file is correct and says nothing +about whether the Firebase project is running it. Nothing here holds credentials for the +live project, so nothing here can check. Deploying is part of merging a rules change — see +[Known limitations](#known-limitations). + --- ## Project structure diff --git a/firestore-tests/rules.test.js b/firestore-tests/rules.test.js index 42eac83..57b06f2 100644 --- a/firestore-tests/rules.test.js +++ b/firestore-tests/rules.test.js @@ -495,7 +495,56 @@ describe("notes", () => { test("the uploader may edit their note's metadata", async () => { await assertSucceeds( - updateDoc(doc(owner, "notes", NOTE), { title: "Güncellenmiş başlık" }), + updateDoc(doc(owner, "notes", NOTE), { + title: "Güncellenmiş başlık", + updatedAt: serverTimestamp(), + }), + ); + }); + + /** + * The check used to be a blacklist — it named the fields that must not move and + * let everything else through. A field nobody thought of, including one added + * next year, was writable by default, and the metadata that *was* allowed had no + * type or size bound at all. + */ + test("an edit cannot introduce a field nobody expects", async () => { + await assertFails( + updateDoc(doc(owner, "notes", NOTE), { + title: "Güncellenmiş başlık", + updatedAt: serverTimestamp(), + isFeatured: true, + }), + ); + }); + + test("an edit cannot put the wrong type or size in the metadata", async () => { + await assertFails( + updateDoc(doc(owner, "notes", NOTE), { + title: 42, + updatedAt: serverTimestamp(), + }), + ); + await assertFails( + updateDoc(doc(owner, "notes", NOTE), { + title: "a".repeat(201), + updatedAt: serverTimestamp(), + }), + ); + await assertFails( + updateDoc(doc(owner, "notes", NOTE), { + title: "", + updatedAt: serverTimestamp(), + }), + ); + }); + + test("an edit cannot backdate updatedAt", async () => { + await assertFails( + updateDoc(doc(owner, "notes", NOTE), { + title: "Güncellenmiş başlık", + updatedAt: 1, + }), ); }); @@ -620,6 +669,22 @@ describe("note content", () => { await assertSucceeds(deleteDoc(doc(owner, "notes", NOTE, "content", "file"))); }); + /** + * Create pinned the shape and update did not, so the whole check could be skipped + * by writing the document twice — a valid one first, anything at all second. + */ + test("replacing a file must keep the same shape", async () => { + await assertFails( + updateDoc(doc(owner, "notes", NOTE, "content", "file"), { + fileData: "eWVuaQ==", + uploaderUid: RATER, + }), + ); + await assertFails( + updateDoc(doc(owner, "notes", NOTE, "content", "file"), { fileData: 42 }), + ); + }); + test("someone else cannot overwrite or delete the file", async () => { await assertFails( setDoc(doc(rater, "notes", NOTE, "content", "file"), { @@ -684,6 +749,54 @@ describe("note content", () => { describe("ratings", () => { const validVote = { uid: RATER, noteId: NOTE, rating: 4 }; + /** Seeds a vote without going through the rules, so a broken rule shows up as a + * failed assertion rather than a failed setup. */ + const seedVote = async (vote = validVote, id = `${RATER}_${NOTE}`) => { + await testEnv.withSecurityRulesDisabled(async (context) => { + await setDoc(doc(context.firestore(), "ratings", id), vote); + }); + }; + + + test("a user may read their own vote", async () => { + await seedVote(); + await assertSucceeds(getDoc(doc(rater, "ratings", `${RATER}_${NOTE}`))); + }); + + /** + * The open read rule exposed every vote in the database to every signed-in user: + * who rated what, and how. The app never needed that — the voter reads their own + * document inside the rating transaction, and the note's owner reads the votes on + * their own note when deleting it. + */ + test("a user cannot read someone else's vote", async () => { + await seedVote({ uid: OWNER, noteId: NOTE, rating: 4 }, `${OWNER}_${NOTE}`); + await assertFails(getDoc(doc(rater, "ratings", `${OWNER}_${NOTE}`))); + }); + + test("the whole ratings collection cannot be pulled", async () => { + await assertFails(getDocs(collection(rater, "ratings"))); + }); + + test("a user may query their own votes", async () => { + await assertSucceeds( + getDocs(query(collection(rater, "ratings"), where("uid", "==", RATER))), + ); + }); + + test("the note's owner may query the votes on their note", async () => { + // This is the read `deleteNote` performs before removing a note. + await assertSucceeds( + getDocs(query(collection(owner, "ratings"), where("noteId", "==", NOTE))), + ); + }); + + test("a stranger cannot query the votes on someone else's note", async () => { + await assertFails( + getDocs(query(collection(rater, "ratings"), where("noteId", "==", NOTE))), + ); + }); + test("a vote is written under _", async () => { await assertSucceeds(vote(rater, RATER, NOTE, 4, { sum: 4, count: 1 })); }); @@ -835,14 +948,6 @@ describe("ratings", () => { } }); - /** Seeds a vote without going through the rules, so a broken rule shows up as a - * failed assertion rather than a failed setup. */ - const seedVote = async (vote = validVote, id = `${RATER}_${NOTE}`) => { - await testEnv.withSecurityRulesDisabled(async (context) => { - await setDoc(doc(context.firestore(), "ratings", id), vote); - }); - }; - /** * A vote on its own must stay put. Deleting one does not lower the note's total, * but it does hand the voter a second first-time vote — the rule would count it @@ -873,6 +978,37 @@ describe("ratings", () => { await assertSucceeds(batch.commit()); }); + /** + * The realistic delete: a note with many votes, all removed in the commit that + * removes it. Rules cap the document access calls one request may make, and each + * rating delete here evaluates `existsAfter()` and `get()` on the note. If those + * are counted per rating rather than cached per path, a note with a dozen votes + * becomes undeletable — which is exactly the size of note that would have them. + */ + test("a note with many votes can still be deleted in one commit", async () => { + const voters = Array.from({ length: 12 }, (_, i) => `voter-${i}`); + + await testEnv.withSecurityRulesDisabled(async (context) => { + const db = context.firestore(); + for (const uid of voters) { + await setDoc(doc(db, "ratings", `${uid}_${NOTE}`), { + uid, + noteId: NOTE, + rating: 4, + }); + } + }); + + const batch = writeBatch(owner); + for (const uid of voters) { + batch.delete(doc(owner, "ratings", `${uid}_${NOTE}`)); + } + batch.delete(doc(owner, "notes", NOTE, "content", "file")); + batch.delete(doc(owner, "notes", NOTE)); + + await assertSucceeds(batch.commit()); + }); + test("a stranger cannot delete votes by deleting a note they do not own", async () => { await seedVote(); diff --git a/firestore.rules b/firestore.rules index 590a0ab..1a226fd 100644 --- a/firestore.rules +++ b/firestore.rules @@ -244,16 +244,36 @@ service cloud.firestore { || isRatingWrite(noteId) ); - // Sahibi metadata'yı düzenleyebilir ama puan alanlarına, sahiplik bilgisine - // ve bölüme dokunamaz — kendi notunu öne çıkarmanın en kısa yolu buydu. - // `avgRating` listede: alan artık yazılmıyor, düzenlemeyle geri gelmesin. + /** + * Sahibi yalnızca metadata'yı düzenleyebilir. + * + * Bu kontrol eskiden kara listeydi: dokunulmaması gereken alanlar sayılıyor, + * geri kalan her şey serbest bırakılıyordu. Sayılmayan bir alan — bugün var + * olmayan ama yarın eklenecek olan da dahil — sessizce yazılabilir demekti; + * üstelik `title`'a sayı, `description`'a megabaytlık metin koymak da mümkündü. + * + * Artık beyaz liste: değişebilecek alan kümesi `hasOnly` ile sabit ve her biri + * oluşturmadakiyle aynı tip/boyut kontrolünden geçiyor. Yeni bir alan eklemek + * kuralı da güncellemeyi gerektiriyor, ki doğrusu bu. + */ function isMetadataEditByOwner() { return resource.data.uploaderUid == request.auth.uid - && !changedFields().hasAny([ - 'ratingSum', 'ratingCount', 'avgRating', - 'uploaderUid', 'uploaderName', 'uploaderEmail', - 'department', 'createdAt' - ]); + && changedFields().hasOnly(editableNoteFields()) + && hasValidEditedMetadata(); + } + + /** `updatedAt` yalnızca düzenlemede yazılıyor, oluşturmada yok. */ + function editableNoteFields() { + return ['course', 'title', 'description', 'tag', 'updatedAt']; + } + + function hasValidEditedMetadata() { + let data = request.resource.data; + return data.course is string && data.course.size() > 0 && data.course.size() <= 100 + && data.title is string && data.title.size() > 0 && data.title.size() <= 200 + && data.description is string && data.description.size() <= 2000 + && data.tag is string && data.tag.size() <= 50 + && data.updatedAt == request.time; } function ratingPath(noteId) { @@ -326,14 +346,41 @@ service cloud.firestore { && request.resource.data.keys().hasOnly(['fileData']) && request.resource.data.fileData is string; - allow update, delete: if isSignedIn() + // Güncelleme oluşturmayla aynı şemayı geçmek zorunda: create'te alan kümesi + // sabitlenip update'te serbest bırakılırsa, kontrol yalnızca ilk yazmayı + // bağlar ve ikincisinde atlanır. + allow update: if isSignedIn() + && get(/databases/$(database)/documents/notes/$(noteId)) + .data.uploaderUid == request.auth.uid + && request.resource.data.keys().hasOnly(['fileData']) + && request.resource.data.fileData is string; + + allow delete: if isSignedIn() && get(/databases/$(database)/documents/notes/$(noteId)) .data.uploaderUid == request.auth.uid; } } match /ratings/{ratingId} { - allow read: if isSignedIn(); + /** + * Oylar herkese açık değil. + * + * Kural `isSignedIn()` iken oturum açmış herkes bütün koleksiyonu okuyabiliyor + * ve her satır bir kullanıcının hangi nota kaç verdiğini söylüyordu — uid, not + * kimliği ve puan. Uygulamanın buna ihtiyacı yoktu; yalnızca iki okuma var: + * oy verenin transaction içinde kendi oyunu okuması, ve not sahibinin notu + * silerken o notun oylarını `where noteId == ...` ile bulması. + * + * İki dal tam olarak o iki okumaya karşılık geliyor. Firestore liste + * sorgusunu kurala göre değerlendirdiği için `where uid == ben` ya da + * `where noteId == kendi notum` taşımayan bir sorgu geçmiyor; koleksiyonun + * tamamını çekmek artık mümkün değil. + */ + allow read: if isSignedIn() && ( + resource.data.uid == request.auth.uid + || get(/databases/$(database)/documents/notes/$(resource.data.noteId)) + .data.uploaderUid == request.auth.uid + ); // Doküman kimliği "_" biçiminde; bu kalıbı zorunlu // kılmak bir kullanıcının başkasının oyunu yazmasını engelliyor.