Repository navigation
security(slm-api): omit the provider key from the agent LLM config response, and guard the class (#17865) - #17865
Merged
Merged
Conversation
…sponse, and guard the class (#17865) `GET /agents/{agent_id}/llm` previously included the provider API key as a response-model field. It no longer returns it. Omitted rather than masked: a value the client never receives cannot be written back, so a GET-then-PUT round-trip is a no-op for the stored key. This is the same reasoning `public_provider_view` already records on the settings path, and it is why omission beats masking here. `AgentLLMConfigWithKey` is deleted outright. Its only use in the tree was this route, and its own docstring said "for backend only" while it was serving as the `response_model` of a GET -- a comment is not an enforcement mechanism, and a schema that is safe only when used carefully is not safe. The way to stop it being returned is for it not to exist. Server-side code reads the key through the decrypt helper, which now carries an explicit note that its return value must never reach a response model, a log line or an error body. Fixed as a CLASS, not an instance. `no_secret_in_response_model_17865_test.py` walks both backends and fails if any `response_model=` resolves to a schema carrying a stored credential, FOLLOWING INHERITANCE -- the field is usually declared on a parent in another module, which a per-file check would miss. The matcher is deliberately narrow. A first draft matching any name containing "token" flagged 27 distinct fields, nearly all of them LLM token COUNTS (`input_tokens`, `llm_max_tokens`, `tokens_spent`). A guard that fires mostly on correct code gets switched off, so breadth would have cost the guard rather than bought coverage. Auth tokens are excluded for the same reason -- returning `access_token` is a login endpoint's job. `*_ref`, `*_id`, `*_encrypted` and `*_hash` are excluded because naming a pointer or a ciphertext is what a safe response does. One waiver, carrying a written reason rather than a suppression: `MFASetupResponse.secret`. TOTP enrolment needs the shared secret, once, and that handler resolves the caller's own account. A waiver that stops matching a real site fails `test_every_waiver_still_describes_a_real_site`, so it cannot rot into a standing exemption. Tenancy scoping for this route is NOT part of this change. The `agents` table carries no company, tenant or owner column -- only a nullable `created_by` -- so a predicate written against it would pass for every row where it is NULL, which is worse than none because it reads as enforcement. Tracked separately; this change does not depend on it. `schemas.py` loses a line, so its size ceiling is ratcheted 2181 -> 2180 in both recorded places. The ratchet only turns down, and an unlowered ceiling re-licenses the line just cut. Verified: 22 tests pass against this tree, and the guard fails against the previous one, reporting the exact site.
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe agent LLM endpoint now returns provider, endpoint, model, timeout, temperature and token limit without returning the stored API key. The OpenAPI contract reflects this response. An AST-based test scans backend response models for secret-bearing fields. ChangesAgent LLM key omission
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🟡 Moderate · up to The agent endpoint now omits its stored key, but the new guard can miss credential-bearing response models in two supported exclusion cases. Strengthen the guard before relying on it to catch future exposures. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The response change removes provider credentials while preserving stored keys and the settings used by a known downstream consumer. The new regression guard has exclusion-resolution gaps that could allow future credential disclosures to escape detection. No current production disclosure caused by those gaps was demonstrated. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
Contributor
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
Auto-regenerated (py3.14) to match the backend and/or SLM backend schema so the required verify-generated-types gate(s) pass. Triggered by auto-fix-generated-types.yml.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 9542f8a1-886b-48f6-aba9-2184c1dea689
⛔ Files ignored due to path filters (3)
autobot-slm-frontend/src/types/generated/api.tsis excluded by!**/generated/**repo_tests/python_file_size_ratchet_baseline.pyis excluded by!repo_tests/python_file_size_ratchet_baseline.pyscripts/python_file_size_known_large.pyis excluded by!scripts/python_file_size_known_large.py
📒 Files selected for processing (4)
autobot-slm-backend/api/agents.pyautobot-slm-backend/models/schemas.pyautobot-slm-frontend/openapi.jsonrepo_tests/no_secret_in_response_model_17865_test.py
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
…that uncovers (#17865) CodeRabbit raised seven findings on the guard. All seven were right, and together they say one thing: the detector resolved a response model by only one of the three routes a field can reach it by. 1. GENERIC SUBSCRIPTS. `_model_names` now recurses through `ast.Subscript`, `Tuple`, `BinOp` and `Attribute`, so `DataResponse[SecretCreatedData]` is seen. Reading only `ast.Name` made FIVE existing secret-bearing routes invisible -- the finding paid for itself immediately. 2. NESTING. A field annotation is an edge: `LLMConfigResponse` reaches `LLMProviderConfig.api_key` through `config.providers`, not through a base class. `_reached_fields` now walks bases AND annotations. 3. PARSE FAILURES ARE REPORTED, NOT SKIPPED. `REACH.completed()` compares a count to a floor of 4600, so unparseable modules vanished into the margin. `test_every_discovered_module_parsed` compares discovered against parsed directly. "Could not look" reported as "found nothing" is the exact failure this guard family exists to prevent, and the guard was committing it. 4. ROUTE-SCOPED WAIVERS. `_WAIVED` keyed on (model, field) exempted every future route reusing the model, whatever its authorisation posture. Now keyed on (model, field, METHOD, path), with a fixture proving the waived model IS still reported on a different route. 5. CONTRAST PAIRS THROUGH THE WHOLE DETECTOR. The old positive fixture called `_secret_fields` directly, so route discovery and reachability had no positive case at all -- the repository only ever supplies the negative. Seven fixture trees now drive `_build_index` + `_violations` end to end, each with its clean twin. 6. SUFFIX MATCHING. `password` and `secret` were anchored, missing `webhook_secret` and `service_password`. Unanchored; the exemption pattern is what keeps `password_hash` and `secret_type` out, and it does that job either way. 7. Module identity: `_build_index` now merges rather than overwrites same-named classes, so a collision widens the reachable set instead of replacing it -- the safe direction. WHAT THE WIDENED DETECTOR FOUND, AND WHY IT IS A BASELINE NOT A WAIVER. Ten pre-existing sites surfaced. Two were plainly fine and the exemption pattern now covers them (`masked_secret`, a mask; `secret_staleness`, a metric). The remaining eight are frozen in `_UNAUDITED_BASELINE`, which is deliberately NOT the waiver list: _WAIVED audited, intentional, here is the reason _UNAUDITED_BASELINE present before the guard, NOT YET AUDITED Conflating those is how a baseline becomes a permanent exemption, so they are separate structures with separate tests. Each entry carries its real status: five `secrets.py` routes return `Dict[str, Any]`, an untyped shape this guard cannot see into -- that unauditability is itself the finding -- and two `llm_config.py` routes are masked today and being moved to omission by PR #17846, which owns that file and is not touched here. The baseline only shrinks: `test_the_unaudited_baseline_only_shrinks` fails on an entry that stops firing, and its size is pinned. 42 tests pass.
…text, and split the baseline out (#17865) A `code-reviewer` pass found three things in the guard and one bug in the production change. All four are fixed here. THE BUG. My new docstring on `_decrypt_api_key` left the old one orphaned below it as a dead string expression. Merged. THE BLIND SPOT, which is the one that matters. `_BACKENDS` covered only the two backend trees, so a response model DEFINED elsewhere resolved to nothing and was reported clean -- `_reached_fields` returns [] for a name it never indexed, which is indistinguishable from a model with no secrets. `UserListResponse` is routed from a backend and declared in `autobot_shared/user_management/schemas/user.py`, so it was passing on invisibility. That is exactly the failure this file's own docstring warns about, committed by the file itself. `autobot_shared` is now scanned (4707 -> 5057 modules) and `test_every_routed_model_resolves_or_is_a_known_builtin` fails on any routed name the index has never seen. CIPHERTEXT AND HASHES ARE NO LONGER EXEMPT. `_encrypted` and `_hash` sat in the exemption list, which contradicts the guard's purpose: handing a client a stored ciphertext or a password hash is an offline-attack gift. Measured before changing -- un-exempting them adds ZERO violations against the current tree, so it was pure risk for no convenience. Added the credential names the matcher simply lacked: `signing_key`, `hmac_key`, `encryption_key`, `ssh_key`, `access_key`, `passwd`, `api_token`, `webhook_token`, `bearer_token`. A ROUTE FLOOR, because the module floor cannot see this failure. `REACH` counts modules: if route discovery broke and found zero routes, every file would still parse, the floor would still clear, and the violation test would pass over an empty set. `test_route_discovery_did_not_collapse` pins >=2400 of the 2766 sites found. WHAT THE WIDER SCAN UNCOVERED. Ten more pre-existing sites, checked rather than assumed: they are REAL NESTING, not the name-collision false positive I first suspected. `LLMConfigResponse` nests `LLMConfig`, which declares `anthropic_api_key`, `brave_search_api_key` and `openai_api_key` -- so the nested traversal added in the previous commit is doing its job. Plus `TerminalSessionCreateResponse.ssh_keys`, another `Dict[str, Any]` this guard cannot see into. The baseline goes 8 -> 18, and the frozen count is now a named constant whose comment states the only legitimate reason to raise it: a WIDENED DETECTOR newly seeing pre-existing sites, never a new violation. All ten predate the guard. BASELINE SPLIT INTO ITS OWN MODULE. The guard file hit 650 lines against a 600 ceiling, and the ceiling is split, not raised. It also buys what the review asked for: a change to an exemption is now a diff to `no_secret_in_response_model_baseline_17865.py` and nothing else, so granting one cannot ride along inside an unrelated change to the detector. 563 and 118 lines respectively. `test_every_exemption_carries_a_real_reason` keeps the two structures from blurring -- it requires a real reason on both and rejects a baseline entry claiming to be "audited", since that would be a waiver wearing a baseline's key. 45 tests pass.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: c1a921de-d0a8-48e1-83dc-c8d7754dde13
📒 Files selected for processing (3)
autobot-slm-backend/api/agents.pyrepo_tests/no_secret_in_response_model_17865_test.pyrepo_tests/no_secret_in_response_model_baseline_17865.py
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
… where it bites (#17865) Two more CodeRabbit findings, both correct. THE COLLISION FIXTURE I SHOULD HAVE WRITTEN. The previous commit claimed `_build_index` "merges rather than overwrites same-named classes, so a collision widens the reachable set instead of replacing it -- the safe direction". That was an assertion in a commit message with no test behind it, which is the kind of claim this repository treats as unsupported. Now tested end to end: two modules each defining `Cfg`, one carrying `api_key` and one clean, with a route returning `Cfg`. Files are walked in sorted order, so the CLEAN definition is parsed second -- exactly the order in which last-writer-wins would discard the secret and report the route clean. Verified the fixture is load-bearing rather than merely passing, by running the detector against both semantics on the same tree: MERGE (current): REPORTED OVERWRITE (the bug): MISSED Paired with `same-name-collision-clean`, identical but with `api_key_ref` in place of `api_key`, so the positive cannot be passing merely because collisions are always reported. THE ROUTE FLOOR WAS TOO LOW TO BITE. `>= 2400` against a live 2766 leaves room for a matcher failure to drop 266 sites and still pass -- a material discovery collapse reported as success, which is the exact shape the floor exists to catch. A floor that cannot fail before a third of the tree stops being reached is decoration. Raised to 2700. 47 tests pass.
…ls (#17865) The guard is syntactic, so it resolves a model's fields from their declarations. `response_model_exclude` removes a field from the WIRE and leaves the declaration standing, which gave the guard two problems and the second is the dangerous one: * a correctly-fixed route is reported as unaudited for ever, so its baseline entry quietly becomes permanent * and a route that LATER DROPS its exclusion is excused by the baseline entry recorded while it was still safe -- the protection disappears and the guard stays green Found because #17846 fixes two `llm_config.py` routes exactly this way: _NO_PROVIDER_KEYS = {"config": {"providers": {"__all__": {"api_key"}}}} @router.get("", response_model=LLMConfigResponse, response_model_exclude=_NO_PROVIDER_KEYS) I had told that PR's author the two baseline entries would go stale when it landed. They will not -- the declaration survives -- and checking why surfaced the false-negative path instead. `excluded_field_names` collects the leaf strings from the exclude expression, following a module-level constant by name since that is how #17846 writes it, and dropping the `__all__` marker. A field named there never reaches a client, so the route is clean. Four fixtures, as two pairs, because a suppression feature that suppresses everything also looks correct: excluded-nested-is-clean nested dict form -> clean exclusion-dropped-is-reported same route, no exclude -> REPORTED excluded-via-module-constant-is-clean `_NO_KEYS` by name -> clean excluding-the-wrong-field-still-reports excludes `name` -> REPORTED The second is the one that matters: it is the regression this feature must not hide. The two `llm_config.py` baseline entries are deliberately NOT removed here. #17846 has not landed, so they still fire. When it does, `test_the_unaudited_baseline_only_shrinks` catches them as stale and they get deleted -- which is that test doing real work rather than theoretical work. 51 tests pass. The fixture trees move to their own module: the guard file crossed the 600 line ceiling again and the ceiling is split, not raised. It also separates the detector from its evidence, so a change to what the guard DOES and a change to what it is TESTED AGAINST are different diffs. 509 and 193 lines.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 8bfbebe0-9899-41b0-aaee-5cff224dd6b5
📒 Files selected for processing (2)
repo_tests/no_secret_in_response_model_17865_test.pyrepo_tests/no_secret_in_response_model_fixtures_17865.py
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
1 of 5 tasks
mrveiss
added a commit
that referenced
this pull request
Oct 3, 2026
…d stop the cross-check naming a cause it cannot establish (#17872) (#17882) * test(gates): compare each gate's two ceiling records for EQUALITY, not one direction (#17872) The duplication is NOT the defect, and #17872's original ask was wrong --------------------------------------------------------------------- I filed that issue asking for one record to be derived from the other. Reading the files says not to. Both baselines document the two copies as a mechanism: python: "neither copy is derived from the other at runtime ... a list only ever compared against itself can drift anywhere ... no ceiling can be raised by editing one file alone" shell: "Two copies is the point, not an accident ... The mirror is what makes a shrink irreversible: the gap between a lowered hook and an unlowered mirror is exactly the lines just cut" The hook's copy is anchored to real file sizes by `audit_ceilings`; the baseline is anchored by the ratchet test. Deriving one from the other collapses two independent anchors into one and destroys exactly the protection that makes a raise detectable. Derivation would have been a regression dressed as cleanup. The real defect: the comparison was asymmetric on the Python side ---------------------------------------------------------------- The mechanism only holds if the two records are compared BOTH ways. They were not. `test_no_ceiling_may_be_raised` fires on `ceiling > baseline` alone, and `test_no_entry_may_be_added` compares keys in one direction and skips `rel not in RATCHET_BASELINE` silently. There was no equality assertion at all. So raising the BASELINE alone passed every check -- the direction that actually happened, on #17725: I edited the baseline, confirmed the new number was in the file, and the hook kept enforcing the old one. An edit that looks applied and changes nothing. The shell gate has had the equality assertion all along ------------------------------------------------------- `shell_file_size_ratchet_test.py` asserts `hook.KNOWN_LARGE == RATCHET_BASELINE` outright, names both files, and asserts no direction. Fourth time in this batch that the shell copy was the correct one. Asserted for BOTH pairs rather than only the broken one: the defect is two hand-kept copies per gate with an asymmetric comparison, and fixing one gate leaves the identical arrangement one directory away with nothing pointing at it. The data-source difference -- 479 entries loaded from a module versus 10 declared inline -- is a parameter, not an obstacle: the hook is the authority either way. Demonstrated, not argued ------------------------ Raised one baseline entry alone, 713 -> 763, hook untouched: test_no_ceiling_may_be_raised PASSED <- the existing check misses it test_the_two_records_are_equal FAILED <- this one catches it Both green again on revert. The message names both values and asserts no direction, because lowering a baseline used to fail a test called "no ceiling may be raised" -- sending the reader after a raise that never happened. A wrong diagnosis costs more than a vague one. Touches no baseline file and no gate, so it does not collide with #17865. Refs #17872, #14236, #17353 * test(gates): drive the disagreement message on synthetic records (#17872) The issue's last criterion asks for a test that proves the one-sided edit fails "with the message asserted too, since the message is the whole deliverable here". The first commit did not deliver that. I had mutation-verified the one-sided edit by hand -- raise the baseline alone, watch this file go red -- but a check I ran once in a worktree is not a check the repo keeps running, and the AC asks for the latter. Ticking it on my own transcript would have been evidence of the wrong kind. The obstacle was that a message only ever produced by a real disagreement in the checked-in records cannot be asserted on without breaking the tree. So the comparison moves into `_disagreement_report(known, recorded, ...)`, which takes its two records as arguments; the real pair feeds it the checked-in records, and three new tests feed it synthetic ones: * a one-sided edit (1050 vs 1028, the exact #17725 shape) must name the file, carry BOTH values in (hook, baseline) order so the reader knows which file to edit, name both records, and headline the symmetric fact rather than a direction -- the old failure said "no ceiling may be raised" about an edit that raised nothing; * a key in one record only, BOTH directions -- the Python key check covered one and skipped the other in silence; * agreeing records produce no report at all. Without that contrast case a reporter that always complains would pass both tests above. Mutation-verified, each mutation confirmed applied by grep before trusting the result: swap (hook, baseline) order -> 1 failed (the both-values test) drop only_in_baseline -> 1 failed (the baseline-only case) reporter always returns "" -> 3 failed (both failure tests + ...) restored, md5 matches -> 6 passed * test(gates): stop the cross-check naming a cause it cannot establish (#17872) Finding 1 of the issue, which the first two commits left standing: a lowered baseline failed a test called `test_no_ceiling_may_be_raised`, and nothing had been raised. The reader is sent after a cause that did not occur, which is worse than being told only that something disagrees. `ceiling > baseline` is produced equally by a raised ceiling and a lowered baseline -- the predicate cannot distinguish them -- so neither the name nor the message claims one any more: test_no_ceiling_may_be_raised -> test_no_ceiling_may_exceed_its_baseline "ceilings raised (now, baseline)" -> "the hook's ceiling exceeds its baseline (hook, baseline) ... which record moved is not established here" The predicate itself is unchanged and still one-directional on purpose. The reverse direction is the parity test's job; writing it in both places would make two records of one check, which is the shape this issue is about. Mutation-verified in both directions, which is also what proves the new file is load-bearing rather than redundant: hook ceiling 910 -> 911 both tests fail, both reporting (911, 910) baseline 910 -> 960 the renamed test PASSES; only the parity test fails, reporting (910, 960) -- the #17725 direction both files restored, md5 matches, 37 passed One more thing the reviewer should see: `python_file_size_ratchet_test.py` was at 589 of the 600-line hard limit with no KNOWN_LARGE entry, and my first version of this change took it to 608. Adding an entry to the hook's record to accommodate that would have been raising a ceiling to fit a docstring. So the rationale moved into the parity test's docstring, where there is room, and this file keeps a one-line pointer and 5 lines of headroom. * test(gates): the parity guard had the defect it was written to catch (#17872) Review on #17882 found it, and it is the third variant of the same class tonight -- the first one inside a guard built specifically to close that class. `test_the_two_records_are_equal` passed when BOTH records were empty, because `{} == {}` is True. Ask what that PASS licensed: "the two ceiling records agree", on the strength of having read neither. Measured before fixing, emptying only each mapping so the result isolates emptiness rather than a broken module: before 3 failed, 34 passed all three in python_file_size_ratchet_test.py; this file PASSED after this file FAILS, naming both records So the suite was not blind -- test_recorded_ceilings_match_the_files_today and two others do fail on an emptied record -- but a guard has to fail for its own reason, and this one reported agreement. A non-empty assert, deliberately NOT a declared floor ----------------------------------------------------- Both records only ever shrink; the hook's header says so. A static floor near today's size fails the first time a file is legitimately split, and a floor an order of magnitude below it is the number-chosen-by-feel that `declare`'s own docstring names as that mechanism's recurring failure. A floor needing to be lowered every time the ratchet succeeds is an incentive against doing the work. Nor is partial degradation reachable: each record is a literal dict run by `exec_module`, so a broken loader raises rather than returning a subset. Empty is the only silent degradation, and empty is what this catches. The reasoning is in the code so it is not "simplified" in either direction, and it says what would reopen the floor question -- a loader that parses or filters. The tree-anchored direction stays where it already lives: every file over MAX_LINES must be recorded, owned by test_recorded_ceilings_match_the_files_today. Asserting it here too would make two records of one check. Mutation-verified both ways: both python records emptied -> 1 failed, message names both records reach check always returns "" -> 3 failed; the contrast case still passes restored -> md5 matches, 56 passed `test_populated_records_produce_no_reach_failure` is that contrast case: without it a reach check that always complains would pass the three failure tests -- the same hole one level down. * test(gates): one owner for ceiling equality, and two files per comparison (#17872) Review found this PR's own thesis turned on itself: parametrising over "shell" made a SECOND record of a check `shell_file_size_ratchet_test.py` already performed -- `test_the_two_copies_of_the_ceilings_agree` asserted `hook.KNOWN_LARGE == RATCHET_BASELINE` for that gate already. Why I missed it is worth recording: Python has NO equality test (31 `def test_` in its ratchet file, none asserting equality -- that absence is why #17872 was filed), and shell had one. Reasoning about the file I was editing was right for Python and silently wrong for shell, so parametrising generalised the gap along with the fix. I had asked exactly this question of the tree-anchored direction and declined to duplicate it; I never asked it of the shell file. The old shell test is removed and this file now owns equality for both gates. Mine is strictly stronger, and that is measured rather than asserted: shell hook ceiling 818 -> 800 (one side only) -> [shell] fails: disagreeing (hook, baseline) {'scripts/pr-preflight.sh': (800, 818)} shell hook record emptied -> [shell] fails the reach check, naming scripts/check_shell_file_size.py The deleted test said something neither of mine did -- the gap between a lowered hook and an unlowered mirror "is exactly the lines just cut, and it is spendable". That is what a disagreement MEANS, so it moved into the failure message a reader actually sees, not into a comment. Also found while in here: equality passes trivially if both halves of a pair resolve to ONE module, which is precisely "a list only ever compared against itself can drift anywhere" -- the baselines' own warning, reachable through my own fixture. Two files and two objects are now asserted before the comparison. Smaller items from the same review: * `_empty_record_failure` was 34 lines of which ~8 were body; the rationale moved to the module docstring. Longest function in the file is now 29. * the `_PAIRS` comment quoted "479 entries" and "10 inline". Both shrink by design, so both would go stale -- dropped, for the same reason a numeric floor was rejected. * `test_no_ceiling_may_exceed_its_baseline`'s message said "Lower BOTH in the same commit", which is wrong for ITS direction: when the hook exceeds the baseline, lowering both RECORDS the raise instead of undoing it. It now says lower the hook to its baseline, or find the raise that put it above. Left alone deliberately, with the reason recorded in the module docstring so it is not re-derived: both gates' exceed-direction tests are logically subsumed by this equality, but each gives the common direction a specific diagnosis, and the redundancy is pre-existing and symmetric rather than introduced here. 55 passed (56 before, minus the deleted duplicate). Both gates exit 0.
4 tasks
mrveiss
commented
Oct 3, 2026
mrveiss
left a comment
mrveiss
left a comment
Owner
Author
There was a problem hiding this comment.
Reviewed at behind=0 against origin/main, read-only via git show on the PR branch; no tests run. Approving the change. One real reach gap in the guard, filed as #17899 rather than held here, and one precision point on the title.
The security fix is right, and right in the way that matters
AgentLLMConfigWithKey is deleted, not left for reuse — and the comment in its place is the lesson: "A schema that is safe only when used carefully is not safe." The handler no longer decrypts at all (_decrypt_api_key is gone from the path, not just the field), so there is no value to omit by accident. _decrypt_api_key's own docstring now says server-side only, which is where the next caller will read it.
The docstring's honesty about what this does NOT fix is the strongest part of the diff. It states that the endpoint is still not ownership-scoped and cannot be here — the agents table has only a nullable created_by, so a predicate against it would pass for every NULL row, "which is worse than none because it reads as enforcement". That is a correct refusal to ship a check that looks like authorisation, recorded at the place someone would try it.
Omitting rather than masking, with the GET-then-PUT reasoning, matches public_provider_view on the settings path — the same trade, decided the same way.
The guard, against the three questions worth asking of one
Does it reach the response path or only the model? The response path. It indexes response_model= at the route decorator, keyed by method and path, and honours response_model_exclude=.
Does it follow the shape the original defect used? Yes, and this is what I most expected to be missing. The vulnerable model was AgentLLMConfigWithKey(AgentLLMConfig) — a subclass adding the field — so a guard reading only a named class's own fields would have missed it. _build_index resolves inheritance and nesting, across modules.
What would a PASS license? It has a declare(..., floor=4950) reach floor, DETECTOR_FIXTURES as contrast cases, and a parse failure is collected rather than skipped — the comment at :233 says a skip the floor absorbs is exactly the bypass. Good.
It guards the class, not one field. _SECRET_FIELD_DENY covers seventeen name families; nothing pins llm_api_key.
Two details I would highlight to anyone copying this:
_encryptedand_hashwere un-exempted during review, with the argument that ciphertext and a password hash are credential material and handing either to a client is an offline-attack gift — and that un-exempting them costs zero violations against the current tree. A change justified by measuring its cost rather than by how it sounds.- The waiver record is keyed by ROUTE, not by model, because a model-keyed waiver would exempt every future route reusing that model, including one with a different authorisation posture. That is the right key and the reason is written down.
The gap: a pass-through model is invisible to it — #17899
A model that declares no fields and allows extras has no names to match, so its route passes while the model permits anything in the response:
class LLMConfigResponse(BaseModel): # schemas_agent.py:793
model_config = {"extra": "allow"}
@router.get("/config", response_model=LLMConfigResponse) # api/llm.py:64grep -i extra over the guard returns one unrelated docstring word, so this is a shape the matcher cannot see rather than an exemption with a reason.
This PR's own baseline is the evidence it is not hypothetical: four entries name api_key, openai_api_key, anthropic_api_key and brave_search_api_key on that exact route — field names the model does not declare. Someone saw them in the endpoint's real output; the detector could not. The record and the detector disagree, and the record is the one with evidence behind it.
Suggested shape in #17899: a pass-through reports unverifiable rather than clean — a third verdict, because "cannot say" must not read as "does not".
Smaller: 8 of 19 baseline entries are a to-do, not a waiver
They read "Not yet audited. Needs the call site checked for masking or omission." That is honest, and it is also indistinguishable in the record from a reasoned exemption like the TOTP one two lines above ("the shared secret IS the deliverable"). The file cites only #17846 and #17865, so nothing tracks them. Please point those eight at #17899 — a to-do that cites an issue is traceable; one that does not becomes a reasoned waiver by age.
Title precision
Routes with no response_model= are skipped (if "response_model" not in kws: continue), so a handler returning a raw dict or a JSONResponse is outside this. The guard's own docstring is exact — "No FastAPI response_model may carry a stored credential" — while the PR title says "guard the class", which reads wider than what is enforced. Either narrow the title or state the limit in the body; the docstring already has it right.
Verdict: this should merge. The endpoint fix is correct and complete for the disclosure it names, the guard is a large net improvement, and both things I would change are follow-ups rather than blockers.
On the form of this review, which corrects something I asserted earlier today. I tried to submit it as an APPROVE and GitHub refused: "Can not approve your own pull request." Every session here pushes as mrveiss, so no review on any PR in this repo can ever reach APPROVED — and CLAUDE_REVIEW.md:302 already says so: "PR authors cannot self-approve — post a detailed review comment instead."
So a COMMENTED review is the sanctioned mechanism, not a sloppy one. This morning I corrected my own claim that #17887 was "approved by two reviewers" — the correction was right that reviewDecision said REVIEW_REQUIRED, and wrong in what it implied: I wrote as though peers had been loose with the word, when the platform forbids the act and the repo documents the substitute. A reviewer saying APPROVE in a COMMENTED review is following the rule, and reviewDecision is not available as the state of record here. What stands is the narrower version: a session's verdict and a PR's merge-gate state are different objects — which is true, and says nothing against the people who gave verdicts.
mrveiss
added a commit
that referenced
this pull request
Oct 10, 2026
…backend.llm subtree (#18193) (#18219) * test(security): GET /config waivers record the operator-set key leak, and every unaudited response-model site cites #17899 (#17899) * test(guards): make exclusion matching path-aware and per-module (#17865) Two CodeRabbit findings, and both are FALSE NEGATIVES I introduced. The exclusion support added in the previous commit was meant to remove false positives; done wrong it gave the guard two ways to hide a real leak, which is strictly worse than the noise it was fixing. 1. PATH-AWARENESS. `response_model_exclude` is path-scoped: `{"providers": {"__all__": {"api_key"}}}` excludes `providers[*].api_key` and says NOTHING about a top-level `api_key` on the same model. I flattened it to bare names, so the nested exclusion suppressed the top-level field too. `_reached_paths` now returns full paths -- inheritance does not extend a path, nesting does -- and the exclusion parser builds paths, treating `__all__` as the list-item segment and honouring `{"field": True}`. 2. PER-MODULE CONSTANTS. `consts` was one dict for the whole tree, so a route using `response_model_exclude=NAME` whose own module does not define NAME resolved it from whichever module did -- importing an unrelated module's field names into this route's excluded set. Reset per file. Three fixtures, each a pair: nested-exclusion-does-not-cover-the-top-level-field -> REPORTED both-paths-excluded-is-clean -> clean exclusion-constant-from-another-module-does-not-resolve -> REPORTED A NOTE ON METHOD, because it is the actual lesson here. Four of my edits in this file were `str.replace` calls without an assertion, and they SILENTLY DID NOTHING -- the anchor had already been changed by an earlier edit. The worst was `_violations`, which went on comparing a leaf NAME against a set of PATHS. That comparison is always False, so every exclusion was disabled while the parser, the paths and the fixtures all looked correct in isolation. The suite reported green at one point purely because the feature was inert. A replace that does not match is indistinguishable from one that does, unless you assert. Every edit in this commit asserts its anchor. 54 tests pass. (cherry picked from commit 481aeead97a809b3e30aebb2a7c1bc4ed05d8b12) * test(guards): the staleness check disagreed with the violation check (#17865) the prediction I made about it was wrong twice and the second wrong answer is what found the bug. I said the two `llm_config.py` baseline entries would NOT go stale, because the guard is syntactic and `response_model_exclude` leaves the field declaration standing. Then I taught the guard to read the exclusion, which should have made them stale. After merging main, all 51 tests still passed. The exclusion reader was working -- it extracts `api_key` from `_NO_PROVIDER_KEYS = {"config": {"providers": {"__all__": {"api_key"}}}}` on both routes. `test_the_unaudited_baseline_only_shrinks` was wrong: live = {... for field in _reached_fields(r.model, idx)} # no filter It computed "still firing" from reachability alone while `_violations` filters on `route.excluded`. Two codepaths answering the same question differently, so a baseline entry for a field that a route had since started excluding stayed live for ever and could never age out. A ratchet that cannot notice a fixed site is not a ratchet. Now computed the same way a violation is. It immediately reported both entries stale, which is the mechanism doing its job rather than being asserted to. _BASELINE_FROZEN_AT 18 -> 16 Deleted rather than left standing: the two routes are genuinely fixed, and a stale entry is a standing exemption for the next route that lands on the same shape. The three `anthropic_api_key` / `brave_search_api_key` / `openai_api_key` entries on those same routes REMAIN -- the exclusion names only `api_key`, and excluding one field is not evidence about three others. 51 tests pass. (cherry picked from commit 189448dbd4efe670aaa31d3564572b3dd7389dd8) * test(guards): contrast cases for path-aware exclusion and per-module constants (#17865) * security(llm): GET /api/llm/config redacts credentials in the nested backend.llm subtree through the canonical redactor (#18193) SECURITY-SENSITIVE: API response redaction; review on Opus. GET /api/llm/config returned the whole backend.llm subtree, so an operator-set provider key at backend.llm.cloud.providers.<p>.api_key and endpoint URLs with user:pass@ userinfo reached any authenticated caller in full. - secret_redaction.redact_nested: recursive (dicts, lists) redactor in the canonical module, BROAD policy, masks like redact_value, scrubs URL userinfo and credential query params, leaves non-credential values and types alone. - api/llm.py: the handler returns redact_nested(get_llm_config()); auth, response model and shape are unchanged. - REDACTION_BOUNDARY.md: documents the API-response boundary caller. - Tests: TestClient test through the real get_llm_config path (fails on the parent) plus redactor unit tests. - no_secret_in_response_model baseline: the four GET /config waivers state the fixed verdict. Closes #18193 * security(llm): /current is redacted too, and redact_nested masks credential URLs, scans plain strings and threads BROAD into query params (#18193) SECURITY-SENSITIVE: API response redaction (second review round); review on Opus. - GET /api/llm/current returned the same unredacted backend.llm tree under "config"; it now goes through redact_nested like /config. - redact_nested: a URL-valued string under a credential name (webhook secret) is masked whole; only URL-shaped names (db_url) keep host with userinfo scrubbed. Other strings are content-scanned (sk-, JWT, inline user:pass@). Tuples and sets are walked; unknown non-scalar types fail closed. - redact_url_credentials/_redact_credential_query_params take a policy (default PRECISE, so every existing caller is unchanged); redact_nested passes BROAD (apikey, access-token, auth, X-Amz-Signature). Only credential pairs are rewritten in place, so a credential-free URL is byte-identical. - redact_url_userinfo reads the port inside the try (a non-numeric port failed closed instead of raising a 500). - exempt_counts docs now name both callers (template export, API config readout). Census entry points include redact_nested (#18205 convergence). Refs #18193 * security(llm): GET /api/llm/config is admin-only, as decided on #18193 (#18193) SECURITY-SENSITIVE: authorization change on a config readout; review on Opus. Owner decision on #18193: redaction AND admin-only. The route now depends on check_admin_permission (the dominant gate across api/llm.py and api/) instead of get_current_user; the redact_nested pass and the response shape are kept as defence in depth. /current and other routes' auth are unchanged. Tests: a non-admin gets 403 (fails on the parent), an admin gets 200 with the redacted body and the positive controls. Refs #18193 * security(llm): GET /api/llm/current returns only the active model and provider; the full tree stays admin-only on /config (#18193) SECURITY-SENSITIVE: response slimming on an authenticated route; review on Opus. Owner decision on #18193. /current no longer returns the backend.llm tree to anyone: "config" stays in LLMCurrentResponse (the generated frontend type is unchanged) but is always empty. model and provider are derived as before. Admins read the redacted tree on /config. llm.py shrank by 2 lines, so its size ceilings are lowered to 1080 in both records. Refs #18193 * docs(api): /llm/current returns only model and provider, and /llm/config is admin-only and redacted (#18193) * fix(llm): GET /api/llm/current reports the active model and provider, not the default (#18215) /current read config.get("model"/"provider") on a dict whose only keys are "ollama" and "unified", so it always returned the default model and "ollama". config.model_config.resolve_active_llm is now the one place that decides "active", mirroring the Settings panel: unified.provider_type picks local or cloud, that side's provider names the entry, its selected_model (or model) is the model; local ollama reuses the already-resolved get_selected_model value. The default constant is returned only when nothing is selected. The response stays {model, provider, config: {}}. llm.py shrank, so its size ceilings are lowered to 1078. Closes #18215 * fix(llm): /current's cloud fallback and default-model test are explicit (#18215) A cloud provider with no selected model now reports "" (SettingsPanel.vue shows "Not selected" there) instead of the default Ollama model under a cloud provider name. The default constant still stands in for local ollama only. The default-model test is hermetic: it clears AUTOBOT_DEFAULT_LLM_MODEL and the cached config.llm.default_model, the only inputs get_selected_model reads. Refs #18215 * fix(redaction): THREAT_MODEL anchors follow redact_text/redact_mapping, and api.ts carries the new /config docstring (#17899) Inserting redact_nested moved redact_text to :363 and redact_mapping to :373. api.ts gets the generator's one-line diff from the verify-generated-types run (the GET /config description). * fix(redaction): user-only URL userinfo masked under ordinary keys; __all__ only on collections; ancestor exclusions cover descendants (#17899) * fix(redaction): redact_nested masks the whole URL userinfo, user and password, under any key (#17899) A key can sit in either userinfo slot (a user-only URL, or a token with a dummy password), so keeping the username leaked it. The user-only special case is dropped: every URL leaf masks all userinfo and keeps scheme/host/port/path. The tests that pinned the kept username now pin the masked form.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thinking Path
GET /agents/{agent_id}/llmincluded the provider API key as a response-model field. The schema it returned carried the docstring "LLM config including decrypted API key (for backend only)" while serving as theresponse_modelof a GET — so the invariant was written down next to the code that broke it. A comment is not an enforcement mechanism.Two choices followed from that, and both are about not leaving the same shape behind:
Omit, don't mask. A value the client never receives cannot be written back, so a GET-then-PUT round-trip is a no-op for the stored key.
public_provider_viewalready records this reasoning on the settings path; this matches it rather than inventing a second convention.Delete the schema, don't just stop using it.
AgentLLMConfigWithKey's only use in the tree was this one route. A schema that is safe only when used carefully is not safe, and the reliable way to stop it being returned is for it not to exist.Then the question that matters more than either: what stops the next one? Fixing the route alone leaves the class open, so the real deliverable here is the guard.
What Changed
autobot-slm-backend/api/agents.pyresponse_model→AgentLLMConfig; the key is no longer read or returned._decrypt_api_keykeeps its role and gains an explicit note that its return value must never reach a response model, a log line or an error body.autobot-slm-backend/models/schemas.pyAgentLLMConfigWithKeydeleted, with a three-line note recording why.repo_tests/no_secret_in_response_model_17865_test.pyscripts/python_file_size_known_large.py,repo_tests/python_file_size_ratchet_baseline.pyThe guard follows inheritance, which is the whole point: the field is declared on a parent in another module, so a per-file check would have seen a clean route and a clean schema and missed the defect sitting between them. It indexes both backends first, then resolves each
response_model=through its base classes.The matcher is deliberately narrow, and this was measured rather than guessed. A first draft matching any name containing
tokenflagged 27 distinct fields, nearly all of them LLM token counts —input_tokens,llm_max_tokens,tokens_spent,total_tokens_used. A guard that fires mostly on correct code gets switched off, so breadth would have cost the guard rather than bought coverage. Excluded, each for a reason:access_tokenis a login endpoint's entire job.*_ref,*_id,*_encrypted,*_hash,has_*— naming a pointer or a ciphertext is what a safe response does; flagging them would make the guard fire on the correct pattern.Narrowed, the whole tree yields exactly one hit, and it is intentional:
MFASetupResponse.secret. TOTP enrolment needs the shared secret, returned once, and that handler resolves the caller's own account. It is waived with a written reason rather than suppressed, andtest_every_waiver_still_describes_a_real_sitefails if a waiver stops matching a real route — so a waiver cannot rot into a standing exemption for a name some future model might take.Verification
The guard was run against the previous tree to prove it is not decoration:
It names the exact site. Restored, 22 pass. The reach floor is
completed-based rather than discovery-based, so a file that stops parsing is a skip and not a silent pass; the floor is pinned at the live population (4707 modules), not at a round number below it.Pre-push caught that I had re-derived the repo root from
__file__instead of callingrepo_tests._paths.repo_root()— fixed, andone_repo_root_spelling_15925_test.pypasses alongside.Out of scope, stated rather than silently skipped
Tenancy scoping for this route is not in this change. The
agentstable carries no company, tenant or owner column — only a nullablecreated_by. A predicate written against it would pass for every row where it is NULL, which is worse than none, because it reads as enforcement. That gap is tracked separately and this change does not depend on it.Consumer impact: none. The one caller of this route is server-to-server and reads
model/provider/enabled, while the response carriesllm_model/llm_provider— so it reads nothing this endpoint emits and falls through to defaults regardless. That vocabulary mismatch is a separate live defect and is being linked to its existing umbrella, not fixed here. No TypeScript, Vue, e2e fixture or generated client reads the removed field;openapi.jsonandapi.tsregenerate.Model Used
Claude Opus 5 (1M context)
Refs #17865
🤖 Generated with Claude Code
Summary by CodeRabbit