From f8349f048751fc7faef13c16e0d3c1b9ee03ae87 Mon Sep 17 00:00:00 2001 From: Glenn Gore Date: Sun, 13 Sep 2026 06:57:31 +0200 Subject: [PATCH 1/3] Add a preview server regression test (work in progress) 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 --- quartz/cli/fixtures/serve/index.md | 8 + quartz/cli/fixtures/serve/nested/page.md | 5 + quartz/cli/handlers.test.ts | 238 +++++++++++++++++++++++ 3 files changed, 251 insertions(+) create mode 100644 quartz/cli/fixtures/serve/index.md create mode 100644 quartz/cli/fixtures/serve/nested/page.md create mode 100644 quartz/cli/handlers.test.ts diff --git a/quartz/cli/fixtures/serve/index.md b/quartz/cli/fixtures/serve/index.md new file mode 100644 index 0000000..4873789 --- /dev/null +++ b/quartz/cli/fixtures/serve/index.md @@ -0,0 +1,8 @@ +--- +title: Preview server fixture +--- + +A minimal site for the preview server test. The pages only need to exist, so the +server has something real to answer with. + +[[nested/page|A nested page]] diff --git a/quartz/cli/fixtures/serve/nested/page.md b/quartz/cli/fixtures/serve/nested/page.md new file mode 100644 index 0000000..ee0e1de --- /dev/null +++ b/quartz/cli/fixtures/serve/nested/page.md @@ -0,0 +1,5 @@ +--- +title: Nested page +--- + +A nested page, so the test can request a path with a directory segment in it. diff --git a/quartz/cli/handlers.test.ts b/quartz/cli/handlers.test.ts new file mode 100644 index 0000000..c39262a --- /dev/null +++ b/quartz/cli/handlers.test.ts @@ -0,0 +1,238 @@ +import test, { after, before, describe } from "node:test" +import assert from "node:assert" +import { spawn, type ChildProcess } from "node:child_process" +import fs from "node:fs" +import net from "node:net" +import os from "node:os" +import path from "node:path" +import { fileURLToPath } from "node:url" + +// Runs the real `quartz build --serve` preview server and drives it over raw sockets, because an +// HTTP client collapses `..` out of a request target before it ever leaves the process and the +// point here is what the server does with a target that still has it. Covers the two preview +// server issues: the listener bound every interface while the banner claimed localhost, and the +// redirect probes ran `fs.existsSync` on the un-normalized request path, which answered whether a +// file outside the output directory existed. + +const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "..", "..") +const fixtureDir = path.join(repoRoot, "quartz", "cli", "fixtures", "serve") +const cacheDir = path.join(repoRoot, "quartz", ".quartz-cache") +const outputDir = path.join(cacheDir, "serve-test-output") + +// Siblings of the output directory, so `/..//` aims an existence probe at them from the +// web root. One exists and one does not: before the fix that difference was visible in the +// response, which is the oracle. +const presentOutside = path.join(cacheDir, "serve-test-present.html") +const absentName = "serve-test-absent" + +const ansi = /\x1b\[[\d;]*m/g + +function freePort(): Promise { + return new Promise((resolve, reject) => { + const probe = net.createServer() + probe.on("error", reject) + probe.listen(0, "127.0.0.1", () => { + const { port } = probe.address() as net.AddressInfo + probe.close(() => resolve(port)) + }) + }) +} + +// Whether a TCP connection to this address is answered. A timeout counts as unreachable: a +// filtered address is not serving either. +function connects(host: string, port: number): Promise { + return new Promise((resolve) => { + const socket = net.connect({ host, port }) + const settle = (reachable: boolean) => { + socket.destroy() + resolve(reachable) + } + socket.setTimeout(5_000) + socket.on("timeout", () => settle(false)) + socket.on("connect", () => settle(true)) + socket.on("error", () => settle(false)) + }) +} + +// Sends the request target verbatim and returns the whole response. +function request(port: number, target: string): Promise { + return new Promise((resolve, reject) => { + const socket = net.connect({ host: "127.0.0.1", port }) + let response = "" + socket.setTimeout(20_000) + socket.on("timeout", () => socket.destroy(new Error(`timed out requesting ${target}`))) + socket.on("connect", () => { + socket.end(`GET ${target} HTTP/1.1\r\nHost: 127.0.0.1:${port}\r\nConnection: close\r\n\r\n`) + }) + socket.on("data", (chunk) => (response += chunk)) + socket.on("error", reject) + socket.on("close", () => resolve(response)) + }) +} + +function statusLine(response: string): string { + return response.slice(0, response.indexOf("\r\n")) +} + +function status(response: string): number { + return Number(statusLine(response).split(" ")[1]) +} + +// Addresses this machine answers on that are not loopback. Anything reachable on one of these is +// reachable by anyone who can route to the machine. +function offLoopbackAddresses(): string[] { + return Object.values(os.networkInterfaces()) + .flat() + .filter((iface) => iface !== undefined && !iface.internal && iface.family === "IPv4") + .map((iface) => iface!.address) +} + +// `listen(port)` with no host binds `::`, which answers on `::1` and on every other interface; +// `listen(port, "127.0.0.1")` answers on 127.0.0.1 and nowhere else. So a refusal on `::1` is +// what tells the two apart -- but only where `::1` works at all, which this checks first, so a +// machine with IPv6 off cannot turn the probe into a free pass. +function ipv6LoopbackWorks(): Promise { + return new Promise((resolve) => { + const probe = net.createServer() + probe.on("error", () => resolve(false)) + probe.listen(0, "::1", async () => { + const { port } = probe.address() as net.AddressInfo + const reachable = await connects("::1", port) + probe.close(() => resolve(reachable)) + }) + }) +} + +describe("preview server", () => { + let server: ChildProcess + let port = 0 + let wsPort = 0 + let banner = "" + + before(async () => { + fs.rmSync(outputDir, { recursive: true, force: true }) + fs.mkdirSync(cacheDir, { recursive: true }) + fs.writeFileSync(presentOutside, "

