tao.chen/mcp-server
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
129 Commits 2 Branches 0 Tags
307da276bf3efe00418103a056b56f8ab35b61e3
Commit Graph
7 Commits
Author SHA1 Message Date
ClaudeandClaude Fable 5 16fe011fa0 fix(fetch_url): surface network errors as 400 with detail 
Previously, when httpx.get raised an HTTPError (ConnectError for host
unreachable, ReadTimeout for slow servers, RemoteProtocolError, etc.)
the exception bubbled up through the route handler as a bare 500
"Internal Server Error". The LLM got no information about what
actually went wrong — could not tell whether the host was down, the
port was closed, DNS failed, TLS handshake broke, or the request
timed out. The only thing the agent could do was guess.

Wrap the redirect loop in try/except for httpx.HTTPError and
translate to ValueError. The existing exception handler in
server.py turns ValueError into HTTP 400 with the message in the
response detail, so the LLM now sees e.g.:

  fetch_url could not reach 'http://nm01.prod.internal:8042/':
  ConnectError: Connection refused. Check that the URL is reachable
  from the MCP service, the host is in Connection.url_allowlist, and
  the connection's auth/SSL settings are correct.

The original exception is chained via `raise ... from exc` so loguru
still records the full traceback with the original type, and the
`__cause__` attribute is set on the ValueError for programmatic
inspection.

Note: upstream HTTP 4xx/5xx responses (server replied, even with an
error status) are NOT translated — the FetchUrlResult carries the
status code and body so the LLM can read what the server actually
said. This is the intentional contrast with the no-response-at-all
case (which now has clear 400 detail).

Tests (3 new in tests/unit/test_fetch_url.py):
  - test_fetch_url_raises_400_with_detail_on_connect_error
    ConnectError("Connection refused") -> ValueError with
    "ConnectError", "Connection refused", the URL, and __cause__
    chained.
  - test_fetch_url_raises_400_with_detail_on_timeout
    ReadTimeout("Timed out reading") -> ValueError with
    "ReadTimeout", "Timed out reading", __cause__ chained.
  - test_fetch_url_returns_body_for_4xx_5xx_upstream
    Upstream 503 with body "Service Unavailable - try again later"
    -> FetchUrlResult(status_code=503, body=...). Proves the
    intentional contrast.

Route description in server.py updated with a new **Errors** section
explaining the two error paths (no response = 400 with detail, got
a response = body returned).

Tests: 401 passed (was 398, +3 net).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
 2026-07-09 18:00:03 +08:00
ClaudeandClaude deafb5a26b refactor: drop fetch_url body cap and modernize datetime usage 
- fetch_url: remove the 1MB body cap and the streaming helper. The
  manual redirect loop with allowlist re-check (the SSRF fix) is kept
  intact; the body is now read in full via resp.text. Drop the now-
  meaningless `truncated` field from FetchUrlResult and the tests that
  asserted on it. Switched from httpx.stream() back to httpx.get()
  for the redirect loop — cleaner without the body cap.

- datetime: replace deprecated datetime.utcnow() with
  datetime.now(timezone.utc) in submit.py (3 sites) and
  core/job_writer.py (1 site). Update the stale comment in
  core/pending_store.py that referenced the old call.

- Clean up an unused `from common.config import settings` import in
  submit.py that ruff flagged.

Co-Authored-By: Claude <noreply@anthropic.com>
 2026-07-09 14:35:16 +08:00
ClaudeandClaude 7d4e512cb0 fix: address review findings on fetch-url-tool 
- fetch_url: revalidate allowlist on every redirect hop (fixes SSRF where
  302 to disallowed host / 169.254.169.254 / file:// bypassed the
  url_allowlist). Stream response body with iter_bytes and cap at 1MB
  so a multi-GB response from an allowlisted host cannot OOM the service.
  Reuses the manual-redirect-loop pattern from yarn_client.

- list_applications: stop swallowing 404 (YARN returns 200+empty for
  "no match"; 404 means the RM doesn't support the endpoint — surface
  the YarnError instead of hiding it as an empty result). Add
  Field(ge=1, le=10000) to ListApplicationsRequest.limit so a runaway
  limit is rejected at the Pydantic layer with 422.

- save_connection: PATCH semantics for existing records. Re-route to
  update_connection when the name already exists so partial updates
  (e.g. only master) no longer wipe url_allowlist back to []. Uses an
  _UNSET sentinel in the tool function to distinguish "omitted" from
  "None" without breaking the existing parameter list.

