Skip to content

fix(platform): enforce the document write matrix and record freeze - #3136

Merged
larryro merged 4 commits into
mainfrom
fix/document-write-guards
Sep 3, 2026
Merged

fix(platform): enforce the document write matrix and record freeze#3136
larryro merged 4 commits into
mainfrom
fix/document-write-guards

Conversation

@larryro

@larryro larryro commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Fixes the verified review class document mutations bypass the org role write matrix and the controlled-record freeze (5 findings, all confirmed in code).

The enforced matrix

Role documents read documents write (create / edit / trash / delete / record + replacement flows / RAG retry / folder cascade)
owner / admin / developer / editor yes yes
member yes refused — RBAC_FORBIDDEN 403
disabled no no

One implementation: assertDocumentsWriteRole (documents service) delegates to authorizeRls(role, 'documents', 'write') — the same matrix auth/access.ts defines, the same set as the frontend's knowledgeWrite ability and EDITOR_ROLES. Project-scoped documents keep their additional project-canEdit check; agent standing-grant writes stay authorized by their dispatch (no human role there, by design).

Per-finding outcome

1. Org role write matrix never enforced on document mutations — fixed (critical/security).
assertDocumentsWriteRole now gates every mutating service function: createDocumentFromUpload, createDocumentFromBlobUpload, createHubDocument, updateDocument, setDocumentTrashed, attachDocumentToProject, detachDocumentFromProject, deleteDocumentHard, deleteFolderCascade, retryRagIndexingForDocument, plus the shared write standards in records.ts (requireDocumentWriteAccess → mark-controlled / submit / respond / open-revision) and replacement.ts (requireReplacementDocument → begin / finalize / resume). The REST retry-indexing route (its own inline body) gets the same gate. Reviewer eligibility (isEligibleDocumentReviewer) is tightened to write-capable roles on hub documents so a designated reviewer can never be someone the respond gate refuses (previously any visible role — including member — was hub-eligible server-side while the UI picker filtered to EDITOR_ROLES).
Regression tests: unit matrix table (write-guards.test.ts), plus integration member-refusal sweeps across the app door (rename/trash/delete/blob-bind/mark-controlled/replacement-begin/RAG-retry/folder-cascade) and the REST door (POST/PATCH/DELETE/retry) with reads proven still open.

2. Agent document upsert overwrites frozen controlled records — fixed (high/bug).
upsertAgentDocument's update path now selects the row's record and applies assertGenericDocumentContentWritableJson — the same generic-content-writer rule the human paths and WebDAV apply (controlled bytes move only through the attested replacement flow; in_review/approved report frozen). content_hash is kept coherent on both paths: storeAgentTextBlob stamps the blob's sha256 on its file_metadata row (the knowledge-entry convention) and the upsert resolves the hash from that ledger row, so a refreshed document never carries the previous blob's hash.
Regression tests: integration — create/rerun hash coherence (sha256 equality against the row), mark-controlled then rerun → DOCUMENT_RECORD_REPLACEMENT_REQUIRED with file_ref unchanged.

3. updateDocument misses the content-freeze guard; REST PATCH edits frozen records — fixed (high/bug).
The guard lives at the service seam: updateDocument calls assertGenericDocumentContentWritableJson whenever content/mimeType/extension/sourceProvider are present, so the app door, REST PATCH, and any future caller inherit it. Renames, folder moves, team and metadata edits stay allowed in every record state (the access-layer doctrine). The guard is the jsonb-projection twin of core/documents/access.ts#assertGenericDocumentContentWritable (same precedent as the existing assertRecordTrashableJson), throwing DocumentError so both error surfaces stay coded: app {error: code}, REST {error: message, code}.
Regression tests: unit state table (uncontrolled passes; draft → DOCUMENT_RECORD_REPLACEMENT_REQUIRED; in_review/approved → DOCUMENT_RECORD_FROZEN); integration REST PATCH content/mime refusals on draft and in_review, title-only PATCH still 204.

