Skip to content

[mcp] fix: reject encoded leading-slash routes - #248

Merged
Wangmerlyn merged 1 commit into
mainfrom
codex/http-encoded-leading-slash-routes
Jul 1, 2026
Merged

[mcp] fix: reject encoded leading-slash routes#248
Wangmerlyn merged 1 commit into
mainfrom
codex/http-encoded-leading-slash-routes

Conversation

@Wangmerlyn

@Wangmerlyn Wangmerlyn commented Jul 1, 2026

Copy link
Copy Markdown
Owner

Summary

  • classify encoded leading-slash API/RPC aliases like /%2Fapi/gpus and /%2Frpc as noncanonical routes
  • return structured JSON 404s instead of static 403s or BaseHTTPRequestHandler HTML 501s
  • add GET, unsupported-method, pre-dispatch, and list_gpus-not-called regressions
  • document the encoded-alias route boundary in AGENTS and MCP guide docs

Verification

  • RED: targeted encoded-route tests failed with 403 static fallback or HTML 501 before the classifier fix
  • targeted route shard -> 25 passed
  • PYTHONPATH=src pytest tests/mcp/test_http_api.py tests/mcp/test_server.py -q -> 286 passed
  • PYTHONPATH=src pytest tests -q -> 943 passed, 11 skipped
  • mkdocs build --strict -> passed with known Material warning
  • pre-commit run --all-files --show-diff-on-failure -> passed
  • local subagent code review -> clean, ready to PR

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling of noncanonical and encoded API/RPC URLs so invalid path variants now return consistent structured 404 Unknown endpoint responses.
    • Prevented unsupported /rpc and /api/... variants from falling through to unintended behavior, including static or dashboard handling.
  • Documentation

    • Clarified supported and unsupported HTTP route forms, including encoded leading-slash variants and double-slash paths.

@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 introduces handling for HTTP encoded leading-slash routes (such as /%2Fapi/gpus and /%2Frpc) to ensure they are correctly classified as noncanonical and rejected with a structured JSON 404 error instead of falling back to static-file handlers or leaking HTML errors. The changes include adding a _route_path_candidates helper in the MCP server to generate and check unquoted and collapsed path candidates, updating documentation in AGENTS.md and docs/guides/mcp.md, adding an implementation plan, and expanding the test suite with corresponding regression tests. There are no review comments, and 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 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0c8b9ab9-f7e6-4572-8579-69905067c7ea

📥 Commits

Reviewing files that changed from the base of the PR and between 2b4f027 and 7fe0475.

📒 Files selected for processing (5)
  • AGENTS.md
  • docs/guides/mcp.md
  • docs/plans/http-encoded-leading-slash-routes.md
  • src/keep_gpu/mcp/server.py
  • tests/mcp/test_http_api.py

📝 Walkthrough

Walkthrough

This PR adds a shared _route_path_candidates() helper in the HTTP JSON-RPC handler to normalize encoded leading-slash route spellings (e.g., /%2Fapi/gpus, /%2Frpc), updates noncanonical API/RPC route detection to use it, expands test coverage for these variants, and updates documentation/plan docs accordingly.

Changes

Encoded route detection and docs

Layer / File(s) Summary
Route candidate helper and noncanonical detection logic
src/keep_gpu/mcp/server.py
Adds _route_path_candidates() producing deduplicated candidate route paths (raw, unquoted, collapsed leading slashes) and refactors _is_noncanonical_api_route/_is_noncanonical_rpc_route to use it for classifying encoded/double-slash aliases.
Test coverage for encoded route variants
tests/mcp/test_http_api.py
Extends parametrized tests covering /%2Frpc and /%2Fapi/gpus across HEAD, exact-alias, unsupported-method, and listing-suppression scenarios, including parameterizing a previously single-path test.
Documentation and plan updates
AGENTS.md, docs/guides/mcp.md, docs/plans/http-encoded-leading-slash-routes.md
Clarifies routing guidance for encoded/noncanonical /api and /rpc spellings, updates the MCP guide examples, and introduces a plan document detailing background, goal, solution, verification, and checklist.

Estimated code review effort: 2 (Simple) | ~15 minutes

Possibly related PRs

  • Wangmerlyn/KeepGPU#172: Both PRs modify _JSONRPCHandler logic to classify and reject noncanonical /rpc URL variants with structured JSON 404s, extending the same test file.
  • Wangmerlyn/KeepGPU#228: Both PRs modify _is_noncanonical_rpc_route to handle percent-/encoded noncanonical /rpc path aliases returning structured JSON 404s.
  • Wangmerlyn/KeepGPU#232: Both PRs change routing/canonicality handling for encoded /rpc//api paths to return structured JSON 404s, relying on shared canonicality checks.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.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 describes the main change: rejecting encoded leading-slash MCP 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 docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/http-encoded-leading-slash-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.

@Wangmerlyn
Wangmerlyn merged commit 6afe3d8 into main Jul 1, 2026
6 checks passed
@Wangmerlyn
Wangmerlyn deleted the codex/http-encoded-leading-slash-routes branch July 1, 2026 14:39
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