security: centralize secret-name redaction heuristics - #400
codeforester wants to merge 2 commits into
Conversation
|
|
||
| REDACTED = "[REDACTED]" | ||
| SECRET_KEY_RE = re.compile(r"(token|password|secret|api[-_]?key|authorization)", re.IGNORECASE) | ||
| SECRET_KEY_PATTERN = ( |
There was a problem hiding this comment.
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.
| `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, |
There was a problem hiding this comment.
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.
|
Following up on the camelCase finding: the fix adds explicit alternatives for the 5 examples I named in the failure scenario ( But this patches the named examples rather than the underlying gap: the single-word alternatives ( Several of these ( Root-cause options worth considering instead of enumerating more compounds one at a time:
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. |
Fixes #384