From 59d1368b692e8802a9e0e742e8dbf9343942d786 Mon Sep 17 00:00:00 2001 From: Duygu <84078855+duuyguyiilmaz@users.noreply.github.com> Date: Mon, 31 Aug 2026 17:45:16 +0300 Subject: [PATCH] fix: stop the client from awarding itself points MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The README described this as "a malicious client could still award itself points", which understated it. There were two holes and neither needed a forged rating. `allow update: if isOwner(userId)` let a user write any field of their own document, so `points: 999999` was a one-line request. And any signed-in user could set any note's `ratingSum` to any non-negative value, which is the number both the leaderboard and the profile total are read from. The fix turned out to be mostly deletion. `users.points` is never read: ProfileViewModel already computes the total as `posts.sumOf { it.ratingSum }` over the user's own notes. The field existed only to be written — and storing it is what forced the rules to let one user write another user's document. Removing it removes the cross-user write from the rating transaction, the clause that permitted it, and `RatingCalculator.pointsDelta`, which had no other caller. Nothing changes on screen. It is the same reasoning already applied to the contribution gate: a value derived from the notes cannot drift from them, and cannot be forged separately from them. The user document is now `allow update: if false`, matching what the app does — saveUser is called once, during registration, and never again. What could not be deleted is the note's own totals, so those are verified instead. A vote writes the note and the rating document in one atomic commit, and the note's rule uses getAfter() to read the rating being written in that same commit, recompute ratingSum and ratingCount from it, and reject anything that does not match. Totals cannot move without a vote behind them, a changed vote cannot be counted twice, a note cannot be created with a score, an owner cannot raise their own note while editing it, and rating your own note is now refused by the server rather than only by the client. The expected totals are RatingCalculator's arithmetic restated, floors included, because client and rule must agree exactly. avgRating is range-checked rather than compared: rules do integer division, and it is a derived display field whose inputs are pinned. math.max does not exist in rules, hence the ternaries. Six new rule tests, and one existing test got sharper for a reason worth recording. Weakening the ratingSum check to `>= 0` failed to turn anything red: the old assertion used a sum of 500, which was already rejected by the avgRating range check, so it passed for the wrong reason. Rewritten with values that sit inside every range check, it fails on the arithmetic alone. Re-running the mutation now turns three tests red — that one, and the two covering the user document. Firestore suite: 38 rules tests, 46 total. Android suite: 107. Co-Authored-By: Claude Opus 5 --- README.md | 45 +++-- .../data/model/RatingCalculation.kt | 3 - .../campusnote/data/model/UserProfile.kt | 1 - .../repository/FirebaseRatingRepository.kt | 25 ++- .../data/repository/FirebaseUserRepository.kt | 1 - .../data/model/RatingCalculatorTest.kt | 5 - .../yilmaz/campusnote/testing/Fixtures.kt | 5 +- .../ui/auth/RegisterViewModelTest.kt | 1 - firestore-tests/rules.test.js | 156 ++++++++++++------ firestore.rules | 107 ++++++++---- 10 files changed, 226 insertions(+), 123 deletions(-) diff --git a/README.md b/README.md index 51c9c44..3d0b03c 100644 --- a/README.md +++ b/README.md @@ -98,6 +98,11 @@ flowchart TD by a `limit(1)` query for the user's own notes rather than a `hasUploadedNote` flag on the profile. A stored flag drifted: it was set on upload and never cleared on delete, so uploading once and deleting granted permanent access. +- **Points are derived too, and that is what made them safe.** A user's point total is + the sum of `ratingSum` over their own notes, computed where it is displayed. It used to + live in `users.points`, written by whoever cast the vote — which forced the rules to + let one user write another user's document, and left the owner able to write their own. + Deleting the field deleted both holes; nothing read it anyway. - **Rating uses a transaction.** `runTransaction` re-reads the note inside the transaction, so two people rating the same note at once cannot lose one of the votes. - **File content lives in a subcollection.** `notes/{id}/content/file` is separate from @@ -148,11 +153,11 @@ Cloud Firestore, four collections: | `id` | string | matches the Firebase Auth UID | | `email` | string | | | `department` | string | | -| `points` | number | earned from ratings on your notes | | `createdAt` | number | | -Older user documents may still carry a `hasUploadedNote` boolean. It is no longer read -or written — the contribution gate is derived from the user's notes instead. +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 +point total. Both were stored once and both drifted from the truth — see Key decisions. **`notes/{noteId}`** @@ -182,8 +187,22 @@ firebase deploy --only firestore:rules ``` The rules require authentication everywhere, restrict note edits and deletes to the -uploader, and pin each rating document to its author's UID. Note that the delete rule -is the *only* ownership check on deletion — the app code does not verify it. +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. + +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 expected totals are the same arithmetic as +[`RatingCalculator`](app/src/main/java/duygu/yilmaz/campusnote/data/model/RatingCalculation.kt), +floors included — the client and the rule have to agree exactly, so the rule is written +as that function's twin. `avgRating` is checked only for range: rules do integer +division, so an exact comparison is not available, and it is a derived display field +whose inputs are already pinned. They are covered by their own test suite; see [Rules tests](#rules-tests). @@ -193,12 +212,10 @@ They are covered by their own test suite; see [Rules tests](#rules-tests). Honest list of things a reviewer would spot, and why they are the way they are. -**The points system is client-authoritative.** -Rating a note updates the note's totals *and* the uploader's `points` directly from the -client, so the security rules have to permit one user to modify another user's points. -Values are constrained by type and range, but a malicious client could still award -itself points. The correct fix is to move the calculation into a Cloud Function so the -client only writes its vote — which again needs the Blaze plan. +**Rating totals are still computed on the client.** A Cloud Function would compute them +server-side, and that needs the Blaze plan. The rules make up most of the difference — +they recompute the expected totals from the vote and reject anything else (see +[Security rules](#security-rules)) — but the arithmetic itself still runs on a device. **Email addresses are never verified.** Registration only checks that the address ends in `@ogr.akdeniz.edu.tr`; no confirmation mail is sent, so anyone can register with a @@ -339,7 +356,7 @@ enforces — who may delete a note, who may write to whose `points` — was prev unverified. That is the layer an attacker actually meets: the Android client can be replaced, the rules cannot. -32 tests in [`firestore-tests/rules.test.js`](firestore-tests/rules.test.js) run +38 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. @@ -347,8 +364,8 @@ 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, and the narrow exception that lets a rater raise *another* user's `points` — including that it cannot carry a second field along, go negative, or be a non-integer. | -| `notes` | `uploaderUid` cannot be forged on create; metadata edits are the uploader's alone; a rater may touch only `ratingSum`, `ratingCount` and `avgRating`; delete is owner-only. | +| `users` | Self-registration only, the document id matching the `id` field, and that nobody updates a profile afterwards — including the two shapes of the old hole: awarding yourself points, and writing points onto someone else. | +| `notes` | `uploaderUid` cannot be forged and a note cannot be born with a score; the uploader may edit metadata but not the totals; 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 content` | The batched note-plus-file upload, owner-only replace and delete, and a test that deliberately asserts the *open* create rule, so the gap documented in `firestore.rules` cannot be closed by accident and go unnoticed. | | `ratings` | The `_` document id, which is what stops one user voting as another, plus the 1–5 integer range and the ban on deleting votes. | | `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. | diff --git a/app/src/main/java/duygu/yilmaz/campusnote/data/model/RatingCalculation.kt b/app/src/main/java/duygu/yilmaz/campusnote/data/model/RatingCalculation.kt index a084f26..cf6b8b6 100644 --- a/app/src/main/java/duygu/yilmaz/campusnote/data/model/RatingCalculation.kt +++ b/app/src/main/java/duygu/yilmaz/campusnote/data/model/RatingCalculation.kt @@ -5,8 +5,6 @@ data class RatingTotals( val sum: Long, val count: Long, val average: Double, - /** Not sahibinin puanına eklenecek fark; oy düşürüldüyse negatif olur. */ - val pointsDelta: Long, /** Kullanıcı ilk kez mi oy verdi, yoksa eski oyunu mu değiştirdi. */ val updatedExistingRating: Boolean ) @@ -61,7 +59,6 @@ object RatingCalculator { sum = sum, count = count, average = average, - pointsDelta = delta, updatedExistingRating = updatedExistingRating ) } diff --git a/app/src/main/java/duygu/yilmaz/campusnote/data/model/UserProfile.kt b/app/src/main/java/duygu/yilmaz/campusnote/data/model/UserProfile.kt index e8754b2..84d0a5d 100644 --- a/app/src/main/java/duygu/yilmaz/campusnote/data/model/UserProfile.kt +++ b/app/src/main/java/duygu/yilmaz/campusnote/data/model/UserProfile.kt @@ -4,7 +4,6 @@ data class UserProfile( val id: String = "", val email: String = "", val department: String = "", - val points: Long = 0L, val createdAt: Long = 0L ) { companion object { diff --git a/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseRatingRepository.kt b/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseRatingRepository.kt index bcf216e..9282ce0 100644 --- a/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseRatingRepository.kt +++ b/app/src/main/java/duygu/yilmaz/campusnote/data/repository/FirebaseRatingRepository.kt @@ -9,6 +9,17 @@ class FirebaseRatingRepository( private val firestore: FirebaseFirestore = FirebaseFirestore.getInstance() ) : RatingRepository { + /** + * Oy, notun toplamları ve oy dokümanı aynı transaction'da yazılıyor — güvenlik + * kurallarının ikisini birbirine bağlayabilmesi buna dayanıyor. Kural, notun yeni + * `ratingSum`'ını aynı commit'te yazılan oy dokümanından hesaplıyor, yani ikisi + * ayrılırsa yazma reddedilir. + * + * Not sahibinin puanı burada güncellenmiyor. Puan artık `users` dokümanında + * saklanmıyor; profil ekranı kullanıcının kendi notlarının `ratingSum` toplamından + * türetiyor. Saklandığı sürece kurallar bir kullanıcının başka bir kullanıcının + * dokümanına yazmasına izin vermek zorundaydı ve o izin kapatılamıyordu. + */ override suspend fun submitRating( noteId: String, raterUid: String, @@ -30,12 +41,6 @@ class FirebaseRatingRepository( if (ownerUid == raterUid) throw OwnNoteRatingException() val ratingSnapshot = transaction.get(ratingReference) - val ownerReference = ownerUid - .takeIf { it.isNotBlank() } - ?.let { firestore.collection(USERS_COLLECTION).document(it) } - val ownerSnapshot = ownerReference?.let { reference -> - transaction.get(reference) - } val totals = RatingCalculator.recalculate( currentSum = noteSnapshot.getLong(RATING_SUM_FIELD) ?: 0L, @@ -65,12 +70,6 @@ class FirebaseRatingRepository( ) ) - if (ownerReference != null && ownerSnapshot?.exists() == true) { - val currentPoints = ownerSnapshot.getLong(POINTS_FIELD) ?: 0L - val newPoints = (currentPoints + totals.pointsDelta).coerceAtLeast(0L) - transaction.update(ownerReference, POINTS_FIELD, newPoints) - } - RatingResult( average = totals.average, count = totals.count, @@ -83,7 +82,6 @@ class FirebaseRatingRepository( private companion object { const val NOTES_COLLECTION = "notes" const val RATINGS_COLLECTION = "ratings" - const val USERS_COLLECTION = "users" const val UPLOADER_UID_FIELD = "uploaderUid" const val RATING_SUM_FIELD = "ratingSum" const val RATING_COUNT_FIELD = "ratingCount" @@ -91,6 +89,5 @@ class FirebaseRatingRepository( const val RATING_FIELD = "rating" const val USER_ID_FIELD = "uid" const val NOTE_ID_FIELD = "noteId" - const val POINTS_FIELD = "points" } } 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 d24ca8e..3f350ea 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 @@ -27,7 +27,6 @@ class FirebaseUserRepository( id = document.getString("id") ?: document.id, email = document.getString("email") ?: "", department = document.getString("department") ?: "", - points = document.getLong("points") ?: 0L, createdAt = document.getLong("createdAt") ?: 0L ) } diff --git a/app/src/test/java/duygu/yilmaz/campusnote/data/model/RatingCalculatorTest.kt b/app/src/test/java/duygu/yilmaz/campusnote/data/model/RatingCalculatorTest.kt index fbd000e..e114acb 100644 --- a/app/src/test/java/duygu/yilmaz/campusnote/data/model/RatingCalculatorTest.kt +++ b/app/src/test/java/duygu/yilmaz/campusnote/data/model/RatingCalculatorTest.kt @@ -28,7 +28,6 @@ class RatingCalculatorTest { assertEquals(4L, totals.sum) assertEquals(1L, totals.count) assertEquals(4.0, totals.average, DELTA) - assertEquals(4L, totals.pointsDelta) assertFalse(totals.updatedExistingRating) } @@ -44,7 +43,6 @@ class RatingCalculatorTest { assertEquals(12L, totals.sum) assertEquals(3L, totals.count) assertEquals(4.0, totals.average, DELTA) - assertEquals(3L, totals.pointsDelta) } @Test @@ -59,7 +57,6 @@ class RatingCalculatorTest { assertEquals(10L, totals.sum) assertEquals(2L, totals.count) assertEquals(5.0, totals.average, DELTA) - assertEquals(1L, totals.pointsDelta) assertTrue(totals.updatedExistingRating) } @@ -74,7 +71,6 @@ class RatingCalculatorTest { assertEquals(6L, totals.sum) assertEquals(2L, totals.count) - assertEquals(-4L, totals.pointsDelta) } @Test @@ -88,7 +84,6 @@ class RatingCalculatorTest { assertEquals(7L, totals.sum) assertEquals(2L, totals.count) - assertEquals(0L, totals.pointsDelta) } @Test diff --git a/app/src/test/java/duygu/yilmaz/campusnote/testing/Fixtures.kt b/app/src/test/java/duygu/yilmaz/campusnote/testing/Fixtures.kt index 82726e4..ea488e5 100644 --- a/app/src/test/java/duygu/yilmaz/campusnote/testing/Fixtures.kt +++ b/app/src/test/java/duygu/yilmaz/campusnote/testing/Fixtures.kt @@ -18,9 +18,8 @@ fun authenticatedUser( fun userProfile( uid: String = "uid-1", email: String = "ogrenci@ogr.akdeniz.edu.tr", - department: String = "Bilgisayar Mühendisliği", - points: Long = 0L -) = UserProfile(id = uid, email = email, department = department, points = points) + department: String = "Bilgisayar Mühendisliği" +) = UserProfile(id = uid, email = email, department = department) fun post( id: String = "note-1", diff --git a/app/src/test/java/duygu/yilmaz/campusnote/ui/auth/RegisterViewModelTest.kt b/app/src/test/java/duygu/yilmaz/campusnote/ui/auth/RegisterViewModelTest.kt index c11e547..9e7ecc2 100644 --- a/app/src/test/java/duygu/yilmaz/campusnote/ui/auth/RegisterViewModelTest.kt +++ b/app/src/test/java/duygu/yilmaz/campusnote/ui/auth/RegisterViewModelTest.kt @@ -51,7 +51,6 @@ class RegisterViewModelTest { val saved = userRepository.savedUsers.single() assertEquals("duygu@ogr.akdeniz.edu.tr", saved.email) assertEquals("Bilgisayar Mühendisliği", saved.department) - assertEquals(0L, saved.points) } @Test diff --git a/firestore-tests/rules.test.js b/firestore-tests/rules.test.js index 0639295..659636a 100644 --- a/firestore-tests/rules.test.js +++ b/firestore-tests/rules.test.js @@ -55,14 +55,12 @@ beforeEach(async () => { id: OWNER, email: "owner@ogr.akdeniz.edu.tr", department: "Bilgisayar Mühendisliği", - points: 10, createdAt: 1, }); await setDoc(doc(db, "users", RATER), { id: RATER, email: "rater@ogr.akdeniz.edu.tr", department: "Bilgisayar Mühendisliği", - points: 0, createdAt: 1, }); await setDoc(doc(db, "notes", NOTE), { @@ -84,6 +82,31 @@ beforeEach(async () => { }); }); +/** + * Bir oyu uygulamanın yazdığı gibi yazar: notun toplamları ve oy dokümanı tek + * atomik commit'te. Kural, notun yeni toplamlarını aynı commit'teki oy + * dokümanından doğruluyor, o yüzden ikisini ayırmak testin anlamını kaybettirir. + * + * @param db oy veren bağlam + * @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 = {} }) { + const batch = writeBatch(db); + batch.update(doc(db, "notes", noteId), { + ratingSum: sum, + ratingCount: count, + avgRating: count === 0 ? 0 : sum / count, + ...extra, + }); + batch.set(doc(db, "ratings", `${uid}_${noteId}`), { + uid, + noteId, + rating, + }); + return batch.commit(); +} + describe("signed-out access", () => { // Every collection matches on isSignedIn() first, so this is the one guard // that has to hold everywhere: an unauthenticated client reads nothing. @@ -114,7 +137,6 @@ describe("users", () => { id: RATER, email: "rater@ogr.akdeniz.edu.tr", department: "Bilgisayar Mühendisliği", - points: 0, createdAt: 1, }), ); @@ -122,44 +144,36 @@ describe("users", () => { 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, points: 0 }), - ); + await assertFails(setDoc(doc(rater, "users", OWNER), { 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, points: 0 }), - ); + await assertFails(setDoc(doc(rater, "users", RATER), { id: OWNER })); }); - test("a user may edit their own profile freely", async () => { - await assertSucceeds( + test("nobody may update a profile, not even its owner", async () => { + // The app only ever creates this document. Leaving update open is what forced + // the old rule to permit writing `points` — the field that made self-awarding + // a one-line request. + await assertFails( updateDoc(doc(rater, "users", RATER), { department: "Matematik" }), ); - }); - - test("rating another user's note may raise only their points", async () => { - // The client-side rating flow needs this: it writes the owner's new total. - await assertSucceeds( - updateDoc(doc(rater, "users", OWNER), { points: 14 }), + await assertFails( + updateDoc(doc(owner, "users", OWNER), { department: "Matematik" }), ); }); - test("a points write cannot smuggle another field along with it", async () => { + test("a user cannot award themselves points", async () => { + // This was the hole: `allow update: if isOwner(userId)` let a user write any + // field of their own document, so `points: 999999` needed no rating at all. await assertFails( - updateDoc(doc(rater, "users", OWNER), { - points: 14, - email: "attacker@example.com", - }), + updateDoc(doc(rater, "users", RATER), { points: 999999 }), ); }); - test("points must be a non-negative integer", async () => { - await assertFails(updateDoc(doc(rater, "users", OWNER), { points: -1 })); - await assertFails(updateDoc(doc(rater, "users", OWNER), { points: 4.5 })); - await assertFails(updateDoc(doc(rater, "users", OWNER), { points: "9" })); + test("a user cannot write points onto anyone else either", async () => { + await assertFails(updateDoc(doc(rater, "users", OWNER), { points: 14 })); }); test("a stranger cannot rewrite another profile's department", async () => { @@ -184,6 +198,8 @@ describe("notes", () => { title: "Note", uploaderUid: RATER, department: "Bilgisayar Mühendisliği", + ratingSum: 0, + ratingCount: 0, }), ); await assertFails( @@ -191,6 +207,20 @@ describe("notes", () => { title: "Note", uploaderUid: OWNER, department: "Bilgisayar Mühendisliği", + ratingSum: 0, + ratingCount: 0, + }), + ); + }); + + test("a note cannot be born with a score", async () => { + await assertFails( + setDoc(doc(rater, "notes", "head-start"), { + title: "Note", + uploaderUid: RATER, + department: "Bilgisayar Mühendisliği", + ratingSum: 500, + ratingCount: 100, }), ); }); @@ -207,41 +237,65 @@ describe("notes", () => { ); }); - test("a rater may update only the three rating fields", async () => { - await assertSucceeds( - updateDoc(doc(rater, "notes", NOTE), { - ratingSum: 4, - ratingCount: 1, - avgRating: 4, + test("the uploader cannot raise their own note's score while editing it", async () => { + // The shortest route to the top of the leaderboard used to be editing your + // own note, since ownership alone authorised the whole document. + await assertFails( + updateDoc(doc(owner, "notes", NOTE), { + title: "Güncellenmiş başlık", + ratingSum: 500, }), ); }); - test("a rating update cannot carry a metadata change with it", async () => { - await assertFails( - updateDoc(doc(rater, "notes", NOTE), { - ratingSum: 4, - ratingCount: 1, - avgRating: 4, - title: "Ele geçirildi", - }), - ); + test("a vote and the totals it implies are accepted together", async () => { + await assertSucceeds(vote(rater, RATER, NOTE, 4, { sum: 4, count: 1 })); }); - test("rating totals cannot go negative", async () => { + test("totals cannot move without a vote to justify them", async () => { + // This is the whole point of the getAfter() check: the note update is only + // authorised by a rating document written in the same commit. await assertFails( updateDoc(doc(rater, "notes", NOTE), { - ratingSum: -1, - ratingCount: 1, - avgRating: 0, + ratingSum: 500, + ratingCount: 100, + avgRating: 5, }), ); + }); + + test("the totals must match the vote, not exceed it", async () => { + // Every value here stays inside the range checks, so the only thing that can + // reject them is the arithmetic: a 1-star vote claiming a sum of 5, and a + // 5-star vote claiming a second voter that does not exist. + await assertFails(vote(rater, RATER, NOTE, 1, { sum: 5, count: 1 })); + await assertFails(vote(rater, RATER, NOTE, 5, { sum: 5, count: 2 })); + await assertFails(vote(rater, RATER, NOTE, 3, { sum: 3, count: 0 })); + }); + + test("changing a vote moves the sum by the difference and leaves the count", async () => { + await assertSucceeds(vote(rater, RATER, NOTE, 2, { sum: 2, count: 1 })); + await assertSucceeds(vote(rater, RATER, NOTE, 5, { sum: 5, count: 1 })); + }); + + test("a changed vote cannot be counted twice", async () => { + await assertSucceeds(vote(rater, RATER, NOTE, 2, { sum: 2, count: 1 })); + await assertFails(vote(rater, RATER, NOTE, 5, { sum: 5, count: 2 })); + }); + + test("nobody can rate their own note", async () => { + await assertFails(vote(owner, OWNER, NOTE, 5, { sum: 5, count: 1 })); + }); + + test("a vote cannot be laundered through someone else's rating document", async () => { + // Writing the totals while pointing getAfter at a rating the caller does not + // own: the rating rule rejects the forged document, so the batch fails whole. + await assertFails(vote(rater, OWNER, NOTE, 5, { sum: 5, count: 1 })); + }); + + test("a rating update cannot carry a metadata change with it", async () => { await assertFails( - updateDoc(doc(rater, "notes", NOTE), { - ratingSum: 4, - ratingCount: -1, - avgRating: 4, - }), + vote(rater, RATER, NOTE, 4, { sum: 4, count: 1, extra: { title: "Ele geçirildi" } }), ); }); @@ -264,6 +318,8 @@ describe("note content", () => { title: "Note", uploaderUid: RATER, department: "Bilgisayar Mühendisliği", + ratingSum: 0, + ratingCount: 0, }); batch.set(doc(rater, "notes", "batched", "content", "file"), { fileData: "ZmFrZQ==", diff --git a/firestore.rules b/firestore.rules index 144ec2e..4266594 100644 --- a/firestore.rules +++ b/firestore.rules @@ -7,18 +7,11 @@ rules_version = '2'; // // firebase deploy --only firestore:rules // -// ÖNEMLİ — bilinen zayıflık: puanlama işlemi tamamen istemcide yapılıyor. -// RatingRepository bir oy verirken not sahibinin `points` alanını ve notun -// `ratingSum`/`ratingCount`/`avgRating` alanlarını doğrudan güncelliyor. -// Bu yüzden aşağıdaki kurallar, oturum açmış bir kullanıcının başka bir -// kullanıcının puanını ve bir notun ortalamasını değiştirmesine izin vermek -// zorunda. Değerler tür ve aralık olarak sınırlandırıldı ama kötü niyetli bir -// istemci hâlâ kendine puan yazabilir. -// -// Bunun doğru çözümü puanlamayı bir Cloud Function'a taşımaktır (istemci -// yalnızca oyunu yazar, toplamları sunucu hesaplar). Cloud Functions -// faturalandırmalı Blaze planı gerektirdiği için proje ücretsiz planda -// kaldığı sürece bu risk kabul edilmiş durumda. +// Puanlama hâlâ istemcide hesaplanıyor — Cloud Functions faturalandırmalı Blaze +// planı istiyor, proje ücretsiz planda. Ama "istemci hesaplıyor" ile "istemciye +// güveniliyor" aynı şey değil: notun yeni `ratingSum`/`ratingCount` değerleri +// aşağıda, aynı commit'te yazılan oy dokümanından yeniden hesaplanıp +// karşılaştırılıyor. İstemci sonucu üretiyor, sunucu doğruluyor. service cloud.firestore { match /databases/{database}/documents { @@ -44,14 +37,14 @@ service cloud.firestore { allow create: if isOwner(userId) && request.resource.data.id == userId; - allow update: if isOwner(userId) - // Puanlamanın not sahibinin puanını artırması için gereken izin. - // Yalnızca `points` alanı değişebilir ve negatif olamaz. - || ( - changedFields().hasOnly(['points']) - && request.resource.data.points is int - && request.resource.data.points >= 0 - ); + // 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ı: + // `points` burada saklandığı sürece kurallar hem sahibinin kendi puanını + // yazmasına hem de bir kullanıcının başkasının puanını değiştirmesine izin + // vermek zorundaydı. Puan artık saklanmıyor, notların `ratingSum` + // toplamından türetiliyor; dolayısıyla güncellemeye hiç gerek kalmadı. + // İleride profil düzenleme eklenirse kendi dar kuralını da getirmeli. + allow update: if false; // Hesap silme uygulamada yok; kaza ile veri kaybını engelle. allow delete: if false; @@ -60,21 +53,68 @@ service cloud.firestore { match /notes/{noteId} { allow read: if isSignedIn(); - // Not oluşturan kişi kendini başkası olarak gösteremez. + // Not oluşturan kişi kendini başkası olarak gösteremez ve puanı sıfırdan + // başlatmak zorunda: aksi halde nota doğrudan yüksek puanla doğulurdu. allow create: if isSignedIn() - && request.resource.data.uploaderUid == request.auth.uid; + && request.resource.data.uploaderUid == request.auth.uid + && request.resource.data.ratingSum == 0 + && request.resource.data.ratingCount == 0; allow update: if isSignedIn() && ( - // Sahibi metadata'yı düzenleyebilir. - resource.data.uploaderUid == request.auth.uid - // Oy veren yalnızca puan alanlarını değiştirebilir. - || ( - changedFields().hasOnly(['ratingSum', 'ratingCount', 'avgRating']) - && request.resource.data.ratingSum >= 0 - && request.resource.data.ratingCount >= 0 - ) + isMetadataEditByOwner() + || isRatingWrite(noteId) ); + // Sahibi metadata'yı düzenleyebilir ama puan alanlarına ve sahiplik + // bilgisine dokunamaz — kendi notunu öne çıkarmanın en kısa yolu buydu. + function isMetadataEditByOwner() { + return resource.data.uploaderUid == request.auth.uid + && !changedFields().hasAny([ + 'ratingSum', 'ratingCount', 'avgRating', 'uploaderUid' + ]); + } + + function ratingPath(noteId) { + return /databases/$(database)/documents/ratings/$(request.auth.uid + '_' + noteId); + } + + /** + * Oyu veren kişinin yazabileceği tek değişiklik. + * + * `getAfter` aynı commit'in *sonrasındaki* oy dokümanını okur; oy dokümanı + * yazılmadan notun toplamı değiştirilemez. Beklenen toplamlar + * RatingCalculator ile birebir aynı hesaplanıyor — tabanları dahil, çünkü + * istemci de aynı tabanları uyguluyor ve eşitlik tutmak zorunda. + * + * `math.max` kurallarda yok, o yüzden tabanlar üçlü işleçle yazıldı. + */ + function isRatingWrite(noteId) { + let path = ratingPath(noteId); + let votedBefore = exists(path); + let previous = votedBefore ? get(path).data.rating : 0; + let submitted = getAfter(path).data; + let rawSum = resource.data.ratingSum + submitted.rating - previous; + + return changedFields().hasOnly(['ratingSum', 'ratingCount', 'avgRating']) + // Kendi notuna oy vermek burada yasaklanıyor; eskiden yalnızca + // istemci kodunda kontrol ediliyordu. + && resource.data.uploaderUid != request.auth.uid + && submitted.noteId == noteId + && request.resource.data.ratingSum == (rawSum < 0 ? 0 : rawSum) + // Oy değiştirildiğinde sayı artmaz. + && request.resource.data.ratingCount == ( + votedBefore + ? (resource.data.ratingCount < 1 ? 1 : resource.data.ratingCount) + : resource.data.ratingCount + 1 + ) + // Ortalama türetilmiş bir gösterim alanı; asıl doğrulama toplam ve + // sayıda. Kurallarda tam sayı bölmesi yapıldığı için burada + // birebir eşitlik yerine aralık kontrolü var. + && request.resource.data.avgRating is number + && request.resource.data.avgRating >= 0 + && request.resource.data.avgRating <= 5; + } + // Uygulama kodu silme öncesi sahiplik kontrolü YAPMIYOR; tek gerçek // engel bu kural. (NoteRepository.deleteNote doğrudan siliyor, arayüz // sadece kendi notlarında silme düğmesini gösteriyor.) @@ -106,7 +146,12 @@ service cloud.firestore { && ratingId == request.auth.uid + '_' + request.resource.data.noteId && request.resource.data.rating is int && request.resource.data.rating >= 1 - && request.resource.data.rating <= 5; + && request.resource.data.rating <= 5 + // Kendi notuna oy veren bir kayıt hiç oluşmasın: + // 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; allow delete: if false; }