Skip to content

fix(http): require auth and bind loopback for HTTP transport (RG-4626) - #85

Merged
lucadruda merged 3 commits into
mainfrom
fix/rg-4626-http-transport-auth
Jul 30, 2026
Merged

fix(http): require auth and bind loopback for HTTP transport (RG-4626)#85
lucadruda merged 3 commits into
mainfrom
fix/rg-4626-http-transport-auth

Conversation

@lucadruda

Copy link
Copy Markdown
Collaborator

Summary

Fixes RG-4626 (CWE-306 Missing Authentication + CWE-346/CWE-350 DNS rebinding; CVSS 7.4, High). Reported privately to security@docker.com.

In --transport=http mode the server was a confused deputy:

  • The listener bound all interfaces (app.listen(port) with no host) → reachable by any TCP peer.
  • POST /mcp had no authentication and no Host/Origin validation.
  • Every tool call was dispatched under the operator's process-wide HUB_PAT_TOKEN, including write tools (createRepository, updateRepositoryInfo).

So an uncredentialed caller — directly over the network, or via DNS rebinding from a website the operator visited — could enumerate private repos and create/modify repositories under the operator's Docker Hub identity.

Changes

Hardened the HTTP transport to match the accepted upstream pattern (docker/mcp-gateway, GHSA-46gc-mwh4-cc5r). stdio transport is unchanged.

  • Loopback by default — binds 127.0.0.1; expose deliberately with --host=0.0.0.0 (logs a warning).
  • Auth required, fail-closed — refuses to start HTTP unless MCP_AUTH_TOKEN is set (clients send Authorization: Bearer <token>) or --allow-unauthenticated is explicitly passed. Token comparison is constant-time (crypto.timingSafeEqual over SHA-256 digests).
  • DNS-rebinding / CSRF guard — rejects disallowed Host headers (allowlist: loopback + --host + --allowed-hosts) and any browser Origin not in --allowed-origins. Non-browser MCP clients (no Origin, loopback Host) are unaffected.
  • Fatal startup errors now print synchronously to stderr (the async logger was truncated by process.exit, hiding the fail-closed reason).
  • New config: MCP_AUTH_TOKEN env, --host, --allowed-hosts, --allowed-origins, --allow-unauthenticated. Documented in the README ("Securing the HTTP transport").

Tests

Added src/server.test.ts (Node built-in test runner, compiled via tsconfig.test.json — no esbuild/tsx dependency so it runs under the repo's ignore-scripts CI) and wired npm test into the Lint workflow. Coverage:

  • fails closed without a token / opt-out
  • 401 on missing and wrong bearer token
  • 403 on spoofed Host (DNS rebinding) and disallowed browser Origin
  • 200 on an authenticated loopback request
  • --allow-unauthenticated serves without a token
  • explicitly allowed Origin is honored

Local: build, lint, format:check, test (8/8) all green.

Follow-up

Per the ticket's coordinated-disclosure note: once merged, publish a GHSA/CVE with the security team.

🤖 Generated with Claude Code

The --transport=http server bound to all interfaces, exposed POST /mcp
with no authentication or Host/Origin validation, and dispatched every
tool under the operator's process-wide HUB_PAT_TOKEN. Any TCP peer, or a
website the operator visited (DNS rebinding), could therefore act as the
operator on Docker Hub, including creating and modifying repositories
(CWE-306, CWE-346/CWE-350; CVSS 7.4).

Harden the HTTP transport to match the accepted upstream pattern
(docker/mcp-gateway, GHSA-46gc-mwh4-cc5r):

- Bind to 127.0.0.1 by default; expose deliberately with --host.
- Require a bearer token (MCP_AUTH_TOKEN) on every /mcp request and fail
  closed at startup unless --allow-unauthenticated is explicitly passed.
  Token comparison is constant-time.
- Add a DNS-rebinding/CSRF guard: reject disallowed Host headers and any
  browser Origin not in --allowed-origins. Non-browser MCP clients, which
  send no Origin and a loopback Host, are unaffected.

Also surface fatal startup errors synchronously on stderr (the async
logger was truncated by process.exit, hiding the fail-closed reason), and
add an integration test suite plus a CI step covering all the above.

stdio transport behaviour is unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@lucadruda
lucadruda requested a review from a team as a code owner July 29, 2026 09:30
Comment thread src/server.ts Fixed
Comment thread src/server.ts Fixed
Comment thread src/server.ts Fixed
- Replace the `/^Bearer\s+(.+)$/i` Authorization parser with linear
  indexOf/slice parsing to remove the polynomial-ReDoS exposure on the
  attacker-controlled header (CodeQL js/polynomial-redos).
- Sanitize attacker-controlled Host/Origin header values (strip control
  chars incl. CR/LF) before logging rejections, preventing forged/split
  log entries (CodeQL js/log-injection).

Behaviour is unchanged for well-formed requests; tests still pass (8/8).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@lucadruda

Copy link
Copy Markdown
Collaborator Author

Addressed the CodeQL findings in c84d09e:

  • Polynomial ReDoS (src/server.ts, Authorization parsing) — replaced the /^Bearer\s+(.+)$/i regex with linear indexOf/slice parsing (parseBearerToken), so an attacker-controlled Authorization header can no longer trigger catastrophic backtracking.
  • Log injection (rejected Origin/Host) — the header values are now passed through sanitizeForLog, which strips control characters (incl. CR/LF) before they reach the logger, preventing forged/split log entries.

Behaviour is unchanged for well-formed requests; the test suite still passes 8/8. CodeQL should clear alerts 46–48 on the next scan.

Comment thread src/server.ts Fixed
Comment thread src/server.ts Fixed
The previous sanitizer stripped control characters via a single
Unicode-range replace, which CodeQL did not recognize as a
log-injection barrier (alerts 49/50 re-fired on the Host/Origin log
lines). Lead with a newline-stripping replace -- the same pattern
already used for the request body elsewhere in this file and accepted
by CodeQL -- then keep the control-character strip as defense in depth.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@lucadruda

Copy link
Copy Markdown
Collaborator Author

Follow-up in 5392a36: the ReDoS alert (46) cleared, but the log-injection alerts re-fired (49/50) because CodeQL didn't recognize the Unicode-range strip as a barrier. sanitizeForLog now leads with a newline-stripping replace(/\n|\r/g, '') — the same pattern already used on the request body in this file and accepted by CodeQL — and keeps the control-character strip as defense in depth. build/lint/format/test still green (8/8).

@lucadruda
lucadruda merged commit 2ec0681 into main Jul 30, 2026
5 checks passed
@lucadruda
lucadruda deleted the fix/rg-4626-http-transport-auth branch July 30, 2026 09:32
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.

3 participants