fix: bound multipart buffering in FileSizeFilter to prevent memory DoS (#6627) - #7061
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new size check uses DataBuffer.capacity() (allocation size) instead of readableByteCount(), which can mis-measure payload size for pooled/growing buffers and cause incorrect rejections/logging.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR hardens shenyu-web’s multipart upload protection by reintroducing a bounded maxInMemorySize for multipart buffering in FileSizeFilter, mapping DataBufferLimitException to the existing “payload too large” response, and ensuring pooled buffers are released on rejection to prevent memory exhaustion/DoS.
Changes:
- Bound multipart buffering via
defaultCodecs().maxInMemorySize(...)derived fromshenyu.file.max-size, avoiding unbounded in-memory joins. - Handle
DataBufferLimitExceptionto preserve the existing rejection response for oversized multipart uploads. - Add a regression test that asserts the request is rejected before the entire multipart body is buffered.
File summaries
| File | Description |
|---|---|
| shenyu-web/src/main/java/org/apache/shenyu/web/filter/FileSizeFilter.java | Adds bounded multipart buffering, centralizes “payload too large” response handling, and releases buffers on rejection. |
| shenyu-web/src/test/java/org/apache/shenyu/web/filter/FileSizeFilterTest.java | Adds a test ensuring oversized multipart bodies are rejected during buffering and the filter chain is not invoked. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (dataBuffer.capacity() > maxInMemorySize) { | ||
| final int actualSize = dataBuffer.capacity(); | ||
| DataBufferUtils.release(dataBuffer); | ||
| return payloadTooLarge(exchange, | ||
| "The actual size is " + actualSize / Constants.BYTES_PER_MB + "M"); | ||
| } |
Aias00
left a comment
There was a problem hiding this comment.
Approved as PMC (Aias00). Small, well-scoped fix with regression tests; green CI, mergeable. Reviewed the diff.
Make sure that:
./mvnw clean install -Dmaven.javadoc.skip=true.Summary
Fixes #6627.
FileSizeFilterput no bound on what it buffered: the constructor built its codecs withmaxInMemorySize(-1), soserverRequest.bodyToMono(DataBuffer.class)joined the entire multipart body in memory and the size check only ran after that. A client could stream an arbitrarily large multipart body and have the gateway buffer all of it before the request was finally rejected — a memory-exhaustion DoS (one request can take the process down).The bound had been removed on purpose in #4507 ("Corrected
maxInMemorySizeto-1to resolve theDataBufferLimitException: Exceeded limit on max bytes to buffererror"), becauseDataBufferLimitExceptionwas not handled and broke the existing rejection response. This PR re-introduces the bound and handles that exception, which is what makes it safe.Changes:
FileSizeFilter(FileSizeFilter.java:65,:73) — newmaxInMemorySizefield, computed asfileMaxSize * Constants.BYTES_PER_MB(saturating atInteger.MAX_VALUEto avoidintoverflow) and passed toconfigurer.defaultCodecs().maxInMemorySize(...), so the codec aborts while buffering as soon as the configured limit is crossed instead of buffering the whole body first.FileSizeFilter.filter(FileSizeFilter.java:111) —onErrorResume(DataBufferLimitException.class, ...)maps the codec limit trip to the same rejection response the filter already produced (400+ShenyuResultEnum.PAYLOAD_TOO_LARGE), preserving the existing API behavior for oversized uploads.FileSizeFilter.filter(FileSizeFilter.java:83) — a non-positiveshenyu.file.max-sizenow rejects the multipart request before reading the body at all, so no configuration is left with unbounded buffering (previously such a request was buffered in full and only then rejected).FileSizeFilter.filter(FileSizeFilter.java:90) — the rejection branch now releases the joined pooledDataBufferbefore returning; the existing.doFinallyrelease only covered the success path, so every oversized upload leaked one pooled buffer (the leak tracked in [BUG] FileSizeFilter leaks pooled DataBuffer on oversized-file rejection path #6626).FileSizeFilter.payloadTooLarge(FileSizeFilter.java:125) /maxInMemorySize(FileSizeFilter.java:141) — the three rejection sources now share one response/logging path instead of duplicating the status + error construction.Test Cases:
FileSizeFilterTest#testFilterRejectsOversizedBodyWhileBuffering(FileSizeFilterTest.java:128) — sends 4 × 512 KB against a 1 MB limit and asserts the request is rejected with400while the body is not buffered in full (only the buffers consumed until the limit trips) and the filter chain is never called. With the previousmaxInMemorySize(-1)the same test fails withconsumed buffers: 4, i.e. it pins the vulnerability.FileSizeFilterTest#testFilter/#testDecorateare unchanged and still pass, including thefileMaxSize = -1rejection case.Verification
./mvnw clean install -Dmaven.javadoc.skip=trueon JDK 21: whole reactor passes, with the one pre-existing order-dependent test excluded (-Dtest='!DubboReconcilerTest' -DfailIfNoTests=false).org.apache.shenyu.k8s.DubboReconcilerTestshares the staticIngressCachewithWebSocketReconcilerTest/DivideIngressReconcilerTest(all usemockedNamespace/mockedIngress) and fails purely on test execution order — it passes in isolation and reproduces the same failure on unmodifiedmaster;shenyu-kubernetes-controlleris unrelated to this change.shenyu-webmodule: 82 tests, 0 failures.checkstyle:check— 0 violations;mvn validate(checkstyle + Apache RAT) passes.Note: the release in change 4 also fixes the pooled-buffer leak tracked in #6626, since it is the same rejection path.
@Aias00, could you please help review this PR? Thank you!
close #6627