Skip to content

fix(platform): restore the audit-log, conversation and 2FA gates - #3149

Closed
Israeltheminer wants to merge 1 commit into
mainfrom
fix/restore-role-and-audit-gates
Closed

fix(platform): restore the audit-log, conversation and 2FA gates#3149
Israeltheminer wants to merge 1 commit into
mainfrom
fix/restore-role-and-audit-gates

Conversation

@Israeltheminer

Copy link
Copy Markdown
Collaborator

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, whose conversations and conversationMessages rules put each write through authorizeRls at editor-or-above.

viewerCanWrite reuses 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 through loadVisibleConversation, which applies conversationAssignmentAllows per 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_removed and passkey_revoked_by_admin were distinct events there and stay distinct here.

Tests

access.test.ts pins the matrix, and integration-check.ts covers the doors end to end.

Mutation Went red
auditLogs: ['read', 'write'] restored to the developer role two assertions

Each 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 member table 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_scrubbed recompute skip and its oldest-row trust anchor are untouched. Both are signed off in backend/MIGRATION.md as intentional divergences, so they are a design question rather than a porting defect.

Gate: typecheck, oxlint --type-aware, oxfmt --check, lint:sast green; the new unit suite 7/7.

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.
@Israeltheminer

Copy link
Copy Markdown
Collaborator Author

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. main fixed it independently and more thoroughly than this branch did — the ability matrix pins auditLogs per role, routes.ts consults it via authorizeRls, and checkAuditSurface asserts the refusal end-to-end. My route-level isAdminRole check was a divergent second expression of the same rule.

Worth recording: my first attempt at removing that third deleted main's own checkAuditSurface functionmain has one, and my commit had appended a block inside it, so cutting "my" block took main's with it. Caught by diffing function names against main; restored byte-for-byte and re-applied my checks with a 3-way patch instead of a hand splice. That is the second time today a splice ate main's content, the other being MIGRATION.md on #3146.

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 authorizeRls(role, 'conversations', 'write') and this repo's matrix gives member read only — but it does not survive the harness:

result
both halves aborts at 178 checks, 9 failures
main 373/379, 6 known failures
2FA half alone 373/379, identical 6

So the gate owns the damage, and the first observable failure is a genuine interaction rather than a fixture accident:

FAIL  blob-ref authority: outbound mail attaches only the sender's uploads
      compose → 403 FORBIDDEN (want 403 attachment_not_owned)

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.

@Israeltheminer

Copy link
Copy Markdown
Collaborator Author

Superseded by #3187, which rebases this onto current main.

Two changes beyond a rebase:

  • The audit-log third is dropped. fix(platform): close account and credential authorization holes #3159 landed the same role matrix, and main's doors already call authorizeRls(role, 'auditLogs', 'read') — which that matrix now denies below admin. Swapping them to isAdminRole would restate the rule in a second place.
  • The attachment-fetch door is gated too, so all twelve write-shaped doors carry the check, plus a static test that fails when a new door ships without it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant