Fix multipart body-map cap bypass and charset handling in RESTEasy - #12085
Fix multipart body-map cap bypass and charset handling in RESTEasy#12085jandro996 wants to merge 12 commits into
Conversation
…n handling Cap collectBodyMap() by total accumulated values (not distinct field names) to close a bypass where a repeated field name could exhaust the limit. Filter to text/plain parts only and default undeclared charsets to UTF-8 for body-map values, matching the Jersey reference. Widen per-part read/header exceptions to Exception with debug logging so one malformed part doesn't abort body/filename/content collection for the whole request.
jandro996
added
type: bug fix
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
🎯 Code Coverage (details) 🔗 Commit SHA: 9496d86 | Docs | Datadog PR Page | Give us feedback! |
🟡 Java Benchmark SLOs — Performance SLO warning (near threshold)
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
![datadog-official[bot]](https://avatars.githubusercontent.com/in/1358509?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
The multipart changes preserve the intended bounded, text-only collection behavior across repeated fields, mixed file/text parts, missing headers, charset defaults, and body-reader failures. Full module tests could not run because the wrapper required an unavailable network download and the installed Gradle could not satisfy this checkout's Java 25 toolchain.
📊 Validated against 1 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit 6b93c56 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b93c566fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A part declaring Content-Type: text/plain but also carrying a filename attribute was still treated as a form field by collectBodyMap(), so an attacker could pad the body-map cap with disposable text/plain file parts and push a genuine field out of server.request.body. Exclude any part with a filename attribute (present, even empty) regardless of its declared media type, matching the file/field distinction collectFilesContent() already uses. Addresses a P1 finding from the Codex review on PR #12085.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 310e07ea2d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- hasFilenameParam() detects the extended filename* Content-Disposition parameter as well as plain filename, closing the same cap-padding bypass via the extended form (Codex P1 follow-up on PR #12085) - add regression test for a part carrying only filename* (no plain filename)
…resteasy-bound-multipart' into alejandro.gonzalez/APPSEC-69139-resteasy-bound-multipart
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6348c70d57
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
hasFilename() now excludes filename*-only parts from the body map, but collectFilesContent() still gated on rawFilenameFromContentDisposition(), which is filename*-blind — such a part reached neither AppSec callback. Switch the gate to hasFilenameParam() so content inspection still covers it (Codex P1 follow-up on PR #12085).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afbe420f61
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…entDecoder RFC 7230 optional whitespace (OWS) allows both space and horizontal tab. extractCharset() only accepted space, so "type;\tcharset=X" was treated as no-charset-declared, causing MultipartHelper to silently override the client's declared charset with UTF-8.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46aace9411
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…harset param extractCharset() only matched the literal token "charset=", so "charset = X" or "charset =X" (whitespace around the equals sign) was treated as no-charset-declared and silently overridden with UTF-8.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcb7cdcb2c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…nizer Replace the index-based substring search with a proper media-type parameter walk: quoted parameter values are consumed as opaque tokens (so a "charset"-looking substring inside another parameter's quoted value can never be mistaken for a real charset parameter), and an invalid charset value no longer aborts the search: a later, valid charset parameter is still found.
57589a2 to
3b634fc
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21569459f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
contentTypeWithDefaultUtf8(null) produced a bare "charset=UTF-8" string with no media type, which extractCharset()'s tokenizer cannot parse (it assumes a media type always precedes the first parameter). This made decodeBytes() fall back to the JVM platform charset instead of UTF-8 for parts with no Content-Type header at all.
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
More details
The multipart cap and classification changes are consistent with the Jersey reference and the added RESTEasy cases: text fields are bounded by total values, file parts do not consume that budget, and malformed part access is isolated. Charset parsing also passed adversarial declared, quoted, malformed, and substring cases; the full Gradle suites could not start because Java 25 is unavailable.
📊 Validated against 8 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit 9496d86 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9496d866ed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!isTextPlain(contentType) || hasFilename(headers)) { | ||
| continue; |
There was a problem hiding this comment.
When a filename-less multipart field declares a media type such as application/json, this condition drops it from server.request.body, while collectFilesContent() also skips it because it has no filename parameter. Previously every InputPart was added through getBodyAsString(), so applications using JSON/XML @RequestPart values now leave their payload absent from both AppSec callbacks, allowing attacks in those parts to evade inspection; retain all filename-less form fields or route non-text ones to another callback.
Useful? React with 👍 / 👎.
1 participant
What Does This Do
MultipartHelper.collectBodyMap(MultipartFormDataInput), called fromMultipartFormDataReaderInstrumentation.ReadFromAdvice.after()to build theserver.request.bodyAppSec address for RESTEasy 3.0 multipart requests.total >= MAX_FILES_TO_INSPECT) rather than by distinct field name, sincegetFormDataMap()already groups parts by field name — a per-key cap would be trivially bypassed by repeating the same field name on every part.collectBodyMaptotext/plainparts with nofilenameattribute, matching the Jersey reference (jersey3/MultiPartHelper.collectBodyPart): file parts are reported separately viacollectFilenames/collectFilesContentand must not also consume the body-map budget. A part with noContent-Typeheader defaults totext/plain, perInputPart's own javadoc. The filename check recognizes both the plainfilenameand the RFC 5987 extendedfilename*Content-Disposition parameters (hasFilenameParam), so a part carrying onlyfilename*can't bypass the exclusion;collectFilesContent()'s own filename gate was updated to the same check for consistency, since it had the identical blind spot.contentTypeWithDefaultUtf8), matching Jersey's owngetValue()behavior, instead of falling back toMultipartContentDecoder's JVM-platform-charset default — which is still used as-is forcollectFilesContent()(binary file content), left unchanged. The null-content-type fallback synthesizes a full"text/plain; charset=UTF-8"value (not a bare"charset=UTF-8"parameter) so it parses correctly underextractCharset().readContent/contentTypeOffromcatch (IOException)tocatch (Exception)withlog.debug, so a single malformed part (e.g. no matchingMessageBodyReader, or an unexpectedInputPartimplementation) can't abort body/filename/content collection for the rest of the request.MultipartContentDecoder.extractCharsetrewriteThree consecutive review comments against the original index-based
charset=substring search (missing tab-as-OWS support, no whitespace tolerance around=, and no awareness of quoted parameter values) showed the search-for-a-literal-token approach itself was the problem, not any single missing character class. Replaced it with a proper media-type parameter tokenizer:;/,, then walksname=valueparameters, trimming RFC 7230 optional whitespace (space/tab) around bothnameandvalue.charset-looking substring inside another parameter's quoted value (e.g.boundary="charset=oops") can never be mistaken for a realcharsetparameter.charsetvalue (Charset.forNamethrowing) instead of returningnullimmediately, in case a later, validcharsetparameter exists.This is a shared class used by ~17 other instrumentation modules (vertx-web, jersey2/3, tomcat7, jetty, netty41, undertow, akka-http, play25/26, commons-fileupload, ...); the public signature is unchanged and every behavioral delta is strictly more correct than the substring-search version.
Motivation
APPSEC-69139: extend the RESTEasy 3.0 multipart AppSec instrumentation to inspect multipart form-field values (
server.request.body) with the same bounded-cap semantics already used for file content, replicating the pattern established by the Jersey 3 reference implementation.The charset-parsing and filename-detection fixes were driven by Codex review comments on this PR that surfaced real charset-confusion and cap-bypass edge cases (tab-separated parameters,
filename*, quotedcharset=-looking substrings, and a null-Content-Type fallback that produced an unparseable string) as the implementation was iterated on.Additional Notes
None.
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: APPSEC-69139
Note: Once your PR is ready to merge, add it to the merge queue by commenting
/merge./merge -ccancels the queue request./merge -f --reason "reason"skips all merge queue checks; please use this judiciously, as some checks do not run at the PR-level. For more information, see this doc.