Skip to content

[mcp] fix: reject noncanonical RPC routes - #172

Merged
Wangmerlyn merged 3 commits into
mainfrom
codex/rpc-noncanonical-routes
Jun 30, 2026
Merged

[mcp] fix: reject noncanonical RPC routes#172
Wangmerlyn merged 3 commits into
mainfrom
codex/rpc-noncanonical-routes

Conversation

@Wangmerlyn

@Wangmerlyn Wangmerlyn commented Jun 30, 2026

Copy link
Copy Markdown
Owner

Summary

  • Reject noncanonical /rpc URLs such as /rpc/, /rpc;..., and /rpc?... with structured JSON 404 responses.
  • Keep exact POST /rpc JSON-RPC behavior and GET /rpc 405 behavior intact.
  • Document the exact /rpc endpoint contract in AGENTS, README, and MCP/service docs.

Test Plan

  • PYTHONPATH=src python -m pytest tests/mcp/test_http_api.py::test_http_rpc_trailing_slash_returns_json_404_without_static_fallback tests/mcp/test_http_api.py::test_http_rpc_noncanonical_path_rejects_before_jsonrpc_parse -q
  • PYTHONPATH=src python -m pytest tests/mcp/test_http_api.py -q
  • python -m ruff check .
  • git diff --check
  • PYTHONPATH=src python -m pytest tests/mcp -q
  • PYTHONPATH=src mkdocs build --strict --site-dir /tmp/keepgpu-rpc-noncanonical-site
  • PYTHONPATH=src pre-commit run --all-files

Summary by CodeRabbit

  • Documentation

    • Clarified that the MCP JSON-RPC endpoint is available only at the exact /rpc path.
    • Added guidance that malformed or non-canonical /rpc URLs return structured 404 or 405 responses.
  • Bug Fixes

    • Requests to /rpc with trailing slashes, extra path parts, or query strings are now consistently rejected.
    • GET /rpc now returns a clear 405 Method Not Allowed response with Allow: POST.
    • Added coverage to confirm invalid /rpc URLs do not fall back to other handlers.

@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@Wangmerlyn, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 51 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 81bd4ae1-2041-4b35-bb68-0b5cb0edfbf0

📥 Commits

Reviewing files that changed from the base of the PR and between b42ff1d and 38185db.

📒 Files selected for processing (3)
  • README.md
  • src/keep_gpu/mcp/server.py
  • tests/mcp/test_http_api.py
📝 Walkthrough

Walkthrough

Noncanonical /rpc URL variants (trailing slashes, query strings, semicolons) are now rejected with a structured JSON 404 Unknown endpoint before any JSON-RPC dispatch in _JSONRPCHandler. Guards are added to do_GET, do_POST, and do_DELETE. Tests and documentation are updated to match.

Strict /rpc path enforcement

Layer / File(s) Summary
Noncanonical RPC detection and early rejection
src/keep_gpu/mcp/server.py
Adds _is_noncanonical_rpc_route and _reject_noncanonical_rpc_route helpers on _JSONRPCHandler; inserts early-return guards in do_GET, do_POST, and do_DELETE that return JSON 404 Unknown endpoint for any /rpc-prefixed but noncanonical path.
Tests for noncanonical /rpc paths
tests/mcp/test_http_api.py
Adds a GET /rpc/ test (asserts JSON 404, no HTML fallback) and a parametrized POST test for /rpc/, /rpc;debug, /rpc?debug=1 (asserts 404 rejection before JSON-RPC parsing, using an invalid JSON body).
Documentation alignment
AGENTS.md, README.md, docs/guides/mcp.md, docs/reference/cli.md
Updates all documentation to state that /rpc must be exact, noncanonical variants return structured 404 Unknown endpoint, and GET /rpc returns 405 Method Not Allowed with Allow: POST.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • Wangmerlyn/KeepGPU#60: Introduced the _JSONRPCHandler and HTTP JSON-RPC mode that this PR extends with noncanonical path rejection.
  • Wangmerlyn/KeepGPU#122: Also modifies do_POST on /rpc for JSON parse-error handling; the new early-404 guard affects whether that parse-error path is reached.

Poem

🐇 Hoppity-hop down the /rpc lane,
No trailing slashes, no query-string pain!
A semicolon crept in? Four-oh-four it goes!
The path must be exact — that's how the server knows.
Clean routes only, says the rabbit with glee! 🌿

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly reflects the main change: rejecting noncanonical MCP RPC routes.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/rpc-noncanonical-routes

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the HTTP server to strictly enforce exact matching for the /rpc endpoint. Noncanonical paths, such as those with trailing slashes, parameters, or query strings, are now rejected with a structured JSON 404 "Unknown endpoint" error before any JSON-RPC parsing or static fallback occurs. Relevant documentation and tests have been updated to reflect and verify this behavior. I have no feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
README.md (1)

170-172: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider adding the noncanonical 404 behavior to README for consistency.

The other updated docs (AGENTS.md, docs/guides/mcp.md, docs/reference/cli.md) explicitly mention that noncanonical /rpc URLs return structured 404 errors. Adding a brief note here would keep the user-facing docs aligned.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@README.md` around lines 170 - 172, The README HTTP mode description should
mention the noncanonical /rpc 404 behavior so it stays aligned with the other
docs. Update the HTTP mode section to note that only the exact /rpc endpoint is
accepted and that noncanonical /rpc URLs return structured 404 errors, keeping
the wording consistent with the existing HTTP mode explanation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@README.md`:
- Around line 170-172: The README HTTP mode description should mention the
noncanonical /rpc 404 behavior so it stays aligned with the other docs. Update
the HTTP mode section to note that only the exact /rpc endpoint is accepted and
that noncanonical /rpc URLs return structured 404 errors, keeping the wording
consistent with the existing HTTP mode explanation.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: be3c559a-6043-4ea5-9a03-82339cfc9e48

📥 Commits

Reviewing files that changed from the base of the PR and between 2ac3a5e and b42ff1d.

📒 Files selected for processing (6)
  • AGENTS.md
  • README.md
  • docs/guides/mcp.md
  • docs/reference/cli.md
  • src/keep_gpu/mcp/server.py
  • tests/mcp/test_http_api.py

@Wangmerlyn

Copy link
Copy Markdown
Owner Author

Resolved the README consistency note in 38185db by documenting that noncanonical /rpc URLs return structured 404 errors. Final local verification remains green.

@Wangmerlyn
Wangmerlyn merged commit da4c4f2 into main Jun 30, 2026
5 checks passed
@Wangmerlyn
Wangmerlyn deleted the codex/rpc-noncanonical-routes branch June 30, 2026 04:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant