fix(admin): redact saved model endpoint secrets - #615
Conversation
|
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:
📝 WalkthroughWalkthroughSecrets are redacted in admin config and model endpoint responses, model endpoint updates preserve stored API keys unless replaced, and the admin UI can reveal, reuse, and submit endpoint API keys while keeping read responses masked. ChangesAdmin config and secret redaction
Model endpoint API-key flow
UI API client and admin dialog
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AdminUI
participant ApiRouter
participant ModelEndpointService
participant Store
AdminUI->>ApiRouter: POST /model-endpoints/{type}/{name}/reveal-api-key
ApiRouter->>Store: load endpoint record
Store-->>ApiRouter: endpoint.extra.api_key
ApiRouter-->>AdminUI: RevealApiKeyResponse
AdminUI->>ApiRouter: POST /model-endpoints/validate
ApiRouter->>Store: load stored endpoint by type/name
Store-->>ApiRouter: stored endpoint.extra.api_key
ApiRouter->>ModelEndpointService: validate_endpoint(url, api_key)
ModelEndpointService-->>ApiRouter: validation result
ApiRouter-->>AdminUI: validation response
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
fb56fd5 to
ffc83bc
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb56fd5c68
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ffc83bc to
76ce3d8
Compare
76ce3d8 to
4513681
Compare
andyne13
left a comment
andyne13
left a comment
There was a problem hiding this comment.
Thanks for this — the write-only handling is clean and extending redaction to /config is a good catch. A few points from reviewing the diff:
Blocker — draft validation can send a stored key to an arbitrary URL
validate_endpoint_draft (api/routers/admin/model_endpoints.py) now reuses a stored endpoint's api_key (via stored_api_key_name / stored_api_key_model_type) while probing body.endpoint, which is fully caller-controlled. validate_endpoint (services/orchestrators/model_endpoint_service.py) then sends Authorization: Bearer <stored key> to {url}/models with no binding between url and the stored endpoint and no URL safety check.
So a caller can set endpoint to a server they control, reference any saved endpoint by name/type, and read the bearer token off the wire — recovering the key this PR otherwise makes write-only. The same unbound URL also lets the probe reach internal addresses (SSRF).
Suggestion: when a stored key is reused, bind the probe to that endpoint's saved URL (or require body.endpoint's host to match it); if the URL differs, require the key to be passed explicitly rather than reused. An is_safe_url-style guard on the probe URL with follow_redirects=False would close the SSRF angle too.
Should-fix — /config redaction is a denylist, not safe-by-default
redact_secrets redacts by exact field name against SECRET_FIELD_NAMES. It covers everything sensitive in the settings today, but because it's exact-match, a future secret field with an unlisted name (e.g. access_key, private_key, signing_key) would be returned in clear by default.
Two options: build the /config payload from an explicit allowlist of safe fields (new fields default to hidden), or keep the denylist but add a guard test that fails when a new str field is added to the settings model without being classified — so it can't silently rot.
Minor
tokenandsecretas exact names will redact any field literally named that, secret or not. Fine for the current settings; just flagging in case a benign field namedtokenever appears.
The redaction helper, has_api_key, and preserve-on-update all look correct otherwise. 👍
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/api/routers/admin/test_phase14_admin_routers.py (1)
250-260: 🔒 Security & Privacy | 🔵 TrivialConsider covering audit logging for secret reveal.
This test confirms the reveal endpoint returns the raw stored secret to an authorized caller, which matches the intended design. Since revealing a raw credential is a sensitive, high-value action, consider whether the production route should emit a structured audit log entry (via
get_logger()), and if so, add a test asserting that log emission alongside this response check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/api/routers/admin/test_phase14_admin_routers.py` around lines 250 - 260, The reveal-api-key test currently only verifies the response from the model-endpoint reveal flow, but it does not cover the expected audit logging for this sensitive action. Update the test around test_model_endpoint_reveal_api_key_returns_stored_secret to also assert that the route emits a structured audit log via get_logger() when the secret is revealed, while keeping the existing response assertion for the raw api_key. Use the existing reveal endpoint path and the FakeModelEndpointService/_build_app setup to locate the production flow and verify both the returned secret and the log emission.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openrag/services/orchestrators/model_endpoint_service.py`:
- Around line 344-345: The URL validation in model_endpoint_service should
handle malformed inputs before urlsplit() can raise ValueError. Update the
validation path around the parsed urlsplit(url) logic to catch parsing errors
and return the same structured absolute HTTP(S) validation response instead of
letting an exception bubble out. Keep the fix localized to the URL-checking flow
in the relevant validation method so existing scheme/netloc checks still run for
valid inputs.
In `@ui/src/pages/admin/models.tsx`:
- Around line 271-273: The revealed API-key cache in the admin models dialog is
currently a single string, so a late response from one endpoint can be reused
for a different one. Update the state and related logic in the models page
(including the editing/reveal flow around revealedApiKey, fetchStoredApiKey, and
the reveal handlers) to store the cached secret together with its { modelType,
name } identity, and only return or display it when it matches the current
editing target. Ensure any reveal/reset logic also clears or ignores stale
cached values when editing switches to a different endpoint.
---
Nitpick comments:
In `@tests/unit/api/routers/admin/test_phase14_admin_routers.py`:
- Around line 250-260: The reveal-api-key test currently only verifies the
response from the model-endpoint reveal flow, but it does not cover the expected
audit logging for this sensitive action. Update the test around
test_model_endpoint_reveal_api_key_returns_stored_secret to also assert that the
route emits a structured audit log via get_logger() when the secret is revealed,
while keeping the existing response assertion for the raw api_key. Use the
existing reveal endpoint path and the FakeModelEndpointService/_build_app setup
to locate the production flow and verify both the returned secret and the log
emission.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b492b3f4-639c-4a31-91b2-674675d953c8
📒 Files selected for processing (12)
openrag/api/main.pyopenrag/api/routers/admin/model_endpoints.pyopenrag/api/schemas/admin/model_endpoint_schemas.pyopenrag/core/utils/redaction.pyopenrag/services/orchestrators/model_endpoint_service.pytests/integration/api/test_model_endpoints.pytests/unit/api/routers/admin/test_phase14_admin_routers.pytests/unit/api/test_secret_redaction.pytests/unit/services/orchestrators/test_model_endpoint_service.pyui/src/lib/api/models.test.tsui/src/lib/api/models.tsui/src/pages/admin/models.tsx
andyne13
left a comment
andyne13
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround — re-reviewed the latest commits.
Resolved ✅
- Draft-validate key reuse is now bound to the saved URL via
_same_endpoint_url(400 otherwise). Checked it for bypasses — the compare is strict (case differences only make it stricter/fail-closed), so a stored key can't be redirected to an arbitrary host.follow_redirects=Falseon the probe is a nice addition too. - Denylist robustness —
SECRET_FIELD_SUFFIXESnow catches compound names (*_api_key,*_token,*_password,*_secret,*_access_key, …), so things likeminio_secret_key/smtp_passwordare covered, and themax_tokensfalse-positive test is a good guard. 👍
🔴 New regression to fix before merge — /config now leaks the first 3 chars of every secret
redact_secrets (used by /config, main.py:333) no longer fully redacts — it now calls mask_secret_value, which for any secret ≥ 8 chars returns value[:3] + "********" (redaction.py:58-60). Your own test confirms it: redact_secrets(...)["websearch"]["api_token"] == "sea********".
So /config returns the first 3 characters of rdb.password, the OIDC token_encryption_key (session-encryption Fernet key), oidc.client_secret, every api_key, and websearch.api_token. That's a partial-disclosure regression from the full <redacted> this PR started with.
The prefix hint makes sense for a user-supplied endpoint API key (to identify which key) — but mask_secret_value is only ever reached via /config (the endpoint path drops the key through redact_secret_mapping + has_api_key). So the masking applies exclusively to infrastructure secrets, where it has no UI purpose: revealing the first 3 chars of a human-chosen DB password aids guessing, and exposing any bytes of the session-encryption key is best avoided.
Suggestion: keep /config on full <redacted> (or omit) and don't route it through mask_secret_value — e.g. a full-redact variant for the config path, leaving masking (if you want it) for endpoint-key display only.
Minor (unchanged): the validate probe still has no is_safe_url guard, so it can reach internal addresses with an admin-supplied key. Admin-gated and follow_redirects=False covers the redirect pivot, so a should-fix rather than a blocker.
|
Fixed the /config redaction regression in 1637c17. Config secrets now return full |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07b4e11ecc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6587e174b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2ef2b727c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Ahmath-Gadji
left a comment
Ahmath-Gadji
left a comment
There was a problem hiding this comment.
Approving — all substantive findings are resolved and I validated the branch locally at 9b7bca0e.
Findings closed
- ✅ Endpoint
extradata-loss (c6587e17) —redact_secret_mappingnow masks secrets in place instead of whitelisting, so non-secret config (temperature,enable_thinking, nestedauth/headers) survives the read→edit→save round-trip. Locked in bytest_redact_secret_mapping_keeps_non_secret_endpoint_extra_shape. - ✅
/configprefix leak (1637c174) —redact_secretsreturns full<redacted>for all secret fields; no more first-3-char disclosure of DB password / Fernet key / client secret. - ✅ URL-edit UX (
c6587e17,f2ef2b72) — changing an endpoint URL with a hidden key now gives a clear "reveal or enter the API key" prompt instead of a cryptic 400, a revealed key feeds validation, and explicit key-clearing is supported.
Validation run locally
pytestredaction + model-endpoint-service + admin-routers → 59 passedvitest run src/lib/api/models.test.ts→ 20 passed- CI green (api-tests, tests, milvus-integration, lint, layer-import-guard)
Non-blocking follow-up: the draft-validate probe still has no private-range/is_safe_url guard, so an admin-supplied URL can reach internal addresses — admin-gated and mitigated by follow_redirects=False, so fine to land and harden later.
3 participants
Summary
Fixes #614 by redacting saved secrets from admin-facing read responses while keeping the real values available server-side.
Model endpoint responses now hide secret values from
extraand exposehas_api_keyso the Admin UI can show that a key exists without receiving it. The service also preserves existing stored secrets when an endpoint update omits the key, which prevents the edit form from accidentally clearing private endpoint credentials.The
/configresponse now redacts known secret fields before returning the settings object to the Admin UI.Why
The Admin UI needs to manage model endpoints and runtime config, but raw API keys, tokens, and passwords should not be sent back to the browser after they are saved. At the same time, saved model endpoint keys are still required by backend validation and indexing, so this keeps storage/internal use intact.
Validation
uv run --no-env-file pytest tests/unit/api/routers/admin/test_phase14_admin_routers.py tests/unit/services/orchestrators/test_model_endpoint_service.py tests/unit/api/test_secret_redaction.py tests/unit/api/test_main_proxy_headers.py -quv run --no-env-file ruff check openrag/core/utils/redaction.py openrag/api/schemas/admin/model_endpoint_schemas.py openrag/api/main.py openrag/services/orchestrators/model_endpoint_service.py tests/unit/api/test_secret_redaction.py tests/unit/api/routers/admin/test_phase14_admin_routers.py tests/unit/services/orchestrators/test_model_endpoint_service.py tests/integration/api/test_model_endpoints.pygit diff --checkcd ui && npm run lint && npm run test && npm run buildapi_key.Notes
I also ran
tests/integration/api/test_model_endpoints.pyagainst the local CPU stack. Three tests passed; the CRUD validation test failed on the existing local setup because it expects a mock vLLM service athttp://vllm:8000/v1, which this CPU stack does not start.TestSprite preflight passed, but I did not run a TestSprite suite because this change is only deployed on localhost and TestSprite requires a publicly reachable target URL.
Summary by CodeRabbit
extrasecret data are now redacted in API responses.