Conversation
|
@blotus: There are no 'kind' label on this PR. You need a 'kind' label to generate the release automatically.
DetailsI am a bot created to help the crowdsecurity developers manage community feedback and contributions. You can check out my manifest file to understand my behavior and what I can do. If you want to use this for your project, you can check out the BirthdayResearch/oss-governance-bot repository. |
|
@blotus: There are no area labels on this PR. You can add as many areas as you see fit.
DetailsI am a bot created to help the crowdsecurity developers manage community feedback and contributions. You can check out my manifest file to understand my behavior and what I can do. If you want to use this for your project, you can check out the BirthdayResearch/oss-governance-bot repository. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4355 +/- ##
==========================================
- Coverage 63.54% 63.51% -0.04%
==========================================
Files 476 476
Lines 33841 33894 +53
==========================================
+ Hits 21505 21528 +23
- Misses 10201 10229 +28
- Partials 2135 2137 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/kind enhancement |
github-actions
bot
added
kind/enhancement
|
update PR desc and content |
There was a problem hiding this comment.
Pull request overview
Adds request-body size enforcement at the appsec datasource level (before handing data to Coraza), with configurable behavior on oversized bodies and a pre-eval hook to disable body inspection per request.
Changes:
- Extend appsec hook environments to configure max body size and oversize action at load time, plus a
DisableBodyInspection()pre-eval helper. - Update request parsing to cap buffered body size, mark truncation/oversize, and propagate these flags through the appsec runner.
- Add unit/integration tests covering oversize handling, truncation, and dynamic body inspection disabling.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/appsec/waf_helpers.go | Exposes new on_load setters and pre_eval DisableBodyInspection() into expr env. |
| pkg/appsec/request.go | Implements capped body read, oversize detection, and truncation/oversize flags in ParsedRequest. |
| pkg/appsec/appsec.go | Introduces BodySettings + defaults, setters for on_load hooks, and per-request disable flag. |
| pkg/acquisition/modules/appsec/config.go | Passes runtime BodySettings into request parsing. |
| pkg/acquisition/modules/appsec/appsec_runner.go | Enforces oversize “drop” and skips body write when inspection is disabled; logs truncation. |
| pkg/appsec/request_test.go | Adds unit tests for NewParsedRequestFromRequest body size behavior. |
| pkg/acquisition/modules/appsec/appsec_bodysize_test.go | Adds end-to-end tests for BodySizeExceeded, truncation, and DisableBodyInspection hook behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Drain the remaining body so the client doesn't time out waiting for the server | ||
| // to finish reading. The LimitReader stopped at maxSize+1, so r.Body may still | ||
| // have unread bytes. | ||
| _, _ = io.Copy(io.Discard, r.Body) |
There was a problem hiding this comment.
io.Copy(io.Discard, r.Body) drains the entire remaining request body when the limit is exceeded. This can be abused for resource-exhaustion (slowloris/DoS) with very large or slow bodies, and it also defeats the goal of quickly bailing out on oversized uploads (especially for the drop action). Consider not draining at all (and letting the server close the connection), or draining only up to a small capped amount and then closing r.Body / disabling keep-alive for that request.
| // Drain the remaining body so the client doesn't time out waiting for the server | |
| // to finish reading. The LimitReader stopped at maxSize+1, so r.Body may still | |
| // have unread bytes. | |
| _, _ = io.Copy(io.Discard, r.Body) | |
| // Do not drain the remaining body: continuing to read an oversized request can | |
| // be abused to tie up server resources with very large or slow bodies. | |
| // Close the original body and stop reading further from the client stream. | |
| _ = r.Body.Close() |
| maxSize := bodySettings.MaxSize | ||
| if maxSize <= 0 { | ||
| maxSize = DefaultMaxBodySize | ||
| } | ||
|
|
||
| action := bodySettings.Action | ||
| if action == "" { | ||
| action = BodySizeActionDrop | ||
| } | ||
|
|
||
| // Always read from the actual stream — never trust Content-Length. | ||
| // Read up to maxSize+1 bytes so we can detect whether the body exceeds the limit. | ||
| body, err = io.ReadAll(io.LimitReader(r.Body, maxSize+1)) |
There was a problem hiding this comment.
maxSize+1 can overflow if bodySettings.MaxSize is set near math.MaxInt64, causing io.LimitReader to receive a negative/zero limit and silently skip reading. It would be safer to guard against overflow (and/or enforce a reasonable upper bound on MaxSize) before computing maxSize+1.
| // io.ErrUnexpectedEOF means the connection was closed without a clean EOF (e.g. no | ||
| // Content-Length and no body). Treat whatever was read as the complete body. | ||
| if err != nil && !errors.Is(err, io.ErrUnexpectedEOF) { | ||
| return ParsedRequest{}, fmt.Errorf("unable to read body: %w", err) |
There was a problem hiding this comment.
The io.ErrUnexpectedEOF handling/comment here looks incorrect for io.ReadAll: ReadAll normally returns nil on EOF and does not generate ErrUnexpectedEOF itself. This makes the special-case misleading and may hide genuine read errors if an underlying reader ever returns ErrUnexpectedEOF. Consider removing the check or revising the comment and error handling to match io.ReadAll semantics (e.g., treat any non-EOF error as a failure).
Allow to enforce body size limitation in the appsec datasource itself:
This configuration is different from the one in coraza. Because we read the body ourselves before passing it to coraza, we need to be able to properly limit the amount of body buffered in memory.
For now, this only allows setting a global value used for all requests as handling this properly in
pre_evalwould require a lot of refactoring/changes.A
DisableBodyInspection()helper is also added to dynamically disable reading the body from apre_evalhook.