Repository navigation
security(kb): only admins may make a fact platform-wide, and visibility writes keep the indexes current #16663
Description
Activity
- added a commit that references this issue
on Sep 14, 2026 Closure audit against
origin/mainat 7b49b52, after #16677 (squash 7b49b52). I'm reopening this: AC1 is not met as written. AC2 and AC3 are met and ticked.AC2: met.
update_factreindexes whenever ownership changes (knowledge/facts.py:1144,await reindex_ownership(...)), andindex_ownershipgoes throughownership_manager.set_owner. Tested bytest_a_permissions_change_is_written_through_update_fact_not_the_hashandtest_update_fact_takes_a_demoted_fact_out_of_the_system_index.AC3: met. Ingestion calls
await index_ownership(self.ownership_manager, fact_id, metadata)atknowledge/facts.py:602, and the forwarded keys includeorganization_idandgroup_ids. Tested bytest_indexing_forwards_org_group_and_access_levelandtest_ingestion_files_the_fact_under_its_organization_and_group.AC1: not met. The criterion reads "a non-admin gets 403 when setting SYSTEM or PUBLIC through any endpoint, and an admin can set them".
- The permissions route does this:
api/knowledge_collaboration.py:115-116raises 403 for a non-admin asking for a platform-wide visibility. - The MCP add route does not refuse.
api/knowledge_mcp.py:514passes the metadata throughdrop_ownership_unless_admin(request.metadata, current_user.get("role")), which silently removes a non-admin's SYSTEM/PUBLIC request and returns success. The caller is told the write worked, but the fact is not platform-wide. - Chat-derived ingestion at
api/chat_knowledge_manager.py:331calls the same helper with a role ofNone, so it strips visibility for every caller, admins included.
The privilege escalation is closed on every path; the gap is the refusal contract. To meet AC1, make the MCP add route return 403 when a non-admin asks for SYSTEM or PUBLIC, and keep stripping only the owner fields the caller cannot set. Add a route-level test for both the non-admin 403 and the admin success. The chat path has no caller-supplied visibility to refuse, so it can keep stripping, but that should be stated here as an explicit scope note rather than left implicit.
- The permissions route does this:
Re-opened AC1: the refusal contract is being fixed in PR #16705 (batched with #16662's remainder).
MCP add route (
api/knowledge_mcp.py). A non-admin asking for platform-wide reach now gets 403 instead of a silent strip and a reported success. Platform-wide reach meansvisibilitysystem or public, or anaccess_levelof general or autobot: the owner's rule gives both every signed-in user. The check runs before the route'stry, which would otherwise swallow the error. Otherwise only the owner fields (owner_id,user_id) are stripped, and the rest of the metadata is kept. An admin's request is stored as sent. There are route-level tests for the non-admin 403 on all three shapes, admin success, and the owner field being stripped.Scope note, the chat path (
api/chat_knowledge_manager.py, ADD_TO_KB). It keepsdrop_ownership_unless_admin(..., None), the full strip. That path has no caller role (the router is unauthenticated, tracked on #16375). The metadata it forwards is the item the chat flow stored, and the system then setsowner_idandvisibilityitself, so a caller-supplied visibility never needs a refusal there. AC1's "any endpoint" therefore means every endpoint where a caller can ask for a visibility. The collaboration permissions route already returns 403 (_apply_visibility_to_metadata).- added a commit that references this issue
on Sep 14, 2026 Correction to my previous comment. The MCP add route does not keep a non-admin's non-owner fields.
The delta code review of PR #16705 found that stripping only
owner_id/user_idwould let a non-admin keepvisibility: organizationwith another org'sorganization_id, or group, shared,group_idsorshared_withvalues. That files a fact into orgs and groups the caller isn't in, becauseis_visibleand the ChromaDB pre-filter grant org and group reach from the metadata alone, and the route has no identity binding to validate those fields.So the behaviour in #16705 is:
- a non-admin asking for platform-wide reach gets 403, which is AC1's refusal contract. That means SYSTEM or PUBLIC visibility, or a general or autobot access level, compared case- and whitespace-insensitively;
- all of a non-admin's other ownership fields are dropped (
drop_ownership_unless_admin), as before; - an admin's request is stored as sent.
A cross-tenant regression test pins it.
A correction to my audit comment above. "Keep stripping only the owner fields the caller cannot set" should not be read as "strip only
owner_id/user_id".A non-admin cannot set any ownership field through MCP add. That covers
organization_id,group_idsandshared_withas well, together with the visibility or scope that goes with them. Otherwise a non-admin could file a fact as readable by another org, group or set of users, becauseis_visiblegrants org and group access from the metadata alone, and the MCP route has no identity binding that could validate those fields.The required behaviour for AC1 on this route:
- A non-admin requesting SYSTEM or PUBLIC visibility, or GENERAL or AUTOBOT access level, gets 403.
- Every other ownership field a non-admin supplies is dropped, exactly as
drop_ownership_unless_admindoes. - A cross-tenant regression test pins that org, group and share fields never reach storage from a non-admin.
#16705 is implementing this.
- added a commit that references this issue
on Sep 14, 2026 Closure audit against
origin/mainafter today's merge of PR #16705 (vehicle #16746, merge commit1d5a2d472), delivering the refusal contract discussed in the comments above.AC verdict evidence Non-admin gets 403 for SYSTEM/PUBLIC or GENERAL/AUTOBOT (case/whitespace-normalised) through any endpoint; admin can set them met api/knowledge_mcp.py:508-509(mcp_add_to_knowledge_base):if refuses_platform_wide(request.metadata, current_user.get("role")): raise HTTPException(403, ...), checked before thetryso it isn't swallowed;knowledge/ownership_index.py'srequests_platform_wide/_normcovervisibilitysystem/public andaccess_levelgeneral/autobot,.strip().lower()-normalised;drop_ownership_unless_adminstrips every ownership field (OWNERSHIP_KEYS+_OWNER_KEYS), not just owner fields, for a non-admin; admin path callsdict(metadata or {})unchanged. Tests:api/knowledge_mcp_add_policy_16663_test.py::test_a_non_admin_asking_for_platform_wide_reach_gets_403(parametrized),test_an_admin_stores_a_system_fact_as_asked,test_a_non_admin_cannot_file_a_fact_into_another_org_group_or_share(cross-tenant regression test)Every visibility write goes through set_ownermet (already ticked) unchanged from prior audit Ingestion writes org/group ownership met (already ticked) unchanged from prior audit 3/3 met. Kept closed.
Parent: #16654.
Problem
The evidence is on
mainate42be9cb6.PUT /api/knowledge/collaboration/facts/{id}/permissions(api/knowledge_collaboration.py:505) lets any signed-in owner set SYSTEM. Only ORGANIZATION is restricted (:112-118).autobot_shared/scoping/scope_level.py:44-45;autobot_shared/scoping/visibility.py:76-78).autobot-frontend/src/components/knowledge/KnowledgeScopeSelector.vue:29).PUT /api/knowledge/facts/{id}/visibility(api/knowledge_ownership.py:207) writes throughupdate_fact(:250) withoutset_owner, so the Redis visibility indexes go stale.set_ownercall (knowledge/facts.py:599-608) never passesorganization_idorgroup_ids, soget_all_accessible_factsmisses those facts.Owner decision (2026-09-13, on #16654)
Only admins may set SYSTEM, and PUBLIC too, since it behaves the same.
Acceptance criteria
set_owner, so the indexes match the metadata. Tested.Refs #16654