mirror of
https://github.com/morluto/rea.git
synced 2026-11-03 05:16:16 +00:00
fix(mcp): harden the streamable HTTP listener's session replacement and shutdown
Addresses the automated review findings on the PR: refuse requests once the session slot is closed and join an arming initialize during shutdown; retry failed close stages instead of replaying the rejection; classify loopback binds by address class and add the bind's own hostname to the Host and Origin allowlist; let only a POST with the streamable headers replace an open session; announce the bound URL at every log level; route `mcp -h --http` to the HTTP entry. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
1 parent
46dd8f0e2b
commit
b0ec63cc03
8 files changed
+283
-61
No files matched your search
+2
-2
@@ -322,8 +322,8 @@ rea mcp --http --port 8765
|
||||
|
||||
`--host` and `--port` default to `MCP_HOST` and `MCP_PORT` from the environment,
|
||||
then to `127.0.0.1` and an ephemeral port. The bound URL is written to stderr as
|
||||
one JSON log line (`"msg":"MCP transport listening"`); nothing is written to
|
||||
stdout. `rea mcp --http --help` lists the accepted options. The listener serves
|
||||
one JSON log line (`"msg":"MCP transport listening"`) whatever `REA_LOG_LEVEL`
|
||||
is set to; nothing is written to stdout. `rea mcp --http --help` lists the accepted options. The listener serves
|
||||
one client at a time: a session ends with the client's `DELETE`, and a client
|
||||
that connects while another session is open takes it over, so a client that
|
||||
exited without ending its session needs no restart. The listener runs until
|
||||
|
||||
+15
-6
@@ -427,7 +427,8 @@ stdio is the default transport and the only one that client registrations,
|
||||
streamable HTTP transport at `/mcp`; it replaces stdio for that process rather
|
||||
than running beside it. Host and port come from the flags, then `MCP_HOST` and
|
||||
`MCP_PORT`, then `127.0.0.1` and an ephemeral port. The bound URL is announced
|
||||
as one JSON log line on stderr (`"msg":"MCP transport listening"` with `url`);
|
||||
as one JSON log line on stderr (`"msg":"MCP transport listening"` with `url`)
|
||||
at every `REA_LOG_LEVEL`, because an ephemeral port is otherwise unreachable;
|
||||
stdout stays unused.
|
||||
|
||||
The HTTP server is stateful and serves one client at a time on one server
|
||||
@@ -439,11 +440,19 @@ and the shared active target, and the next client opens a fresh session against
|
||||
them. The newest client replaces an open session: an `initialize` that arrives
|
||||
while a session is open ends that session instead of being refused, because a
|
||||
client that exits without `DELETE` must not wedge the listener; the replaced
|
||||
client's open streams are closed and its later requests are refused. Requests
|
||||
that carry no valid session id are refused with `400` or `404` as the SDK
|
||||
transport defines. On a loopback bind, requests whose `Host` header is outside
|
||||
the SDK loopback allowlist are refused with `403`; a non-loopback bind relaxes
|
||||
that check and logs the relaxation. Request bodies are bounded by the SDK's
|
||||
client's open streams are closed and its later requests are refused. Only an
|
||||
`initialize` that carries the streamable headers (`Content-Type:
|
||||
application/json` and an `Accept` naming both `application/json` and
|
||||
`text/event-stream`) and passes the Host and Origin checks can replace a
|
||||
session; any other sessionless POST goes to the live transport, which refuses
|
||||
it without disturbing the session. Requests that carry no valid session id are
|
||||
refused with `400` or `404` as the SDK transport defines. A bind is loopback by
|
||||
address class, not spelling: any `127.0.0.0/8` address, `::1`, an IPv4-mapped
|
||||
loopback and `localhost` names. On such a bind, requests whose `Host` header is
|
||||
outside the SDK loopback allowlist plus the bind's own hostname are refused with
|
||||
`403`, and a browser `Origin` outside that allowlist is refused the same way (a
|
||||
missing `Origin` passes); a non-loopback bind relaxes both checks and logs the
|
||||
relaxation. Request bodies are bounded by the SDK's
|
||||
4 MiB limit, and the response budget keeps its stdio-derived floor. The
|
||||
listener ends on SIGINT or SIGTERM only, not on stdin EOF, and exits `130` or
|
||||
`143` after cleanup settles.
|
||||
|
||||
@@ -16,7 +16,7 @@ client registration keep stdio.
|
||||
dispatcher without changing bare `mcp`, `mcp doctor` or `mcp add`.
|
||||
- Resolve host and port from the flags, then `MCP_HOST`/`MCP_PORT` from the
|
||||
environment snapshot, then `127.0.0.1` and port `0`; announce the bound URL as
|
||||
one pino JSON line on stderr and leave stdout unused.
|
||||
one pino JSON line on stderr at every log level and leave stdout unused.
|
||||
- Use `WebStandardStreamableHTTPServerTransport` in stateful mode around one
|
||||
server instance built once. `DELETE` ends the session on its transport without
|
||||
running the instance's shutdown hooks, so the same instance keeps its
|
||||
@@ -29,8 +29,13 @@ client registration keep stdio.
|
||||
- Bridge node:http to web Request/Response in `src/server/http/`, streaming
|
||||
response chunks as they are produced and turning a client disconnect into a
|
||||
request abort and body cancellation.
|
||||
- Validate the `Host` header with the SDK loopback allowlist for loopback binds;
|
||||
relax it for other binds and log that relaxation.
|
||||
- Classify a loopback bind by address class (`127.0.0.0/8`, `::1`, IPv4-mapped
|
||||
loopback, `localhost` names), validate the `Host` and `Origin` headers with
|
||||
the SDK loopback allowlist plus the bind's own hostname on such binds, and
|
||||
relax both for other binds and log that relaxation.
|
||||
- Let only an `initialize` that carries the streamable headers and passes those
|
||||
checks replace an open session; refuse requests once the slot is closed, join
|
||||
an arming session during shutdown, and keep close stages retryable.
|
||||
- End the HTTP listener on SIGINT/SIGTERM only, never on stdin EOF, and exit with
|
||||
the signal status (`130`/`143`) after cleanup settles.
|
||||
- Export `serveStreamableHttp({ host, port, createMcpServer, routes, logger })`
|
||||
|
||||
@@ -155,3 +155,44 @@ re-runs, not by repeating the full projects on an idle host. The error-level
|
||||
log line that names the bind cause was observed in earlier runs, not asserted
|
||||
by `mainErrors.test.ts`. Response budgets keep the stdio-derived floor and were
|
||||
not re-measured over HTTP. Windows was not run.
|
||||
|
||||
## Automated review findings on the PR (2026-10-18)
|
||||
|
||||
Codex and Greptile left eight inline findings on morluto/rea#4955 at
|
||||
46dd8f0e; each was checked against the code and addressed in one follow-up
|
||||
commit, with a regression test where the behaviour is observable:
|
||||
|
||||
- A late `initialize` could arm a fresh transport after shutdown (Codex, P1):
|
||||
the slot now records that it is closed, `close()` joins an arming session
|
||||
before ending it, and every request after that is refused with `503`. Test:
|
||||
an initialize racing `close()` gets `503`, the shutdown hook fires once, and
|
||||
the next request gets `503`.
|
||||
- A failed `close()` was memoized forever (both reviewers, P1): the close runs
|
||||
three stages once each and a failed stage is retried by the next call. Test:
|
||||
the instance's first `close()` rejects, the second call resolves and the port
|
||||
is released.
|
||||
- Loopback aliases skipped Host validation (both reviewers, P2 and P1): a bind
|
||||
is now loopback by address class (`127.0.0.0/8`, `::1`, IPv4-mapped loopback,
|
||||
`localhost` names) after the same normalization the SDK applies to the Host
|
||||
header, and the allowlist adds the bind's own hostname so requests to
|
||||
`127.0.0.2` or `localhost.` are still served. Test: twelve spellings.
|
||||
- A sessionless POST could close the live session before any header check
|
||||
(Greptile, P1): on loopback binds the route validates `Origin` with the same
|
||||
allowlist as `Host`, and only a POST carrying `Content-Type:
|
||||
application/json` and an `Accept` naming both `application/json` and
|
||||
`text/event-stream` may replace a session; others reach the live transport,
|
||||
which refuses them. Test: a `text/plain` initialize and one with a foreign
|
||||
`Origin` are refused (4xx and 403) while the connected client keeps echoing.
|
||||
- The bound URL disappeared at quiet log levels (Greptile, P2): the listening
|
||||
line is written through a child logger pinned to `info`. Test: a `warn`
|
||||
logger still receives the line with the URL.
|
||||
- `rea mcp -h --http` reached the ordinary CLI (Greptile, P2): the dispatcher
|
||||
predicate accepts any flag after the entry. Test: the dispatcher case.
|
||||
|
||||
Checks after the change: `oxfmt --check .` clean; `tsc --noEmit` clean;
|
||||
`oxlint` on the touched sources clean after extracting the staged close into
|
||||
its own function; `npx knip` no findings; `npm run jscpd` 3.35 % duplicated
|
||||
lines; `scan:todos` none; `verify:test-discovery` 1,583 files once; `vitest
|
||||
run` over the listener and bridge adapters tests 14 passed in 2 files; after
|
||||
`npm run build:cached`, `vitest run` over the dispatcher, option, streamable
|
||||
HTTP, shutdown and main-error boundary tests 22 passed in 5 files.
|
||||
Reference in new issue
Block a user