fix(platform): documents authz gates, corpus scope, and replacement race - #3172
Draft
larryro wants to merge 8 commits into
Draft
fix(platform): documents authz gates, corpus scope, and replacement race#3172larryro wants to merge 8 commits into
larryro wants to merge 8 commits into
Conversation
Two replacement finalizes racing on the same current file both passed their mint- and acquire-time checks and both swapped: the second overwrote the first's blob, which ended up neither current nor in history — a version lost behind two success responses. The bind now loads the document under FOR UPDATE (loadDocumentForUpdate through the shared requireDocumentWriteAccess door, which records, replacement, update and trash all consult) and the swap UPDATE carries a file_ref compare-and-swap; the loser re-reads the winner's commit, fails the target match and gets DOCUMENT_RECORD_VERSION_MISMATCH. The shared door also pins the rename/move/trash finding: the documents:write role set equals the project canEdit set, so a read-only member never reaches those doors — write-guards.test.ts locks the two matrices together so a drift in either shows up as a failing test.
The respond door checked the write matrix but never who was asked: any writer — the submitter included — could decide a review addressed to someone else, and a submitter could name themselves as reviewer. Submit now refuses self-designation (REVIEWER_SELF_NOT_ALLOWED) and the picker never offers the caller; respond admits only the designee (REVIEW_NOT_ASSIGNED) and never the submitter (REVIEW_SELF_APPROVAL_FORBIDDEN), so a review is always a second pair of eyes. The review dialog disables the decision buttons for everyone but the designee and says who decides; the docs say the same in en/de/fr.
readProjectTextValues queried by (org, project, folder, file) behind org membership only, so any member could read another team's automation settings by guessing the well-known file names. The read now runs the same project read gate the project listing does (assertReadable): an unknown or foreign project is not found, no read access is forbidden.
Only the team-change route re-stamped retrieval scope; attach/detach changed project_id and left the corpus rows alone, so a hub document moved into a restricted project stayed org-wide retrievable and a detached one vanished from hub retrieval. attach/detach hand back the blob ref and the routes re-stamp after commit, exactly like the team change does.
upsertAgentDocument wrote project_id unverified, so a mistyped or foreign id filed the document where no listing (the hub is project_id IS NULL) and no retrieval scope reached it — saved, answered ok, lost. It now requires the project to exist in the org (PROJECT_NOT_FOUND, the task writer's rule). runDocumentCreate on a multi-bound org run that named no project fell through to the org hub — wider than the run's authority; it now refuses like task_create does and names the bound projects.
The race check parks both binds behind a row lock it holds and releases once two backends wait. A serialized bind waits at its locked load of app.documents; an unserialized one waits earlier, at its file_metadata INSERT — the FK to documents.id takes a KEY SHARE lock the held FOR UPDATE conflicts with. Counting either statement as parked keeps "parked" meaning what it says whichever path a bind takes.
An unserialized bind reads the document freely and blocks later — one at the swap UPDATE behind the gate, the other behind that bind on the per-org upload counter both touch first — so a statement-text filter saw one waiter and reported the race as not parked. Counting blocked backends (pg_blocking_pids) makes "parked" mean both binds waited, whichever statement each waits in.
Only the app route re-stamped retrieval scope after a team change; the REST v1 PATCH /documents/:id took a teamId and left the corpus rows carrying the old team's scope. Same rule as the app route and the attach/detach doors: re-stamp after commit. The corpus-scope check gains the REST leg (team via REST → stamped, cleared → null).
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.
Six documents-domain findings from the backend deep review, verified against main at 5f9dc6e (which already carries #3136 write matrix, #3140 corpus purge, #3160 upload authz). Four were still live, one is closed in effect by #3136 and is now pinned, one is refuted in part.
Per-finding outcome
records.ts)REVIEWER_SELF_NOT_ALLOWED), the picker never offers the caller, respond admits only the designee (REVIEW_NOT_ASSIGNED) and never the submitter (REVIEW_SELF_APPROVAL_FORBIDDEN). Review dialog disables the decision buttons for non-designees and says who decides; docs (en/de/fr) say the same.service.ts)updateDocument/setDocumentTrashedrunassertDocumentsWriteRole;authorizeRls(documents:write)admits exactly {owner, admin, developer, editor} =EDITOR_ROLESbehindcheckProjectAccess.canEdit, so any writer who can see a project file can edit it. The existing check "documents write matrix: app door refuses read-only member" already covers rename + trash → 403. This PR unifies the door (requireDocumentWriteAccessinservice.ts, reused by records, replacement, update, trash — the private copies in records/replacement are gone) andwrite-guards.test.tspins the two matrices together so a drift in either fails a test.readProjectTextValuesskips project access (project-text.ts)assertReadable): unknown/foreign project → 404, no read access → 403.service.ts)syncRagDocumentScopeafter commit, like the team change. (The REST v1PATCH /documents/:idteam change had the same gap — see below.)replacement.ts)FOR UPDATE(loadDocumentForUpdatevia the shared door) and the swapUPDATEcarries afile_refcompare-and-swap; the loser re-reads the winner's commit and getsDOCUMENT_RECORD_VERSION_MISMATCH(intent →failed, blob swept). No migration needed — a row lock, not a constraint.document_createunvalidatedprojectIdon org-level runs (agent-write.ts) + multi-bound run writes the org hub (workspace_domain_tools.ts)documents.project_id → projectsFK rejected it with a raw Postgres error (a 500-class failure, not a clean refusal). Confirmed: a foreign org's project id passed the FK and was saved where no listing or retrieval scope reaches it. NowupsertAgentDocumentrequires the project to exist in the org (PROJECT_NOT_FOUND, the task writer's rule), andrunDocumentCreateon a multi-bound run with noprojectIdrefuses liketask_createand names the bound projects. One rule across both halves: a document lands in one existing, authorized project of this org — or, for a truly org-level run only, the hub.Tests
Unit (vitest
server):records.test.ts(new — designee / self-designation predicates),write-guards.test.ts(+ matrix drift guard),workspace_tools_bridge.test.ts(+ multi-bound refusal, + named bound project). Red on base: the same three files on 5f9dc6e → 9 failed / 79 passed (8 predicate tests:assertReviewResponder is not a function; bridge:expected 'ok' to be 'invalid_args'); the drift guard passes on base, as an already-closed finding should.Integration (
backend:integration, new/changed checks): eligibility never self + self-submit 400; designee-only respond (submitter → 403, designee → 200,respondedBy= designee); write-guards owner-approve → 403; project text read (org-wide 200 / team-restricted 403PROJECT_FORBIDDEN/ unknown 404); corpus scope stamped on attach and cleared on detach; replacement finalize race made deterministic by parking both binds behind a test-held row lock (exactly onebound, loserfailed+DOCUMENT_RECORD_VERSION_MISMATCH, chain intact);document_createauthority (multi-bound no project →invalid_argsnaming bound ids, bound project ok, outside bindings refused, truly org-level: hub / named project / bogus →not_found, no row); agent upsert bogus + foreign-org project →PROJECT_NOT_FOUND, zero unreachable rows.Verification
bunx tsc --noEmit(platform): exit 0.oxlint --type-aware(whole platform workspace): exit 0.serverproject (whole platform): 421 files / 5461 tests pass; 3app/routes/**test files fail to load identically on base in this worktree (Denied ID …/@fontsource/inter/…woff2— Vite fs-allow with a symlinkednode_modules), unrelated. Touched files: 4 files / 92 tests.lib/i18n/messages.test.ts: 24/24.@tale/uii18n suite: 9/9.@tale/docs test: 30 files / 194 tests.origin/mainmoved on during the work; no rebase performed).backend:integrationon throwaway Postgres + MinIO (SANDBOX_LLM_GATEWAY_ADMIN_PASSWORDset — no environmental gateway fails):test:ui,app/features/documents): 166/166 tests pass; 9 test files fail to load identically on base in this worktree (Denied ID …/@fontsource/inter/…woff2— Vite fs-allow with a symlinkednode_modules), unrelated.Cross-class discoveries (not fixed here unless noted)
PATCH /documents/:idwith ateamIdchange never re-stamps corpus scope (only the app route did) — same class as finding 4; fixed in this PR as part of the scope re-stamp.REVIEW_ALREADY_RESOLVEDis observable by a caller without write access (minor existence oracle, pre-existing).listProjectDocumentstreats a foreign org's org-wide project as readable and answers an empty list instead of 404 (harmless — the query is org-scoped — but inconsistent with the other project doors).upsertAgentDocumenton base surfaced FK violations as raw Postgres errors to the agent (status: 'error'with the SQL message) rather than a coded refusal.Notes
documents.record.review.onlyDesignee,…NoName) ship in en/de/fr (de-CHneeds no override); docs sentence in en/de/frplatform/knowledge/documents.md.checkDocumentsadds an editor-role reviewer for the four-eyes flow and removes the membership at its end;checkDocumentWriteGuardsadds an editor beside its member.