fix(platform): restore the audit-log, conversation and 2FA gates - #3149
fix(platform): restore the audit-log, conversation and 2FA gates#3149Israeltheminer wants to merge 1 commit into
Conversation
Three authorization and audit behaviours the port dropped. Audit-log reads were open to every non-disabled role; 0.4 was admin and owner only (#1505). The log enumerates who did what to whom across the org, and carries the GDPR erasure trail naming data subjects and lawful grounds. Fixed in the ability matrix rather than per route, so the six doors — list, detail, summary, export, integrity verify and status — are all covered. Developers keep 'write' so their own actions still append. A read-only member could reply, send outbound mail, close, bulk-act and delete conversations. 0.4 ran every conversation mutation through mutationWithRLS, whose rules put each write through authorizeRls at editor-or-above. viewerCanWrite reuses that same matrix rather than declaring a second role list, and composes with the assignment-privacy half that was correctly ported: a write door checks the role, then loads the row through loadVisibleConversation. Sending mail leaves the building under the org's name, so it is the sharpest of these. 2FA and passkey lifecycle events audited only on FAILURE, so an attacker who added a passkey and disabled 2FA left no successful-action trail. The five 0.4 action names are reused verbatim — 2fa_enrolled, 2fa_disabled, passkey_added, passkey_removed, passkey_sign_in — because a renamed action breaks any existing query or export grouping on it.
|
Split. The verified half is #3181; this branch's remaining finding needs real work, so I am not shipping it behind a passing fix. Dropped: the audit-log third. Worth recording: my first attempt at removing that third deleted Shipped: the 2FA lifecycle audit → #3181. Verified 374/380 with the full five-name trail. Held: the conversation write-role gate. It is faithful — 0.4's rule is literally
So the gate owns the damage, and the first observable failure is a genuine interaction rather than a fixture accident: The gate fires before the attachment-ownership check, so that assertion receives the wrong 403, and several chat checks cascade behind it. Two possible resolutions, and picking between them is a decision rather than a fix: raise the roles of the harness actors that compose mail (if the fixtures were written assuming members may write), or move the gate relative to the ownership check so the more specific refusal still wins. Leaving this open as the record of that decision. The finding itself stands — a read-only member can currently reply, send outbound mail, close, bulk-act and delete conversations — and it is in #3142. |
|
Superseded by #3187, which rebases this onto current main. Two changes beyond a rebase:
|
Three authorization and audit behaviours the port dropped. Any member could read the audit log; a read-only member could send outbound mail.
Refs #3142.
Audit-log reads were open to every role
0.4 restricted them to admin and owner (#1505). The log enumerates who did what to whom across the organization, and it carries the GDPR erasure trail — data subject names and the lawful grounds for erasing them.
Fixed in the ability matrix rather than route by route, so all six doors are covered together: list, detail, summary, export, integrity verify and status. Gating them individually invites the next door to ship ungated.
Developers keep
write, so a developer's own actions still append their audit rows. Members hold neither.A read-only member could write to conversations
Reply, send outbound mail, close, bulk-act, delete. 0.4 ran every conversation mutation through
mutationWithRLS, whoseconversationsandconversationMessagesrules put each write throughauthorizeRlsat editor-or-above.viewerCanWritereuses that same matrix rather than declaring a second role list. It composes with the half that was correctly ported: a write door checks the role first, then loads the row throughloadVisibleConversation, which appliesconversationAssignmentAllowsper row. Role decides whether you may change a conversation; assignment decides which ones you can see at all; assignment itself is stricter still.Sending outbound mail is the sharpest of the five — it leaves the building under the organization's name.
2FA and passkey events audited only on failure
An attacker who added a passkey and disabled 2FA left no successful-action trail. All five events now audit, inside the same serializable transaction as the state change.
The 0.4 action names are reused verbatim —
2fa_enrolled,2fa_disabled,passkey_added,passkey_removed,passkey_sign_in. A renamed action silently breaks any existing query or export that groups on it. Checked against 0.4's full vocabulary:passkey_removedandpasskey_revoked_by_adminwere distinct events there and stay distinct here.Tests
access.test.tspins the matrix, andintegration-check.tscovers the doors end to end.auditLogs: ['read', 'write']restored to the developer roleEach finding asserts both directions — the lower role is refused and the higher role still allowed. Testing only the refusal is half a test: a gate that refuses everyone passes it.
The caller audit
Tightening a gate breaks callers, so every door was traced before commit. The audit-log doors are reached only from the admin-facing settings surface. The conversation write doors are reached from the inbox UI, the REST surface (which carries its own editor-role gate), and the connector lanes, which run under system auth. The 2FA changes add audit writes and gate nothing, so they have no caller risk.
Role comes from the session-resolved value where the seam offers it, not a raw
membertable read — trusted-headers deployments insert a placeholder member row, so a proxy-declared admin can hold a non-admin role in that table.Scope
Retention destruction still emits no audit rows — sixteen 0.4 action names exist nowhere in the tree. Same class, different file, and #3143 is already editing it.
The audit chain's
pii_scrubbedrecompute skip and its oldest-row trust anchor are untouched. Both are signed off inbackend/MIGRATION.mdas intentional divergences, so they are a design question rather than a porting defect.Gate:
typecheck,oxlint --type-aware,oxfmt --check,lint:sastgreen; the new unit suite 7/7.