Skip to content

fix: close five security findings and correct the docs that no longer matched the code - #24

Merged
duuyguyiilmaz merged 6 commits into
mainfrom
chore/updates
Sep 1, 2026
Merged

fix: close five security findings and correct the docs that no longer matched the code#24
duuyguyiilmaz merged 6 commits into
mainfrom
chore/updates

Conversation

@duuyguyiilmaz

@duuyguyiilmaz duuyguyiilmaz commented Sep 1, 2026

Copy link
Copy Markdown
Owner

Six commits, five of them security fixes found by reviewing the Firestore rules against what the app actually does. Every rule change is deployed to the live project and verified on a device, not just in the emulator.

The average could be faked during a legitimate vote

The rules recomputed ratingSum and ratingCount from the rating document written in the same commit and rejected anything that disagreed. avgRating got only a range check, because rules do integer division and the exact comparison was not available to write — so a modified client could cast a real 1-star vote and set the displayed average to 5.0 in the same batch.

Answered by deletion rather than another check: an average is derived from two numbers the rules already pin exactly, so a third unverifiable copy bought nothing and cost a forgery. Screens show ratingSum, which the leaderboard already ranked on.

Note creation accepted almost anything

The rule checked uploaderUid and the two score fields and stopped. A client talking to Firestore directly could file a note into another department's feed, forge the uploader's identity, backdate createdAt to sit at the top of a feed that orders on it, or add fields nobody expected. Form validation in the app was never a boundary — nobody is obliged to use the app.

The shape is now fixed with hasOnly + hasAll and every field type- and size-checked, with identity taken from the session rather than the request.

Every read was if isSignedIn()

Any signed-in student could pull every profile, every note and every attached file in the university. Profiles are now readable only by their owner — all four getUser calls in the app pass the caller's own uid, so the open rule granted an access nobody used. Notes and their file subcollections are scoped to the caller's department, or to their own notes.

That second branch is not slack, and finding out why is the one thing that reached production: Firestore evaluates a list against the query, not the documents it would return, so the contribution gate's where uploaderUid == me — carrying no department filter — was refused outright and the feed opened empty. ebd347a adds the branch and the two tests that would have caught it first.

The uploader's address was stored on every note

Three screens rendered email.substringBefore("@"), which kept the address off the screen but not out of the document. Notes now carry uploaderName; the rule requires it to equal the local part of the caller's own token email, and writing an uploaderEmail is refused. strip-uploader-email.js converts existing notes.

Anyone could attach a file to anyone's note

allow create: if isSignedIn() on the content subcollection, documented as unavoidable because a batch cannot get() its own uncommitted parent note. True of get(), false as a conclusion — getAfter() reads the post-commit state, and the rating rule was already using it. Now owner-only, with the document pinned to a single fileData string.

Deleting a note left its votes behind

ratings is a top-level collection, so deleteNote never touched them and the documents piled up unreachable. existsAfter() lets a vote be deleted only in the commit that removes its note, and only by the owner — a vote deleted on its own is still refused, because that would hand the voter a fresh first-time vote and let them pad the score. prune-orphan-ratings.js clears the ones written before this.

Documentation and build

app/README.md deleted — the original course submission, describing a report feature that was never built and fields since removed, contradicting the root README throughout. The architecture claim that "Firebase types never reach the ui package" is corrected: two places cross that line deliberately, and the README now names them. KSP removed — applied in both build files, used by no dependency. Three limitations written down: nothing runs on a device in CI, no test executes a real Firestore query, and a green rules suite is not a deployed rule.

Tests

Suite Before After
rules.test.js 38 65
firestore-tests total 46 85
JVM 129 126

The JVM count drops because removing avgRating removed the assertions that existed only to check it. Three migration scripts now, each with its own emulator-backed test, each a dry run unless --apply is passed.

Verified on a device

Against the live project with these rules deployed: the feed and leaderboard load, a note uploads with uploaderName and its file in one batch, deleting a note from the profile succeeds, and the logcat carries no PERMISSION_DENIED or FAILED_PRECONDITION.

🤖 Generated with Claude Code

duuyguyiilmaz and others added 6 commits September 1, 2026 18:30
@duuyguyiilmaz @claude
fix: stop storing an average the rules cannot verify, and pin note cr…
63c77f1 
…eation

Two findings, and one of them is answered by deletion rather than by another check.

**The average could be faked during a legitimate vote.** The rules recompute
`ratingSum` and `ratingCount` from the rating document written in the same
commit and reject anything that disagrees. `avgRating` got no such treatment —
only "is a number, is between 0 and 5" — because rules do integer division, so
the exact comparison was not available to write. That gap let a modified client
cast a real 1-star vote and set the displayed average to 5.0 in the same batch.

The fix is to stop storing it. An average is derived from two numbers the rules
already pin exactly, so keeping a third, unverifiable copy of that information
bought nothing and cost a forgery. Screens now show `ratingSum` — the running
total, which is what the leaderboard already ranked on. `RatingCalculator` no
longer computes an average, `RatingResult` no longer carries one, and the note
detail view's `tvAvgRating` is now `tvRatingSum`, because a name that says
"average" over a total is the next bug. Old documents keep the field; nothing
reads it, and the create rule refuses to write it again.

**Note creation accepted almost anything.** The rule checked `uploaderUid` and
the two score fields and stopped. A client talking to Firestore directly — which
is the only client that matters here, since nobody is obliged to use the Android
app — could file a note into another department's feed, put someone else's
address in `uploaderEmail`, backdate `createdAt` to sit at the top of a feed that
orders on it, or add fields nobody expected. The form validation in the app was
never a security boundary; it only ever described what the app itself sends.

The shape is now fixed with `hasOnly` + `hasAll`, so a missing field is refused
as firmly as an invented one, and each field is checked for type and size.
Identity comes from the session instead of the request: `uploaderUid` is the
caller's uid, `uploaderEmail` is the token's email, `createdAt` is
`request.time`, and `department` must equal the department on the caller's own
profile. Editing a note can no longer move any of those either.

One behaviour change worth stating: a profile whose department is blank can no
longer upload at all, where it used to write into a "Bilinmiyor" bucket.
Registration cannot complete without choosing a department, so this only reaches
corrupted data — and no feed ever showed those notes.

Tests: rules 38 -> 47, one refusal test per check, so relaxing any single check
turns exactly one test red. 55 green in the rules suite including the backfill,
128 green on the JVM side, lint clean.

Verified against the live project with the new rules deployed: a note uploads,
appears in the feed, and the logcat carries no PERMISSION_DENIED.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@duuyguyiilmaz @claude
fix: scope reads to the caller and stop storing uploaders' addresses
9881d4c 
Every read rule was `if isSignedIn()`. That made three things true at once: any
signed-in student could pull every profile in the university, every note in every
department, and every attached file. The app only ever asked for its own
department, but that is a property of the app, and the app is not the only client
anyone can write.

**Profiles are now readable only by their owner.** The old comment claimed the
leaderboard and note detail needed to read other people's profiles. They do not —
all four `getUser` calls in the app pass the caller's own uid, and those screens
take the uploader's name from the note itself. The open rule was granting an
access nobody had ever used, and what it granted was every student's address and
department in a single query.

**Notes and their file subcollections are scoped to the caller's department.**
Rules are evaluated against each document a query returns and the whole query
fails if any is denied, so the feed's `where department == …` filter stops being
a courtesy and becomes the condition for the query to run at all. The file
subcollection is checked against its parent note's department, or the boundary
could be walked around through the file path while the note stayed unreadable.

**The uploader's email is gone from the note.** Three screens rendered
`email.substringBefore("@")`, which kept the address off the screen but not out
of the document — masking at render time is a rendering choice, not a privacy
boundary, and every student in the department could read the raw field. Notes now
carry `uploaderName`, the rule requires it to equal the local part of the
caller's own token email, and writing `uploaderEmail` is refused outright.
`Post` and `LeaderboardEntry` no longer hold an address to leak, so the masking
is structural rather than a getter someone can route around.

`strip-uploader-email.js` converts existing notes, following the same contract as
the createdAt backfill: dry run by default, `--apply` to write, safe to re-run.
It sets the name and deletes the address in one update so no note is left
carrying both, and `toPost()` still derives the name from a legacy
`uploaderEmail` when it finds one, so the app works before and after the run.

Tests: rules 47 -> 55, adding the reads that must fail — another department's
note, an unfiltered collection query, a query filtered to someone else's
department, another department's file, someone else's profile — plus the refusal
to write an address onto a note. 70 green in the firestore-tests suite with the
new migration's own 9, 126 green on the JVM side, lint clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@duuyguyiilmaz @claude
fix: let the department read rule cover the app's own-notes query
ebd347a 
The previous commit scoped note reads to the caller's department and nothing
else. Deployed against the live project, the feed opened empty: the contribution
gate and the profile screen both query `where uploaderUid == me`, carrying no
department filter, and the rule rejected them.

The reason is a property of Firestore I had wrong. Rules on a list operation are
evaluated against the *query*, not against the documents it would return —
Firestore will not run a query unless its filters guarantee the rule holds. That
these particular notes were the caller's own, in the caller's own department, is
not something it works out. So the rule needs a branch matching each query the
app actually builds, and the second branch — `uploaderUid == request.auth.uid` —
is what the profile and the gate depend on. It is not slack: a query with neither
filter still satisfies no branch and is still refused.

Two tests now cover it, and they are the ones that would have caught this before
the deploy: the own-notes query succeeds without a department filter, and the
same query aimed at another uid fails. The cross-department fixture had to change
too — it was written as the reader's own note, so the new branch let it through,
which the suite caught immediately.

Verified on the device with the corrected rules deployed: the feed lists the
department's notes again, the profile shows its own, and an upload lands with
`uploaderName` and no PERMISSION_DENIED anywhere in the log.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@duuyguyiilmaz @claude
fix: bind content creation to the note's owner
3637651 
`allow create: if isSignedIn()` on notes/{id}/content let any signed-in user hang
a content document off anybody's note. Overwriting an existing file was already
owner-only, so the hole was narrow but real: a note that had no file yet would
take one from a stranger, and the file document is what the detail screen
downloads and shows.

