Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -139,22 +139,22 @@ This file defines how coding agents should work in this repository.
responses. `/api/sessions/{job_id}` accepts exactly one raw path component;
raw extra segments such as `/api/sessions/foo/bar` are unknown routes.
- Encoded or otherwise noncanonical route spellings whose raw or decoded target
is API-shaped (`//api/...`, `/api`, `/api/...`, `/api;...`, `/api?...`, or
`/api#...`) must return structured JSON `404 Unknown endpoint` responses
instead of serving the dashboard/static fallback or `BaseHTTPRequestHandler`
HTML errors. Exact API endpoints such as `/api/gpus` must not accept params,
query strings, or fragments unless the handler explicitly documents those
components. Canonical API paths may still validate encoded `job_id` path
components normally.
is API-shaped (`//api/...`, `/%2Fapi/...`, `/api`, `/api/...`, `/api;...`,
`/api?...`, or `/api#...`) must return structured JSON `404 Unknown endpoint`
responses instead of serving the dashboard/static fallback or
`BaseHTTPRequestHandler` HTML errors. Exact API endpoints such as `/api/gpus`
must not accept params, query strings, or fragments unless the handler
explicitly documents those components. Canonical API paths may still validate
encoded `job_id` path components normally.
- Implemented HTTP verb handlers (`do_POST`, `do_DELETE`, and future siblings)
must call the shared unsupported-method helper before local unknown-endpoint
404 branches, so known paths never regress to 404 or body-parse errors solely
because the wrong implemented verb was used.
- `/rpc` is an exact POST-only JSON-RPC endpoint; `GET /rpc` must return
structured JSON `405 Method Not Allowed` with `Allow: POST`, while
noncanonical spellings such as `//rpc`, `/rpc/`, `/rpc;...`, `/rpc?...`,
`/rp%63`, and `/%72pc` return structured JSON `404 Unknown endpoint` without
JSON-RPC dispatch or dashboard/static fallback.
noncanonical spellings such as `//rpc`, `/%2Frpc`, `/rpc/`, `/rpc;...`,
`/rpc?...`, `/rp%63`, and `/%72pc` return structured JSON `404 Unknown
endpoint` without JSON-RPC dispatch or dashboard/static fallback.
- Missing dashboard asset URLs, including `GET`/`HEAD` requests for `/assets/*`
and extension-bearing static paths, must return structured JSON `404` errors
instead of the dashboard HTML shell; `HEAD` responses must not include a body.
Expand Down
6 changes: 4 additions & 2 deletions docs/guides/mcp.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ This is the same backend used by `keep-gpu start/status/stop/list-gpus`.
Exact `/api` and unknown `/api/*` paths return structured JSON `404` errors
instead of dashboard HTML. `/api/sessions/{job_id}` uses one raw path component;
extra raw path segments are unknown endpoints. Noncanonical raw aliases such as
`//api/sessions` are also unknown endpoints and do not start or stop sessions.
`//api/sessions` and encoded leading-slash aliases such as `/%2Fapi/gpus` are
also unknown endpoints and do not start or stop sessions.
Missing packaged asset URLs also return JSON `404` responses instead of the
dashboard shell.

Expand Down Expand Up @@ -65,7 +66,8 @@ HTTP mode is KeepGPU's local JSON-RPC/REST/dashboard service. It accepts the
same JSON-RPC message shapes at the exact `/rpc` endpoint, but it is not a
Streamable HTTP MCP endpoint. Noncanonical `/rpc` URLs, including trailing
slashes, query strings, leading double slashes such as `//rpc`, or encoded
aliases such as `/rp%63`, return structured `404 Unknown endpoint` errors.
aliases such as `/rp%63` and `/%2Frpc`, return structured `404 Unknown endpoint`
errors.

Malformed HTTP JSON-RPC bodies return a JSON-RPC `-32700 Parse error` envelope
with `id: null`; REST routes keep REST-shaped JSON errors.
Expand Down
44 changes: 44 additions & 0 deletions docs/plans/http-encoded-leading-slash-routes.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
# HTTP Encoded Leading-Slash Routes Plan

## Background

