Skip to content

Add client_secret redaction pass to credential scrubber - #17

Open
jamesgol wants to merge 2 commits into
capitalone:mainfrom
jamesgol:fix/redact-oauth-broker-tokens
Open

Add client_secret redaction pass to credential scrubber#17
jamesgol wants to merge 2 commits into
capitalone:mainfrom
jamesgol:fix/redact-oauth-broker-tokens

Conversation

@jamesgol

Copy link
Copy Markdown

redact() handles URL basic-auth, Bearer headers, query-string tokens, and known token prefixes, but doesn't cover client_secret values. When the bedrock_oauth auth mode is in use, error responses from the token endpoint can include the client secret in form-encoded or header format, and those flow through to the audit stream unmasked.

This adds a regex pass for client_secret alongside the existing ones.

Add a regex pass that masks OAuth client_secret values in form-encoded
and header-style formats.
@jamesgol
jamesgol requested a review from a team as a code owner July 25, 2026 04:14

@schenksj schenksj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this gap — the direction is right and the change is low-risk in isolation (an added, non-destructive pass that can't affect the existing ones). Two things block merge for me: the new regex misses the most likely shape of the leak it's targeting, and there are no test updates in a file that has a dedicated security regression suite.

No unit tests were updated

This PR touches only agent/_url.py. redact() currently has per-pass coverage in two places:

  • tests/test_url.pyTestRedact, one case per pass
  • tests/verify_012_audit_redaction.py — the VULN-012 security regression file, with test_bearer_header_redacted, test_access_token_query_redacted, test_raw_token_prefixes_redacted_prefix_preserved

I ran both with this patch applied: 28 passed. No regressions, but nothing exercises the new pass — the added regex could be deleted and the suite would stay green. That missing test is also what would have caught the bug below.

Stale docstring

redact()'s docstring still says "Four passes (CWE-532)" and the numbered list stops at item 4. There are now five, and client_secret is undocumented. Every other pass is enumerated there; this one should be too. (Outside the diff hunk, so noting it here rather than inline.)

Reachability not demonstrated

redact is applied at agent/audit.py:368 (all audit-record strings) and agent/_stream_events.py:80. The AuthTokenError raised at agent/auth.py:134 — which interpolates response.text — is re-raised at runner.py:540, runner.py:676, verify.py:825, _llm.py:425 and isn't caught locally. I couldn't trace an end-to-end path proving that message text lands in the audit stream. Worth confirming, and the kind of thing an integration-level test would pin down.

Requested before merge

  1. Fix the quoted-value gap (JSON + TOML) — see inline.
  2. Add test_client_secret_redacted to tests/verify_012_audit_redaction.py covering form-encoded, JSON, TOML, single/double-quoted, and case-variant forms, plus a TestRedact case in tests/test_url.py matching the existing per-pass convention.
  3. Update the redact() docstring to five passes and list client_secret.

)
_FORM_SECRET_RE = re.compile(
r"(?i)(client_secret\s*[=:]\s*)[^\s,}&\"']+"
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exclusion class [^\s,}&\"']+ is meant to stop at a closing quote, but it also blocks the opening one — so any quoted value fails to match at all and passes through verbatim. Probing the regex directly:

ok    client_secret=SUPERSECRET&grant_type=x   -> client_secret=***&grant_type=x
ok    client_secret: SUPERSECRET               -> client_secret: ***
ok    CLIENT_SECRET=SUPERSECRET                -> CLIENT_SECRET=***
LEAK  {"client_secret": "SUPERSECRET"}         -> unchanged
LEAK  {"client_secret":"SUPERSECRET"}          -> unchanged
LEAK  client_secret = "SUPERSECRET"            -> unchanged   (TOML config form)
LEAK  client_secret='SUPERSECRET'              -> unchanged

This matters for the threat model in the PR description. RFC 6749 §5.2 specifies token-endpoint error responses as JSON, and agent/auth.py:134 interpolates response.text straight into AuthTokenError — so the most likely shape of the leak is exactly the shape not covered. The TOML form is live too: tests/test_audit.py:496 writes client_secret = "csecret" in a config fixture, and agent/audit.py:368 redacts every string in an audit record.

Suggested fix — consume the opening quote explicitly, and pick up the client-secret / clientSecret spellings that also leak today:

_FORM_SECRET_RE = re.compile(
    r"(?i)(client[_-]?secret\"?\s*[=:]\s*)[\"']?[^\s,}&\"']+[\"']?"
)

Please re-check idempotence after changing this. The current pattern happens to be idempotent because * isn't in the exclusion class, and that property is worth keeping — note that test_property_redact_is_idempotent (tests/test_url.py:164) only generates URL inputs, so it won't check this for you.

s = _BEARER_RE.sub(r"\1***", s)
s = _QUERY_TOKEN_RE.sub(r"\1***", s)
s = _RAW_TOKEN_RE.sub(r"\1***", s)
s = _FORM_SECRET_RE.sub(r"\1***", s)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pass ordering is fine — this runs after the token-prefix pass and the patterns don't overlap, so no interaction to worry about.

This new pass is the line with no test behind it. Please add a test_client_secret_redacted alongside the existing per-pass cases in tests/verify_012_audit_redaction.py, parametrized over the forms in my other comment (form-encoded, JSON with and without a space after the colon, TOML key = "value", single-quoted, uppercase), asserting the secret is absent and *** is present — plus an idempotence assertion.

Widen the regex to consume optional surrounding quotes and match
client-secret / clientSecret spellings. Add parametrized test
covering form-encoded, JSON, TOML, and single-quoted forms with
idempotence assertion.
@schenksj

Copy link
Copy Markdown
Contributor

@jamesgol - Do you expect to have an opportunity to tackle the feedback?

@jamesgol

jamesgol commented Aug 18, 2026 via email

Copy link
Copy Markdown
Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants