[poshinchen]

Replica of https://github.com/strands-agents/harness-sdk/pull/4756 (backticked: no cross-reference). Below: the v4-reason optimized review and a sanitized copy of live Strandly's actual comments.

[poshinchen]

Strandly v4-reason (optimized) review — run af1746, Opus 4.8

Assessment: comment

Assessment: Comment

Well-structured addition with careful shell quoting, an injection-resistant curl invocation, and strong test coverage (both command-shape mocks and a live curl-against-stdlib-server contract suite). A few items are worth addressing before merge.

Review Categories

• Robustness: The common timeout=None path passes no curl timeout and no exec timeout, so a stalled connection can hang indefinitely — the most impactful concern.
• Security: Temp artifacts live in shared /tmp; worth confirming the sandbox FS is single-tenant or using a private mktemp -d dir to avoid symlink redirection.
• Documentation: The public request() Raises: contract omits the synchronous validation errors it can raise; PR description says fetch() but implements request().
• Testing: Behavioral contract is skipped when curl is absent — ensure CI guarantees curl.
• Nit: Comment typo ("equest").

Nicely considered edge-case handling around redirects, caps, and 304 responses.

[poshinchen]

[Important]
Issue: When timeout is None, no --max-time (or --connect-timeout) is passed to curl and exec_timeout is also None, so a stalled connection or a slow-loris server can hang request() indefinitely with no way to recover. The default (most common) call omits timeout, so the default behavior is "no timeout at all".
Suggestion: Consider always passing a --connect-timeout (and/or a sane default overall timeout) even when timeout is None, so the common path is bounded against transport-level hangs. If an unbounded default is intentional, please document that explicitly in the request() docstring and the PR description.

[poshinchen]

[Suggestion]
Issue: Typo in the comment — "smuggle a second equest" should read "request".
Suggestion:

Suggested change

	# Anything outside this set (or a CR/LF/NUL in a value) could inject additional headers
	# Anything outside this set (or a CR/LF/NUL in a value) could inject additional headers
	# or smuggle a second request, so header names are validated against this pattern.

[poshinchen]

[Important]
Issue: The base request() docstring's Raises: section only lists network/transport errors and the max_bytes overflow. The actual implementation also raises SandboxHttpError for input-validation failures (unsupported method, non-http(s) scheme, missing host, URL chars outside the RFC 3986 allowlist, and header name/value injection). Callers reading the public API contract won't know these synchronous validation errors share the same exception type.
Suggestion: Document the validation failures in the Raises: section here (and mirror in the PosixShellSandbox.request docstring), e.g. "SandboxHttpError: On invalid method/URL/headers, network/transport errors, or when the response body exceeds max_bytes."

[poshinchen]

[Suggestion]
Issue: Temp files are created under the world-writable, shared /tmp with names derived from uuid4(). In a multi-tenant sandbox a pre-existing symlink at /tmp/strands-http-<tag> (or a race) could cause curl's -o/-D writes or the : > body_file truncation to land on an attacker-chosen path. The uuid makes collisions unlikely but doesn't defend against a planted symlink.
Suggestion: Consider writing to a per-request private directory created with restrictive perms (e.g. mktemp -d) rather than individual files in the /tmp root, so a planted symlink can't redirect the writes. If the sandbox's filesystem is already assumed single-tenant/isolated, a short note documenting that assumption would suffice.

[poshinchen]

