fix http body parsing to respect Content-Length#733
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTrim captured HTTP request and response bodies to explicit Content-Length when non-negative and smaller than the captured buffer; decode chunked Transfer-Encoding for both requests and responses, replace bodies accordingly, clear related headers, and add tests simulating BPF-style garbage buffers and multiple body scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/containerwatcher/v2/tracers/httpparse_test.go`:
- Around line 33-36: The test's expected body is wrong for the case with
"Content-Length: 0": update ParseHttpRequest to treat content-length zero as an
empty body by changing the conditional from "> 0" to ">= 0", and then change the
test case in httpparse_test.go (the case named "empty body with content-length
zero") to set expectedBody to the empty string ""; reference the
ParseHttpRequest function and the test case name to locate both spots to modify.
- Around line 87-90: The test case named "empty body with content-length zero"
in httpparse_test.go currently expects "GARBAGE" despite the response having
Content-Length: 0; update the test's expectedBody (and any related expected
payload fields) for that test case to an empty string to reflect that the parser
should return no body when Content-Length is 0 (locate the test case by the name
"empty body with content-length zero" in the table of cases).
In `@pkg/containerwatcher/v2/tracers/httpparse.go`:
- Around line 133-135: The response-body trimming logic ignores the
Content-Length==0 edge case; update the block around resp.ContentLength and
bodyData in ParseHttpResponse (the snippet using resp.ContentLength and
bodyData) to explicitly handle zero-length bodies: if resp.ContentLength == 0
set bodyData to an empty slice (or nil), and otherwise trim when
resp.ContentLength >= 0 and less than len(bodyData) (i.e. use resp.ContentLength
>= 0 in the comparison so zero is handled), ensuring negative (-1) unknown
lengths remain untouched.
- Around line 102-104: The current check uses "if req.ContentLength > 0 &&
req.ContentLength < int64(len(bodyData)) { bodyData =
bodyData[:req.ContentLength] }" which fails to handle an explicit
"Content-Length: 0" (req.ContentLength == 0) and leaves garbage in bodyData;
update the conditional to use >= (i.e. req.ContentLength >= 0 &&
req.ContentLength < int64(len(bodyData))) so when req.ContentLength == 0 you
truncate bodyData to zero length; locate the conditional that references
req.ContentLength and bodyData in httpparse.go and make the comparison change
ensuring the slice expression still uses the int conversion already present.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/containerwatcher/v2/tracers/httpparse.gopkg/containerwatcher/v2/tracers/httpparse_test.go
YakirOren
force-pushed
the
fix/http-body-parsing
branch
3 times, most recently
from
99d47e4 to
ac5b68f
Compare
Signed-off-by: Yakir Oren <yakiroren@gmail.com>
YakirOren
force-pushed
the
fix/http-body-parsing
branch
from
ac5b68f to
98d12b9
Compare
slashben
previously approved these changes
Mar 1, 2026
Signed-off-by: Ben <ben@armosec.io>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/containerwatcher/v2/tracers/httpparse.go (1)
145-156:⚠️ Potential issue | 🟡 MinorResponse path missing chunked Transfer-Encoding decoding.
ParseHttpRequestdecodes chunked bodies (lines 107-115), butParseHttpResponsedoes not. HTTP responses commonly use chunked encoding, especially for dynamic content. Without decoding, responses withTransfer-Encoding: chunkedwill have raw chunked-formatted data in the body (e.g.,5\r\nhello\r\n0\r\n\r\n).Consider adding the same chunked decoding logic here for consistency:
Proposed fix
// Set body directly without re-reading. // See ParseHttpRequest for why we need the Content-Length guard. bodyData := data[headerEnd:] + if len(resp.TransferEncoding) > 0 && resp.TransferEncoding[0] == "chunked" { + decodedBody, err := io.ReadAll(httputil.NewChunkedReader(bytes.NewReader(bodyData))) + if err == nil { + bodyData = decodedBody + } + } if resp.ContentLength >= 0 && resp.ContentLength < int64(len(bodyData)) { bodyData = bodyData[:resp.ContentLength] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/containerwatcher/v2/tracers/httpparse.go` around lines 145 - 156, ParseHttpResponse currently slices bodyData and assigns it to resp.Body without decoding chunked Transfer-Encoding, so responses with "Transfer-Encoding: chunked" will retain raw chunked framing; update the block around resp, bodyData (the code that currently does resp.Body = io.NopCloser(bytes.NewReader(bodyData)), resp.ContentLength = ..., resp.TransferEncoding = nil, resp.Header.Del(...)) to detect if resp.Header.Get("Transfer-Encoding") contains "chunked" (or resp.TransferEncoding includes "chunked") and decode the chunked stream into a contiguous body (reuse the same chunked decoding logic used in ParseHttpRequest lines ~107-115), then assign the decoded bytes to resp.Body, set resp.ContentLength to the decoded length, clear Transfer-Encoding, and remove the Content-Length header as done now; ensure the decoding handles truncated/invalid chunks by falling back to the raw body or returning the appropriate error consistent with ParseHttpRequest behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/containerwatcher/v2/tracers/httpparse.go`:
- Around line 145-156: ParseHttpResponse currently slices bodyData and assigns
it to resp.Body without decoding chunked Transfer-Encoding, so responses with
"Transfer-Encoding: chunked" will retain raw chunked framing; update the block
around resp, bodyData (the code that currently does resp.Body =
io.NopCloser(bytes.NewReader(bodyData)), resp.ContentLength = ...,
resp.TransferEncoding = nil, resp.Header.Del(...)) to detect if
resp.Header.Get("Transfer-Encoding") contains "chunked" (or
resp.TransferEncoding includes "chunked") and decode the chunked stream into a
contiguous body (reuse the same chunked decoding logic used in ParseHttpRequest
lines ~107-115), then assign the decoded bytes to resp.Body, set
resp.ContentLength to the decoded length, clear Transfer-Encoding, and remove
the Content-Length header as done now; ensure the decoding handles
truncated/invalid chunks by falling back to the raw body or returning the
appropriate error consistent with ParseHttpRequest behavior.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/containerwatcher/v2/tracers/httpparse.gopkg/containerwatcher/v2/tracers/httpparse_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/containerwatcher/v2/tracers/httpparse_test.go
Signed-off-by: Ben <ben@armosec.io>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/containerwatcher/v2/tracers/httpparse.go (2)
107-118: Silently ignoring chunked decoding errors may propagate malformed bodies.When
httputil.NewChunkedReaderfails (e.g., BPF buffer truncated mid-chunk), the error is silently swallowed and the raw chunked-encoded data (with garbage) becomes the body. Downstream consumers would receive malformed data with no indication of failure.Consider logging the error or returning it when decoding fails.
Proposed improvement to log decoding failures
if len(req.TransferEncoding) > 0 && req.TransferEncoding[0] == "chunked" { decodedBody, err := io.ReadAll(httputil.NewChunkedReader(bytes.NewReader(bodyData))) if err == nil { bodyData = decodedBody req.TransferEncoding = nil req.Header.Del("Transfer-Encoding") req.Header.Del("Content-Length") + } else { + // Chunked decoding failed; body may be truncated or malformed. + // Fall back to using raw body data. } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/containerwatcher/v2/tracers/httpparse.go` around lines 107 - 118, The chunked-decoding error is currently swallowed in the block that uses httputil.NewChunkedReader, which can leave raw/malformed chunked data in bodyData; update the error path in the chunked decoding branch (the code that calls httputil.NewChunkedReader and io.ReadAll) to surface failures instead of ignoring them: if io.ReadAll returns an error, either log the error with the tracer/logger used in this package (include context: that chunked decode failed for this request) or return the error to the caller so downstream consumers don’t receive malformed bodyData; ensure you still only clear req.TransferEncoding/headers when decoding succeeds and keep the subsequent ContentLength truncation unchanged (refer to symbols req, bodyData, req.TransferEncoding, httputil.NewChunkedReader, req.Header.Del and req.ContentLength).
148-152: Same silent error handling concern for response chunked decoding.Consistent with the request handling, consider logging when chunked decoding fails so operators can diagnose issues with truncated BPF captures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/containerwatcher/v2/tracers/httpparse.go` around lines 148 - 152, The chunked response decoding branch in tracers/httpparse.go currently swallows errors when io.ReadAll(httputil.NewChunkedReader(bytes.NewReader(bodyData))) fails; update that branch to log the error instead of ignoring it so operators can diagnose truncated BPF captures — when err != nil, emit a clear log message including resp (or resp.TransferEncoding) and the err (e.g., via the package logger or log.Printf), while keeping the existing behavior of replacing bodyData only on success.pkg/containerwatcher/v2/tracers/httpparse_test.go (1)
76-91: Consider adding a test for truncated/malformed chunked data.The chunked decoding tests all use well-formed chunked data. Given that BPF buffers may truncate mid-chunk, consider adding a test case where chunked encoding is incomplete (e.g.,
"4\r\nWi"without the terminating0\r\n\r\n) to document the fallback behavior when decoding fails.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/containerwatcher/v2/tracers/httpparse_test.go` around lines 76 - 91, Add a unit test that covers truncated/malformed chunked responses: create a dummy request via ParseHttpRequest, build a BPF buffer with an incomplete chunked response (e.g., "HTTP/1.1 200 OK\r\nTransfer-Encoding: chunked\r\n\r\n4\r\nWi" wrapped via makeBPFBuffer and FromCString), call ParseHttpResponse(cleaned, dummyReq) and assert the observed fallback behavior (either require an error from ParseHttpResponse or read resp.Body and assert it contains the expected partial data like "Wi" and no panic). Name the test something like TestBPFBufferGarbageResponse_Chunked_Truncated and reference ParseHttpRequest, makeBPFBuffer, FromCString and ParseHttpResponse in the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/containerwatcher/v2/tracers/httpparse_test.go`:
- Around line 76-91: Add a unit test that covers truncated/malformed chunked
responses: create a dummy request via ParseHttpRequest, build a BPF buffer with
an incomplete chunked response (e.g., "HTTP/1.1 200 OK\r\nTransfer-Encoding:
chunked\r\n\r\n4\r\nWi" wrapped via makeBPFBuffer and FromCString), call
ParseHttpResponse(cleaned, dummyReq) and assert the observed fallback behavior
(either require an error from ParseHttpResponse or read resp.Body and assert it
contains the expected partial data like "Wi" and no panic). Name the test
something like TestBPFBufferGarbageResponse_Chunked_Truncated and reference
ParseHttpRequest, makeBPFBuffer, FromCString and ParseHttpResponse in the test.
In `@pkg/containerwatcher/v2/tracers/httpparse.go`:
- Around line 107-118: The chunked-decoding error is currently swallowed in the
block that uses httputil.NewChunkedReader, which can leave raw/malformed chunked
data in bodyData; update the error path in the chunked decoding branch (the code
that calls httputil.NewChunkedReader and io.ReadAll) to surface failures instead
of ignoring them: if io.ReadAll returns an error, either log the error with the
tracer/logger used in this package (include context: that chunked decode failed
for this request) or return the error to the caller so downstream consumers
don’t receive malformed bodyData; ensure you still only clear
req.TransferEncoding/headers when decoding succeeds and keep the subsequent
ContentLength truncation unchanged (refer to symbols req, bodyData,
req.TransferEncoding, httputil.NewChunkedReader, req.Header.Del and
req.ContentLength).
- Around line 148-152: The chunked response decoding branch in
tracers/httpparse.go currently swallows errors when
io.ReadAll(httputil.NewChunkedReader(bytes.NewReader(bodyData))) fails; update
that branch to log the error instead of ignoring it so operators can diagnose
truncated BPF captures — when err != nil, emit a clear log message including
resp (or resp.TransferEncoding) and the err (e.g., via the package logger or
log.Printf), while keeping the existing behavior of replacing bodyData only on
success.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/containerwatcher/v2/tracers/httpparse.gopkg/containerwatcher/v2/tracers/httpparse_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Truncate HTTP body to Content-Length in ParseHttpRequest and ParseHttpResponse to avoid BPF buffer garbage in parsed bodies.
Note: this only fixes the case where Content-Length is present. The proper fix is adding a buf_len field to the HTTP BPF gadget (syscall return value) so we can truncate at the source.
Summary by CodeRabbit
Bug Fixes
Tests