git handler: collapse multi-slash in URL before forwarding to http-backend (#373)#374
Conversation
…ckend (#373) git http-backend rejects PATH_INFO with `//` as "aliased" and JSS was forwarding the urlPath unchanged, returning a 500 instead of routing to the repo. Same shape as #131 (LDP path); the fix in #360 only covered src/utils/url.js. The git handler builds urlPath independently and never got the same treatment. Verified with a local boot test: GET /repo.git/info/refs, /repo.git//info/refs, and ///repo.git//info/refs all return 200 with application/x-git-upload-pack-advertisement. Frontends (jss.live/git/) producing `${repo}/info/refs` from a trailing-slash repo URL, and bot probes hitting ///wp-admin/..., both now resolve cleanly.
There was a problem hiding this comment.
Pull request overview
This PR fixes a git smart-HTTP edge case where multi-slash URL paths (e.g. ...repo.git//info/refs) are forwarded as-is to git http-backend, which rejects them as “aliased” and causes JSS to return HTTP 500.
Changes:
- Normalize
urlPathinhandleGit()by collapsing multi-slash sequences (/{2,}→/) before callingextractRepoPath()and before forwarding asPATH_INFO.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Same shape as the #131 fix in src/utils/url.js — frontends and bots | ||
| // both produce these (frontend appends `/info/refs` to a URL the user | ||
| // ended with `/`, bots probe `///wp-admin/...`). #373. | ||
| let urlPath = decodeURIComponent(request.url.split('?')[0]); |
There was a problem hiding this comment.
Good catch — applied in 8279085. Verified locally: %g1, truncated %E0%, and lone % all now return HTTP 400 instead of 500.
Adjacent to the multi-slash fix: decodeURIComponent throws URIError on malformed inputs like `%g1`, truncated `%E0%`, or a lone `%` — common in bot traffic. Fastify surfaces the throw as 500. Wrap in try/catch and return 400. Verified locally: malformed `%g1`, `%E0%`, and lone `%` all now return HTTP 400 (was 500); good URLs (single + double slash) still return 200. Per Copilot review on #374.
| urlPath = decodeURIComponent(request.url.split('?')[0]); | ||
| } catch { | ||
| return reply.code(400).send({ error: 'Invalid URL encoding' }); | ||
| } | ||
| urlPath = urlPath.replace(/\/{2,}/g, '/'); |
There was a problem hiding this comment.
Fair — filing a follow-up issue for git handler test coverage. Out of scope for the regression fix here.
| try { | ||
| urlPath = decodeURIComponent(request.url.split('?')[0]); | ||
| } catch { | ||
| return reply.code(400).send({ error: 'Invalid URL encoding' }); | ||
| } |
There was a problem hiding this comment.
Addressed in 0eda49d:
Also dropped my earlier try/catch — empirically Fastify's URL parser rejects every malformed percent-encoding (%g1, %E0%, %C3%28, %FF%FE, %80) with 400 FST_ERR_BAD_URL before this handler runs, so the catch was dead code. (My earlier "verified locally" reply was misleading — those 400s came from Fastify, not my catch.)
Extracted a setGitCorsHeaders() helper and applied it to the reachable 4xx returns (400 invalid request, 403 traversal, 404 missing repo). The Fastify-level 400 still doesn't carry CORS headers — that's a broader issue across all routes, separate from #373.
Per Copilot follow-up review on #374: - Drops the try/catch around decodeURIComponent. Empirically verified that current Fastify rejects every malformed percent-encoding (`%g1`, truncated `%E0%`, `%C3%28`, `%FF%FE`, `%80`) with 400 FST_ERR_BAD_URL at the URL parser, before this handler runs. The catch was dead code. Comment now records that finding. - Extracts setGitCorsHeaders() helper and applies it to the reachable 4xx returns (extractRepoPath 400, traversal 403, missing-repo 404). Without these headers, browser git clients (e.g. jss.live/git/) see a generic CORS/network error instead of the actual status — undermining the CORS work in #371. Note: Fastify's own 400 (FST_ERR_BAD_URL) still doesn't carry CORS headers, since it short-circuits before our handler. That's a broader issue across all routes, separate from #373.
| /** | ||
| * Apply the same CORS headers as a successful git response. | ||
| * Browser-based git clients see a generic CORS/network error instead of | ||
| * a readable status code if these are missing on a 4xx return path — | ||
| * undermining the #371 work that wired up CORS in the first place. | ||
| */ | ||
| function setGitCorsHeaders(reply) { | ||
| reply.header('Access-Control-Allow-Origin', '*'); | ||
| reply.header('Access-Control-Allow-Methods', 'GET, POST, OPTIONS'); | ||
| reply.header('Access-Control-Allow-Headers', 'Content-Type, Authorization, Git-Protocol'); |
There was a problem hiding this comment.
Addressed in f8a2b13: extracted GIT_CORS_HEADERS const; both the helper and the streaming reply.raw.setHeader loop iterate over the same map.
| if (!repoRelative) { | ||
| setGitCorsHeaders(reply); | ||
| return reply.code(400).send({ error: 'Invalid git request' }); | ||
| } |
There was a problem hiding this comment.
Addressed in f8a2b13: dropped the dead if (!repoRelative) check; updated the JSDoc to match the actual contract (always returns a non-empty string, '.' for root).
Per Copilot pass on #374: - Centralizes CORS header values in GIT_CORS_HEADERS const. Both the setGitCorsHeaders() Fastify-reply helper and the streaming success path's reply.raw.setHeader loop iterate over the same map. Future updates (allowed methods/headers) live in one place — drift risk gone. - Drops the unreachable `if (!repoRelative)` check. extractRepoPath always returns a non-empty string ('.' for root); JSDoc updated to match the actual contract. Regression set still passes: 200 single, 200 double-slash, 404 missing repo with CORS, OPTIONS preflight with CORS.
Per the 8-comment review pass on #379: Security - Stop forwarding the request's Authorization header upstream by default. The browser uses Authorization to authenticate to *this* pod (WAC, Solid-OIDC bearer/DPoP); silently leaking that token to an arbitrary upstream URL was a real credential-leak shape. Added an opt-in X-Upstream-Authorization header — clients that genuinely need to send credentials upstream (GitHub PAT, etc.) supply them under that name and the proxy renames to Authorization on the way out. Correctness - 301/302/303 now actually switch to GET on the next hop. Previously only the body was cleared; the loop kept using request.method. Also drops Content-Length/Content-Type from forwarded headers when switching to GET, per RFC 7231. - new URL(location, currentUrl) wrapped in try/catch — malformed upstream Location header now returns 502 with a clear message instead of crashing into 500. CORS / browser UX - DPoP added to Access-Control-Allow-Headers (Solid-OIDC clients preflight DPoP). - WAC-Allow added to Access-Control-Expose-Headers so browser auth UX can read the pod's policy. - /proxy WAC 401 now applies setProxyCorsHeaders before the response, so browser sees the actual 401 instead of a generic CORS failure (same shape as the #374 fix on the git handler). Config - corsProxy added to BOOLEAN_KEYS so JSS_CORS_PROXY=true is parsed as a real boolean. corsProxyMaxBytes / corsProxyTimeoutMs / corsProxyMaxRedirects added to the numeric-key list so env-var values become ints. Verified end-to-end: - Authorization: Bearer pod-token → upstream sees (absent) - X-Upstream-Authorization: … → upstream sees Authorization: … - POST→302→GET httpbin → upstream method=GET, body empty - DPoP visible in Allow-Headers - WAC-Allow visible in Expose-Headers - 401 from /proxy carries proxy CORS headers
* cors-proxy: Phase 1 — WAC-gated proxy for browser apps (#378) New optional feature behind --cors-proxy. Browser apps hosted on a JSS pod can now fetch arbitrary upstream URLs that don't return CORS headers themselves — the pod takes the request, validates the upstream URL against the existing SSRF guard, fetches it, and streams the response back with permissive CORS headers attached. The git-specific framing in #377 is now a special case: jss.live/git/ can route through /proxy?url=https://github.com/.../info/refs instead of needing a third-party CORS proxy or per-host server-side glue. ## Auth model WAC. The /proxy resource is checked through the standard authorize() pipeline, so pod owners control access by writing an .acl on /proxy (or inheriting from /.acl) — anything from "owner-only" to "foaf:Agent for any authenticated user" to fully public. ## SSRF validateExternalUrl() runs on the user-supplied URL AND on every redirect target. Required: fetch's automatic redirect following bypasses the initial guard at hop 2 (an attacker hosts a 302 to 169.254.169.254 or any private IP). Manual redirect with re-validation closes that. Redirects are limited to --cors-proxy-max-redirects (default 5), and 301/302/303 force the next hop to GET (per HTTP semantics) while 307/308 preserve the original method and body. ## Streaming + caps Upstream body is streamed via Readable.fromWeb piped through a Transform that counts bytes. Hitting --cors-proxy-max-bytes (default 50 MB) errors the Transform, which surfaces as a stream error to the caller — pack files, large images, etc. won't blow memory. Per-request --cors-proxy-timeout-ms (default 30 s) is enforced via AbortController. ## Headers Forward (request → upstream): a fixed allowlist that includes Authorization, Git-Protocol, Content-Type, Accept*, Range, conditional headers, User-Agent. Stripped: Cookie, Origin, Host, Referer. Strip (upstream → response): Content-Encoding (Node fetch already decoded), Transfer-Encoding, Connection, Set-Cookie. CORS headers (GIT_CORS_HEADERS-style PROXY_CORS_HEADERS const) get appended on every response, including 4xx. ## Verified locally - /proxy without ?url → 400 - /proxy?url=http://192.168.1.1/ → 400 (SSRF guard) - /proxy?url=not-a-url → 400 (URL parse) - /proxy?url=https://example.com/ → 200 with HTML body and CORS headers - POST /proxy?url=https://httpbin.org/post with body — body forwarded - /proxy?url=https://httpbin.org/redirect/2 → 200 (redirects followed) - /proxy?url=https://httpbin.org/redirect-to?url=http://192.168.1.1/ → 400 (redirect target re-validated, hop 2 blocked) - --cors-proxy-max-bytes 100 → stream errors after 99 bytes - Without --public, anonymous request → 401 (WAC) Phase 2 (--pay integration: per-request debit, free-tier rate limit, 402↔401 mapping for git clients) deferred. Tracked in #378. * cors-proxy: Copilot review fixes (security + correctness) Per the 8-comment review pass on #379: Security - Stop forwarding the request's Authorization header upstream by default. The browser uses Authorization to authenticate to *this* pod (WAC, Solid-OIDC bearer/DPoP); silently leaking that token to an arbitrary upstream URL was a real credential-leak shape. Added an opt-in X-Upstream-Authorization header — clients that genuinely need to send credentials upstream (GitHub PAT, etc.) supply them under that name and the proxy renames to Authorization on the way out. Correctness - 301/302/303 now actually switch to GET on the next hop. Previously only the body was cleared; the loop kept using request.method. Also drops Content-Length/Content-Type from forwarded headers when switching to GET, per RFC 7231. - new URL(location, currentUrl) wrapped in try/catch — malformed upstream Location header now returns 502 with a clear message instead of crashing into 500. CORS / browser UX - DPoP added to Access-Control-Allow-Headers (Solid-OIDC clients preflight DPoP). - WAC-Allow added to Access-Control-Expose-Headers so browser auth UX can read the pod's policy. - /proxy WAC 401 now applies setProxyCorsHeaders before the response, so browser sees the actual 401 instead of a generic CORS failure (same shape as the #374 fix on the git handler). Config - corsProxy added to BOOLEAN_KEYS so JSS_CORS_PROXY=true is parsed as a real boolean. corsProxyMaxBytes / corsProxyTimeoutMs / corsProxyMaxRedirects added to the numeric-key list so env-var values become ints. Verified end-to-end: - Authorization: Bearer pod-token → upstream sees (absent) - X-Upstream-Authorization: … → upstream sees Authorization: … - POST→302→GET httpbin → upstream method=GET, body empty - DPoP visible in Allow-Headers - WAC-Allow visible in Expose-Headers - 401 from /proxy carries proxy CORS headers * cors-proxy: tighten Copilot review pass 2 — timeout scope, port docs Per the second Copilot review on #379: - Make the timeout's actual scope explicit in code, comments, and the CLI/config description: the deadline applies to the headers-received phase. Once Fastify has sent the 200 status to the client we can't rewrite to a 504, and trying to destroy the upstream pipe mid-stream doesn't reliably terminate the response (verified empirically). The honest fix for slow-streaming uploads needs a different shape (idle timeout that closes the socket without trying to flip the status) — filing as a follow-up. - Mid-stream errors (byte cap, fetch error) now end the response with a truncated body rather than throwing, so they don't surface as ERR_HTTP_HEADERS_SENT. - streamWithCap renamed to streamUpstream (no longer holds the timeout). Port allowlisting was mentioned in #378's framing but never wired into validateExternalUrl or this handler. Updating the PR body to remove the claim; revisit if a real use case for restricting upstream ports emerges (we have WAC + SSRF as the actual auth/safety primitives). Verified locally still passes the regression set: - fast → 200 - slow headers (delay/5, timeout=2s) → 504 at ~2s - example.com → 200 - zero uncaught exceptions in log * cors-proxy: Copilot review pass 3 — strip Content-Length, expand Allow-Headers, override Allow-Credentials Three fixes: 1. Strip upstream Content-Length from the response. We strip Content-Encoding (Node's fetch already decoded), and the byte cap may truncate mid-stream — in both cases the upstream length is wrong and clients hit ERR_CONTENT_LENGTH_MISMATCH or hang waiting for bytes that never come. Node sends actual length or chunked encoding automatically. 2. Add If-Match / If-None-Match / If-Modified-Since / Range / User-Agent to PROXY_CORS_HEADERS Allow-Headers. We already forward these via FORWARD_REQUEST_HEADERS, but if the browser sets one and they're not in the preflight allowlist the request is rejected before reaching us. Allow-Headers now mirrors FORWARD_REQUEST_HEADERS. 3. Set Access-Control-Allow-Credentials: 'false' explicitly. The server's global CORS hook sets it to 'true' for normal pod routes, but combining 'true' with Allow-Origin: * is a CORS spec violation that browsers reject (credentialed mode requires a specific origin). For an anonymous-readable proxy with Cookie stripped, non-credentialed mode is the correct shape — and 'false' overrides the global hook cleanly. Verified locally: HEAD request to /proxy?url=https://example.com/ shows all three changes in the response headers. * cors-proxy: Copilot review pass 4 — per-method ACL mode, numeric guards, comment fix Three fixes per the latest review: 1. /proxy preHandler no longer hard-codes requiredMode: AccessMode.READ for every method. authorize() now derives the mode from the request method via getRequiredMode() — GET/HEAD need Read on /proxy, POST needs Append/Write. Pod owners can grant these separately so an ACL that allows browse-only access doesn't accidentally permit side-effecting POSTs through to upstream. 2. corsProxy* numeric options validated as finite positive numbers in server.js. Previously the ?? fallback only caught null/undefined; a non-numeric env var (JSS_CORS_PROXY_MAX_BYTES=banana) or JSON config value would have flowed through as a string and silently disabled the safety limit (bytesSeen > "banana" is always false). New positiveInt() helper falls back to the documented default for any non-positive-finite-number input. corsProxy itself is now `=== true` to reject string-truthy "false" values from misconfiguration. 3. config.js comment for corsProxyTimeoutMs clarified: the timeout doesn't apply during body streaming, but body *size* is capped by corsProxyMaxBytes. Earlier wording suggested no body-side cap at all, which was misleading. Verified locally: - Boot clean with JSS_CORS_PROXY_MAX_BYTES=banana (falls back to 50MB) - Anonymous GET and POST both 401 against non-public pod (mode now derived per-method by authorize()) * cors-proxy: Copilot review pass 5 — paymentRequired pass-through, drop forwarded content-length Two fixes: 1. /proxy preHandler now handles paymentRequired from authorize(), same shape as the git hook. Pod owners who attach a PaymentCondition to the /proxy ACL get a 402 with the payment challenge instead of 401, matching every other WAC-gated endpoint. 2. content-length removed from FORWARD_REQUEST_HEADERS. We may re-stringify Fastify-parsed JSON bodies before the upstream fetch, so the caller's declared length doesn't match what we send. Node's fetch sets Content-Length automatically based on the actual body. (Browsers don't need it in Allow-Headers — Content-Length is CORS-safelisted on simple requests.) Verified locally: - POST text body 34 bytes → upstream sees content-length: 34 - POST JSON {"x":42,"y":"hi"} → upstream sees content-length: 17 (matches the re-stringified body, not what curl sent) * cors-proxy: emit partial slice at byte cap instead of dropping whole chunk Per Copilot review pass 6: The Transform's previous behavior was "if this chunk pushes us over the cap, drop the entire chunk and end the stream." For a small maxBytes (e.g. 100) the first incoming chunk (typically a full TLS record, ~16KB) would always exceed the cap, and the client received an empty body despite the cap being well above zero. Fixed by emitting the partial slice up to the cap, then push(null) + abort. Verified locally: --cors-proxy-max-bytes 100 against example.com now returns 100 bytes (the leading "<!doctype html><html lang=\"en\">..." HTML), not zero. * cors-proxy: Copilot review pass 7 — virtual-resource WAC + byte-cap boundary Two more: 1. authorize() has a "non-existent resource + write method → check parent container" fallback in src/auth/middleware.js:101. /proxy is a virtual endpoint with no backing storage, so POST /proxy was getting authorized against the parent (/) instead of /proxy itself. That's too permissive: an ACL granting acl:Read on / would let any Solid client POST through the proxy. Added a `skipParentForMissing` option to authorize() that disables the parent-fallback. The cors-proxy preHandler now passes it. WAC now correctly checks /proxy directly regardless of storage state. 2. The byte-cap Transform's exact-boundary case (chunk.length === remaining) used to pass the chunk through unchanged and continue reading. Subsequent chunks fell into the `remaining <= 0` drop branch but the stream stayed open — slow-streaming clients could hang and the upstream kept sending bytes we silently discarded. Fixed by collapsing both "fills exactly" and "would exceed" branches into a single "emit slice, end stream, abort" path. Verified locally: - cap=100 against example.com → 200 with 100 bytes, ~0.1 s - POST anonymous on a non-public pod → 401 (was previously evaluated against /, now against /proxy) * cors-proxy: Copilot review pass 8 — strip Authorization on cross-origin redirect, JSDoc Three fixes: 1. SECURITY: strip Authorization on cross-origin redirects. forwardHeaders is computed once and reused across hops. If the client opted in to upstream credentials via X-Upstream-Authorization, that token (renamed to Authorization) was being sent unchanged to every redirect target. A redirect from github.com → evil.com would leak the user's GitHub PAT to the attacker. Now we recompute the target origin per hop and delete Authorization when it differs from the initial origin — matches curl/browser/well-behaved-client behavior. Auth still rides along on same-origin redirects. 2. authorize() JSDoc updated to document skipParentForMissing — the new option that lets virtual endpoints (like /proxy) opt out of the non-existent-resource → check-parent fallback. Includes the why (without it, POST /proxy authorizes against / instead of /proxy). 3. PR description updated separately via the REST API to remove a stale "forwards Authorization" claim (gh pr edit --body was silently no-op'ing for me). The current header policy section explicitly states Authorization is NOT forwarded by default and describes the X-Upstream-Authorization opt-in. * cors-proxy: Copilot review pass 9 — WAC-Allow on 402, Allow on 405, drop dead branch Three small fixes: 1. 402 PaymentRequired response in the proxy preHandler now sets WAC-Allow before sending. Matches the global WAC hook and 401/403 paths; consistent header surface for browser clients (which can already read it via Access-Control-Expose-Headers). 2. 405 Method Not Allowed now carries an Allow header listing the supported methods (GET, POST, HEAD, OPTIONS). RFC 9110 requires this; without it, well-behaved clients can't discover what's acceptable. 3. isCorsProxyRequest() simplified to `urlPath === '/proxy'`. The `startsWith('/proxy?')` branch was dead code — every callsite already passes `request.url.split('?')[0]`. JSDoc updated to document that contract. Verified: PUT /proxy → 405 with `allow: GET, POST, HEAD, OPTIONS` GET /proxy?url=… on public pod → 200 GET /proxy on non-public → 401 with WAC-Allow + full proxy CORS headers * cors-proxy: Copilot review pass 10 — JSDoc completeness, comment accuracy Both documentation polish, no behavioral changes: 1. authorize() JSDoc return type now lists every field the function actually returns (paid, balance, currency in addition to the already-documented paymentRequired). Callers in server.js use these for paid-access headers. 2. cors-proxy.js comment that suggested "Fastify-parsed JSON gets re-stringified" was misleading. JSS registers a wildcard parseAs:'buffer' parser at server.js:190, so request bodies arrive as Buffer and pass through unchanged. The defensive JSON.stringify branch remains for safety, but the comment now reflects that it's a fallback rather than the common path. * cors-proxy: Copilot review pass 11 — surface paid-access headers Mirror the standard WAC hook (server.js:564-569) for the /proxy auth path: when checkAccess() returns paid (the cost) plus balance/currency, set X-Cost / X-Balance / X-Pay-Currency on the response. Without this, a successful paid ACL access debits the ledger silently and the browser side can't render charge UI for /proxy traffic. Note: the path-based multi-pod gap raised in the same review pass (/proxy is a single global endpoint, not per-pod /<pod>/proxy) is filed as #382 — too big a design change for Phase 1. * cors-proxy: Copilot review pass 12 — short-circuit OPTIONS, JSDoc paid type Two fixes: 1. OPTIONS preflight now bypasses authorize() entirely and goes straight to the handler (which already returns 204 + proxy CORS headers). authorize() does have its own OPTIONS short-circuit, but routing the preflight through there at all is unnecessary risk: future changes to authorize()/checkAccess() could end up debiting a ledger or evaluating a PaymentCondition on a preflight, which is never what we want. Defense-in-depth. 2. authorize() JSDoc had `paid?: boolean` but checkAccess() returns the cost as a number (src/wac/checker.js:189), and callers stringify it for the X-Cost header. Updated to `paid?: number` with a note explaining the runtime contract. Verified: OPTIONS /proxy on non-public pod → 204 with full proxy CORS (no auth) GET /proxy on non-public pod → 401 (unchanged) * cors-proxy: Copilot review pass 13 — expose payment headers, WAC-Allow on success Two consistency fixes to match the global WAC hook's response-header contract: 1. Add X-Cost / X-Balance / X-Pay-Currency to PROXY_CORS_HEADERS Access-Control-Expose-Headers. Pass 11 wired the proxy preHandler to *set* these headers, but without exposing them browser-side readers can't access them cross-origin. Aligns with the global getCorsHeaders() in src/ldp/headers.js. 2. Set WAC-Allow on the success path. Previously only the 401/403/402 branches did, so successful /proxy responses lost the header. The global WAC hook sets it for every response — the proxy hook now matches. Verified: GET /proxy?url=… on a public pod returns 200 with both \`wac-allow\` and \`access-control-expose-headers: …, X-Cost, X-Balance, X-Pay-Currency\`. * cors-proxy: Copilot review pass 14 — strip pod-authoritative headers, CSP sandbox Two real security fixes: 1. Pod-authoritative response headers (WAC-Allow, X-Cost, X-Balance, X-Pay-Currency) added to STRIP_RESPONSE_HEADERS. copyResponseHeaders would otherwise blindly forward an upstream's spoofed values and override our local ones. WAC-Allow describes *this* pod's ACL decision; the X-* payment headers describe a debit on this pod's ledger — neither is the upstream's to set. 2. Content-Security-Policy: sandbox + X-Content-Type-Options: nosniff added to setProxyCorsHeaders. Without these, navigating a browser to `/proxy?url=https://evil/page.html` would render attacker HTML in this pod's origin, with full access to other pod resources (cookies, localStorage, fetch). CSP sandbox neutralizes scripts/ plugins/navigation when the response is rendered as a document; nosniff blocks MIME-confusion tricks. fetch()-based callers are unaffected — they consume the bytes regardless. Verified: - 200 + 400 responses both carry `content-security-policy: sandbox` and `x-content-type-options: nosniff` - Upstream sending `WAC-Allow: evil` and `X-Cost: 999` via httpbin /response-headers — JSS strips them and our `wac-allow: public=…` survives, no x-cost in the response. * cors-proxy: Copilot review pass 15 — strip Content-Range/Accept-Ranges from upstream If the byte-cap Transform truncates an upstream 206 mid-stream, the forwarded Content-Range header still reflects the upstream's full range and no longer matches the bytes the client receives. Caller trusts the header → mis-handles the truncated body. Stripping both Content-Range and Accept-Ranges on the response side (via STRIP_RESPONSE_HEADERS) means the caller can't trust a stale range and just consumes whatever bytes actually arrived. Range still helps in the request direction — upstream sends fewer bytes when the caller sets it. Byte-accurate range proxying when the cap is smaller than the requested range is a Phase 2 concern. * cors-proxy: Copilot review pass 16 — preserve HEAD across redirects, fix dotfile guard Two real fixes: 1. Dotfile guard (server.js:402) was splitting the full URL on '/' before stripping the query string, so for /proxy?url=https://example .com/.git/config the segments included '.git' and the request was rejected with 403 — even though the dotfile is in the *upstream* URL, not the pod's path. The guard is about this pod's filesystem, not arbitrary URL contents. Fixed by splitting on '?' first. 2. Redirect handler in cors-proxy.js was unconditionally turning 301/302/303 responses into GET on the next hop, including for HEAD requests. That'd stream a body back on what was originally a body-less HEAD. Per fetch/RFC 7231 semantics, only POST (and other non-GET/non-HEAD) should be downgraded to GET on these redirects; HEAD must stay HEAD. Verified: - /proxy?url=…example.com/.git/config → 404 (upstream's response), no longer 403 from our guard - HEAD /proxy?url=…/redirect-to?...status_code=302 → 200 with empty body, method preserved across the redirect - GET redirect still works → 200 - Direct /.git/config → 403 (pod-internal dotfile guard intact) * cors-proxy: review pass 17 — fix misleading STRIP_RESPONSE_HEADERS comment about WAC-Allow ordering The comment said server.js "re-applies" the local values after upstream headers are copied. Actual flow: server.js sets WAC-Allow / X-Cost / X-Balance / X-Pay-Currency in the proxy preHandler *before* handleCorsProxy runs, and stripping them on the upstream side ensures copyResponseHeaders can't overwrite. Updated wording to describe the actual order so future maintainers (human or LLM) get accurate provenance.
* server: inject CORS headers on Fastify-internal errors (#376) Surfaced during #374 review. When Fastify itself rejects a request before any handler runs (most commonly FST_ERR_BAD_URL for malformed percent-encoding like `%g1`, truncated `%E0%`, invalid UTF-8 sequences), the 400 response carries no `Access-Control-Allow-*` headers — browsers surface it as a generic CORS / network error instead of the real status, which is confusing for debugging and undermines the per-handler CORS work in #371 / #374. ## Root cause `FST_ERR_BAD_URL` is thrown by Fastify's URL parser BEFORE the request enters the normal handler chain. Confirmed by direct probe: neither `setErrorHandler` nor `onSend` hooks fire for this code path. Fastify writes the 400 response directly via `res.writeHead({...})` / `res.end(body)` in `onBadUrl()` (node_modules/fastify/fastify.js:752–771) when no `frameworkErrors` option is configured. ## Fix Configure Fastify's `frameworkErrors` option in `createServer`. This is the explicit hook Fastify provides for framework-level errors (FST_ERR_BAD_URL and FST_ERR_ASYNC_CONSTRAINT). When set, Fastify routes the error through this function instead of the direct-write path, giving us a Reply object to attach headers to. The handler: - Reads `request.headers.origin`; if present, sets the full JSS CORS header set via `getCorsHeaders(origin)` (ACAO mirrors the request Origin, plus Allow-Methods / Allow- Headers / Expose-Headers / Credentials / Max-Age). - Sends a JSON body matching the prior shape (`error`, `code`, `message`, `statusCode`) — minimal change for clients that were parsing the old format. ## Test plan Three new integration tests against a live JSS server: 1. `%g1` with Origin → 400, ACAO mirrors Origin, full CORS set 2. `%g1` without Origin → 400, JSON body still well-shaped (CORS not enforced when no Origin was sent) 3. Normal 404 path (no FST_ERR) → CORS still injected by the existing wildcard handler — confirms the new hook didn't displace existing behavior Test count: 777 → 780 in full suite. * Address copilot pass 1 on #421 Three findings, all valid. 1. The `if (origin)` guard made the frameworkErrors path inconsistent with the rest of the server, which sets CORS on EVERY response via the global onRequest hook (with ACAO defaulting to `*` when no Origin was sent). Removed the guard — getCorsHeaders is now called unconditionally. 2. `error: err.name` produced "FastifyError" (unhelpful), while Fastify's default error body uses the HTTP status text ("Bad Request"). Switched to `STATUS_CODES[statusCode] || 'Error'` from node:http — 400 → "Bad Request", 500 → "Internal Server Error", etc. Matches Fastify's default body shape so any pre-fix client that was parsing `error` keeps working. 3. The "no Origin" test name implied CORS was set with ACAO=* but the assertions only checked the body. Now asserts ACAO=*, ACAM contains GET, ACAH is set, and the body's `error` field is "Bad Request". The "with Origin" test also gained a body-shape assertion on the new `error: "Bad Request"` field so any future regression (like reverting to err.name) is caught. Test count: same 780. * Address copilot pass 2 on #421 — doc cleanups Two stale-docs findings, both real. 1. The test file's block comment said the fix attaches CORS "whenever the bad request carried an Origin header" — that was the pass-0 behavior. The pass-1 review (correctly) removed that guard so CORS is now set unconditionally (with ACAO=* when no Origin was sent). Updated the comment to match the actual behavior. 2. The PR description's "After" example body still showed `error: "FastifyError"` from my initial draft, before the pass-1 fix that switched to `STATUS_CODES[statusCode]` ("Bad Request" for 400). Updated the PR body via REST patch so the example matches the shipped response shape. No code or test changes — pure documentation alignment. Test count: same 780.
… auto-init suite (#375) src/handlers/git.js had zero automated coverage — #371 (CORS) and #373 (multi-slash) were verified by hand. New test/git-handler.test.js pins the HTTP contract with 8 cases: - OPTIONS preflight → 200 + canonical git CORS headers (#371) - single/double/triple-slash info/refs advertise → 200 (#373) - missing repo → 404 WITH CORS headers (#374) - path traversal (percent-encoded so fetch doesn't normalize it away) → blocked 403/404, never 200/500, with CORS - unauthenticated push advertise → 401 + WWW-Authenticate on a non-public server. Deliberately does NOT assert CORS: the WAC preHandler's 401 path doesn't set the git CORS headers today — that gap is now tracked as #548; the test asserts what is true. - end-to-end push: real `git push HEAD:main` via async spawn → exit 0 → pushed file served as a static resource (receive-pack POST through http-backend + updateInstead extraction). The push MUST use async spawn — spawnSync blocks the event loop and the in-process server can never respond (deadlock). Also fixes a latent cross-test pollution bug in test/git-auto-init.test.js: the unauthenticated-ACL test boots a second server with root './test-data-git-auto-init-authcheck', and createServer({ root }) mutates process.env.DATA_ROOT globally without restoring it. Every test after it silently ran against the WRONG data root — handleGit auto-inited public/apps/penny inside the authcheck dir, the suite cleanup missed it, and the orphaned directory kept reappearing after every full test run. Snapshot/restore the env in the test's finally block (same pollution class as the #543 review finding). The authcheck dir no longer survives a run. Full suite: 934/934 passing. Closes #375.
… auto-init suite (#375) (#549) src/handlers/git.js had zero automated coverage — #371 (CORS) and #373 (multi-slash) were verified by hand. New test/git-handler.test.js pins the HTTP contract with 8 cases: - OPTIONS preflight → 200 + canonical git CORS headers (#371) - single/double/triple-slash info/refs advertise → 200 (#373) - missing repo → 404 WITH CORS headers (#374) - path traversal (percent-encoded so fetch doesn't normalize it away) → blocked 403/404, never 200/500, with CORS - unauthenticated push advertise → 401 + WWW-Authenticate on a non-public server. Deliberately does NOT assert CORS: the WAC preHandler's 401 path doesn't set the git CORS headers today — that gap is now tracked as #548; the test asserts what is true. - end-to-end push: real `git push HEAD:main` via async spawn → exit 0 → pushed file served as a static resource (receive-pack POST through http-backend + updateInstead extraction). The push MUST use async spawn — spawnSync blocks the event loop and the in-process server can never respond (deadlock). Also fixes a latent cross-test pollution bug in test/git-auto-init.test.js: the unauthenticated-ACL test boots a second server with root './test-data-git-auto-init-authcheck', and createServer({ root }) mutates process.env.DATA_ROOT globally without restoring it. Every test after it silently ran against the WRONG data root — handleGit auto-inited public/apps/penny inside the authcheck dir, the suite cleanup missed it, and the orphaned directory kept reappearing after every full test run. Snapshot/restore the env in the test's finally block (same pollution class as the #543 review finding). The authcheck dir no longer survives a run. Full suite: 934/934 passing. Closes #375.
Closes #373.
Summary
Symptom (from melvin.me running v0.0.170)
```
git http-backend stderr: fatal: '/melvin/public/git/test//info/refs': aliased
HTTP 500
```
Triggered by frontend at jss.live/git/ appending `/info/refs` to a repo URL the user typed with a trailing slash.
Verification
Boot test on this branch with a bare repo at `/tmp/jss-373-test/repo.git` and `--public`:
```
single-slash HTTP 200 application/x-git-upload-pack-advertisement
double-slash HTTP 200 application/x-git-upload-pack-advertisement
triple-slash HTTP 200 application/x-git-upload-pack-advertisement
```
Test plan
git-protocolheader in CORS preflight responses #371