The rules documented this as unavoidable — a batch cannot `get()` its own
uncommitted parent note, so ownership could not be checked at create time. The
first half is true and the conclusion is not. `getAfter()` reads the state the
commit will leave behind, which is exactly what the rating rule already uses to
tie a note's totals to the vote written beside it. The same call reads the parent
note's `uploaderUid` from inside the upload batch.

Create is now owner-only and the document shape is pinned to a single `fileData`
string, so a content document cannot carry fields nobody expects either. Because
`getAfter()` returns the current note when the write does not touch it, the rule
also covers attaching a file to an existing note of your own, which is the other
path the app can take.

The test that asserted the gap is replaced by four that assert the closure: a
stranger cannot attach content, the owner can — both to a batched note and to an
existing one — a document carrying anything but `fileData` is refused, and a
batch pairing a legitimate note of the caller's own with a file smuggled onto
someone else's fails as a whole. Rules suite 57 -> 61, 75 green overall.

Deployed and verified on the device: a note-plus-file upload still commits, and
the log carries no PERMISSION_DENIED.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@duuyguyiilmaz @claude
fix: delete a note's votes with the note
55da2ec 
`ratings` is a top-level collection keyed `<uid>_<noteId>`, not a subcollection of
the note, so `deleteNote` — which removed the note and its file — never touched
the votes cast on it. They stayed forever, invisible to every screen, holding
document ids nobody could reuse.

The delete rule made it worse by being airtight. `allow delete: if false` was
right on its own terms: removing a vote does not lower the note's total, but it
does hand the voter a fresh first-time vote, which the rule counts anew and the
tally climbs. Free deletion was a way to pad a score. But it also meant no client
could ever clean up after itself.

`existsAfter()` ties the two together. A vote may be deleted only when the same
commit removes its note, and only by that note's owner — which is precisely the
write `deleteNote` now issues: query the votes for the note, then delete them,
the file and the note in one batch. A vote deletion sent on its own is still
refused, because the note is still there afterwards.

The batch caps this at Firestore's 500 writes, so a note with more than ~498
votes cannot be deleted this way. Not a number a department feed reaches, and it
fails loudly rather than half-deleting.

Votes orphaned before this change stay out of reach — the rule calls `get()` on a
note that no longer exists, which is an evaluation error and fails closed — so
`prune-orphan-ratings.js` covers them, following the same contract as the other
two migrations. It compares the two collections in memory rather than issuing a
`get()` per vote, and leaves a rating with no `noteId` alone rather than deleting
on a guess.

Rules suite 61 -> 65, plus the script's own 6: a vote cannot go alone, not even
for the owner; the owner may remove it with the note; a stranger cannot reach it
by deleting a note they do not own; and an already-orphaned vote stays refused.
85 green in firestore-tests, 126 on the JVM side, lint clean.

Deployed and verified on the device: deleting a note from the profile succeeds
with no PERMISSION_DENIED.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@duuyguyiilmaz @claude
docs: correct the claims the code does not support, drop what nothing…
b048d53 
… uses

Six things a reviewer flagged, five of which were the documentation describing a
project that no longer exists.

**`app/README.md` is deleted.** It was the original course submission: a report
mechanism that was never built, a `points` field on users and an `avgRating` on
notes that have both since been removed, and a `Reports` collection that never
existed in Firestore. It contradicted the root README on nearly every point. Two
READMEs where one is stale is worse than one, and nothing linked to it.

**The architecture claim was overstated.** "Firebase types stay behind the
repositories and never reach the `ui` package" is not true twice over: every
ViewModel defaults its constructor to a concrete `FirebaseXRepository`, and
`AuthErrorMessages` matches on `FirebaseAuthException.errorCode` — deliberately,
since that code is the only stable description of an auth failure. The separation
is real and worth having; it is just not absolute, and the README now says which
two places cross the line and why, rather than claiming a purity the code does
not have.

**KSP is removed.** The plugin was applied in both build files and pinned in the
version catalog, and no `ksp(...)` dependency existed anywhere — an annotation
processor configured to process nothing, costing a plugin resolution on every
build and implying a code-generation step a reader would go looking for.

**Three limitations are now written down** rather than left for a reviewer to
discover: nothing runs on a device in CI, no test executes a real Firestore query
(the fakes stop at the repository interfaces — the rules suite is the other half
of that gap), and a green rules suite says the file is correct, not that the
Firebase project is running it. The last one is the sharpest: the repo cannot
detect the drift, so deploying is part of merging a rules change.

**`.claude/` is ignored.** Per-machine tool permissions, not project config.

Not changed: the point that repository tests use fakes was already documented
under Tests, and it was accurate.

Build, 126 JVM tests and lint all green without the KSP plugin.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@duuyguyiilmaz
duuyguyiilmaz merged commit 547cdab into main Sep 1, 2026
2 checks passed
@duuyguyiilmaz
duuyguyiilmaz deleted the chore/updates branch September 1, 2026 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@duuyguyiilmaz