Skip to content

Fix reflected XSS in the MCP docs page - #50

Merged
murrayju merged 2 commits into
mainfrom
murrayju/fix-docs-page-xss
Sep 1, 2026
Merged

murrayju merged 2 commits into
mainfrom
murrayju/fix-docs-page-xss

Conversation

@murrayju

@murrayju murrayju commented Sep 1, 2026 •

Copy link
Copy Markdown
Member

Summary

Fixes a reflected XSS in the HTML docs page served by mcpRouterFactory (GET on the MCP mount path with Accept: text/html).

The page built a fullUrl from client-controlled headers and interpolated it into the response with no escaping:

  • X-Forwarded-Proto
  • X-Forwarded-Host
  • X-Original-Request-Uri / X-Original-Uri

The claude mcp add ... line had no escaping at all, so a crafted header could close the <pre><code> block early and land live markup in the page. The sibling inspector link was protected only incidentally, by encodeURIComponent.

Reproduction against an affected server:

curl -s "https://<host>/mcp" -H "Accept: text/html" \
  -H 'X-Forwarded-Host: INJECTED</code></pre><img src=x onerror=alert(document.domain)>'

Changes

src/http/mcp.ts

  • Escape every interpolated value in the docs page with the escape-html package: the server name, req.get('host'), fullUrl, and the inspector href.
  • Add firstHeaderValue() so a repeated header yields the first value instead of a comma-joined list. This is a small correctness bug in the same code.

src/http/mcp.spec.ts (new)

  • Regression tests that request the docs page with the reporter's exact payload, plus injections through X-Original-Request-Uri and X-Forwarded-Proto.
  • Each test asserts both that the raw payload is absent and that the escaped payload is present. The positive assertion means a test cannot pass by accident if the value is dropped instead of escaped.

package.json / bun.lock

  • Add escape-html to dependencies and @types/escape-html to devDependencies. escape-html was already in the tree through express, but a direct import needs a direct declaration.

bunfig.toml

  • Set root = "src" under [test]. Without it, a bare bun test also collected the compiled specs in ./dist, which fail because the skills fixtures resolve relative to the source tree.

Scope

This change escapes the output. It does not stop the server from trusting the forwarded headers, because those headers exist so the page can print the externally visible URL when it runs behind the ingress. With escaping in place, the remaining effect is that a self-directed request can show a misleading URL in its own response body: no script runs, and there is no stored or cross-user impact. Host allow-listing for the rendered URL and a Content-Security-Policy header on this response are reasonable follow-ups, kept out of this PR to keep it focused.

Downstream servers (including pg-aiguide) need a version bump after this is released.

Verification

  • ./check passes (tsc, biome, tests).
  • The three tests fail when the escaping in src/http/mcp.ts is reverted, and pass with the fix.
  • The three tests also fail when fullUrl is replaced with a constant, which confirms that the new positive assertions test the escaped value and not just the absence of the payload.

Credit

Reported by Syed Anas Mohiuddin, independent security researcher.

@murrayju
murrayju requested a review from a team as a code owner September 1, 2026 14:22
Comment thread src/http/mcp.spec.ts Resolved
Comment thread src/http/mcp.spec.ts Resolved
gregsaab
gregsaab previously approved these changes Sep 1, 2026

@gregsaab gregsaab left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, just test suggestion

The HTML docs page put the values of client-controlled headers into the
response without escaping. A request with a crafted X-Forwarded-Host,
X-Forwarded-Proto, X-Original-Request-Uri, or X-Original-Uri header could
break out of the <pre><code> block and inject live markup.

Escape every interpolated value with the escape-html package. Also take the
first value when a header repeats, so the page does not print a comma-joined
list.

Set `root = "src"` for bun test, so that `bun test` does not also collect the
compiled specs in ./dist.

Reported by Syed Anas Mohiuddin, independent security researcher.
The two tests only checked that the raw payload was absent. Also check that
the escaped payload is present, like the forwarded host test does. This
proves that the value still reaches the page, so the tests cannot pass by
accident if the value is dropped instead of escaped.
@murrayju
murrayju merged commit 1d6cb4e into main Sep 1, 2026
3 checks passed
@murrayju
murrayju deleted the murrayju/fix-docs-page-xss branch September 1, 2026 15:43
murrayju added a commit to timescale/pg-aiguide that referenced this pull request Sep 1, 2026
## Summary