4. REST DELETE destroys retained approved history of controlled records — fixed (high/bug).
DELETE /api/v1/documents/:id no longer re-derives protection from state !== 'draft' (the exact re-derivation bug class core/documents/access.ts names). It calls deleteDocumentHard — the session delete whole — inheriting the single trash predicate (assertRecordTrashableJson: in_review, approved, and retained approved history), the write matrix, legal holds, the sync stop, and the audit row REST previously skipped. Wire parity kept: protected records still answer this door's established 409 {error: 'DOCUMENT_RECORD_PROTECTED', message}.
Regression tests: unit — revision-draft-with-history refuses; integration — approve → DELETE 409, open-revision → DELETE 409 with the row proven still present, uncontrolled DELETE 204 with the audit row now written.

5. Unvalidated teamId on document create/patch makes documents permanently invisible — fixed (high/bug).
assertHubTeamAssignable applies the plural-teamIds lane's rule (caller must belong → TEAM_ACCESS_DENIED 403; membership implies the team exists and is in-org) to every lane that stamps team_id/team_tags: createHubDocument (REST POST), createDocumentFromUpload's hub lane (including the previously-skipped teamId+folderId combination), and updateDocument's singular-teamId lane (app + REST PATCH). Clearing (teamId: null) stays allowed. This matches the product's team-visibility semantics — hasTeamAccess has no admin bypass, and the UI's document team pickers (useTeamsmyTeamsQuery) already offer only the caller's own teams.
Regression tests: unit membership rule; integration — bogus and existing-but-foreign teamId refused on REST create and app patch, member-of-team create succeeds with team_id/team_tags stamped, patch + clear succeed.

Refuted findings: none — all five reproduced in code as reported.

Call-site sweep (every mutating entry into the documents domain)

