Repository navigation
Fix reflected XSS in the MCP docs page - #50
Merged
Merged
Conversation
gregsaab
reviewed
Sep 1, 2026
gregsaab
reviewed
Sep 1, 2026
gregsaab
previously approved these changes
Sep 1, 2026
gregsaab
left a comment
Contributor
There was a problem hiding this comment.
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
force-pushed
the
murrayju/fix-docs-page-xss
branch
from
September 1, 2026 15:17
fb63f03 to
c6f5c6e
Compare
gregsaab
approved these changes
Sep 1, 2026
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a reflected XSS in the HTML docs page served by
mcpRouterFactory(GETon the MCP mount path withAccept: text/html).The page built a
fullUrlfrom client-controlled headers and interpolated it into the response with no escaping:X-Forwarded-ProtoX-Forwarded-HostX-Original-Request-Uri/X-Original-UriThe
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, byencodeURIComponent.Reproduction against an affected server:
Changes
src/http/mcp.tsescape-htmlpackage: the server name,req.get('host'),fullUrl, and the inspectorhref.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)X-Original-Request-UriandX-Forwarded-Proto.package.json/bun.lockescape-htmltodependenciesand@types/escape-htmltodevDependencies.escape-htmlwas already in the tree through express, but a direct import needs a direct declaration.bunfig.tomlroot = "src"under[test]. Without it, a barebun testalso 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-Policyheader 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
./checkpasses (tsc, biome, tests).src/http/mcp.tsis reverted, and pass with the fix.fullUrlis 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.