Skip to content

security: centralize secret-name redaction heuristics - #400

Open
codeforester wants to merge 2 commits into
mainfrom
security/384-20260930-security-widen-and-de-duplicate-the-secret-name-redaction-he
Open

codeforester wants to merge 2 commits into
mainfrom
security/384-20260930-security-widen-and-de-duplicate-the-secret-name-redaction-he

Conversation

@codeforester

Copy link
Copy Markdown
Contributor

Fixes #384


REDACTED = "[REDACTED]"
SECRET_KEY_RE = re.compile(r"(token|password|secret|api[-_]?key|authorization)", re.IGNORECASE)
SECRET_KEY_PATTERN = (

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Security regression (verified): unifying the secret-name regexes replaces the old unanchored substring match with (?<![A-Za-z0-9])...(?![A-Za-z0-9])-anchored alternatives, which silently narrows coverage for single-word alternatives (token, password, secret, credential, authorization, bearer, session, cookie, signature, otp, salt, sas, pem) whenever they're concatenated with another word and no -/_ separator. Verified directly: accessToken, refreshToken, idToken, clientSecret, and authToken all matched the old bare-substring SECRET_KEY_RE but do not match the new pattern (only the three compound alternatives that bake in an optional separator — private[-_]?key, access[-_]?key, api[-_]?key — still match their concatenated forms). This reaches all three real call sites of is_secret_key (JSON details redaction, CLI parameter auto-detection, inline log-text redaction), none of which normalize camelCase before testing, and it's untested by this PR's own new cases (which only cover hyphen/underscore-separated forms). Realistic OAuth/JS-SDK-style keys like accessToken or clientSecret would now be logged/output in plaintext where they were previously redacted.

Comment thread docs/json-contracts.md Outdated
`password`, `secret`, `api_key`, and `authorization`) and credential-bearing
URLs are redacted recursively.
URLs are redacted recursively. The heuristic covers token, password/passphrase,
credential, private/access/API key, authorization/bearer, session/cookie,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Doc accuracy (minor): this enumerated list omits passwd/pwd, even though both are present in the actual SECRET_KEY_PATTERN (docs/security-threat-model.md's list is also incomplete in a similar way, e.g. missing password/passphrase and signature). A consumer auditing which of their own field names get auto-redacted, based on this doc, wouldn't know passwd/pwd are covered too.

@codeforester

Copy link
Copy Markdown
Contributor Author

Following up on the camelCase finding: the fix adds explicit alternatives for the 5 examples I named in the failure scenario (accessToken, refreshToken, idToken, clientSecret, authToken) and they now correctly redact. Verified with SECRET_KEY_RE directly.

But this patches the named examples rather than the underlying gap: the single-word alternatives (token, password, secret, session, cookie, signature, etc.) still require a non-alphanumeric boundary on both sides, so any other camelCase/compound secret-like key still slips through un-redacted. Verified directly against the current pattern — all of these still return False from is_secret_key():

sessionToken    userPassword    mySecret       apiSecret
secretKey       passwordHash    cookieValue    bearerToken
tokenValue      dbPassword      authSecret     sessionCookie
signatureKey

Several of these (sessionToken, bearerToken, dbPassword, sessionCookie) are at least as realistic as the 5 that were fixed. The new tests in tests/test_redaction_security.py only cover the 5 named cases, so this gap ships untested.

Root-cause options worth considering instead of enumerating more compounds one at a time:

  1. Tokenize the key (split on hyphen/underscore/camelCase boundaries) and check whether any resulting word matches the single-word list, rather than requiring the whole match to be boundary-anchored in the original string.
  2. Or, cheaper: drop the boundary requirement for the single-word alternatives entirely (closer to the original unanchored SECRET_KEY_RE behavior) and only keep anchoring for the compound alternatives that already need it (key alone must not match, e.g. --key-file/--public-key, which the test suite explicitly protects).

Happy to be more specific if useful — flagging since this is security-classified code and the current fix could read as "resolved" from the diff without closing the actual gap.

This branch has not been deployed

No deployments
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.

security: widen and de-duplicate the secret-name redaction heuristic

1 participant