fix: cap only generic sample tokens, one step, last; bump plugin-guard to v4
Follow-up to the cherry-picked #112146 (@KoNit-K): - Move the __main__ demotion to the end of _filter_findings behind a severity == "critical" guard and a MAIN_GUARD_DEMOTIONS table (the DOC_PROSE_DEMOTIONS idiom), so it is a one-step cap that can never re-raise a finding an earlier remap already lowered. - Treat ValueError from ast.parse (NUL bytes, undecodable text) like a SyntaxError: no cap, the file is runtime code (fail closed). - Bump PLUGIN_SCANNER_VERSION to plugin-guard-v4: the verdict semantics for a plugin shape changed, and the version is what install output and recorded provenance show. - Trim to two invariant tests: the reporter's repro shape (token inside the guard -> caution + confirmable/--force, same token above the guard -> dangerous, --force refused) and narrowness (destructive payload and a provider-shaped sk- key inside the guard, plus an unparseable file, all stay critical -> dangerous). - Docs: one paragraph on the __main__ cap next to the test-tree cap. Co-authored-by: kokhlo <konstantin.khlopkov93@gmail.com>
This commit is contained in:
@@ -264,40 +264,55 @@ class TestCautionPolicy:
|
||||
|
||||
|
||||
class TestRuntimeSelfTestTokens:
|
||||
def test_main_guard_sample_token_is_reviewable_caution(self, tmp_path):
|
||||
"""#112139: a sample token inside a root-level runtime file's
|
||||
``if __name__ == "__main__":`` self-test block is a fixture the loader never executes,
|
||||
so it caps at a confirmable ``caution``; the same literal above the guard is a real
|
||||
hardcoded credential and stays an un-overridable ``dangerous``."""
|
||||
|
||||
ENGINE = (
|
||||
"def make_execution_decision(**kw):\n"
|
||||
" return kw.get('token') is not None\n\n\n"
|
||||
)
|
||||
TOKEN_LINE = 'token="USR-session123-abc123def4567890"\n'
|
||||
|
||||
def test_main_guard_sample_token_is_reviewable_caution_but_module_level_is_not(self, tmp_path):
|
||||
files = dict(BASE_FILES)
|
||||
files["runtime.py"] = (
|
||||
"def request():\n"
|
||||
" return None\n\n"
|
||||
"if __name__ == '__main__':\n"
|
||||
" token = 'sampletokenvalue1234567890abcdef'\n"
|
||||
" assert token.startswith('sample')\n"
|
||||
files["phase6_policy_engine.py"] = (
|
||||
self.ENGINE + "if __name__ == '__main__':\n " + self.TOKEN_LINE
|
||||
)
|
||||
result = scan_plugin(_mk_plugin(tmp_path, files))
|
||||
(tmp_path / "guarded").mkdir()
|
||||
result = scan_plugin(_mk_plugin(tmp_path / "guarded", files))
|
||||
finding = next(f for f in result.findings if f.pattern_id == "hardcoded_secret")
|
||||
assert finding.severity == "high"
|
||||
assert result.verdict == "caution"
|
||||
assert should_allow_plugin_install(result)[0] is None
|
||||
assert should_allow_plugin_install(result, force=True)[0] is True
|
||||
|
||||
def test_runtime_token_outside_main_guard_stays_dangerous(self, tmp_path):
|
||||
files = dict(BASE_FILES)
|
||||
files["runtime.py"] = "token = 'sampletokenvalue1234567890abcdef'\n"
|
||||
result = scan_plugin(_mk_plugin(tmp_path, files))
|
||||
files["phase6_policy_engine.py"] = self.ENGINE + self.TOKEN_LINE
|
||||
(tmp_path / "module_level").mkdir()
|
||||
result = scan_plugin(_mk_plugin(tmp_path / "module_level", files))
|
||||
finding = next(f for f in result.findings if f.pattern_id == "hardcoded_secret")
|
||||
assert finding.severity == "critical"
|
||||
assert result.verdict == "dangerous"
|
||||
assert should_allow_plugin_install(result, force=True)[0] is False
|
||||
|
||||
def test_dedicated_token_signature_stays_critical_inside_main_guard(self, tmp_path):
|
||||
def test_only_generic_sample_tokens_are_demoted_inside_main_guard(self, tmp_path):
|
||||
"""The block is still executable code: a destructive payload and a provider-shaped
|
||||
key inside it keep their critical patterns, and a file that does not parse gets no cap."""
|
||||
files = dict(BASE_FILES)
|
||||
files["runtime.py"] = (
|
||||
"if __name__ == '__main__':\n"
|
||||
files["engine.py"] = (
|
||||
"import os\n\n"
|
||||
"if '__main__' == __name__:\n"
|
||||
" os.system('rm -rf /')\n"
|
||||
" token = 'sk-abcdefghijklmnopqrstuvwxyz'\n"
|
||||
)
|
||||
files["broken.py"] = "if __name__ == '__main__':\n " + self.TOKEN_LINE + "def broken(:\n"
|
||||
result = scan_plugin(_mk_plugin(tmp_path, files))
|
||||
assert any(
|
||||
f.pattern_id == "openai_key_leaked" and f.severity == "critical"
|
||||
for f in result.findings
|
||||
)
|
||||
critical = {(f.file, f.pattern_id) for f in result.findings if f.severity == "critical"}
|
||||
assert {("engine.py", "destructive_root_rm"), ("engine.py", "openai_key_leaked"),
|
||||
("broken.py", "hardcoded_secret")} <= critical
|
||||
assert result.verdict == "dangerous"
|
||||
assert should_allow_plugin_install(result, force=True)[0] is False
|
||||
|
||||
|
||||
class TestInstallIntegration:
|
||||
|
||||
@@ -19,7 +19,7 @@ from tools.skills_guard import (
|
||||
Finding, ScanResult, SUSPICIOUS_BINARY_EXTENSIONS, _determine_verdict, format_scan_report,
|
||||
scan_file)
|
||||
|
||||
PLUGIN_SCANNER_VERSION = "plugin-guard-v3"
|
||||
PLUGIN_SCANNER_VERSION = "plugin-guard-v4"
|
||||
|
||||
# Never scanned: VCS internals, caches, vendored envs.
|
||||
EXCLUDED_DIRS = {
|
||||
@@ -88,6 +88,15 @@ DOC_PROSE_DEMOTIONS = {
|
||||
"hardcoded_secret": "high",
|
||||
}
|
||||
|
||||
# A root-level ``if __name__ == "__main__":`` block is the module's own self-test harness:
|
||||
# ``plugins_loader`` imports plugins and never runs them as scripts, so a sample credential
|
||||
# quoted there is a fixture, not a shipped secret — the test-tree reasoning applied where a
|
||||
# root-level runtime file has no ``tests/`` to hold it (#112139). Narrower than the
|
||||
# test-tree cap because the block is still directly executable code: only the generic
|
||||
# sample-token pattern is demoted; destructive/persistence/exfil findings and the
|
||||
# provider-signature patterns (``sk-``, ``AKIA``, ``ghp_`` ...) keep full severity there.
|
||||
MAIN_GUARD_DEMOTIONS = {"hardcoded_secret": "high"}
|
||||
|
||||
# Structural limits — plugins are real codebases, far larger than skills.
|
||||
MAX_PLUGIN_FILE_COUNT = 400
|
||||
MAX_PLUGIN_TOTAL_SIZE_KB = 10 * 1024 # 10MB of scannable tree
|
||||
@@ -134,7 +143,7 @@ def _main_guard_body_lines(file_path: Path) -> set[int]:
|
||||
"""
|
||||
try:
|
||||
tree = ast.parse(file_path.read_text(encoding="utf-8"))
|
||||
except (OSError, SyntaxError, UnicodeDecodeError):
|
||||
except (OSError, SyntaxError, ValueError): # ValueError: UnicodeDecodeError, NUL bytes
|
||||
return set()
|
||||
lines: set[int] = set()
|
||||
for node in ast.walk(tree):
|
||||
@@ -162,8 +171,6 @@ def _filter_findings(findings: List[Finding], rel_path: str, file_path: Path) ->
|
||||
)
|
||||
if is_doc_prose and f.pattern_id in DOC_PROSE_DEMOTIONS:
|
||||
f.severity = DOC_PROSE_DEMOTIONS[f.pattern_id]
|
||||
if f.pattern_id == "hardcoded_secret" and f.line in main_guard_lines:
|
||||
f.severity = "high"
|
||||
if in_test_tree and f.severity == "critical":
|
||||
f.severity = "high"
|
||||
if (
|
||||
@@ -171,6 +178,14 @@ def _filter_findings(findings: List[Finding], rel_path: str, file_path: Path) ->
|
||||
and f.severity in _COMMENT_SEVERITY_CAP
|
||||
):
|
||||
f.severity = _COMMENT_SEVERITY_CAP[f.severity]
|
||||
# Last and critical-only: a one-step cap that can never re-raise a finding an
|
||||
# earlier remap already lowered.
|
||||
if (
|
||||
f.pattern_id in MAIN_GUARD_DEMOTIONS
|
||||
and f.severity == "critical"
|
||||
and f.line in main_guard_lines
|
||||
):
|
||||
f.severity = MAIN_GUARD_DEMOTIONS[f.pattern_id]
|
||||
out.append(f)
|
||||
return out
|
||||
|
||||
|
||||
@@ -693,6 +693,11 @@ there is capped at **caution**: their fixtures deliberately hold hostile
|
||||
strings to prove the plugin rejects them, so it asks for confirmation and
|
||||
`--force` overrides it instead of blocking the install outright. The same
|
||||
finding in any other file (`setup.sh`, `src/spec/…`) is still **dangerous**.
|
||||
Likewise, a generic sample token (`hardcoded_secret`) inside a runtime `.py`
|
||||
file's `if __name__ == "__main__":` self-test block is capped at **caution**
|
||||
— the loader imports plugins and never runs that block — while every other
|
||||
finding inside it (destructive commands, provider-shaped keys such as `sk-…`)
|
||||
and the same token anywhere above the guard keep full severity.
|
||||
|
||||
Scanning is on by default; disable it in `config.yaml`:
|
||||
|
||||
|
||||
Reference in New Issue
Block a user