Skip to content

git handler: collapse multi-slash in URL before forwarding to http-backend (#373)#374

Merged
melvincarvalho merged 4 commits into
gh-pagesfrom
issue-373-git-double-slash
May 8, 2026
Merged

git handler: collapse multi-slash in URL before forwarding to http-backend (#373)#374
melvincarvalho merged 4 commits into
gh-pagesfrom
issue-373-git-double-slash

Conversation

@melvincarvalho

Copy link
Copy Markdown
Contributor

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

…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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 urlPath in handleGit() by collapsing multi-slash sequences (/{2,}/) before calling extractRepoPath() and before forwarding as PATH_INFO.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/handlers/git.js
// 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]);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

Comment thread src/handlers/git.js Outdated
Comment on lines +116 to +120
urlPath = decodeURIComponent(request.url.split('?')[0]);
} catch {
return reply.code(400).send({ error: 'Invalid URL encoding' });
}
urlPath = urlPath.replace(/\/{2,}/g, '/');

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair — filing a follow-up issue for git handler test coverage. Out of scope for the regression fix here.

Comment thread src/handlers/git.js Outdated
Comment on lines +115 to +119
try {
urlPath = decodeURIComponent(request.url.split('?')[0]);
} catch {
return reply.code(400).send({ error: 'Invalid URL encoding' });
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

Comment thread src/handlers/git.js Outdated
Comment on lines +90 to +99
/**
* 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');

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in f8a2b13: extracted GIT_CORS_HEADERS const; both the helper and the streaming reply.raw.setHeader loop iterate over the same map.

Comment thread src/handlers/git.js Outdated
Comment on lines 131 to 134
if (!repoRelative) {
setGitCorsHeaders(reply);
return reply.code(400).send({ error: 'Invalid git request' });
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@melvincarvalho
melvincarvalho merged commit 5ce259f into gh-pages May 8, 2026
4 checks passed
@melvincarvalho
melvincarvalho deleted the issue-373-git-double-slash branch May 8, 2026 01:46
melvincarvalho added a commit that referenced this pull request May 8, 2026
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
melvincarvalho added a commit that referenced this pull request May 8, 2026
* 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.
melvincarvalho added a commit that referenced this pull request May 12, 2026
* 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.
melvincarvalho added a commit that referenced this pull request Jun 10, 2026
… 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.
melvincarvalho added a commit that referenced this pull request Jun 10, 2026
… 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

git handler: 500 on double-slash in URL (PATH_INFO not normalized)

2 participants