The HTTP API/RPC route classifiers rejected raw double-slash aliases such as
`//api/...` and `//rpc`, but missed aliases whose first extra slash was percent
encoded. For example, `/%2Fapi/gpus` decoded to `//api/gpus` and fell through to
static-file protection as `403`, while unsupported methods such as
`OPTIONS /%2Frpc` could leak `BaseHTTPRequestHandler` HTML `501` responses.

## Goal

Keep malformed API/RPC-looking route spellings inside KeepGPU's structured JSON
error boundary. Encoded leading-slash API/RPC aliases should return JSON
`404 Unknown endpoint`, with no dashboard/static fallback, HTML errors, or
JSON-RPC dispatch.

## Solution

- Add regression coverage for `/%2Fapi/gpus` and `/%2Frpc` across GET,
unsupported method, and pre-dispatch parse paths.
- Add route-detection candidates that include decoded leading-slash collapses
such as `//api/gpus -> /api/gpus` and `//rpc -> /rpc`.
- Use those candidates only for noncanonical route detection; canonical dispatch
still uses the actual parsed path.
- Document the encoded leading-slash examples in `AGENTS.md` and
`docs/guides/mcp.md`.

## Verification

- RED:
`PYTHONPATH=src pytest tests/mcp/test_http_api.py::test_http_rpc_noncanonical_get_returns_json_404_without_static_fallback tests/mcp/test_http_api.py::test_http_rpc_encoded_exact_alias_rejects_before_jsonrpc_parse tests/mcp/test_http_api.py::test_http_rpc_encoded_exact_alias_unsupported_method_returns_json_404 tests/mcp/test_http_api.py::test_http_encoded_api_routes_return_json_404_without_static_fallback tests/mcp/test_http_api.py::test_http_encoded_api_route_unsupported_method_returns_json_404 tests/mcp/test_http_api.py::test_http_get_api_gpus_noncanonical_route_returns_json_404_without_listing -q`
failed with `403` static fallback or HTML `501` for encoded leading-slash
paths.
- GREEN:
the same command passed with 25 tests after route candidate normalization.

## Remaining Checks

- [x] Run the MCP HTTP/server slice.
- [x] Run the full test suite.
- [x] Run `mkdocs build --strict`.
- [x] Run `pre-commit run --all-files --show-diff-on-failure`.
- [x] Run local subagent code review before PR.
19 changes: 14 additions & 5 deletions src/keep_gpu/mcp/server.py
Original file line number Diff line number Diff line change
Expand Up @@ -1044,14 +1044,23 @@ def _collapse_leading_slash_target(raw_target: Optional[str]) -> Optional[str]:
return None
return "/" + raw_target.lstrip("/")

@staticmethod
def _route_path_candidates(path: str) -> tuple[str, ...]:
candidates: list[str] = []
for candidate in (path, unquote(path)):
candidates.append(candidate)
if candidate.startswith("//"):
candidates.append("/" + candidate.lstrip("/"))
return tuple(dict.fromkeys(candidates))

