test(mcp): pin GET/DELETE on /mcp as 405, behind authentication - #994
test(mcp): pin GET/DELETE on /mcp as 405, behind authentication#994moonming wants to merge 1 commit into
Conversation
The sessionless posture means only POST carries MCP protocol messages, but nothing in this repo asserted it — the guarantee rested on the vendored SDK's dispatch arm, so an upgrade that started serving GET (SSE streams) or DELETE (session teardown) by default would have changed the wire surface silently. Two layers: the gateway service answers 405 for both verbs (flipping legacy_session_mode fails it), and the proxy pins the ordering — anonymous GET/DELETE is 401, authenticated is 405, so the endpoint never reveals which paths exist to an unauthenticated caller.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe pull request adds regression coverage for ChangesMCP method handling
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This PR adds localized regression coverage for authenticated and unauthenticated /mcp method handling; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds regression tests to pin the /mcp endpoint’s intended HTTP-method surface: POST only, with GET/DELETE returning 405, and ensures the proxy’s authentication runs before method handling so unauthenticated callers see 401 rather than learning method availability.
Changes:
- Added an
aisix-mcpintegration test that assertsGET /mcpandDELETE /mcpreturn405for the sessionless Streamable HTTP service configuration. - Added an
aisix-proxytest that assertsGET/DELETEare rejected as401when anonymous, and as405only after a validAuthorizationheader is provided.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| crates/aisix-proxy/src/mcp.rs | Adds a proxy-level test pinning auth-before-method behavior for GET/DELETE on /mcp (401 anonymous, 405 authenticated). |
| crates/aisix-mcp/tests/protocol_generations.rs | Adds a gateway-level test pinning GET/DELETE on /mcp to 405 for the sessionless serving posture. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Closes #991.
What
/mcpserves POST only —GET(the SSE stream a stateful server would offer) andDELETE(session teardown) return405. The docs state it, but no test in this repo pinned it: the behavior came from the vendored SDK's dispatch arm underlegacy_session_mode = false+ no event store, so an SDK upgrade that started serving either verb by default would have changed the endpoint's wire surface with nothing going red.Two layers, matching where each guarantee actually lives:
crates/aisix-mcp/tests/protocol_generations.rs— the gateway service answers405for both verbs. Revert-checked: flippinglegacy_session_modetotruefails it.crates/aisix-proxy/src/mcp.rs— the ordering. An anonymousGET/DELETEis401; only an authenticated one reaches405, so the endpoint doesn't disclose which paths exist to an unauthenticated caller.Verification
cargo test -p aisix-mcp -p aisix-proxygreen; both new tests confirmed to fail when the behavior they pin is reverted.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests