From dd2141b928ee2daf2bea513bafde191ed84725ee Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Wed, 16 Sep 2026 10:06:09 -0700 Subject: [PATCH] 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 --- tests/tools/test_plugin_guard.py | 53 +++++++++++++-------- tools/plugin_guard.py | 23 +++++++-- website/docs/user-guide/features/plugins.md | 5 ++ 3 files changed, 58 insertions(+), 23 deletions(-) diff --git a/tests/tools/test_plugin_guard.py b/tests/tools/test_plugin_guard.py index 9e31b15e39..61d3d695e2 100644 --- a/tests/tools/test_plugin_guard.py +++ b/tests/tools/test_plugin_guard.py @@ -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: diff --git a/tools/plugin_guard.py b/tools/plugin_guard.py index c93a6c9ffc..13702e67cc 100644 --- a/tools/plugin_guard.py +++ b/tools/plugin_guard.py @@ -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 diff --git a/website/docs/user-guide/features/plugins.md b/website/docs/user-guide/features/plugins.md index f78eb98048..a807e96b42 100644 --- a/website/docs/user-guide/features/plugins.md +++ b/website/docs/user-guide/features/plugins.md @@ -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`: