fix(rest): handle HTTP parsing edge cases

[0xC3B6]

Summary

• handle unknown-length JSON bodies in rest/httpx while preserving empty-body behavior
• close response bodies when rest/httpc header parsing fails
• harden rest/httpc path handling for reserved characters, slash-containing variables, and encoded literal path segments

Test Plan

• go test ./rest/httpc -run 'Test(Do_RejectsSlashInPathVariables|BuildRequest_EscapesReservedPathCharacters|BuildRequest_AllowsEncodedSlashInLiteralPath|ParseHeaderErrorClosesBody)'
• go test ./rest/httpx -run 'TestParseJsonBody/(chunked_body|empty_chunked_body)'
• go test ./rest/httpc ./rest/httpx

[kevwan]

Post-merge follow-ups

Tracking items to address after this lands.

---

1. Release note — path variable slash rejection (rest/httpc/requests.go near fillPath)

fillPath now returns an error when a path variable value contains /. This is the right security default, but it is a breaking change for any caller that was previously passing slash-containing values (e.g. "org/repo").
Action: add a short entry to the release notes / CHANGELOG that documents this new restriction so users can adapt their callers before upgrading.

---

2. Memory / heap pressure — chunked JSON body buffering (rest/httpx/requests.go:117)

The chunked branch (ContentLength < 0) now calls io.ReadAll and buffers the entire body before handing it to the JSON decoder. For large upload payloads this trades streaming for correctness, which can significantly increase heap allocation under load.
Action: add a // TODO(perf) note here and open a tracking issue for evaluating a bounded-streaming approach (e.g. respect maxBodyLen strictly, or stream into the decoder while capping memory via an io.LimitReader).

---

3. Benchmarks — chunked JSON parse path (rest/httpx/requests_test.go)

The new test cases validate correctness for chunked bodies but there are no allocation or latency benchmarks. The io.ReadAll path is measurably different from the streaming decoder path.
Action: add BenchmarkParseJsonBody_Chunked (small, medium, large payload sizes) alongside the existing TestParseJsonBody table so the tradeoff can be quantified in CI.

---

4. Streaming-safe refactor for empty chunked bodies (rest/httpx/requests.go:117)

The current fix reads the whole body to detect "empty chunked" before deciding between UnmarshalJsonBytes(nil) and UnmarshalJsonBytes(data). An alternative that avoids full buffering for large bodies: peek only the first byte (e.g. with bufio.Reader.ReadByte) to distinguish empty vs. non-empty, and fall back to the streaming decoder only for non-empty bodies.
Action: consider this as a follow-on PR once benchmarks (item 3) confirm the cost is meaningful.

---

5. Regression-guard tests — URL escaping & encoded literal segments (rest/httpc/requests_test.go)

The new TestBuildRequest_EscapesReservedPathCharacters and TestBuildRequest_AllowsEncodedSlashInLiteralPath tests are a good start. This area (url.Path vs. url.RawPath vs. EscapedPath()) is subtle and easy to regress.
Action: keep these tests and consider adding additional cases: percent-encoded segments other than %2F, mixed encoded/plain segments, and variables adjacent to literal %-encoded characters (e.g. /a%2Fb/:key/:other).

[0xC3B6]

Understood.

The follow-ups make sense. I can start by adding benchmark coverage for the chunked-body path first, so the memory / allocation impact is quantified before touching the parsing logic further. Then I can follow up on the perf note and the streaming-safe refactor separately if the benchmark results show that the current tradeoff is meaningful.

For the slash-containing path variable restriction, I couldn't find a root-level changelog / release-notes file in this repo. If there is a preferred place for release-note entries, point me to it and I can add a short note for this behavior change.

Separately, the current failing Linux job appears to be in mcp/request_metadata_test.go, while rest/httpc and rest/httpx are passing in CI.

[kevwan]

Review

A well-scoped PR that fixes three distinct but related HTTP parsing edge cases.

Fix 1: Chunked JSON bodies (rest/httpx)

Change: ContentLength > 0 → ContentLength != 0 in withJsonBody, plus special handling when ContentLength == -1.

Analysis: ContentLength = -1 means the server didn't send a Content-Length header (chunked transfer encoding). The old code silently dropped such bodies. The new code reads all data first when the length is unknown, then handles the empty-body case explicitly. This is correct behavior for HTTP/1.1 chunked requests.

One question: the empty chunked body path calls mapping.UnmarshalJsonMap(nil, v) — is passing nil supported and well-tested in that function?

Fix 2: Response body leak on header parse error (rest/httpc)

Change: Closes resp.Body when ParseHeaders fails.

Analysis: Without this, a failed header parse would leave the response body open, leaking a TCP connection. This is a straightforward resource leak fix. ✅

Fix 3: Path variable encoding (rest/httpc)

Changes:

• Rejects path variables containing / (prevents path traversal attacks / routing ambiguity)
• URL-percent-encodes reserved characters in path variables via nurl.PathEscape()
• Sets both u.Path and u.RawPath correctly

Analysis: The dual-path approach (maintaining both decoded Path and encoded RawPath) is the correct way to handle URL encoding per Go's net/url semantics. The slash rejection is a security improvement.

One concern: The fill closure is called twice — once for fields (decoded), once for rawFields (encoded). The slash check runs in both. But if a slash is in the value, the first fill(fields, false) call will return an error before fill(rawFields, true) runs — so the check is effectively done once. This is correct, just worth noting.

Security note: The slash-in-path check prevents path traversal (e.g., a value like ../../admin containing / could escape intended route boundaries).

Overall: Three clean, targeted fixes. The chunked body handling is the most complex and the one that warrants the most scrutiny. The other two are clear correctness/security improvements. LGTM with the one question about UnmarshalJsonMap(nil, v).