Skip to content

Commit 77e2d6d

Browse files
rahuldass19rahuldass19
committed
fix(#34): token-complete placeholder exemptions; Sonar explicit concat (#30)
- Placeholder exemptions now must occupy the full value: password=required-secret, api_key=optional-token-9f3a, private_key=redacted-live-key are BLOCKED again (word-boundary escape closed). Pure placeholders (password: required, api_key: not set) still pass. PY+TS parity, 9/9 probe matrix. - Sonar: explicit string concatenation in HARMFUL_PATTERNS exempt-list (no implicit concat). - CodeRabbit PII-schema comment already fixed in prior working-tree commit; skipped with reason. - Tests: 244 passed incl. suffixed-placeholder parity regression tests.
1 parent abf7930 commit 77e2d6d

3 files changed

Lines changed: 77 additions & 26 deletions

File tree

‎npm/src/guards.ts‎

Lines changed: 32 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -637,15 +637,18 @@ export class SafetyGuard extends BaseGuard {
637637
// The value part excludes benign placeholder labels ("password:
638638
// required", "api_key: not set") while still matching real credentials
639639
// (Sentry/Greptile P1, PR #34). Mirrors safety_guard.py HARMFUL_PATTERNS.
640+
// Each exemption alternative must match the ENTIRE value token
641+
// ((?=\s|$) not \b) — otherwise "password=required-secret" bypasses
642+
// (Greptile/CodeRabbit P1, PR #34).
640643
private static HARMFUL_PATTERNS = [
641-
/password\s*[=:]\s*(?!(?:required|optional|none|null|redacted|omitted|placeholder|invalid|expired|not[_\s]?(?:set|provided)|n\/?a)\b|\*{3,}|x{3,})\S+/i,
642-
/api[_-]?key\s*[=:]\s*(?!(?:required|optional|none|null|redacted|omitted|placeholder|invalid|expired|not[_\s]?(?:set|provided)|n\/?a)\b|\*{3,}|x{3,})\S+/i,
643-
/secret\s*[=:]\s*(?!(?:required|optional|none|null|redacted|omitted|placeholder|invalid|expired|not[_\s]?(?:set|provided)|n\/?a)\b|\*{3,}|x{3,})\S+/i,
644+
/password\s*[=:]\s*(?!(?:required|optional|none|null|redacted|omitted|placeholder|invalid|expired|not[_\s]?(?:set|provided)|n\/?a)(?=\s|$)|\*{3,}(?=\s|$)|x{3,}(?=\s|$))\S+/i,
645+
/api[_-]?key\s*[=:]\s*(?!(?:required|optional|none|null|redacted|omitted|placeholder|invalid|expired|not[_\s]?(?:set|provided)|n\/?a)(?=\s|$)|\*{3,}(?=\s|$)|x{3,}(?=\s|$))\S+/i,
646+
/secret\s*[=:]\s*(?!(?:required|optional|none|null|redacted|omitted|placeholder|invalid|expired|not[_\s]?(?:set|provided)|n\/?a)(?=\s|$)|\*{3,}(?=\s|$)|x{3,}(?=\s|$))\S+/i,
644647
// Value-aware label form (same placeholder exemption as above) —
645648
// "private[_-]?key" bare-matching blocked benign labels such as
646649
// "private_key: not set" (Greptile P1, PR #34). [\s_-]? also catches
647650
// the spaced "private key: <value>" form. Mirrors safety_guard.py.
648-
/private[\s_-]?key\s*[=:]\s*(?!(?:required|optional|none|null|redacted|omitted|placeholder|invalid|expired|not[_\s]?(?:set|provided)|n\/?a)\b|\*{3,}|x{3,})\S+/i,
651+
/private[\s_-]?key\s*[=:]\s*(?!(?:required|optional|none|null|redacted|omitted|placeholder|invalid|expired|not[_\s]?(?:set|provided)|n\/?a)(?=\s|$)|\*{3,}(?=\s|$)|x{3,}(?=\s|$))\S+/i,
649652
// Generic PEM header: BEGIN [TYPE] PRIVATE KEY — covers RSA/DSA/EC
650653
// plus generic "BEGIN PRIVATE KEY", OPENSSH and ENCRYPTED variants
651654
// that were missed (CodeRabbit, PR #34). Python applies re.I to all
@@ -681,19 +684,30 @@ export class SafetyGuard extends BaseGuard {
681684
{ error: String(err) },
682685
);
683686
}
684-
const issues: string[] = [];
687+
// Python parity: issues is a uniform array of
688+
// {type, severity, details} objects for BOTH paths — the error path
689+
// (all issues, errors AND warnings) and the warning-only path
690+
// (Sentry, PR #34: PII used to be plain strings here and verbose
691+
// {type: 'PII detected: email', details: []} objects there).
692+
const issues: Array<{ type: string; severity: string; details: string[] }> = [];
685693
// Error-severity issues are COLLECTED across checks — matching
686694
// Python, which appends injection and harmful findings together and
687695
// reports the total (CodeAnt nitpick, PR #34: returning on the first
688696
// harmful pattern discarded PII and other diagnostics).
689697
const errorIssues: Array<{ type: string; severity: string; details: string[] }> = [];
690698

691699
if (this.checkPii) {
700+
// Python parity: one {type:'pii', severity:'warning'} entry whose
701+
// details carry the matched PII types (email, phone, ...).
702+
const piiTypes: string[] = [];
692703
for (const [type, pattern] of Object.entries(SafetyGuard.PII_PATTERNS)) {
693704
if (pattern.test(content)) {
694-
issues.push(`PII detected: ${type}`);
705+
piiTypes.push(type);
695706
}
696707
}
708+
if (piiTypes.length > 0) {
709+
issues.push({ type: 'pii', severity: 'warning', details: piiTypes });
710+
}
697711
}
698712

699713
if (this.checkInjection) {
@@ -704,7 +718,9 @@ export class SafetyGuard extends BaseGuard {
704718
}
705719
}
706720
if (injections.length > 0) {
707-
errorIssues.push({ type: 'injection', severity: 'error', details: injections });
721+
const entry = { type: 'injection', severity: 'error', details: injections };
722+
issues.push(entry);
723+
errorIssues.push(entry);
708724
}
709725
}
710726

@@ -718,7 +734,9 @@ export class SafetyGuard extends BaseGuard {
718734
}
719735
}
720736
if (harmful.length > 0) {
721-
errorIssues.push({ type: 'harmful', severity: 'error', details: harmful });
737+
const entry = { type: 'harmful', severity: 'error', details: harmful };
738+
issues.push(entry);
739+
errorIssues.push(entry);
722740
}
723741
}
724742

