Skip to content

fix(proxy): forward original encoded request-path instead of decoded …#76

Open
Victor-De-Decker wants to merge 4 commits into
mainfrom
fix/proxy-forward-encoded-path
Open

fix(proxy): forward original encoded request-path instead of decoded …#76
Victor-De-Decker wants to merge 4 commits into
mainfrom
fix/proxy-forward-encoded-path

Conversation

@Victor-De-Decker

@Victor-De-Decker Victor-De-Decker commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Summary

One ordinary request through the proxy could crash the entire gateway — and with it every running huddle.

Since the finding #7 hardening (#67), the proxy normalizes request paths with decodeURIComponent() for the firewall rule check, but then forwarded that decoded path to the upstream. Any percent-encoded character that decodes to something illegal in an HTTP request-target (%20, any non-ASCII UTF-8) makes Node's http(s).request() throw ERR_UNESCAPED_CHARACTERS synchronously; the throw reached the uncaughtException handler in index.ts, which calls process.exit(1). In practice this was triggered by legitimate traffic: an Azure DevOps project name containing a space (/org/My%20Project/_git/...) killed the gateway on every git fetch, minutes after each huddle started.

This PR:

  1. Forwards the original encoded bytes in both the plain-HTTP and MITM handlers, while the rule check keeps deciding on the decoded form (unchanged). This is the design normalizePathname()'s documentation in rules.ts already describes. It also strengthens finding Mcp policies token hiding #7: forwarding the decoded form let double-encoded %252e%252e reach the upstream half-decoded (whose own decode turns it into .., reopening the traversal differential), and silently corrupted legitimate %2F/%20 in forwarded paths.
  2. Wraps the http(s).request() construction in try/catch in both handlers, so any future synchronous validation throw fails that single request with a 400 instead of exiting the process.
  3. Adds an e2e regression test (boundary.e2e.ts) covering both proxy paths.

Verified end-to-end against a live container: the previously crashing URL now returns the upstream's normal response (302 from dev.azure.com) on both paths, traversal (/foo/..%2f..%2fadmin) is still blocked with 403, and the gateway stays up.

Related issue

Fixes #

Type of change

  • Bug fix (fix)
  • New feature (feat)
  • Documentation (docs)
  • Refactor / maintenance (refactor / chore)
  • Other:

Related issue

Checklist

  • My branch is up to date with main and focused on a single change.
  • The project builds and type-checks locally.
  • Tests pass (npm --prefix gateway test) and I added/updated tests where relevant.
  • I ran Prettier and did not reformat unrelated code.
  • Commit messages follow Conventional Commits.
  • I did not introduce logging of secrets, tokens, or credentials.
  • Documentation is updated where needed (README / relevant docs). (No docs impact.)

Notes for reviewers

  • The security-relevant hunks are the two path: changes (gateway/src/proxy.ts). The rule check still receives the decoded form — only the forwarded bytes changed. The existing finding Mcp policies token hiding #7 e2e test (traversal → 403) passes unchanged.
  • The try/catch blocks are defense-in-depth: with encoded forwarding, Node's server parser already guarantees a valid request-target, so they should never fire — but a synchronous constructor throw must never take down the shared gateway again.
  • Note on unit tests when run on Windows: 3 socket-proxy.test.ts failures are pre-existing on main (Unix-socket EACCES); the skip fix (de51151) lives on experiment/58-wslc-support and hasn't landed on main. Linux CI is unaffected.
  • Repro of the crash on the current image, if you want to see it fail before the fix:
  docker run -d --name repro -p 18081:80 -v repro:/data ghcr.io/infosupport/huddle:<current-tag>
  docker exec repro node -e "new (require('better-sqlite3'))('/data/huddle.db').prepare(\"INSERT INTO rules (domain, container_id, status) VALUES ('dev.azure.com', NULL, 'allow')\").run()"
  curl -x http://localhost:18081 -k "https://dev.azure.com/org/My%20Project/_git/My%20Project/info/refs?service=git-upload-pack"
  docker inspect repro --format '{{.State.Status}} exit={{.State.ExitCode}}'   # → exited exit=1

@Victor-De-Decker
Victor-De-Decker requested a review from a team July 20, 2026 14:42
…guard

Deduplicates the identical try/catch around both http(s).request call
sites, as flagged by the aikido-pr-checks review.
insuT0ver added a commit that referenced this pull request Jul 21, 2026
Addresses review feedback on #76:

1. Revert the accidental import reformatting in proxy.ts (brace-spacing +
   reordering) back to main's style — keeps the diff to the security-relevant
   hunks and matches the file's own `{ ... }` convention.

2. Add proxy-forward-path.test.ts: a unit-level regression that drives
   createProxyServer() against a real local upstream (no Docker, no live
   container) and asserts the upstream receives the *encoded* bytes
   (`/foo/a%20b`, `/foo/%E2%9C%93`, query preserved) while traversal
   (`/foo/..%2f..%2fadmin`) is still fail-closed (403, never forwarded).
   Guards the fix under `npm test`, not only the E2E_ENABLED-gated e2e.
   Binding-gated like the other SQLite suites (skips where better-sqlite3
   isn't built; runs in CI/image).

3. Tighten the e2e assertions: instead of `not.toBe('403') / not.toBe('000')`
   (passes on almost any status), require a well-formed upstream status that
   is neither '000' (crash / refused CONNECT), '403' (Huddle block), nor '400'
   (the bad_request forward-guard — exactly what firing would mean the decoded
   raw-space form was forwarded again). Non-brittle: no dependency on
   example.org's exact status.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses review feedback on #76:

1. Revert the accidental import reformatting in proxy.ts (brace-spacing +
   reordering) back to main's style — keeps the diff to the security-relevant
   hunks and matches the file's own `{ ... }` convention.

2. Add proxy-forward-path.test.ts: a unit-level regression that drives
   createProxyServer() against a real local upstream (no Docker, no live
   container) and asserts the upstream receives the *encoded* bytes
   (`/foo/a%20b`, `/foo/%E2%9C%93`, query preserved) while traversal
   (`/foo/..%2f..%2fadmin`) is still fail-closed (403, never forwarded).
   Guards the fix under `npm test`, not only the E2E_ENABLED-gated e2e.
   Binding-gated like the other SQLite suites (skips where better-sqlite3
   isn't built; runs in CI/image).

   To make this testable, createProxyServer() takes an optional `port`
   (default = PROXY_PORT, so index.ts is unchanged); the test binds port 0
   (ephemeral) instead of the privileged :80.

3. Tighten the e2e assertions: instead of `not.toBe('403') / not.toBe('000')`
   (passes on almost any status), require a well-formed upstream status that
   is neither '000' (crash / refused CONNECT), '403' (Huddle block), nor '400'
   (the bad_request forward-guard — exactly what firing would mean the decoded
   raw-space form was forwarded again). Non-brittle: no dependency on
   example.org's exact status.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@insuT0ver
insuT0ver force-pushed the fix/proxy-forward-encoded-path branch from 7655176 to e9be8ed Compare July 21, 2026 15:00
@insuT0ver

Copy link
Copy Markdown
Contributor

huddle init --experiment 76

Can you try to run it with this. if everything succeeds and you checked my last commit. I will approve and merge.

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.

2 participants