Skip to content

fix: bound multipart buffering in FileSizeFilter to prevent memory DoS (#6627) - #7061

Merged
Aias00 merged 6 commits into
apache:masterfrom
wy471x:fix_FileSizeFilter-maxInMemorySize-buffers
Sep 20, 2026
Merged

Aias00 merged 6 commits into
apache:masterfrom
wy471x:fix_FileSizeFilter-maxInMemorySize-buffers

Conversation

@wy471x

@wy471x wy471x commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Make sure that:

  • You have read the contribution guidelines.
  • You submit test cases (unit or integration tests) that back your changes.
  • Your local test passed ./mvnw clean install -Dmaven.javadoc.skip=true.

Summary

Fixes #6627.

FileSizeFilter put no bound on what it buffered: the constructor built its codecs with maxInMemorySize(-1), so serverRequest.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 maxInMemorySize to -1 to resolve the DataBufferLimitException: Exceeded limit on max bytes to buffer error"), because DataBufferLimitException was 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:

  1. FileSizeFilter (FileSizeFilter.java:65, :73) — new maxInMemorySize field, computed as fileMaxSize * Constants.BYTES_PER_MB (saturating at Integer.MAX_VALUE to avoid int overflow) and passed to configurer.defaultCodecs().maxInMemorySize(...), so the codec aborts while buffering as soon as the configured limit is crossed instead of buffering the whole body first.
  2. 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.
  3. FileSizeFilter.filter (FileSizeFilter.java:83) — a non-positive shenyu.file.max-size now 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).
  4. FileSizeFilter.filter (FileSizeFilter.java:90) — the rejection branch now releases the joined pooled DataBuffer before returning; the existing .doFinally release 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).
  5. 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 with 400 while the body is not buffered in full (only the buffers consumed until the limit trips) and the filter chain is never called. With the previous maxInMemorySize(-1) the same test fails with consumed buffers: 4, i.e. it pins the vulnerability.
  • Existing FileSizeFilterTest#testFilter / #testDecorate are unchanged and still pass, including the fileMaxSize = -1 rejection case.

Verification

  • ./mvnw clean install -Dmaven.javadoc.skip=true on JDK 21: whole reactor passes, with the one pre-existing order-dependent test excluded (-Dtest='!DubboReconcilerTest' -DfailIfNoTests=false). org.apache.shenyu.k8s.DubboReconcilerTest shares the static IngressCache with WebSocketReconcilerTest / DivideIngressReconcilerTest (all use mockedNamespace/mockedIngress) and fails purely on test execution order — it passes in isolation and reproduces the same failure on unmodified master; shenyu-kubernetes-controller is unrelated to this change.
  • shenyu-web module: 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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 from shenyu.file.max-size, avoiding unbounded in-memory joins.
  • Handle DataBufferLimitException to 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.

Comment on lines +90 to 95
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 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved as PMC (Aias00). Small, well-scoped fix with regression tests; green CI, mergeable. Reviewed the diff.

@Aias00
Aias00 merged commit 4d62fb7 into apache:master Sep 20, 2026
46 of 48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] FileSizeFilter maxInMemorySize(-1) buffers entire request body in memory before size check (DoS)

3 participants