- README: drop leading space on 5 new connection-tool table rows that
  was breaking GitHub Flavored Markdown table continuity.

- Indentation: normalize connections.py and requests.py to 4-space
  indent (auth_password/auth_principal/auth_keytab were 3-space).

Co-Authored-By: Claude <noreply@anthropic.com>
 2026-07-09 14:23:16 +08:00
ClaudeandClaude Fable 5 a5b9539663 refactor(fetch_url): trust the allowlist, rename to url_allowlist 
Two changes:

1) Drop every check except the allowlist lookup.

  Old _validate_url_host did: scheme check, host-presence check,
  IP-literal check, empty-allowlist check, then glob match.

  New _validate_url_host does: parse host, return on glob match,
  raise on miss. That's it. The only remaining structural check is
  'the URL must have a host' (otherwise the glob has nothing to
  test against).

  Security implication: scheme (file://, gopher://, ftp://) and
  IP literals (10.0.0.1, ::1) are NO LONGER rejected by the
  validator. The allowlist is the single source of truth. If the
  user writes ['*.*.*.*'], they have opted in to 4-label hosts
  including IP literals; if they write ['ccam*'], they get ccam1-
  ccam99 and nothing else. The default ['ccam*'] / [] pattern is
  tight by construction.

  Removed: import ipaddress, the scheme/IP rejection branches, the
  'allowlist empty' explicit branch (the empty list naturally
  matches nothing).

2) Rename allowed_url_hosts -> url_allowlist.

  The previous name was a verbose double-negative ('allowed ... hosts').
  The new name is short, modern (allowlist > whitelist), and matches
  the pattern of the field (URL hosts allowed). Renamed in:
    - Connection (models.py)
    - SaveConnectionRequest, UpdateConnectionRequest, FetchUrlRequest
      (requests.py)
    - _validate_url_host, _host_matches_any_glob parameters
      (fetch_url.py)
    - save_connection / update_connection call sites and error
      messages (connections.py, fetch_url.py)
    - Route descriptions (server.py)
    - All test files
    - README

  No backward-compat alias: the field was added in 0291f36 and
  hasn't shipped, so no production migration. Local dev data
  (data/connections.json, gitignored) with allowed_url_hosts set
  will be silently dropped by Pydantic v2 (default for extra
  fields is ignore) — those connections lose their allowlist and
  fetch_url will reject everything until re-saved.

Test cleanup:
  - Removed 9 obsolete tests (scheme/IP/suffix rejection)
  - Renamed allowed_url_hosts -> url_allowlist in 11 surviving tests
  - Added 4 new tests documenting the 'allowlist is the only gate'
    model: IP literal accepted, HTTPS accepted, no-host rejected,
    empty allowlist rejected, error message mentions url_allowlist

  -1 obsolete test, net -4 from 386 -> 382 tests passing.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
 2026-07-09 12:09:41 +08:00
ClaudeandClaude Fable 5 6d7387b022 feat: simplify fetch_url allowlist + add update_connection tool 
Part 1 — fetch_url validation simplification
  Now that Connection.allowed_url_hosts exists, it's the ONLY check.
  The old 2-label suffix-overlap rule against yarn_rm_url is gone.

  - Connection.allowed_url_hosts: list[str] = Field(default_factory=list)
    (was list[str] | None = None). Default empty list means the
    connection has no fetch access; the user must explicitly opt in
    via save_connection or update_connection.
  - _validate_url_host drops the yarn_rm_url parameter and the
    _host_suffix_overlap helper. Now: scheme/host/IP checks, then
    empty-reject, then glob match. Reject everything else.
  - fetch_url's error message now points the user at
    Connection.allowed_url_hosts as the fix.
  - The label-by-label glob check from the previous commit is kept
    (SSRF guard: 'ccam*' matches 'ccam50' but NOT 'ccam50.evil.com').

Part 2 — update_connection tool
  PATCH-style update for an existing Connection record. Only the
  fields the caller provides are changed. Same pattern as
  update_pending_job: req.model_dump(exclude_none=True), with
  'name' popped before passing to the store.

  To CLEAR a field (e.g. drop auth_password), use delete_connection
  + save_connection. This is YAGNI; the alternative (model_fields_set
  to distinguish 'omitted' from 'null') adds surface for bugs.

  - ConnectionStore.update(name, **fields): get, model_copy(update=...),
    save. Lock + atomic write. Re-validates the patched record.
  - update_connection tool function: passes fields through to the
    store, logs which fields were changed.
  - UpdateConnectionRequest Pydantic model: 12 mutable fields + name.
  - /update_connection route with operation_id='update_connection'.
  - 9 new unit tests in test_connection_tools.py (PATCH, dict/list
    replace-not-merge, unknown name 404, validation of patched record,
    disk persistence).
  - test_fetch_url.py: existing 21 tests updated; 3 new tests for
    the empty/None/missing allowed_url_hosts cases.
  - test_mcp_routes.py: assert 22 tool routes.
  - README: update_connection row added; fetch_url row updated.

