Gate the preview server hardening with a regression test - #32
Merged
Conversation
Drives the real `quartz build --serve` over raw sockets and checks the two preview server issues fixed in #30: the listener bound every interface while the banner claimed localhost, and the redirect probes ran `fs.existsSync` on the un-normalized request path. Committed as-is before review so it is not left only in a working tree. Signed-off-by: Glenn Gore <glenn.g@affinidi.com>
Three corrections, all found by measuring the server with the #30 guards removed rather than reasoning about them. The raw client half-closed the socket with `socket.end(request)`. Node's server tears the connection down when it sees the FIN, so every response read back empty and every status compared `NaN`, which failed even with the fix in place. It writes without ending now and lets `Connection: close` finish the exchange. `/index` is a 301, not a 200 -- serve-handler redirects an explicit /index to /. Four of the six traversal targets passed with the guard removed, because serve-handler refuses them with a 400 of its own: `/../outside-absent/`, `/../outside-present`, `/../outside-absent` and a long `../` run at /etc/passwd. Asserting 400 on those pins serve-handler rather than the guard, so they are gone, with a comment recording why. What is left is the `/trailing/` form aimed at a file that exists outside the root, which answered 302 before the fix, and the existence oracle itself -- the present and absent siblings now answer identically. `/../` is asserted to still serve the site root, so a blanket reject of every path containing `..` cannot pass in place of normalizing. Scratch files move to a mkdtemp directory instead of `quartz/.quartz-cache`, so the test writes nothing inside the repository. Signed-off-by: Glenn Gore <glenn.g@affinidi.com>
The comment on the test step listed only the sanitize test. Both security regression tests now run there, so both are named. Signed-off-by: Glenn Gore <glenn.g@affinidi.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
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.
Targets
v4.#30 hardened the
quartz build --servepreview server: both sockets bind127.0.0.1by default, the banner reports the address actually bound, and arequest path that escapes the output directory is refused before any
fs.existsSynccall. Nothing tested any of it. This adds a test that runs thereal server and fails if any of that is undone.
quartz/cli/handlers.test.tsspawnsquartz build --serveagainst atwo-page fixture site in
quartz/cli/fixtures/serve, with no--host, so itexercises the default. It drives the server over raw TCP, because an HTTP client
collapses
..out of a request target before it ever leaves the process and thewhole question here is what the server does with a target that still has it.
It asserts:
127.0.0.1, and neither answers on::1or on any non-loopback IPv4 addressthis machine has.
listen(port)with no host binds::, which answers on::1as well as on every other interface, so a refusal on::1is what tellsthe two binds apart — checked only after confirming
::1works at all here,so a machine with IPv6 off cannot turn the probe into a free pass. The test
fails if no off-loopback address is available to probe, rather than passing
vacuously.
http://127.0.0.1:PORT, the addressthe socket is on, not the hardcoded
http://localhost:PORTprinted before./trailing/form aimed at a file that exists just outside the outputdirectory, and the same form reached through a legitimate-looking prefix
(
/nested/../../…/), both get a 400; and a present sibling and anever-created one now return byte-identical status lines, where before one was
a 302 and the other a 400.
/../normalizes back to the siteroot and is still served with a 200, as Harden the preview server, container image and deploy workflow #30 describes. Without this a change
that simply 400s every path containing
..would pass the traversalassertions while breaking ordinary requests.
/200,/index301,/nested/page200,/no-such-page404.Which inputs actually gate, and which only look like they do
The first draft of this test asserted a 400 on six traversal targets. Four of
them pass with #30's guard removed, because
serve-handlerrefuses them with a400 of its own —
/../outside-absent/,/../outside-present,/../outside-absent, and a long../run at/etc/passwd. Asserting 400 onthose pins
serve-handlerand not the guard the test is named for. They aregone, with a comment recording why, and the file keeps only the two forms the
guard alone can refuse.
The reason is worth stating, since it is not obvious from reading the handler:
path.posix.join(fp, "index.html")drops a leading..from an absolute path,so the
/trailing/index.htmlprobe never escapes. It ispath.posix.join(argv.output, base)in the same branch that does, because therethe
..sits after the root rather than at the start. So the escape needs the/trailing/form and a file that exists outside the root — which is exactlythe 302 that made it an oracle.
Proof that it fails when the fix is reverted
Each guard was reverted on its own, in a single shell invocation with a
traprestoring the file on exit, and the test run against it:
server.listen(argv.port)/WebSocketServer({ port }), banner kept302 !== 400, oracle test fails302 Found !== 400 Bad Request,/../fails400 !== 200; bind tests passhttp://localhost:PORTargs.jshostdefault back to0.0.0.0Each revert isolates one guard and fails only the assertions belonging to it,
so no assertion is passing for a neighbouring reason.
git diff v4on thetouched files is empty after each run; no revert is left in the branch.
Hermetic, and quick
Every address the test connects to is an address of the machine it runs on —
loopback, or one reported by
os.networkInterfaces()— asserted rather thanassumed, so a later edit that hardcodes an address trips the check. Nothing
reaches the network, with the fix in place or without it. That holds in the
reverted runs too, which is the case that matters: a wildcard bind makes the
server reachable on the host's own LAN address, and that address is the one
probed.
The server is spawned in its own process group and the group is
SIGKILLed inafter, so the build workers cannot outlive the run holding the ports. Portsare taken from the kernel rather than hardcoded. Scratch files live in a
mkdtempdirectory that teardown removes, so the test writes nothing inside therepository.
Whole suite: 78 tests, 1.9s wall (73 before, 5 new). The reverted runs are
the same, 1-2s each — a failing run does not hang or stall.
Verification
npm test: 78 tests, all passing.npx quartz buildsucceeds; 216 files emitted from 42 input files.tsc --noEmitandprettier --checkproduce exactly the same output as onv4, compared side by side against a cleanorigin/v4worktree: the onepre-existing
moduleResolution=node10deprecation and the same 44pre-existing Markdown formatting warnings.
npm run checkfails onv4today and fails identically here; unchanged by this branch.
Two things a reviewer should know
ci.yamlis not a required check, so nothing here blocks a merge yet. Thedefault branch
v4has no branch protection, and the only ruleset on it is theorganization's
DCOcheck. Thetestjob added in #29 runs on every pullrequest and reports, but a merge is not gated on it, so a red run does not stop
anything. The same gap makes the
.github/CODEOWNERSadded in #29 advisory:without a rule requiring code-owner review it only requests reviewers. Both need
a repository or organization setting that a pull request cannot make. Adding
testto the default branch's required checks is what turns this test into amerge gate.
--host 0.0.0.0is deliberately not tested. Asserting that the escape hatchreally does bind every interface means binding every interface in CI. The
loopback default and the banner are covered instead; the warning printed on a
non-loopback bind is not.
quartz/cli/handlers.jsis inherited fromjackyzha0/quartzand alreadydiverges from it after #30. This branch adds only a test file and fixtures, so
it adds no further divergence in the handler itself.