diff --git a/skills/kvb-check-security/SKILL.md b/skills/kvb-check-security/SKILL.md index ddc3862..0a5d33a 100644 --- a/skills/kvb-check-security/SKILL.md +++ b/skills/kvb-check-security/SKILL.md @@ -46,10 +46,12 @@ value. Pass `` as `--security` and the critical count as `--critical` `--security '~'` (quoted: zsh expands a bare `~100` as a user's home) and the critical findings you found as `--critical`. - Exit 2: usage error. - **Accepted false positives**: `security.py accept --repo --rule - [--rule ...] --path --sha1 --reason ""` + [--rule ...] --path --sha1 --line --reason ""` (from `--json`) appends a row per rule to the council root, which the `kvb-org-organize` watch hook mirrors to `~/.local/state/agents-council/`. - A match leaves the score as ` accepted` until the line changes. Accept + A match leaves the score as ` accepted` until the line or its + neighbourhood changes; `--line` is required when the same text occurs on + several lines, so one acceptance never hides an identical line. Accept only after `kvb-think-criticize` judged it a false positive or by design; name each acceptance in the commit message. diff --git a/skills/kvb-check-security/scripts/security.py b/skills/kvb-check-security/scripts/security.py index a0dfe1f..91835a3 100755 --- a/skills/kvb-check-security/scripts/security.py +++ b/skills/kvb-check-security/scripts/security.py @@ -6,9 +6,11 @@ Args: --base: git ref the change starts from. --json: print the figures as JSON instead of the line. - accept --repo --rule [--rule ...] --path --sha1 <40 hex> --reason : + accept --repo --rule [--rule ...] --path --sha1 <40 hex> [--line ] --reason : record reviewed false positives of one line instead, a row per rule; --json - of a scan prints rule, path and sha1 of every finding. + of a scan prints rule, path, line and sha1 of every finding. --line binds + the row to that line's neighbourhood and is required when the line's text + occurs more than once in the file. Scan: The regular files `git diff ` names plus untracked ones, symlinks left @@ -20,13 +22,17 @@ Scan: Accepted: One JSON row per reviewed false positive (id, rule, path, sha1 of the stripped - source line, or of the whole file for a finding without a line, reason, repo, - date), appended by accept to + source line, or of the whole file for a finding without a line, context: the + sha1 of the stripped lines up to REACH above and below it when accept got + --line, reason, repo, date), appended by accept to /security-accepted.jsonl of the kvb-org-organize watch.py, whose hook keeps the rows in ~/.local/state/agents-council/security-accepted.jsonl; - a scan reads both. A finding with that rule, path and text is accepted and - leaves the score; an edit of the line, or of the file, brings it back. A row - without rule, path and sha1 is a usage error. + a scan reads both. A finding with that rule, path, text and context is + accepted and leaves the score; a row without context accepts it only while no + other finding of the scan shares its rule, path and text, so one acceptance + never hides an identical line elsewhere. An edit of the line, its + neighbourhood or the file brings it back. A row without rule, path and sha1 + is a usage error. Output: `🛡️ security % (medusa: findings, accepted in files: :, ...)`, @@ -55,11 +61,13 @@ import shutil import subprocess import sys import tempfile +from collections import Counter from pathlib import Path WEIGHTS = {"critical": 50, "high": 25, "medium": 10, "low": 2, "info": 0} SAFE = 100 SHOWN = 5 +REACH = 2 SHA1 = re.compile(r"[0-9a-f]{40}") WATCH = Path(__file__).resolve().parents[2] / "kvb-org-organize" / "scripts" / "watch.py" HAND = ( @@ -115,17 +123,27 @@ def located(name: str, tree: Path) -> str: return name -def fingerprint(tree: Path, path: str, line: int | None) -> str | None: +def digest(text: str) -> str: + return hashlib.sha1(text.encode()).hexdigest() + + +def fingerprint(tree: Path, path: str, line: int | None) -> tuple[str | None, str | None]: + """Returns: + (sha1 of the stripped line or of the whole file without a line, sha1 of + the stripped lines up to REACH around the line or None), (None, None) for + an unreadable file or a line outside it. + """ try: text = (tree / path).read_text(encoding="utf-8", errors="replace") except OSError: - return None + return None, None if line is None: - return hashlib.sha1(text.encode()).hexdigest() + return digest(text), None rows = text.splitlines() if not 1 <= line <= len(rows): - return None - return hashlib.sha1(rows[line - 1].strip().encode()).hexdigest() + return None, None + around = rows[max(0, line - 1 - REACH):line + REACH] + return digest(rows[line - 1].strip()), digest("\n".join(row.strip() for row in around)) def watch(): @@ -137,7 +155,7 @@ def watch(): return loaded -def accepted(listings: tuple[Path, ...]) -> set[tuple[str, str, str]]: +def accepted(listings: tuple[Path, ...]) -> set[tuple[str, str, str, str | None]]: entries = set() for listing in listings: if not listing.is_file(): @@ -147,28 +165,48 @@ def accepted(listings: tuple[Path, ...]) -> set[tuple[str, str, str]]: continue try: row = json.loads(line) - entries.add((row["rule"], row["path"], row["sha1"])) - except (ValueError, KeyError, TypeError): + entries.add((row["rule"], row["path"], row["sha1"], row.get("context"))) + except (ValueError, KeyError, TypeError, AttributeError): fail(f"{listing}:{number}: want a JSON row with rule, path and sha1") return entries +def covered(finding: dict, reviewed: set[tuple[str, str, str, str | None]], shared: Counter) -> bool: + key = (finding["rule"], finding["path"], finding["sha1"]) + return (*key, finding["context"]) in reviewed or (shared[key] == 1 and (*key, None) in reviewed) + + def accept(argv: list[str]) -> int: parser = argparse.ArgumentParser(prog="security.py accept", description="Record a reviewed false positive.") for flag in ("--repo", "--path", "--sha1", "--reason"): parser.add_argument(flag, required=True) parser.add_argument("--rule", action="append", required=True) + parser.add_argument("--line", type=int) args = parser.parse_args(argv) if not SHA1.fullmatch(args.sha1): parser.error("--sha1 must be 40 lowercase hex digits") if not args.reason.strip() or "\n" in args.reason: parser.error("--reason must be one non-empty line") root = git(Path(args.repo), "rev-parse", "--show-toplevel").strip() + context = None + if args.line is not None: + sha1, context = fingerprint(Path(root), args.path, args.line) + if sha1 != args.sha1: + parser.error(f"--sha1 is not the sha1 of line {args.line} of {args.path}") + else: + try: + rows = (Path(root) / args.path).read_text(encoding="utf-8", errors="replace").splitlines() + except OSError: + rows = [] + twins = sum(digest(row.strip()) == args.sha1 for row in rows) + if twins > 1: + parser.error(f"the line occurs {twins} times in {args.path}; name the reviewed one with --line") date = datetime.datetime.now(datetime.UTC).isoformat(timespec="seconds") rows = [ { - "id": hashlib.sha1(f"{rule}\t{args.path}\t{args.sha1}".encode()).hexdigest(), "rule": rule, - "path": args.path, "sha1": args.sha1, "reason": args.reason, "repo": root, "date": date, + "id": digest("\t".join([rule, args.path, args.sha1, *([context] if context else [])])), "rule": rule, + "path": args.path, "sha1": args.sha1, **({"context": context} if context else {}), + "reason": args.reason, "repo": root, "date": date, } for rule in args.rule ] @@ -199,12 +237,14 @@ def findings(reports: Path, tree: Path) -> list[dict]: raise Unreadable(f"{files[-1].name}: a finding without a known severity") line = entry.get("line") if isinstance(entry.get("line"), int) else None path = located(str(entry.get("file") or "?"), tree) + sha1, context = fingerprint(tree, path, line) found.append({ "severity": severity, "rule": entry.get("rule_id") if isinstance(entry.get("rule_id"), str) else None, "path": path, "line": line, - "sha1": fingerprint(tree, path, line), + "sha1": sha1, + "context": context, }) rank = list(WEIGHTS) return sorted(found, key=lambda f: (rank.index(f["severity"]), f["path"], f["line"] or 0)) @@ -266,7 +306,8 @@ def main() -> int: print(f"🛡️ security – ({problem}: {HAND})") return 4 - passed = [f for f in every if (f["rule"], f["path"], f["sha1"]) in reviewed] + shared = Counter((f["rule"], f["path"], f["sha1"]) for f in every) + passed = [f for f in every if covered(f, reviewed, shared)] found = [f for f in every if f not in passed] counts = {severity: sum(f["severity"] == severity for f in found) for severity in WEIGHTS} value = max(0, SAFE - sum(WEIGHTS[s] * n for s, n in counts.items())) diff --git a/tests/test_security.py b/tests/test_security.py index 83aeb50..d628e7b 100644 --- a/tests/test_security.py +++ b/tests/test_security.py @@ -108,6 +108,7 @@ class TestSecurity(unittest.TestCase): { "severity": "high", "rule": "FAKE-HIGH", "path": "app.py", "line": 2, "sha1": hashlib.sha1(f"token = '{PLACEHOLDER}' # MARK-HIGH".encode()).hexdigest(), + "context": hashlib.sha1(f"print(1)\ntoken = '{PLACEHOLDER}' # MARK-HIGH".encode()).hexdigest(), }, ) self.assertEqual(found["accepted"], []) @@ -148,6 +149,35 @@ class TestSecurity(unittest.TestCase): ("🛡️ security 50 % (medusa: 1 finding in 1 file: critical app.py:2)\n", 1), ) + def test_one_acceptance_never_hides_an_identical_line_elsewhere(self): + twin = "x = fetch(url) # MARK-HIGH" + self.write("app.py", f"print(1)\n{twin}\ny = 2\n{twin}\n") + sha1 = hashlib.sha1(twin.encode()).hexdigest() + both = ("🛡️ security 50 % (medusa: 2 findings in 1 file: high app.py:2, high app.py:4)\n", 3) + listing = self.base / "council" / "security-accepted.jsonl" + listing.parent.mkdir(parents=True) + listing.write_text(json.dumps({"rule": "FAKE-HIGH", "path": "app.py", "sha1": sha1}) + "\n", encoding="utf-8") + result = self.scan(env=self.env) + self.assertEqual((result.stdout, result.returncode), both) + listing.unlink() + refused = self.accept("--rule", "FAKE-HIGH", "--path", "app.py", "--sha1", sha1, "--reason", "fixture") + self.assertEqual(refused.returncode, 2) + self.assertIn("the line occurs 2 times in app.py; name the reviewed one with --line", refused.stderr) + self.assertFalse(listing.exists()) + wrong = self.accept("--rule", "FAKE-HIGH", "--path", "app.py", "--sha1", sha1, "--line", "3", "--reason", "fixture") + self.assertEqual(wrong.returncode, 2) + self.assertIn("--sha1 is not the sha1 of line 3 of app.py", wrong.stderr) + named = self.accept("--rule", "FAKE-HIGH", "--path", "app.py", "--sha1", sha1, "--line", "4", "--reason", "fixture") + self.assertEqual(named.returncode, 0, named.stderr) + result = self.scan(env=self.env) + self.assertEqual( + (result.stdout, result.returncode), + ("🛡️ security 75 % (medusa: 1 finding, 1 accepted in 1 file: high app.py:2)\n", 3), + ) + self.write("app.py", f"print(1)\n{twin}\ny = 3\n{twin}\n") + result = self.scan(env=self.env) + self.assertEqual((result.stdout, result.returncode), both) + def test_kept_rows_count_and_broken_rows_stop(self): line = "y = other() # MARK-HIGH" self.write("app.py", f"{line}\n")