Routine dependency refresh. The most important part is the
`@tigerdata/mcp-boilerplate` bump to `1.6.5`, which pulls in a reflected
XSS fix that affects this server.

## Security fix: reflected XSS in the HTML docs page

`@tigerdata/mcp-boilerplate` `1.6.4` → `1.6.5` picks up
[timescale/mcp-boilerplate-node#50](timescale/mcp-boilerplate-node#50).

The HTML docs page served by `mcpRouterFactory` (`GET` on the MCP mount
path with `Accept: text/html`) built its displayed URL from
client-controlled headers (`X-Forwarded-Proto`, `X-Forwarded-Host`,
`X-Original-Request-Uri`, `X-Original-Uri`) and interpolated them into
the response with no escaping. A crafted header could close the
`<pre><code>` block early and land live markup in the page.

This repo is affected: `src/httpServer.ts` uses `httpServerFactory`, so
it serves that docs page. The upstream fix escapes every interpolated
value with `escape-html` and adds `firstHeaderValue()` so a repeated
header yields the first value instead of a comma-joined list. Verified
the escaping is present in the installed `1.6.5`.

Per the upstream PR, real-world impact was limited: the forwarded
headers are trusted by design so the page can print the externally
visible URL behind the ingress, and the remaining effect is that a
self-directed request can show a misleading URL in its own response
body. No stored or cross-user impact. Upstream flagged host
allow-listing and a CSP header on that response as reasonable
follow-ups.

## Dependency updates

| Package | From | To |
|---|---|---|
| `@tigerdata/mcp-boilerplate` | 1.6.4 | 1.6.5 |
| `ai` | 7.0.2 | 7.0.87 |
| `@ai-sdk/openai` | 4.0.0 | 4.0.53 |
| `zod` | ^4.3.6 | ^4.5.4 |
| `pg` | ^8.22.0 | ^8.23.0 |
| `@biomejs/biome` | ^2.5.1 | ^2.5.11 |
| `@types/bun` | ^1.3.14 | ^1.4.0 |
| `@types/node` | ^24.13.2 | ^24.13.3 |
| `@types/pg` | ^8.20.0 | ^8.23.1 |
| `@types/semver` | ^7.7.1 | ^7.8.0 |

Also bumps bun `1.3.14` → `1.4.0` (the `./bun` launcher and the
`oven/bun` base image in `Dockerfile`, kept in sync).

Deliberately held back:

- `typescript` stays at `6.0.3`. That is the latest stable 6.x; `7.0.2`
is a major.
- `@types/node` stays within 24.x to match the Node 24 target. Latest is
`26.4.0`.

## Breaking changes reviewed

**zod 4.4.0 was the only release with real risk.** It shipped a set of
"potentially breaking bug fixes". Two were relevant here:

- **Floating-point `multipleOf`.** This directly affects
`semanticWeight: z.number().multipleOf(0.1)` in
`src/apis/searchDocs.ts`. No existing test covers non-trivial decimals,
so this was verified by hand rather than trusted to the suite. The fix
works in our favor: `0.3` and `0.7` (the classic float-precision failure
cases, and `0.7` is our documented default) now validate correctly,
while `0.15` and `0.35` still correctly reject.
- **JSON Schema generation.** Confirmed the emitted `inputSchema` and
`outputSchema` are unchanged, so there is no MCP client contract change.
Checked against a live `tools/list` response.

The other 4.4.0 fixes (tuple defaults, `.merge()` refinements, stricter
string validators, record key transforms) do not apply. This code only
uses `z.enum`, `z.string`, `z.number`, `z.coerce.number`, `z.object`,
`z.array`, and `z.union`.

**AI SDK:** reviewed the full 7.x changelog range. The `embed` /
`openai.embedding` surface used by `embedQueryJson` is unchanged. The
one embedding-related change splits OpenAI requests by a UTF-8 byte
budget, which is internal batching and only affects `embedMany`, not our
single-value `embed`.

## Verification

- `./check` passes.
- Full `./bun test` run: 86/86 pass. Note that `./check` runs tests with
`--only-failures`, so a green `./check` alone does not prove the suite
ran; it was run separately.
- `./bun run build` succeeds.
- Smoke-tested the stdio server end to end: migrations run, `initialize`
succeeds, and both `search_docs` and `view_skill` list with the expected
schemas.
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