fix(gate): refuse IP literals and non-https URLs before probing them by msalvatti · Pull Request #64 · bymaxone/nest-queue

msalvatti · 2026-08-01T18:55:09Z

Hardening for scripts/check-published-surface.mjs, found by Copilot while reviewing the same script in nest-cache (bymaxone/nest-cache#42) and synced here byte-for-byte.

Why

The gate runs on pull_request and fetch()es every http(s) URL it finds in the README. Nothing stopped a fork from putting https://169.254.169.254/latest/meta-data or a loopback address in the README and having the CI runner probe an endpoint the author cannot reach themselves.

What changed

Every bare IP literal is refused — v4 and v6, public and private — before any request is made. The first version of this guard enumerated the IPv4 private ranges plus ::1, which still let unique-local (fd00::/8), link-local (fe80::/10) and IPv4-mapped (::ffff:…) literals through. Enumerating ranges means enumerating them again for the next notation, so the class goes instead: a link in the documentation points at a hostname. A URL parser brackets every IPv6 literal, so the brackets are the test — no address notation to recognise. The private ranges keep their own message because naming the range is the more useful diagnosis when a link does hit one. Non-https URLs are refused the same way.

Rejected as reported findings, not silent skips: a published README linking to a private address is a documentation defect either way.

Red-checked one class at a time, each by appending a link to the README:

https://[fd00::1]/x              → is a unique-local address
https://[fe80::1]/x              → is a link-local address
https://[::1]/x                  → is a loopback address
https://[::ffff:127.0.0.1]/x     → is an IPv6 literal, not a hostname
https://[2606:4700::1111]/x      → is an IPv6 literal, not a hostname
https://93.184.216.34/x          → is an IPv4 literal, not a hostname
https://169.254.169.254/l        → is a link-local address (cloud metadata range)
https://10.1.2.3/x               → is a private address
http://example.com/x             → uses http, not https

Also in this change

The consumer link is created as a 'junction' rather than a 'dir' symlink. A directory symlink needs elevation or Developer Mode on Windows, which would make prepublishOnly unrunnable there; Node ignores the type on POSIX.
The link-check section header claimed every link "resolves". The catch around the fetch returns null, so a transport error is a note and not a failure — deliberately, since an offline runner is not a defect in the documentation. The header now says exactly that.
release.yml no longer invokes pnpm check:published. prepublishOnly already runs it and the step above invokes pnpm prepublishOnly, so the second run re-probed every README link and recompiled the snippets for no new information — adding time and one more chance of a network flake to the one job that must not fail spuriously. This was flagged earlier and is fixed here alongside the rest.
URL joins the declared globals for the .mjs/.cjs tooling files; no-undef only knows the globals it is told about.

No published artifact changes — scripts/, .github/ and eslint.config.mjs are all outside package.json → files, so no version bump.

The published-surface gate runs on pull_request and fetches every http(s) URL it finds in the README, so a fork could point a link at a loopback, private or link-local address and have the CI runner probe an endpoint the author cannot reach themselves. Every bare IP literal is now refused, v4 and v6, public and private: enumerating ranges means enumerating them again for the next notation, and a link in the documentation points at a hostname. The private ranges keep their own message because naming the range is the more useful diagnosis when a link does hit one. Non-https URLs go the same way. Rejected as reported findings rather than silent skips — a published README linking to a private address is a documentation defect in its own right. The consumer link is created as a junction instead of a directory symlink, which needs elevation or Developer Mode on Windows, and the link-check section header now says what it actually enforces: a bad HTTP status fails, an unreachable host is only a note. release.yml no longer invokes check:published: prepublishOnly already runs it and the step above invokes prepublishOnly, so repeating it re-probed every README link for no new information. URL joins the declared globals for the plain-JavaScript tooling files.

Copilot

Pull request overview

This PR hardens the published-surface gate by refusing non-https URLs and documentation links that target IP literals before any network probe occurs, reducing SSRF risk during CI runs on untrusted PRs.

Changes:

Adds URL pre-validation to the README link checker (rejects IP literals, local names, and non-https URLs).
Adds a timeout to git fetch --tags and improves Windows portability for the consumer scaffold symlink.
Removes redundant pnpm check:published invocation from the release workflow and declares URL as an ESLint global for tooling scripts.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
scripts/check-published-surface.mjs	Adds URL/host hardening for link probing, bounds tag fetch time, and improves Windows path/symlink handling.
eslint.config.mjs	Declares `URL` as a known global for `.mjs/.cjs` tooling linting.
.github/workflows/release.yml	Avoids re-running `check:published` since it already runs in `prepublishOnly`.

A fully-qualified name keeps its trailing dot through the URL parser, so `localhost.` and `foo.localhost.` compared unequal to the loopback names and reached the fetch — a one-character bypass of the guard. The dot is stripped before the name is compared. 0.0.0.0 was reported as a loopback address. It is the unspecified address; the URL was already refused, only the diagnosis was wrong.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

scripts/check-published-surface.mjs:98

The message returned for .local hosts is misleading: .local (mDNS) is not inherently a loopback name, so reporting it as loopback can confuse diagnostics when the gate refuses a .local URL.

  if (h === 'localhost' || h.endsWith('.localhost') || h.endsWith('.local')) {
    return 'is a loopback name'
  }

…ectly The guard refused IP literals but not a hostname pointed at one, so a link to a name with a static 169.254.169.254 record still reached the CI runner. Each hostname is resolved first and refused if any address it answers with is loopback, private, link-local or unique-local. This narrows the hole rather than closing it: the fetch resolves the name a second time, so a record that changes between the two calls still gets through. Pinning the resolved address into the connection is more machinery than a documentation gate warrants — a static private record is the case worth stopping, and this stops it. A name that does not resolve is left to the fetch, which reports an unreachable host as a note. .local is mDNS, not loopback, and said so incorrectly.

msalvatti · 2026-08-01T19:08:27Z

Applied the suppressed comment on .local in 71365aa — it is an mDNS name resolvable only on the local link, not a loopback name, and the message now says that. Also in 71365aa: hostnames are resolved before they are fetched and refused when any resolved address is loopback, private, link-local or unique-local — a name pointed at 169.254.169.254 reaches the same endpoint an IP literal would. Red-checked against 169.254.169.254.nip.io, 10.0.0.1.sslip.io, 127.0.0.1.nip.io and localtest.me, all refused before any request. The limit is stated in the code: the fetch resolves a second time, so a record that changes between the two calls still gets through; pinning the address into the connection is more machinery than a docs gate warrants.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

scripts/check-published-surface.mjs:146

dns.promises.lookup() is awaited without any timeout. If the runner’s resolver hangs (e.g., broken / slow DNS), one lookup can stall Promise.all(...) indefinitely, blocking prepublishOnly/CI even though the subsequent fetch() probe is time-bounded. Consider bounding the lookup and treating a timeout the same as “could not resolve” (return null) so the gate can continue and fetch() can emit its existing note on unreachable hosts.

  let addresses
  try {
    addresses = await lookup(h, { all: true })
  } catch {
    return null
  }

The fetch carries AbortSignal.timeout, the lookup carried nothing, so a resolver that hangs would stall Promise.all indefinitely and take CI with it. A lookup that cannot answer in five seconds tells us nothing anyway: it falls through to the fetch, which reports an unreachable host as a note rather than failing the gate.

msalvatti · 2026-08-01T19:12:05Z

Applied the suppressed comment on the unbounded lookup in 830e098 — correct, and it was the one asymmetry left: the fetch carries AbortSignal.timeout(10_000) and the lookup carried nothing, so a hanging resolver would stall Promise.all indefinitely and take CI with it. The lookup is now raced against a 5s bound (unrefed, so a pending timer never holds the process open), and a timeout is treated exactly like an unresolvable name: fall through to the fetch, which reports an unreachable host as a note rather than failing the gate.

Verified both paths still behave — the DNS rejection still fires (169.254.169.254.nip.io, 10.0.0.1.sslip.io) and a nonexistent host still degrades to a note, gate green.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

scripts/check-published-surface.mjs:93

privateAddressReason() treats only the single IPv4 address 0.0.0.0 as unspecified, and does not classify other addresses in 0.0.0.0/8 ("this network") or the IPv6 unspecified address ::. That means a hostname could resolve to e.g. 0.0.0.1 / :: and slip past the resolved-address guard even though those are non-routable/special-use addresses the gate should not probe.

    const [a, b] = [Number(v4[1]), Number(v4[2])]
    if (a === 127) return 'a loopback address'
    if (h === '0.0.0.0') return 'the unspecified address'
    if (a === 10 || (a === 172 && b >= 16 && b <= 31) || (a === 192 && b === 168)) {
      return 'a private address'

…0/8 and :: The link section claimed to verify every link in the README but collected only http(s) targets, so a relative link to a renamed file shipped unnoticed — the ones most likely to rot, and the cheapest to verify. Fragments are stripped before the path is tested. A tsx fence was never collected, so a React example importing the package was silently outside the gate. The fence language is kept now and decides the file extension, with jsx enabled in the throwaway consumer. privateAddressReason classified only the single address 0.0.0.0 and missed the rest of 0.0.0.0/8 as well as the IPv6 unspecified address. The lookup promise that loses the timeout race is caught, so a late rejection cannot surface as an unhandled rejection.

msalvatti · 2026-08-01T19:18:13Z

Applied the suppressed comment on privateAddressReason in cdb3371 — correct, it classified only the single address 0.0.0.0 and missed the rest of 0.0.0.0/8 ("this network") as well as the IPv6 unspecified address ::.

https://0.0.0.1/x   → is in the unspecified network (0.0.0.0/8)
https://[::]/x      → is the unspecified address
https://0.0.0.0/x   → is in the unspecified network (0.0.0.0/8)

Same commit carries three findings the review raised on the sibling PRs, since the script is shared byte-for-byte across the ten libraries:

Relative links were never checked. The section claimed to verify every link in the README but collected only http(s) targets, so a link to a renamed file shipped unnoticed — the ones most likely to rot and the cheapest to verify. Six of them in this README alone. Fragments are stripped before the path is tested; red-checked with a broken ./docs/… link and with ./CONTRIBUTING.md#setup, which correctly resolves to the file.
tsx fences were silently excluded, so a React example importing the package was outside the gate. The fence language now decides the extension, with jsx enabled in the throwaway consumer. Proven on nest-notification, which has a real React snippet: injecting a type error into that block now fails the gate, where before it compiled nothing.
The losing promise in the timeout race is caught, so a late DNS rejection cannot surface as an unhandled rejection.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

scripts/check-published-surface.mjs:211

Relative-link validation uses existsSync(join(ROOT, r)), but path.join() drops the prefix when r is absolute (e.g. /docs/x.md), and .. segments can escape the repo root (e.g. ../../etc/hostname). That can make the gate report a link as “existing in the repository” while actually probing the runner filesystem outside the checkout.

  for (const r of relative) {
    if (!existsSync(join(ROOT, r))) fail('links', `${r} does not exist in the repository`)
  }

A README target with .. segments resolved out of the repository and had the runner stat arbitrary paths, which matters because this gate runs on pull_request. Targets are resolved and confined first; a leading slash is read as repository-root relative, which is what a reader means by it. A link that escapes the repository is broken documentation either way — the file it names is not in the package.

msalvatti · 2026-08-01T19:23:33Z

Applied — the .. half is real, the stated mechanism is not. Fixed in ce1f3aa.

path.join does not discard its first argument for an absolute second one — that is path.resolve:

join('/repo', '/etc/passwd')      = /repo/etc/passwd
join('/repo', '../../etc/passwd') = /etc/passwd     ← the real escape
resolve('/repo', '/etc/passwd')   = /etc/passwd

So /etc/passwd was already being read as repository-root relative, which is what a reader means by a leading slash. The .. traversal was the actual hole, and it is closed: targets are resolved and confined to the repository before anything is stat-ed. A link that escapes the repository is broken documentation regardless — the file it names is not in the package.

[a](../../etc/passwd)      → resolves outside the repository
[b](/etc/passwd)           → does not exist in the repository
[c](./docs/nao-existe.md)  → does not exist in the repository
[d](./CONTRIBUTING.md)     → accepted
[e](/README.md)            → accepted

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

scripts/check-published-surface.mjs:160

The DNS preflight timeout uses Promise.race, but the losing lookup() promise is not cancelled and can keep the Node event loop alive if the resolver hangs. That undermines the “bounded” guarantee and can still stall CI even though the race resolves.

    // Bounded like the fetch below is. A resolver that hangs would otherwise
    // stall Promise.all indefinitely and take CI with it, and a lookup that
    // cannot answer in five seconds tells us nothing anyway. `unref` so a
    // pending timer never holds the process open.
    addresses = await Promise.race([

Copilot AI review requested due to automatic review settings August 1, 2026 18:55

Copilot started reviewing on behalf of msalvatti August 1, 2026 18:55 View session