Don't store voter emails/IPs on rolls for elections that don't authenticate - #1477
Don't store voter emails/IPs on rolls for elections that don't authenticate#1477jacksonloper wants to merge 2 commits into
Conversation
…ticate
getOrCreateElectionRoll computes `email` and `require_ip_hash` from the
election's voter_authentication settings, correctly leaving them null when
the election doesn't authenticate on them. But the roll it builds ignored
those and copied `req.user.email` and `hashString(req.ip)` in unconditionally.
The guard that decides whether to persist the roll does consult the settings,
so this looked safe. It isn't: the cast-vote path reaches the "not persisted"
branch and hands the roll to CastVoteStore.submitBallotEvent, which inserts
whatever it's given. So an open_open election — voter_access 'open',
voter_authentication {} , i.e. the mode that authenticates on nothing — still
wrote the signed-in voter's email address and an IP hash next to their
ballot_id.
Use the already-computed values instead. ElectionRoll.ip_hash is documented as
"set when voter_authentication.ip_address is enabled"; now it is.
Receipt emails are unaffected: castVoteController falls back to
extractUserFromRequest(req)?.email when the roll has no email.
The mock CastVoteStore only called rollStore.update(), which no-ops when
there's no head row to update — exactly the case here — so the whole class of
bug was invisible to the suite. It now inserts when there's no head row, which
is what the real store does.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
✅ Deploy Preview for bettervoting ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change limits new election rolls to configured authentication fields. Cast-vote persistence now updates an existing roll or creates a new roll when needed. Integration tests verify roll contents for unauthenticated, email-authenticated, and IP-authenticated open elections. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The name read as a boolean predicate, but the value is `string | null` — the hash to authenticate against, or null when the election doesn't authenticate on IP. It's passed straight into getElectionRoll's `ip_hash: string | null` parameter. `email` on the next line already follows the value-or-null convention; this now matches. No behavior change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The bug
getOrCreateElectionRollderives two locals from the election'svoter_authenticationsettings, correctly leaving themnullwhen the election doesn't authenticate on them:The roll it then builds ignored both and copied the raw request values in:
The guard that decides whether to persist the roll does consult the settings, which is why this looked safe. It isn't. The cast-vote path reaches the "not persisted"
elsebranch and returns the roll anyway, andCastVoteStore.submitBallotEventinserts whatever roll it is handed:So on the voting path that guard is effectively dead code — it only protects a branch nothing reaches.
Net effect: an
open_openelection —voter_access: 'open'+voter_authentication: {}, the mode the UI presents as requiring no authentication at all — stores the voter's email address and an IP hash next to theirballot_id. The email lands whenever the voter happens to have a session cookie, which they often do; the IP hash lands always, session or not. Neither is anything the election asked for.ElectionRoll.ip_hashis already documented in the domain model as "sha256(req.ip); set whenvoter_authentication.ip_addressis enabled." After this change that's actually true.Scope on prod
Querying
electionRollDBjoined against head elections withvoter_authentication = '{}'andvoter_access = 'open':ip_hashstoredEvery one of those rows has both
ballot_idset andsubmitted = true— i.e. all of them came throughcastVote, none from an admin adding voters. Still happening; most recent was yesterday.Not proposing a data migration here — happy to do that separately.
How exposed is it, really?
Lower than it first looks, and worth stating plainly so this gets prioritised correctly:
GET /Election/:id/rollsrefuses outright forvoter_access: 'open'elections, owner included — so admins can't list this today.GET /Election/:id/rolls/:voter_idscrubsballot_idandip_hash, but notemail, and has noexpectPermissioncall. Voter IDs aren't enumerable, and the party most likely to hold one is the voter themselves — but a forwarded ballot-update link (/:election_id/id/:voter_id, which receipt emails contain whenballot_updatesis on) would disclose that voter's email. I've left that alone here; it's a separate fix.So the practical case for this PR is data minimisation rather than an open door: the rows shouldn't exist, they're in every backup, and the only thing standing between them and disclosure is one
ifin one controller that any future endpoint or CSV export has to remember to reproduce.What's in here
voterRollUtils.ts— use the already-computedemail/require_ip_hashinstead of the raw request values. Also corrected the comment on theelsebranch, which claimed nothing reaches it.__mocks__/CastVoteStore.ts— the mock only calledrollStore.update(), which no-ops when there's no head row to update. That's exactly theopen_opencase, so the mock silently dropped every roll the cast-vote path writes and this entire class of bug was invisible to the suite. It now inserts when there's no head row, matching the real store.openElectionRollPrivacy.test.ts—open_openstores neither field;{email: true}still stores the email;{ip_address: true}still stores the hash and nothing else. Verified these fail without thevoterRollUtilschange.The test asserts against the roll store rather than
GET /rolls, since that endpoint won't list open elections — which is a decent part of why nobody noticed. The row was being written; nothing was reading it back.Not affected
Receipt emails.
castVoteControlleralready falls back throughevent.roll?.email ?? extractUserFromRequest(req)?.email ?? req.body.receiptEmail, so a signed-in voter on anopen_openelection still gets their receipt from the second link in that chain.Full backend suite: 158/158.
🤖 Generated with Claude Code