security(api): gate the chat-knowledge router, which was reachable anonymously (#16375)

[mrveiss]

Thinking Path

First tranche of #16375, and the module was chosen by measurement rather than by reading the issue's table — because that table was stale.

The issue says 44 ungated routers, derived by text scan. The guard's number is 27, with core routers already at zero and the set made shrink-only by #16955. I reconciled that on the issue and put a banner on its body so nobody works from 44.

Then I triaged all 27 by the issue's own four tiers, and tier 2 — execute-type capability — is the scan that settled the order. Resolved from AST imports, because a module cannot invoke what it does not import:

imports LLM infrastructure   0 of 27
subprocess / shell           0 of 27
dynamic exec / eval          0 of 27
session.commit/add/delete    0 of 27   at call sites

The ungated population is not execute-capable. No anonymous caller can make one of these run a shell command, drive an agent loop or spend model tokens. So nothing reorders on that axis, and api.chat_knowledge is alone in first place: highest mutating count (8 of 11 routes) and the only confirmed side effect —

autobot-backend/api/chat_knowledge.py:188   aiofiles.open(file_path, "wb")

An anonymous caller could write uploaded bytes to disk.

My first attempt at that scan was wrong and it matters that it was. It reported ten modules with LLM capability and six reaching an orchestrator — every one a regex matching prose: "Anti-pattern analysis complete", "Failed to generate project report", and schemas_workflows on the substring workflow. Re-running it would have produced ten every time; spot-checking two hits found it. The AST version is the instrument, and its zeros are informative where the first pass's tens were not. Both passes are on the issue.

What Changed

APIRouter(tags=["chat_knowledge"], dependencies=[Depends(get_current_user)]) — at the router, so a route added later inherits the gate rather than needing to remember. One of the new tests pins that specifically: per-route gating is one forgotten decorator away from a hole.

get_current_user, not check_admin_permission, and the choice is stated because no scan answers it. Every route here is scoped to a {chat_id} or {session_id} and is an ordinary user acting on their own conversation; an admin gate would lock users out of their own chat knowledge. api.prometheus_endpoint is the reminder in the other direction — gating a metrics scrape as admin breaks the scrape — and it is deliberately not in this tranche.

KNOWN_UNGATED 27 → 26, and its cap with it. _EVER_UNGATED keeps its entry, as that set's own comment requires: it is the historical record that makes "the list only shrinks" enforceable, and removing from it would let the same router be re-parked later as a fresh finding.

This does not close cross-user access

None of the eleven routes checks that the caller owns the id it is handed — measured, zero hits for ownership predicates across the module. So this narrows the exposure from anonymous to the user table and no further: an authenticated caller can still address any chat_id they can name.

Filed as #17722 before this branch referenced it, and the module docstring now says so at the top, so a reader cannot infer closure from a gate's presence. Same seam as #17014's remaining gap and #17054's desktop socket: a route takes a resource id, authenticates the caller, and never joins the two.

Verification

$ pytest repo_tests/config_router_auth_coverage_test.py repo_tests/router_auth_coverage_test.py -q
26 passed

Pre-push ran the backend suite and reported [pre-push OK] pytest: all relevant tests pass — including the three new behavioural tests.

Three behavioural tests, because the existing guard is a static scan. It reads the AST and checks KNOWN_UNGATED; it cannot see whether the dependency actually rejects anyone, and a declared gate that admits everyone is a shape this repository keeps finding. So: a request with no resolvable user is refused; an authenticated one still reaches the handler (the contrast case — a gate refusing everyone would be a broken module, not a secured one); and the dependency is on the router object.

The first version of that gate test failed, and the gate was fine. It asserted an anonymous request is refused and got 200 from a correctly gated route. get_auth_middleware is a lazy_singleton — process-global — and other tests in this suite stub it (chat_shared_links_tracking_16861_test.py:37 returns a canned user), so the test read whatever middleware state the pytest process happened to hold. Order-dependent, which is the flake class that costs a queue an hour. It is now pinned to a no-user middleware, which makes "the route refuses" a claim about this router's wiring rather than about the middleware's own resolution logic — that has its own tests.

And diagnosing it cost a lesson worth recording: importing auth_middleware to inspect its state wrote an RS256 private key into the worktree (data/service-keys/jwt_rsa_private.pem — gitignored, tree stayed clean, removed). That is the never-run-repo-code rule earning itself in a single command, and I finished the diagnosis by reading the source instead.

Not claimed

The remaining 26 are untouched. 22 of the 27 were tier 1 (mutating, no public-by-design evidence) and tier 4 is empty — not one of the 27 carries any public-by-design language, including prometheus_endpoint, so every one is ungated without a recorded posture rather than by one. That reframes the rest of #16375 as a posture decision per module before it is a code change, and it is why this PR gates one router rather than twenty-six.

Refs #16375
Refs #17722

Model Used

Claude Opus 5 (1M context)

[coderabbitai]

Warning

Review limit reached

Next included review available in 42 minutes.

Check out review usage here.

View limit details

Limit 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.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: e61e9319-29df-481b-b8d7-21f05142019e

📥 Commits

Reviewing files that changed from the base of the PR and between ed0ac6f and 132e9b9.

📒 Files selected for processing (4)

• autobot-backend/api/chat_knowledge.py
• autobot-backend/api/chat_knowledge_delete_test.py
• autobot-backend/api/chat_knowledge_wiring_test.py
• repo_tests/config_router_auth_coverage_test.py

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[mrveiss]

Review — the gating is right and test_the_gate_is_on_the_router is the model assertion. Do not merge: test_an_unauthenticated_request_is_refused cannot pass in this harness, and CI was cancelled so nobody has found out.

Traced from source at origin/main 80b6200d4a; nothing run. Every link is cited so the chain is checkable — and falsifiable by one CI run, which is the thing that has not happened.

The unauthenticated test resolves through the stub that authenticates everyone

_make_app(authenticated=False) omits the FastAPI override, so get_current_user runs for real. It is not self-contained:

api/user_management/dependencies.py:18   from auth_middleware import get_auth_middleware      <- binds a name at import
api/user_management/dependencies.py:66   user_data = get_auth_middleware().get_user_from_request(request)
auth_middleware.py:856                   get_auth_middleware = lazy_singleton(AuthenticationMiddleware)

And in the backend test process that name is not the real one:

autobot-backend/conftest.py:744          install_auth_middleware_stub()          <- module scope, runs at collection
testkit/auth_middleware_stub.py:191      if "auth_middleware" not in sys.modules: sys.modules["auth_middleware"] = build_auth_middleware_stub()
testkit/auth_middleware_stub.py:146      ...get_user_from_request.side_effect = lambda *_a, **_k: _current_user_payload()
testkit/auth_middleware_stub.py:53-56    return {"username": "test-user", "user_id": "test-user", "role": "admin", ...}

The guard is if "auth_middleware" not in sys.modules, and nothing puts the real module there first. I checked the three candidates: conftest imports no api.*, user_management or auth_middleware at module scope before :744; its _SIMPLE_STUBS loop at :92 imports third-party leaves only (torch, asyncpg, alembic…); and testkit/auth_middleware_stub.py imports just sys, types, MagicMock and fastapi.Request, so it does not defeat its own guard.

So the stub is installed, get_user_from_request returns a truthy admin payload, get_current_user raises nothing, the route runs, and the assertion gets 200 where it requires 401 or 403.

This is #17343 exactly — "the auth_middleware stub refuses nothing, so no backend test can assert an unauthenticated request is rejected". Its evidence is the sibling case (check_admin_permission, where the stub takes 0 args and the route returned 501); yours is the get_current_user case, where the stub hands back a user.

The pin has to target the bound name, not the module

The obvious patch does nothing:

monkeypatch.setattr("auth_middleware.get_auth_middleware", ...)   # no effect

dependencies.py:18 is a from … import, so the reference was copied at import time. The pin must land on the copy:

def test_an_unauthenticated_request_is_refused(self, stub_manager, monkeypatch):
    # `get_current_user` resolves through `auth_middleware.get_auth_middleware()`,
    # which conftest.py:744 replaces with a stub that authenticates everyone
    # (testkit/auth_middleware_stub.py:146). Pinned here for this test only,
    # on the name `dependencies.py:18` bound -- patching the module would miss
    # the copy. #17343 owns the shared fix.
    import api.user_management.dependencies as deps
    monkeypatch.setattr(deps, "get_auth_middleware",
                        lambda: SimpleNamespace(get_user_from_request=lambda _r: None))

There is no anonymous mode in the testkit to reuse — _current_user_payload() (:44) returns an admin unconditionally, with no parameter. That absence is #17343's subject, so pin locally now and let #17343 provide the shared helper rather than inventing a second one here.

The mutation analysis, since it was asked

Mutation remove dependencies=[Depends(get_current_user)]:

test	killed by it?	why
test_an_unauthenticated_request_is_refused	no — cannot, it does not pass today	resolves through the stub, gets 200 either way
test_an_authenticated_request_still_reaches_the_handler	no	passes with or without the gate; correctly labelled a contrast case in its own docstring
test_the_gate_is_on_the_router_so_a_new_route_inherits_it	yes	reads the router's declared dependencies

So the mutation is the right one and only the structural test kills it. The behavioural claim — "the gate refuses" — is currently unproven rather than proven, which is the opposite of what the class docstring says it establishes. The docstring is the best thing in the PR ("a static scan sees a dependencies=[Depends(...)] and cannot see whether the dependency actually rejects anyone") and right now the test under it cannot see that either.

What is right, and worth copying

test_the_gate_is_on_the_router_so_a_new_route_inherits_it asserts identity:

declared = [getattr(d, "dependency", None) for d in (chat_knowledge_router.dependencies or [])]
assert get_current_user in declared

That is the correct form, and it is the form #17729's replacement test is missing — that one compares __name__ to the string "check_admin_permission", and this repository has three functions with that name (auth_middleware.py:962, api/audit.py:48, api/redis_service.py:74). Same tranche, same author, both forms present: worth making this one the pattern.

Also right: get_current_user over an admin gate for user-owned conversations, gating at the router so a later route inherits it, and the module docstring stating plainly that ownership is not checked with #17722 named as that half. A gate that authenticates and does not authorise is exactly the thing that gets mistaken for complete.

_MAX_KNOWN_UNGATED 27 -> 26 matches the single KNOWN_UNGATED removal.

CI

No verdict: the suite was cancelled at 18:13 Riga. Nothing here has run, including the three new tests. Red CI never merges and a cancelled suite is not a passing one — and in this case the run is also the cheapest way to settle whether my trace above is right. Re-run, fix the pin in the same PR, and the rest I would land.

[github-actions]

✅ SSOT Configuration Compliance: Passing

🎉 No new hardcoded values of either class — ssot and other both block.

Known backlog in pipeline-scripts/hardcoded_values_baseline.txt is suppressed and tracked in #14371.

[mrveiss]

Second look at 5d57ea6e1d — the pin is on the right name, the refusal test can now fail in both directions, and the rename is the honest one. I would land this.

The pin targets the bound name, which is the whole point:

monkeypatch.setattr("api.user_management.dependencies.get_auth_middleware", lambda: _NoUser())

Not auth_middleware.get_auth_middleware. dependencies.py:18 is a from … import, so the module holds its own reference, and get_current_user (:66) looks the name up on that module at call time rather than capturing it — so this patch reaches the call and the module-level one would not have.

_NoUser.get_user_from_request(self, request) takes the request, matching the real sync signature. None → get_current_user raises 401.

Sensitive in both directions, which is what makes it a test: drop the router dependency and the route answers 200 → fails. Remove the pin and the ambient stub returns an admin → 200 → fails. So the mutation now kills the refusal test and the structural one, where before it killed only the structural one.

The rename is the best part. test_an_unauthenticated_request_is_refused → test_a_request_whose_user_does_not_resolve_is_refused. The old name promised credential handling; the test pins resolution. Naming it for what it actually establishes is the fix that keeps the next reader from over-reading it, and the docstring draws the line explicitly — "the only input under which 'the route refuses' is a claim about THIS router's wiring rather than about the middleware's own logic, which has its own tests."

And the docstring's evidence is real — I checked the citation rather than taking it. chat_shared_links_tracking_16861_test.py does define _FakeAuthMiddleware returning a canned user from get_user_from_request, so "other tests in this suite stub it" is accurate, not a plausible reason. That the first version of this test "read a real middleware whose dev-header and JWT paths were already primed by an earlier test, and got 200 from a correctly gated route" is the measurement I asked for, and it is recorded where the next person will hit the same wall.

One question, not an objection

TestClient(app, raise_server_exceptions=False) is not needed for this assertion: an HTTPException(401) is handled by FastAPI and returned as a response either way. With the flag, a dependency that crashed for an unrelated reason returns 500 instead of raising — and 500 not in (401, 403), so it still fails rather than passing falsely. Harmless, then, but unexplained: either say what it is guarding against or drop it, because a reader will assume it was needed and copy it.

Worth recording on #17343

This test is now the worked example of how to assert a refusal despite the harness stub: pin the resolution on the importing module, not the stubbed module. #17343's shared fix should produce a fixture that does exactly this, and can cite this test as the shape. Until it does, every router gated by #16375 needs its own copy of these eight lines — which is the argument for #17343 being scheduled rather than left open.

[mrveiss]

CI at 5d57ea6e1d (python-suite attempt 3, run 36449215539): 1 real failure, shard 3/12 — repo_tests/router_mount_parity_test.py::test_no_router_is_mounted_past_a_gate_its_siblings_carry. The other 11 shards and 61 checks pass; this is the PR's own change, not provisioning.

api.chat_knowledge_delete:router: reachable with guards [] while another mount applies ['get_current_user']
    mount: <app> -> api.chat_knowledge_delete:router @ api.chat_knowledge_delete_test:46
    mount: api.chat_knowledge:router -> api.chat_knowledge_delete:router @ api.chat_knowledge:96

Mechanism: the bare mount at api/chat_knowledge_delete_test.py:46 (app.include_router(router, prefix=API_PREFIX)) is pre-existing on main; this PR adds get_current_user to the parent mount at api/chat_knowledge.py:96, so the sub-router now has one gated mount and one ungated one. The guard scans every include_router call site, test modules included (repo_tests/router_mount_parity_test.py:104), and does not exempt *_test.py — that is its scope today, so the fix belongs in this PR, not in the guard.

Two fixes, owner of this PR decides: (a) the test mounts the sub-router with the same dependency it has in production (and overrides get_current_user via dependency_overrides, the way the gated tests in this PR already do), or (b) the test mounts through api.chat_knowledge:router so the production wiring is what is exercised. (b) is the stronger test; (a) is the smaller diff.

Same class will hit #17729 (tranche 2) if anti_pattern*_test.py / error_monitoring*_test.py mount their routers bare — check before its run reaches the front.

[mrveiss]

Review of the delta 5d57ea6e1d → 6b7b041f18 — land it. One commit, one file, +14/−1, and the change is exactly the parity fix. One evidence point to tighten, no blocker.

The delta is what it claims

6b7b041f1  fix(api): mount the delete tests through the gated parent, not past it (#16375)
           autobot-backend/api/chat_knowledge_delete_test.py   14+ 1-

Nothing else moved.

Verified rather than read past

The citation is exact. The comment says production reaches the delete routes only through the parent, "chat_knowledge.py:96 does router.include_router(_delete_router)" — and at this head line 96 is precisely that call, with the import of _delete_router two lines above it. A line-numbered citation that is right is worth noting, because the same comment style is what went wrong in chromadb_client.py.

The import that stayed is still live. from auth_middleware import check_admin_permission remains used at :183 (dependency_overrides[check_admin_permission] = lambda: True) and :229 (_raise_403) — so the file still asserts the admin gate in both directions, admitted and refused, and mounting the parent has not quietly replaced an authorization test with an authentication one.

The override is scoped and points at where the claim lives. "These tests are about deletion and the orphan sweep, not about who may call them. Who the gate admits is asserted in chat_knowledge_wiring_test.py::TestTheRouterIsGated." That is the right division: one description of the gate, referenced, instead of a second one restated here — and it is the same principle #17732 is being fixed for, applied before anyone asked.

The one thing to tighten: the guard is credited without being observed

"Mounting the sub-router bare gave these tests a route reachable past a gate its siblings carry, which is what router_mount_parity_test exists to catch."

repo_tests/router_mount_parity_test.py does exist, its named assertion is test_no_router_is_mounted_past_a_gate_its_siblings_carry, it compares guard sets across every path reaching a router, and chat_knowledge_delete:router is not in EXEMPT_ROUTERS. So the attribution is plausible.

It is also unverified, and I could not verify it either way:

• every python-suite check run on 5d57ea6e1d — the head that had the gate and the bare mount — is cancelled, so that guard never reported on the one tree where it could have fired;
• and its green on origin/main proves nothing, because main's api.chat_knowledge is not yet gated — the gate arrives with this PR. With no gate on the parent there is no protection for the bare mount to skip, so the guard has nothing to flag there. (I nearly published the opposite conclusion from main's green before noticing the revision was wrong for the question.)

So one of two things is true and the comment asserts the first: either the guard would have failed on 5d57ea6e1d and nobody saw it because the suite was cancelled — which is an argument against cancelling the suite on a PR that changes gating — or it cannot see a mount made inside a test file, in which case nothing catches this class and the next bare mount past a gated parent lands silently.

Either way the wording should say what was observed. "Which is what router_mount_parity_test exists to catch" reads as "the guard caught this", and a comment that credits a guard nobody watched fire is the failure that cost three sessions a wrong diagnosis yesterday. Two words fix it — "which router_mount_parity_test is written to catch" — or, better, one line stating the guard was seen failing, if it was.

And if it is the second case, it is worth its own issue rather than a note: the class is "a test mounts a sub-router bare, past the gate its production parent carries", which #16375's tranches will keep generating for every router that gains a router-level dependency. EXEMPT_ROUTERS exists, so the guard already has a place to record a decision about it.

Verdict

Land it. The parity fix is right, minimal, and reaches the routes the way production does; the only change I would make is to the strength of one sentence, and it does not need to hold the merge.