ci: the outbound check drops what it cannot parse, and cannot see non-JS files

[pitimon]

Deferred from #109 on the reviewer's own advice ("the rest can follow as issues"). Both halves are real; neither is fixable without care that would have rushed that PR.

1. Unresolvable authorities are dropped, not reported

scripts/validate-outbound.cjs returns null when an authority cannot be reduced to a host, and collectHosts skips it. Three live consequences, all real requests the check never mentions:

• https://[2606:4700:4700::1111]/… — bracketed IPv6
• https://gіthub.com/… — Cyrillic і; the browser punycodes it to an attacker domain, the check drops it
• https://intranet-host/ — single label, no dot

Failing closed is the right instinct and was implemented and then reverted inside #109, because it immediately fired on eight legitimate patterns: ${hostHeader} for the local bind, ${LOCAL_BIND_HOST}:${port}, ${values.HTTPSProxy}, git-remote parsing in rollout.js, and a log string with an ANSI reset after the URL.

A check that flags ordinary code is a check someone switches off, so this needs per-pattern handling — most likely a small declared set of known-local interpolations — rather than a flag flip.

2. The walker sees two directories and one extension set

Named places a host could live and be missed:

Where	Why it matters
dashboard/index.html, dashboard/pet.html	The served SPA shell. A <link rel=preconnect>, font CDN or <script src> here is a browser request. Today they load only local assets — verified.
dashboard/src/styles.css	Inside a scanned directory, skipped by extension. CSS url() is a request. No url() today — verified.
dashboard/src/content/i18n/*.json	Same: strings that flow into href/src props.
dashboard/vite.config.js	Dev proxy target; a new one is invisible.
scripts/build-pricing-seed.cjs	Fetches raw.githubusercontent.com outside the scan. Maintainer-run today.
Half B	Covers only dashboard/src. A server file adding fetch(cfg.someUrl) beside an existing host literal is unchecked, though the README's promise covers the server.

Also: isTestFile exempts by filename, not by import graph. A module named analytics.test.js is importable by the app and exempt from the scan.

3. link_from is an unconditional per-file waiver

Once a file is on a host's link_from, any request to that host from it passes forever — including an <img src> added later.

The concrete scenario, which is the shape of #100: adding <img src={https://github.com/${skill.repoOwner}.png`} />toSkillDetailPanel.jsx— a plausible "show skill-author avatars" PR — passes today. That file holds agithub.com` waiver and already interpolates owner names.

Fix: pin each link_from entry to a line number or to the exact URL literal, so a new string in a waived file is a visible diff in the inventory rather than silently covered.

4. Trailing comments will produce false findings

COMMENT_LINE_RE matches only line-leading comments, so

const x = 5; // see https://github.com/nodejs/node/issues/123

classifies as a request. Reference links in trailing comments are common, and this is the likeliest source of pressure to disable the check.

Priority

4 first — it costs nothing and prevents the check being switched off, which would take the rest with it. Then 3, then 1, then 2.

[pitimon]

Adding the items that survived three QA rounds on #109 and were deliberately deferred rather than rushed.

5. URL composition defeats the literal scan

scripts/validate-outbound.cjs scans literals. Host resolution is now correct — it defers to new URL(), so punycode, ideographic dots, backslash-as-slash, userinfo, percent-encoding and case all resolve the way the runtime resolves them. What it still cannot see is a URL that never exists as one literal:

const BASE = "https://" + host;              // concatenation
const url = `https://${a}${b}.example/x`;    // two interpolations, no dot boundary
import { API } from "./config";              // base imported from elsewhere
fetch(new URL(pathPart, API));               // joined at the call site

Also flagged: a URL split across the lines of a template literal, and String.raw (not used in this repo today, but its decoding differs and would need its own handling).

This is the honest boundary of the current design. Closing it means moving from "scan literals" to "follow expressions" — a small dataflow pass or a Babel/TS AST walk with constant folding across module boundaries. That is a different piece of work with its own failure modes, not a patch, which is why it is here rather than in #109.

Interim mitigation already in place: checkDynamicFetches flags a non-literal fetch() target in any dashboard/src file that also names an external host. It is narrow — it does not cover imported bases or Image.src — and it should be treated as a speed bump, not the control.

6. Inventory metadata is not validated

from, user_data and purpose are free text that nothing checks. user_data: true with readme: false passes; so does a wrong from, or purpose prose that describes something the code does not do. Only the readme flag and a substring search of the README are enforced.

The reviewer's own note: "add a schema/metadata-consistency validator as follow-up; current entries were source-checked."

Worth pairing with a check that from matches where the host actually appears — a host requested only from src/ cannot honestly be browser.

Already closed, for the record

Two items from the earlier list are done and should not be re-planned:

• The avatar proxy followed redirects off its own allowlist — fetch(..., { redirect: "follow" }) validates nothing after the first request, so an allowlisted CDN redirect reached 169.254.169.254 or any local service. Fixed in fix(validate): check where a host is REQUESTED, not merely mentioned #109 with per-hop revalidation, a scheme check and a hop ceiling, plus five tests including one asserting the metadata address is never requested.
• scripts/ scope — still open as item 2 above, unchanged.

Suggested order

4 (trailing comments) → 3 (link_from pinning) → 6 (metadata) → 1 (fail-closed) → 2 (walker scope) → 5 (composition).

5 is last on purpose. It is the largest and the least likely to be exercised by an accidental regression; items 3 and 4 are small, and 4 in particular protects the whole control from being switched off for noise.

[pitimon]

Item 7 — the avatar proxy buffers the upstream response without a size limit

Found while closing the round-3 port finding, and not introduced by it — this is pre-existing and out of scope for #109.

AVATAR_PROXY_MAX_BYTES reads like a download cap. It is not one. At src/lib/local-api.js:923 the full upstream body is buffered first:

const body = Buffer.from(await upstream.arrayBuffer());
if (body.length <= AVATAR_PROXY_MAX_BYTES) {   // :924 — gates CACHING only

The check at :924 decides only whether the body enters avatarProxyCache. An oversized response is still fully read into memory and still written to the client at :932. So an allowlisted host serving a very large image/* response drives loopback-server memory with no ceiling.

Fix direction: cap during the read rather than after it — check content-length before buffering, and stream the body with a running byte count that aborts past the limit. Then AVATAR_PROXY_MAX_BYTES means what its name says.

Correction to the round-3 QA record

The round-3 reviewer marked "final-hop body/type checks … remain applied" as Pass. That is accurate as written — the port fix did not break those checks — but it should not be read as "the download is capped." It was not, before or after.

Note on Definition-of-Done item 4 (inventory accuracy)

validate:outbound reporting "24 declared hosts, all reachable ones accounted for" is scoped to what the literal scanner sees. The 12 hosts in AVATAR_HOST_ALLOWLIST (src/lib/local-api.js:861-874) are reachable by the server and are absent from outbound-hosts.json — invisible to the scanner because they are bare hostname strings with no scheme for URL_RE and no // for PROTOCOL_RELATIVE_RE.

Already covered by item 4 (scripts/ and server scan scope) and accepted as a deferral in QA round 2. Recorded here so the green validator line is not mistaken for full inventory coverage.

[pitimon]

Six of seven items are closed

Item	Status	Where
1 — unresolvable authorities dropped	closed	#114
2 — walker scope	closed	#114
3 — link_from file-wide waiver	closed	#111
4 — trailing comments	closed	#111
5 — URL composition	open	see below
6 — metadata unvalidated	closed	#114
7 — avatar body cap	closed	#112

main is at e6fb9eb; ci:local exit 0 with 909 root tests and 256 dashboard.

Two corrections to what this issue said

Item 1 was much narrower than written. All three examples in the original text — bracketed IPv6, the Cyrillic lookalike, the single-label intranet host — resolve correctly today. They were written before #109 replaced hand-parsing with new URL(). What actually remained was interpolation that pins nothing: 15 occurrences across 8 files, every one ordinary code. That is why the flag-flip version had to be reverted, and why each is now declared with a reason rather than the check being made stricter.

Item 2 was worth more than it looked. Widening the walker surfaced eight things nothing had ever mentioned, including two real requests: scripts/build-pricing-seed.cjs fetching raw.githubusercontent.com, and dashboard/vite.config.js proxying to ip.net.coffee. Both were outside the scan entirely.

Item 5 stays open, deliberately

Closing it means moving from scanning literals to following expressions — a dataflow or AST pass with constant folding across module boundaries. That is a different piece of work with its own failure modes, not a patch on the current design.

The honest boundary today: host resolution is correct (it defers to new URL(), so punycode, ideographic dots, backslash-as-slash, userinfo, percent-encoding and case all resolve the way the runtime resolves them), and an authority that cannot be resolved is now reported rather than dropped. What remains invisible is a URL that never exists as one literal — concatenation, two interpolations with no dot boundary, an imported base, a join at the call site.

checkDynamicFetches is a speed bump on that, not a control: it only covers a non-literal fetch() in a dashboard/src file that also names an external host, and does not cover imported bases or Image.src.

Also worth recording

The AVATAR_HOST_ALLOWLIST hosts (src/lib/local-api.js) are still absent from outbound-hosts.json. They are bare hostname strings with no scheme, so URL_RE cannot see them even now that src/ is fully scanned. The proxy is closed — allowlisted per hop, port-checked, size-capped — but the inventory does not name those 12 hosts. Reachable-and-undeclared, tracked here rather than left implied by a green validator line.