fix(security): close conversation hijacking, profile mass assignment, avatar MIME trust - #254
Merged
Merged
Conversation
… avatar MIME trust Three findings from the consolidated assessment (GHSA-3hqc-9v44-j37g) that were still live, verified against production before writing the fix. V-007 -- orphaned conversation hijacking. The UPDATE policy on `conversations` began `(created_by IS NULL) AND (auth.uid() IS NOT NULL)`, so any authenticated account could take over any conversation with a NULL creator. Replaced with a creator-only policy that also carries a WITH CHECK, so the row cannot be handed to someone else on the way out. V-009 -- mass assignment on `users`. `Users can update own profile` had no WITH CHECK and no column restriction, letting a user rewrite any column of their own row including `phone_number` and `unique_identifier`. Enforced with a BEFORE UPDATE trigger rather than column-level REVOKE: a REVOKE has to enumerate every writable column correctly and any column added later defaults back to writable, whereas the trigger states the invariant once and fails closed. service_role is exempt, since every legitimate writer of those columns is server-side (salt, verify-sms, CoinPay callback, register-anon). NEW-08 -- avatar upload trusted the client's Content-Type, so arbitrary content could be stored in the public avatars bucket and served back as an image. Now sniffed from magic bytes, and the detected type drives both the extension and the stored content-type. Also drops four UPDATE policies and one SELECT policy comparing `auth.uid() = id` (IA-040 identity domain drift: auth.uid() is the Auth UUID, users.id is the internal key). Confirmed against production that 0 of 86 rows have id = auth_user_id, so these have never matched a row and grant nothing. Not fixed here and still open from the umbrella: V-011/V-012/V-013/V-022 (invitation quota and recipient validation), V-014, V-023, V-025, NEW-05 (webhook SSRF), and the remaining lows. 500 tests pass; build clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ThreatCrush Security Scan91 finding(s) HIGH/CRITICAL: 5 | MEDIUM: 10 | LOW: 76
…and 41 more. Full results in the Security tab. Snippets are redacted; ThreatCrush never prints matched credential material. |
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.
Wave 3. Three findings from the consolidated assessment (
GHSA-3hqc-9v44-j37g) that were still live in production, each verified against prod before writing the fix.conversationsbegan(created_by IS NULL) AND (auth.uid() IS NOT NULL)— any authenticated account could take over any conversation with a NULL creatorWITH CHECKso the row can't be handed offUsers can update own profilehad noWITH CHECKand no column restriction — a user could rewrite any column of their own row, includingphone_numberandunique_identifierBEFORE UPDATEtrigger pinning the identity columnsContent-TypeWhy a trigger and not column-level REVOKE for V-009
A
REVOKEhas to enumerate every writable column correctly, and any column added later defaults back to writable. The trigger states the invariant once and fails closed for columns nobody thought about.service_roleis exempt because every legitimate writer of those columns is server-side —/api/auth/salt,/api/auth/verify-sms, the CoinPay callback,/api/auth/register-anon. The authenticated role legitimately writes onlybio,website,updated_atandsms_notifications_enabled, none of which are pinned.Dead policies dropped
Four UPDATE policies and one SELECT policy compare
auth.uid() = id— the IA-040 identity domain drift, whereauth.uid()is the Auth UUID andusers.idis the internal key. Verified against production: 0 of 86 rows haveid = auth_user_id, so these have never matched a row and grant nothing.Still open from the umbrella
Not addressed here: V-011, V-012, V-013, V-022 (invitation quota / recipient validation), V-014 (recursive realtime subscription), V-023 (SSE spam), V-025 (
register-anonatomicity), NEW-05 (webhook SSRF via DNS rebinding), and the remaining lows. V-017 and V-024 turned out to be moot —get_user_conversations_enhanced,get_user_groupsandget_group_roomsdo not exist in production.Verification
pnpm buildclean.🤖 Generated with Claude Code