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

Multipart request parsing fails in LF mode with certain binary data. #2581

Closed
cyburgee opened this issue Jun 27, 2019 · 2 comments
Closed

Multipart request parsing fails in LF mode with certain binary data. #2581

cyburgee opened this issue Jun 27, 2019 · 2 comments
Labels
1 - triaged Tickets that are safe to pick up for contributing in terms of likeliness of being accepted bug help wanted Identifies issues that the core team will likely not have time to work on nice-to-have (low-prio) Tasks which make sense, however are not very high priority, feel free to help out! t:core Issues related to the akka-http-core module t:marshalling
Milestone
10.2.0

Comments

@cyburgee
Copy link
Contributor

Hello maintainers,

I've encountered an issue whereby POST requests with certain binary data fail with an error message reading:
akka.http.scaladsl.model.ParsingException: Unexpected end of multipart entity

This is rectified by formatting the multipart request in CRLF mode, so I believe there is an issue with the EOL detection here:
https://github.com/akka/akka-http/blob/master/akka-http-core/src/main/scala/akka/http/impl/engine/parsing/BodyPartParser.scala#L338

I put together what might be a fix here:
https://github.com/cyburgee/akka-http/blob/multipart-eol-parsing/akka-http-core/src/main/scala/akka/http/impl/engine/parsing/BodyPartParser.scala#L338

and a test with an example entity here:
https://github.com/cyburgee/akka-http/blob/multipart-eol-parsing/akka-http-core/src/test/scala/akka/http/impl/engine/parsing/RequestParserSpec.scala#L682

My change makes the assumption that the EOL should be indicated immediately after the occurrence of a boundary pattern in the request. However, I may be totally off here.

Please let me know if you need any more information.
@jrudolph jrudolph added 0 - new Ticket is unclear on it's purpose or if it is valid or not bug t:core Issues related to the akka-http-core module labels Jul 10, 2019
@jrudolph jrudolph added t:marshalling 1 - triaged Tickets that are safe to pick up for contributing in terms of likeliness of being accepted help wanted Identifies issues that the core team will likely not have time to work on nice-to-have (low-prio) Tasks which make sense, however are not very high priority, feel free to help out! and removed 0 - new Ticket is unclear on it's purpose or if it is valid or not labels Apr 6, 2020
@jrudolph
Copy link
Member

jrudolph commented Apr 6, 2020

Hi @cyburgee, sorry, it seems this one fell through the cracks. I guess one reason is that the relaxed line ending parsing mode was contributed and we are not so confident with its workings.

In any case, thanks for the details.

If I understand correctly, the current behavior is to use a heuristic by default to figure out line endings of body parts. However, what you found out, is that the heuristic can be mislead when the body part contents contain CR or LF characters that are not supposed to be a line ending (which is likely with binary data).

My basic stance is that we must make sure that the heuristic works fine for all cases of well-defined body parts according to the specs (i.e. using CRLF). For all other cases, we are in nice-to-be-supported area.

Your suggestion looks reasonable, though, so if you still want, please open a PR.

cyburgee added a commit to cyburgee/akka-http that referenced this issue Apr 16, 2020
@cyburgee
Fix request body line parsing: Issue akka#2581
ee8a5e4 
* Improved the heuristic for detecting end of line character(s)
* Added tests for UndefinedEndOfLineConfiguration
cyburgee added a commit to cyburgee/akka-http that referenced this issue Apr 16, 2020
@cyburgee
Fix request body line parsing: Issue akka#2581
bca40a4 
* Improved the heuristic for detecting end of line character(s)
* Added tests for UndefinedEndOfLineConfiguration
cyburgee added a commit to cyburgee/akka-http that referenced this issue Apr 16, 2020
@cyburgee
Fix request body line parsing: Issue akka#2581
a943532 
* Improved the heuristic for detecting end of line character(s)
* Added tests for UndefinedEndOfLineConfiguration
cyburgee added a commit to cyburgee/akka-http that referenced this issue Apr 16, 2020
@cyburgee
Fix request body line parsing: Issue akka#2581
8ceb32c 
* Improved the heuristic for detecting end of line character(s)
* Added tests for UndefinedEndOfLineConfiguration
@cyburgee cyburgee mentioned this issue Apr 16, 2020
Merged
jrudolph pushed a commit to cyburgee/akka-http that referenced this issue Apr 22, 2020
@cyburgee @jrudolph
Fix request body line parsing: Issue akka#2581
bf6f855 
* Improved the heuristic for detecting end of line character(s)
* Added tests for UndefinedEndOfLineConfiguration
jrudolph added a commit to cyburgee/akka-http that referenced this issue May 11, 2020
@cyburgee @jrudolph
core: fix EOL detection for body part parsing
428877e 
Refs akka#2581

 * Improved the heuristic for detecting end of line character(s)
 * Added tests for UndefinedEndOfLineConfiguration

Co-authored-by: Johannes Rudolph <johannes.rudolph@gmail.com>
jrudolph pushed a commit to jrudolph/akka-http that referenced this issue May 11, 2020
@cyburgee @jrudolph
[backport] core: fix EOL detection for body part parsing
139d0cd 
10.1 Backport of akka#3076

Refs akka#2581

 * Improved the heuristic for detecting end of line character(s)
 * Added tests for UndefinedEndOfLineConfiguration

Co-authored-by: Johannes Rudolph <johannes.rudolph@gmail.com>
(cherry picked from commit 428877e)
@jrudolph jrudolph mentioned this issue May 11, 2020
Merged
raboof pushed a commit that referenced this issue May 11, 2020
@jrudolph @cyburgee
[backport] core: fix EOL detection for body part parsing (#3144)
0d84f65 
10.1 Backport of #3076

Refs #2581

 * Improved the heuristic for detecting end of line character(s)
 * Added tests for UndefinedEndOfLineConfiguration

Co-authored-by: Johannes Rudolph <johannes.rudolph@gmail.com>
(cherry picked from commit 428877e)

Co-authored-by: Collin Burger <collin.e.burger@gmail.com>
@jrudolph jrudolph added this to the 10.2.0 milestone May 13, 2020
@jrudolph
Copy link
Member

Fixed by #3076.

@ennru ennru closed this as completed May 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
1 - triaged Tickets that are safe to pick up for contributing in terms of likeliness of being accepted bug help wanted Identifies issues that the core team will likely not have time to work on nice-to-have (low-prio) Tasks which make sense, however are not very high priority, feel free to help out! t:core Issues related to the akka-http-core module t:marshalling
Projects
None yet
Milestone
10.2.0
Development

No branches or pull requests
3 participants
@jrudolph @ennru @cyburgee