Repository navigation
fix: close the remaining rule gaps, including the one the tests were endorsing - #25
Merged
Merged
Conversation
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three commits. The first is the sharpest finding of the review so far, and the one CI was actively wrong about.
A vote could be written without the score that follows from it
The link between a vote and a note's total was enforced in one direction only. The note's rule said "if the totals move, show me the vote that justifies them." Nothing said the reverse. So a rating document could be written entirely on its own — and the test suite asserted that as correct behaviour, which is why the suite stayed green over it. The suite described the hole instead of catching it.
It is unbounded score inflation from a single voter, four points a lap:
sum += 4, count unchanged, because the rule correctly sees a changed voteStep 1 is what was permitted. The ratings rule now mirrors the note's arithmetic from its own side and requires the note to actually carry the numbers.
Verified by mutation, not by a green run: deleting the single new clause turns exactly three tests red and leaves the other 86 green. Several existing ratings tests were rewritten too — they wrote a lone rating document and passed for the wrong reason, the range check among them.
Two further tests cover the mechanism the app actually ships:
submitRatingusesrunTransaction, notwriteBatch, and nothing had exercised that.The university restriction was never enforced on the server
The profile create rule checked that you were writing your own document and that its
idfield matched. Nothing else — so@ogr.akdeniz.edu.trlived only inRegisterActivity. Firebase Auth accepts any address; signing up with a personal one and writing the profile directly was enough to become a student. Since reads are scoped by department andownDepartment()reads it from this very document, that forged profile was read access to a department's notes.The rule now pins the field set, takes the address from
request.auth.token.emailrather than the request body, checks the domain itself, requires a non-empty department, and requirescreatedAtto berequest.time.A bug this introduced, caught by running the app rather than by the suite:
getTimestamp()throws rather than returning null when the field holds a number, so reading any profile written before the change crashed the profile screen.toPost()already had the guard and it was not reused. Now both carry it, each pointing at the other. This is the gap the README documents — no test executes repository code.Edits were a blacklist; votes were world-readable
existsAfter()andget()on the note; counted per rating rather than per path, a note with a dozen votes would have become undeletable. A test now deletes one with twelve. It passes, so the calls are cached per path.Documentation
RatingCalculatorTestno longer claims to check "average recalculation" — there has been no average since the field was removed — and Data migrations no longer says the feed downloads every note.Continuous integrationwas sitting under Data migrations as though CI were a migration; it is its own section now, and it 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.Tests
rules.test.jsVerified
Rules deployed to the live project after each commit. On the device: registration passes the new profile rule end to end, and the feed, upload and note-plus-file batch still work. Not re-verified on the device: note editing and deletion —
updateNotewrites exactly the five whitelisted fields with a server timestamp, checked by reading both sides, and the emulator covers the positive case.🤖 Generated with Claude Code