[jwvanderstam]

What

A follow-up from the separate security review of #528 (Low, reachable by a global admin and the SyncWorker only).

A global contribution and a document uploaded without a workspace both carry workspace_id IS NULL. As a result:

• documents.exists(filename, None), with IS NOT DISTINCT FROM, matched a contribution. An admin upload naming no workspace, an admin-created connector without a workspace, or the re-ingest worker could therefore update a contribution in place. Every workspace reads the global tier.
• documents.stale() returned contributions too, so the worker tried to re-ingest a copy that has no source file.

Change

• Both queries add contributed_at IS NULL.
• A workspace-less upload sharing a contribution's filename now creates its own row. Migration 0020 already narrowed 0013's uniqueness index to non-contributions, so that does not collide.

Tests

• test_a_contribution_is_not_mistaken_for_a_document_without_a_workspace in tests/integration/test_gkb_contribution.py. Against Postgres it fails on the old code with (True, True): exists matched the contribution, and stale returned it.
• The GKB integration file and test_db_operations pass: 39 passed. ruff is clean.

touches the global tier → security-critical per CONTRIBUTING, so it gets a separate review.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KkWPqAcWbxfqz1aCP6Qsdx

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[jwvanderstam]

Security review (separate agent session, CONTRIBUTING.md)

Reviewed: PR #532 at 5054eb8 (diff against main a04da4d). I read the diff and the code around it: src/db/documents.py, src/rag/processor.py, src/connectors/worker.py, src/db/connectors.py, src/db/contributions.py, and migrations 0013 and 0020.

Verdict: the fix is correct and narrow. Both queries that treated "no workspace" as one bucket now exclude the global tier. No other query in src/ matches documents by a NULL workspace. I have one Low finding, about the test.

Checklist

1. Identity: not touched.
2. Authorisation: not touched. No route or guard changed. The paths reaching exists(filename, None) are DocumentProcessor._prepare_for_ingestion (src/rag/processor.py:361), used by a workspace-less upload or connector and by SyncWorker._reingest_document.
3. Scope: unchanged.
   • exists still opens scope=workspace_id or ALL_WORKSPACES (src/db/documents.py:653).
   • stale still takes the scope the caller passes. The worker passes ALL_WORKSPACES (src/connectors/worker.py:185), which is reasonable for an installation-wide maintenance loop that no request reaches.
   • The change narrows what those ALL_WORKSPACES paths can match (src/db/documents.py:661, :860). That is the intended direction.
4. Row-level security: not touched. There is no migration. The PR's claim about uniqueness checks out: 0020 narrowed documents_filename_workspace_uidx to contributed_at IS NULL (migrations/versions/0020_gkb2_contribution_identity.py:43-45). A workspace-less upload sharing a contribution's filename therefore inserts a separate row without a conflict.
5. Data integrity: improved.
   • Before this change, exists(filename, None) returned the contribution's id, and _prepare_for_ingestion handed that id to documents.update() as replace_doc_id. That update (src/db/documents.py:130) filters on id only, so it would have overwritten a global-tier row's content and chunks in place. That is now unreachable from this path.
   • Archived contributions keep contributed_at set, so they are excluded too, which is correct.
   • No DELETE is introduced.
6. Input: both changes are literal predicates. Parameters are unchanged, and the scope_sql f-string fragment comes from scope_predicate() (a literal column with a %s value). No string formatting of input.
7. Secrets: not touched.
8. Image: not touched.
9. Tests: partly met, see the finding below.
   • test_a_contribution_is_not_mistaken_for_a_document_without_a_workspace (tests/integration/test_gkb_contribution.py:257) fails on the old code by construction: the contribution has workspace_id IS NULL, so the old IS NOT DISTINCT FROM NULL matched it, and the old stale had no tier filter.
   • It ran rather than skipped in this PR's integration-tests job: PASSED in the log, and the fixture runs the Alembic chain, so contributed_at exists.
   • It asserts only the negative side, though.

Related paths checked, no finding

• ConnectorsRepository.retire_document_by_filename (src/db/connectors.py:290) matches on source_id = %s. A contribution copies no source_id (src/db/contributions.py:61-67), so a connector delete cannot reach the global tier.
• _handle_delete also refuses a workspace-less connector (src/connectors/worker.py:224).
• The global-tier read predicate (src/db/documents.py:24) requires contributed_at IS NOT NULL, so a workspace-less upload is never read as global knowledge.

Findings

Low: the regression test has no positive control. The test asserts (exists(...)[0], global_id in stale_ids) == (False, False) and nothing else. A change that broke both queries outright would also pass it, for example a predicate that is always false or a stale that returns []. That is the "a test of a refusal proves it would pass for the permitted caller" half of checklist item 9.

• Failure scenario: a later edit makes exists stop matching genuine workspace-less documents. Every workspace-less re-upload would then insert a duplicate, or hit the 0013/0020 unique index and fail. The test would stay green.
• Suggested fix: in the same test, show that the permitted cases are still matched:
  • insert a genuine workspace-less document with the same filename and assert exists(filename, None) returns its id rather than global_id, or at least that exists(filename, workspaces["a"])[0] is True;
  • assert that the source document doc is in stale_ids.
• Severity: Low. It is answered in the PR and does not block the merge under CONTRIBUTING's rules.

🤖 Generated with Claude Code