Context
Raised in the KB-7 review of PR #1304 (tenant scoping). Deferred from the knowledge epic (feature/autonomous-pipeline) as a design decision rather than a clear bug.
Problem
tenant_read_condition() (crates/knowledge/src/storage.rs) is applied to both list_documents and find_by_id. Because all mutations (update_document, delete_document) locate the row through find_by_id first, the OR organization_id IS NULL arm means a tenant scoped to Some("org-a") can update and delete NULL-org legacy documents, not just read them.
The doc comment states NULL-org rows "must remain readable by every authenticated tenant until an operator runs the backfill command" — but the implementation also makes them writable by any tenant. One tenant could delete a legacy document every other tenant depends on.
There is currently no test for: "org-a calls delete_document on a NULL-org document."
Options
- Strict write isolation (safer default): add a separate
tenant_write_condition using strict equality only (organization_id = X, no OR IS NULL), so reads stay permissive during the transition window but writes are isolated per-org. Add a test asserting org-a cannot delete a NULL-org doc.
- Document intentional behavior: if any tenant self-servicing a backfill of NULL-org docs into their org is desired, document that
tenant_read_condition intentionally governs both reads and writes during the transition, and add a test asserting that behavior.
Recommendation
Option 1 (strict write isolation) is the more defensible security posture, but this is a product/policy call for the owner.
Follow-up from review: #1304 (review)
Context
Raised in the KB-7 review of PR #1304 (tenant scoping). Deferred from the knowledge epic (feature/autonomous-pipeline) as a design decision rather than a clear bug.
Problem
tenant_read_condition()(crates/knowledge/src/storage.rs) is applied to bothlist_documentsandfind_by_id. Because all mutations (update_document,delete_document) locate the row throughfind_by_idfirst, theOR organization_id IS NULLarm means a tenant scoped toSome("org-a")can update and delete NULL-org legacy documents, not just read them.The doc comment states NULL-org rows "must remain readable by every authenticated tenant until an operator runs the backfill command" — but the implementation also makes them writable by any tenant. One tenant could delete a legacy document every other tenant depends on.
There is currently no test for: "org-a calls
delete_documenton a NULL-org document."Options
tenant_write_conditionusing strict equality only (organization_id = X, noOR IS NULL), so reads stay permissive during the transition window but writes are isolated per-org. Add a test asserting org-a cannot delete a NULL-org doc.tenant_read_conditionintentionally governs both reads and writes during the transition, and add a test asserting that behavior.Recommendation
Option 1 (strict write isolation) is the more defensible security posture, but this is a product/policy call for the owner.
Follow-up from review: #1304 (review)