fix(security): harden log_sms_notification and move SMS path to service role - #249
Merged
Merged
Conversation
…ce role Closes the last function that was still abusable by an authenticated user. log_sms_notification inserts into sms_notifications with a caller supplied phone number and message content, and had no checks at all. It cannot take a plain auth.uid() ownership binding because it is legitimately cross-user -- the sender logs a notification on behalf of the recipient. So the guard is conversation membership: the caller must be an active participant, and the user being logged for must be a participant too, which stops arbitrary phone numbers being attached to a conversation. Also switches the SMS notification path in the websocket handler to the service-role client. Reading other participants' phone numbers and writing rows on their behalf is server-side work, and passing the caller's client is what forced this function and get_inactive_participants to stay reachable by `authenticated`. The guard is deliberately used instead of a REVOKE: Railway deploys on merge rather than in lockstep with migrations, so a revoke would break SMS notifications in the window before this code is live. The guard is correct in either order. Once deployed, both functions can be locked to service_role outright. Note tests/sms-notification-service.test.js is in the vitest exclude list, so this path has no automated coverage in CI. Already applied to production as 20260814040456. Full suite: 81 files, 483 tests, all passing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ThreatCrush Security Scan90 finding(s) HIGH/CRITICAL: 4 | MEDIUM: 10 | LOW: 76
…and 40 more. Full results in the Security tab. Snippets are redacted; ThreatCrush never prints matched credential material. |
ralyodio
added a commit
that referenced
this pull request
Aug 15, 2026
…250) * fix(security): scope cleanup endpoints and harden backup PIN storage Closes the two GitHub security advisories still open against this repo. The other four were remediated in #244-#249. GHSA-2wqx-qppx-4rgf (High) -- cleanup endpoints: /api/cleanup/legacy-messages and /api/cleanup/empty-conversations authenticated with getSession(), which reads the cookie without re-validating the JWT against the auth server, and then operated platform-wide: legacy-messages selected and deleted up to 1000 matching messages with no user filter at all, and empty-conversations enumerated every conversation in the table. Both now use getUser(), resolve the internal user id, and scope every query to the caller -- matching the pattern already used by cleanup/legacy-keys. GHSA-jpfm-vrpc-p6rr (Medium) -- backup PIN: PINs were hashed with unsalted SHA-256, while users_select_authenticated lets any logged-in account read every row of `users`, so one throwaway signup could dump every backup_pin_hash and reverse the 6-12 digit keyspace with a precomputed table. Narrowing the RLS policy alone does not fix this -- the app legitimately reads other users' rows and several callers do select('*'), which a column-level REVOKE would break. So the credential column moves out of `users` into user_backup_pins, a table anon and authenticated hold no privileges on. New PINs are derived with scrypt (N=16384, r=8, p=1) over a per-user random salt. The old digests are discarded rather than relocated: nothing verified them server-side, they only ever backed the hasPin boolean, and being reversible is the entire problem. Row existence preserves hasPin for the 5 affected users. The column drop is a separate migration (20260815120100) because it must not land until this code is deployed. 20260815120000 is additive and is already applied to prod. * chore(db): align migration filenames with the applied prod versions MCP apply_migration restamps the version it records; prod logged 20260815204334, not the 20260815120000 in the filename. Rename so supabase db push does not rerun it, and renumber the follow-up drop migration to stay after it.
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 the last function that was still abusable by an authenticated user, completing the remediation started in #244.
Already applied to production as
20260814040456. Full suite green locally: 81 files, 483 tests.The problem
log_sms_notificationinserts intosms_notificationswith a caller-supplied phone number and message content, and had no checks whatsoever. Any authenticated user could write arbitrary rows.It can't take a plain
auth.uid()ownership binding, because it's legitimately cross-user — the sender logs a notification on behalf of the recipient. That's why it was left open in #248.The fix
Guard on conversation membership instead: the caller must be an active participant, and the user being logged for must be a participant too (so arbitrary phone numbers can't be attached to a conversation).
Plus the SMS notification path in
lib/websocket/handlers/messages.jsnow uses the service-role client. Reading other participants' phone numbers and writing rows on their behalf is server-side work; passing the caller's client is precisely what forced this function andget_inactive_participantsto stay reachable byauthenticated.Why a guard and not a REVOKE
Railway deploys on merge, not in lockstep with migrations. A
REVOKEapplied now would break SMS notifications in the window before this code is live — and silently, since the call sits inside atry. The membership guard is correct in either order:service_rolehas a NULLauth.uid()and passes through,authenticatedmust be a real participant.Follow-up once this is deployed:
log_sms_notificationandget_inactive_participantscan both be locked toservice_roleoutright, which also removes the residual — a genuine participant can still write an arbitrary phone/content pair for another participant of the same conversation.Coverage gap worth knowing
tests/sms-notification-service.test.jsis in vitest'sexcludelist, so this path has no automated coverage — it doesn't run in CI either. The change is verified by reading the call sites, not by tests.🤖 Generated with Claude Code