From 76a6fd0fab09643e21c9bf7abd0163c99a5ea08d Mon Sep 17 00:00:00 2001 From: Melvin Carvalho Date: Thu, 11 Jun 2026 09:12:41 +0200 Subject: [PATCH 1/2] fix: wire --body-limit into createServer; real CONTENT_LENGTH for chunked git pushes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two halves of the same user-facing bug (#561) — 'git push of a pack over 10MB cannot succeed no matter what the operator configures': 1. bin/jss.js start parsed --body-limit / JSS_BODY_LIMIT into config.bodyLimit and then dropped it — the hand-written options object passed to createServer() omitted bodyLimit, so every CLI-started server enforced the 10MiB default. The #474 knob was dead wiring; only programmatic createServer({ bodyLimit }) worked. 2. With the limit actually raised, pushes > 1MiB still died: git sends packs over http.postBuffer (1MiB) with Transfer-Encoding: chunked and no Content-Length, and the git handler set the CGI's CONTENT_LENGTH from the absent header ('0'), so http-backend read zero bytes and receive-pack failed with 'the remote end hung up unexpectedly'. Use the buffered body's real length instead. Regression tests (test/body-limit-cli.test.js) spawn the real CLI and assert the limit reaches Fastify via flag and env — 2 of 3 fail without fix 1. Verified end-to-end: 'jss install solid-chat/app=chat JavaScriptSolidServer/git' (13.6MB / >1MiB packs, previously impossible) now completes 2/2 with extracted working trees serving 200. Full suite: 961 pass, 0 fail. Fixes #561 --- bin/jss.js | 4 ++ src/handlers/git.js | 10 ++- test/body-limit-cli.test.js | 124 ++++++++++++++++++++++++++++++++++++ 3 files changed, 137 insertions(+), 1 deletion(-) create mode 100644 test/body-limit-cli.test.js diff --git a/bin/jss.js b/bin/jss.js index 286fd57e..a4c3ef0e 100755 --- a/bin/jss.js +++ b/bin/jss.js @@ -190,6 +190,10 @@ program const server = createServer({ port: config.port, host: config.host, + // Wire the parsed --body-limit / JSS_BODY_LIMIT value through — + // omitting it here silently pinned every CLI-started server to + // the 10MB default and made the #474 knob dead wiring (#561). + bodyLimit: config.bodyLimit, logger: config.logger, conneg: config.conneg, notifications: config.notifications, diff --git a/src/handlers/git.js b/src/handlers/git.js index 03fbb468..642b7a35 100644 --- a/src/handlers/git.js +++ b/src/handlers/git.js @@ -279,7 +279,15 @@ export async function handleGit(request, reply) { CONTENT_TYPE: request.headers['content-type'] || '', QUERY_STRING: queryString, REMOTE_USER: request.webId || '', // Pass authenticated user - CONTENT_LENGTH: request.headers['content-length'] || '0', + // Use the buffered body's real length, not the Content-Length header: + // git sends packs larger than http.postBuffer (1 MiB default) with + // Transfer-Encoding: chunked and NO Content-Length, so the header + // fallback told http-backend "0 bytes" and receive-pack died with + // "the remote end hung up unexpectedly" on any push > 1 MiB (#561). + // Fastify has already buffered the full body either way. + CONTENT_LENGTH: request.body?.length + ? String(request.body.length) + : (request.headers['content-length'] || '0'), }; // For regular repositories, set GIT_DIR diff --git a/test/body-limit-cli.test.js b/test/body-limit-cli.test.js new file mode 100644 index 00000000..df292488 --- /dev/null +++ b/test/body-limit-cli.test.js @@ -0,0 +1,124 @@ +/** + * CLI wiring for --body-limit / JSS_BODY_LIMIT (#561). + * + * test/body-limit.test.js covers createServer({ bodyLimit }) — the + * programmatic path — but the CLI start command builds its createServer + * options object by hand, and originally omitted `bodyLimit`. Result: + * the #474 knob parsed fine and was then silently dropped, pinning every + * CLI-started server to the 10 MiB default. git pushes of packs over the + * default failed with 413 no matter what the operator passed (#561). + * + * These tests spawn `bin/jss.js start` as a real subprocess (same + * pattern as cli-flag-like-values.test.js) and assert the limit + * actually reaches Fastify: + * + * - `--body-limit 1KB` → a 2 KiB PUT is rejected with 413 + * - `--body-limit 5MB` → the same PUT is NOT rejected with 413 + * - `JSS_BODY_LIMIT=1KB` (env, no flag) → 413, same as the flag + * + * The over-limit assertions are deliberately status-exact (413) while + * the under-limit assertion is only "not 413": body parsing runs before + * auth/WAC, so an under-limit request may still be refused later in the + * pipeline — that's fine, the wiring under test is the parser cap. + */ + +import { describe, it, afterEach } from 'node:test'; +import assert from 'node:assert'; +import { spawn } from 'node:child_process'; +import { createServer as createNetServer } from 'node:net'; +import { fileURLToPath } from 'node:url'; +import path from 'node:path'; +import fs from 'fs-extra'; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const BIN = path.join(__dirname, '..', 'bin', 'jss.js'); +const TEST_DATA_DIR = './test-data-body-limit-cli'; + +let child; + +function freePort() { + return new Promise((resolve, reject) => { + const srv = createNetServer(); + srv.once('error', reject); + srv.listen(0, '127.0.0.1', () => { + const { port } = srv.address(); + srv.close(() => resolve(port)); + }); + }); +} + +async function startCli({ args = [], env = {} }) { + await fs.emptyDir(TEST_DATA_DIR); + const port = await freePort(); + child = spawn(process.execPath, [ + BIN, 'start', + '--port', String(port), + '--host', '127.0.0.1', + '--root', TEST_DATA_DIR, + ...args, + ], { + env: { ...process.env, ...env }, + stdio: ['ignore', 'pipe', 'pipe'], + }); + const baseUrl = `http://127.0.0.1:${port}`; + + // Poll until the server answers (or the child dies / 15 s passes). + const deadline = Date.now() + 15_000; + let exited = false; + child.once('exit', () => { exited = true; }); + while (Date.now() < deadline) { + if (exited) throw new Error('jss CLI exited before becoming ready'); + try { + await fetch(baseUrl, { signal: AbortSignal.timeout(1000) }); + return baseUrl; + } catch { + await new Promise(r => setTimeout(r, 250)); + } + } + throw new Error('jss CLI did not become ready within 15s'); +} + +async function stopCli() { + if (!child) return; + const c = child; + child = null; + if (c.exitCode === null && c.signalCode === null) { + const gone = new Promise(r => c.once('exit', r)); + c.kill('SIGTERM'); + await Promise.race([gone, new Promise(r => setTimeout(r, 3000))]); + if (c.exitCode === null && c.signalCode === null) c.kill('SIGKILL'); + await gone; + } +} + +async function putTwoKiB(baseUrl) { + const res = await fetch(`${baseUrl}/body-limit-probe.bin`, { + method: 'PUT', + headers: { 'Content-Type': 'application/octet-stream' }, + body: Buffer.alloc(2048, 0x61), + signal: AbortSignal.timeout(5000), + }); + return res.status; +} + +describe('bin/jss.js — --body-limit reaches Fastify (#561)', () => { + afterEach(async () => { + await stopCli(); + await fs.remove(TEST_DATA_DIR); + }); + + it('--body-limit 1KB rejects a 2KiB PUT with 413', async () => { + const baseUrl = await startCli({ args: ['--body-limit', '1KB'] }); + assert.strictEqual(await putTwoKiB(baseUrl), 413); + }); + + it('--body-limit 5MB does not 413 a 2KiB PUT', async () => { + const baseUrl = await startCli({ args: ['--body-limit', '5MB'] }); + assert.notStrictEqual(await putTwoKiB(baseUrl), 413); + }); + + it('JSS_BODY_LIMIT=1KB (env, no flag) rejects a 2KiB PUT with 413', async () => { + const baseUrl = await startCli({ env: { JSS_BODY_LIMIT: '1KB' } }); + assert.strictEqual(await putTwoKiB(baseUrl), 413); + }); +}); From b17c30a23aaad3e414b510ae3025d15ac47f5a7e Mon Sep 17 00:00:00 2001 From: Melvin Carvalho Date: Fri, 12 Jun 2026 01:07:12 +0200 Subject: [PATCH 2/2] review fix (#562): bound stopCli's final await; document why the attach can't race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot flagged a race between the exitCode check and attaching the 'exit' listener. That specific interleaving can't occur — both are in the same synchronous block, and Node can't emit 'exit' between two synchronous statements (if the event already fired, exitCode is non-null and we return before attaching). Documented in a comment. The comment's spirit stands though: the final `await gone` was unbounded, so a pathological unkillable child (stuck in uninterruptible I/O) would hang the suite forever. Capped with a 2s race — SIGKILL can't be caught, so 'exit' follows promptly in every realistic case; the bound is belt-and-braces. 3/3 passing. --- test/body-limit-cli.test.js | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/test/body-limit-cli.test.js b/test/body-limit-cli.test.js index df292488..13bf95cc 100644 --- a/test/body-limit-cli.test.js +++ b/test/body-limit-cli.test.js @@ -82,13 +82,19 @@ async function stopCli() { if (!child) return; const c = child; child = null; - if (c.exitCode === null && c.signalCode === null) { - const gone = new Promise(r => c.once('exit', r)); - c.kill('SIGTERM'); - await Promise.race([gone, new Promise(r => setTimeout(r, 3000))]); - if (c.exitCode === null && c.signalCode === null) c.kill('SIGKILL'); - await gone; - } + // The exitCode check and the once('exit') attach below are in the + // same synchronous block — Node can't emit 'exit' between two + // synchronous statements, so the listener can't miss the event (if + // it already fired, exitCode is non-null and we return here). + if (c.exitCode !== null || c.signalCode !== null) return; + const gone = new Promise(r => c.once('exit', r)); + c.kill('SIGTERM'); + await Promise.race([gone, new Promise(r => setTimeout(r, 3000))]); + if (c.exitCode === null && c.signalCode === null) c.kill('SIGKILL'); + // SIGKILL can't be caught, so 'exit' follows promptly. The cap is a + // belt-and-braces bound so the suite can never hang on a pathological + // unkillable child (e.g. stuck in uninterruptible I/O). + await Promise.race([gone, new Promise(r => setTimeout(r, 2000))]); } async function putTwoKiB(baseUrl) {