fix(security): close every open finding in the security handoff - #3
Open
DaChelimo wants to merge 17 commits into
Open
fix(security): close every open finding in the security handoff#3DaChelimo wants to merge 17 commits into
DaChelimo wants to merge 17 commits into
Conversation
Web-claim: payer flow, marketing site, and auth hardening
Web claim surface + marketing site fixes
Both platforms require their unmodified badge for store-download links (exact wordmark, colors, and clear space) rather than a hand-built icon+text button. Swap in Apple's and Google's own badge assets, with a variant prop so the correct Apple badge (black/white) is used on the light hero vs. the dark closing band. Green on web tests + typecheck. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…download `extract-receipt` split `storagePath` on the first slash and used the left half as the bucket, then downloaded with the service role. The caller named the bucket, so any private bucket in the project was one request away from being read. Receipts are photographs of people's lives; the day a private bucket exists this is the read primitive for it. The bucket is now hardcoded to `receipts`. A leading `receipts/` is tolerated and stripped so older clients keep working, `..` segments are rejected, and the request-shape comment at the top no longer documents the bucket-prefixed form as legitimate input. Finding #18 in SECURITY_FIX_HANDOFF.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… at scale Two findings that are the same story: #22 is why sync breaks, #25 is why nobody could see it. #25 — `runCatchingSync` caught `Exception` and mapped everything to `Network.Unreachable`. An RLS denial, a decode drift, a wedged unique index and a dead radio were indistinguishable, and `push()` had a second hardcoded copy of the same mapping. Both now go through `classifySyncError`: RestException 401/403 to SessionExpired/NotAuthorized and anything else to `AppError.Backend(status, code)`, decode failures to Backend, and `Network.Unreachable` reserved for genuine transport failure. The `CancellationException` rethrow is untouched. `SyncEngine.health` is a new `StateFlow<SyncHealth>` (consecutive-failure count + last error kind) so a permanently-failing sync is observable. `SyncManager` swallows results on purpose, which is what made this silent. #22 — `selectIn` sent one `id=in.(…)` carrying every expense id. Past the gateway's URL limit every pull failed, for every table, permanently, and thanks to #25 the user was told they were offline. It lands first on the most engaged group. Now chunked at 100 ids. The loop lives in the companion rather than the inline body: `pull()` inlines `selectIn` at ~25 call sites and was already close to the JVM's 64KB method ceiling, which the first inline version blew. Green on Android + iOS; tests pass (13 in SyncEngineTest, 6 new). Findings #22 and #25 in SECURITY_FIX_HANDOFF.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…res must sum to it Two money invariants that were quietly false, plus the guards that stop them coming back. #21 — `validate()` took only (title, lines), so `extras` never reached it and the `total()` helper that subtracts the discount was never called from it. An oversized discount produced a zero or negative expense, breaking the always-positive entity invariant and then hiding behind the `> 0` outstanding filters, so the bill simply vanished from the balances instead of failing. `validate` now takes `extras` and rejects a non-positive total with a field error on `discount`; both call sites (createBill, editBill) pass it. #20 — under EVEN tip, `tipShares` allocated across `participants` but the breakdown was built from `subtotals`, so a participant with no item subtotal got a slice that was computed and then discarded. Two cases were tangled there and only one is a bug: someone who has NOT YET CLAIMED should keep today's behaviour (their tip rides in with their first claim, and the UNCLAIMED_BUCKET keeps everyone else stable), while someone on a FULLY CLAIMED bill who took no items genuinely owes the tip. Only the second now gets a breakdown row. Ported to web/src/lib/money/billSplit.ts in the same commit, per WEB_CLAIM_SPEC.md §7. The guards (handoff §5), because a markdown checkbox decays and a vector does not: both runners now assert, over EVERY splitBill vector, that a fully-claimed bill's total is positive and that its shares sum to it. A new vector pins #20 directly — $80 of items and a $9 EVEN tip across three participants where one claimed nothing; her $3 used to be dropped. Also corrected this test's own KDoc: recording is EVENLY_VECTORS_RECORD=true, not -D. Only the env var reaches the test JVM, and the -D form fails with "empty `expect`", which reads like a bad case rather than a flag that never arrived. Green on Android + iOS; tests pass (26 Kotlin vectors, 44 TS assertions). Findings #20 and #21 in SECURITY_FIX_HANDOFF.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nder account B `signOut()` was four lines that never touched Room: capture an event, reset analytics, sign out of Supabase, null the user id. Sign out, sign in as someone else on the same phone, and the next push sent account A's rows up under account B's session. A cross-account data leak in a money app. Evenly Pro widened it, since `group_passes` and `user_subscriptions` are per-user and rode along too. The wipe itself is `SignOutWipeDao`, one transaction, every table deleted explicitly. Explicitly, because `RoomDatabase.clearAllTables()` resolves on Android/JVM and fails on Kotlin/Native (AGENTS.md §4.1) — so the test is an iOS test, where an Android-only one would have passed against code that cannot ship. Sync bookkeeping goes with the rows it describes: a surviving fingerprint would make the next account's freshly-pulled rows look already-pushed and they would never sync at all. FX reference data stays; it carries nothing about anyone. Unsynced local writes are real user data (data/AGENTS.md Rule 1), so nothing is wiped until they are safe. Sign-out pushes first; only if that push fails AND `SyncEngine.countPendingLocalWrites()` is non-zero does it return `SignOutOutcome.UnsyncedChanges` having done nothing at all — still signed in, cache untouched, so Cancel is a true no-op. That count uses the same fingerprint comparison `pushDirty` uses, so "clean" means precisely "push would send nothing". Deliberately NOT shared with `requestAccountDeletion`: a deletion is cancellable inside its grace period, so its local state must survive, and its failure branch must wipe nothing. Also folds in the `SecureStorage.clear()` that `data/AGENTS.md` already required and nothing called — the web bill-link plaintext tokens are bearer credentials for one bill each and were outliving the session that minted them. UI: sign-out is no longer instant, so the row shows "Saving your changes" while the push is in flight, and `SignOutFlow` carries the confirmation both call sites share rather than letting them drift. Copy went through a cold ux-firsttimer walk as Sam, which took out "couldn't reach the server" (our vocabulary, not his) and made the retry concrete: "Get back online, then tap Sign out again to save them first." Verified on the iOS simulator end to end: signed out of a populated test account, no dialog (online, nothing pending), and every user table in evenly.db read 0 afterwards. Green on Android + iOS; tests pass. Finding #24 in SECURITY_FIX_HANDOFF.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…locks Two schema defects that both come from trusting the client payload too far. #16 — both expense RPCs inserted via `jsonb_populate_record(null::public.expenses, p_expense)`. That base row is what supplies any key the payload OMITS, so every column an older client didn't send became an explicit NULL and the column default never ran. Every defaulted column on `expenses` is `not null default X`, so this is not a quiet wrong number: it is a not-null violation that fails the INSERT and takes that expense's sync down entirely. It breaks the additive-migration promise at precisely the moment it's relied on: add a column server-side first, as the rule requires, and every not-yet-updated client stops being able to create expenses. (Before #25 that surfaced to the user as "you're offline".) Fixed by passing a real base row: `_expense_defaults()` and `_share_defaults()` spell the defaults out by name. `shares` had the identical bug one line away in `_replace_expense_shares` (`row_version` is `not null default 1`), so it is fixed here too rather than left half-closed. The maintenance rule this creates is now in supabase/AGENTS.md. #19 — `v_now` and every incoming `*_updated_at` came straight off the payload, and neither RPC authenticates `p_actor`. A device with a wound-forward clock, or a crafted payload, stamping `title_updated_at = 2099` won `greatest(...)` FOREVER: that field silently stopped accepting any later edit from anybody, with nothing on screen to explain it. All of them now go through `_clamp_client_ts()` = `min(value, server_now + 60s)`, in both RPCs, including the tombstone stamp. The 60s of slack absorbs ordinary device skew; clamping to exactly `now()` would make a marginally-fast phone lose its own writes. Verified against a real Postgres 18, not by reading: schema.sql applies clean to a scratch DB; a payload missing every defaulted column now inserts with the defaults intact where the old form fails with `null value in column "kind"`; and a payload clocked at year 2100 is clamped, then a normal edit takes the field back 65 seconds later instead of never. NOT YET APPLIED to the live project (ref wfpfgbipjmkysalfmyub) — these are `create or replace function` only, no table changes, but applying them is the owner's call. Findings #16 and #19 in SECURITY_FIX_HANDOFF.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`editSettlement` voided the old payment and re-recorded the new one across two separate transactions. A crash in between left the payment voided with no replacement: money the debtor had actually paid silently became owed again, on every device, with nothing to say why. `SettlementDao.replaceSettlement` is now one `@Transaction` doing both. The order inside it is void-then-record and must not be flipped: voiding first is what frees the old allocations so the over-apply guard sees the ceiling the caller validated against, and record-then-void would look safer on a crash but is worse — both payments live, the expense reading as double-paid, which is the harder error to spot and the one that stops someone chasing a debt they are still owed. The guard refuses by throwing, because Room only rolls a `@Transaction` back on a throw and a bare `false` would have left the void committed — the exact bug this commit is fixing. `SettlementReplaceRefused` is internal to the DAO layer and the repository turns it back into the same over-allocation validation error. Because the two halves are now atomic, the new allocation can no longer be computed by voiding first and re-reading. It is computed against the state the void WILL produce: `outstandingForPair` drops fully-paid shares (`remaining_subunits > 0`), so the shares this payment currently covers are added back explicitly. Leaving them out would have silently re-allocated the correction onto the wrong lines. Two new DAO tests, on Kotlin/Native: a correction lands as one transaction with the old payment soft-deleted rather than removed (Rule 1), and a refused replacement rolls the void back so the original payment survives intact. Existing SettlementRepositoryTest cases pass unchanged. Green on Android + iOS; tests pass. Finding #17 in SECURITY_FIX_HANDOFF.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ngs that don't exist #28 — `computeExpenseStatus` has no definition anywhere in the tree, yet `ExpenseEntity` and `Expense` both pointed at it as the owner of `status`. Both now say what is actually true: `status` holds ACTIVE or DELETED only, and settlement state is derived on read from the non-voided allocations with nothing stored to go stale. `ExpenseDao.updateStatus` had zero callers (the `updateStatus` hits in `ReceiptUploadManager` are `ReceiptUploadDao`'s, a different method), so it is deleted rather than left as a loaded gun pointing at an invariant. Also corrected `domain/AGENTS.md`, which claimed a person is in at most one shared slice per line "enforced by the active `(item_id,user_id)` unique index". Both the schema and `ItemShareEntity` key on `(item_id, user_id, portion_id)` and deliberately allow several slices — solo on one serving, sharing another. The sentence described an index that does not exist, which is worse than no comment. #29 — added `drop index if exists item_shares_item_user_active_uidx;` before the current index. A from-scratch apply is already clean (the old name is gone from this file entirely), so this only matters to a legacy environment reapplying it, where the stale per-item index would still be live and would reject exactly the per-serving assignment the new one exists to allow. Verified schema.sql applies clean to a scratch Postgres 18 and is idempotent on reapply. Green on Android + iOS; tests pass. Findings #28 and #29 in SECURITY_FIX_HANDOFF.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…losed Ticks #16-#22, #24-#26 and #28/#29 now that each is implemented and verified, per this file's own rule that a checkbox means fixed rather than triaged. Two entries needed more than a tick: #27 is marked closed as WRONG, with the reason inline. It asks for `users` to be added to the `supabase_realtime` publication, which AGENTS.md §4.3 and schema.sql both forbid; doing it once fanned out one message per row per connected client and burned 13.9M messages against a 5M quota. Left as a bare unticked box it reads as outstanding work, and the next agent rediscovers it as actionable. #23 stays open but its two halves are now separated: `observeEditConflicts` returning empty is deliberate, documented and asserted by a test, so it must not be "fixed"; the four stranded live rows need DB access this session did not take, and removing the dead table from sync is deferred at the owner's request until the concurrent RowSyncState work settles. The header records what is not yet applied to the live project, and notes that #20 and #21 are now enforced by the money-vector CI gate rather than by a checkbox. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Applying #16/#19 to the live project surfaced a hole in my own fix, found by dry- running the migration against a scratch Postgres rather than trusting it. **The clamp did not cover expense CREATION.** `_clamp_client_ts` was applied at the points where timestamps are COMPARED, but the `not found` branch of both RPCs inserts via `jsonb_populate_record`, which copies `title_updated_at` and friends straight out of the payload. A poisoned stamp on a brand-new expense sailed straight past and the field was locked from birth — the whole defect #19 describes, just one branch over. The dry run printed `ts_clamped = f` and gave it away. Fixed properly: `_clamp_expense_payload()` sanitises the jsonb ONCE, and both RPCs now read `v_exp` and never `p_expense`, so create and merge cannot diverge again. It only rewrites keys that are present, so it does not resurrect the NULL-over-default bug `_expense_defaults()` exists to fix. Also pinned `search_path = ''` on the four new helpers. They are pure and fully schema-qualified, so it costs nothing, and a fix should not leave the security advisor's baseline worse than it found it. **Applied live** (ref wfpfgbipjmkysalfmyub) in four migrations. Verified on the live DB inside a rolled-back transaction: a payload missing every defaulted column inserts with defaults intact, and a year-2100 stamp comes back clamped, on both paths. No test rows left behind. **Finding #23 closed.** All four stranded `expense_edit_conflicts` rows were exported in full, sha256-matched against the live rows to prove the archive is faithful, judged individually, and only then marked `KEEP_SERVER` — marked, never deleted (Rule 1). Two were seeded demo fixtures. One was a genuine rejected split edit (an even 25%-each that lost to 40/25/25/10); one was a self-supersede with an empty share set. All four sit in `@sharecost.test` groups, so no real money was involved. The record and the reasoning are in `supabase/exports/`, because "we discarded someone's edit" needs to be answerable later. Correcting something I said earlier in this session: I first read rows 3 and 4 as materially identical to canonical. That was true of the expense row and NOT of the shares — row 3 was a real split change. The share-level comparison is what caught it. The verdict is unchanged; the reasoning behind it is not. Schema applies clean and idempotently to a scratch Postgres 18. Green on Android + iOS; tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…feedback work Wraps up the fix/security-handoff branch: server clock plumbing, sync-gate and payment-handle validation, feedback outbox, Pro pass polish, and doc/test updates across shared, admin, and web. Green on Android + iOS. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…(true)` All 20 app tables carried `for all to authenticated using (true) with check (true)`, so any signed-in account could read, overwrite, or delete every other group's ledger. They are now scoped to ACTIVE members of the row's group. - `is_group_member(group_id)` / `can_access_expense(expense_id)`: `security definer` with a pinned `search_path`. Definer is load-bearing, not incidental -- a membership policy *on* `members` that selects *from* `members` recurses forever. `auth.uid()` is wrapped as `(select auth.uid())` so it is evaluated once per statement rather than once per row, plus a partial index on `members (user_id, group_id) where status = 'ACTIVE'`. - Four tables are not plain membership scoping, each arm load-bearing: `groups` also accepts `created_by = auth.uid()` on insert (push sends users -> groups -> members, so a new group predates its creator's membership row); `members` also accepts `user_id = auth.uid()` (pull step 1 reads by user_id before knowing any group and lands LEFT rows; joining; leaving); `users` is "me or anyone I share a group with" for read and "me or a placeholder in my group" for write; and `device_tokens` is per-user. - No policy filters `deleted_at is null`. Hiding tombstones would stop deletions reaching other devices and the data would resurrect on the next pull. - Swept `revoke truncate on all tables in schema public` -- RLS does not apply to TRUNCATE and Supabase grants ALL on every new public table, so policies alone left a one-statement wipe open on all 34 tables. `resolve_group_by_invite_token` + the matching `RemoteGroupGateway` change are the one client-visible consequence, and were agreed before implementing. A joiner is by definition not a member yet, and an RLS predicate cannot see the query's WHERE clause, so no policy can express "allow this row because they supplied its token" -- any policy permitting that read permits reading every group. The definer RPC moves the token match inside the function, where it is the authorisation check rather than a filter the caller chose. Membership gates which group you may write to, not which row within it: any ACTIVE member can still edit any expense in their groups, which is how the app already behaves. Per-actor rules are a separate design. Verified on the live project, not assumed. As a signed-in non-member: 0 rows of another group's data on all 20 tables (67 shares, 318 item_shares, 43 members, 37 users, 25 expenses and 16 groups were readable before); update/delete affect 0 rows, insert and `merge_expense` raise, TRUNCATE is denied, and an anonymous principal sees nothing. `merge_expense` is SECURITY INVOKER so these policies run inside it -- a full round trip on the simulator (add member, expense, split, edit, settle) lands correctly server-side. Known gap, deliberately out of scope: the `receipts` Storage bucket is `public = true`, so receipt *bytes* stay readable by URL. Closing it needs signed URLs, which is a client change. Green on Android + iOS; tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds group deletion with a Recently Deleted recovery screen (GroupDeletion domain logic, GroupPurgeDao, DeletedGroupRow projection), an emoji/icon picker for groups, and the RLS row-scoping schema changes needed to back it. Also carries the in-progress waitlist landing page redesign and design export assets. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Prerenders marketing routes at build time (prerender.mjs), adds sitemap.xml, robots.txt, a web app manifest, and full favicon/touch-icon set, and wires per-route SEO metadata (lib/seo.ts). Co-Authored-By: Claude Sonnet 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
1 participant
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.
Closes every open finding in
SECURITY_FIX_HANDOFF.md. One commit per finding, both platforms green at each.Base is
feat/pro-passes, notmain— this branches off it, so targeting main would drag 23 unrelated Pro commits into the diff.What's fixed
extract-receiptno longer lets the caller pick the bucket for a service-role downloadSyncEngine.healthselectInchunked at 100 ids, so pulls stop dying wholesale at ~800 expensesjsonb_populate_recordgets a real base row, so partial payloads keep column defaultsserver_now + 60s, on the create path as well as the merge patheditSettlementvoid + re-record is now one transactionexpense_edit_conflictsrows exported, judged, resolvedsupabase/exports/updateStatusdeleted, 3 docs corrected, defensive index dropThe three worth reviewing closely
#24 — sign-out (
SignOutWipeDao,SignOutFlow). The leak was real: sign out, sign in as someone else, and the next push sent the previous account's rows up under the new session. The wipe deletes every table explicitly becauseclearAllTables()resolves on Android and fails on Kotlin/Native — so the test is an iOS test, where an Android-only one would have passed against code that cannot ship. Unsynced local writes are real user data, so nothing is wiped until a push succeeds; if it fails and writes are still pending, the user is asked and nothing happens until they answer. Verified on the simulator: signed out of a populated test account, then readevenly.dbdirectly — every user table 0.#16/#19 — applied live (
wfpfgbipjmkysalfmyub), four migrations. Dry-running against a scratch Postgres caught a hole in my own first attempt: the clamp covered the merge path but not create, wherejsonb_populate_recordcopies timestamps straight from the payload. A poisoned stamp on a new expense locked the field from birth. Now the payload is sanitised once and both RPCs readv_exp, neverp_expense.#20/#21 — now enforced by CI, not by a checkbox. Both the Kotlin and TS runners assert over every vector that a fully-claimed bill's total is positive and its shares sum to it. A markdown checkbox decays; a vector does not.
Behaviour changes worth knowing
ux-firsttimerwalk.AuthSession.signOut()is nowsuspendand returnsSignOutOutcome.Not done, deliberately
Removing
expense_edit_conflictsfromSYNC_TABLES/SyncEngineis deferred until the concurrentRowSyncStatework settles — a scheduling call, not an open defect. Finding #27 is closed as wrong: addingusersto the realtime publication re-creates the 13.9M-message outage.Verification
schema.sqlapplies clean and idempotently to a scratch Postgres 18.🤖 Generated with Claude Code