chore(deps): bump mcp/sdk to ^0.7.1 for GHSA-7m52-jw36-44r3 - #676
Merged
Conversation
CVE-2026-53965 affects mcp/sdk >=0.5.0,<0.7.1: the client HttpTransport's SSE buffer grows unbounded when a server withholds the event delimiter. composer audit has failed the "Lint & Static Analysis" job on every run since the advisory was published. ^0.6 cannot reach the fix (a caret on a 0.x version pins the minor), so the constraint moves to ^0.7.1. symfony/mcp-bundle v0.10 requires mcp/sdk ^0.6 and therefore has to move too; v0.11 is the first release accepting ^0.7, and its changes are additive (an http.allowed_hosts option, an MCP Apps attribute, and a middleware factory passed to the bundle's own controller). Compatibility of the SDK surface this application uses: - The McpTool/Schema attributes, ToolCallException, Mcp\Server and the three HTTP middleware classes are unchanged. - StreamableHttpTransport gained a trailing $maxBodyBytes parameter; McpEndpointController passes $logger and $middleware by name, so the call site is unaffected. - 0.7.0 made element loading lazy (a documented BC break): the registry loads on the first read instead of at Builder::build(). Nothing covered that path, because the tool tests call the tool services directly, so McpHttpEndpointTest gained a tools/list case over the real /mcp endpoint. Claude-Session: https://claude.ai/code/session_01AcqcEjgwcQfp3vpnFa3gh6 Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
CybotTM
marked this pull request as ready for review
August 14, 2026 16:58
|
Member
Author
|
Self-review (Copilot review unavailable — monthly quota exhausted; diff reviewed manually). Checked on head b340640: the constraint moves Verification ran in the CI-equivalent container (PHP 8.5.8): |
CybotTM
added a commit
that referenced
this pull request
Aug 15, 2026
Follow-up to [#676](#676). That PR brought `symfony/mcp-bundle` to v0.11.0, which adds an `http.allowed_hosts` option and an `mcp.middleware_factory` service — exactly what `App\Mcp\McpEndpointController` was written for. The bundle's own controller used to hardcode the SDK's localhost-only DNS-rebinding default, so a request arriving under a real domain was answered with `403 Invalid Host header`; the app worked around that by overriding `mcp.server.controller` with its own controller and its own middleware list. That workaround is now redundant, so this PR removes it and configures the bundle instead. `MCP_ALLOWED_HOSTS` stays the single source of truth and `.env` is unchanged — the env var now feeds `mcp.http.allowed_hosts` directly instead of the `app.mcp_allowed_hosts` parameter. ## Parity Every behaviour the override carried, and where it lands upstream. | Override behaviour | Upstream equivalent | |---|---| | Middleware `CorsMiddleware`, `DnsRebindingProtectionMiddleware`, `ProtocolVersionMiddleware`, in that order | `MiddlewareFactory::create()` iterates `StreamableHttpTransport::defaultMiddleware()`, which is that same list in that same order, and replaces the DNS-rebinding entry | | `DnsRebindingProtectionMiddleware(allowedHosts: %env(csv:MCP_ALLOWED_HOSTS)%)` | `mcp.http.allowed_hosts: '%env(csv:MCP_ALLOWED_HOSTS)%'` → `MiddlewareFactory` constructor arg | | DNS-rebinding protection stays enabled (never disabled) | option is an array, not `false` — `false` is the documented opt-out and is not used | | `maxBodyBytes` left at the SDK default | upstream also omits the argument; both get `StreamableHttpTransport::DEFAULT_MAX_BODY_BYTES` | | PSR factory wiring (`mcp.server`, `mcp.psr_http_factory`, `mcp.http_foundation_factory`, `mcp.psr17_factory` ×2) | identical arguments, registered by `McpBundle::configureClient()` | | Logger injected as `@logger` | upstream injects `logger` too, and additionally tags the controller `monolog.logger` channel `mcp` — strictly more, not less | | Public service tagged `controller.service_arguments` | upstream sets `setPublic(true)` and the same tag | | Route `/mcp` via the bundle's route loader | unchanged; `config/routes/mcp.yaml` and `mcp.http.path` are untouched | | SSE detection tolerant of media-type parameters (`str_starts_with`) | upstream compares exactly against `text/event-stream`. Not observable: `mcp/sdk` emits the header with no parameters in both v0.6.0 and the locked v0.7.1, so the tolerance never fired | | `implements App\Security\ApiToken\SelfEnforcesScope` | **no upstream equivalent** — see below | ## The one gap, and how it is closed `SelfEnforcesScope` is a marker interface that exempts the MCP endpoint from `RequireScopeSubscriber`'s fail-closed gate. It exists because `/mcp` multiplexes many tools that each enforce their own scope through `App\Mcp\ScopeGuard`, so no single controller-level `#[RequireScope]` can express the endpoint's requirement. The bundle's `McpController` is `final` and therefore cannot carry the marker. Rather than keep a 76-line controller alive for a marker interface, `RequireScopeSubscriber` now names the bundle controller directly, and the marker — which would otherwise have no implementor left — is removed. The exemption is unchanged in scope: it still applies to exactly one controller, and every other controller still needs a declared scope or gets a 403. If a reviewer would rather keep the abstraction than reference a vendor class from `src/Security`, that is the one decision in this PR worth reversing; say so and I will restore the interface alongside the vendor check. ## Verification Run in the CI-equivalent container (`COMPOSE_PROFILES=e2e docker compose run --rm app-e2e …`, the same recipe `.github/workflows/ci.yml` uses), against commit `a5c0358a`. | Gate | Result | |---|---| | `bin/phpunit --testsuite unit` | OK (1938 tests, 4501 assertions) | | `bin/phpunit --testsuite integration,controller,api-functional,mcp` | OK (766 tests, 4584 assertions; 2 deprecations + 1 skip, both pre-existing and unrelated — a deprecated v1 `GetTimeSummaryAction` and a data-dependent repository skip) | | `bin/phpunit --testsuite mcp` | OK (83 tests, 424 assertions) | | `bin/phpstan analyze --no-progress` (full tree, level 10) | No errors | | `bin/phpstan analyze -c config/quality/phpat.neon` | No errors | | `bin/php-cs-fixer fix --dry-run --diff` | 0 of 681 files need fixing | | `bin/rector process src --config=config/quality/rector.php --dry-run` | Rector is done, no changes | | `bin/console lint:container --env=test` | all services injected with compatible values | | `bin/console cache:warmup` (test, dev, prod) | OK in all three | `tests/Mcp/McpHttpEndpointTest.php` keeps all four test methods byte-identical; only its class docblock changed, to name the new mechanism instead of the deleted class. One caveat that a green suite would otherwise hide: the test environment's `MCP_ALLOWED_HOSTS` is `localhost,127.0.0.1,[::1]`, which is byte-for-byte the SDK's own default. A passing `McpHttpEndpointTest` therefore does not by itself prove the new option is wired — the same result would appear if the config were ignored entirely. So the option was probed by re-running that file with `MCP_ALLOWED_HOSTS=evil.example.com`: `testInitializeSucceedsForAnAllowedHost` flipped to 403 and `testDisallowedHostIsForbidden` flipped to 200, while `testBogusTokenIsUnauthorized` stayed green. Both host assertions invert with the configuration, which is what proves the guard reads it. `debug:container mcp.server.controller` confirms the wiring on the other side: the controller is `Symfony\AI\McpBundle\Controller\McpController`, taking `mcp.middleware_factory`, whose sole argument is `%env(csv:MCP_ALLOWED_HOSTS)%`.
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.
Why
composer audithas failed the Lint & Static Analysis job on every run since 2026-08-14T06:19Z — for example run 31807512759. The finding is CVE-2026-53965 / GHSA-7m52-jw36-44r3: inmcp/sdk>=0.5.0,<0.7.1the clientHttpTransport's SSE buffer grows unbounded when a server withholds the event delimiter. We were locked at v0.6.0.^0.6cannot reach the fix, because a caret constraint on a0.xversion pins the minor — so the constraint moves to^0.7.1.Why
symfony/mcp-bundlemoves toosymfony/mcp-bundlev0.10.0 requiresmcp/sdk: ^0.6, which makes it a hard blocker for the SDK bump; it has to move in the same commit. v0.11.0 is the first release that accepts^0.7(^0.6|^0.7), and it is the smallest step that unblocks us — v0.12.0 would additionally replace the SDK's file-based element discovery with container-based registration, which is a behavioural change this security fix has no reason to carry. DiffingMcpBundle.phpv0.10.0 against v0.11.0, everything v0.11 adds is additive: anhttp.allowed_hostsoption with itsmcp.middleware_factoryservice, the#[AsMcpApp]MCP Apps attribute, and aMiddlewareFactoryreference appended to the bundle's ownmcp.server.controllerarguments. We override that service withApp\Mcp\McpEndpointControllerand supply our own argument list, so the appended reference does not reach us.The lock diff is exactly these two packages — nothing else moves (a first attempt with
--with-all-dependenciesalso carriedsymfony/uidv8.1.0 → v8.1.4, which is unrelated to the advisory, so the update was redone without it).Compatibility, per usage site
The application imports eight SDK symbols.
Mcp\Capability\Attribute\McpTool,Mcp\Capability\Attribute\SchemaandMcp\Exception\ToolCallExceptionare used acrosssrc/Mcp/Tool/*,src/Mcp/ScopeGuard.phpandsrc/Mcp/AdminEntityResolver.php;Mcp\Server,Mcp\Server\Transport\StreamableHttpTransportand the threeHttp\Middlewareclasses (CorsMiddleware,DnsRebindingProtectionMiddleware,ProtocolVersionMiddleware) are used insrc/Mcp/McpEndpointController.php. Against the upstream CHANGELOG for 0.7.0 and 0.7.1:Mcp\Server::run()is unchanged.StreamableHttpTransport::__construct()gained a trailing$maxBodyBytesparameter (default 4 MiB, POST bodies above it get a413).McpEndpointControllerpasses$loggerand$middlewareas named arguments, so the new trailing parameter does not shift anything at our call site.Builder::build(), andinitializeadvertises capabilities from the configured sources rather than the loaded registry. Verified below.Verification
Every command ran in the CI environment (
COMPOSE_PROFILES=e2e docker compose run --rm app-e2e …, PHP 8.5.8), against the same image the Lint job uses.composer audit --format=plaincomposer validate --no-check-publish./composer.json is validphp bin/console cache:warmup --env=devphp -d memory_limit=1G bin/phpstan analyze --no-progress[OK] No errorsphp -d memory_limit=1G bin/phpstan analyze -c config/quality/phpat.neon --no-progress[OK] No errorsphp -d memory_limit=512M bin/php-cs-fixer fix --dry-run --diffphp -d memory_limit=1G bin/rector process src --config=config/quality/rector.php --dry-run[OK] Rector is done!php bin/console lint:twig templates/bin/phpunit --testsuite=unitbin/phpunit --testsuite integration,controller,api-functional,mcpGetTimeSummaryActionADR-022 notices, unrelated to MCPThe one gap this PR closes
The lazy-loading BC break is the only change that could have broken us silently, and nothing in the suite covered it:
tests/Mcp/McpToolsTest.phpand its siblings fetch the tool services out of the container and invoke them directly, so an MCP registry that came up empty would still have shown a green suite whiletools/listreturned nothing over the wire.McpHttpEndpointTesttherefore gainstestToolsListExposesTheRegisteredTools, which drives the real/mcpendpoint — handshake, session id, thentools/list— and asserts the registered tools are actually in the response. It passes on v0.7.1, and it is placed after a real registry read, so an empty registry fails it.Not in scope
v0.11's new
mcp.http.allowed_hostsoption covers exactly whatApp\Mcp\McpEndpointControllerwas written for (the bundle's own controller hardcoded the SDK's localhost-only DNS-rebinding default). Dropping our controller override in favour of the upstream option is now possible and worth doing, but it is a behavioural refactor of the MCP entry point and does not belong in a security bump.