Fix user-visible correctness bugs in sitemap, clean, retry and links - #13
Open
Hugoer wants to merge 7 commits into
Open
Fix user-visible correctness bugs in sitemap, clean, retry and links#13Hugoer wants to merge 7 commits into
Hugoer wants to merge 7 commits into
Conversation
The sitemaps protocol requires `&` to be escaped as `&` inside <loc>, so extractUrls returned a corrupted URL for every sitemap entry carrying a query string: `https://a.com/p?x=1&y=2` instead of `https://a.com/p?x=1&y=2`. The bad URL flowed straight into --urls-file for psi, lab and crux, which then audited a URL that does not exist. Decodes the five predefined entities plus decimal and hex character references in a single pass, so an escaped entity such as `&lt;` yields the literal `<` rather than being decoded twice. Unknown named entities and out-of-range character references are left as written rather than throwing, so a malformed entity in someone else's sitemap cannot abort the extraction.
runCleanCmd logged "Input not found" but returned an all-empty result, so the command printed "0 succeeded, 0 failed" and exited 0. A mistyped path in a script or CI step was indistinguishable from a run that legitimately had nothing to do. It now throws, which withCatch in bin/web-perf.js already maps to exit 1. Existence is checked directly instead of being inferred from an empty file list, so the two states stay distinct: a directory that exists but holds no .json files, and a glob that matches nothing, both remain empty result sets that exit 0.
The retryable check keyed entirely off err.statusCode, but global fetch rejects with a bare `TypeError: fetch failed` and hangs the real reason off `cause`, so a dropped connection or a DNS blip carried no statusCode at all. `undefined >= 500` is false, so the retry layer threw on the first attempt for the most common transient failure it exists for. In a long batch that meant dropping URLs a single retry would have saved. isRetryableError now walks the cause chain for an OS or undici error code when no status is present. ENOTFOUND and ECONNREFUSED are excluded on purpose: a hostname that does not resolve, or a port with nothing behind it, is nearly always a permanent mistake, and retrying one costs the full backoff on every URL of a batch that was never going to succeed. An explicit statusCode still wins over any code, since an answered request is not transient.
runLinks launched on '--headless' alone, so it was missing --no-sandbox and --disable-dev-shm-usage and failed in exactly the Docker and CI environments where lab works. It now reuses the CHROME_FLAGS array lab already exports, taking all eleven flags. No regression test: chrome-launcher is required at module scope and lib modules are loaded with require(), which bypasses the test runner's module graph, so vi.mock cannot intercept it. Covering this would need an injectable launcher seam like the one runLabPlan takes, which is a structural change and does not belong in a behaviour fix.
parseSitemaps followed <loc> to any host, so a sitemap index fetched from an untrusted domain could steer the crawler at http://localhost:... or a link-local metadata address, and any <loc>-shaped substring of the response landed in the output file. The scope is fixed by the URL the crawl started from and carried down the recursion unchanged, so a nested index cannot widen it one hop at a time. Off-origin children are logged and skipped rather than dropped silently. Behaviour change worth naming: origin comparison includes scheme, host and port, so an index that lists https://www.example.com children under https://example.com now skips them. That is out of spec — the sitemaps protocol scopes a sitemap to its own host and requires verification for cross-host submission — but it will be visible to anyone relying on it.
sitemap.js wrote straight to console.log and console.warn while every other module goes through lib/logger — lab.js goes as far as reporting through hooks to stay console-free. Progress lines become logger.info and the two failure paths become logger.warn. logger.warn supplies its own "Warning: " prefix, so the literal one is dropped from both messages rather than being printed twice.
Picks up the three new exports (decodeXmlEntities, partitionByOrigin, isRetryableError), the withRetry doc change, and the @throws on runCleanCmd. Also sweeps up one pre-existing drift: runBatch's onProgress already declared a statusCode argument in lib/utils.js at d55335e, but the checked-in .d.ts did not, so generate-types had not been run after that change landed.
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 #10.
Five correctness bugs found reviewing
mainat d55335e, one commit each, plus the loggersweep and regenerated types.
Behaviour changes
Sitemap URLs with entities are now decoded.
extractUrlsreturned the raw text between<loc>tags, so every sitemap entry carrying a query string came out corrupted:https://a.com/p?x=1&y=2instead ofhttps://a.com/p?x=1&y=2. The bad URL flowedstraight into
--urls-filefor psi, lab and crux, which then audited a URL that does notexist. Decoding covers the five predefined entities plus decimal and hex character
references, in a single pass so
&lt;yields the literal<rather than decodingtwice. Unknown entities and out-of-range references are left as written.
cleannow exits 1 on a missing input. It logged "Input not found" but returned anall-empty result, printed "0 succeeded, 0 failed" and exited 0 — a mistyped path was
indistinguishable from a legitimate no-op. Existence is checked directly rather than
inferred from an empty file list, so a directory that exists but holds no
.jsonfiles, anda glob matching nothing, both still exit 0.
withRetrynow retries network faults. The retryable check keyed entirely offerr.statusCode, but global fetch rejects with a bareTypeError: fetch failedand hangsthe reason off
cause, so a dropped connection or DNS blip carried no status and threw onthe first attempt.
isRetryableErrornow walks the cause chain for an OS or undici code.ENOTFOUND and ECONNREFUSED stay excluded on purpose: a hostname that does not resolve is
nearly always a permanent mistake, and retrying costs the full backoff on every URL of a
batch that was never going to succeed. An explicit status still wins over any code.
linksnow launches Chrome with the full flag set. It launched on--headlessalone,missing
--no-sandboxand--disable-dev-shm-usage, so it failed in exactly the Docker andCI environments where
labworks. It now reuses theCHROME_FLAGSarraylabexports.Sitemap index following is now scoped to the starting origin.
parseSitemapsfollowed<loc>to any host, so an index from an untrusted domain could steer the crawler athttp://localhost:...or a link-local metadata address and copy any<loc>-shapedsubstring of the response into the output file. The scope is fixed by the URL the crawl
started from and carried down unchanged, so a nested index cannot widen it one hop at a time.
Not a behaviour change
sitemap.jswrote straight toconsole.log/console.warnwhile every other module goesthrough
lib/logger. Routed through the logger; the literalWarning:prefixes weredropped because
logger.warnsupplies its own.Verification
npm run lint— 0 errors, the same 6 pre-existingmax-lenwarnings, no files modified.npm test— 544 passed, up from 506. New coverage for entity decoding, the missing-inputexit path, network retry classification, and origin partitioning.
npm run generate-types— committed. Also sweeps one pre-existing drift:runBatch'sonProgressalready declaredstatusCodein the source at d55335e but not in thechecked-in
.d.ts.169.254.169.254andevil.comchildren fetches neither, and?a=1&b=2arrives as?a=1&b=2.clean does-not-exist.jsonexits 1;clean <empty-dir>exits 0.Known gap
The
linksfix has no regression test.chrome-launcheris required at module scope and libmodules load through
require(), which bypasses the test runner's module graph, sovi.mockcannot intercept it. Covering it needs an injectable launcher seam like the onerunLabPlantakes — a structural change that belongs with #11, not in a behaviour fix.