From 906d754a284e8587d1e183de5c5c8dd76e0d8a72 Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Mon, 12 Oct 2026 18:12:43 +0800 Subject: [PATCH 1/4] ci: pr-guard: close the gaps an audit of the merged guard found A full read of the guard for changes that get no finding, or a lighter one than the rule promises. Each item was reproduced before it was fixed, and has a test. Refused for everyone: - criss-cross (new): a branch with more than one common ancestor with main. Its diff showed nothing while merging it took a merged fix back out (a "git merge -s ours" of an old side branch). No merge on main has that shape. - odd-path: any character outside ASCII in a file name. A Mac folds the long s onto s, so "contract/src/..." and ".vcode/tasks.json" landed on the real files at checkout and matched no rule. - install-script: binding.gyp. npm builds it at install time with no script in package.json. - lockfile: an entry's name and version have to be a name and a version. A version that walks the path gave one package another package's tarball. - unread (new): a file the guard cannot read (over 25 MB, past 1500 files, an error) was "unscanned", which an owner's own pull request passes. A lockfile and a text file over 2 MB are still read for credentials and hidden characters. - hidden-unicode: a variation selector away from an emoji, and a carriage return inside a line (a compiler ends a "//" comment there). - case-collision: two new names that differ only by case. A file renamed only in its case is no longer one. - dev-machine: CLAUDE.local.md, AGENTS.override.md, .devcontainer.json, *.code-workspace, .clinerules/, .zed/, .roo/, .kiro/ and more of their kind. For an owner to read: - dot-file (new): any other new file or folder whose name begins with a dot (.coderabbit.yaml, .eslintrc.js). A .gitignore or .gitkeep is left alone. - A file under pages/ is a page whatever it is called, so "x.test.tsx" there is not exempt as a test. LICENSES.ts is code, LICENSE-MIT is prose. - auth-removed knows the app's own names (validate..., check..., admit..., assert...), and a check that is commented out counts as removed. - new-env: process.env?.NAME, and a helper called with the name (intEnv("NAME", 5), vm.envOr("NAME", ...)). - contract-rules: owner, paused, feeBps, setProtocolFee, written without a library. Also: a finding never repeats a credential (a git dependency's token was quoted in the job log); the lock script refuses a line that is not an entry in the one shape the guard reads; CONTRIBUTING says what "contracts" and "binary" cover and lists the three new rules. 161 tests (146 before). Every merged pull request on main and all eight open ones keep their outcome. --- .github/scripts/pr-guard.mjs | 102 +++++++++++---- .github/scripts/pr-guard.test.mjs | 143 ++++++++++++++++++++- .github/scripts/workflows.test.mjs | 24 +++- CONTRIBUTING.md | 24 ++-- contracts/script/check-deployed-sources.sh | 9 +- 5 files changed, 260 insertions(+), 42 deletions(-) diff --git a/.github/scripts/pr-guard.mjs b/.github/scripts/pr-guard.mjs index 762e315..0348e6e 100644 --- a/.github/scripts/pr-guard.mjs +++ b/.github/scripts/pr-guard.mjs @@ -50,7 +50,9 @@ export const RULES = { "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." }, + "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." }, "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." }, @@ -65,16 +67,17 @@ export const RULES = { "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." }, 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,22 @@ 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; + for (const f of scanLines(path, plainLines, [], facts(), true).slice(0, MAX_FINDINGS_PER_FILE)) add(f.rule, f.path, f.detail, f.line); + 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..61259e2 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": "" })), []); + 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,51 @@ 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/); + // 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 +867,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 +935,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..9aa02fe 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); @@ -293,8 +296,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 +350,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..ebbc4aa 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` or `.gitkeep` 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..0c71976 100755 --- a/contracts/script/check-deployed-sources.sh +++ b/contracts/script/check-deployed-sources.sh @@ -38,8 +38,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 From ba892f740d7c53631ab8026f817dbde8d181e50c Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Mon, 12 Oct 2026 19:54:21 +0800 Subject: [PATCH 2/4] ci: pr-guard: a large file's findings past the limit are counted; CONTRIBUTING names .keep From the CodeRabbit review of #90. - A lockfile or a file over 2 MB is read for credentials and hidden characters, and at most 200 findings are listed per file. Past that the report now says how many it left out, as it already did for every other file. - The new dot-file rule leaves .gitignore, .gitkeep and .keep alone. CONTRIBUTING named only the first two. --- .github/scripts/pr-guard.mjs | 4 +++- .github/scripts/pr-guard.test.mjs | 6 +++++- CONTRIBUTING.md | 2 +- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/.github/scripts/pr-guard.mjs b/.github/scripts/pr-guard.mjs index 0348e6e..9905cdc 100644 --- a/.github/scripts/pr-guard.mjs +++ b/.github/scripts/pr-guard.mjs @@ -699,7 +699,9 @@ export function analyze(repo, { lockedPaths }) { 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; - for (const f of scanLines(path, plainLines, [], facts(), true).slice(0, MAX_FINDINGS_PER_FILE)) add(f.rule, f.path, f.detail, f.line); + 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; } diff --git a/.github/scripts/pr-guard.test.mjs b/.github/scripts/pr-guard.test.mjs index 61259e2..7abcb80 100644 --- a/.github/scripts/pr-guard.test.mjs +++ b/.github/scripts/pr-guard.test.mjs @@ -312,7 +312,7 @@ describe("CI and machine configuration", () => { 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": "" })), []); + 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"]); @@ -666,6 +666,10 @@ describe("dependencies", () => { 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"]); diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ebbc4aa..774144c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -74,7 +74,7 @@ your code. It has two answers besides "passed": `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`, `dot-file`: the license and the security policy; `.gitattributes`; any other new file or - folder whose name begins with a dot (a `.gitignore` or `.gitkeep` aside). + 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); a file the guard read only in part (a text file over 2 MB is read for credentials and hidden characters only). From dccfff4b8eeb76d70debf44e65f41d4eedd9bf29 Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Mon, 12 Oct 2026 20:04:08 +0800 Subject: [PATCH 3/4] ci: pr-guard: each rule's own text says what the rule covers From the CodeRabbit review of #90: the "contracts" finding explained itself as "new contract sources", while it is given for every change under contracts/ that is not blocked, edits and deletions too. Read the other rules' texts against the code as well: contract-locked names the lock's own guards, submodule says "removed", binary names the image and font types it accepts, case-collision covers two new paths, dangerous-code mentions the environment and key stores, dot-file names its exceptions. --- .github/scripts/pr-guard.mjs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/.github/scripts/pr-guard.mjs b/.github/scripts/pr-guard.mjs index 9905cdc..6ac7777 100644 --- a/.github/scripts/pr-guard.mjs +++ b/.github/scripts/pr-guard.mjs @@ -35,39 +35,39 @@ 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." }, + "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." }, + "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 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." }, From 31d645ac25a23ffc30c3b889bb2eaf2818bce2e0 Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Mon, 12 Oct 2026 20:14:49 +0800 Subject: [PATCH 4/4] contracts: the lock's --update refuses a path with a space in it From the CodeRabbit review of #90. --update wrote such a path into contracts/deployed.lock, and the check then refused the line it had just written: an entry is "kind hash path" with no space in the path, which is also the only shape the guard reads. The path is refused before anything is written. --- .github/scripts/workflows.test.mjs | 8 ++++++++ contracts/script/check-deployed-sources.sh | 2 ++ 2 files changed, 10 insertions(+) diff --git a/.github/scripts/workflows.test.mjs b/.github/scripts/workflows.test.mjs index 9aa02fe..eb0cd6a 100644 --- a/.github/scripts/workflows.test.mjs +++ b/.github/scripts/workflows.test.mjs @@ -249,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")); diff --git a/contracts/script/check-deployed-sources.sh b/contracts/script/check-deployed-sources.sh index 0c71976..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