not part of the site

") + fs.rmSync(path.join(cacheDir, `${absentName}.html`), { force: true }) + + port = await freePort() + wsPort = await freePort() + + // Its own process group, so teardown takes the build workers with it rather than leaving + // them holding the ports. + server = spawn( + process.execPath, + [ + "./quartz/bootstrap-cli.mjs", + "build", + "--serve", + "-d", + fixtureDir, + "-o", + outputDir, + "--port", + String(port), + "--wsPort", + String(wsPort), + ], + { cwd: repoRoot, detached: true, env: { ...process.env, NO_COLOR: "1", FORCE_COLOR: "0" } }, + ) + + banner = await new Promise((resolve, reject) => { + let output = "" + const timer = setTimeout( + () => reject(new Error(`preview server did not start:\n${output}`)), + 180_000, + ) + const done = (error: Error) => { + clearTimeout(timer) + reject(error) + } + const read = (chunk: Buffer) => { + output += chunk.toString().replace(ansi, "") + const started = output.match(/Started a Quartz server listening at (\S+)/) + if (started) { + clearTimeout(timer) + resolve(started[1]) + } + } + server.stdout?.on("data", read) + server.stderr?.on("data", read) + server.on("error", done) + server.on("exit", (code) => done(new Error(`preview server exited with ${code}:\n${output}`))) + }) + }) + + after(() => { + if (server?.pid !== undefined) { + try { + process.kill(-server.pid, "SIGKILL") + } catch { + server.kill("SIGKILL") + } + } + fs.rmSync(outputDir, { recursive: true, force: true }) + fs.rmSync(presentOutside, { force: true }) + }) + + test("serves the fixture site", async () => { + assert.strictEqual(status(await request(port, "/")), 200) + assert.strictEqual(status(await request(port, "/index")), 200) + assert.strictEqual(status(await request(port, "/nested/page")), 200) + assert.strictEqual(status(await request(port, "/no-such-page")), 404) + }) + + test("the banner reports the address the socket is bound to", () => { + assert.strictEqual(banner, `http://127.0.0.1:${port}`) + }) + + test("the server and the hot-reload socket answer on loopback only", async () => { + assert.ok(await connects("127.0.0.1", port), "the server does not answer on 127.0.0.1") + assert.ok( + await connects("127.0.0.1", wsPort), + "the hot-reload socket does not answer on 127.0.0.1", + ) + + const offLimits = offLoopbackAddresses() + if (await ipv6LoopbackWorks()) offLimits.push("::1") + assert.ok( + offLimits.length > 0, + "no address left to probe, so this cannot tell a loopback bind from a wildcard one", + ) + + for (const address of offLimits) { + assert.strictEqual( + await connects(address, port), + false, + `the server answers on ${address}, not just loopback`, + ) + assert.strictEqual( + await connects(address, wsPort), + false, + `the hot-reload socket answers on ${address}, not just loopback`, + ) + } + }) + + test("request paths that leave the output directory are refused", async () => { + const targets = [ + `/../${absentName}/`, + "/../serve-test-present/", + "/../serve-test-present", + "/nested/../../serve-test-present/", + "/../../../../../../etc/passwd", + ] + for (const target of targets) { + assert.strictEqual(status(await request(port, target)), 400, `${target} was not refused`) + } + }) + + test("a refused path does not reveal whether the file outside the root exists", async () => { + // Before the fix these two differed: 302 when the out-of-root file existed and 404 when it + // did not, which is the whole oracle. + const present = await request(port, "/../serve-test-present/") + const absent = await request(port, `/../${absentName}/`) + assert.strictEqual(statusLine(present), statusLine(absent)) + assert.doesNotMatch(present, /not part of the site/) + }) +}) From 9e415c7ad6fdf249e4ef3d8cf38a9262edea0695 Mon Sep 17 00:00:00 2001 From: Glenn Gore Date: Sun, 13 Sep 2026 07:02:12 +0200 Subject: [PATCH 2/3] Make the preview server test assert what the fix actually changed 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 --- quartz/cli/handlers.test.ts | 89 +++++++++++++++++++++++++------------ 1 file changed, 60 insertions(+), 29 deletions(-) diff --git a/quartz/cli/handlers.test.ts b/quartz/cli/handlers.test.ts index c39262a..0441c75 100644 --- a/quartz/cli/handlers.test.ts +++ b/quartz/cli/handlers.test.ts @@ -13,17 +13,21 @@ import { fileURLToPath } from "node:url" // server issues: the listener bound every interface while the banner claimed localhost, and the // redirect probes ran `fs.existsSync` on the un-normalized request path, which answered whether a // file outside the output directory existed. +// +// Every address this test connects to is an address of the machine it runs on -- loopback, or one +// reported by os.networkInterfaces(). Nothing here reaches the network, with the fix in place or +// without it. const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "..", "..") const fixtureDir = path.join(repoRoot, "quartz", "cli", "fixtures", "serve") -const cacheDir = path.join(repoRoot, "quartz", ".quartz-cache") -const outputDir = path.join(cacheDir, "serve-test-output") -// Siblings of the output directory, so `/..//` aims an existence probe at them from the -// web root. One exists and one does not: before the fix that difference was visible in the -// response, which is the oracle. -const presentOutside = path.join(cacheDir, "serve-test-present.html") -const absentName = "serve-test-absent" +// A scratch root holding the served directory plus two siblings of it, so `/..//` aims an +// existence probe out of the web root. One sibling exists and one never does: before the fix that +// difference was visible in the response, and that difference is the oracle. +const scratchRoot = fs.mkdtempSync(path.join(os.tmpdir(), "quartz-serve-test-")) +const outputDir = path.join(scratchRoot, "site") +const presentName = "outside-present" +const absentName = "outside-absent" const ansi = /\x1b\[[\d;]*m/g @@ -54,7 +58,10 @@ function connects(host: string, port: number): Promise { }) } -// Sends the request target verbatim and returns the whole response. +// Sends the request target verbatim and returns the whole response. Writes without ending the +// socket: a half-close makes Node's server tear the connection down before it replies, so +// `socket.end(request)` would read back an empty response for every target and the status +// assertions below would all compare NaN. `Connection: close` is what ends the exchange. function request(port: number, target: string): Promise { return new Promise((resolve, reject) => { const socket = net.connect({ host: "127.0.0.1", port }) @@ -62,7 +69,7 @@ function request(port: number, target: string): Promise { socket.setTimeout(20_000) socket.on("timeout", () => socket.destroy(new Error(`timed out requesting ${target}`))) socket.on("connect", () => { - socket.end(`GET ${target} HTTP/1.1\r\nHost: 127.0.0.1:${port}\r\nConnection: close\r\n\r\n`) + socket.write(`GET ${target} HTTP/1.1\r\nHost: 127.0.0.1:${port}\r\nConnection: close\r\n\r\n`) }) socket.on("data", (chunk) => (response += chunk)) socket.on("error", reject) @@ -110,14 +117,15 @@ describe("preview server", () => { let banner = "" before(async () => { - fs.rmSync(outputDir, { recursive: true, force: true }) - fs.mkdirSync(cacheDir, { recursive: true }) - fs.writeFileSync(presentOutside, "

