Conversation
|
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| // If allowedHosts is explicitly provided, use that for validation | ||
| if (allowedHosts) { | ||
| app.use(hostHeaderValidation(allowedHosts)); | ||
| } else { | ||
| // Apply DNS rebinding protection automatically for localhost hosts | ||
| const localhostHosts = ['127.0.0.1', 'localhost', '::1']; | ||
| if (localhostHosts.includes(host)) { | ||
| app.use(localhostHostValidation()); | ||
| } else if (host === '0.0.0.0' || host === '::') { | ||
| // Warn when binding to all interfaces without DNS rebinding protection | ||
| // eslint-disable-next-line no-console | ||
| console.warn( | ||
| `Warning: Server is binding to ${host} without DNS rebinding protection. ` + | ||
| 'Consider using the allowedHosts option to restrict allowed hosts, ' + | ||
| 'or use authentication to protect your server.' | ||
| ); | ||
| if (!skipHostHeaderValidation) { | ||
| // If allowedHosts is explicitly provided, use that for validation | ||
| if (allowedHosts) { | ||
| app.use(hostHeaderValidation(allowedHosts)); | ||
| } else { | ||
| // Apply DNS rebinding protection automatically for localhost hosts | ||
| const localhostHosts = ['127.0.0.1', 'localhost', '::1']; | ||
| if (localhostHosts.includes(host)) { | ||
| app.use(localhostHostValidation()); |
There was a problem hiding this comment.
🔴 Integration tests in test/integration/test/server.test.ts (lines 2311-2322) still assert that console.warn is called when binding to 0.0.0.0 or :: without allowedHosts, but this PR removes those console.warn calls. These two tests will fail, breaking CI. They need to be removed or replaced with tests for the new skipHostHeaderValidation behavior.
Extended reasoning...
What the bug is
This PR removes the console.warn calls from createMcpExpressApp (in packages/middleware/express/src/express.ts) and createMcpHonoApp (in packages/middleware/hono/src/hono.ts) that previously fired when binding to 0.0.0.0 or :: without providing allowedHosts. However, the integration test file test/integration/test/server.test.ts was not updated to reflect this change.
The specific failing tests
Two integration tests will fail:
-
Line 2311: "should warn when binding to 0.0.0.0" — creates a console.warn spy, calls createMcpExpressApp({ host: "0.0.0.0" }), then asserts expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("0.0.0.0")). Since the console.warn call no longer exists in the production code, the spy will never be called, and this assertion will fail.
-
Line 2318: "should warn when binding to :: (IPv6 all interfaces)" — identical pattern with host: "::" and expect.stringContaining("::"). Same failure for the same reason.
Step-by-step proof
- Before this PR, createMcpExpressApp({ host: "0.0.0.0" }) would hit the else-if branch (host === "0.0.0.0" || host === "::") at the old line 93, which called console.warn(...).
- After this PR, that entire else-if branch is removed. When host is "0.0.0.0" and no allowedHosts is provided, the code simply falls through without any warning — the localhostHosts.includes(host) check fails (since "0.0.0.0" is not in ["127.0.0.1", "localhost", "::1"]), and no other action is taken.
- The integration test at line 2311 spies on console.warn and asserts it was called. Since console.warn is never invoked, the spy records zero calls, and toHaveBeenCalledWith(...) fails.
- The same applies to the test at line 2318 for host: "::".
Why existing code does not prevent it
The PR correctly updated the unit tests in packages/middleware/express/test/express.test.ts and packages/middleware/hono/test/hono.test.ts — both had their warn-related tests removed and replaced with skipHostHeaderValidation tests. However, the integration tests in test/integration/test/server.test.ts were missed. This file is not listed among the 4 changed files in the PR.
Impact
This will cause CI test failures when the integration test suite runs. Additionally, the test at line 2326 that creates a warnSpy for the allowedHosts test and asserts expect(warnSpy).not.toHaveBeenCalled() will pass vacuously (since the warn is gone), but the spy setup is now unnecessary dead code.
How to fix
Remove the two failing tests (lines 2311-2323) from test/integration/test/server.test.ts. Optionally, add integration tests for the new skipHostHeaderValidation behavior. The warnSpy at line 2326 can also be cleaned up since console.warn is no longer called.
…extprotocol/typescript-sdk into middleware-remove-console-warn
1 participant
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.