Repository navigation
Move to mcp 1.28.1 with end-to-end tests of the served endpoint, and compare the bearer token in constant time - #4
Merged
Conversation
test_secret_path_gate drives SecretPathAuth against a stub, so nothing checked that the real app still speaks MCP once FastMCP's transport sits behind the gate. An SDK upgrade that changed how the streamable-HTTP transport answers, or that began rejecting the tunnel's Host header, would only have shown up as a broken connector in ChatGPT. These build the app exactly as main() does and drive it with Starlette's TestClient (no uvicorn, no tunnel, no AutoCAD): initialize and tools/list through a tunnel-style Host header, the stateless contract (no session needed, no session id issued), JSON rather than SSE responses, the health route living under the secret path, and the bearer check sitting in front of the transport. Each assertion was checked against a deliberately broken server.py: leaving DNS-rebinding protection on, going stateful, switching to SSE, dropping the gate, and moving the health route each fail the suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mcp 1.27.1 is listed as affected by GHSA-jpw9-pfvf-9f58 (HTTP transports did not bind sessions to the authenticated principal), GHSA-hvrp-rf83-w775 (experimental task handlers not scoped to their session) and GHSA-vj7q-gjh5-988w (no Host/Origin validation on the WebSocket transport). 1.28.1 is the first release that fixes all three. By each advisory's own "am I affected" test, none of them reaches this server: it runs streamable HTTP in stateless mode, never calls enable_tasks(), and does not use the WebSocket transport. The point is that anyone installing from requirements.txt, or running pip-audit over it, is not handed a version with open CVEs, and that real findings are not buried under these. The 1.x changes in between are the two fixes above, deprecation warnings for WebSocket and experimental tasks (neither used here), Python 3.14 support, and buffered per-request streamable-HTTP streams. The new end-to-end tests pass on both 1.27.1 and 1.28.1, and the real server under uvicorn answered initialize and tools/list through a tunnel-style Host header on 1.28.1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
SecretPathAuth compared the presented Authorization header with != which returns as soon as a character differs, so response timing can in principle reveal how much of a guessed token was right. Use hmac.compare_digest instead. Behaviour is otherwise unchanged: a missing header is still allowed, surrounding whitespace is still tolerated, and every existing gate test passes untouched. The comparison is done on UTF-8 bytes because compare_digest raises TypeError for str containing non-ASCII characters, and the header is attacker-controlled. Two new tests: one asserts the gate goes through compare_digest (it errors on the old code), the other sends a non-ASCII token and expects a 401 (it errors on a naive str comparison, which would have turned a bad token into a 500). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
What
Three commits, one theme: the endpoint that ngrok exposes to the internet.
End-to-end tests of the assembled server (
tests/test_assembled_server.py, 5 tests).build_app()is driven through Starlette'sTestClient, exactly asmain()assembles it. No uvicorn, tunnel or AutoCAD is needed: initialising and listing tools never touch COM.mcp1.27.1 → 1.28.1 inrequirements.txt. 1.28.1 is the first release that clears all three advisories listed against 1.27.1.The bearer token is compared with
hmac.compare_digestinstead of!=(Backlog item named as a follow-up in Test the secret-path gate the README's security model rests on #3).Why
test_secret_path_gatecoversSecretPathAuthagainst a stub, but nothing checked that the real app still speaks MCP once FastMCP's transport sits behind the gate. The SDK bump needs exactly that check. Without it, a transport change, or the DNS-rebinding check starting to reject the tunnel's Host header, would only show up as a broken ChatGPT connector.The advisories against 1.27.1: GHSA-jpw9-pfvf-9f58 (HTTP sessions not bound to the authenticated principal), GHSA-hvrp-rf83-w775 (experimental task handlers not session-scoped) and GHSA-vj7q-gjh5-988w (no Host/Origin validation on the WebSocket transport). By each advisory's own "am I affected" test, none reaches this server. It runs streamable HTTP in stateless mode, never calls
enable_tasks(), and doesn't use WebSocket. The bump is so thatpip install -r requirements.txtandpip-auditdon't hand out a version with open CVEs, and so real findings aren't buried under these. The 1.x changes in between are those fixes, deprecation warnings for WebSocket and tasks (unused here), Python 3.14 support, and buffered per-request streamable-HTTP streams.!=returns at the first differing character.compare_digestdoesn't. The comparison runs on UTF-8 bytes:compare_digestraisesTypeErroron non-ASCIIstr, and the header is attacker-controlled, so a naive swap would have turned a bad token into a 500.How it was verified
Full suite on both SDK versions in throwaway venvs: 42 tests OK on
mcp==1.27.1and onmcp==1.28.1before the gate change, 44 after it. A clean venv fromrequirements.txtpassespip check, the CIcompileallstep and the suite.The new e2e tests have teeth. Each of these deliberately broken
server.pycopies fails them: DNS-rebinding protection left on (3 failures),stateless_http=False(1),json_response=False(1 failure + 1 error), the gate removed from the assembled app (1), and the health route moved off the secret path (1).server.pywas restored byte-identical afterwards (same sha1).Gate change:
test_token_is_compared_in_constant_timeerrors on main'sserver.py.test_non_ascii_token_is_refused_not_crashederrors on a naivecompare_digest(str, str)version. All 14 existing gate tests pass untouched: a missing header is still allowed, whitespace is still tolerated, and a bad path is still a 404 before any token check.The real server on 1.28.1:
python server.pyunder uvicorn on a spare port answeredinitializeandtools/list(18 tools) through aHost: example.ngrok-free.appheader, and returned 404 for/mcp.CI (windows-latest) installed
mcp-1.28.1fromrequirements.txtand ran 44 tests OK.The
pydantic_settingsIncompleteFieldDefinitionWarningprinted at startup appears identically on 1.27.1. It comes from pydantic-settings 2.15.0, not from this bump.Not verified
No live AutoCAD and no real tunnel. No drawing tool was called, and nothing ran through ngrok, ChatGPT or Manus. The tool layer's COM code is unchanged, but the pins comment now says only what was actually checked for
mcp.To pick up the new SDK on the machine that runs the server:
pip install -r requirements.txt. That machine still has 1.27.1 installed globally.