Repository navigation
fix(core): distinguish secret prose from credential fields - #278
loveRhythm1990 merged 3 commits into
Conversation
ReviewThe issue is valid. On Blocking: credential forms that
|
| input | main |
this PR |
|---|---|---|
GITHUB_CLIENT_SECRET=ghs_abcdef123456 |
blocked | allowed |
JWT_SECRET=abcdef123456 |
blocked | allowed |
app_secret: abcdef123456 |
blocked | allowed |
webhook_secret = abcdef123456 |
blocked | allowed |
clientSecret: abcdef123456 |
blocked | allowed |
userPassword=abcdef123456 |
blocked | allowed |
dbPassword: abcdef123456 |
blocked | allowed |
rootpassword=abcdef123456 |
blocked | allowed |
- secret: abcdef123456 (YAML list item) |
blocked | allowed |
These causes produce the misses:
\b(?:api|client|auth)[ _-]secret: inGITHUB_CLIENT_SECRET,_is a word character, so there is no\bbeforeCLIENT. The\bsecret\balternative fails for the same reason. Every*_SECRET=env var outside the closed list is missed.- camelCase (
clientSecret,userPassword) has no word boundary before the keyword, and the optional prefix only allows_/-separators. - The structured-field branch accepts only
^,{,[, and,as boundaries, so it misses a YAML list item (- secret:).
The false positive in #276 comes only from the standalone word secret followed by :. Narrowing the change to that case would fix the issue with no loss of coverage, for example:
password/passwd: keep themainbehavior, with no leading\b.password:is almost never harmless prose.secretthat is part of an identifier (preceded by[A-Za-z0-9_-], e.g.JWT_SECRET,clientSecret,app-secret): block on both:and=.- standalone
\bsecret\b: block on=anywhere. Block on:only at a field boundary, i.e.^,{,[,,, or a YAML-list marker.
That covers every case in the table, and the prose examples in this PR's tests still pass. I'd also drop compassword: ordinary-text / notpassword=ordinary-text from embedded_words_are_not_credential_field_names. Under the rule above they would (correctly) be blocked, and they aren't realistic false positives. Please add the rows above as regression cases.
Scope relative to #276
#276's acceptance criteria also ask for an actionable, non-secret classification in the API response (so callers can tell a content-policy 403 from an auth/scope 403), plus a re-run of the first-history selection. This PR covers only the detector, so Fixes #276 would close the issue too early. Please change it to Related: #276 or split out the remaining items.
Minor
Our client secret: great serviceand a line that starts withSecret: ...are still blocked. The PR description says so, and that trade-off is fine.- The original MEDIUM-tier and other HIGH-tier checks are unchanged and still covered by tests. 👍
Re-review of
|
| input | main |
this PR |
|---|---|---|
I'll share a secret: **dry brining** |
blocked | allowed ✅ |
**Secret:** dry brining |
blocked | allowed ✅ |
export SECRET_KEY=… / DJANGO_SECRET_KEY=… |
allowed | blocked ✅ |
spring.datasource.password: … |
blocked | blocked |
jwt.secret: abcdef123456 |
blocked | allowed |
(secret: …), | secret: … | |
blocked | allowed |
Top-secret: the family recipe… |
blocked | blocked |
Non-blocking suggestion
jwt.secret: value is a real config form (.properties accepts : as a separator, and flattened Spring/Helm keys use it too). It's the one identifier case still lost, because . isn't in [\w-]. Widening the class to [\w.-]secret would fix it. Please add jwt.secret: … to the identifier test. The parenthesis/table cases are ambiguous, and allowing them is a reasonable trade-off.
Top-secret: … / trade-secret: … prose is still blocked because of the - in the class, but main already blocks those, so it's not a regression.
LGTM with or without the . tweak.
What type of PR is this?
Which issue(s) this PR fixes
Related: #276 (detector portion). An actionable API classification distinguishing content-policy rejection from authentication/scope rejection, and the live first-history benchmark rerun, remain outstanding; this PR must not automatically close the issue.
What this PR does / why we need it
Ordinary text such as
I'll share a secret: **dry brining**is rejected by the HIGH-tier credential detector, blocking the first LongMemEval-S history before retrieval. Require a field boundary for a baresecret:key instead of matching that standalone word anywhere in prose.Preserve legacy password/passwd assignment detection without requiring a leading word boundary. Block secret suffixes in arbitrary identifiers, including environment variables, snake_case, camelCase, hyphenated names, and Unicode prefixes. Examples include
GITHUB_CLIENT_SECRET,JWT_SECRET,app_secret,clientSecret,userPassword,dbPassword, androotpassword. Continue blocking explicit credential aliases, baresecret=anywhere, and structuredsecret:fields at line/object boundaries, including quoted keys and YAML list items such as- secret: value.The change uses syntax-based rules and no dataset-specific phrase allowlist. A standalone line beginning with
Secret: ...and an explicit alias such asOur client secret: ...remain conservatively blocked even if intended as prose. Real cloud keys, private keys, bearer tokens, and MEDIUM-tier redaction retain their protection.Validation
cargo test --offline -p memoria-core --lib: 18 passed, including five regression test functions with multiple input cases.--all-targets -- -D warnings), rustfmt, andgit diff --check: passed.