Repository navigation
security(llm): GET /api/llm/config redacts credentials in the nested backend.llm subtree (#18193) - #18219
Conversation
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)
…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)
…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
…ential 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
#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
… 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
…fig is admin-only and redacted (#18193)
… 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
…it (#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
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 16 minutes. View limit details
📝 Walkthrough
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🟡 Moderate · up to The admin-only configuration endpoint can still return an API key placed in the username part of a URL that has no password. Separately, the guard that checks response models for secrets can accept an exclusion that does not actually remove a nested credential. Address both before merging. Pre-merge checks |
|
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
…g, 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).
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
7fb8cf13-fbac-4922-96a7-8fd0773b31d7
⛔ Files ignored due to path filters (3)
autobot-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 (14)
autobot-backend/api/llm.pyautobot-backend/api/llm_config_redaction_18193_test.pyautobot-backend/config/model_config.pyautobot_shared/secret_redaction.pyautobot_shared/secret_redaction_nested_18193_test.pydocs/api/public-api-reference.mddocs/developer/03-api-reference.mddocs/developer/REDACTION_BOUNDARY.mddocs/developer/THREAT_MODEL.mdrepo_tests/no_secret_in_response_model_17865_test.pyrepo_tests/no_secret_in_response_model_baseline_17865.pyrepo_tests/no_secret_in_response_model_contrast_17865_test.pyrepo_tests/no_secret_in_response_model_fixtures_17865.pyrepo_tests/redaction_concept_census_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.
…all__ only on collections; ancestor exclusions cover descendants (#17899)
…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.
…less userinfo is masked, and redact_dict composes redact_nested with no partial reveal (#18194) (#18228) * feat(redaction): pin and passcode are credentials as whole tokens under both policies (#18194) Owner decision on #18194: add pin and passcode to the redaction vocabulary, matched as whole words. A new WHOLE_TOKEN_NOUNS tuple (not BROAD_FRAGMENTS, which is substring-matched and feeds credential_redaction.SENSITIVE_KEYS) is consulted by PRECISE and BROAD alike. Names are tokenised on _ - . and camelCase; the name matches when it ends in pin, pin code or passcode. Masked: pin, user_pin, pinCode, card_pin, passcode. Unmasked: pin_memory, pinned_revision, pin_length, spinner, opinion, pinyin. secret_redaction.py was 599/600 lines, so the vocabulary and matcher (CREDENTIAL_SUFFIXES, AUTHORIZATION_TERMS, BROAD_*, quantity rule, MatchPolicy, matcher) move to secret_redaction_vocabulary.py. secret_redaction re-exports every public name; the census guard treats the pair as one canonical home. Corpus delta, measured before (parent commit) vs after over autobot_shared/redaction_corpus_17336.txt plus the 11 appended contrast names, value int / digit string / True, nothing removed anywhere: - The 3,373 pre-existing names: 0 newly masked under either policy (no name there contains the token pin or passcode). - Appended names newly masked, PRECISE (redact_value, RedactedReprMixin, URL query, portability export): pin, user_pin, pinCode, card_pin, passcode. - Appended names newly masked, BROAD (redact_mapping, redact_dict, cot_events, config_revision, redact_nested): pin, user_pin, pinCode, card_pin. passcode was already masked under BROAD (substring pass). * feat(redaction): redact the password in a scheme-less user:password@host[:port] value (#18206) The canonical redactor only scrubbed userinfo when a value carried scheme://. A bare user:pw@host:5432 (DSN, proxy or endpoint field) passed through. redact_schemeless_userinfo masks the password of a WHOLE value of that shape to user:**********@host:5432 and is applied by redact_value and redact_nested (any depth, any name, over a URL-shaped name too). No false positives, each pinned by a contrast test: name@host.com (no colon), host:port (no @), mailto: style addresses, prose with whitespace, and scheme URLs (still handled by the URL path). Not applied in redact_text/redact_content: free text has no value boundary, so note:bob@example.com would read as userinfo; the boundary doc says so. Masked set per caller: unchanged for every name (the change is value-side and only widens), asserted over the full corpus. * refactor(redaction): credential_redaction.redact_dict composes redact_nested, with a fixed-width mask (#18205) redact_nested is now the one recursive mapping redactor. redact_dict is a thin composition over it: - exempt_counts becomes an explicit keyword on redact_nested (default True, unchanged for GET /config and /current); redact_dict passes False because this layer always masked a count of a credential noun (max_tokens: 4096), and no name may leave a caller's masked set. - extra_scan is a new optional hook run over every string left unmasked; redact_dict passes redact_string so the API_KEY_PATTERNS content scan (incl. the 32+ char generic key) that replay_service, safe_repr and the log filter rely on is kept. - The partial reveal value[:4]...value[-4:] (and the *** for other types) is replaced by the fixed-width REDACTED_PLACEHOLDER: eight characters of a key are a disclosure. redact_api_key keeps the partial form only for redact_string's pattern matches, reported on the issue. - _redact_list is gone (dead after the composition). Behaviour changes, both widening or neutral: tuples/sets are now walked; None or "" under a credential name is returned as-is instead of ***. Proof the masked set does not shrink: test_no_name_leaves_redact_dicts_masked_set runs the frozen main implementation against the new one over the whole 3,384-name corpus for int, float, digit string, JWT, True and an sk- string: zero names unmasked. The #17336 policy test's redact_dict caller entry (BROAD, no count exemption) is unchanged and still green. * fix(redaction): redact_api_key returns the fixed-width placeholder, so no llm_shared path reveals part of a key (#18205) redact_string's API_KEY_PATTERNS matches (CredentialRedactionFilter, safe_repr) still showed value[:4]...value[-4:]. #18205 asks for the partial reveal to be justified or replaced; nothing justifies it, so it is replaced. THREAT_MODEL anchors follow the vocabulary split. * fix(redaction): pin_hash, pincode and pin digest/salt are credentials under both policies; gpio_pin consequence recorded (#18194) * fix(redaction): schemeless userinfo splits on the last @, takes / and # in the password and an IPv6 host, spares a drive path (#18206) * fix(redaction): redact_dict never unmasks a credential-named URL, redact_nested scrubs only plain endpoint names, scalars pass through, no-shrink test pins base as data (#18205) * fix(redaction): keep #18219's mask-all-userinfo rule through the rebase, and re-point THREAT_MODEL anchors (#18205) Rebasing onto #18219's 0037917 replayed _redact_string_leaf with mask_username=cred; #18219 masks ALL URL userinfo (a key can sit in the user slot). Restored here, with the docstring to match. The redact_text/redact_mapping anchors follow the vocabulary split. * fix(redaction): endpoint-shaped credential names stay masked whole as on main, redact_nested joins the per-caller no-shrink test (#18205) * test(redaction): restore the userinfo-in-both-forms test a rebase dropped (#17899) * docs(redaction): inventory the config fields that can hold scheme-less userinfo (#18206) * fix(redaction): pin_number, pins and pin_codes are credentials, pinned and spinners are not (#18194) * test(redaction): pin the 12:30@site over-mask as the documented trade-off (#18206) * test(redaction): the redact_nested base set is one line per name, 3,415 lines instead of 19,640 (#18205) Same 19,635 (name, value) pairs, now `<name><TAB><value ids>`. Three of the seven values (jwt, sk, userinfo) are masked under every corpus name, so the pair-per-line form was mostly repetition. Mutation re-adding "_endpoint" to the exemption still fails the urlpath case. * fix(tests): the agent-venv guard reads the stdlib from sys.stdlib_module_names, not a hand-kept set (#18205) The hand-kept _STDLIB lacked decimal, so the first module on the agent's import path to use it read as an uninstalled third-party package. The interpreter's own list replaces it; a contrast pair pins that decimal/asyncio need no install and yaml/httpx do.
Thinking Path
#17899 asked for
GET /api/llm/configto be audited for what it actually serialises. The audit tracedapi/llm.py:64→ConfigService.get_llm_config()→config/model_config.py, which returns the wholebackend.llmsubtree asunifiedthrough a response model with no fields andextra="allow", to any authenticated user. No code writes a key there, butget_api_key()documentsbackend.llm.cloud.providers.<provider>.api_keyas a key location. An operator-set key there was therefore returned in full, as was URL userinfo (user:pass@). That is #18193. Its fix had to go through the canonical redactor, so it waited for #17336 to merge, which has now happened.The same branch carries two
#17865guard commits from the orphan sweep. They make the no-secret-in-response-model guard's exclusion matching path-aware, so a nestedx.api_keyexclusion can no longer hide a top-levelapi_key, and resolve constants per module.What Changed
autobot_shared/secret_redaction.py:redact_nested(value, policy=BROAD), the one recursive mapping redactor (599/600 lines):exempt_countssomax_tokensstays a number (as at the template export; both callers are named inREDACTION_BOUNDARY.mdand the docstring);*_url), in which case only its userinfo is scrubbed. Sowebhook_secret: "https://…/<secret>"is masked whole;?apikey=,?apiKey=,?access-token=,?auth=and?X-Amz-Signature=. Existing callers keep the PRECISE default, unchanged;redact_content(sk-…, JWTs, inlineuser:pass@);autobot-backend/api/llm.py:GET /configreturnsredact_nested(ConfigService.get_llm_config())and is now admin-only (Depends(check_admin_permission), the repo's dominant admin gate, already used 6 times in this file). Owner decision on security(llm): GET /api/llm/config returns operator-set provider keys from backend.llm in full #18193: both redaction and admin-only.GET /current, which returned the same tree unredacted to any authenticated user, is slimmed to{"model", "provider", "config": {}}for everyone. The owner decision on security(llm): GET /api/llm/config returns operator-set provider keys from backend.llm in full #18193 reads: "GET /api/llm/currentis slimmed to the active model name and provider for non-admin users, which is all the Settings panel displays. The full LLM config tree is available only through the admin-onlyGET /api/llm/config." Admins get the same slim body, because the full tree is available only through/config.LLMCurrentResponse:model,provider,config), so the generatedapi.tsstays valid.configis kept as an always-empty envelope because it is a required field./current.SettingsPanel.vue's model display reads/api/settings/, which this PR does not touch. The pre-existing bug that/currentalways reported the default model, not the active selection (bug(llm): GET /api/llm/current always reports the default model, not the active selection #18215), is fixed in this PR: it now resolves the active provider and model the way the config service exposes them.repo_tests/no_secret_in_response_model_baseline_17865.py:GET /configwaivers record the audited verdict, now fixed;llm_config.pyapi_keyentries are removed as genuinely fixed byresponse_model_exclude=_NO_PROVIDER_KEYS(fix(slm-secrets): stop sending LLM provider keys to the client, so a save cannot overwrite them (#17826) #17846), taking the count from 14 to 12.repo_tests/no_secret_in_response_model_17865_test.pyand its fixtures: path-aware exclusion and per-module constants (cherry-picked from the#17865commits 481aeead97 and 189448dbd4).repo_tests/no_secret_in_response_model_contrast_17865_test.py: 7 contrast cases. 3 of them fail on main's name-only guard.docs/developer/REDACTION_BOUNDARY.md:redact_nesteddocumented for nested API response bodies.docs/api/public-api-reference.mdanddocs/developer/03-api-reference.md:/llm/currentis documented as returning only model and provider, and/llm/configas admin-only and redacted.api/llm.pyshrank to 1080 lines, and its ceiling is lowered in both size baselines.Before → after for the sentinel fixture (redacted fields only):
unified.cloud.providers.openai.api_key**********unified.cloud.providers.embedding[0].api_key**********unified.local.providers.ollama.host(http://user:<pw>@host:1234)http://user:**********@host:1234Verification
autobot-backend/api/llm_config_redaction_18193_test.py:get_llm_config()path, patching only the config tree;max_tokensare unchanged;autobot_shared/secret_redaction_nested_18193_test.py: unit tests forredact_nested.test_config_is_admin_only(non-admin → 403, no sentinels in the body) andtest_an_admin_gets_the_redacted_body.test_current_returns_only_model_and_provider_to_a_non_admin: the exact body{model, provider, config: {}}, the key set exactly{model, provider, config}, and no sentinel or tree content. It fails on the pre-slim head.secret_redaction.py596/600,api/llm.py1082, at its ceiling and not grown.Risks
redact_nesteduses BROAD: over-masking a non-secret field whose name ends in a credential noun is possible and preferred to a leak on a response readable by any authenticated user./currentno longer returns the config tree to anyone. Its only code callers are none; the SDK quickstarts only show the call. Admins use/config.user:pass@hoststrings (security(redaction): scheme-less user:password@host strings are not scrubbed by the canonical redactor #18206) and the second recursive redactorcredential_redaction.redact_dict(tech-debt(redaction): llm_shared credential_redaction.redact_dict is a second recursive redactor that partially reveals keys #18205).Model Used
Anthropic Claude Opus 5.5 (claude-opus-5-5), coordinating; Anthropic Claude Sonnet 5.5 (claude-sonnet-5-5) subagents for the audit, implementation and review.
Issue Link
Closes #17899
Closes #18193
Closes #18215
Refs #17865
Changelog fragment
Summary by CodeRabbit