[marcelklehr]

🤖 AI (if applicable)

• The content of this PR was partly or fully generated using AI (N/A)

[WSHAPER]

Disclaimer: Claude Sonnet 5.5 Review

The approach is good (urlparse instead of startswith, exact host/port matching, per-hop redirect validation, streamed reads capped at 100 kB), but a few things need fixing or confirming first.

Should fix

1. Malformed URLs will raise instead of returning an error. urlparse(url).path raises ValueError on something like http://[::1, and parsed.port raises on host:abc or host:99999. The URL comes from the model, so treat it as untrusted. Today this propagates out of is_an_internal_file_link and _split_authority_path, so web_fetch crashes instead of returning a clean message. Wrap both in try/except and return False or an error string.

2. is_an_internal_file_link went from sync to async. Any caller that still calls it without await gets a coroutine, which is always truthy, so every URL would count as an internal link and fail silently. Please grep for other callers (other tools, tests) to confirm they're all updated.

3. lib/url_guard.py isn't in this diff. web.py imports BlockedUrlError, PublicAddressResolver and assert_public_url from it. I couldn't review the core of the SSRF logic. I'd also verify that niquests.AsyncSession(resolver=...) accepts your resolver class as written, since resolver interfaces are easy to get subtly wrong.

Worth a look

4. Scheme isn't checked before the Nextcloud-host bypass. is_known_nc_host is scheme-agnostic, so ftp://cloud.example.com/... skips assert_public_url, which is where the http/https check lives. niquests will probably reject it anyway and you catch RequestException, but a cheap scheme in ('http', 'https') check at the top of validate_hop is better than relying on that.

5. Redirect loop edge case. On the last iteration a redirect closes res and validates next_url, then falls into the for/else, which calls res.close() a second time. It's probably harmless, but it's wasted work and a double close.

6. Proxies. With an env proxy, the proxy resolves the target itself, so the connect-time rebinding protection doesn't apply there. Your comment says as much; just make sure that's an accepted trade-off. NO_PROXY is also ignored by the getproxies() filter.

Nits

• The docstring rewording in files.py ("agree with the pattern", "with the internal link to that file") is unrelated churn, and "must match" was clearer. This text is what the LLM reads, so I'd revert it.
• The two todo: comments (multimodal, PR #211) don't belong in this change.
• There's no test coverage in the diff. The cases worth testing are cloud.example.com.evil.net, a /f/123 in the query string, a sub-path install, redirect to a private IP, and a server that ignores Range.

This is also several changes in one: URL matching, SSRF guard, streaming, and error-format changes (HTTP errors now return a message instead of raw text). Splitting it up would help review. If it stays as one commit, the message should explain why: the old startswith check was bypassable via host-prefix tricks, and web_fetch could be redirected to internal targets.

[marcelklehr]

"crashes" is overstated. graph.py:82 (handle_tool_error) catches tool exceptions and passes them back to the model as an error message

[marcelklehr]

is_an_internal_file_link went from sync to async.

Valid concern, nothing to fix. The only caller is web.py:39, and it awaits.