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 9 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". |
This comment has been minimized.
This comment has been minimized.
🟢 Java Benchmark SLOs — All performance SLOs passed
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
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 only, 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.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.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.MultipartHelperTest.groovycovering: unchecked-exception resilience,getHeaders()throwing/returning null, UTF-8 charset defaulting, thetext/plainfilter excluding file parts from the cap, and dummy file parts not exhausting the cap before a real text field.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.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.