fix(configmap-loader): redact secret values from validation errors#891
Open
fix(configmap-loader): redact secret values from validation errors#891
Conversation
Pydantic's default ValidationError stringifier echoes input_value
into the error message for every rejected field. When a resolved
secret (access_key, slack_token, etc.) is rejected — e.g. because an
adjacent field like `_comment` triggers a discriminator mismatch in a
strict union model — the plaintext secret ends up in the loader's
ERROR log line AND in the K8s Event that the loader writes to the
ConfigMap. Both channels are visible to anyone with read access.
Discovered during the staging ConfigMap rollout: three stray
`_comment:` fields under workflow_{data,log,app}.credential caused
the discriminator to fail, and every subsequent rejected field in the
same credential — including access_key — was echoed into the logs
with input_value=<plaintext>. The Swift access_key was exposed.
Fix:
- Catch `pydantic.ValidationError` specifically in _validate_configs
- Walk the structured .errors() list; for leaf field names in a
curated sensitive-name set (access_key, password, slack_token,
smtp_password, api_key, secret_key, auth, value), substitute
`<redacted>` for the input value
- Non-sensitive fields keep their input values visible so operators
can debug typos in _comment, endpoint, region, etc.
Fields deliberately NOT redacted: secretName, secretKey, access_key_id
(these are references/metadata, not the secret content).
5 new unit tests; the regression test reproduces the staging bug end
to end and asserts the plaintext never appears in the returned error.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughFormats Pydantic Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/service/core/config/configmap_loader.py`:
- Around line 284-290: The current branch that builds input_repr inspects only
loc_parts against _SENSITIVE_FIELD_NAMES and then calls repr(input_value), which
can leak nested secrets; add a helper (e.g., sanitize_recursive or
_sanitize_nested_input) that walks dicts/lists/tuples and replaces any value
whose key (case-insensitive) matches _SENSITIVE_FIELD_NAMES with _REDACTED, then
call this sanitizer on input_value before calling repr; update the logic around
loc_parts/input_value in the block that computes input_repr so that if loc_parts
contains a sensitive key we still redact, otherwise we compute sanitized =
sanitize_recursive(input_value) and set input_repr = repr(sanitized) (preserve
existing None handling).
In `@src/service/core/config/tests/test_configmap_loader_unit.py`:
- Line 359: Replace the mutable dict assignment used for Pydantic model
configuration—change the class attribute model_config = {'extra': 'forbid'} on
the helper model to use pydantic's ConfigDict (e.g. model_config =
ConfigDict(extra='forbid')) and add the import from pydantic (ConfigDict) so the
class-level config is immutable and RUF012 is satisfied.
🪄 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 Plus
Run ID: 14e4d565-e7b8-424b-b08f-5a5e3ab46729
📒 Files selected for processing (2)
src/service/core/config/configmap_loader.pysrc/service/core/config/tests/test_configmap_loader_unit.py
Two security issues + one design critique from the review:
1. Broaden name matching (substring + whitelist)
The previous hardcoded-exact-name set missed real OSMO field
names: postgres_password, redis_password, cluster_api_key,
default_admin_password, private_key, oidc_secret. Switch to
substring match on password/token/secret/access_key/private_key/
api_key with an explicit whitelist for metadata fields
(access_key_id, secretName, secretKey, secret_file, mek_file,
currentMek). New test `test_substring_matches_real_field_names`
asserts every previously-leaking name is now redacted; new
`test_whitelisted_metadata_fields_not_redacted` asserts the
whitelist still shows values for debuggability.
2. Defensive recursive redaction of dict/list inputs
When Pydantic surfaces a parent dict as the error input (can
happen with union discriminator / model_validator failures),
repr(dict) would leak any secret-valued key inside. New
`_redact_nested()` walks dicts + lists and replaces sensitive
values in place before repr. New `test_redact_nested_walks_deeply`
covers nested dicts and lists of dicts.
3. Drop over-aggressive entries
Removed 'value' (too common, over-redacted debug info) and
narrowed 'auth' so it only matches via substring ('auth_' etc.),
not as a standalone top-level key name.
All 64 tests pass; pylint + mypy clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/service/core/config/tests/test_configmap_loader_unit.py (1)
358-360:⚠️ Potential issue | 🟡 MinorUse
ConfigDictformodel_configto avoid mutable class config.
model_config = {'extra': 'forbid'}is a mutable class attribute and triggers RuffRUF012. Prefer PydanticConfigDict(...)here.🧩 Proposed fix
class FakeModel(configmap_loader.pydantic.BaseModel): - model_config = {'extra': 'forbid'} + model_config = configmap_loader.pydantic.ConfigDict(extra='forbid') name: str = ''#!/bin/bash # Verify mutable model_config usage and confirm ConfigDict-based fix target. rg -nP 'model_config\s*=\s*\{' src/service/core/config/tests/test_configmap_loader_unit.py rg -nP 'model_config\s*=\s*.*ConfigDict\s*\(' src/service/core/config/tests/test_configmap_loader_unit.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/service/core/config/tests/test_configmap_loader_unit.py` around lines 358 - 360, Replace the mutable dict assigned to the Pydantic model config in FakeModel with a ConfigDict: update the class FakeModel (subclassing configmap_loader.pydantic.BaseModel) so its model_config uses pydantic's ConfigDict(...) rather than a plain dict (e.g., model_config = ConfigDict(extra='forbid')), and add the necessary import for ConfigDict from pydantic so Ruff RUF012 is resolved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/service/core/config/tests/test_configmap_loader_unit.py`:
- Around line 358-360: Replace the mutable dict assigned to the Pydantic model
config in FakeModel with a ConfigDict: update the class FakeModel (subclassing
configmap_loader.pydantic.BaseModel) so its model_config uses pydantic's
ConfigDict(...) rather than a plain dict (e.g., model_config =
ConfigDict(extra='forbid')), and add the necessary import for ConfigDict from
pydantic so Ruff RUF012 is resolved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5763afe9-18a2-4ff5-9e3d-af40092e1f8b
📒 Files selected for processing (2)
src/service/core/config/configmap_loader.pysrc/service/core/config/tests/test_configmap_loader_unit.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/service/core/config/configmap_loader.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #891 +/- ##
==========================================
+ Coverage 42.32% 42.45% +0.13%
==========================================
Files 211 211
Lines 27217 26831 -386
Branches 4193 4195 +2
==========================================
- Hits 11520 11392 -128
+ Misses 15095 14834 -261
- Partials 602 605 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Addresses coderabbitai review comment on the curated sensitive-name
set being fragile (postgres_password, redis_password, cluster_api_key
etc. were all leaking because they weren't in the list).
Replace the entire hardcoding machinery with a simple policy: never
echo `input_value` from Pydantic errors. The output now contains only
the field path, Pydantic's reason, and the Python type of the
submitted value (e.g. `input_type=str`). That's enough to diagnose
"I put a string where an int was expected" without ever touching the
content.
Before (leaky, required maintaining substring + whitelist lists):
workflow.workflow_data.credential.DefaultDataCredential.access_key:
Extra inputs are not permitted [type=extra_forbidden,
input_value='3b197f00166babf3284c9de6ea77ebda', input_type=str]
After (zero hardcoding, no value ever echoed):
workflow.workflow_data.credential.DefaultDataCredential.access_key:
Extra inputs are not permitted (input_type=str)
The checked-in Helm values / ConfigMap are the source of truth for
what was submitted; the error message doesn't need to echo it back.
This matches the K8s admission-webhook error style and eliminates
the whole class of "did we remember every sensitive field name?"
bugs. Adding a new secret field is now zero-touch.
Removed: _SENSITIVE_NAME_SUBSTRINGS, _NON_SENSITIVE_FIELD_NAMES,
_is_sensitive_name(), _redact_nested(), and all associated tests.
Net: -108 lines from the prior revision.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/service/core/config/tests/test_configmap_loader_unit.py (1)
360-362:⚠️ Potential issue | 🟡 MinorUse
ConfigDictformodel_configto avoid mutable class-attribute config.
model_config = {'extra': 'forbid'}still triggersRUF012and remains mutable on the class.🧹 Proposed fix
class FakeModel(configmap_loader.pydantic.BaseModel): - model_config = {'extra': 'forbid'} + model_config = configmap_loader.pydantic.ConfigDict(extra='forbid') name: str = ''#!/bin/bash set -euo pipefail # Verify this mutable model_config pattern still exists rg -nP --type=py "model_config\s*=\s*\{" # Verify ConfigDict is available/used in the repo rg -nP --type=py "ConfigDict\s*\(" src/ # Verify pydantic dependency declaration in manifests fd -i "pyproject.toml|requirements*.txt|setup.py|poetry.lock|uv.lock" -x rg -n "pydantic" {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/service/core/config/tests/test_configmap_loader_unit.py` around lines 360 - 362, The test defines FakeModel inheriting from configmap_loader.pydantic.BaseModel and sets a mutable class attribute model_config = {'extra': 'forbid'} which triggers RUF012; replace that mutable dict with a pydantic ConfigDict instance (e.g., set model_config = configmap_loader.pydantic.ConfigDict(extra='forbid') or import ConfigDict and use ConfigDict(extra='forbid')) so the model configuration is immutable and compliant; update the FakeModel declaration accordingly (reference symbols: FakeModel, configmap_loader.pydantic.BaseModel, model_config, ConfigDict).
🧹 Nitpick comments (1)
src/service/core/config/configmap_loader.py (1)
266-274: Prefer descriptive names in_format_validation_errorloop.Use full variable names instead of abbreviations (
err,loc,msg) to match repository Python naming guidance.♻️ Suggested refactor
- for err in error.errors(): - loc_parts = tuple(str(p) for p in err.get('loc', ())) - loc = '.'.join(loc_parts) if loc_parts else '<root>' - msg = err.get('msg', '') - if 'input' in err: - input_type = type(err['input']).__name__ - parts.append(f'{loc}: {msg} (input_type={input_type})') + for validation_error in error.errors(): + location_parts = tuple( + str(location_part) for location_part in validation_error.get('loc', ()) + ) + location = '.'.join(location_parts) if location_parts else '<root>' + message = validation_error.get('msg', '') + if 'input' in validation_error: + input_type = type(validation_error['input']).__name__ + parts.append(f'{location}: {message} (input_type={input_type})') else: - parts.append(f'{loc}: {msg}') + parts.append(f'{location}: {message}')As per coding guidelines: "Do not use abbreviations in Python variable names unless they are well-understood ... Use full, descriptive names."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/service/core/config/configmap_loader.py` around lines 266 - 274, In _format_validation_error, replace short variable names in the loop with descriptive ones: rename err -> error, loc_parts -> location_parts, loc -> location, msg -> message, and input_type -> input_value_type (or similar) and update all references inside that for block accordingly; keep the same logic (build location_parts from error.get('loc', ()), join into location or '<root>', extract message from error.get('msg', ''), detect 'input' and derive input_value_type = type(error['input']).__name__, and append formatted strings to parts) so behavior is unchanged but names follow the repository's no-abbreviations guideline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/service/core/config/tests/test_configmap_loader_unit.py`:
- Around line 360-362: The test defines FakeModel inheriting from
configmap_loader.pydantic.BaseModel and sets a mutable class attribute
model_config = {'extra': 'forbid'} which triggers RUF012; replace that mutable
dict with a pydantic ConfigDict instance (e.g., set model_config =
configmap_loader.pydantic.ConfigDict(extra='forbid') or import ConfigDict and
use ConfigDict(extra='forbid')) so the model configuration is immutable and
compliant; update the FakeModel declaration accordingly (reference symbols:
FakeModel, configmap_loader.pydantic.BaseModel, model_config, ConfigDict).
---
Nitpick comments:
In `@src/service/core/config/configmap_loader.py`:
- Around line 266-274: In _format_validation_error, replace short variable names
in the loop with descriptive ones: rename err -> error, loc_parts ->
location_parts, loc -> location, msg -> message, and input_type ->
input_value_type (or similar) and update all references inside that for block
accordingly; keep the same logic (build location_parts from error.get('loc',
()), join into location or '<root>', extract message from error.get('msg', ''),
detect 'input' and derive input_value_type = type(error['input']).__name__, and
append formatted strings to parts) so behavior is unchanged but names follow the
repository's no-abbreviations guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6b0cf14b-95cb-471e-be55-887482b2a335
📒 Files selected for processing (2)
src/service/core/config/configmap_loader.pysrc/service/core/config/tests/test_configmap_loader_unit.py
|
📖 Docs preview: https://d3in15bfzp49i0.cloudfront.net/891/index.html |
Rationale fits in two lines. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Problem
Pydantic's default
ValidationErrorechoesinput_value=<repr>into the error message. When a rejected field holds a resolved secret (e.g. anaccess_keymerged from a mounted K8s Secret), the plaintext lands in the loader's ERROR log line and in the K8s Event the loader writes to the ConfigMap.Fix
Never echo submitted values. Output:
<path>: <reason> (input_type=<type>).Before:
After:
Earlier revisions tried a curated sensitive-name allowlist — review surfaced that it kept missing real OSMO fields (
postgres_password, etc.). The final version has zero hardcoding: no allowlist, no introspection, no per-field work when new secret fields are added.What operators lose
For type typos like
max_num_tasks: 'not-a-number', the echoed'not-a-number'is gone. Operator sees the path +input_type=strand grep their values file. Checked-in Helm values are the source of truth.Test plan
bazel test //src/service/core/config/...— pass (4 new tests)Regression test
test_real_models_never_leak_any_valuereproduces the staging scenario and asserts no submitted value appears in the output.🤖 Generated with Claude Code
Issue - None
Checklist
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests