Conversation
|
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
…extprotocol/typescript-sdk into middleware-remove-console-warn
|
@claude review |
There was a problem hiding this comment.
The previously flagged integration test bug has been fixed. The implementation looks correct, but this PR adds a new option to bypass DNS rebinding protection, so a human should sign off on the API design.
Extended reasoning...
Overview
This PR modifies the Express and Hono middleware packages to (1) remove console.warn log pollution when binding to 0.0.0.0/:: without allowedHosts, and (2) add a new skipHostHeaderValidation option using a discriminated union type that makes allowedHosts and skipHostHeaderValidation: true mutually exclusive at the type level. Five files are changed across both middleware packages and integration tests.
Security risks
The core change introduces an explicit opt-in to disable DNS rebinding protection entirely via skipHostHeaderValidation: true. While the implementation is correct (requires explicit opt-in, default behavior unchanged, clean type constraints), this is a new security-relevant API surface. A human reviewer should validate that this escape hatch is appropriate and that the documentation/naming adequately communicates the security implications.
Level of scrutiny
This warrants careful human review. Although the code is well-structured and the discriminated union type prevents misconfiguration at compile time, the PR touches security middleware (host header validation / DNS rebinding protection). The interface-to-type change is also technically a minor breaking change for anyone extending the options interface. A missing changeset was also flagged by the changeset bot.
Other factors
The author addressed the integration test bug I previously flagged — the old console.warn assertion tests are now replaced with proper HTTP-level supertest assertions. The nit about Express unit tests only checking toBeDefined() is mitigated by the new integration tests which do verify HTTP behavior with supertest. Test coverage is now solid across both packages.
Remove console.warn log pollution and add
skipHostHeaderValidationoptionMotivation and Context
Fixes #1515. The middleware packages (
@modelcontextprotocol/express,@modelcontextprotocol/hono) emit aconsole.warnwhen binding to0.0.0.0or::withoutallowedHosts. This pollutes server logs for users who intentionally bind to all interfaces (e.g., behind a reverse proxy in a container). Libraries should not produce unsolicited log output in environments they don't own.Additionally, there was no way to opt out of the automatic localhost host-header validation — useful when running behind a reverse proxy that rewrites the
Hostheader.This PR:
console.warnentirely. TheallowedHostsoption is already well-documented in JSDoc.skipHostHeaderValidation— an explicit opt-out from all automatic host-header validation middleware.HostHeaderValidationOptions) so thatallowedHostsandskipHostHeaderValidation: trueare mutually exclusive at the type level — passing both is a compile error, not a silent precedence rule.How Has This Been Tested?
@modelcontextprotocol/expressand@modelcontextprotocol/honoskipHostHeaderValidation: trueon both localhost and0.0.0.0hostsHostheaders succeed)Breaking Changes
console.warnwhen binding to0.0.0.0/::withoutallowedHostsis removed. This is a behavior change but not a breaking API change — no user code depended on this warning.CreateMcpExpressAppOptionsandCreateMcpHonoAppOptionschanged frominterfacetotype(intersection withHostHeaderValidationOptions). This is source-compatible for all usage patterns exceptextends/implementson the interface, which is unlikely for options objects.Types of changes
Checklist
Additional context
The
HostHeaderValidationOptionsdiscriminated union type is defined independently in both packages (no shared import) to avoid coupling the middleware packages. The pattern is identical:This ensures that
allowedHostsis only accepted when validation is enabled, catching misconfiguration at compile time rather than silently ignoring one option.