Repository navigation
feat(dns-rebinding): check Host and Origin validation separately - #575
Open
ninadphalak wants to merge 3 commits into
Open
ninadphalak wants to merge 3 commits into
ninadphalak wants to merge 3 commits into
Conversation
The echo tool declared inputSchema as a plain JSON Schema object, which @modelcontextprotocol/sdk 1.29 rejects, so the fixture answered every request with HTTP 500. The dns-rebinding-protection negative test passed on that 500 instead of on the missing protection. With z.string() the fixture serves the forged request with 200, as intended.
localhost-host-rebinding-rejected forges Host and Origin in one request
and accepts any 4xx, so a server that validates only one of the headers
passes. Add two checks to the same scenario:
- localhost-host-only-rebinding-rejected: non-localhost Host, no Origin,
expects any 4xx. No spec text requires Host validation, so a miss is
WARNING.
- localhost-origin-only-rebinding-rejected: localhost Host, non-localhost
Origin, expects exactly 403 per the Streamable HTTP transport ("If the
Origin header is present and invalid, servers MUST respond with HTTP
403 Forbidden"). A miss is FAILURE.
The combined check and localhost-host-valid-accepted are unchanged. The
expected status and both miss severities sit in one block so they can
follow modelcontextprotocol/modelcontextprotocol#3370.
The everything-server now returns 403 for a present, non-localhost
Origin; createMcpExpressApp() validates Host only. Two negative fixtures,
dns-rebinding-host-only.ts and dns-rebinding-origin-only.ts, show each
partial setup passing the combined check and failing its split check.
The Host-only and Origin-only checks pass when the forged request is rejected, so a server that rejects every request passed both without validating either header. Send a control request first (localhost Host, no Origin). If the server does not answer it with 2xx, report both checks as not testable via untestableCheck(), at their usual severity (Host-only WARNING, Origin-only FAILURE). The combined check and localhost-host-valid-accepted are unchanged. Adds a negative test with a server that answers 403 to everything. Control request idea from @JosephDoUrden in modelcontextprotocol#535.
This branch has not been deployed
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.
Closes #535.
localhost-host-rebinding-rejectedforgesHostandOriginin the same request and accepts any 4xx, so a server that validates only one of the two headers passes. This adds two checks to thedns-rebinding-protectionscenario so each header is tested on its own:localhost-host-only-rebinding-rejectedHost, noOriginWARNING(no spec text requires Host validation)localhost-origin-only-rebinding-rejectedHost, non-localhostOriginFAILURE(Streamable HTTP: "If the Origin header is present and invalid, servers MUST respond with HTTP 403 Forbidden")localhost-host-rebinding-rejectedandlocalhost-host-valid-acceptedare unchanged.Examples
Origin(createMcpExpressApp()validatesHostonly).dns-rebinding-host-only.tsanddns-rebinding-origin-only.ts. Each passes the combined check and fails its own split check;negative.test.tsasserts both.no-dns-rebinding-protection.tsused an invalidinputSchemaand returned 500; it now uses a zod schema (separate commit).Spec follow-up. modelcontextprotocol/modelcontextprotocol#3370 proposes rewording Host/Origin validation (Host for DNS rebinding, Origin for CSRF). It has no maintainer reply yet. The expected Origin status and both miss severities sit in one block in
dns-rebinding.ts, so they can follow #3370 with a small change if it lands.Checked
npm run build,npm run check(typecheck + lint) andnpm test(643 tests) pass, also with the branch rebased on currentmain(no conflicts).