@@ -729,16 +747,16 @@ export class SafetyGuard extends BaseGuard {
729747
// which returns details={'issues': issues} with everything
730748
// (Sentry, PR #34: PII warnings were discarded when a
731749
// critical issue co-existed).
732-
{
733-
issues: errorIssues.concat(
734-
issues.map((w) => ({ type: w, severity: 'warning', details: [] })),
735-
),
736-
},
750+
{ issues },
737751
);
738752
}
739753

740754
if (issues.length > 0) {
741-
return this.failResult(`Safety issues detected: ${issues.join(', ')}`, { issues }, 'warning');
755+
return this.failResult(
756+
`Safety warnings: ${issues.length} warning(s)`,
757+
{ issues },
758+
'warning',
759+
);
742760
}
743761

744762
return this.passResult('All safety checks passed');

‎src/qwed_open_responses/guards/safety_guard.py‎

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -62,23 +62,25 @@ class SafetyGuard(BaseGuard):
6262
# labels ("password: required", "api_key: not set") that are common in
6363
# ordinary status text but still matches real credentials
6464
# ("api_key=sk-12345") (Sentry/Greptile P1, PR #34). Mirrored in npm.
65+
# The exemption alternatives must match the ENTIRE value token — the
66+
# old \b let "password=required-secret" bypass (placeholder prefix +
67+
# suffix), so each alternative asserts whitespace/end-of-string next
68+
# (Greptile/CodeRabbit P1, PR #34).
69+
_CREDENTIAL_EXEMPTION = (
70+
r"(?!(?:required|optional|none|null|redacted|omitted|placeholder|"
71+
r"invalid|expired|not[_\s]?(?:set|provided)|n/?a)(?=\s|$)"
72+
r"|\*{3,}(?=\s|$)|x{3,}(?=\s|$))\S+"
73+
)
74+
6575
HARMFUL_PATTERNS = [
66-
r"password\s*[=:]\s*(?!(?:required|optional|none|null|redacted|"
67-
r"omitted|placeholder|invalid|expired|not[_\s]?(?:set|provided)|"
68-
r"n/?a)\b|\*{3,}|x{3,})\S+",
69-
r"api[_-]?key\s*[=:]\s*(?!(?:required|optional|none|null|redacted|"
70-
r"omitted|placeholder|invalid|expired|not[_\s]?(?:set|provided)|"
71-
r"n/?a)\b|\*{3,}|x{3,})\S+",
72-
r"secret\s*[=:]\s*(?!(?:required|optional|none|null|redacted|"
73-
r"omitted|placeholder|invalid|expired|not[_\s]?(?:set|provided)|"
74-
r"n/?a)\b|\*{3,}|x{3,})\S+",
76+
r"password\s*[=:]\s*" + _CREDENTIAL_EXEMPTION,
77+
r"api[_-]?key\s*[=:]\s*" + _CREDENTIAL_EXEMPTION,
78+
r"secret\s*[=:]\s*" + _CREDENTIAL_EXEMPTION,
7579
# Value-aware label form (same placeholder exemption as above) —
7680
# "private[_-]?key" bare-matching blocked benign labels such as
7781
# "private_key: not set" (Greptile P1, PR #34). The [\s_-]? class
7882
# also catches the spaced "private key: <value>" form.
79-
r"private[\s_-]?key\s*[=:]\s*(?!(?:required|optional|none|null|"
80-
r"redacted|omitted|placeholder|invalid|expired|"
81-
r"not[_\s]?(?:set|provided)|n/?a)\b|\*{3,}|x{3,})\S+",
83+
r"private[\s_-]?key\s*[=:]\s*" + _CREDENTIAL_EXEMPTION,
8284
# Generic PEM header: BEGIN [TYPE] PRIVATE KEY — covers RSA/DSA/EC
8385
# (the old list) plus generic "BEGIN PRIVATE KEY", OPENSSH and
8486
# ENCRYPTED variants that were missed (CodeRabbit, PR #34).

‎tests/test_guards.py‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1272,6 +1272,37 @@ def test_private_key_credential_forms_blocked(self):
12721272
result = SafetyGuard().check({"type": "text", "content": text})
12731273
assert result.passed is False, text
12741274

1275+
# ------------------------------------------------------------------
1276+
# #34: placeholder exemptions must consume the ENTIRE value token —
1277+
# placeholder-prefixed credentials must still be detected
1278+
# (Greptile/CodeRabbit P1).
1279+
# ------------------------------------------------------------------
1280+
1281+
def test_placeholder_prefixed_credentials_blocked(self):
1282+
cases = [
1283+
"password=required-secret",
1284+
"api_key=optional-token-9f3a",
1285+
"private_key=redacted-live-key",
1286+
"secret=n/a-backup-key",
1287+
"password=***-secret",
1288+
]
1289+
for text in cases:
1290+
result = SafetyGuard().check({"type": "text", "content": text})
1291+
assert result.passed is False, text
1292+
1293+
def test_pure_placeholders_still_pass(self):
1294+
cases = [
1295+
"password=required",
1296+
"api_key=not set",
1297+
"private_key: not set",
1298+
"secret=none",
1299+
"password: ***",
1300+
"system: healthy",
1301+
]
1302+
for text in cases:
1303+
result = SafetyGuard().check({"type": "text", "content": text})
1304+
assert result.passed is True, text
1305+
12751306
def test_failed_result_details_include_warnings(self):
12761307
"""Error result details carry PII warnings too (Python parity)."""
12771308
result = SafetyGuard().check({

0 commit comments

Comments
 (0)