7d4e512cb09aff8601e81e7c174ed430f1157be7
5
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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> |
||
|
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
|
||
|
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>
|
||
|
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> |
||
|
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>
|