Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue #11387: Reintroduce MultiPartCompliance.LEGACY in ee9/ee8 #11388

Merged
merged 29 commits into from
Feb 27, 2024
Merged

Issue #11387: Reintroduce MultiPartCompliance.LEGACY in ee9/ee8 #11388

merged 29 commits into from
Feb 27, 2024

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Feb 7, 2024

Reintroduce MultiPartCompliance.LEGACY behaviors.

@joakime joakime self-assigned this Feb 7, 2024
@joakime joakime added Enhancement Sponsored This issue affects a user with a commercial support agreement Specification For all industry Specifications (IETF / Servlet / etc) labels Feb 7, 2024
@joakime joakime linked an issue Feb 7, 2024 that may be closed by this pull request
Reintroduce MultiPartCompliance.LEGACY (not as default) too allow for parsing of non-compliant multipart/form-data #11387
Closed
@joakime
Correcting javadoc
e7a92ca
@joakime
Updating MultiPartCaptureTest to ...
9120cfd 
* Test with MultiPartFormData.Parser and MultiPart.Parser
* Enable all test cases
  * base64 behaviors modified to not auto-decode base64 content
  * forms submitted without `_charset_` part (some using a different
     charset than UTF-8, like `Shift_JIS`)
@joakime
Fixing checkstyle warning
9a7371f
@joakime
Re-enable Part-ContainsContents expectations
542f3ff
@joakime
Rename MultiPartCompliance.NO_CRLF_AFTER_PREAMBLE to WHITESPACE_AFTER…
744e382 
…_PREAMBLE to fit spec better
@joakime
Make ee9/ee8 legacy parser use legacy tokenization
a2f2583
@joakime
Testing ee9/ee8 legacy parser base64 auto-decoding behaviors
f82683a
@joakime
Cleanup jetty-test-multipart class naming
c018cae
@joakime
Adding ee10 tests against raw multipart examples
56652f1
@joakime
Adding shorter whitespace multipart test
ebd109d
@joakime
Adding jetty-core version of failing ee10 tests
eba54dd
@joakime
Fixing checkstyle
367efd8
sbordet and others added 6 commits February 13, 2024 20:09
@sbordet
Fixed missed notification for CR content in case of 1 chunk ending wi…
9321e5e 
…th CR and the next chunk ending with LF.

Removed internal unused class MultiPartParser.

Signed-off-by: Simone Bordet <[email protected]>
@joakime
Update test case, as there is now an extra notifyCRContent event
6e80cb7
@joakime
Update test case, as there is now an extra notifyCRContent event
18a0acd
@joakime
Adding MultiPartCompliance.Violation events
edabdb5 
+ in MultiPart.Parser
+ in MultiPartFormData.Parser
@joakime
Rename WHITESPACE_AFTER_PREAMBLE to WHITESPACE_BEFORE_BOUNDARY
d56838e 
+ lenient mode behavior
+ new name fits violation better
+ new tests
@joakime
Notifying WHITESPACE_BEFORE_BOUNDARY
65bba39 
+ adding violation to MultiPart.Parser.parseHeaderStart
This was referenced Feb 14, 2024
Merged
Closed
@joakime
Copy link
Contributor Author

joakime commented Feb 14, 2024 •
edited
Loading

For people reviewing this PR.
Don't be scared by the number of files.
Over 252 files are just moved and consolidated testcases from various parts of the codebase into a new tests/jetty-test-multipart module.

If you exclude (uncheck) the entries for .raw and .txt and Deleted Files in the "File Filter" drop-down, then the review becomes much easier to manage.

raw-txt-exclusions

gregw
gregw requested changes Feb 15, 2024
View reviewed changes
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

The violation/compliance stuff looks good, but the actual handing of a hanging CR looks wrong and needs some careful analysis.

Do we have a test that parses multi-part form 1 byte chunks?

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java Outdated Show resolved Hide resolved
jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java Outdated Show resolved Hide resolved
@joakime
Copy link
Contributor Author

joakime commented Feb 15, 2024

Do we have a test that parses multi-part form 1 byte chunks?

I don't know, but it shouldn't be that hard to setup.

@joakime
Copy link
Contributor Author

joakime commented Feb 20, 2024

This PR is largely complete.
But I'm waiting on PR #11409 to complete so I can update this PR and eliminate some of the no longer relevant noise from this PR.

@joakime joakime marked this pull request as ready for review February 23, 2024 17:03
@joakime joakime requested a review from gregw February 23, 2024 17:25
@sbordet sbordet self-requested a review February 27, 2024 08:44
gregw
gregw previously approved these changes Feb 27, 2024
View reviewed changes
@sbordet
Cosmetics.
cc745c7 
Signed-off-by: Simone Bordet <[email protected]>
@sbordet
More cosmetics.
460b83e 
Signed-off-by: Simone Bordet <[email protected]>
@sbordet sbordet self-requested a review February 27, 2024 15:21
@joakime joakime merged commit 98ceb73 into jetty-12.0.x Feb 27, 2024
8 checks passed
@joakime joakime deleted the fix/12.0.x/multipart-legacy-parser branch February 27, 2024 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbordet sbordet sbordet approved these changes

@gregw gregw gregw left review comments

@lachlan-roberts lachlan-roberts Awaiting requested review from lachlan-roberts

Assignees

@joakime joakime

Labels
Enhancement Specification For all industry Specifications (IETF / Servlet / etc) Sponsored This issue affects a user with a commercial support agreement
Projects
No open projects
Jetty 12.0.7 - FROZEN
Status: ✅ Done
Milestone
No milestone
3 participants
@joakime @gregw @sbordet