@classmethod
def _is_noncanonical_api_route(
cls, parsed, raw_target: Optional[str] = None
) -> bool:
collapsed_raw = cls._collapse_leading_slash_target(raw_target)
if collapsed_raw is not None:
raw_parsed = urlparse(collapsed_raw)
route_paths = (raw_parsed.path, unquote(raw_parsed.path))
route_paths = cls._route_path_candidates(raw_parsed.path)
if any(cls._is_api_path(path) for path in route_paths):
return True
if parsed.path == "/api/gpus" and bool(
Expand All @@ -1060,7 +1069,7 @@ def _is_noncanonical_api_route(
return True
if cls._is_api_path(parsed.path):
return False
route_paths = (parsed.path, unquote(parsed.path))
route_paths = cls._route_path_candidates(parsed.path)
return any(
path.startswith(("/api/", "/api;", "/api?", "/api#"))
for path in route_paths
Expand All @@ -1073,16 +1082,16 @@ def _is_noncanonical_rpc_route(
collapsed_raw = cls._collapse_leading_slash_target(raw_target)
if collapsed_raw is not None:
raw_parsed = urlparse(collapsed_raw)
route_paths = (raw_parsed.path, unquote(raw_parsed.path))
route_paths = cls._route_path_candidates(raw_parsed.path)
if any(path == "/rpc" for path in route_paths):
return True
route_paths = (parsed.path, unquote(parsed.path))
route_paths = cls._route_path_candidates(parsed.path)
return (
any(
path.startswith(("/rpc/", "/rpc;", "/rpc?", "/rpc#"))
for path in route_paths
)
or (parsed.path != "/rpc" and unquote(parsed.path) == "/rpc")
or (parsed.path != "/rpc" and any(path == "/rpc" for path in route_paths))
or (
any(path == "/rpc" for path in route_paths)
and bool(parsed.params or parsed.query or parsed.fragment)
Expand Down
26 changes: 18 additions & 8 deletions tests/mcp/test_http_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,15 @@ def test_http_rpc_noncanonical_head_returns_json_404_without_body(rpc_path):

@pytest.mark.parametrize(
"rpc_path",
["/rpc/", "/rpc%2F", "/rpc%3Bdebug", "/rpc%3Fdebug=1", "/rp%63", "/%72pc"],
[
"/rpc/",
"/rpc%2F",
"/rpc%3Bdebug",
"/rpc%3Fdebug=1",
"/rp%63",
"/%72pc",
"/%2Frpc",
],
)
def test_http_rpc_noncanonical_get_returns_json_404_without_static_fallback(
rpc_path,
Expand Down Expand Up @@ -407,7 +415,7 @@ def test_http_rpc_noncanonical_path_rejects_before_jsonrpc_parse(rpc_path):
assert payload["error"]["message"] == "Unknown endpoint"


@pytest.mark.parametrize("rpc_path", ["/rp%63", "/%72pc"])
@pytest.mark.parametrize("rpc_path", ["/rp%63", "/%72pc", "/%2Frpc"])
def test_http_rpc_encoded_exact_alias_rejects_before_jsonrpc_parse(rpc_path):
server = make_server()
httpd, thread, base = _start_http_server(server)
Expand Down Expand Up @@ -445,7 +453,7 @@ def test_http_rpc_raw_double_slash_rejects_before_jsonrpc_parse():
assert payload == {"error": {"message": "Unknown endpoint"}}


@pytest.mark.parametrize("rpc_path", ["/rp%63", "/%72pc"])
@pytest.mark.parametrize("rpc_path", ["/rp%63", "/%72pc", "/%2Frpc"])
def test_http_rpc_encoded_exact_alias_unsupported_method_returns_json_404(rpc_path):
server = make_server()
httpd, thread, base = _start_http_server(server)
Expand Down Expand Up @@ -501,6 +509,7 @@ def test_http_unknown_api_routes_reject_unsupported_methods_with_json_404(method
"/api%2Fsessions",
"/api%2Funknown",
"/api%2Fsessions%2Fjob",
"/%2Fapi/gpus",
"/api%3Bdebug",
"/api%3Fsessions",
"/api%23sessions",
Expand Down Expand Up @@ -587,14 +596,13 @@ def test_http_raw_double_slash_api_session_delete_returns_404_without_stop():
assert [job["job_id"] for job in status_payload["active_jobs"]] == [job_id]


def test_http_encoded_api_route_unsupported_method_returns_json_404():
@pytest.mark.parametrize("path", ["/api%2Fsessions", "/%2Fapi/gpus"])
def test_http_encoded_api_route_unsupported_method_returns_json_404(path):
server = make_server()
httpd, thread, base = _start_http_server(server)

try:
status, headers, body = _request_http_response(
"OPTIONS", f"{base}/api%2Fsessions"
)
status, headers, body = _request_http_response("OPTIONS", f"{base}{path}")
finally:
httpd.shutdown()
httpd.server_close()
Expand Down Expand Up @@ -1754,7 +1762,9 @@ class _Server(TCPServer):
thread.join(timeout=2)


@pytest.mark.parametrize("path", ["/api/gpus?bad=query", "/api/gpus;bad"])
@pytest.mark.parametrize(
"path", ["/api/gpus?bad=query", "/api/gpus;bad", "/%2Fapi/gpus"]
)
def test_http_get_api_gpus_noncanonical_route_returns_json_404_without_listing(path):
server = make_server()
list_calls = []
Expand Down
Loading