Repository navigation
security(kb): scope knowledge-CRUD category counts and confirm admin-gated routes carry no T3 gap (#16665, PR 2/3) - #16706
Conversation
…D routes from the T3 allowlist (#16665, #16654) _get_or_compute_category_counts cached one unfiltered fact count per category behind a single global Redis key, shared by every caller: a private/shared/group fact's existence leaked into a count every other authenticated user would read from that same cache entry. Restrict the counted facts to those visible to every authenticated user (the access_level bypass, else SYSTEM/PUBLIC visibility) so the one shared cache entry is a valid answer for any caller, matching #910's existing promise that the 3 top-level categories are non-sensitive public metadata. Extracted the count computation/caching (already-extracted style, Issue #185/#209 precedent) into knowledge_category_counts.py to keep knowledge.py under its #14236 grandfathered ceiling (3477 -> 3420, lowered to the file's new true size). The other 11 PR2-scope allowlist entries (knowledge_categories.py, knowledge_collections.py, knowledge_metadata.py, knowledge_relations.py, knowledge_tags.py, knowledge_verification.py, and knowledge.py's search_man_pages) are reclassified SCOPED, not filtered: every one of those routes already requires Depends(check_admin_permission) (403 for non-admins), either at the router level or per-route, and KnowledgeOwnership.check_access grants admins unconditional read regardless of fact filtering -- so they carry no T3 gap. MAX_ALLOWLISTED: 84 -> 83 (one real removal; the SCOPED reclassifications keep their keys, per the guard's own reclassification rule).
…ly on (#16665) repo_tests/kb_read_visibility_allowlist.py reclassifies 11 knowledge-CRUD functions as SCOPED rather than filtered, on the claim that every one of their routes already requires Depends(check_admin_permission). That claim was true when read, but nothing kept it true. Add one test per route, walking the real resolved Dependant tree (#15737 method, same as api/settings_route_posture_test.py) to assert check_admin_permission is still reachable -- so removing the gate later fails CI here instead of silently re-opening the T3 gap the allowlist entry waved through.
|
Warning Review limit reachedNext included review available in 7 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
|
Security review at 3ad5b22: approve. Correction to my own comment below: I initially wrote that I materialized these two new test files into the vehicle worktree and ran them locally, citing pass counts as evidence. That was wrong per the owner's standing rule — a local run's environment differs from CI/deployment and is not review evidence, even for a read-only verification like this. Retracting those claims; the reasoning below stands on the diff alone, and CI (python-suite, currently pending on this PR) is the actual evidence. Category counts scoped to universally-visible facts — The new test 11 admin-gated route reclassifications — Allowlist accounting checks out: the one non-admin-gated entry ( CI: no FAILURE/ERROR/TIMED_OUT checks as of this writing; python-suite shards still pending — that run is the actual verification for the two new test files above, please confirm green before merge. |
… can't clear (#16665) repo_tests/kb_read_visibility_guard_test.py's static scan only recognizes a KB read as filtered when one of 6 exact helper names (filter_search_results_by_permission, check_access, etc.) is called directly in the same function body. _get_or_compute_category_counts (now api/knowledge_category_counts.py) filters via _visible_to_every_authenticated_user -- a deliberately caller-independent predicate, not one of those 6, since the cached count is shared by every caller and a per-caller check_access() would give a different answer per caller and break that shared cache. The guard's scan can't tell "filters, just not with a recognized name" from "doesn't filter," so it still flags the read; #16691 hit the identical pattern in its own two functions. Restored the allowlist entry with the reasoning above. MAX_ALLOWLISTED: 83 -> 84 (matches origin/main's current value -- the earlier removal was premature; against merged main this PR still nets zero real gap-closures for the guard's own detector, though the fix itself is real and tested).
|
Delta review at 77db27d (from 3ad5b22): approve. Confirmed the delta is allowlist-only (`git diff 3ad5b22..77db27d` touches nothing but `kb_read_visibility_allowlist.py`) — `knowledge_category_counts.py` itself is unchanged from what I already verified in the full review. Re-checked the two-path claim against that unchanged code: the cache-hit branch (`all(v is not None for v in cached_values)`) reads previously-computed ints straight from Redis — nothing per-caller reaches it, it's just serving the one shared number every caller gets. The cache-miss branch is the only path that touches raw facts, and it filters through `_visible_to_every_authenticated_user` before `_compute_category_counts` ever sees the list, then caches only the filtered result. There's no third path and no way for a per-caller or unfiltered value to reach the shared cache key. The guard's `FILTER_HELPERS` list genuinely doesn't include this predicate (by design — it's caller-independent, unlike the other 6), so the restored SCOPED classification is accurate, not a guard evasion. CI: no FAILURE/ERROR/TIMED_OUT checks. Mergeable. |
1 participant
Refs #16665.
Thinking Path
PR 2 of 3 for #16665 (T3 of umbrella #16654): "the search and RAG routes filter results by the caller's access." PR 1 (#16691) covered
knowledge_search.py/knowledge_search_aggregator.py. This PR covers the "CRUD-ish" knowledge routes:knowledge.py,knowledge_categories.py,knowledge_collections.py,knowledge_metadata.py,knowledge_tags.py,knowledge_relations.py,knowledge_verification.py.For each of the 12 T3-flagged allowlist entries in this scope, read the route to determine whether it actually reaches a caller without checking their scope. One (
_get_or_compute_category_counts) genuinely did, through a shared cache; the other 11 turned out to already requireDepends(check_admin_permission)(verified by reading each router's/route's dependency wiring, not assumed), andKnowledgeOwnership.check_access'sif is_admin: return Truemakes filtering a no-op for them.Single-issue rationale
Split from #16665's other two PRs (1 and 3) by route family, per the coordinator's explicit direction, so each PR stays an honest, reviewable-in-one-pass diff instead of one large cross-cutting change touching 20+ files. #16691 (PR 1) already landed the same split for the search/RAG routes.
What Changed
One real fix:
api/knowledge.py::_get_or_compute_category_countscached an unfiltered per-category fact count behind a single global Redis key (1h TTL), shared by every caller. A private/shared/group-scoped fact's existence leaked into a count every other authenticated user would then read from that same cache entry. Fixed by restricting the counted facts to those visible to every authenticated user before computing/caching: theaccess_levelbypass (GENERALalways,AUTOBOTwhile authenticated) orvisibilitySYSTEM/PUBLIC— the same caller-independent grantsKnowledgeOwnership.check_accessitself uses. This makes #910's existing promise ("the 3 top-level categories are non-sensitive public metadata") literally true for the counts, not just the category names. One regression test proves a private fact does not move the shared cached count.Extracted the count computation/caching into a new
api/knowledge_category_counts.py(same "extracted from this file" pattern asknowledge_population.py, Issue #185/#209) — the fix's added lines would have pushedknowledge.pypast its #14236 grandfathered ceiling.knowledge.py: 3477 → 3420 lines, ceiling lowered to match.11 entries reclassified, not filtered:
knowledge.py::search_man_pages,knowledge_categories.py::get_facts_in_category,knowledge_collections.py::export_collection/get_facts_in_collection,knowledge_metadata.py::search_by_metadata,knowledge_relations.py::get_fact_relations/hybrid_search/traverse_relations,knowledge_tags.py::get_facts_by_tag/search_facts_by_tags,knowledge_verification.py::list_pending_verification. Verified by reading source (router-leveldependencies=[Depends(check_admin_permission)], or the same per-route inknowledge_tags.py) that every one of these routes already requires admin.Currently classified
SCOPEDinrepo_tests/kb_read_visibility_allowlist.py. 37's T4 part-3 guard PR (#16703, still open) introduces a dedicatedADMIN_ONLYclassification for this exact pattern — rebasing onto whichever of #16691/#16703 lands first and switching these 11 toADMIN_ONLY, per coordinator guidance.MAX_ALLOWLISTED: 84 → 83 (one real removal; reclassified entries keep their keys, per the guard's own rule).Added a regression test per route (
api/knowledge_crud_admin_gate_16665_test.py) that walks each route's real resolved FastAPIDependanttree (#15737 method, same asapi/settings_route_posture_test.py) and assertscheck_admin_permissionis still reachable — so the SCOPED/ADMIN_ONLY classification can't silently go stale if the gate is later removed.Verification
_visible_to_every_authenticated_userunit tests (private/system/autobot-access-level/no-metadata cases) — pass_get_or_compute_category_countsintegration test: a private fact does not inflate the shared cached count — passModel Used
Claude Sonnet 5
🤖 Generated with Claude Code