fix(gate): refuse IP literals and non-https URLs before probing them - #49
Conversation
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. The dedicated check:published step is removed from release.yml. It packs nothing, so it lives in prepublishOnly and already ran there; the second run re-probed every README link for no new information.
There was a problem hiding this comment.
🟡 Not ready to approve
The new unroutable-host guard can be bypassed via trailing-dot loopback hostnames (e.g. localhost.), which should be blocked to meet the stated hardening goal.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR hardens the published-surface gate used during CI and prepublish checks by refusing non-https links and IP-literal hosts before any network probing, reducing the risk of CI runner SSRF-style probing from README links. It also improves cross-platform behavior for the consumer scaffold and removes a redundant release workflow step to reduce flaky network exposure during releases.
Changes:
- Add URL pre-validation to refuse IP literals (IPv4/IPv6) and non-
httpsURLs beforefetch()ing README links. - Make the consumer fixture symlink Windows-friendly (
junction) and improve Windows path handling for snippet diagnostics. - Remove redundant
check:publishedinvocation from the release workflow (already covered byprepublishOnly).
File summaries
| File | Description |
|---|---|
| scripts/check-published-surface.mjs | Adds unroutable URL screening before link probing; bounds git fetch time; improves Windows portability for symlinks and path parsing. |
| .github/workflows/release.yml | Removes redundant check:published step to reduce release-job duration and network flake risk. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
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.
There was a problem hiding this comment.
🟡 Not ready to approve
The new IPv6-literal detection is currently ineffective because URL.hostname does not include brackets for IPv6, allowing bracketed IPv6 URLs (e.g. https://[::1]/) to bypass the guard and still be fetched.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
…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.
There was a problem hiding this comment.
🟢 Ready to approve
The changes are narrowly scoped to CI gating/scripts and appear consistent with the stated hardening goals without introducing breaking behavior in the published library.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
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.
There was a problem hiding this comment.
🟡 Not ready to approve
The DNS timeout implementation risks unhandled promise rejections because the lookup() promise can reject after Promise.race() resolves.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
scripts/check-published-surface.mjs:154
Promise.race([lookup(), timeout])leaves thelookup()promise running after the timeout wins; if it later rejects, it can surface as an unhandled promise rejection. Attaching a catch to the lookup promise avoids that without changing the intended behavior (DNS failure/timeout still returns null and defers tofetch()).
addresses = await Promise.race([
lookup(h, { all: true }),
new Promise((resolve) => {
setTimeout(() => {
resolve(null)
}, 5_000).unref()
})
])
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
…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.
|
Applied the suppressed comment on the Same commit carries the findings raised on the sibling PRs, since the script is shared byte-for-byte: relative links in the README are now verified (they were never checked, despite the section claiming to check every link), |
There was a problem hiding this comment.
🟡 Not ready to approve
The new relative-link existence check can be coerced into probing arbitrary filesystem paths (absolute or ../ traversal) on the CI runner, which undermines the hardening goal.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
scripts/check-published-surface.mjs:211
- The new relative-link existence check can be abused by an untrusted PR: if the README contains an absolute path like
/etc/passwdthenjoin(ROOT, r)resolves to the filesystem root (becauseris absolute), causing the CI runner to probe for arbitrary local paths. Similarly,../segments allow probing outside the repo. Since this gate runs onpull_request, sanitize/normalize relative targets so they can only resolve within the repository (and treat leading/as repo-root relative, not OS-root).
for (const r of relative) {
if (!existsSync(join(ROOT, r))) fail('links', `${r} does not exist in the repository`)
}
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
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.
|
Applied — the
So |
There was a problem hiding this comment.
🟡 Not ready to approve
The new relative-link validation checks the repository filesystem rather than the published npm package surface, so it can miss broken links for consumers when targets aren’t included in package.json.files.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
scripts/check-published-surface.mjs:219
- Relative README links are currently validated against the repository working tree (
existsSync(target)), but this gate is meant to verify the published surface. Sincepackage.jsonwhitelists published files (files: ["dist", "LICENSE", "README.md", "CHANGELOG.md"]), links like./docs/mutation_testing_results.mdwill pass this check but be broken for npm consumers becausedocs/is not published. Consider validating relative link targets againstpackage.json#files(directory or exact file entries) after confirming they stay withinROOT.
const target = resolve(ROOT, r.replace(/^\/+/, ''))
if (target !== ROOT && !target.startsWith(ROOT + sep)) {
fail('links', `${r} resolves outside the repository`)
} else if (!existsSync(target)) {
fail('links', `${r} does not exist in the repository`)
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Hardening for
scripts/check-published-surface.mjs, found by Copilot while reviewing the same script innest-cache(bymaxone/nest-cache#42) and synced here byte-for-byte.Why
The gate runs on
pull_requestandfetch()es everyhttp(s)URL it finds in the README. Nothing stopped a fork from puttinghttps://169.254.169.254/latest/meta-dataor 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-httpsURLs 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:
Also in this change
'junction'rather than a'dir'symlink. A directory symlink needs elevation or Developer Mode on Windows, which would makeprepublishOnlyunrunnable there; Node ignores the type on POSIX.catcharound thefetchreturnsnull, 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. Its comment argued the step was kept because the gate packs nothing and so the nested-pack objection that excludescheck:exportsdoes not apply — true, but that is an argument for it living inprepublishOnly, which it does, and the step above already invokespnpm prepublishOnly. 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.No published artifact changes —
scripts/and.github/are outsidepackage.json→files, so no version bump.