feat(protocol): serve modern MCP with SDK v2 - #255
Merged
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| BestPractice | 1 medium |
| Security | 1 medium |
🟢 Metrics 151 complexity · 14 duplication
Metric Results Complexity 151 Duplication 14
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR migrates the server from the monolithic @modelcontextprotocol/sdk to the split MCP SDK v2 packages and adds support for the modern 2026-07-28 protocol over both HTTP and STDIO, while preserving legacy transport behavior, contracts, and security/operational bounds.
Changes:
- Switched MCP imports to
@modelcontextprotocol/{server,node,client,core}and updated dependency/runtime verification to forbid the legacy monolithic SDK. - Reworked HTTP + STDIO serving to support modern sessionless requests (official handler) alongside retained legacy stateful/stateless behavior.
- Added request-scoped modern logging bridging (AsyncLocalStorage) and updated docs/tests to lock in protocol/tool/resource contracts.
Reviewed changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/url-reader.ts | Updates MCP server import to SDK v2 split package. |
| src/types.ts | Switches Tool type import to @modelcontextprotocol/server. |
| src/suggestions.ts | Updates MCP server import to SDK v2 split package. |
| src/searxng-response.ts | Updates MCP server import to SDK v2 split package. |
| src/search.ts | Updates MCP server import to SDK v2 split package. |
| src/resources.ts | Updates McpServer type import to SDK v2 split package. |
| src/logging.ts | Adds modern request-scoped logging bridge and modern/legacy routing behavior. |
| src/instance-info.ts | Updates MCP server import to SDK v2 split package. |
| src/index.ts | Rebuilds server registration/dispatch for SDK v2 and wires modern vs legacy eras for STDIO/HTTP. |
| src/http-server.ts | Migrates to Node SDK v2 HTTP transport + handler; adds modern request path + compatibility guard. |
| src/browser-solver.ts | Updates McpServer type import to SDK v2 split package. |
| SECURITY.md | Documents modern vs legacy protocol matrix and modern header requirement/guard behavior. |
| scripts/verify-packed-consumer.mjs | Updates verifier output to report split SDK runtime versions. |
| scripts/packed-consumer-contracts.mjs | Enforces exact split SDK runtime dependency set and forbids monolithic SDK. |
| README.md | Updates transport docs to describe modern protocol support and legacy retention. |
| package.json | Replaces monolithic SDK dependency with split v2 packages; adds zod@4.2.0 and v2 client devDependency. |
| package-lock.json | Locks dependency graph to split v2 packages and zod@4.2.0; removes monolithic SDK and related deps. |
| CONFIGURATION.md | Documents modern/legacy transport behavior and modern header expectations. |
| tests/unit/packed-consumer.test.ts | Updates dependency-contract expectations for split SDK runtime. |
| tests/unit/packed-consumer-fixtures.ts | Updates fixtures to model split SDK runtime dependency tree. |
| tests/unit/logging.test.ts | Adds tests for modern request-scoped logging behavior and fail-closed behavior outside scope. |
| tests/integration/mcp-handlers.test.ts | Updates integration tests to v2 client and locks in tool/resource structural contracts. |
| tests/integration/http-server.test.ts | Adds modern HTTP handler coverage, header-guard coverage, and capacity bounds validation. |
| tests/integration/diagnostic-security.test.ts | Updates to v2 client and ensures diagnostics remain credential-safe. |
| tests/e2e/url-reader.e2e.ts | Adjusts assertions to handle v2-era tool error vs thrown error shapes. |
| tests/e2e/timeout.e2e.ts | Extends e2e coverage for modern STDIO discovery and updated admission behavior. |
| tests/e2e/http-transport.e2e.ts | Updates e2e HTTP transport client to v2 package. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
431
to
+434
|
|
||
| if (stateless.enabled) { | ||
| if (rejectInvalidStatelessHeaders(req, res)) { | ||
| if (rejectInvalidStatelessHeaders(req, res)) return; | ||
|
|
||
| if (!req.is("application/json")) { |
Owner
Author
There was a problem hiding this comment.
Addressed in bfe2e2a — verified valid. Renamed the helper to rejectInvalidHostHeader so its name matches its actual Host-only responsibility.
Owner
Author
|
Review findings handled in bfe2e2a:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 27 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/index.ts:352
logging/setLevelhandler acceptsrequest.params.levelwithout runtime validation. SincesetLogLevel()stores whatever string is provided, an invalid level will makeshouldLog()comparisons useindexOf(...) === -1, which can effectively enable logging for all levels (or otherwise break filtering). Consider validatinglevelagainst the allowedLOG_LEVELSset before callingsetLogLevel, and reject invalid values with a sanitized error response.
mcpServer.server.setRequestHandler("logging/setLevel", async (request, context) => {
const callback = async () => {
logMessage(mcpServer, "info", `Setting log level to: ${request.params.level}`);
setLogLevel(mcpServer, request.params.level);
return {};
Owner
Author
|
Follow-up review findings handled in 91b8b28:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 27 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/http-server.ts:307
- The Host/DNS-rebinding rejection response reflects the raw
Hostheader value back to the client (Invalid Host header: ${host}). SinceHostis attacker-controlled, prefer a fixed/non-reflecting error message (and log the received value via diagnostics instead) to avoid echoing untrusted input in responses and to align with the non-reflecting Origin/auth rejection pattern used elsewhere in this handler.
function rejectInvalidHostHeader(
req: express.Request,
res: express.Response,
): boolean {
if (!security.enableDnsRebindingProtection) {
return false;
}
const host = req.headers.host;
if (security.allowedHosts.length > 0 && (!host || !security.allowedHosts.includes(host))) {
res.status(403).json({
jsonrpc: "2.0",
error: { code: -32000, message: `Invalid Host header: ${host}` },
id: null,
});
Owner
Author
|
Latest review finding handled in f1b0c4d:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 27 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/http-server.ts:460
- Modern HTTP requests are sessionless, but when MCP_HTTP_STATELESS is false the shared postRateLimiter earlier in this route still chooses initLimiter whenever there is no live mcp-session-id. That means normal modern traffic will be throttled by MCP_RATE_INIT_MAX (and MCP_RATE_SESSION_MAX becomes effectively unused for modern clients). Consider updating the POST /mcp rate-limiter selection to treat modern requests (e.g., based on MCP-Protocol-Version/MCP-Method headers or a light _meta/protocolVersion check) as sessionLimiter traffic.
const modern = !(await isLegacyRequest(await toWebRequest(req, req.body), req.body));
if (modern) {
const headerError = missingModernProtocolHeaderError(req.headers, req.body);
if (headerError) {
res.status(400).json({ jsonrpc: "2.0", ...headerError });
Owner
Author
|
Review follow-up for this head:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 27 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/http-server.ts:460
- In stateful HTTP mode (MCP_HTTP_STATELESS is false), modern requests are classified only after the POST rate limiter runs. That limiter currently upgrades any request that presents a live legacy
mcp-session-idto the more generous session bucket. A modern (2026-07-28) request is sessionless and should not be able to select the session limiter via a legacy session ID; this contradicts the documented behavior that only stateful POSTs with a live session use the session limit (CONFIGURATION.md Rate Limiting section). Consider ensuring modern requests always use the init bucket in stateful mode (e.g., ignoremcp-session-idwhen the body claims 2026-07-28, or classify before choosing the limiter).
const modern = !(await isLegacyRequest(await toWebRequest(req, req.body), req.body));
if (modern) {
const headerError = missingModernProtocolHeaderError(req.headers, req.body);
if (headerError) {
res.status(400).json({ jsonrpc: "2.0", ...headerError });
Owner
Author
|
Review follow-up for this head:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 27 changed files in this pull request and generated no new comments.
Suppressed comments (2)
SECURITY.md:183
- This paragraph states that capacity controls run only with
MCP_HTTP_STATELESS=true, but modern HTTP requests are always handled as per-request (sessionless) and the server enforces stateless capacity/timeouts for them regardless of that flag. Consider rewording to reflect that modern requests are always subject toMCP_HTTP_STATELESS_MAX_IN_FLIGHT(_PER_IP)andMCP_HTTP_STATELESS_REQUEST_TIMEOUT_MS, while legacy requests only opt into per-request servers whenMCP_HTTP_STATELESS=true.
With `MCP_HTTP_STATELESS=true`, bearer authorization and the hardened Host and Origin checks run before any per-request MCP server is constructed. Rate limiting also runs before construction, followed by the stateless capacity controls. Modern and legacy stateless requests share that capacity, while `/health` and MCP control traffic remain available during tool-admission exhaustion.
HTTP and STDIO support modern `2026-07-28` and legacy `2025-11-25`, `2025-06-18`, `2025-03-26`, `2024-11-05`, and `2024-10-07`. Modern HTTP is sessionless POST-only; legacy stateful and stateless behavior retains the documented session and 405 boundaries.
CONFIGURATION.md:318
- The text implies stateless capacity controls only run when
MCP_HTTP_STATELESS=true, but modern HTTP requests are always sessionless and the implementation appliesadmitStatelessRequest()andMCP_HTTP_STATELESS_REQUEST_TIMEOUT_MSto modern requests regardless of that flag (seesrc/http-server.tsmodern branch). Please clarify that theMCP_HTTP_STATELESS_*limits/timeouts also bound modern HTTP requests even when legacy stateless mode is disabled, or adjust the code/docs to match the intended behavior.
By default the server communicates over STDIO. Set `MCP_HTTP_PORT` to enable HTTP mode instead. The SDK v2 server accepts modern MCP requests and retains legacy compatibility; modern HTTP clients should send the negotiated `MCP-Protocol-Version` header.
Both transports support modern `2026-07-28` plus legacy `2025-11-25`, `2025-06-18`, `2025-03-26`, `2024-11-05`, and `2024-10-07`. Modern HTTP requests are sessionless POSTs; legacy HTTP requests retain the stateful default or the configured legacy stateless mode.
The published server SDK `2.0.0` has a temporary compatibility guard for a 2026-07-28 request that omits that header: it returns the standard HTTP 400 HeaderMismatch response. The guard will be removed only after upgrading to a stable SDK containing upstream PR 2594 and proving that the SDK itself returns the same response.
Owner
Author
|
Review follow-up for this head:
|
Owner
Author
|
Review follow-up for this head:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 27 changed files in this pull request and generated no new comments.
Suppressed comments (1)
scripts/packed-consumer-contracts.mjs:7
- The file-level
eslint-disable security/detect-object-injectionsuppresses the rule for the entire module, but the dynamic key access is limited to a couple of lookups (REQUIRED_SDK_RUNTIME[name]). To keep lint coverage meaningful, prefer scoping the disable to the exact line(s) that need it (or switch REQUIRED_SDK_RUNTIME to a Map and use.get()/.has()instead).
/* eslint-disable security/detect-object-injection -- all dynamic keys are validated against the fixed SDK runtime allowlist. */
const REQUIRED_SDK_RUNTIME = Object.freeze({
'@modelcontextprotocol/core': '2.0.0',
'@modelcontextprotocol/node': '2.0.0',
'@modelcontextprotocol/server': '2.0.0',
zod: '4.2.0',
});
Owner
Author
|
Review follow-up for this head:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 27 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/http-server.ts:492
warnDiagnosticpre-sanitizeserrorviasanitizeDiagnosticValue, but that sanitizer intentionally strips Error fields likemessage/stack(seediagnostic-sanitizer.ts), so this warning will log almost no useful context about why cleanup failed. Prefer passing the Error throughwriteDiagnostic(it already sanitizes Errors withsanitizeErrorForTransport) or updatewarnDiagnosticto treatErrorspecially so safemessage/stackare preserved.
}),
]);
} catch (error) {
warnDiagnostic("⚠️ Stateless HTTP cleanup did not complete normally.", error);
} finally {
Owner
Author
|
Final review disposition:
|
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
Verification