Repository navigation
[Server] Bind Streamable HTTP sessions to the principal that opened them - #627
nicolas-grekas wants to merge 2 commits into
Conversation
chr-hertel
added
Server
There was a problem hiding this comment.
🟡 Changes recommended
A session-store read failure can bypass ownership checks, allowing a caller with a valid subjectless token to delete another principal’s session.
3 open issues
What changed in this PR
This PR adds principal binding to handshake-era Streamable HTTP sessions, addressing #529 within Protocol without requiring custom session stores to understand authentication.
Changes:
- Derives a stable principal from token claims and passes it through transport callbacks.
- Stores a principal fingerprint and checks ownership before session use or termination.
- Adds binding tests and documents external authentication integration and compatibility changes.
| File | Description |
|---|---|
tests/Unit/Server/Transport/SessionBindingTest.php |
Adds session ownership, token refresh, and deletion tests. |
tests/Unit/Server/Transport/Http/Middleware/AuthorizationMiddlewareTest.php |
Tests principal derivation from token claims. |
tests/Unit/Server/Transport/AccessTokenFlowTest.php |
Adapts token-flow tests to principal-bound sessions. |
tests/Unit/Server/BuilderTest.php |
Updates expected session reads during termination. |
src/Server/Transport/TransportInterface.php |
Documents principal arguments for callbacks. |
src/Server/Transport/StreamableHttpTransport.php |
Reads the principal attribute for POST and DELETE requests. |
src/Server/Transport/ManagesTransportCallbacks.php |
Updates callback type annotations. |
src/Server/Transport/Http/Middleware/AuthorizationMiddleware.php |
Derives and attaches the authenticated principal. |
src/Server/Transport/BaseTransport.php |
Forwards principals to transport listeners. |
src/Server/Protocol.php |
Stores fingerprints and checks session ownership. |
docs/run/authorization.md |
Explains binding and application-provided principals. |
CHANGELOG.md |
Records binding behavior and callback signature changes. |
Review effort: Balanced
| $fingerprint = $session->get(self::SESSION_PRINCIPAL); | ||
|
|
||
| return (\is_string($fingerprint) ? $fingerprint : null) === self::fingerprint($principal); |
There was a problem hiding this comment.
Fixed in 3c8363a: a session opened without a principal now records that, and a session that records no principal at all serves no one. So after a failed read, both the request and the DELETE are refused. Sessions opened before the upgrade get a 404 too, the CHANGELOG says so.
| return $this->sessionManager->createWithId($sessionId); | ||
| $session = $this->sessionManager->createWithId($sessionId); | ||
|
|
||
| if (!$this->servesPrincipal($session, $principal)) { |
There was a problem hiding this comment.
Fixed in 3c8363a: who opened the session is checked before waiting for the lock, and again under it. Another principal gets the 404 of an unknown session and can't keep the lock busy. An initialize sent with the session id leaked the same way (503 instead of 400), and it now never takes the lock.
| } | ||
|
|
||
| $this->handleSessionEnd($this->sessionId); | ||
| $this->handleSessionEnd($this->sessionId, $principal); |
There was a problem hiding this comment.
A DELETE for an unknown session also gets a 200, since handleDeleteRequest() always answers 200. In 3c8363a, the test now checks that both answers are the same. Answering 404 to both would also change unknown sessions, so I'd rather do that separately.
A session opened without a principal now records it, so a session the store fails to read serves no one. Who opened a session is checked before waiting for its lock, so another principal can't tell the session is busy, and an initialize never takes the lock.
Fixes #529
A handshake-era session now only serves the principal that opened it, as the security best practices recommend. When another principal sends the session id, the server answers 404 like for an unknown session, and its
DELETEdoesn't end the session.I found this while building an MCP server for the Symfony showcase app: carol could call tools on the session alice opened.
On the open questions of #529:
Protocol, next to the other session checks, so custom stores don't need to know about it.subidentifies no principal. An app that authenticates requests itself sets the newStreamableHttpTransport::PRINCIPAL_ATTRIBUTErequest attribute.AuthorizationMiddlewarebuilds the principal fromiss,subandclient_id/azp, like #625 does forrequestState.Protocolonly gets an opaque string and stores its SHA-256 in the session.McpBundle (symfony/ai) authenticates with Symfony Security, not with
AuthorizationMiddleware. It'll need to set this attribute from the user identifier for the binding to work there.