feat(curl): support the options the Strands web_fetch harness uses - #131
ferdingler wants to merge 2 commits into
Conversation
…rands-agents#130) Accept -g/--globoff (no-op), -A/--user-agent, -m/--max-time (exit 28 on timeout, whole transfer incl. redirects), and --proto/--proto-redir with curl's list syntax. -w now substitutes %{content_type} and %{url_effective}. Treat a Location with any scheme:// prefix as absolute so non-http redirect targets are no longer resolved as relative paths and can be refused by --proto-redir.
|
@strandly-the-agent review pls :) |
There was a problem hiding this comment.
Changes requested — the five options are implemented correctly and track curl closely, but the PR doesn't actually close #130. With -o set, -w output is dropped (curl.rs:488-492 leaves out = None), so web_fetch's real command line still returns empty stdout. That's pre-existing and the description says so, but it's the whole point of #130 — either fold the ~3-line fix in, or drop Closes #130 so the issue doesn't auto-close while web_fetch is still broken.
Two smaller should-fixes, both new in this PR, inline: -w re-scans the server-controlled Content-Type value for %{…}/\n tokens (curl.rs:539-542), and --max-time 1e19 panics inside the builtin and the panic is swallowed → exit 0 with no output (curl.rs:222, :234).
This widens the agent-facing CLI contract (5 options, exit 28, new stderr strings); no needs-api-review label on the PR — flagging, not blocking, since it's curl parity rather than a new primitive.
✅ What I verified (d07a8d5)
cargo test --test curl_integration→ 51/51 pass. The full workspace suite was not run in my sandbox (disk/time); the PR description reports it green.-o+-wgap:curl -sSL -g --fail --proto '=http,https' --proto-redir '=http,https' --max-time 30 -A ua -o /tmp/out -w '%{content_type}\n%{url_effective}' -- $base/redirect→ exit 0, stdout""(body in the file).web_fetch.pyreadslines[-2]/lines[-1]→ both empty. Same onmain. Also:-wis skipped on the--fail(exit 22) and timeout (exit 28) early returns, where real curl still prints it.-wre-substitution: a route returningContent-Type: text/plain\nhttp://evil.example/spoofed(literal backslash-n) with-w '\n%{content_type}\n%{url_effective}'→ 3 lines instead of 2;Content-Type: text/plain;%{url_effective}→ value expanded.mainsubstituted%{content_type}→"", so this is new.--max-time 1e300/1e19/99999999999999999999→ panics (Duration::from_secs_f64/Instant + Durationoverflow), swallowed atsrc/exec.rs:2939(if let Ok(exit) = handle.await) → status 0, empty stdout/stderr. Log:strands-agents/shell/pr/131/max-time-overflow-repro.log.- Held up:
Protocols::applyvs real curl 7.88.1 on=,+,-alone (exit 2 both),+-http,-=http,=ftp,-all,=all,-http,HTTP,,http,;is_absoluteonhello?next=http://evil/anda/b://c(relative, correct);-Asent on redirect hops;-H 'user-agent:x'beats-A;-A ''sends none;-L -m 0.3on redirect→slow → 28 (deadline spans hops);-m 0no limit;-m -1→ 2;-m .5ok; refusal silent under-s, printed under-sS. - A redirect hop must pass both
--protoand--proto-redir: matches curl (man curl: "Protocols denied by --proto are not overridden by this option"). - Docs parity: no curl option list outside the
HELPstring; all five options are there (curl.rs:23-28).
Questions (non-blocking)
- Since the
-o/-wfix islet mut out = Some(io::stdout()?);plus moving the write-out block above the early returns, is there a reason to keep it out of this PR? One knock-on to decide:-iheaders would then go to stdout under-o(real curl writes them to the file; today they're dropped). - The test at
tests/curl_integration.rs:877is "web_fetch's line, less-o" — once the above lands it can run the real line, which is the test #130 actually wants.
Reading order
curl.rs:30-93 (Protocols) → :219-237 (--max-time) → :283-302 (proto check in the loop) → :355-409 (timeout wrapping; mirrored at :420-480) → :521-546 (-w). Tests: tests/curl_integration.rs:676-897.
Appendix — non-blocking (6)
- ⚪ Repeated
--protoaccumulates (curl.rs:185-190); real curl resets per flag (last wins).--proto -http --proto -https→ everything refused here, http allowed in curl. Only narrows; nobody passes it twice. - ⚪
(current_url != url)atcurl.rs:287skips--proto-redirfor a redirect back to the original URL (self-loop → 10 hops, exit 0). Contrived. - ⚪ Strictness:
--proto '=http, https'(space) exits 2; curl also rejects it.+++exits 2 where curl warns and continues. Fine either way. - ⚪ Test gaps on documented behaviour:
-A '',--max-time 0,-mspanning redirect hops. ~3 lines each. - ⚪ Pre-existing, filed #132:
Location: //host/path(protocol-relative) is resolved as a path on the current host. Untouched by this PR. - ⚪ The redirect+timeout handling is now verbatim in both arms of
if current_url == url(:355-409,:420-480); this PR addedwithin(...)and four error arms twice. A helper would halve the next change here.
Reviewed by strandly-the-agent — correctness + adversarial passes in fresh contexts, synthesized here. Solid work to be checked by a human, not a verdict.
I was already aware about the |
Address review on strands-agents#131: - `-w` always goes to stdout; `-o` takes only the body and, as in curl, the `-i` headers. `-w` is also written on the `--fail` (22) and timeout (28) exits, with `%{http_code}` `000` when no response came. - `-w` expands in one pass, so a server's `Content-Type` is never read as a variable or an escape. - A `--max-time` too large to represent is no limit, not a panic. - Each `--proto` / `--proto-redir` replaces the one before it. The web_fetch test now runs the real command line, `-o` included.
|
Thanks for the review. Addressed in 325d590:
Left out on purpose: the request-block helper (a refactor, better as its own PR), and the redirect back to the original URL. |
There was a problem hiding this comment.
Approve — 325d590 fixes all four of my findings, and the fixes hold up under attack. web_fetch's real command line (with -o) now returns the content type and final URL on stdout, so Closes #130 is earned. The four inline threads below are verified-fixed; strandly-the-agent lacks permission to resolve threads on this repo, so a maintainer should collapse them.
Status of previous findings:
-wdropped under-o/ on--fail+ timeout exits → fixed (curl.rs:565,:587-596,:413-422;000when no response, matching curl).-wre-scanning the server'sContent-Type→ fixed by the one-passexpand_write_out(curl.rs:117-148), pinned by/tricky-content-type.--max-timeoverflow panic → fixed (try_from_secs_f64+checked_add,curl.rs:270-284);0/1e19/1e300/99999999999999999999tested.- web_fetch test "less
-o" → fixed, runs the real line.
✅ Verified (325d590)
cargo test --test curl_integration→ 60/60;cargo fmt --checkclean on both files. Full workspace suite not run in my sandbox.- Attacked the fixes with throwaway tests (reverted):
-sSf -o /tmp/x -w … /status/404→ exit 22, trailer still printed; unknown%{nope}and unterminated%{http_codekept verbatim, no panic;-won a kernel-denied URL → exit 1 with no trailer (fine —web_fetchchecks the exit code before parsing); web_fetch line against anftp://redirect → exit 1Protocol "ftp" not supported. Log:strands-agents/shell/pr/131/curl-integration-325d590.log. -iwith-onow puts headers in the file, as curl does.- Repeated
--proto: last wins (Protocols::ALL.apply), matching curl.
Appendix — non-blocking (2)
- ⚪
expand_write_outdoesn't handle%%(curl: literal%), so-w '%%{http_code}'prints%200where curl prints%{http_code};\t/\rescapes also unhandled. Pre-existing behaviour, unchanged by this PR. - ⚪
-wis skipped on the exit-1 (kernel deny) and exit-6 paths; curl prints it there too. Harmless forweb_fetch.
Reviewed by strandly-the-agent — follow-up pass on the delta only, run in one context. Solid work to be checked by a human, not a verdict.
Description
The builtin
curlrejected several options that the Strands harnessweb_fetchtool passes. This adds them, soweb_fetch's command line runs unchanged.Options:
-g/--globoff: accepted, no-op (the builtin never globs).-A/--user-agent: sets User-Agent on every hop. An explicit-H 'User-Agent: …'takes precedence;-A ''sends none.-m/--max-time: bounds the whole transfer, redirects included (tokio::time::timeout_at). On timeout, exits 28 withcurl: (28) Operation timed out.0, or a value too large to represent, means no limit; a non-number exits 2. On wasm32 it is accepted but not enforced, because WASI HTTP calls block.--proto/--proto-redir: curl's list syntax (=,+,-,all, applied left to right; unknown protocols ignored; malformed modifier exits 2). A refused URL exits 1 withProtocol "x" disabled(http/https) ornot supported(anything else). A redirect hop must pass both sets, and each flag replaces the one before it. These only narrow what is allowed;check_url/SafeResolverstill decide every request.-w: now substitutes%{content_type}(from the response header) and%{url_effective}(final URL after redirects). It expands in one pass, so a substituted value is never re-read as a variable or escape.-wwith-o:-walways goes to stdout, and-otakes only the body and, as in curl, the-iheaders. Before,-odropped-woutput, soweb_fetchgot nothing back.-wis also written on the--fail(22) and timeout (28) exits, with%{http_code}000when no response came.Related fix: a
Locationwith anyscheme://prefix is now treated as absolute. Before, e.g.ftp://…was resolved as a relative path (<base>/ftp://…), so--proto-redircould never see it.Related Issues
Closes #130
Documentation PR
N/A (the
curl --helptext is updated in this PR).Type of Change
New feature
Testing
23 new tests in
tests/curl_integration.rs, including one that runsweb_fetch's full command line,-oincluded.cargo test --workspace --all-targets: all green.cargo fmt --checkclean.cargo clippy --workspace --all-targets: no new warnings (17 before and after, all pre-existing in other files).cargo docwith-D warnings: clean.Manual CLI run of
web_fetch's command against example.com and an httpbin redirect: correct content type and final URL.--max-time 1on httpbin/delay/5exited 28.Not done: wasm32 build (target not installed locally). Python/Node bindings are untouched, so
pytest/npm testwere not run.I ran the relevant test suites for the bindings I touched (
cargo test --workspace --all-targets,pytest tests/python,npm test)If I touched Rust, I ran
cargo fmtandcargo clippyChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.