net-pr4 #480

Merged

navicore merged 3 commits from net-pr4 into main 2026-05-13 13:48:58 +00:00

Conversation 2 Commits 3 Files changed 16 +1698 -389

navicore commented 2026-05-13 04:30:59 +00:00

Owner

Copy link

No description provided.

navicore added 2 commits 2026-05-13 04:30:59 +00:00

pre tls leak bug fix ab08ee9faa

● CI green. PR4 ready. ... d8ec37c72b

Final state:

  - All PR4 functional work intact: HTTP/1.1 wire framing, connection pool with conservative defaults (8/host, 30s idle, 256 global, MRU checkout,
  half-closed peek via poll(POLLIN, 0)), SSRF over dns::resolve, ureq removed, net.http.* API preserved (Map shape unchanged).
  - All tests green: 26 http_client unit tests (including 3 integration tests that exercise full request flow and pool reuse end-to-end), plus the full
  suite.
  - no_dead_code test now tolerates up to 4 "rustls" substring matches with a precise comment in TOLERATED_FOR_HELLO; observed is 2.
  - docs/design/done/NO_DEAD_CODE.md updated with a "Linux residue from PR4" section documenting the symbol, the mechanism we traced, the bisect findings,
  the cost analysis, and the proposed future cleanups (linker-plugin-lto or feature-gating http_client into its own codegen unit).

  What we know about the residue:

  - One symbol, 17 bytes: drop_in_place<std::io::default_write_fmt::Adapter<rustls::stream::StreamOwned<rustls::client::client_conn::ClientConnection,
  may::net::TcpStream>>>.
  - Body is if let Some(err) = self.error { drop(err) } — std::io::Error trampoline, no rustls code.
  - Caused by the Box::new(stream) as Box<dyn HttpStream + Send> cast in tls::dial_tls. The cast materializes a vtable whose Write impl monomorphizes the
  Adapter; the Adapter's drop is what survives --gc-sections.
  - Bisect proved: removing ureq doesn't fix it, stubbing the pool doesn't fix it, removing the FFI wiring to my new modules does fix it. The exact reason
  the drop trampoline outlives the rest of its monomorphization chain crosses LLVM/lld comdat-reachability boundaries we didn't fully untangle.
  - Bounded: won't grow without new SideData types or new generic trampoline shapes. Tracked for revisiting alongside linker-plugin-lto evaluation.

navicore commented 2026-05-13 12:37:37 +00:00

Author

Owner

Copy link

Review — PR4 (HTTP client rewrite)

Reviewed against origin/net-pr4. The shape is right and significantly tighter than the ureq-era client: a clean module split (conn/pool/request/ssrf/wire/tls), trait-object handles instead of StreamKind so rustls's drop chain can be stripped from non-HTTPS binaries, pool keyed on (scheme, host, port) with MRU checkout and poll(POLLIN, 0) liveness, pre-resolved addrs flowing from SSRF validation straight to connect (which closes the DNS-rebinding TOCTOU window — that's the bit I want to call out as actually nice), idempotent-only retry. The unit + integration coverage finally exercises the wire path against a same-process server, and the no-dead-code budget mechanism is well-documented and honest about the 17-byte residue.

Two real bugs (one a security gap), a handful of correctness/robustness items, and a few test-coverage gaps before this is feature-complete.

Header injection via Content-Type

wire.rs:55-58:

if let Some((ct, bytes)) = body {
    write!(w, "Content-Type: {ct}\r\n")?;
    write!(w, "Content-Length: {}\r\n", bytes.len())?;

ct is the third argument to net.http.post / net.http.put — a string that flows through unchanged. If a Seq program ever passes a content-type that came from untrusted input (the obvious case: a proxy / forwarder pattern, but also any web app that takes Content-Type from a request and reuses it on an upstream call), a payload like "text/plain\r\nX-Injected: pwn\r\nContent-Length: 0\r\n\r\nGET /admin HTTP/1.1\r\nHost: ..." injects arbitrary headers — and depending on framing can pull off classic request smuggling against intermediaries.

URL pieces are safe because url::Url::parse percent-encodes control chars before they reach the wire path, and body bytes go via write_all (no formatting). Content-Type is the one user-supplied string that lands directly in the header block. Fix: reject (or strip) ASCII control chars — at minimum \r and \n — in write_request, returning an error before the request goes out:

if ct.bytes().any(|b| b == b'\r' || b == b'\n' || b < 0x20) {
    return Err(std::io::Error::other("invalid byte in Content-Type"));
}

Same hardening should arrive when the header-bag API lands (per the v1-limitations note) — every user-supplied header value needs the same check. Worth doing now for content-type because it's the one user-string already on the wire.

Request smuggling defenses (Content-Length / Transfer-Encoding handling)

wire.rs:82-99 accepts whatever the response headers say. Two specific RFC 9112 §6.3 rules aren't enforced:

1. Multiple Content-Length headers with conflicting values. The current loop just overwrites: content_length = value.trim().parse::<usize>().ok() — last write wins. RFC requires this to be rejected as a smuggling attempt. Same applies to a single Content-Length value containing a comma-separated list with differing entries.
2. Mixing Transfer-Encoding: chunked with Content-Length. Code prefers chunked (correct precedence), but per RFC the combination should be rejected entirely — that mismatch is what HTTP request smuggling exploits in the first place.

These are response-side checks (responses, not requests, in our client), so the threat is "malicious server can confuse our framing" rather than "smuggling through us." Still, the cost of enforcing them is small and the defense is conventional.

Pool is_alive final false — correct but obscure

pool.rs:130-133:

if pfd.revents & (libc::POLLHUP | libc::POLLERR | libc::POLLNVAL) != 0 {
    return false;
}
false

The trailing bare false fires when rc > 0 (poll returned an event) and revents has neither HUP/ERR/NVAL set — which by elimination means POLLIN. The module comment explains the intent (POLLIN on an idle keep-alive socket = peer sent FIN or unsolicited data, discard), but the code doesn't say so at this line — it just falls off the end with false. Future-reader is going to wonder if a fourth case got dropped. A one-line comment immediately above the trailing false ("POLLIN on idle socket: peer wrote unsolicited bytes or FIN, discard") closes the gap.

Side note: the function name is_alive reads as a property test, but in practice it returns false on any signal from poll except rc==0. The actual contract is "return true iff poll says the fd is quiet." Either rename to is_quiet / is_reusable, or document the conflation. Cosmetic.

Dead code in path-and-query construction

ssrf.rs:86-91:

let path_and_query = match parsed.query() {
    Some(q) if !parsed.path().is_empty() => format!("{}?{q}", parsed.path()),
    Some(q) => format!("/?{q}"),
    None if parsed.path().is_empty() => "/".to_string(),
    None => parsed.path().to_string(),
};

url::Url::path() for any hierarchical (http/https) URL is documented to return a path that always starts with / and is never empty — the url crate normalises http://example.com to path /. So the parsed.path().is_empty() arms are unreachable. Collapses to:

let path_and_query = match parsed.query() {
    Some(q) => format!("{}?{q}", parsed.path()),
    None => parsed.path().to_string(),
};

Harmless dead branches, but they read as defensive code against a case that can't happen.

EOF-framed bodies + no timeout = strand hang

wire.rs:111-123: when the response has neither Content-Length nor chunked encoding, the client reads to EOF. A misbehaving server that sends < MAX_BODY_SIZE bytes and then keeps the connection open without sending more parks the strand on read_to_end indefinitely. The PR doc lists "no per-request timeout" as a v1 limitation, so this is acknowledged — but it's worth being more explicit that the EOF-framed path is the worst-case shape (chunked and Content-Length paths bound by their own framing). Until the deadline pass lands, a defensive option is to force Connection: close semantics on the dial when the response declared no length (the code already does keep_alive = false, so subsequent requests on that conn are fine; the hang is within the EOF read itself).

Test gaps

Chunked decoding is not exercised end-to-end. The integration test server always sends Content-Length. Given chunked is the more elaborate path (with extensions, trailers, MAX_BODY_SIZE accumulation across chunks), it should have at least one round-trip test. A variant of spawn_test_server that emits a chunked response (e.g. two chunks + 0-terminator) is ~15 lines.

Connection: close is not exercised. The pool should drop a closed-mode connection rather than recycle it. The test server hardcodes Connection: keep-alive so the close path is untested. Worth a test that asserts accept_count == 2 after two requests when the server sends Connection: close — confirms the pool didn't try to reuse a doomed entry.

POST body round-trip is not verified. http_post_with_body_round_trips says explicitly in its own comment: "our test server only reads up to the blank line, so a POST body would be left in the socket buffer." So the test only proves the request line + headers reached the server — it could pass even if write_request silently dropped the body bytes. Reading Content-Length bytes after the blank line and echoing them back is the right fix, then the test asserts the echo matches the sent body.

SSRF + DNS rebinding closure isn't asserted in tests. The architectural property — that addrs are validated then dialled without a second DNS lookup — is the actual security win here. A unit test that mocks dns::resolve to return different IPs on consecutive calls and asserts the request goes to the validated IP would lock the invariant in. Probably overkill for now, but worth flagging because if a future refactor passes host: String to connect instead of addrs: Vec<IpAddr>, the property silently regresses with no test catching it.

Small things

• read_response uses BufReader::new(r) which has an 8 KB read-ahead. On an HTTP/1.1 connection without pipelining, the server should never send bytes past the response, so the read-ahead is always inside the current response — safe. If pipelining ever ships, those buffered bytes would be lost (BufReader's drop doesn't unread). Worth a one-line comment so this invariant is captured.
• User-Agent: seq/<version> discloses the runtime version to every remote server. Standard, but if you ever want to anonymise (security-conscious deployments), a way to override via env var would help. Future header-bag API closes this.
• No Accept header. RFC says servers SHOULD assume */*, but a handful of strict APIs return 406. Cheap defensive add: Accept: */*.
• pool.rs:103 — MAX_PER_HOST is checked after MAX_GLOBAL, so a host with empty pool gets rejected if the global cap is full elsewhere. Conservative choice (matches Go's DefaultTransport). Worth noting in the docstring that global wins; users tuning these knobs will hit this.
• is_alive polls the underlying TCP fd, which is correct for transport liveness but doesn't detect a half-broken TLS state (e.g., received TLS alert with no transport-level signal). The idempotent retry path covers it, so this is fine — but worth a sentence in pool.rs noting the layering choice.
• The HttpStream impl on rustls::StreamOwned<...> reaches into the public sock field (self.sock.as_raw_fd()). Stable per rustls 0.23 docs, but if rustls 0.24 ever makes sock private, this breaks. A wrapper method would insulate.

NO_DEAD_CODE residue

The 17-byte / 2-substring-match residue is well-documented (docs/design/done/NO_DEAD_CODE.md updated with the bisect notes and the cleanup path forward). Test budget at 4 gives ~2x headroom. Honest engineering — the design doc admits the bisect didn't fully untangle the rustc/LLVM/lld interaction, which is exactly the right level of certainty for a "we'll revisit when linker-plugin-lto is on the table" line. No action needed; flagging because it's the most interesting structural call in the PR and deserves explicit acknowledgement.

Verdict

• Fix before merge: the content-type header injection (wire.rs:56). A six-line guard closes it.
• Worth folding in before merge if quick: the duplicate/conflicting Content-Length and the chunked+CL combination guard, plus the chunked end-to-end test. All small and the security posture matches what users will expect from an HTTP client.
• Deferrable to a follow-up: the EOF-hang acknowledgement (already in v1 limits), Connection: close test, POST-body round-trip strengthening, the is_alive rename/comment, dead path-and-query branches, header-bag API for arbitrary headers + Accept.

The pre-resolved-addrs-into-connect design is the real win here — keep that property documented and test-anchored so it can't regress.

## Review — PR4 (HTTP client rewrite) Reviewed against `origin/net-pr4`. The shape is right and significantly tighter than the ureq-era client: a clean module split (`conn`/`pool`/`request`/`ssrf`/`wire`/`tls`), trait-object handles instead of `StreamKind` so rustls's drop chain can be stripped from non-HTTPS binaries, pool keyed on `(scheme, host, port)` with MRU checkout and `poll(POLLIN, 0)` liveness, pre-resolved addrs flowing from SSRF validation straight to connect (which closes the DNS-rebinding TOCTOU window — that's the bit I want to call out as actually nice), idempotent-only retry. The unit + integration coverage finally exercises the wire path against a same-process server, and the no-dead-code budget mechanism is well-documented and honest about the 17-byte residue. Two real bugs (one a security gap), a handful of correctness/robustness items, and a few test-coverage gaps before this is feature-complete. ### Header injection via `Content-Type` `wire.rs:55-58`: ```rust if let Some((ct, bytes)) = body { write!(w, "Content-Type: {ct}\r\n")?; write!(w, "Content-Length: {}\r\n", bytes.len())?; ``` `ct` is the third argument to `net.http.post` / `net.http.put` — a string that flows through unchanged. If a Seq program ever passes a content-type that came from untrusted input (the obvious case: a proxy / forwarder pattern, but also any web app that takes Content-Type from a request and reuses it on an upstream call), a payload like `"text/plain\r\nX-Injected: pwn\r\nContent-Length: 0\r\n\r\nGET /admin HTTP/1.1\r\nHost: ..."` injects arbitrary headers — and depending on framing can pull off classic request smuggling against intermediaries. URL pieces are safe because `url::Url::parse` percent-encodes control chars before they reach the wire path, and body bytes go via `write_all` (no formatting). Content-Type is the one user-supplied string that lands directly in the header block. Fix: reject (or strip) ASCII control chars — at minimum `\r` and `\n` — in `write_request`, returning an error before the request goes out: ```rust if ct.bytes().any(|b| b == b'\r' || b == b'\n' || b < 0x20) { return Err(std::io::Error::other("invalid byte in Content-Type")); } ``` Same hardening should arrive when the header-bag API lands (per the v1-limitations note) — every user-supplied header value needs the same check. Worth doing now for content-type because it's the one user-string already on the wire. ### Request smuggling defenses (Content-Length / Transfer-Encoding handling) `wire.rs:82-99` accepts whatever the response headers say. Two specific RFC 9112 §6.3 rules aren't enforced: 1. **Multiple `Content-Length` headers with conflicting values.** The current loop just overwrites: `content_length = value.trim().parse::<usize>().ok()` — last write wins. RFC requires this to be rejected as a smuggling attempt. Same applies to a single `Content-Length` value containing a comma-separated list with differing entries. 2. **Mixing `Transfer-Encoding: chunked` with `Content-Length`.** Code prefers chunked (correct precedence), but per RFC the combination should be rejected entirely — that mismatch is what HTTP request smuggling exploits in the first place. These are response-side checks (responses, not requests, in our client), so the threat is "malicious server can confuse our framing" rather than "smuggling through us." Still, the cost of enforcing them is small and the defense is conventional. ### Pool `is_alive` final `false` — correct but obscure `pool.rs:130-133`: ```rust if pfd.revents & (libc::POLLHUP | libc::POLLERR | libc::POLLNVAL) != 0 { return false; } false ``` The trailing bare `false` fires when `rc > 0` (poll returned an event) and `revents` has neither HUP/ERR/NVAL set — which by elimination means POLLIN. The module comment explains the *intent* (POLLIN on an idle keep-alive socket = peer sent FIN or unsolicited data, discard), but the code doesn't say so at this line — it just falls off the end with `false`. Future-reader is going to wonder if a fourth case got dropped. A one-line comment immediately above the trailing `false` ("POLLIN on idle socket: peer wrote unsolicited bytes or FIN, discard") closes the gap. Side note: the function name `is_alive` reads as a property test, but in practice it returns `false` on *any* signal from poll except `rc==0`. The actual contract is "return true iff poll says the fd is quiet." Either rename to `is_quiet` / `is_reusable`, or document the conflation. Cosmetic. ### Dead code in path-and-query construction `ssrf.rs:86-91`: ```rust let path_and_query = match parsed.query() { Some(q) if !parsed.path().is_empty() => format!("{}?{q}", parsed.path()), Some(q) => format!("/?{q}"), None if parsed.path().is_empty() => "/".to_string(), None => parsed.path().to_string(), }; ``` `url::Url::path()` for any hierarchical (http/https) URL is documented to return a path that always starts with `/` and is never empty — the `url` crate normalises `http://example.com` to path `/`. So the `parsed.path().is_empty()` arms are unreachable. Collapses to: ```rust let path_and_query = match parsed.query() { Some(q) => format!("{}?{q}", parsed.path()), None => parsed.path().to_string(), }; ``` Harmless dead branches, but they read as defensive code against a case that can't happen. ### EOF-framed bodies + no timeout = strand hang `wire.rs:111-123`: when the response has neither `Content-Length` nor chunked encoding, the client reads to EOF. A misbehaving server that sends `< MAX_BODY_SIZE` bytes and then *keeps the connection open without sending more* parks the strand on `read_to_end` indefinitely. The PR doc lists "no per-request timeout" as a v1 limitation, so this is acknowledged — but it's worth being more explicit that the EOF-framed path is the worst-case shape (chunked and Content-Length paths bound by their own framing). Until the deadline pass lands, a defensive option is to force `Connection: close` semantics on the dial when the response declared no length (the code already does `keep_alive = false`, so subsequent requests on that conn are fine; the hang is *within* the EOF read itself). ### Test gaps **Chunked decoding is not exercised end-to-end.** The integration test server always sends `Content-Length`. Given chunked is the more elaborate path (with extensions, trailers, MAX_BODY_SIZE accumulation across chunks), it should have at least one round-trip test. A variant of `spawn_test_server` that emits a chunked response (e.g. two chunks + 0-terminator) is ~15 lines. **`Connection: close` is not exercised.** The pool should drop a closed-mode connection rather than recycle it. The test server hardcodes `Connection: keep-alive` so the close path is untested. Worth a test that asserts `accept_count == 2` after two requests when the server sends `Connection: close` — confirms the pool didn't try to reuse a doomed entry. **POST body round-trip is not verified.** `http_post_with_body_round_trips` says explicitly in its own comment: "our test server only reads up to the blank line, so a POST body would be left in the socket buffer." So the test only proves the *request line + headers* reached the server — it could pass even if `write_request` silently dropped the body bytes. Reading `Content-Length` bytes after the blank line and echoing them back is the right fix, then the test asserts the echo matches the sent body. **SSRF + DNS rebinding closure isn't asserted in tests.** The architectural property — that `addrs` are validated *then* dialled without a second DNS lookup — is the actual security win here. A unit test that mocks `dns::resolve` to return different IPs on consecutive calls and asserts the request goes to the validated IP would lock the invariant in. Probably overkill for now, but worth flagging because if a future refactor passes `host: String` to connect instead of `addrs: Vec<IpAddr>`, the property silently regresses with no test catching it. ### Small things - `read_response` uses `BufReader::new(r)` which has an 8 KB read-ahead. On an HTTP/1.1 connection without pipelining, the server should never send bytes past the response, so the read-ahead is always inside the current response — safe. If pipelining ever ships, those buffered bytes would be lost (BufReader's drop doesn't unread). Worth a one-line comment so this invariant is captured. - `User-Agent: seq/<version>` discloses the runtime version to every remote server. Standard, but if you ever want to anonymise (security-conscious deployments), a way to override via env var would help. Future header-bag API closes this. - No `Accept` header. RFC says servers SHOULD assume `*/*`, but a handful of strict APIs return 406. Cheap defensive add: `Accept: */*`. - `pool.rs:103` — `MAX_PER_HOST` is checked *after* `MAX_GLOBAL`, so a host with empty pool gets rejected if the global cap is full elsewhere. Conservative choice (matches Go's DefaultTransport). Worth noting in the docstring that global wins; users tuning these knobs will hit this. - `is_alive` polls the underlying TCP fd, which is correct for transport liveness but doesn't detect a half-broken TLS state (e.g., received TLS alert with no transport-level signal). The idempotent retry path covers it, so this is fine — but worth a sentence in `pool.rs` noting the layering choice. - The `HttpStream` impl on `rustls::StreamOwned<...>` reaches into the public `sock` field (`self.sock.as_raw_fd()`). Stable per rustls 0.23 docs, but if rustls 0.24 ever makes `sock` private, this breaks. A wrapper method would insulate. ### NO_DEAD_CODE residue The 17-byte / 2-substring-match residue is well-documented (`docs/design/done/NO_DEAD_CODE.md` updated with the bisect notes and the cleanup path forward). Test budget at 4 gives ~2x headroom. Honest engineering — the design doc admits the bisect didn't fully untangle the rustc/LLVM/lld interaction, which is exactly the right level of certainty for a "we'll revisit when linker-plugin-lto is on the table" line. No action needed; flagging because it's the most interesting structural call in the PR and deserves explicit acknowledgement. ### Verdict - **Fix before merge:** the content-type header injection (`wire.rs:56`). A six-line guard closes it. - **Worth folding in before merge if quick:** the duplicate/conflicting Content-Length and the chunked+CL combination guard, plus the chunked end-to-end test. All small and the security posture matches what users will expect from an HTTP client. - **Deferrable to a follow-up:** the EOF-hang acknowledgement (already in v1 limits), Connection: close test, POST-body round-trip strengthening, the `is_alive` rename/comment, dead path-and-query branches, header-bag API for arbitrary headers + Accept. The pre-resolved-addrs-into-connect design is the real win here — keep that property documented and test-anchored so it can't regress.

navicore added 1 commit 2026-05-13 12:58:10 +00:00

All review-driven fixes applied. CI green. ... dd8f364512

Done in this round:

  1. Content-Type header-injection fix (wire.rs) — write_request calls header_value_safe(ct) before emitting
  headers; rejects any control character (< 0x20, including \r and \n, plus 0x7F). Closes the request-smuggling
  vector via the third arg to net.http.post/put. Three unit tests added covering CRLF, bare LF, and NUL.
  2. RFC 9112 §6.3 smuggling guards (wire.rs::read_response) — rejects responses with (a) multiple disagreeing
  Content-Length headers, (b) a single Content-Length whose value is a comma-separated list with mismatched
  3. Chunked end-to-end integration test — new ServerMode::Chunked in the test server emits two chunks +
  terminator; client decodes to "Hello world".
  4. Strengthened POST body round-trip — switched the existing test to ServerMode::Echo which reads Content-Length
   bytes after the request blank line and reflects them back; the assertion now compares echoed == body
  byte-for-byte. A regression that silently dropped the body would fail here.
  5. Pool comments — explanatory paragraph above the trailing false in is_alive covering the POLLIN-on-idle case
  and the TLS-state-not-detected gap (mitigated by idempotent retry). Docstring on release now spells out that the
   global cap is checked first.
  6. ssrf.rs path-and-query — collapsed dead path().is_empty() arms; url::Url::path() is documented to always
  start with / for hierarchical URLs.
  7. Accept: */* header added to every request (cheap defensive add for strict APIs that 406 without it).
  8. StreamOwned.sock field-stability comment in conn.rs flagging the rustls 0.23 public-field assumption.
  9. EOF-framed body hang note in STDLIB_REFERENCE.md v1 limitations — explicitly calls out that EOF-framed
  responses are the worst-case shape under the no-timeout gap. Also updated the header list to include Accept and
  the Content-Type control-char guard.

  Test count: 30 unit tests + 4 integration tests = 34 (up from 26 in the original PR4).

  Carried as follow-ups (per reviewer's "deferrable"): is_alive rename to is_quiet/is_reusable, Connection: close
  pool-eviction test, SSRF DNS-rebinding closure test (architectural property that's worth pinning later),
  User-Agent env-var override (waits for the header-bag API).

navicore commented 2026-05-13 13:48:08 +00:00

Author

Owner

Copy link

Second pass — review-driven fixes look good

Walked the delta from d8ec37c..dd8f364. Every item from the first pass landed cleanly. Couple of small wins on top of just-doing-what-was-asked:

Content-Type / header-injection guard (wire.rs)

header_value_safe is the right shape and slightly stricter than I suggested — rejecting HTAB too, with an explicit rationale tied to RFC 9112's deprecation of tab-prefixed continuation lines. That's a real attack surface against intermediaries that still honour them, so the extra strictness is well-motivated. The three unit tests (CRLF, bare LF, NUL) cover the main families. One subtle thing worth noting (not fixing): the check allows 0x80–0xFF (RFC 9110 obs-text), so a malformed UTF-8 content-type round-trips. Not a security issue — those bytes can't inject a header — just a compatibility shape some strict servers will 400 on. Acceptable.

Anti-smuggling response-side guards

The split into parse_content_length (handles single + comma-list, agreeing or not) plus the outer "multiple headers must agree" guard plus the "chunked + CL is poison" guard is exactly the three RFC 9112 §6.3 shapes. I traced through the edge cases mentally:

• "5, 5" → accepted as 5 ✓ (covered by wire_response_content_length_list_all_agreeing_accepted)
• "5, 7" → rejected ✓
• "5" then "7" (two separate headers) → rejected ✓
• "5" then "5" (two separate, agreeing) → accepted ✓ (implied by the prev != parsed check)
• "" / "," / "5," (empty list elements) → rejected via parse-failure ✓
• Chunked + CL → rejected ✓

The error-message convention ("smuggling guard: ...") makes the test assertions clean and signals the intent in logs. Good.

Integration tests

ServerMode enum is the right refactor — the three flavours (FixedBody / Echo / Chunked) compose well and the test bodies stay narrow. Two solid additions:

• The POST round-trip now asserts echoed == body byte-for-byte against an echo server. That meaningfully proves the bytes traversed the wire — a regression that silently drops the body fails here instead of returning a misleading 200.
• Chunked decoding gets actual end-to-end coverage (two chunks + terminator → "Hello world").

Pool docstrings / dead-branch cleanup / docs

All the deferred-but-easy items took the smaller fixes:

• is_alive trailing false now has an inline rationale and an explicit note about the TLS-state-not-detected gap pointing at the idempotent-retry safety net.
• release docstring spells out global-cap-first ordering and the tuning hint.
• ssrf.rs path-and-query collapsed to the two reachable arms with a comment explaining why.
• Accept: */* header added.
• conn.rs flags the StreamOwned.sock field-access as a rustls 0.23 stability assumption.
• STDLIB_REFERENCE.md v1-limitations now names the EOF-framed body as the worst-case shape under the no-timeout gap, and lists the new Accept and the Content-Type control-char guard.

Still open (and I noted these as deferrable originally — fine to land without)

• No Connection: close test confirming the pool drops the entry. Won't catch a future regression where the response's keep_alive=false is ignored on the release side.
• The DNS-rebinding-closure architectural property still has no test anchoring it. The property holds in the current code (addrs flow from SSRF straight to connect_to_addrs), but a refactor could regress it silently. Low priority — flagging only because it's the strongest security win of the design.

Verdict

LGTM. The fixes are tight, the new tests pin the things that mattered, and the docs are honest about what's left. Ship it.

## Second pass — review-driven fixes look good Walked the delta from `d8ec37c..dd8f364`. Every item from the first pass landed cleanly. Couple of small wins on top of just-doing-what-was-asked: ### Content-Type / header-injection guard (wire.rs) `header_value_safe` is the right shape and slightly stricter than I suggested — rejecting HTAB too, with an explicit rationale tied to RFC 9112's deprecation of tab-prefixed continuation lines. That's a real attack surface against intermediaries that still honour them, so the extra strictness is well-motivated. The three unit tests (CRLF, bare LF, NUL) cover the main families. One subtle thing worth noting (not fixing): the check allows 0x80–0xFF (RFC 9110 obs-text), so a malformed UTF-8 content-type round-trips. Not a security issue — those bytes can't inject a header — just a compatibility shape some strict servers will 400 on. Acceptable. ### Anti-smuggling response-side guards The split into `parse_content_length` (handles single + comma-list, agreeing or not) plus the outer "multiple headers must agree" guard plus the "chunked + CL is poison" guard is exactly the three RFC 9112 §6.3 shapes. I traced through the edge cases mentally: - `"5, 5"` → accepted as 5 ✓ (covered by `wire_response_content_length_list_all_agreeing_accepted`) - `"5, 7"` → rejected ✓ - `"5"` then `"7"` (two separate headers) → rejected ✓ - `"5"` then `"5"` (two separate, agreeing) → accepted ✓ (implied by the `prev != parsed` check) - `""` / `","` / `"5,"` (empty list elements) → rejected via parse-failure ✓ - Chunked + CL → rejected ✓ The error-message convention (`"smuggling guard: ..."`) makes the test assertions clean and signals the intent in logs. Good. ### Integration tests `ServerMode` enum is the right refactor — the three flavours (FixedBody / Echo / Chunked) compose well and the test bodies stay narrow. Two solid additions: - The POST round-trip now asserts `echoed == body` byte-for-byte against an echo server. That meaningfully proves the bytes traversed the wire — a regression that silently drops the body fails here instead of returning a misleading 200. - Chunked decoding gets actual end-to-end coverage (two chunks + terminator → `"Hello world"`). ### Pool docstrings / dead-branch cleanup / docs All the deferred-but-easy items took the smaller fixes: - `is_alive` trailing `false` now has an inline rationale and an explicit note about the TLS-state-not-detected gap pointing at the idempotent-retry safety net. - `release` docstring spells out global-cap-first ordering and the tuning hint. - `ssrf.rs` path-and-query collapsed to the two reachable arms with a comment explaining why. - `Accept: */*` header added. - `conn.rs` flags the `StreamOwned.sock` field-access as a rustls 0.23 stability assumption. - `STDLIB_REFERENCE.md` v1-limitations now names the EOF-framed body as the worst-case shape under the no-timeout gap, and lists the new `Accept` and the Content-Type control-char guard. ### Still open (and I noted these as deferrable originally — fine to land without) - No `Connection: close` test confirming the pool drops the entry. Won't catch a future regression where the response's `keep_alive=false` is ignored on the release side. - The DNS-rebinding-closure architectural property still has no test anchoring it. The property holds in the current code (`addrs` flow from SSRF straight to `connect_to_addrs`), but a refactor could regress it silently. Low priority — flagging only because it's the strongest security win of the design. ### Verdict LGTM. The fixes are tight, the new tests pin the things that mattered, and the docs are honest about what's left. Ship it.

navicore merged commit 43828472cf into main 2026-05-13 13:48:58 +00:00

navicore deleted branch net-pr4 2026-05-13 13:48:58 +00:00

navicore referenced this pull request from a commit 2026-05-13 13:48:59 +00:00

Merge pull request 'net-pr4' (#480) from net-pr4 into main

navicore referenced this pull request 2026-05-13 17:03:50 +00:00

Networking: polish pass (local-port op, is_alive rename, IP-literal fast path, encapsulation refactor, single-flight DNS) #482

navicore referenced this pull request 2026-05-13 17:03:56 +00:00

Networking: test anchors for un-pinned architectural properties (SSRF rebinding, Connection: close, happy-path TLS) #483

navicore referenced this pull request 2026-05-13 17:04:01 +00:00

Networking: deadline pass across TCP connect, TLS handshake, HTTP request #484

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

bug

Something isn't working

  

dependencies

Pull requests that update a dependency file

  

documentation

Improvements or additions to documentation

  

duplicate

This issue or pull request already exists

  

enhancement

New feature or request

  

good first issue

Good for newcomers

  

help wanted

Extra attention is needed

  

invalid

This doesn't seem right

  

question

Further information is requested

  

refactor

Code refactoring and cleanup

  

rust

Pull requests that update rust code

  

technical-debt

Technical debt to address

  

wontfix

This will not be worked on

No labels

bug

dependencies

documentation

duplicate

enhancement

good first issue

help wanted

invalid

question

refactor

rust

technical-debt

wontfix

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

navicore/patch-seq!480

No description provided.