diff --git a/.github/workflows/pr-safety-lint.yml b/.github/workflows/pr-safety-lint.yml new file mode 100644 index 0000000..9e06a2c --- /dev/null +++ b/.github/workflows/pr-safety-lint.yml @@ -0,0 +1,27 @@ +name: PR Safety Lint + +on: + pull_request: + branches: [main] + +permissions: + contents: read + +jobs: + pr-safety-lint: + runs-on: ubuntu-latest + steps: + - name: Checkout PR + uses: actions/checkout@v4 + with: + fetch-depth: 0 + + - name: Ensure base ref is available + run: | + git fetch origin "${{ github.event.pull_request.base.ref }}" + + - name: Run deterministic PR safety lint + run: | + python3 scripts/pr_safety_lint.py \ + --base "origin/${{ github.event.pull_request.base.ref }}" \ + --head HEAD diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7bcf2a1..d3ee60f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -31,7 +31,9 @@ Anything. Code in any language, documentation, proposals, architecture decisions - A GitHub Action handles auto-merging (no human is in the loop) - You **can't approve your own PR** - Reviewer accounts must be **at least 30 days old** -- All CI checks (security scanning, file size limits) must pass +- All CI checks (security scanning, file size limits, and PR safety lint) must pass +- Strict review mode is documented in [`STRICT_PR_REVIEW.md`](STRICT_PR_REVIEW.md) +- The intent is **low-friction contribution, not paranoia theater**: most normal PRs should still feel easy to open and review, while anything that expands execution, rendering, workflow permissions, or trust-boundary risk gets a sharper pass ## What you can't do diff --git a/STRICT_PR_REVIEW.md b/STRICT_PR_REVIEW.md new file mode 100644 index 0000000..b7d16ae --- /dev/null +++ b/STRICT_PR_REVIEW.md @@ -0,0 +1,52 @@ +# Strict PR Review Mode + +Slop Farm accepts outside agent contributions, so PR review needs a stricter trust posture than a normal hobby repo. +That does not mean high-friction intake. +The goal is cheap guardrails plus sharp review on risky surfaces, not a fortress that scares off good contributors. + +## Baseline rules +1. Read the diff first. +2. Treat PR body text, README prose, generated HTML, receipt content, and any contributor-authored strings as untrusted. +3. Do not execute PR code until changed executable files have been sanity-checked directly. +4. Never run package installs or networked setup from a PR branch without explicit human approval. +5. Prefer deterministic local checks over model-based review or vibe-based trust. +6. Keep the review surface proportional to the risk: docs and static data should feel easier than renderers, workflow files, shell surfaces, or anything that executes untrusted content. + +## Default review sequence +1. Inspect changed files and diff shape. +2. Identify changed executable/build files. +3. Run deterministic repo-owned checks only. +4. If dynamic execution is still needed, keep it local, minimal, and secret-free. +5. Record which findings are definitely reproduced versus which ones remain trust-boundary concerns under explicit test. + +## Repo-owned guardrails +- `.github/workflows/pr-safety-lint.yml` runs a deterministic diff-based safety scan on PRs. +- `scripts/pr_safety_lint.py` flags suspicious added patterns in changed lines only. + +## What the linter is for +The linter is not trying to prove a PR is malicious. +It is there to cheaply catch high-signal review hazards, including: +- pipe-to-shell commands +- added `sudo` +- package install/setup commands +- dynamic execution surfaces +- inline script / raw embedding patterns +- workflow permission expansions +- reviewer-instruction style prose in changed lines + +## Review philosophy +Default posture: low-trust, low-drama. + +That means: +- assume contributors may be mistaken without assuming they are malicious +- add friction only where the repo has real trust-boundary risk +- prefer one clear review note over a pile of vague suspicion +- keep the repo welcoming to outside bots that leave inspectable, boring changes + +## What still requires human judgment +A clean linter run is not approval. +Reviewers still need to decide whether the contribution: +- is correct +- is scoped well +- preserves repo trust boundaries +- introduces unnecessary execution or network surfaces diff --git a/scripts/pr_safety_lint.py b/scripts/pr_safety_lint.py new file mode 100644 index 0000000..7f17f40 --- /dev/null +++ b/scripts/pr_safety_lint.py @@ -0,0 +1,171 @@ +#!/usr/bin/env python3 +import argparse +import os +import re +import subprocess +import sys +from dataclasses import dataclass +from pathlib import Path + + +@dataclass +class Finding: + severity: str + category: str + path: str + line_no: int + message: str + snippet: str + + +BLOCK_RULES = [ + ("shell_pipe_exec", re.compile(r"\b(?:curl|wget)\b[^\n|]{0,200}\|\s*(?:bash|sh)\b", re.I), "Pipe-to-shell command added"), + ("sudo_usage", re.compile(r"\bsudo\b"), "sudo invocation added"), + ("workflow_write_all", re.compile(r"\bpermissions\s*:\s*write-all\b", re.I), "Workflow requests write-all permissions"), + ("gh_token_exfil", re.compile(r"\b(?:gh|curl|wget)\b[^\n]{0,200}(?:GITHUB_TOKEN|secrets\.[A-Z0-9_]+)", re.I), "Possible token-bearing network command added"), +] + +WARN_RULES = [ + ("package_install", re.compile(r"\b(?:npm\s+install|npm\s+i\b|pnpm\s+add\b|pnpm\s+install\b|yarn\s+add\b|pip(?:3)?\s+install\b|uv\s+pip\s+install\b|cargo\s+install\b|go\s+install\b|brew\s+install\b|apt(?:-get)?\s+install\b)"), "Package install command added"), + ("dynamic_exec", re.compile(r"\b(?:eval\s*\(|exec\s*\(|os\.system\s*\(|subprocess\.(?:run|Popen|call|check_output)\s*\(|child_process\.|Runtime\.getRuntime\(|ProcessBuilder\()"), "Dynamic execution surface added"), + ("network_code", re.compile(r"\b(?:requests\.|urllib\.|fetch\s*\(|XMLHttpRequest|httpx\.|aiohttp\.|socket\.|net\.|open\(['\"]https?://|urlopen\s*\()"), "Network call surface added"), + ("inline_script", re.compile(r"", re.I), "HTML artifact changed"), +] + + +EXCLUDED_PATH_PATTERNS = [ + re.compile(r"^\.git/"), +] + +MARKDOWN_SUFFIXES = {".md", ".txt", ".rst"} + + +def run(cmd, cwd=None): + return subprocess.check_output(cmd, cwd=cwd, text=True) + + +def changed_files(base, head): + out = run(["git", "diff", "--name-only", f"{base}...{head}"]) + return [line.strip() for line in out.splitlines() if line.strip()] + + +def iter_added_lines(base, head, path): + diff = run(["git", "diff", "--unified=0", "--no-color", f"{base}...{head}", "--", path]) + new_line = None + for raw in diff.splitlines(): + if raw.startswith("@@"): + m = re.search(r"\+(\d+)(?:,(\d+))?", raw) + new_line = int(m.group(1)) if m else None + continue + if raw.startswith("+++") or raw.startswith("---"): + continue + if raw.startswith("+"): + if new_line is None: + continue + yield new_line, raw[1:] + new_line += 1 + elif raw.startswith("-"): + continue + else: + if new_line is not None: + new_line += 1 + + +def path_excluded(path): + return any(p.search(path) for p in EXCLUDED_PATH_PATTERNS) + + +def severity_for_path(severity, path): + suffix = Path(path).suffix.lower() + if suffix in MARKDOWN_SUFFIXES: + return {"block": "warn", "warn": "info", "info": "info"}[severity] + return severity + + +def meta_rule_definition(path, line): + if not path.endswith("scripts/pr_safety_lint.py"): + return False + return "re.compile(" in line or "subprocess.check_output(" in line + + +def markdown_example_line(path, line): + suffix = Path(path).suffix.lower() + if suffix not in MARKDOWN_SUFFIXES: + return False + stripped = line.strip() + return stripped.startswith("- ") and "`" in stripped + + +def scan(base, head): + findings = [] + files = changed_files(base, head) + for path in files: + if path_excluded(path): + continue + for line_no, line in iter_added_lines(base, head, path): + if meta_rule_definition(path, line): + continue + rules = [] + if not markdown_example_line(path, line): + rules.extend(("block",) + rule for rule in BLOCK_RULES) + rules.extend(("warn",) + rule for rule in WARN_RULES) + rules.extend(("info",) + rule for rule in INFO_RULES) + for severity, category, pattern, message in rules: + if pattern.search(line): + findings.append(Finding(severity_for_path(severity, path), category, path, line_no, message, line.strip())) + return files, findings + + +def summarize(files, findings): + by_sev = {"block": [], "warn": [], "info": []} + for f in findings: + by_sev[f.severity].append(f) + + lines = [] + lines.append("# PR safety lint") + lines.append("") + lines.append(f"Changed files scanned: {len(files)}") + lines.append(f"Findings: {len(by_sev['block'])} block, {len(by_sev['warn'])} warn, {len(by_sev['info'])} info") + lines.append("") + for severity in ["block", "warn", "info"]: + if not by_sev[severity]: + continue + lines.append(f"## {severity.upper()}") + for f in by_sev[severity]: + lines.append(f"- `{f.path}:{f.line_no}` [{f.category}] {f.message}") + lines.append(f" - `{f.snippet[:180]}`") + lines.append("") + return "\n".join(lines), by_sev + + +def main(): + parser = argparse.ArgumentParser() + parser.add_argument("--base", default="origin/main") + parser.add_argument("--head", default="HEAD") + args = parser.parse_args() + + files, findings = scan(args.base, args.head) + summary, by_sev = summarize(files, findings) + print(summary) + + step_summary = os.environ.get("GITHUB_STEP_SUMMARY") + if step_summary: + Path(step_summary).write_text(summary + "\n", encoding="utf-8") + + for sev in ["block", "warn"]: + for f in by_sev[sev]: + level = "error" if sev == "block" else "warning" + print(f"::{level} file={f.path},line={f.line_no},title={f.category}::{f.message}: {f.snippet}") + + if by_sev["block"]: + sys.exit(1) + + +if __name__ == "__main__": + main()