Tests: 386 passed (up from 377, +9 net).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
 2026-07-09 11:46:07 +08:00
ClaudeandClaude Fable 5 0291f36b01 feat(fetch_url): per-Connection allowed_url_hosts glob allowlist 
The default 'URL host shares >= 2 labels of suffix with yarn_rm_url
host' rule is too strict for clusters whose hostnames are single-label
(e.g. 'ccam1' through 'ccam99'). The user's cluster is reachable at
http://ccam1:8088, and they want fetch_url to work for any ccamN
host — but ccam1 and ccam50 share 0 suffix labels, so the existing
rule rejects everything.

Add an opt-in allowlist field on Connection:

  allowed_url_hosts: list[str] | None

Each entry is an fnmatch glob pattern. The URL host is allowed if it
matches ANY pattern, regardless of the suffix rule. Save a Connection
with ['ccam*'] to allow ccam1, ccam2, ..., ccam99.

SSRF safety: the implementation does NOT use vanilla fnmatch on the
flat host string (because '*' in fnmatch crosses '.', so 'ccam*' would
match 'ccam50.evil.com' — a security hole). Instead, both the pattern
and the host are split on '.' and matched LABEL-BY-LABEL with the
label counts required to match exactly. So 'ccam*' matches 'ccam50'
but NOT 'ccam50.evil.com' (different label counts).

Test coverage:
- 8 new tests in tests/unit/test_fetch_url.py (glob allow, glob deny,
  dot-boundary SSRF test, multiple globs, empty list, None fallback,
  error message hint)
- All 21 fetch_url tests pass; 377 total.

- spark_executor/models.py: Connection.allowed_url_hosts (with description)
- spark_executor/tools/requests.py: SaveConnectionRequest.allowed_url_hosts
- spark_executor/tools/fetch_url.py: new _host_matches_any_glob helper;
  _validate_url_host now takes allowed_hosts and checks globs BEFORE
  the suffix rule
- tests/unit/test_fetch_url.py: 8 new tests
- README.md: fetch_url row mentions the allowlist

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
 2026-07-09 11:22:55 +08:00
ClaudeandClaude Fable 5 627e70f697 feat: add fetch_url tool for proxying HTTP GET to cluster-internal URLs 
Add a new MCP tool that lets the agent fetch URLs on the cluster's
network (YARN tracking UI, Spark History Server, NodeManager web UIs)
when the agent is on a different network and cannot reach those hosts
directly.

The MCP service runs on the YARN RM node, so it can reach every host
the cluster knows about — the agent just needs a way to ask.

Security: SSRF guard via host suffix overlap
  - URL host must share >= 2 labels of suffix with the named
    Connection's yarn_rm_url host (e.g. yarn_rm_url='rm.prod.internal'
    allows 'http://nm01.prod.internal/...')
  - IP literals (10.0.0.1, ::1) rejected
  - Non-http(s) schemes (file://, gopher://, ftp://) rejected
  - Connection with no yarn_rm_url cannot use this tool
- Reuses Connection.auth_for_httpx() and verify_for_httpx() so the
  agent does not need cluster credentials
- Response body capped at 1 MB (truncated=true if larger)
- 30s timeout, follows redirects, loguru INFO audit log on every call

- spark_executor/tools/fetch_url.py: new tool + 2 helpers
  (_host_suffix_overlap, _validate_url_host)
- spark_executor/models.py: FetchUrlResult Pydantic model
- spark_executor/tools/requests.py: FetchUrlRequest with descriptions
- spark_executor/server.py: /fetch_url route, operation_id='fetch_url'
- tests/unit/test_fetch_url.py: 13 unit tests covering all guards,
  truncation, auth/SSL pass-through, redirect follow
- tests/integration/test_mcp_routes.py: assert 21 tool routes
- README.md: 1 row in Spark Executor 工具 table

Tests: 369 passed (up from 356).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
 2026-07-09 10:59:25 +08:00