Caller How it inherits the gates
App routes /api/app/documents/* (routes.ts) all mutations call the gated service/records/replacement functions
App folders DELETE /api/app/folders/:id deleteFolderCascade (gated; record predicate pre-walk unchanged)
REST POST/PATCH /api/v1/documents* (v1-core) createHubDocument / updateDocument (gated + freeze + teamId)
REST DELETE /api/v1/documents/:id now deleteDocumentHard (gated + predicate + audit)
REST POST /documents/:id/retry-indexing inline body → explicit assertDocumentsWriteRole
REST projects upload (v1-projects) createDocumentFromUpload (gated; project lane already required canEdit — same role set, unchanged wire code)
Workflow document store (connectors/service.ts pgDocumentStore) calls createHubDocument with synthetic role: 'owner' — passes by construction
Sandbox workspace tool (workspace-write-shimupsertAgentDocument) standing-grant authority (no role matrix, by design); now carries the record freeze + hash coherence
OneDrive / Google Drive sync loops own injected raw-SQL update in domains/onedrive/service.ts — never touches the service seam, and sync-owned docs are not controllable (CONTROLLABLE_SOURCE_PROVIDERS = upload, agent)
WebDAV handlers already carry the core record guards (assertGenericDocumentContentWritable / assertRecordTrashable); write credentials are minted behind the developer capability
project-text (ensureProjectTextDocument) project lane, already gated via getOrCreateProjectFolderassertProjectFolderWrite (project canEdit); its docs (source_provider: 'project_text') are not controllable

Frontend expectations

No divergence: the app already hides every document mutation (including RAG retry) behind the knowledgeWrite ability, whose role set equals this gate; the reviewer picker already filters to EDITOR_ROLES; the team pickers already offer only the caller's own teams. The backend now enforces what the UI promises.

Verification

  • Unit (write-guards.test.ts, vitest server project): 19/19 on the branch; on base (implementation files reverted to main, tests kept): 14 failed / 5 passed — the 14 exercise the three new guards, the 5 passing are the pre-existing trash predicate the REST door failed to use. Full platform vitest on the branch: 373 files / 72294 tests passed.
  • backend:integration on a throwaway tale-db + MinIO (fresh DB per run): branch 285/285; base proof (implementation reverted, tests kept): 279/285 — exactly the six new checks red, every pre-existing check green. The base reds narrate the holes on main verbatim: member rename/trash/hard-delete/RAG-retry/folder-cascade answered 200 and REST create 201; PATCH content on a draft and an in-review record answered 204; DELETE of a revision draft with approved history answered 204 with the row gone and no audit row; agent rerun over a controlled record was ALLOWED with content_hash null and file_ref swapped; bogus/foreign teamId answered 201; the eligibility listing offered the member as reviewer.
  • Platform tsc --noEmit and oxlint --type-aware: clean.

Behaviour changes to be aware of

  • A review already assigned to a member-role reviewer (possible only via the old API hole) can no longer be answered by that member; re-submitting designates an eligible reviewer (newest submission supersedes).
  • An automation whose report was marked controlled (even a draft) no longer refreshes it in place — the record lifecycle owns those bytes; the run surfaces DOCUMENT_RECORD_REPLACEMENT_REQUIRED.
  • REST DELETE now also writes the document.deleted audit row and stops a 1:1 OneDrive sync, like the session path.

Cross-class discoveries (not fixed here)

  • knowledge_entries has the same write-matrix hole: /api/app/knowledge-entries and the REST knowledge-entry mutations mount only session+org-member, and the service takes no role — a read-only member can create/update/delete entries, which materialize app.documents rows (source_provider: 'knowledge'). The UI hides these behind knowledgeWrite. Left out: separate module, and createKnowledgeEntry also serves a chat-lane caller whose authority model needs its own decision.
  • The core guard (assertGenericDocumentContentWritable) treats a malformed record json as frozen (fail closed) while parseControlledRecord treats it as uncontrolled; the new json twin follows the core fail-closed stance. Only unreachable states differ — the records flow never writes malformed json.

Every mutating documents service function now consults the org role
write matrix (authorizeRls documents:write — the products/contacts
idiom): create-from-upload/blob, hub create, update, trash/restore,
attach/detach, hard delete, folder cascade, RAG retry, and the record
and replacement write standards. A read-only member could previously
create, edit, trash, hard-delete and approve documents through the app
and REST doors — the UI merely hid the buttons.

updateDocument additionally enforces the controlled-record content
freeze (assertGenericDocumentContentWritableJson, the jsonb twin of
core/documents/access.ts) whenever content/mimeType/extension/
sourceProvider are present, and the singular teamId lane on create and
update now validates assignability like the plural teamIds lane
(caller must belong — a document can never be filed into a scope
nobody can see).

Reviewer eligibility (isEligibleDocumentReviewer) now requires a
write-capable role on hub documents too, matching the respond gate so
a designated reviewer can never be one who could not respond.
upsertAgentDocument's update path swapped title/file_ref/mime/extension
on the matched row with no record guard — a document_create re-run with
the same externalItemId silently replaced an approved or in-review
controlled record's current file (agent docs are explicitly
controllable), after which the replacement dialog dead-ended on
SNAPSHOT_INVALID. The update path now applies the same generic
content-freeze rule every human content writer goes through.

content_hash stays coherent with the served bytes on both paths:
storeAgentTextBlob stamps the blob's sha256 on its file_metadata row
(the knowledge-entry convention) and the upsert resolves the hash from
that ledger row instead of leaving the previous blob's hash behind.
DELETE /api/v1/documents/:id re-derived record protection from
state !== 'draft' alone, so a draft opened after approval (which still
retains approvedVersions) purged its approved history — the exact bug
class core/documents/access.ts names. The handler now calls
deleteDocumentHard, inheriting the single protection predicate
(retained history included), the org write matrix, legal holds, the
sync stop, and the audit row the session delete writes; protected
records keep this door's established 409 wire shape.

POST /documents/:id/retry-indexing gets the same write-matrix gate as
the session Retry affordance (it rewrites RAG bookkeeping and enqueues
billable indexing).
A late-running checkDocumentWriteGuards section: member-role refusals
across the app and REST v1 doors (reads stay open), the REST PATCH
content freeze on controlled records, REST DELETE refusing retained
approved history with the audit row present, agent upsert content-hash
coherence plus the controlled-record freeze, reviewer eligibility
excluding read-only roles, and hub teamId assignability on create and
patch. Runs after the earlier sections because it adds a member user
and a team to the org.
@larryro
larryro marked this pull request as ready for review September 3, 2026 03:54
@larryro
larryro merged commit 1788cdc into main Sep 3, 2026
23 checks passed
@larryro
larryro deleted the fix/document-write-guards branch September 3, 2026 03:54
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