From c79d7bc33ed00b6001f7842ac4b29d65d254382f Mon Sep 17 00:00:00 2001 From: Luca Druda Date: Wed, 29 Jul 2026 11:26:47 +0200 Subject: [PATCH 1/3] fix(http): require auth and bind loopback for HTTP transport (RG-4626) 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) --- .github/workflows/lint.yml | 5 +- .gitignore | 1 + README.md | 66 ++++++++++++- eslint.config.mjs | 2 +- package.json | 1 + src/index.ts | 46 ++++++++- src/server.test.ts | 153 +++++++++++++++++++++++++++++ src/server.ts | 192 +++++++++++++++++++++++++++++++++++-- tsconfig.test.json | 9 ++ 9 files changed, 458 insertions(+), 17 deletions(-) create mode 100644 src/server.test.ts create mode 100644 tsconfig.test.json diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 9ca066c..fe4fc4c 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -29,6 +29,9 @@ jobs: - name: Run linters run: npm run lint - + - name: Run Formatting run: npm run format:check + + - name: Run tests + run: npm test diff --git a/.gitignore b/.gitignore index 15428c3..8c08d35 100644 --- a/.gitignore +++ b/.gitignore @@ -1,5 +1,6 @@ node_modules/ dist/ +dist-test/ .vscode/ .env logs/ diff --git a/README.md b/README.md index 7550c36..631f36e 100644 --- a/README.md +++ b/README.md @@ -1,4 +1,5 @@ # Docker Hub MCP Server + [![Trust Score](https://archestra.ai/mcp-catalog/api/badge/quality/docker/hub-mcp)](https://archestra.ai/mcp-catalog/docker__hub-mcp) The Docker Hub MCP Server is a [Model Context Protocol (MCP)](https://modelcontextprotocol.io/introduction) server that interfaces with Docker Hub APIs to make them accessible to LLMs, enabling intelligent content discovery and repository management. @@ -36,13 +37,19 @@ Developers building with containers, especially in AI and LLM-powered workflows, 2. **Run** ```bash - npm start -- [--transport=http|stdio] [--port=3000] + npm start -- [--transport=http|stdio] [--port=3000] [--host=127.0.0.1] ``` - Default args: - - `transport`: Choose between `http` or `stdio` (default: `stdio`) - - `port=3000` - This starts the server with default settings and can only access public Docker Hub content. + - `transport`: Choose between `http` or `stdio` (default: `stdio`) + - `port=3000` + - `host=127.0.0.1` (HTTP transport only) + This starts the server with default settings and can only access public Docker Hub content. + +> [!IMPORTANT] +> The `http` transport binds to `127.0.0.1` (loopback) by default and **requires +> authentication**. See [Securing the HTTP transport](#securing-the-http-transport) +> before exposing it to a network. ### Run in inspector [Optional] @@ -52,6 +59,53 @@ The MCP Inspector provides a web interface to test your server: npx @modelcontextprotocol/inspector node dist/index.js [--transport=http|stdio] [--port=3000] ``` +## Securing the HTTP transport + +The `stdio` transport is only reachable by the local process that spawns it. The +`http` transport, however, dispatches every tool call using the server operator's +Docker Hub Personal Access Token (`HUB_PAT_TOKEN`). Anyone who can reach the HTTP +endpoint can therefore act as that Docker Hub identity — including creating and +modifying repositories. To prevent this, the HTTP transport is locked down by +default: + +- **Loopback binding.** The listener binds to `127.0.0.1` unless you pass + `--host=` (for example `--host=0.0.0.0` to expose it from a container). +- **Authentication required (fail-closed).** In `http` mode the server refuses to + start unless you either provide a bearer token or explicitly opt out. Set the + token via the `MCP_AUTH_TOKEN` environment variable; clients must then send it as + `Authorization: Bearer ` on every request. +- **DNS-rebinding / CSRF protection.** Requests are rejected when the `Host` header + is not in the allow-list (loopback plus `--host`, extendable with + `--allowed-hosts`), or when they carry a browser `Origin` header that is not + listed in `--allowed-origins`. Non-browser MCP clients are unaffected. + +Run the HTTP transport with authentication: + +```bash +MCP_AUTH_TOKEN= npm start -- --transport=http +``` + +Expose it beyond loopback (e.g. inside a container), still authenticated: + +```bash +MCP_AUTH_TOKEN= npm start -- \ + --transport=http --host=0.0.0.0 \ + --allowed-hosts=my-host.internal --allowed-origins=https://my-app.example.com +``` + +| Flag / env var | Purpose | +| ------------------------- | --------------------------------------------------------------- | +| `MCP_AUTH_TOKEN` | Bearer token required on every `/mcp` request. | +| `--host=` | Address to bind (default `127.0.0.1`). | +| `--allowed-hosts=a,b` | Extra `Host` header values to accept (comma-separated). | +| `--allowed-origins=a,b` | Browser `Origin` values to accept (comma-separated). | +| `--allow-unauthenticated` | Serve `/mcp` with **no** authentication. Insecure; opt-in only. | + +> [!WARNING] +> `--allow-unauthenticated` disables authentication entirely and exposes your +> Docker Hub PAT to any client that can reach the port. Only use it on a trusted, +> isolated network. + ## Authenticate with docker By default this MCP server can only query public content on Docker Hub. In order to manage your repositories you need to provide authentication. @@ -67,7 +121,9 @@ HUB_PAT_TOKEN= npm start -- [--username= npx @modelcontextprotocol/inspector node dist/index.js[--username=] ``` + ## Usage in Docker Ask Gordon + You can configure Gordon to be a host that can interact with the Docker Hub MCP server. ### Gordon Setup @@ -77,7 +133,7 @@ You can configure Gordon to be a host that can interact with the Docker Hub MCP You can configure Gordon to be a client that can interact with the Docker Hub MCP server. 1. Create the [`gordon-mcp.yml` file](https://docs.docker.com/ai/gordon/mcp/yaml/) file in your working directory. -2. Replace environment variables in the `gordon-mcp.yml` with your Docker Hub username and a PAT token. +2. Replace environment variables in the `gordon-mcp.yml` with your Docker Hub username and a PAT token. ``` services: diff --git a/eslint.config.mjs b/eslint.config.mjs index e61dc70..7c313d3 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -7,6 +7,6 @@ export default tseslint.config( eslint.configs.recommended, tseslint.configs.recommended, { - ignores: ["node_modules/**", "dist/**", "src/scout/genql/**"], + ignores: ["node_modules/**", "dist/**", "dist-test/**", "src/scout/genql/**"], } ); diff --git a/package.json b/package.json index 9a03984..cd2f328 100644 --- a/package.json +++ b/package.json @@ -5,6 +5,7 @@ "main": "dist/index.js", "scripts": { "build": "tsc", + "test": "tsc -p tsconfig.test.json && node --test \"dist-test/**/*.test.js\"", "start": "node dist/index.js", "clean": "rm -rf dist", "lint": "eslint --ext .ts .", diff --git a/src/index.ts b/src/index.ts index 116b4f5..555d803 100644 --- a/src/index.ts +++ b/src/index.ts @@ -17,6 +17,9 @@ import { logger } from './logger'; import { HubMCPServer } from './server'; const DEFAULT_PORT = 3000; +// Bind to loopback by default so the HTTP transport is not exposed to the +// network unless the operator explicitly opts in with --host. See RG-4626. +const DEFAULT_HOST = '127.0.0.1'; const STDIO_OPTION = 'stdio'; function parseTransportFlag(args: string[]): string { @@ -55,6 +58,32 @@ function parsePortFlag(args: string[]): number { return portParsed; } +function parseHostFlag(args: string[]): string { + const hostArg = args.find((arg) => arg.startsWith('--host='))?.split('=')[1]; + if (!hostArg || hostArg.length === 0) { + logger.info(`host unspecified, defaulting to ${DEFAULT_HOST}`); + return DEFAULT_HOST; + } + + return hostArg; +} + +function parseBooleanFlag(args: string[], name: string): boolean { + return args.includes(`--${name}`); +} + +function parseListFlag(args: string[], name: string): string[] { + const value = args.find((arg) => arg.startsWith(`--${name}=`))?.split('=')[1]; + if (!value) { + return []; + } + + return value + .split(',') + .map((entry) => entry.trim()) + .filter((entry) => entry.length > 0); +} + // Main execution async function main() { const args = process.argv.slice(2); @@ -66,7 +95,15 @@ async function main() { const server = new HubMCPServer(username, patToken); // Start the server - await server.run(port, transportArg); + await server.run(port, transportArg, { + host: parseHostFlag(args), + // The bearer token clients must present on the HTTP transport. Read from + // the environment (not argv) so it is not exposed via the process table. + authToken: process.env.MCP_AUTH_TOKEN, + allowUnauthenticated: parseBooleanFlag(args, 'allow-unauthenticated'), + allowedHosts: parseListFlag(args, 'allowed-hosts'), + allowedOrigins: parseListFlag(args, 'allowed-origins'), + }); logger.info('🚀 dockerhub mcp server is running...'); } @@ -76,7 +113,12 @@ process.on('unhandledRejection', (error) => { }); main().catch((error) => { - logger.info(`failed to start server: ${error}`); + const message = error instanceof Error ? error.message : String(error); + // Write synchronously to stderr as well: the logger's transports flush + // asynchronously and would be truncated by the immediate process.exit below, + // hiding the reason a startup was refused (e.g. the HTTP auth fail-closed check). + console.error(`failed to start server: ${message}`); + logger.error(`failed to start server: ${error}`); process.exit(1); }); diff --git a/src/server.test.ts b/src/server.test.ts new file mode 100644 index 0000000..5893a94 --- /dev/null +++ b/src/server.test.ts @@ -0,0 +1,153 @@ +/* + Copyright 2025 Docker Hub MCP Server authors + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +import assert from 'node:assert/strict'; +import http from 'node:http'; +import { AddressInfo } from 'node:net'; +import { test } from 'node:test'; +import { HttpTransportOptions, HubMCPServer } from './server'; + +// Regression tests for RG-4626: the HTTP transport must not serve tools (which run +// under the operator's Docker Hub PAT) without authentication, and must reject +// browser-driven / DNS-rebinding requests. + +const INIT_BODY = JSON.stringify({ + jsonrpc: '2.0', + id: 1, + method: 'initialize', + params: { + protocolVersion: '2025-06-18', + capabilities: {}, + clientInfo: { name: 'test', version: '1.0' }, + }, +}); + +interface Response { + status: number; + body: string; +} + +// Uses the low-level http client (not fetch) so we can set otherwise-forbidden +// request headers such as Host, which the DNS-rebinding guard inspects. +function post(port: number, headers: Record): Promise { + return new Promise((resolve, reject) => { + const req = http.request( + { + hostname: '127.0.0.1', + port, + path: '/mcp', + method: 'POST', + headers: { + 'Content-Type': 'application/json', + Accept: 'application/json, text/event-stream', + 'Content-Length': Buffer.byteLength(INIT_BODY), + ...headers, + }, + }, + (res) => { + let body = ''; + res.on('data', (chunk) => (body += chunk)); + res.on('end', () => resolve({ status: res.statusCode ?? 0, body })); + } + ); + req.on('error', reject); + req.write(INIT_BODY); + req.end(); + }); +} + +const BASE_OPTIONS: HttpTransportOptions = { + host: '127.0.0.1', + allowUnauthenticated: false, + allowedHosts: [], + allowedOrigins: [], +}; + +const TOKEN = 'super-secret-token'; +const AUTH_OPTIONS: HttpTransportOptions = { ...BASE_OPTIONS, authToken: TOKEN }; +const bearer = { Authorization: `Bearer ${TOKEN}` }; + +// A username/token is supplied so PAT auth is configured; the security guards reject +// unauthorized requests before any Docker Hub call could be attempted. +function newServer(): HubMCPServer { + return new HubMCPServer('test-user', 'test-pat'); +} + +// Binds the transport to an ephemeral loopback port and always closes it afterwards +// so the test process exits cleanly. +async function withServer( + options: HttpTransportOptions, + fn: (port: number) => Promise +): Promise { + const app = newServer().buildHttpApp(options); + const server = app.listen(0, '127.0.0.1'); + await new Promise((resolve) => server.once('listening', () => resolve())); + try { + await fn((server.address() as AddressInfo).port); + } finally { + await new Promise((resolve) => server.close(() => resolve())); + } +} + +test('buildHttpApp fails closed without a token or explicit opt-out', () => { + assert.throws(() => newServer().buildHttpApp(BASE_OPTIONS), /Refusing to start/); +}); + +test('rejects a request with no bearer token (401)', async () => { + await withServer(AUTH_OPTIONS, async (port) => { + assert.equal((await post(port, {})).status, 401); + }); +}); + +test('rejects a request with a wrong bearer token (401)', async () => { + await withServer(AUTH_OPTIONS, async (port) => { + assert.equal((await post(port, { Authorization: 'Bearer wrong' })).status, 401); + }); +}); + +test('rejects a disallowed browser Origin (403)', async () => { + await withServer(AUTH_OPTIONS, async (port) => { + const res = await post(port, { ...bearer, Origin: 'http://evil.example' }); + assert.equal(res.status, 403); + }); +}); + +test('rejects a spoofed Host header / DNS rebinding (403)', async () => { + await withServer(AUTH_OPTIONS, async (port) => { + const res = await post(port, { ...bearer, Host: 'evil.example' }); + assert.equal(res.status, 403); + }); +}); + +test('accepts an authenticated loopback request (200)', async () => { + await withServer(AUTH_OPTIONS, async (port) => { + assert.equal((await post(port, bearer)).status, 200); + }); +}); + +test('--allow-unauthenticated serves without a token (200)', async () => { + await withServer({ ...BASE_OPTIONS, allowUnauthenticated: true }, async (port) => { + assert.equal((await post(port, {})).status, 200); + }); +}); + +test('honours an explicitly allowed Origin', async () => { + const options = { ...AUTH_OPTIONS, allowedOrigins: ['http://app.example'] }; + await withServer(options, async (port) => { + const allowed = await post(port, { ...bearer, Origin: 'http://app.example' }); + assert.equal(allowed.status, 200); + }); +}); diff --git a/src/server.ts b/src/server.ts index 1bb950b..ccf0b23 100644 --- a/src/server.ts +++ b/src/server.ts @@ -14,7 +14,8 @@ limitations under the License. */ -import express, { Express, Request, Response } from 'express'; +import { createHash, timingSafeEqual } from 'crypto'; +import express, { Express, NextFunction, Request, Response } from 'express'; import { StdioServerTransport } from '@modelcontextprotocol/sdk/server/stdio.js'; import { StreamableHTTPServerTransport } from '@modelcontextprotocol/sdk/server/streamableHttp.js'; import { McpServer as Server } from '@modelcontextprotocol/sdk/server/mcp.js'; @@ -33,6 +34,44 @@ import { logger } from './logger'; const STDIO_OPTION = 'stdio'; const STREAMABLE_HTTP_OPTION = 'http'; +// Loopback host names that are always trusted for DNS-rebinding Host validation. +const LOOPBACK_HOSTS = ['localhost', '127.0.0.1', '::1']; +// Wildcard bind addresses that do not correspond to a single trusted host name. +const WILDCARD_HOSTS = ['0.0.0.0', '::']; + +// JSON-RPC error codes used for transport-level rejections (implementation-defined +// server error range, per the JSON-RPC 2.0 spec). +const UNAUTHORIZED = -32001; +const FORBIDDEN = -32003; + +/** + * Options controlling the HTTP (streamable) transport. These have no effect on + * the stdio transport, which is only reachable by the local process that spawns it. + */ +export interface HttpTransportOptions { + /** Address to bind the HTTP listener to. Defaults to loopback (127.0.0.1). */ + host: string; + /** Bearer token required on every /mcp request. */ + authToken?: string; + /** Explicitly serve /mcp without authentication (insecure; opt-in only). */ + allowUnauthenticated: boolean; + /** Extra Host header values accepted in addition to the loopback/bind host. */ + allowedHosts: string[]; + /** Origin header values accepted from browser clients (empty = reject all). */ + allowedOrigins: string[]; +} + +/** + * Constant-time comparison of two secrets. Both sides are hashed first so the + * comparison does not leak their length and so timingSafeEqual never sees + * mismatched buffer sizes. + */ +function safeCompare(a: string, b: string): boolean { + const ah = createHash('sha256').update(a).digest(); + const bh = createHash('sha256').update(b).digest(); + return timingSafeEqual(ah, bh); +} + export class HubMCPServer { private readonly server: Server; private readonly assets: Asset[]; @@ -88,7 +127,11 @@ export class HubMCPServer { } } - async run(port: number, transportType: string): Promise { + async run( + port: number, + transportType: string, + httpOptions?: HttpTransportOptions + ): Promise { let transport = null; switch (transportType) { case STDIO_OPTION: @@ -97,18 +140,151 @@ export class HubMCPServer { logger.info('mcp server listening over stdio'); break; case STREAMABLE_HTTP_OPTION: { - const app = express(); - app.use(express.json()); - this.registerRoutes(app); - app.listen(port, () => { - logger.info(`mcp server listening on port ${port}`); + const options = httpOptions ?? { + host: '127.0.0.1', + allowUnauthenticated: false, + allowedHosts: [], + allowedOrigins: [], + }; + const app = this.buildHttpApp(options); + app.listen(port, options.host, () => { + logger.info(`mcp server listening on ${options.host}:${port}`); + if (options.allowUnauthenticated) { + logger.warn( + 'HTTP transport is running WITHOUT authentication ' + + '(--allow-unauthenticated). Any client able to reach ' + + `${options.host}:${port} can act as you on Docker Hub.` + ); + } + if (WILDCARD_HOSTS.includes(options.host)) { + logger.warn( + `HTTP transport is bound to ${options.host} and is reachable ` + + 'from the network. Ensure it is protected by authentication ' + + 'and network controls.' + ); + } }); break; } } } - private registerRoutes(app: Express) { + /** + * Builds the Express app for the HTTP transport, wiring the security guards and + * the /mcp routes. Enforces the fail-closed authentication requirement (RG-4626): + * the transport dispatches every tool under the operator's Docker Hub PAT, so it + * must not be served without a credential unless the operator explicitly opts out. + * + * Exposed (rather than inlined into run()) so it can be exercised by tests without + * binding a socket. + */ + buildHttpApp(options: HttpTransportOptions): Express { + if (!options.allowUnauthenticated && !options.authToken) { + throw new Error( + 'Refusing to start the HTTP transport without authentication. ' + + 'Set the MCP_AUTH_TOKEN environment variable so clients must present ' + + 'a bearer token, or pass --allow-unauthenticated to run without one ' + + '(NOT recommended: any client that can reach the port could act as ' + + 'you on Docker Hub using the server PAT).' + ); + } + const app = express(); + app.use(express.json()); + this.registerRoutes(app, options); + return app; + } + + /** + * Rejects browser-driven and off-host requests to defeat DNS rebinding. + * + * The rebinding threat is browser-based: a page the operator visits scripts a + * request to the local server. Such requests always carry an Origin header + * (the /mcp endpoint requires application/json, which is never a CORS "simple" + * request), so we reject any request bearing an Origin that is not explicitly + * allow-listed. We additionally validate the Host header against an allow-list + * so a rebound attacker domain (whose Host would not match) is refused. Genuine + * MCP clients send no Origin and a loopback Host, so they are unaffected. + */ + private dnsRebindingGuard(options: HttpTransportOptions) { + const allowedHosts = new Set([ + ...LOOPBACK_HOSTS, + ...options.allowedHosts.map((host) => host.toLowerCase()), + ]); + if (options.host && !WILDCARD_HOSTS.includes(options.host)) { + allowedHosts.add(options.host.toLowerCase()); + } + const allowedOrigins = new Set( + options.allowedOrigins.map((origin) => origin.toLowerCase()) + ); + + return (req: Request, res: Response, next: NextFunction): void => { + const origin = this.headerValue(req.headers['origin']); + if (origin && !allowedOrigins.has(origin.toLowerCase())) { + logger.warn(`rejected request with disallowed origin: ${origin}`); + this.rejectRequest(res, 403, FORBIDDEN, 'Origin not allowed'); + return; + } + + const hostname = this.parseHostname(this.headerValue(req.headers['host'])); + if (!hostname || !allowedHosts.has(hostname.toLowerCase())) { + logger.warn(`rejected request with disallowed host: ${hostname ?? ''}`); + this.rejectRequest(res, 403, FORBIDDEN, 'Host not allowed'); + return; + } + + next(); + }; + } + + /** Requires a valid bearer token unless authentication is explicitly disabled. */ + private authGuard(options: HttpTransportOptions) { + return (req: Request, res: Response, next: NextFunction): void => { + if (options.allowUnauthenticated) { + next(); + return; + } + + const header = this.headerValue(req.headers['authorization'])?.trim() ?? ''; + const match = /^Bearer\s+(.+)$/i.exec(header); + if (!options.authToken || !match || !safeCompare(match[1], options.authToken)) { + this.rejectRequest(res, 401, UNAUTHORIZED, 'Unauthorized'); + return; + } + + next(); + }; + } + + private headerValue(value: string | string[] | undefined): string | undefined { + return Array.isArray(value) ? value[0] : value; + } + + /** Extracts the hostname from a Host header, stripping any port and IPv6 brackets. */ + private parseHostname(hostHeader?: string): string | undefined { + if (!hostHeader) { + return undefined; + } + if (hostHeader.startsWith('[')) { + const end = hostHeader.indexOf(']'); + return end === -1 ? undefined : hostHeader.slice(1, end); + } + const colon = hostHeader.indexOf(':'); + return colon === -1 ? hostHeader : hostHeader.slice(0, colon); + } + + private rejectRequest(res: Response, status: number, code: number, message: string) { + if (!res.headersSent) { + res.status(status).json({ + jsonrpc: JSONRPC_VERSION, + error: { code, message }, + id: null, + }); + } + } + + private registerRoutes(app: Express, options: HttpTransportOptions) { + app.use('/mcp', this.dnsRebindingGuard(options), this.authGuard(options)); + app.post('/mcp', async (req: Request, res: Response) => { const sanitizedBody = JSON.stringify(req.body).replace(/\n|\r/g, ''); logger.info(`received mcp request: ${sanitizedBody}`); diff --git a/tsconfig.test.json b/tsconfig.test.json new file mode 100644 index 0000000..487973e --- /dev/null +++ b/tsconfig.test.json @@ -0,0 +1,9 @@ +{ + "extends": "./tsconfig.json", + "compilerOptions": { + "outDir": "./dist-test", + "noEmit": false + }, + "include": ["src/**/*"], + "exclude": ["node_modules", "dist", "dist-test"] +} From c84d09e11be030e1dc49a0dfae6ff977020757a0 Mon Sep 17 00:00:00 2001 From: Luca Druda Date: Wed, 29 Jul 2026 11:44:16 +0200 Subject: [PATCH 2/3] fix(http): address CodeQL findings on the transport guards - 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) --- src/server.ts | 37 ++++++++++++++++++++++++++++++++----- 1 file changed, 32 insertions(+), 5 deletions(-) diff --git a/src/server.ts b/src/server.ts index ccf0b23..e784af3 100644 --- a/src/server.ts +++ b/src/server.ts @@ -220,14 +220,18 @@ export class HubMCPServer { return (req: Request, res: Response, next: NextFunction): void => { const origin = this.headerValue(req.headers['origin']); if (origin && !allowedOrigins.has(origin.toLowerCase())) { - logger.warn(`rejected request with disallowed origin: ${origin}`); + logger.warn( + `rejected request with disallowed origin: ${this.sanitizeForLog(origin)}` + ); this.rejectRequest(res, 403, FORBIDDEN, 'Origin not allowed'); return; } const hostname = this.parseHostname(this.headerValue(req.headers['host'])); if (!hostname || !allowedHosts.has(hostname.toLowerCase())) { - logger.warn(`rejected request with disallowed host: ${hostname ?? ''}`); + logger.warn( + `rejected request with disallowed host: ${this.sanitizeForLog(hostname ?? '')}` + ); this.rejectRequest(res, 403, FORBIDDEN, 'Host not allowed'); return; } @@ -244,9 +248,8 @@ export class HubMCPServer { return; } - const header = this.headerValue(req.headers['authorization'])?.trim() ?? ''; - const match = /^Bearer\s+(.+)$/i.exec(header); - if (!options.authToken || !match || !safeCompare(match[1], options.authToken)) { + const token = this.parseBearerToken(this.headerValue(req.headers['authorization'])); + if (!options.authToken || !token || !safeCompare(token, options.authToken)) { this.rejectRequest(res, 401, UNAUTHORIZED, 'Unauthorized'); return; } @@ -259,6 +262,30 @@ export class HubMCPServer { return Array.isArray(value) ? value[0] : value; } + /** + * Extracts the token from an `Authorization: Bearer ` header. + * Parsed with indexOf/slice rather than a regex so an attacker-controlled + * header value cannot trigger catastrophic backtracking (ReDoS). + */ + private parseBearerToken(header?: string): string | undefined { + const value = header?.trim() ?? ''; + const space = value.indexOf(' '); + if (space === -1 || value.slice(0, space).toLowerCase() !== 'bearer') { + return undefined; + } + const token = value.slice(space + 1).trim(); + return token.length > 0 ? token : undefined; + } + + /** + * Strips CR/LF and other control characters so an attacker-controlled header + * value cannot forge or split log entries (log injection). + */ + private sanitizeForLog(value: string): string { + // eslint-disable-next-line no-control-regex + return value.replace(/[\u0000-\u001f\u007f]/g, ''); + } + /** Extracts the hostname from a Host header, stripping any port and IPv6 brackets. */ private parseHostname(hostHeader?: string): string | undefined { if (!hostHeader) { From 5392a3683677ba54245d7fb29515b57f3ed68485 Mon Sep 17 00:00:00 2001 From: Luca Druda Date: Wed, 29 Jul 2026 11:52:54 +0200 Subject: [PATCH 3/3] fix(http): use a CodeQL-recognized log-injection barrier 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) --- src/server.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/server.ts b/src/server.ts index e784af3..4eb8ad0 100644 --- a/src/server.ts +++ b/src/server.ts @@ -283,7 +283,7 @@ export class HubMCPServer { */ private sanitizeForLog(value: string): string { // eslint-disable-next-line no-control-regex - return value.replace(/[\u0000-\u001f\u007f]/g, ''); + return value.replace(/\n|\r/g, '').replace(/[\u0000-\u001f\u007f]/g, ''); } /** Extracts the hostname from a Host header, stripping any port and IPv6 brackets. */