not part of the site

") - fs.rmSync(path.join(cacheDir, `${absentName}.html`), { force: true }) + fs.mkdirSync(scratchRoot, { recursive: true }) + fs.writeFileSync(path.join(scratchRoot, `${presentName}.html`), "

not part of the site

") + fs.rmSync(path.join(scratchRoot, `${absentName}.html`), { force: true }) port = await freePort() wsPort = await freePort() + // No --host: the point is what the default does. + // // Its own process group, so teardown takes the build workers with it rather than leaving // them holding the ports. server = spawn( @@ -171,18 +179,26 @@ describe("preview server", () => { server.kill("SIGKILL") } } - fs.rmSync(outputDir, { recursive: true, force: true }) - fs.rmSync(presentOutside, { force: true }) + fs.rmSync(scratchRoot, { recursive: true, force: true }) }) test("serves the fixture site", async () => { assert.strictEqual(status(await request(port, "/")), 200) - assert.strictEqual(status(await request(port, "/index")), 200) + // serve-handler redirects an explicit /index to / + assert.strictEqual(status(await request(port, "/index")), 301) assert.strictEqual(status(await request(port, "/nested/page")), 200) assert.strictEqual(status(await request(port, "/no-such-page")), 404) + + // `/../` normalizes back to the site root, so it is served rather than refused. Asserted + // because it is the difference between normalizing the request path and rejecting every + // path that merely contains `..`: a blanket reject would pass the traversal tests below + // while breaking this. + assert.strictEqual(status(await request(port, "/../")), 200) }) test("the banner reports the address the socket is bound to", () => { + // Was a hardcoded `http://localhost:PORT` printed before listen() resolved, which said + // loopback while the socket was on every interface. assert.strictEqual(banner, `http://127.0.0.1:${port}`) }) @@ -200,6 +216,18 @@ describe("preview server", () => { "no address left to probe, so this cannot tell a loopback bind from a wildcard one", ) + // Keeps the probes on this machine even if the list above is ever edited. + const ownAddresses = new Set([ + ...Object.values(os.networkInterfaces()) + .flat() + .map((iface) => iface?.address), + "127.0.0.1", + "::1", + ]) + for (const address of offLimits) { + assert.ok(ownAddresses.has(address), `${address} is not an address of this machine`) + } + for (const address of offLimits) { assert.strictEqual( await connects(address, port), @@ -214,23 +242,26 @@ describe("preview server", () => { } }) - test("request paths that leave the output directory are refused", async () => { - const targets = [ - `/../${absentName}/`, - "/../serve-test-present/", - "/../serve-test-present", - "/nested/../../serve-test-present/", - "/../../../../../../etc/passwd", - ] - for (const target of targets) { + // The targets below are the ones the guard alone can refuse. `path.posix.join(fp, "index.html")` + // drops a leading `..` from an absolute path, but `path.posix.join(argv.output, base)` does not, + // so it is the `/trailing/` branch's `base` probe that escapes -- and when the file it lands on + // exists, the un-normalized server answered 302. Without the guard these return 302; with it, + // 400. + // + // Deliberately not asserted: `/../outside-absent/`, `/../outside-present`, `/../outside-absent` + // and `/../../../../../../etc/passwd`. serve-handler refuses all four with a 400 of its own, so + // asserting 400 on them pins serve-handler and not this guard -- they pass with the guard + // removed. Measured, not assumed. + test("request paths that escape the output directory are refused", async () => { + for (const target of [`/../${presentName}/`, `/nested/../../${presentName}/`]) { assert.strictEqual(status(await request(port, target)), 400, `${target} was not refused`) } }) - test("a refused path does not reveal whether the file outside the root exists", async () => { - // Before the fix these two differed: 302 when the out-of-root file existed and 404 when it - // did not, which is the whole oracle. - const present = await request(port, "/../serve-test-present/") + test("a refused path does not reveal whether a file outside the root exists", async () => { + // This is the oracle: without the guard the existing sibling answered 302 and the missing one + // 400, so the status told the caller which files outside the site were there. + const present = await request(port, `/../${presentName}/`) const absent = await request(port, `/../${absentName}/`) assert.strictEqual(statusLine(present), statusLine(absent)) assert.doesNotMatch(present, /not part of the site/) From df2c2ef52b0094aa6b3033fc4f67293569864bf8 Mon Sep 17 00:00:00 2001 From: Glenn Gore Date: Sun, 13 Sep 2026 07:05:15 +0200 Subject: [PATCH 3/3] Name the preview server test in the CI workflow comment 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 --- .github/workflows/ci.yaml | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 859faa2..53666e0 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -21,8 +21,13 @@ jobs: node-version: 22 - name: Install dependencies run: npm ci - # Includes quartz/util/sanitize.test.ts, which builds the fixture pages in - # quartz/util/fixtures/sanitize and fails if any of their payloads reach the - # emitted HTML. + # Includes the two security regression tests: + # quartz/util/sanitize.test.ts builds the fixture pages in + # quartz/util/fixtures/sanitize and fails if any of their payloads reach + # the emitted HTML. + # quartz/cli/handlers.test.ts runs the preview server against the fixture + # site in quartz/cli/fixtures/serve and fails if it binds anything other + # than loopback, or if a request path that escapes the output directory is + # answered instead of refused. - name: Run tests run: npm test