diff --git a/outbound-hosts.json b/outbound-hosts.json index 7f43e09a..7d092a07 100644 --- a/outbound-hosts.json +++ b/outbound-hosts.json @@ -6,7 +6,10 @@ "fields": { "from": "server = the local Node process. browser = the user's browser, i.e. it happens even though we call ourselves local-first.", "user_data": "true when the request itself discloses something about the user or their work \u2014 a repo name in the path, an account token, their IP. false for anonymous public fetches.", - "readme": "true when this host must appear in the README outbound table. Every user_data host must." + "readme": "true when this host must appear in the README outbound table. Every user_data host must.", + "request_from": "Files allowed to actually REACH this host. A whole-file permission, because a file listed here is declared to be a caller of this host.", + "link_from": "Places the host appears as something the USER clicks, never a request this code makes. Pinned per URL: {\"file\": ..., \"url\": ...}, where url is the literal as the scanner reads it. A bare filename is rejected — it waived the whole file forever, so a later in that file passed silently, which is the shape of #100.", + "data_from": "Same pinned form. The host is a value this codebase stores or renders and never fetches — a mock fixture's project_ref, for instance." } }, "hosts": [ @@ -205,16 +208,44 @@ ], "request_from": [], "link_from": [ - "dashboard/src/components/LocalOnlyNotice.jsx", - "dashboard/src/pages/SkillsPage.jsx", - "dashboard/src/pages/WidgetsPage.jsx", - "dashboard/src/ui/components/HeaderGithubStar.jsx", - "dashboard/src/pages/SkillDetailPanel.jsx", - "src/commands/init.js", - "src/lib/skills-manager.js" + { + "file": "dashboard/src/components/LocalOnlyNotice.jsx", + "url": "https://github.com/pitimon/TokenTracker/releases/latest" + }, + { + "file": "dashboard/src/pages/SkillsPage.jsx", + "url": "https://github.com/${skill.repoOwner}/${skill.repoName}" + }, + { + "file": "dashboard/src/pages/WidgetsPage.jsx", + "url": "https://github.com/pitimon/TokenTracker/releases/latest" + }, + { + "file": "dashboard/src/ui/components/HeaderGithubStar.jsx", + "url": "https://github.com/${repo}" + }, + { + "file": "dashboard/src/pages/SkillDetailPanel.jsx", + "url": "https://github.com/${skill.repoOwner}/${skill.repoName}" + }, + { + "file": "src/commands/init.js", + "url": "https://github.com/pitimon/TokenTracker" + }, + { + "file": "src/lib/skills-manager.js", + "url": "https://github.com/${owner}/${name}/blob/${branch}/${filePath" + }, + { + "file": "src/lib/skills-manager.js", + "url": "https://github.com/${owner}/${repoName}" + } ], "data_from": [ - "dashboard/src/lib/mock-data.ts" + { + "file": "dashboard/src/lib/mock-data.ts", + "url": "https://github.com/${repo}" + } ] }, { diff --git a/scripts/validate-outbound.cjs b/scripts/validate-outbound.cjs index cd2474f3..4f409c79 100644 --- a/scripts/validate-outbound.cjs +++ b/scripts/validate-outbound.cjs @@ -125,7 +125,76 @@ function resolveHost(rawAuthority) { } const MENTION_CALL_RE = /\.(replace|split|startsWith|endsWith|includes|slice|indexOf|lastIndexOf)\s*\(\s*$/; -const COMMENT_LINE_RE = /^\s*(\/\/|\*|\/\*)/; +// Only the JSDoc continuation line, which is the one shape commentRanges cannot +// see: it carries no block state across lines by design. `//` and `/*` are now +// handled positionally instead — this regex used to match them too, and a line +// beginning `/* note */ fetch("https://…")` was exempted whole, comment and live +// call alike. +const COMMENT_LINE_RE = /^\s*\*/; + +// Where the comments are on this line, as [start, end) ranges. +// +// A leading-comment regex was not enough: it matched only a comment that starts +// the line, so +// +// const x = 5; // see https://github.com/nodejs/node/issues/123 +// +// classified as a request. Reference links in trailing comments are ordinary +// code, and a check that flags ordinary code is a check someone switches off — +// which would take the rest of this control with it. +// +// This scans rather than matches, because the token we are looking for — `//` — +// is the one every URL contains. `fetch("https://x")` must not read as a +// comment, so string state has to be tracked to tell the two apart. +// +// Deliberately line-local: no block-comment state is carried across lines. A +// regex literal like /[/*]/ opens a block comment as far as this scanner is +// concerned, and carrying that state forward would silence the REST OF THE FILE +// — a miss, the one direction this check must never fail in. Confined to one +// line, the same mistake costs at most one false finding, which is visible and +// fixable. Multi-line comments stay covered by COMMENT_LINE_RE's `*` arm, which +// is the JSDoc style used throughout this repo. +function commentRanges(line) { + const ranges = []; + let quote = null; + let blockStart = -1; + for (let i = 0; i < line.length; i += 1) { + const char = line[i]; + const next = line[i + 1]; + if (blockStart >= 0) { + if (char === "*" && next === "/") { + ranges.push([blockStart, i + 2]); + blockStart = -1; + i += 1; + } + continue; + } + if (quote) { + if (char === "\\") i += 1; + else if (char === quote) quote = null; + continue; + } + if (char === '"' || char === "'" || char === "`") { + quote = char; + continue; + } + if (char === "/" && next === "/") { + ranges.push([i, line.length]); + return ranges; + } + if (char === "/" && next === "*") { + blockStart = i; + i += 1; + } + } + // An unterminated `/*` comments out the rest of THIS line and no further. + if (blockStart >= 0) ranges.push([blockStart, line.length]); + return ranges; +} + +function isInComment(ranges, index) { + return ranges.some(([start, end]) => index >= start && index < end); +} // A URL is a REQUEST unless one of three things is true at the URL's own // position: the line is a comment, the URL is the direct argument of a string @@ -139,6 +208,7 @@ const COMMENT_LINE_RE = /^\s*(\/\/|\*|\/\*)/; // diff a reviewer has to approve, which is the same weight as `request_from`. function isMentionOnly(line, matchIndex) { if (COMMENT_LINE_RE.test(line)) return true; + if (isInComment(commentRanges(line), matchIndex)) return true; // Also strip a regex-literal opener. `repoInput.replace(/^https:\/\/github\.com\//, "")` // is string surgery on user input, but the `/^` between `replace(` and the URL // hid that from a test that only looked past quotes and braces. @@ -196,8 +266,16 @@ function collectHosts({ root = ROOT } = {}) { if (isMentionOnly(line, match.index)) continue; if (!mentions.has(host)) mentions.set(host, new Set()); mentions.get(host).add(relative); - if (!requests.has(host)) requests.set(host, new Map()); - requests.get(host).set(relative, index + 1); + // Every occurrence, not one per file. This was a Map keyed by file, so + // a second request to the same host from the same file overwrote the + // first and only the LAST line was ever reported — and with waivers now + // pinned to the URL text, each occurrence has to be matched on its own. + if (!requests.has(host)) requests.set(host, []); + requests.get(host).push({ + file: relative, + line: index + 1, + text: match[0].replace(/^["'`{\s]+/, ""), + }); } }); } @@ -279,6 +357,89 @@ function isIgnored(host, inventory) { return ignored.includes(host); } +// A declared host may only be REQUESTED from where the inventory says. `seen_in` +// records every file that names the host, which is inventory; the three lists +// below record what may actually reach it, which is the security constraint. +// Conflating them is what let the original issue-100 call be re-added to a file +// that legitimately mentions github.com. +// +// `request_from` = this file calls the host, and is a whole-file permission +// because the file is declared to be a caller. `link_from` = the user clicks it. +// `data_from` = the host is a value this codebase stores or renders and never +// fetches — a mock fixture's `project_ref`, for instance. +// +// The last two are PINNED TO THE URL TEXT, not to the file. A file-wide waiver +// was an unconditional one: once a file was listed, any future request to that +// host from it passed forever. The concrete scenario is the shape of #100 — +// adding an owner-avatar `` to SkillDetailPanel.jsx, a plausible "show +// skill authors" change, to a file that already holds a github.com link waiver +// and already interpolates owner names. +// +// Pinned to the literal rather than to a line number because line numbers move +// under every edit above them, and a waiver that churns is a waiver people +// rubber-stamp. A NEW url string in a waived file is a diff in the inventory +// that a reviewer has to approve. +function checkRequestPermission(entry, records) { + const findings = []; + const allowed = new Set(entry.request_from || []); + const waived = [...(entry.link_from || []), ...(entry.data_from || [])]; + + // A bare string here used to mean "waive this whole file". Reject the old + // shape outright rather than letting it read as an unmatched pin: the point of + // the change is that a file-wide waiver cannot be written any more. + for (const pin of waived) { + if (pin && typeof pin.file === "string" && typeof pin.url === "string") continue; + findings.push( + `outbound-hosts.json has a link_from/data_from entry for '${entry.host}' that is not` + + ` {"file": ..., "url": ...}: ${JSON.stringify(pin)} — a bare filename is a file-wide` + + ` waiver, which is what the pinned form replaced`, + ); + } + + const used = new Set(); + for (const record of records) { + if (allowed.has(record.file)) continue; + const pin = waived.findIndex((w) => w && w.file === record.file && w.url === record.text); + if (pin >= 0) { + used.add(pin); + continue; + } + findings.push( + `${record.file}:${record.line} requests '${entry.host}' as ${JSON.stringify(record.text)},` + + ` which no request_from, link_from or data_from entry covers — if this call belongs,` + + ` add it to outbound-hosts.json and re-read the purpose field first; if it does not,` + + ` this is the defect the check exists for`, + ); + } + + // A pin that matches nothing is a stale waiver: a live exemption waiting for a + // URL to drift back onto it. + waived.forEach((pin, index) => { + if (used.has(index) || !pin) return; + findings.push( + `outbound-hosts.json pins ${JSON.stringify(pin.url)} in ${pin.file} for '${entry.host}',` + + ` but no such URL is there — remove the stale waiver`, + ); + }); + + return findings; +} + +// Every host that discloses something about the user must be in the README +// table. The table is the promise; this is what keeps it true. +function checkReadmeTable(root, inventory) { + const readmePath = path.join(root, "README.md"); + if (!fs.existsSync(readmePath)) return []; + const readme = fs.readFileSync(readmePath, "utf8"); + return (inventory.hosts || []) + .filter((entry) => entry.readme && !readme.includes(entry.host)) + .map( + (entry) => + `README.md does not mention '${entry.host}', which outbound-hosts.json marks as` + + (entry.user_data ? " carrying user data" : " user-visible"), + ); +} + function checkOutbound({ root = ROOT } = {}) { const inventory = loadInventory(root); if (!inventory) return ["outbound-hosts.json is missing"]; @@ -308,44 +469,13 @@ function checkOutbound({ root = ROOT } = {}) { } } - // 3. A declared host may only be REQUESTED from the files listed in - // request_from. `seen_in` records every file that names the host, which is - // inventory; `request_from` records the files allowed to actually reach it, - // which is the security constraint. Conflating them is what let the original - // issue-100 call be re-added to a file that legitimately mentions github.com. + // 3. for (const entry of inventory.hosts || []) { - const allowed = new Set(entry.request_from || []); - // `link_from` = the user clicks it. `data_from` = the host appears only as a - // value this codebase stores or renders and never fetches — a mock fixture's - // `project_ref`, for instance. Without the second category such a file ends up - // on the link list, which is an UNCONDITIONAL waiver: once there, any future - // request to that host from that file passes forever, including an . - const linkOnly = new Set([...(entry.link_from || []), ...(entry.data_from || [])]); - for (const [file, line] of found.requests.get(entry.host) || []) { - if (allowed.has(file) || linkOnly.has(file)) continue; - findings.push( - `${file}:${line} requests '${entry.host}', which is not in its request_from list` + - ` — if this call belongs, add the file to outbound-hosts.json and re-read the` + - ` purpose field first; if it does not, this is the defect the check exists for`, - ); - } + findings.push(...checkRequestPermission(entry, found.requests.get(entry.host) || [])); } - // 4. Every host that discloses something about the user must be in the - // README table. The table is the promise; this is what keeps it true. - const readmePath = path.join(root, "README.md"); - if (fs.existsSync(readmePath)) { - const readme = fs.readFileSync(readmePath, "utf8"); - for (const entry of inventory.hosts || []) { - if (!entry.readme) continue; - if (!readme.includes(entry.host)) { - findings.push( - `README.md does not mention '${entry.host}', which outbound-hosts.json marks as` + - (entry.user_data ? " carrying user data" : " user-visible"), - ); - } - } - } + // 4. + findings.push(...checkReadmeTable(root, inventory)); findings.push(...checkDynamicFetches(root, inventory)); diff --git a/test/outbound-inventory.test.js b/test/outbound-inventory.test.js index 78aede17..d7604e35 100644 --- a/test/outbound-inventory.test.js +++ b/test/outbound-inventory.test.js @@ -160,7 +160,11 @@ test("request_from is enforced, so it cannot drift into decoration", () => { files: { "src/a.js": 'const u = "https://x.example/1";\n' }, hosts: [declared("x.example", { request_from: [] })], }); - assert.ok(checkOutbound({ root }).some((f) => f.includes("not in its request_from list"))); + assert.ok( + checkOutbound({ root }).some((f) => + f.includes("no request_from, link_from or data_from entry covers"), + ), + ); }); // --- Half B ------------------------------------------------------------------ @@ -244,13 +248,155 @@ test("a comment is a mention; a clickable link needs link_from", () => { declared("docs.example", { seen_in: ["dashboard/src/Doc.jsx"], request_from: [], - link_from: ["dashboard/src/Doc.jsx"], + link_from: [{ file: "dashboard/src/Doc.jsx", url: "https://docs.example/guide" }], }), ], }); assert.deepEqual(checkOutbound({ root }), []); }); +// --- Comments and waivers ----------------------------------------------------- +// Two ways this control gets switched off: it cries wolf on ordinary code, or it +// hands out a permission wider than the one written down. + +test("a URL in a TRAILING comment is a mention, not a request", () => { + // Only line-LEADING comments were recognised, so a reference link after code + // read as a request. That shape is common enough to be the likeliest source of + // pressure to disable the whole check. + const root = fixture({ + files: { + "src/a.js": 'const x = 5; // see https://docs.example/issues/123\nmodule.exports = x;\n', + }, + }); + assert.deepEqual(checkOutbound({ root }), []); +}); + +test("`//` inside a string is not a comment", () => { + // The token the comment scanner looks for is the one every URL contains, so + // string state has to be tracked. Get this wrong and the check exempts + // everything it exists to catch. + const findings = attack('const x = fetch("https://shared.example/x");'); + assert.ok( + findings.some((f) => f.includes("shared.example")), + `a plain fetch must still be a request: ${JSON.stringify(findings)}`, + ); +}); + +test("code after a CLOSED block comment on the same line is still a request", () => { + // Comments are ranges, not a single start index. Treating the first `/*` as + // "comment from here on" would exempt the live call that follows it. + const findings = attack('/* note */ fetch("https://shared.example/x");'); + assert.ok( + findings.some((f) => f.includes("shared.example")), + `the call after the comment must survive: ${JSON.stringify(findings)}`, + ); +}); + +test("an unterminated /* comments out its own line and no more", () => { + // A regex literal like /[/*]/ opens a block comment as far as this scanner is + // concerned. Carrying that state across lines would silence the rest of the + // file — a miss, the one direction this check must never fail in. + const root = fixture({ + files: { + "dashboard/src/a.jsx": [ + "const re = /[/*]/;", + 'const img = ;', + "", + ].join("\n"), + }, + }); + assert.ok( + checkOutbound({ root }).some((f) => f.includes("tracker.example")), + "the next line must still be scanned", + ); +}); + +test("a waiver covers the URL it names, not the file it sits in", () => { + // The #100 shape: SkillDetailPanel.jsx holds a github.com link waiver and + // already interpolates owner names, so "show skill-author avatars" — an added beside the link — passed under a file-wide waiver. Pinning to the + // literal makes the new string a diff in the inventory instead. + const root = fixture({ + files: { + "dashboard/src/Panel.jsx": [ + 'const link = docs;', + "const avatar = ;", + "", + ].join("\n"), + }, + hosts: [ + declared("docs.example", { + seen_in: ["dashboard/src/Panel.jsx"], + request_from: [], + link_from: [{ file: "dashboard/src/Panel.jsx", url: "https://docs.example/guide" }], + }), + ], + }); + const findings = checkOutbound({ root }); + assert.equal(findings.length, 1, JSON.stringify(findings)); + assert.ok(findings[0].includes("${owner}.png"), findings[0]); +}); + +test("every request to a host from one file is reported, not just the last", () => { + // The request map was keyed by file, so a second call overwrote the first and + // only one line was ever seen. With waivers pinned per URL, each occurrence has + // to be matched on its own. + const root = fixture({ + files: { + "dashboard/src/Panel.jsx": [ + 'fetch("https://docs.example/one");', + 'fetch("https://docs.example/two");', + "", + ].join("\n"), + }, + hosts: [ + declared("docs.example", { seen_in: ["dashboard/src/Panel.jsx"], request_from: [] }), + ], + }); + const findings = checkOutbound({ root }); + assert.equal(findings.length, 2, JSON.stringify(findings)); + assert.ok(findings.some((f) => f.includes("/one")) && findings.some((f) => f.includes("/two"))); +}); + +test("a pin that matches nothing is reported as a stale waiver", () => { + // Left in place it is a live exemption waiting for a URL to drift back onto it. + const root = fixture({ + files: { "dashboard/src/Panel.jsx": 'const link = d;\n' }, + hosts: [ + declared("docs.example", { + seen_in: ["dashboard/src/Panel.jsx"], + request_from: [], + link_from: [ + { file: "dashboard/src/Panel.jsx", url: "https://docs.example/guide" }, + { file: "dashboard/src/Panel.jsx", url: "https://docs.example/gone" }, + ], + }), + ], + }); + const findings = checkOutbound({ root }); + assert.equal(findings.length, 1, JSON.stringify(findings)); + assert.ok(findings[0].includes("stale waiver"), findings[0]); +}); + +test("a bare filename is rejected as a waiver, not read as an unmatched pin", () => { + // The old shape WAS the defect. It has to fail loudly rather than degrade into + // a confusing "no such URL" message. + const root = fixture({ + files: { "dashboard/src/Panel.jsx": 'const link = d;\n' }, + hosts: [ + declared("docs.example", { + seen_in: ["dashboard/src/Panel.jsx"], + request_from: [], + link_from: ["dashboard/src/Panel.jsx"], + }), + ], + }); + assert.ok( + checkOutbound({ root }).some((f) => f.includes("file-wide waiver")), + "the old form must be named as the reason", + ); +}); + // --- Evasion ------------------------------------------------------------------ // Written after adversarially attacking the check rather than only testing that // it works. Three of these were live holes in the first cut of the sink model. @@ -320,7 +466,9 @@ test("a URL that IS the argument of a string operation stays a mention", () => { }); test("link_from covers a host the user clicks, and only where declared", () => { - const asLink = shared({ link_from: ["dashboard/src/A.jsx"] }); + const asLink = shared({ + link_from: [{ file: "dashboard/src/A.jsx", url: "https://shared.example/releases/latest" }], + }); assert.deepEqual(attack('const RELEASES = "https://shared.example/releases/latest";', [asLink]), []); // The same file without the declaration is a request. assert.ok(attack('const RELEASES = "https://shared.example/releases/latest";').length > 0); @@ -413,8 +561,11 @@ test("an interpolated suffix pins only a real zone, never a bare TLD", () => { test("data_from covers a host stored as a value and never fetched", () => { // A mock fixture's `project_ref` is neither a link nor a request. Without its // own category it lands on `link_from`, which is an UNCONDITIONAL waiver: once - // a file is there, any future request to that host from it passes forever. - const asData = shared({ data_from: ["dashboard/src/A.jsx"] }); + // a file is there, any future request to that host from it passed forever — + // which is why both categories are now pinned to the URL rather than the file. + const asData = shared({ + data_from: [{ file: "dashboard/src/A.jsx", url: "https://shared.example/${repo}" }], + }); assert.deepEqual(attack('const row = { project_ref: `https://shared.example/${repo}` };', [asData]), []); });