fix(validate): check where a host is REQUESTED, not merely mentioned - #109
Merged
Conversation
added 4 commits
July 25, 2026 15:28
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
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.
…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.
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
added 6 commits
July 25, 2026 17:50
…tted 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.
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
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.
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.
`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.
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.
pitimon
added a commit
that referenced
this pull request
Jul 25, 2026
…ose (#114) Closes #110 items 2, 6 and 1. Stacked on #111. ITEM 2 — the walker saw two directories and one extension set. Added: dashboard/*.html (the served shell — a preconnect, font CDN or script src here is a browser request), .css (skipped by EXTENSION while the walker was already standing in that directory), i18n .json, dashboard/vite.config.js, and scripts/. Maintainer-run is a caveat for the purpose field, not a reason to be invisible. That surfaced eight things nothing had ever mentioned, each read in source before being classified: scripts/build-pricing-seed.cjs really fetches the LiteLLM list -> request_from seed-snapshot.json records that URL as a value -> data_from dashboard/vite.config.js proxies /proxy/ipcheck -> request_from dashboard/index.html footer links to the upstream repo -> link_from www.tokentracker.cc JSON-LD metadata, not fetched -> new entry www.npmjs.com same block -> new entry schema.org JSON-LD @context identifier -> ignored example.invalid reserved never-resolving TLD -> ignored ITEM 6 — the inventory's prose was unchecked. `user_data: true` with `readme: false` passed; so did a wrong `from` and purpose text describing something the code does not do. The fields a reader trusts most were the ones nothing verified. Types and required values are now checked, and `from` is checked against WHERE THE HOST IS ACTUALLY REACHED — a host only ever requested from src/ cannot honestly be `browser`. ITEM 1 — unresolvable authorities were dropped. The issue's three examples (bracketed IPv6, a Cyrillic lookalike, a single-label intranet host) all resolve now; they predate deferring to new URL(). What is left is interpolation that pins nothing, which is ordinary code — which is why the flag-flip version had to be reverted inside #109. So each is declared with a why, matched on file + authority text, and a new one fails. 15 declared across 8 files: the local bind and Host-header parsing, git remote normalisation into project_ref, the user's own HTTPS_PROXY, dev-server proxying, and this scanner's own probe strings. Also: checkOutbound and collectHosts were both over the 50-line rule; split into scanLine (39), checkUnresolved (30) and checkInventoryMetadata (29), leaving them at 46 and 21. File is 640 lines. 19 new tests. The metadata check was proved non-vacuous against five deliberately-wrong entries before being committed, with a valid control. ci:local exit 0: 903 root tests, 256 dashboard. Co-authored-by: itarun.p <itarun.p@somapait.com>
pitimon
added a commit
that referenced
this pull request
Jul 25, 2026
…caching (#112) Closes #110 item 7. Pre-existing; found while closing the round-3 port finding on #109, and deliberately left out of that PR. The whole upstream body was buffered with arrayBuffer() first, and the constant then only decided whether the result entered avatarProxyCache. An oversized response was still read into memory in full and still written to the client, so an allowlisted host serving a very large image/* drove loopback-server memory with no ceiling. readCappedAvatarBody() reads at most maxBytes and stops there. Two gates, because either alone is soft: content-length is a claim the peer makes and can omit or lie about, so check the claim AND count the bytes. Returning from the for-await calls the iterator's return(), which cancels the stream. Measured: a 5120-byte body against a 1024-byte cap now reads 1280 bytes — one chunk past the limit, which is inherent since a chunk cannot be known to overrun until it has arrived. Not "stops exactly at the cap"; bounded at cap + one chunk. Behaviour change: an over-cap avatar is now REFUSED with 413 rather than served and merely not cached. 512 KiB is far above any real avatar, and the dashboard falls back to its local icon on a failed image load. 6 tests, including that an over-cap content-length is refused with the body never touched, that a lying content-length does not get past the byte count, that a body exactly at the cap is allowed, and that the no-stream path is bounded too. ci:local exit 0: 889 root tests, 256 dashboard. Co-authored-by: itarun.p <itarun.p@somapait.com>
This was referenced Jul 26, 2026
Merged
pitimon
added a commit
that referenced
this pull request
Jul 26, 2026
15 commits since v0.39.43. Tracker: #125. User-facing: - The Projects panel honours the date range. It read no query parameter at all, so picking "24h" narrowed every other card while Projects kept showing all-time totals with nothing on screen saying so (#118). - Per-repo cost, with rows written before the change named as unpriced rather than shown as a confident $0 (#121). - Sources with no per-repo attribution are named, instead of their absence reading as "that tool cost nothing here" (#118). - A plan-value card answering what README:32 already promised, labelled as list-price-equivalent rather than a saving (#122). - The Codex quota chip no longer vanishes on a 401 (#119). - `sync --compact`, which reclaimed 34,924 lines to 5,636 on a real install (#117). Behind the scenes: outbound-privacy validator hardening (#111, #114), avatar proxy per-hop revalidation, port-aware allowlisting and a real download cap (#109, #112), a parser conformance ratchet (#116, #120), scripts/graph out of the product gate (#115), and a usage-limits fixture capture tool (#124). Version bumped in all four lockstep locations by the `version` hook: package.json, package-lock.json, both TokenTrackerBar targets, and the Windows csproj. validate:version-lockstep passes. WATCH AFTER SHIPPING: #121 migrates cursors.json on every user's first sync after upgrading, re-keying project buckets from project|source|hour to project|source|model|hour. Without that the old bucket strands and its usage is counted twice. Verified read-only against a real 2,015-bucket state — every key migrated, total_tokens preserved exactly at 6,281,653,062, and all 2,015 queuedKey markers survived, the loss of which would re-append every row. That is one machine; a differently-shaped state is the residual risk. ci:local exit 0: 972 root tests, 302 dashboard. 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 #101. Finishes what #108 started, after an independent Codex QA pass returned SHIP: NO on the merged result. All six findings were verified against source before being acted on; none were hallucinated.
The validator did not catch the defect it was built for
Re-adding the original leak, in the original file:
Reproduced on
mainafter #108 merged. The cause is structural:seen_inis file-level, andProjectUsagePanel.jsxlegitimately contains"https://github.com/"as a prefix it strips offproject_ref. That putgithub.comin its declared set, and the rule could not tell a mention from a request.Two views of the same scan
seen_inrequest_fromBoth 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. Listing the request constructs instead was tried first and under-detected:const url = new URL("https://skills.sh/...")andfetch(url)sit on different lines, so no sink ever shares a line with that host. For a security control, missing a real destination is worse than asking for one more declaration.A host the scanner could not see
IpCheckPage.jsx:590:A real browser request, to a per-run DNS-probe subdomain. The old host pattern matched nothing when a host began with an interpolation — so this destination was invisible to the check that most needed to see it, while the inventory described
ip.net.coffeeas server-only.Interpolations are now collapsed and the literal suffix pinned (
d.ip.net.coffee), with a guard that returns nothing rather than invent one when the expression runs past the match —http://${req.headers.host || "localhost"}was otherwise reported as a destination calledreq.headers.host, and a check that reports noise gets switched off.Two false README rows, both written in #108
raw.githubusercontent.comwas described as the price list only.skills-manager.js:364also downloads the files of a skill you install, carrying owner, repo, branch and path.Same defect class as #100, in the table meant to prevent it. That is the argument for finishing this issue rather than re-reading the table more carefully.
Smaller, also verified
DataDetails.jsxProjects tab rendered an empty box with no message — reads as "you spent nothing" rather than "nothing is attributed yet". The Daily tab has always explained itself.HeaderGithubStartook 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 this is hardening.Test plan
npm run ci:local— exit 0, 857/857 root (+3), 256 dashboard (+1)<a href>are mentions, not requests — a check that flags prose gets turned offWorking note
Shell
grepreturned empty for content that was present three times during this work — it missedskills.sh, thenraw.githubusercontent.comin the same file. Reading withnodefound both instantly. That discrepancy is a large part of why the original hand-audit in #95 was incomplete: not only the wrong directory, but a tool that under-reported.