Repository navigation
fix: stop the client from awarding itself points - #19
Merged
Merged
Conversation
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 <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.
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, sopoints: 999999was a one-line request.ratingSumto any non-negative value — the number both the leaderboard and the profile total are read from.The fix turned out to be mostly deletion
users.pointsis never read.ProfileViewModelalready computes the total asposts.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 —saveUseris called once, during registration, and never again.What could not be deleted is 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, recomputeratingSumandratingCountfrom it, and reject anything that does not match.So now:
The expected totals are
RatingCalculator's arithmetic restated, floors included, because client and rule must agree exactly.avgRatingis range-checked rather than compared: rules do integer division, and it is a derived display field whose inputs are pinned.math.maxdoes not exist in rules, hence the ternaries.A test that was passing for the wrong reason
Six new rule tests — and one existing test got sharper for a reason worth recording.
Weakening the
ratingSumcheck to>= 0failed to turn anything red. The old assertion used a sum of 500, which was already rejected by theavgRatingrange check, so it passed without ever exercising the arithmetic. Rewritten with values that sit inside every range check, it fails on the arithmetic alone.Re-running the same mutation now turns three tests red: that one, and the two covering the user document.
Verification
Firestore suite: 38 rules tests, 46 total. Android suite: 107.
assembleDebugandlintDebugpass.🤖 Generated with Claude Code