[Suggestion]
Issue: write_file(data_file, ...) for the request body happens before the try/finally that performs the rm -f cleanup. If self.execute(cmd) is never reached but the data file was written (it isn't here since write precedes the try, but note the finally only runs for files created inside the try), the data file is still removed by the finally since its name is in the rm -f list — good. However, if write_file itself raises, no cleanup runs and the body file is already on disk.
Suggestion: Consider moving the body write_file inside the try block so the finally cleanup also covers a partially-written data file on write failure.

[poshinchen]

[Suggestion]
Issue: TestRequestLive is gated on curl being installed (_CURL_AVAILABLE). On CI images without curl, the entire behavioral contract (status/headers/body/resolved_url, cap enforcement, redirects) is silently skipped, leaving only the command-shape mock tests — which can't catch a real curl behavior regression.
Suggestion: Ensure curl is guaranteed present in the CI environment that runs this suite (or make its absence a hard failure there), so the live contract tests can't silently no-op in CI.

[poshinchen]

Live Strandly's actual comments on the original PR 4756 (sanitized copy: mentions escaped, links backticked)

Review (CHANGES_REQUESTED):

Changes requested — the curl transport works end-to-end on the happy path (I ran it for real against local servers), but two of the guarantees the new API advertises don't hold: header values go on the wire unvalidated (CRLF → request smuggling), and max_bytes doesn't bound the download for chunked responses (4.3 GB pulled with max_bytes=100). Both are in PosixShellSandbox.request, both have inline repros + fixes.

Also: this adds a new public primitive (Sandbox.request, HttpResult, SandboxHttpError) and the PR has neither api/needs-review nor api/review-complete — it should go through API review before leaving draft, and I shouldn't be the one to approve the shape solo. Questions for that review are below the fold.

✅ Verified

• Head 91e50a70, base origin/main @ 0ab57b24.
• pytest tests/strands/sandbox → 149 passed; ruff check/format --check clean.
• Real-curl e2e (curl 7.88.1, sh -c sandbox via the repo's own _ShellTestSandbox, local http.server + raw-socket servers): GET/POST/PUT/DELETE/HEAD, redirects → final-hop headers only + resolved_url, 4xx returned not raised, repeated Set-Cookie joined, timeout → SandboxHttpError, DNS failure → SandboxHttpError, temp files cleaned up on success/failure/cancellation, shlex.quote keeps $()/backticks/quotes inert in URLs and header values, -g neutralises [], --proto-redir blocks file:/ftp: redirects, header parser handles HTTP/2 status lines, 100 Continue, obs-fold, bare LF. All passes.
• Each inline finding was reproduced with real curl; scripts + logs are attached as artifacts (e2e-evidence.log, repros.py/repros.log, harness.py).

Questions (for API review, non-blocking here)

1. Error taxonomy. Argument validation (:319-331) raises SandboxHttpError, while the rest of this module raises ValueError for bad input (:51, :195). A timeout is re-wrapped from SandboxTimeoutError into SandboxHttpError (:385-386), so except TimeoutError — the thing errors.py says the hierarchy exists for — no longer catches it. Would it be simpler to raise ValueError for bad args and let SandboxTimeoutError through?
2. Optional capability shape. request() is the first non-abstract method on the ABC that raises NotImplementedError by default. A consumer (web_fetch) can't probe for support except by catching that. Is that the pattern we want, or a supports_request/Protocol? Related: default Agent() uses NotASandboxLocalEnvironment → NotImplementedError (verified); where does the host-side fallback live once feat(vended-tools): route web-fetch through the sandbox strands-agents/harness-sdk#4298 switches to this?
3. TS parity. posix_shell.py:8 says it mirrors posix-shell.ts, which has no request. Is TS a follow-up, or is the Python ABC meant to diverge?

Reading order

1. posix_shell.py:299-407 — request(); the inline findings are all here.
2. base.py:203-237 — the contract

Review (COMMENTED):

Re-review of 1cb4036 — both 🔴s and all three 🟡s from my last pass are fixed, verified with real curl. One new 🟡 the fix introduced: head -c {max_bytes} lets responses that overshoot the cap by up to ~8 KB come back silently truncated with status 200, which is exactly what the new base.py:227-229 docstring promises can't happen. Two-line fix inline. Still a new public primitive with no api/needs-review label, so not approving solo.

Previous findings — status

#	Finding	Status
🔴	CRLF in header value → injection	Fixed (:398-405): CR/LF/NUL in values and non-token names (X\r\nY, @foo, X:Y, X Y) all raise before anything runs; empty value now sent as X-Empty: instead of deleting the header.
🔴	max_bytes unbounded for chunked	Fixed-but-introduced-X: streaming chunked endpoint with max_bytes=100 now raises exceeded max_bytes in 0.0 s with 0.01 MB pushed (was 4.3 GB). Raise-vs-truncate is now one semantics on paper — but see the new inline on :436.
🟡	304 → FileNotFoundError	Fixed (: > body && at :444): 304 → status=304, body=b"" on both capped and uncapped paths.
🟡	-X POST -L bodiless POST	Fixed (:416-421): POST+body through 302 → target sees GET; PUT+body through 302 still replays a bodiless PUT (curl needs -X for PUT) — appendix.
🟡	HEAD + max_bytes → curl (63)	Fixed (capped = … and not is_head): HEAD on 200 KB resource with max_bytes=10 → 200.
🟡	Tests never execute curl	Fixed: TestRequestLive (:976) runs 10 cases against a real http.server via _ShellTestSandbox.

✅ Verified

• Head 1cb4036; pytest tests/strands/sandbox → 152 passed; ruff check / format --check clean (mypy hit an internal error in my env, not run).
• Re-ran all round-1 repros plus the new ones against head: capped path handles 404 (returned), DNS failure (curl: (6) text preserved), --max-time (curl: (28)), redirect (resolved_url = final via the %{stderr}STRANDS_URL: tag), max_bytes=0 (raises), and leaves 0 temp files. Scripts + logs attached as round2-*.

Appendix — non-blocking (3)

• posix_shell.py:416-421: -X PUT/PATCH + -L still replays a bodiless request through 302/303 (verified PUT body 302 → saw PUT body=). httpx switches to GET for 302/303 regardless of method. Could use --upload-file for PUT, or document that only GET/POST follow browser redirect semantics.
• posix_shell.py:395: timeout=0 still reaches curl as --max-time 0 (= no limit) while exec_timeout becomes 5 s → "request timed out after 0s". Reject <= 0.
• Round-1 appendix items (urlparse ValueError for http://[::1, write_file 128 KB body ceiling, curl's implicit Content-Type on bodies, userinfo in resolved_url, _URL_CHARS rejecting {}|^, PR body still says "abstract method") are unchanged.

Inline at strands-py/src/strands/sandbox/posix_shell.py:358:

🔴 Header values go on the wire unvalidated — CRLF in a value injects headers / a second request.

shlex.quote protects the shell, not HTTP framing. Repro (raw-socket server, real curl 7.88.1):

headers={"X-Trace": "abc\r\nX-Injected: yes"}
→ wire: ... X-Trace: abc\r\nX-Injected: yes\r\n
headers={"X-A": "1\r\n\r\nGET /smuggled HTTP/1.1\r\nHost: evil.example\r\n\r\n"}
→ server saw a second request GET /smuggled on the same connection

The motivating consumer passes model-chosen header dicts through, and the host-side http_request tool (httpx) rejects CR/LF, so this is a regression for the sandboxed path. Method and URL are allowlisted a few lines up as "security-critical"; headers need the same treatment. Two smaller curl-isms fall out of the same check: an empty value (-H 'X-A: ') makes curl delete the header ({"User-Agent": ""} → no UA sent), and a name starting with @ is read as @file.

Suggestion (with _HEADER_NAME = re.compile(r"[!#$%&'*+\-.^_|~0-9A-Za-z]+")next to_URL_CHARS`):

            for n, v in headers.items():
                if not _HEADER_NAME.fullmatch(n) or re.search(r"[\r\n\0]", v):
                    raise SandboxHttpError(f"invalid header {n!r}: name must be an RFC 7230 token and value may not contain CR/LF/NUL")
                opts += f" -H {shlex.quote(f'{n};' if v == '' else f'{n}: {v}')}"

Inline at strands-py/src/strands/sandbox/posix_shell.py:376:

🔴 max_bytes doesn't bound the download for chunked responses, and means different things depending on the server.

--max-filesize only fires when Content-Length is known (curl < 8.4; this sandbox has 7.88.1), and head -c runs after curl has already written the whole body to /tmp. Repro against a streaming chunked endpoint, max_bytes=100, timeout=8:

SandboxHttpError: curl: (28) Operation timed out after 8005 milliseconds with 4330897714 bytes received
server pushed 4350.7 MB while max_bytes was 100

With timeout=None that's unbounded. And the same call either raises (Content-Length present → curl: (63)) or silently returns a 200 with a cut-off body (chunked), with nothing on HttpResult saying it was truncated — so a consumer hands the model a partial page as if complete.

Suggestion: stop the transfer at the cap instead of trimming afterwards, and pick one semantics. I verified this shape on 7.88.1 — download stops at 100 bytes, url_effective still recoverable:

curl -sS ... -D hdrs -o - -w '%{stderr}%{url_effective}' -- URL | head -c N > body
# curl exits 23 ("Failure writing output") when head closes the pipe → that's the `truncated` signal

Then either always truncate and expose truncated: bool on HttpResult, or always raise — but not both depending on the remote. Also drop --max-filesize for HEAD: curl checks it against Content-Length even with --head, so HEAD on anything larger than max_bytes fails with curl: (63) although nothing is downloaded (repro in repros.log R4).

Inline at strands-py/src/strands/sandbox/posix_shell.py:465:

🟡 304 Not Modified → bare FileNotFoundError. curl exits 0 but doesn't create the -o file for a 304 (it does for 204 / empty 200 — checked all three), so read_file fails:

GET + If-None-Match → 304:  FileNotFoundError: sh: 1: cannot open /tmp/strands-http-…: No such file
same with max_bytes=1024:   SandboxHttpError: head: cannot open '/tmp/strands-http-…' for reading

Any conditional GET hits this; the docstring says 3xx comes back in status. Simplest fix: create the body file before curl runs (cmd = f": > {body_quoted} && " + cmd, or touch) so read_file always finds a file; alternatively catch FileNotFoundError here and return b"".

Inline at strands-py/src/strands/sandbox/posix_shell.py:362:

🟡 -X POST + -L replays a bodiless POST through 302/303 redirects. With -X, curl keeps the method on redirect but drops the body; without -X it switches to GET like browsers/httpx do. Verified:

-X POST -L, 302 → target saw: method=POST body=b''
no -X,    -L, 302 → target saw: method=GET
no -X,    -L, 307 → target saw: method=POST body=b'payload'

Any http→https or trailing-slash redirect on a POST delivers an empty POST and the caller sees a 4xx that looks like a payload bug. curl's own manual warns against -X POST for this reason. Only force the method where curl can't infer it:

        method_opt = "--head" if is_head else ("" if method == "GET" or (method == "POST" and body is not None) else f"-X {method}")

Inline at strands-py/tests/strands/sandbox/test_posix_shell.py:675:

🟡 These tests assert substrings of the curl command against a canned mock; none execute curl. Every finding in this review (CRLF on the wire, unbounded chunked download, 304 → FileNotFoundError, bodiless POST on redirect) passes this suite. The same file already has _ShellTestSandbox(PosixShellSandbox) at :123 running real sh -c, which every other test here uses — pointing request() at a local http.server through it costs ~3 s and would have caught all of them (my 20-check port is attached as e2e_request.py). Keep the command-shape assertions if you like, but the contract (status, headers, body, resolved_url, raise-vs-return) should be proven against a real response at least once.

Inline at strands-py/src/strands/sandbox/posix_shell.py:436:

🟡 Overshoots of up to ~8 KB come back silently truncated with status 200 — the base.py:227-229 contract ("raises rather than returning a truncated body") doesn't hold.

curl exits 23 only if it still has bytes to write after head closes the pipe. When the whole body arrives in curl's first read, curl writes it in one go, exits 0, and head just keeps the first N bytes. Deterministic for small overshoots, racy near the boundary (6 runs each, max_bytes=100):

CL=   101 -> 200/len100 ×6        CL= 4000 -> 200/len100 ×6
CL=  8000 -> 200/len100, RAISE ×5  CL=16000 -> RAISE ×6

So the one case max_bytes is most likely to hit in practice — a page just over the cap — returns a cut-off body that looks complete. Fix: read one byte past the cap and check the length, so the decision doesn't depend on pipe timing (keeps the download bounded, and exit 23 still works as the fast path):

                f" 2> {err_quoted} | head -c {max_bytes + 1} > {body_quoted}"

…plus, after response_body is read (:465): if capped and len(response_body) > max_bytes: raise SandboxHttpError(f"response body exceeded max_bytes ({max_bytes})"). Worth a TestRequestLive case with a body of max_bytes + 1 bytes — /big is large enough that it never hits this path.