diff --git a/.github/scripts/pr-guard.mjs b/.github/scripts/pr-guard.mjs index 762e315..6ac7777 100644 --- a/.github/scripts/pr-guard.mjs +++ b/.github/scripts/pr-guard.mjs @@ -35,46 +35,49 @@ 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)." }, + "contract-locked": { tier: "block", title: "Deployed contract files changed", help: "These files are pinned in contracts/deployed.lock, or stand guard over it (the lock itself, the scripts that check it, .gitmodules, everything under contracts/lib/), 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." }, + submodule: { tier: "block", title: "Submodule added, removed 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." }, + binary: { tier: "block", title: "A binary that is not a plain image or web font", help: "It cannot be reviewed as text. An image (PNG, JPEG, GIF, WebP, AVIF, ICO) or a web font (WOFF, WOFF2) must be a real file of the type its 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." }, + "case-collision": { tier: "block", title: "A path that differs from another one only by letter case", help: "On macOS and Windows the two overwrite each other at checkout. The other one may be in the repository already or new in this pull request." }, + "odd-path": { tier: "block", title: "A file name with control, invisible or non-ASCII characters", help: "A Mac folds letters such as the long s onto plain ones, so a name like this can land on an existing file at checkout. Rename the file with plain characters." }, + "criss-cross": { tier: "block", title: "A history with more than one common ancestor", help: "With a history like this the diff does not show what merging the pull request changes: the merge can undo work on main that the diff never mentions. Rebase the branch on main, or merge main into it, and push." }, + unread: { tier: "block", title: "A file the guard could not read", help: "The rules that hold for everyone (credentials, hidden characters) could not be applied to it. Split the change up, or an admin merges it after reading it." }, - contracts: { tier: "review", title: "Contracts folder", help: "New contract sources, tests, scripts and deployment records are read by an owner before they merge." }, + contracts: { tier: "review", title: "Contracts folder", help: "Any change under contracts/ that is not blocked above (a new contract, a test, a script, a document; added, edited or deleted) is read by an owner before it merges." }, "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. 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." }, + "dangerous-code": { tier: "review", title: "Code execution, raw HTML or shell", help: "Patterns that run code, inject markup, start processes or decode hidden text, and code that reads the whole environment, the home directory or a key store." }, "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 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." }, + "dot-file": { tier: "review", title: "A hidden file or folder that is new to the repository", help: "Tools read their settings from names that begin with a dot. The ones known to run commands or steer an agent are blocked; any other new one is read by an owner (.gitignore, .gitkeep and .keep aside)." }, 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." }, + unscanned: { tier: "review", title: "Not scanned in full", help: "The guard read this only for what is blocked for everyone, or could not list everything it found, so a person has to read the rest." }, }; // ---------- 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], + ["dev-machine", /(^|\/)\.(claude|cursor|codex|gemini|qwen|windsurf|continue|aider[^/]*|clinerules|roo|kiro|zed|fleet|junie|amazonq|augment|trae|opencode|goose|vscode|idea|devcontainer|husky|githooks)(\/|$)|(^|\/)(CLAUDE|AGENTS|GEMINI|QWEN)(\.[^/]*)?\.md$|(^|\/)(\.cursorrules|\.clinerules|\.windsurfrules|\.goosehints|\.mcp\.json|opencode\.jsonc?|\.devcontainer\.json|[^/]*\.code-workspace|\.envrc|\.huskyrc(\.[^/]*)?|\.lintstagedrc(\.[^/]*)?|\.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. */ @@ -96,10 +99,13 @@ 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); +/** + * Named as a test. App code that imports one of these is flagged, so nothing that ships can hide here. Under a pages/ + * folder Next.js serves every file as a page, whatever it is called, so nothing there is a test. + */ +const isTest = (p) => /\.(test|spec)\.[cm]?[jt]sx?$|\.t\.sol$|^contracts\/test\//.test(p) && !/^app\/(src\/)?pages\//.test(p); +/** Everything that is not plainly prose is read as code, whatever its extension: LICENSE and LICENSE-MIT.txt are prose, LICENSES.ts is not. */ +const isCode = (p) => !/\.(md|markdown|txt|rst|adoc|csv|tsv|diff|patch)$/i.test(p) && !/(^|\/)(LICENSE|NOTICE|AUTHORS)(-[\w.]+)?$/.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(); @@ -143,6 +149,10 @@ const SECRETS = [ ["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())], ]; +/** A finding's text with anything shaped like a credential taken out: a detail may quote a dependency's address or a version. */ +const scrub = (text) => + SECRETS.reduce((out, [what, re]) => out.replace(new RegExp(re.source, `${re.flags.replace("g", "")}g`), `<${what}>`), String(text)).replace(/(:\/\/)[^\s/@"'<>]+@/g, "$1@"); + 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]/; @@ -157,6 +167,13 @@ export function hiddenCharacter(text, atFileStart = false) { 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"; } + // a line break to the compiler and the editor, but not to git or to this scan: what follows a "//" would run + if (text.includes("\r")) return "a carriage return inside a line"; + // a variation selector belongs right after an emoji, or between a digit and its keycap; a run of them carries hidden bytes + for (const m of text.matchAll(/[\uFE00-\uFE0F]/gu)) { + const afterEmoji = /\p{Extended_Pictographic}$/u.test(text.slice(Math.max(0, m.index - 2), m.index)); + if (!afterEmoji && !(m[0] === "\uFE0F" && text[m.index + 1] === "\u20E3")) return "a variation selector 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; @@ -209,7 +226,7 @@ const DANGEROUS = [ ["a computed import or require", /\b(?:import|require)\s*\(\s*(?:(?!["'`])[^)\s]|`[^`]*\$\{)/, JS_FILE], // the environment as one object: printed, spread, taken apart ({ KEY } = process.env) or handed on. A single variable // read by its name is "new-env" below. - ["reading the whole environment", /\b(?:process|import\.meta)\.env\b(?!\s*(?:\?\.|\.)\s*[A-Za-z_$]|\s*\[)/], + ["reading the whole environment", /\b(?:process|import\.meta)\.env\b(?!\s*(?:\?\.|\.)\s*[A-Za-z_$[]|\s*\[)/], ["an environment variable read by a computed name", /process\.env\[\s*(?!["'`])/], ["decoding hidden text", /\batob\s*\(|String\.fromCharCode|Buffer\.from\([^)\n]*["']base64(?:url)?["']/], ["a download piped to a shell", /\b(?:curl|wget)\b[^|\n]*\|\s*(?:sudo\s+)?(?:ba|z|da)?sh\b|base64\s+(?:-d|--decode)\b|\/dev\/tcp\//], @@ -221,10 +238,10 @@ const TEST_IMPORT = /(?:\bfrom|\bimport|\brequire\s*\()\s*\(?\s*["'`][^"'`]*\.(? const WALLET = /\b(?:writeContracts?(?:Async)?|sendTransaction(?:Async)?|sendRawTransaction|sendCalls|signMessage(?:Async)?|signTypedData(?:Async)?|signTransaction|signAuthorization|personal_sign|eth_sign\w*|eth_send\w*|eth_requestAccounts|setApprovalForAll|increaseAllowance|max[Uu]int256|use(?:WriteContract|SendTransaction|SendCalls|SignMessage|SignTypedData|WalletClient)|(?:get|create)WalletClient|privateKeyToAccount|mnemonicToAccount)\b|window\.ethereum|["'`]wallet_[A-Za-z]+["'`]|functionName:\s*["'](?:approve|permit|transfer|transferFrom|setApprovalForAll)["']/g; /** Checks whose removal matters: signatures, nonces, rate limits, validation, escaping. */ -const AUTH = /\b(?:verify[A-Z]\w*|recover\w+Address|consumeNonce|isNonce|issueNonce|rateLimited|cspNonce|isAddress|ADMIN_WALLETS|isAdmin\w*|requireAdmin\w*|sanitize[A-Z]\w*|escape[A-Z]\w*)\b/g; +const AUTH = /\b(?:verify[A-Z]\w*|validate[A-Z]\w*|check[A-Z]\w*|admit[A-Z]\w*|assert[A-Z]\w*|require[A-Z]\w*|authori[sz]e\w*|authenticate\w*|recover\w+Address|consumeNonce|isNonce|issueNonce|rateLimited|cspNonce|isAddress|isOrigin|isOwn[A-Z]\w*|ADMIN_WALLETS|isAdmin\w*|sanitize[A-Z]\w*|escape[A-Z]\w*)\b/g; /** CONTRIBUTING.md ground rules: no fee switch, owner, pause, allowlist or upgrade path. */ -const CONTRACT_RULES = /\b(?:Ownable\w*|onlyOwner|transferOwnership|renounceOwnership|setOwner|Pausable|whenNotPaused|_pause|_unpause|UUPS\w*|\w*Upgradeable|upgradeTo\w*|delegatecall|selfdestruct|tx\.origin|allowlist\w*|whitelist\w*|blacklist\w*|blocklist\w*|setFee\w*|protocolFee\w*|platformFee\w*|feeSwitch\w*|admin)\b/gi; +const CONTRACT_RULES = /\b(?:Ownable\w*|onlyOwner|owner|transferOwnership|renounceOwnership|setOwner|Pausable|whenNotPaused|_?pause[ds]?|_?unpause|UUPS\w*|\w*Upgradeable|upgradeTo\w*|delegatecall|selfdestruct|tx\.origin|allowlist\w*|whitelist\w*|blacklist\w*|blocklist\w*|set\w*Fee\w*|protocolFee\w*|platformFee\w*|feeSwitch\w*|\w*feeBps|admin)\b/gi; const ADDRESS = /0x[0-9a-fA-F]{40}(?![0-9a-fA-F])/g; /** Not anyone's wallet: zero, the burn address, the native-asset placeholder. */ @@ -280,11 +297,15 @@ export function hostsIn(line, code) { /** How often each security check is named in some lines: { name: count }. */ export function checksIn(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); + for (const text of lines) { + if (/^\s*(?:\/\/|\/\*|\*)/.test(text)) continue; // a check that is commented out is not a check + for (const [name] of text.matchAll(AUTH)) counts.set(name, (counts.get(name) ?? 0) + 1); + } return counts; } -const ENV_RE = /process\.env\.([A-Za-z_]\w*)|process\.env\[\s*["'`]([^"'`]+)["'`]\s*\]|import\.meta\.env\.([A-Za-z_]\w*)/g; +/** A variable read by its name: process.env.NAME, process.env["NAME"], or a helper called with it, as in intEnv("NAME", 5). */ +const ENV_RE = /process\.env\??\.([A-Za-z_]\w*)|process\.env(?:\?\.)?\[\s*["'`]([^"'`]+)["'`]\s*\]|import\.meta\.env\??\.([A-Za-z_]\w*)|\b[A-Za-z_$]*[eE]nv[A-Za-z_$]*\(\s*["'`]([A-Z][A-Z0-9_]*)["'`]/g; const OPAQUE = [ ["a long base64 blob", /[A-Za-z0-9+/]{400,}={0,2}/], ["a long hex blob", /(?:0x)?[0-9a-fA-F]{400,}/], @@ -296,7 +317,7 @@ const OPAQUE = [ * Findings for one text file. `added` is [{ n, text }], `removed` is [text]. `facts` holds what the base branch already has: * addresses (lowercase), hosts, env (variable names). */ -export function scanLines(path, added, removed, facts) { +export function scanLines(path, added, removed, facts, blocksOnly = false) { const out = []; const add = (rule, detail, line) => out.push({ rule, path, detail, line }); const code = isCode(path), test = isTest(path), svg = ext(path) === "svg"; @@ -309,6 +330,7 @@ export function scanLines(path, added, removed, facts) { const m = re.exec(text); if (m && (!check || check(m))) add("secret", what, n); } + if (blocksOnly) continue; 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 host of hostsIn(text, code)) { @@ -328,14 +350,14 @@ export function scanLines(path, added, removed, facts) { 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]; + const name = m[1] ?? m[2] ?? m[3] ?? m[4]; 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) { + if (code && !test && !blocksOnly && removed.length > 0) { const kept = checksIn(added.map((l) => l.text)); for (const [name, gone] of checksIn(removed)) { const net = gone - (kept.get(name) ?? 0); @@ -444,11 +466,15 @@ export function lintNpmLock(path, headText, baseText) { 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; + // "x/../../other/-/other-1.0.0" as a version would turn this package's address into another package's + const versioned = typeof entry.version === "string" && /^\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?$/.test(entry.version); + const label = versioned ? `${name}@${entry.version}` : name; if (!key.startsWith("node_modules/") && !key.includes("/node_modules/")) add("lockfile", `${key}: a package outside node_modules`); + else if (!/^(?:@[A-Za-z0-9~-][A-Za-z0-9._~-]*\/)?[A-Za-z0-9~-][A-Za-z0-9._~-]*$/.test(name)) add("lockfile", `${key}: not a package name`); 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 if (!versioned) add("lockfile", `${name}: its version is not a version number`); 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`); @@ -526,7 +552,9 @@ export function openRepo(cwd, base, head) { 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]); + // With more than one common ancestor the diff from any one of them is not what a merge does (analyze refuses that). + const mergeBases = text(["merge-base", "--all", baseSha, headSha]).split("\n").filter(Boolean); + const mergeBase = mergeBases[0]; const changes = parseRaw(git(["diff-tree", "-r", "-z", "--no-renames", "--no-abbrev", mergeBase, headSha]).toString("utf8")); /** @@ -543,7 +571,7 @@ export function openRepo(cwd, base, head) { } }; return { - headSha, baseSha, mergeBase, changes, baseMatches, + headSha, baseSha, mergeBase, mergeBases, changes, baseMatches, /** The file's bytes, or null when it is larger than the guard reads. */ blob: (sha) => { try { @@ -574,11 +602,12 @@ const lazy = (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 add = (rule, path, detail, line = null) => findings.push({ rule, tier: RULES[rule].tier, path, detail: scrub(detail), line }); + if ((repo.mergeBases?.length ?? 1) > 1) add("criss-cross", "(the branch's history)", `${repo.mergeBases.length} common ancestors with the base branch`); 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) => 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)), + env: new Set(repo.baseMatches("(process\\.env|import\\.meta\\.env)(\\??\\.[a-z_][a-z0-9_]*|(\\?\\.)?\\[[^]]+\\])|[a-z_$]*env[a-z_$]*\\([[:space:]]*[\"'`][a-z_][a-z0-9_]*[\"'`]").flatMap((hit) => [...hit.matchAll(ENV_RE)].map((m) => m[1] ?? m[2] ?? m[3] ?? m[4]))), })); /** lowercase path or folder -> its spelling in the base tree */ const spellings = lazy(() => { @@ -595,6 +624,9 @@ export function analyze(repo, { lockedPaths }) { const locked = (path) => lockedPaths.has(path) || path.startsWith(LOCKED_FOLDER); + /** Paths this pull request deletes, and the spellings of what it adds: lowercase path or folder -> as written. */ + const deleted = new Set(repo.changes.filter((change) => change.status === "D").map((change) => change.path)); + const addedSpellings = new Map(); 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(); @@ -603,23 +635,34 @@ export function analyze(repo, { lockedPaths }) { const gone = status === "D"; if (hiddenCharacter(path) || /[\\\n\r\t]/.test(path) || path.split("/").includes("..")) add("odd-path", path, "the file name"); + else if (/[^\x20-\x7E]/.test(path)) add("odd-path", path, "the file name has characters outside plain ASCII"); 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"); + // npm builds a package that has this file with node-gyp at install time, with no script in package.json at all + if (!gone && /(^|\/)binding\.gyp$/i.test(path)) add("install-script", path, "npm runs node-gyp on this file at install time"); + let hiddenName = null; if (status === "A") { const parts = path.split("/"); + let collided = false; 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; } + const prefix = parts.slice(0, i).join("/"), lower = prefix.toLowerCase(); + // against the base branch and against what this pull request itself adds; a file renamed only in its case is one file + const existing = spellings().get(lower) ?? addedSpellings.get(lower); + if (existing && existing !== prefix && !(i === parts.length && deleted.has(existing))) { add("case-collision", path, `collides with ${existing}`); collided = true; break; } + if (!addedSpellings.has(lower)) addedSpellings.set(lower, prefix); } + // .gitignore and the empty files that keep a folder in git configure no tool + if (!collided) hiddenName = parts.find((part, i) => part.startsWith(".") && !/^\.(gitignore|gitkeep|keep)$/.test(part) && !spellings().has(parts.slice(0, i + 1).join("/").toLowerCase())) ?? null; } 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"); + // a new name that begins with a dot, where no rule above already refuses the file or sends it to an owner + if (!match && hiddenName && !SECRET_FILE.test(path)) add("dot-file", path, `${hiddenName} is new`); } 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 @@ -632,7 +675,7 @@ export function analyze(repo, { lockedPaths }) { // 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}`)); + const unread = (why) => (checked ? add("unread", 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); @@ -644,15 +687,24 @@ export function analyze(repo, { lockedPaths }) { 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" ? { added: allLines(bytes.toString("utf8")), removed: [] } : repo.diff(change.oldSha, change.newSha); + // A lockfile (thousands of registry addresses and hashes; its structure was checked above) and a file too large to + // match line by line are still read for what is blocked for everyone: credentials and hidden characters. + const lockfile = NPM_LOCK.test(path), large = bytes.length > MAX_TEXT_BYTES; + if (lockfile || large) { + const plainLines = lockfile ? lines.added.map((l) => ({ n: l.n, text: l.text.replace(/"(?:integrity|resolved)":\s*"[^"]*"/g, "") })) : lines.added; + const blocking = scanLines(path, plainLines, [], facts(), true); + for (const f of blocking.slice(0, MAX_FINDINGS_PER_FILE)) add(f.rule, f.path, f.detail, f.line); + if (blocking.length > MAX_FINDINGS_PER_FILE) add("unscanned", path, `${blocking.length - MAX_FINDINGS_PER_FILE} more findings in this file are not listed`); + if (!lockfile) add("unscanned", path, `a text file of ${Math.ceil(bytes.length / 1_000_000)} MB, read for credentials and hidden characters only`); + continue; + } const found = scanLines(path, lines.added, lines.removed, facts()); if (isCode(path) && !isTest(path)) { const lost = checksIn(lines.removed); diff --git a/.github/scripts/pr-guard.test.mjs b/.github/scripts/pr-guard.test.mjs index 6634ecc..7abcb80 100644 --- a/.github/scripts/pr-guard.test.mjs +++ b/.github/scripts/pr-guard.test.mjs @@ -87,7 +87,7 @@ const BASE = { "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/lib/launchpad/config.ts": `export const NATIVE = "0x0000000000000000000000000000000000000000";\nexport const ROUTER = "${ROUTER}";\nexport const RPC = process.env.BASE_RPC_URL ?? "https://mainnet.base.org";\nexport const OVERLAP = intEnv("SYNC_OVERLAP_BLOCKS", 50);\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', @@ -202,6 +202,37 @@ describe("deployed contracts", () => { assert.doesNotMatch(details(found, "contract-rules"), /admin/, "comment lines are not code"); }); + test("an owner, a pause or a fee that is written without a library is named too", () => { + const found = check({ "contracts/src/Vault.sol": "contract Vault {\n address public owner;\n bool public paused;\n uint16 public feeBps;\n function setProtocolFee(uint16 fee) external { require(msg.sender == owner); feeBps = fee; }\n}\n" }); + for (const word of ["owner", "paused", "feeBps", "setProtocolFee"]) assert.ok(of(found, "contract-rules").some((f) => f.detail === word), word); + }); + + test("a history with two common ancestors is blocked: its diff does not show what merging it would undo", () => { + // main: one commit, then a fix that came in from a side branch by a merge + git(["checkout", "-q", "-f", "--detach", baseSha]); + git(["reset", "-q", "--hard", baseSha]); + git(["clean", "-fdxq"]); + const before = commit({ "docs/notes.md": "notes\n" }, "main moves on"); + git(["checkout", "-q", "-f", "--detach", baseSha]); + const fix = commit({ "app/src/app/api/post/route.ts": ROUTE.replace(" return save(", " if (!isOrigin(request)) return denied();\n return save(") }, "a fix on a side branch"); + git(["checkout", "-q", "-f", "--detach", before]); + git(["merge", "-q", "--no-ff", "-m", "merge the fix", fix]); + const main = git(["rev-parse", "HEAD"]); + // the pull request: main as it was before the fix, with the fix named as a parent ("already merged, and dropped") + const head = git(["commit-tree", `${before}^{tree}`, "-p", before, "-p", fix, "-m", "ours"]); + const repo = openRepo(dir, main, head); + assert.equal(repo.mergeBases.length, 2); + const found = analyze(repo, { lockedPaths: LOCKED }); + assert.ok(found.some((f) => f.rule === "criss-cross" && f.tier === "block" && f.detail === "2 common ancestors with the base branch")); + assert.equal(verdict(found, { author: OWNERS[0], pusher: OWNERS[0], approvedBy: OWNERS[0] }).outcome, "block"); + // and git, asked to merge it, takes the fix back out of main without a conflict + const merged = git(["merge-tree", "--write-tree", main, head]); + assert.ok(!git(["show", `${merged}:app/src/app/api/post/route.ts`]).includes("isOrigin"), "the merge undoes the fix"); + // merging main into the branch leaves one common ancestor, and then the diff shows everything + const updated = git(["commit-tree", `${main}^{tree}`, "-p", head, "-p", main, "-m", "merge main"]); + assert.deepEqual(analyze(openRepo(dir, main, updated), { lockedPaths: LOCKED }), []); + }); + 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"]); @@ -254,7 +285,7 @@ describe("CI and machine configuration", () => { }); 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"]) { + 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", "CLAUDE.local.md", "AGENTS.override.md", ".devcontainer.json", ".clinerules/rules.md", "app.code-workspace", ".zed/tasks.json", ".roo/rules.md", ".kiro/steering.md"]) { 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); @@ -275,6 +306,34 @@ describe("CI and machine configuration", () => { assert.ok(rules(check({ ".yarnrc.yml": "npmRegistryServer: x\n" })).includes("registry-config")); }); + test("any other new file or folder whose name begins with a dot needs an owner", () => { + assert.deepEqual(check({ ".coderabbit.yaml": "reviews:\n auto_review:\n enabled: false\n" }).map((f) => [f.rule, f.tier, f.detail]), [["dot-file", "review", ".coderabbit.yaml is new"]]); + assert.deepEqual(rules(check({ "tools/.cache/list.json": "[]\n" })), ["dot-file"]); + assert.deepEqual(rules(check({ ".gitlab-ci.yml": "stages: []\n" })), ["dot-file"], "only .gitignore and .gitkeep are git's housekeeping"); + // one the repository already has, git's own housekeeping, and ones another rule already refuses or sends to an owner + assert.deepEqual(rules(check({ ".gitignore": "node_modules/\n.env\n.next/\n" })), []); + assert.deepEqual(rules(check({ "tools/.gitignore": "out/\n", "tools/fixtures/.gitkeep": "", "tools/tmp/.keep": "" })), []); + assert.deepEqual(rules(check({ "app/.env.local": "x\n" })), ["secret-file"]); + assert.deepEqual(rules(check({ ".gitattributes": "*.sol linguist-vendored\n" })), ["git-config"]); + assert.deepEqual(rules(check({ ".vscode/tasks.json": "{}\n" })), ["dev-machine"]); + }); + + test("a file name with a character outside ASCII is blocked: a Mac folds some of them onto plain letters", () => { + // the long s: on a Mac this path is contracts/src/LaunchFactory.sol, and the next one is .vscode/tasks.json + const names = [`contract${cp(0x17f)}/src/LaunchFactory.sol`, `.v${cp(0x17f)}code/tasks.json`, `AGENT${cp(0x17f)}.md`, `app/package.j${cp(0x17f)}on`, `docs/caf${cp(0xe9)}.md`]; + for (const name of names) { + const found = check({ [name]: { index: "x\n" } }); + assert.ok(found.some((f) => f.rule === "odd-path" && f.tier === "block" && /outside plain ASCII/.test(f.detail)), name); + } + }); + + test("two new names that differ only by letter case collide; one file renamed only in its case does not", () => { + const pair = check({ "app/src/lib/Helper.ts": { index: "export {};\n" }, "app/src/lib/helper.ts": { index: "export const x = 1;\n" } }); + assert.deepEqual(pair.map((f) => [f.rule, f.path, f.detail]), [["case-collision", "app/src/lib/helper.ts", "collides with app/src/lib/Helper.ts"]]); + assert.ok(rules(check({ "docs/New/a.md": { index: "a\n" }, "docs/new/b.md": { index: "b\n" } })).includes("case-collision")); + assert.deepEqual(rules(check({ "README.md": null, "readme.md": { index: BASE["README.md"] } })), []); + }); + test("a symlink is blocked", () => { assert.deepEqual(rules(check({ "docs/readme": { symlink: "../README.md" } })), ["symlink"]); }); @@ -361,6 +420,22 @@ describe("hidden characters", () => { assert.equal(hiddenCharacter(`export${cp(0xfeff)} {};`, true), "a byte order mark inside the file"); }); + test("a variation selector is fine after an emoji and on a keycap, and blocked anywhere else", () => { + assert.equal(hiddenCharacter(`liked ${cp(0x2764, 0xfe0f)}, step ${cp(0x31, 0xfe0f, 0x20e3)}, flag ${cp(0x1f3f3, 0xfe0f, 0x200d, 0x1f308)}`), null); + assert.equal(hiddenCharacter(`a${cp(0xfe0f)}b`), "a variation selector outside an emoji"); + assert.equal(hiddenCharacter(`${cp(0x2764, 0xfe0f, 0xfe0f)}`), "a variation selector outside an emoji", "never twice"); + // a run of them after a letter is how bytes are hidden in a source file + assert.deepEqual(rules(check({ "app/src/lib/x.ts": `const v = "a${cp(0xfe00, 0xfe01, 0xfe02, 0xfe03)}";\n` })), ["hidden-unicode"]); + }); + + test("a carriage return inside a line is blocked; a Windows line ending is not one", () => { + // to the compiler the comment ends at the return, and what follows is code + const found = check({ "contracts/src/Vault.sol": "contract Vault {\n // nothing to see\raddress attacker;\n}\n" }); + assert.deepEqual(rules(found), ["contracts", "hidden-unicode"]); + assert.equal(details(found, "hidden-unicode"), "a carriage return inside a line"); + assert.deepEqual(rules(check({ "docs/guide.md": "## Guide\r\n\r\nRun `npm test`.\r\n" })), []); + }); + 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` })), []); }); @@ -576,11 +651,55 @@ describe("dependencies", () => { 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"]]); + 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"], ["unread", "block", "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"]]); + assert.deepEqual(analyze(huge, { lockedPaths: LOCKED }).map((f) => [f.rule, f.tier]), [["dependency-source", "block"], ["unread", "block"]]); + }); + + test("a lockfile and a file too large to scan are still read for credentials and hidden characters", () => { + const key = "c0ffee".repeat(10) + "abcd"; + const lock = JSON.parse(lockfile(BASE_LOCK)); + lock.notes = { deployer: `private key 0x${key}`, label: `user${cp(0x202e)}nimda` }; + assert.deepEqual(rules(check({ "app/package-lock.json": `${JSON.stringify(lock, null, 2)}\n` })), ["dependencies", "hidden-unicode", "secret"]); + const large = check({ "app/src/lib/large.ts": `${"export const a = 1;\n".repeat(110_000)}const PRIVATE_KEY = "0x${key}";\nconst role = "user${cp(0x202e)}";\neval(input);\n` }); + assert.deepEqual(large.map((f) => [f.rule, f.tier, f.line]), [["secret", "block", 110_001], ["hidden-unicode", "block", 110_002], ["unscanned", "review", null]]); + assert.match(details(large, "unscanned"), /a text file of 3 MB, read for credentials and hidden characters only/); + // past the number of findings listed for one file, the report says how many it left out, as it does for any file + const flood = check({ "app/src/lib/flood.ts": `${"export const a = 1;\n".repeat(110_000)}${`const role = "user${cp(0x202e)}";\n`.repeat(250)}` }); + assert.equal(of(flood, "hidden-unicode").length, 200); + assert.equal(details(flood, "unscanned"), "50 more findings in this file are not listed | a text file of 3 MB, read for credentials and hidden characters only"); + // a registry hash is not a credential, even where a stretch of it reads like an access key + const lookalike = lockfile({ ...BASE_LOCK, leftpad: entry("leftpad", "1.0.0", { integrity: `sha512-${"x".repeat(65)}+AKIA${"ABCDEFGHIJKLMNOP"}==` }) }); + assert.deepEqual(rules(check({ "app/package-lock.json": lookalike })), ["dependencies"]); + }); + + test("a lockfile entry needs a real package name and a real version, so that its address is its own", () => { + const hash = `sha512-${"A".repeat(86)}==`; + // "left-pad" at a version that walks the address over to another package's tarball + const walked = { version: "x/../../../../is-number/-/is-number-7.0.0", resolved: "https://registry.npmjs.org/left-pad/-/left-pad-x/../../../../is-number/-/is-number-7.0.0.tgz", integrity: hash }; + assert.match(details(lintNpmLock("p", lockfile({ ...BASE_LOCK, "left-pad": walked }), lockfile(BASE_LOCK)), "lockfile"), /^left-pad: its version is not a version number$/); + assert.match(details(lintNpmLock("p", lockfile({ ...BASE_LOCK, "left pad": entry("left pad", "1.0.0") }), lockfile(BASE_LOCK)), "lockfile"), /not a package name/); + assert.match(details(lintNpmLock("p", lockfile({ ...BASE_LOCK, "a/../viem": entry("a/../viem", "1.0.0") }), lockfile(BASE_LOCK)), "lockfile"), /not a package name/); + // names and versions the registry really has + const real = { ...BASE_LOCK, JSONStream: entry("JSONStream", "1.3.5"), "@types/node": entry("@types/node", "20.19.0"), next: entry("next", "16.3.8-canary.1"), semver: entry("semver", "7.7.2+build.5") }; + assert.deepEqual(rules(lintNpmLock("p", lockfile(real), lockfile(BASE_LOCK))), ["dependencies"]); + }); + + test("a binding.gyp is blocked: npm builds it at install time with no script in package.json", () => { + assert.deepEqual(check({ "app/binding.gyp": '{ "targets": [{ "target_name": "addon" }] }\n' }).map((f) => [f.rule, f.tier]), [["install-script", "block"]]); + }); + + test("a credential is never repeated in a finding, wherever it was written", () => { + const token = "ghp_" + "a1B2".repeat(9); + const password = "hunter2" + "-deploy-pass"; + const found = check({ "app/package.json": manifest({ dependencies: { sharp: token, viem: "^2.47.6", priv: `git+https://x-access-token:${token}@github.com/o/r.git`, kit: `git+https://deploy:${password}@git.example.org/o/kit.git` } }) }); + assert.deepEqual(rules(found), ["dependencies", "dependency-source", "secret"]); + assert.equal(details(found, "dependency-source"), 'priv comes from "git+https://x-access-token:@github.com/o/r.git" | kit comes from "git+https://@git.example.org/o/kit.git"'); + assert.match(details(found, "dependencies"), /sharp \^0\.35\.4 → /); + const report = JSON.stringify(found) + renderText(found, verdict(found, {})) + renderSummary(found, verdict(found, {}), { headSha: "a".repeat(40) }); + assert.ok(!report.includes(token) && !report.includes(password)); }); test("an override may point at one of this package's own dependencies, and at nothing else", () => { @@ -752,6 +871,21 @@ describe("application code", () => { assert.deepEqual(rules(check({ "docs/guide.md": "Never call `eval(input)` on user text.\n" })), []); }); + test("a file Next.js serves as a page is app code whatever it is called, and so is code with a prose-like name", () => { + const page = check({ "app/src/pages/airdrop.test.tsx": 'export default function Page() {\n return ;\n}\n' }); + assert.deepEqual(rules(page), ["wallet-call"]); + assert.deepEqual(rules(check({ "app/src/lib/LICENSES.ts": "export const run = (input) => eval(input);\n" })), ["dangerous-code"]); + assert.deepEqual(rules(check({ "NOTICE": "Portions of this software call eval(input) in tests.\n", "docs/LICENSE-MIT.txt": "MIT, see eval(input).\n" })), []); + }); + + test("the app's own checks are known by name, and one that is commented out counts as removed", () => { + const facts = { addresses: new Set(), hosts: new Set(), env: new Set() }; + const dropped = scanLines("app/src/app/api/upload/route.ts", [{ n: 4, text: " return save(body);" }], [" const meta = validateMeta(body);", " if (!(await admitAdmin(request))) return denied();", " await checkUpload(file);", " assertBridgeWalletQueueClear(queue);"], facts); + assert.equal(details(dropped, "auth-removed"), "validateMeta: 1 fewer use | admitAdmin: 1 fewer use | checkUpload: 1 fewer use | assertBridgeWalletQueueClear: 1 fewer use"); + const commented = check({ "app/src/app/api/post/route.ts": ROUTE.replace(" if (await rateLimited(request))", " // if (await rateLimited(request))") }); + assert.deepEqual(commented.map((f) => [f.rule, f.detail]), [["auth-removed", "rateLimited: 1 fewer use"]]); + }); + 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"]); @@ -805,6 +939,11 @@ describe("application code", () => { assert.deepEqual(rules(check({ "app/src/app/api/post/route.ts": moved })), []); }); + test("a variable read through ?. or through the app's own helper is a new one as well", () => { + const found = check({ "app/src/lib/knobs.ts": 'export const a = process.env?.ADMIN_SIGNER_KEY, b = intEnv("SYNC_NEW_KNOB", 5), c = intEnv("SYNC_OVERLAP_BLOCKS", 60), d = process.env?.BASE_RPC_URL;\n' }); + assert.deepEqual(found.map((f) => [f.rule, f.detail]), [["new-env", "ADMIN_SIGNER_KEY"], ["new-env", "SYNC_NEW_KNOB"]]); + }); + 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"]); diff --git a/.github/scripts/workflows.test.mjs b/.github/scripts/workflows.test.mjs index 8e2f896..eb0cd6a 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, RULES, activeSvg } from "./pr-guard.mjs"; +import { OWNERS, RULES, activeSvg, lintNpmLock, parseLock } from "./pr-guard.mjs"; const root = fileURLToPath(new URL("../../", import.meta.url)); const dir = `${root}.github/workflows/`; @@ -200,8 +200,11 @@ test("code owners and the guard name the same people, and cover the guarded path test("the lock names files that exist, and every deployed source is in it", () => { const lock = readFileSync(`${root}contracts/deployed.lock`, "utf8"); + // every line is a comment or an entry in the one shape the script writes, the script checks and the guard reads + for (const line of lock.split("\n")) assert.match(line, /^(#.*|)$|^(sha256 [0-9a-f]{64}|gitlink [0-9a-f]{40}) {2}\S+$/, line); const entries = lock.split("\n").filter((line) => /^(sha256|gitlink) /.test(line)).map((line) => line.split(/\s+/)); assert.ok(entries.length >= 13); + for (const [, , path] of entries) assert.ok(parseLock(lock).has(path), `the guard reads ${path}`); for (const [kind, value, path] of entries) { // the guard compares these with a pull request's paths letter for letter: from the root, with no "./" or ".." assert.doesNotMatch(path, /^\/|(^|\/)\.\.?(\/|$)/, path); @@ -246,6 +249,14 @@ test("the lock check fails when a deployed file, a library pin or a file inside assert.equal(readFileSync(join(repo, "contracts/deployed.lock"), "utf8"), "# deployed sources\n", "the lock is left as it was"); rmSync(join(repo, "contracts/src/Draft.sol")); assert.equal(run("--update", "contracts").status, 1, "a folder is not one file"); + // a path with a space cannot be written as an entry that the check and the guard would read back + writeFileSync(join(repo, "contracts/src/My Token.sol"), "contract MyToken {}\n"); + git(repo, "add", "contracts/src/My Token.sol"); + const spaced = run("--update", "contracts/src/My Token.sol"); + assert.equal(spaced.status, 1); + assert.match(spaced.stderr, /^cannot lock contracts\/src\/My Token\.sol: a path with a space in it cannot be an entry of the lock/m); + assert.equal(readFileSync(join(repo, "contracts/deployed.lock"), "utf8"), "# deployed sources\n", "the lock is left as it was"); + git(repo, "rm", "-q", "-f", "contracts/src/My Token.sol"); // however the path is typed, the lock holds it as git spells it from the root assert.equal(run("--update", "./contracts/src/../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")); @@ -293,8 +304,18 @@ test("the lock check fails when a deployed file, a library pin or a file inside // the lock's last line counts with or without its newline const lock = join(repo, "contracts/deployed.lock"); - writeFileSync(lock, readFileSync(lock, "utf8").trimEnd()); + const entries = readFileSync(lock, "utf8"); + writeFileSync(lock, entries.trimEnd()); assert.match(run().stdout, /^ok: 2 deployed contract files and library pins match/); + // a line the guard would not read as an entry is not quietly checked here either + for (const odd of [entries.replace("sha256 ", " sha256 "), entries.replace(/ {2}contracts\/src/, "\tcontracts/src"), `${entries}sha256 ${"0".repeat(64)} contracts/src/My Token.sol\n`]) { + writeFileSync(lock, odd); + const refused = run(); + assert.equal(refused.status, 1); + assert.match(refused.stdout, /^DRIFT {2}contracts\/deployed\.lock has a line that is not an entry: /m); + } + writeFileSync(lock, entries); + assert.equal(run().status, 0); // a deployed source edited const token = join(repo, "contracts/src/Token.sol"); @@ -337,3 +358,10 @@ test("CONTRIBUTING names every rule of the guard, under the outcome the guard gi // 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); }); + +test("the repository's own lockfile passes the guard's check of every entry", () => { + const lock = readFileSync(`${root}app/package-lock.json`, "utf8"); + assert.ok(Object.keys(JSON.parse(lock).packages).length > 300); + // compared with itself nothing is added or changed, so anything but this one line is an entry the guard refuses + assert.deepEqual(lintNpmLock("app/package-lock.json", lock, lock), [{ rule: "dependencies", path: "app/package-lock.json", detail: "lockfile metadata changed", line: null }]); +}); diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 981ea9a..774144c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -47,17 +47,23 @@ your code. It has two answers besides "passed": - `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. + - `hidden-unicode`, `odd-path`: invisible or direction-changing Unicode in a file; a file name with such characters, + or with any character outside plain ASCII. + - `binary`: a binary file that is not a real image or web font (PNG, JPEG, GIF, WebP, AVIF, ICO, WOFF, WOFF2). 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. + script that runs at install time, a `binding.gyp` (npm builds it at install time). + - `case-collision`: a file name that differs from another one only by letter case. + - `criss-cross`: a branch whose history has more than one common ancestor with `main`. Its diff does not show what a + merge would undo. Rebase it on `main`, or merge `main` into it. + - `unread`: a file the guard could not read: larger than 25 MB, or past the 1500 files one pull request may have read. - **Needs a maintainer's approval.** Allowed, and listed for a maintainer to read. The check turns green when a maintainer approves the exact commit and then re-runs the `pr-guard` job. 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). + - `contracts`, `contract-rules`: any change under `contracts/` that is not blocked above (a new contract, a test, a + script, a document); 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, and a host that @@ -67,9 +73,11 @@ your code. It has two answers besides "passed": - `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`. + - `policy`, `git-config`, `dot-file`: the license and the security policy; `.gitattributes`; any other new file or + folder whose name begins with a dot (a `.gitignore`, `.gitkeep` or `.keep` aside). - `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. + encoded blobs); a file the guard read only in part (a text file over 2 MB is read for credentials and hidden + characters only). 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. diff --git a/contracts/script/check-deployed-sources.sh b/contracts/script/check-deployed-sources.sh index c2c166f..2ed0325 100755 --- a/contracts/script/check-deployed-sources.sh +++ b/contracts/script/check-deployed-sources.sh @@ -27,6 +27,8 @@ if [[ "${1:-}" == "--update" ]]; then # compares the lock's paths with the paths of a pull request letter for letter path=$(git --literal-pathspecs ls-files --full-name -- "$extra" 2>/dev/null) || path= [[ -n "$path" && "$path" != *$'\n'* ]] || { echo "cannot lock $extra: not one file or submodule that git tracks in this repository (git add it first)" >&2; exit 1; } + # an entry is "kind hash path" with no space in the path: the check below and the guard read nothing else + [[ "$path" != *[[:space:]]* ]] || { echo "cannot lock $extra: a path with a space in it cannot be an entry of the lock (rename the file)" >&2; exit 1; } if [[ -n "$(gitlink "$path")" ]]; then echo "gitlink $path"; else echo "sha256 $path"; fi done } | awk '!seen[$2]++' | while read -r kind path; do @@ -38,8 +40,13 @@ if [[ "${1:-}" == "--update" ]]; then fi failed=0; checked=0 -while read -r kind want path || [[ -n "$kind" ]]; do # the last line counts with or without its newline - [[ "$kind" == sha256 || "$kind" == gitlink ]] || continue +# An entry as --update writes it, which is also what the guard reads. A line in any other shape (indented, a tab, a +# path with a space) would be checked here and not locked there, so it is refused. +entry='^(sha256 [0-9a-f]{64}|gitlink [0-9a-f]{40}) [^[:space:]]+$' +while IFS= read -r line || [[ -n "$line" ]]; do # the last line counts with or without its newline + if [[ -z "$line" || "$line" == \#* ]]; then continue; fi + if [[ ! "$line" =~ $entry ]]; then echo "DRIFT $lock has a line that is not an entry: $line"; failed=1; continue; fi + read -r kind want path <<< "$line" checked=$((checked + 1)) got=$(current "$kind" "$path") if [[ "$got" != "$want" ]]; then