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.

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

parse_part dropping any instance of \r\n #17

Open
sgrove opened this issue Aug 22, 2017 · 6 comments
Open

parse_part dropping any instance of \r\n #17

sgrove opened this issue Aug 22, 2017 · 6 comments

Comments

@sgrove
Copy link

sgrove commented Aug 22, 2017

Took awhile to track this down, but our binary uploads may contain \r\n in the file bodies somewhere, and it looks like Multipart is stripping every instance, leading to invalid files being stored to disk.

It looks like it might be here https://github.com/cryptosense/multipart-form-data/blob/master/lib/multipart.ml#L163 where all of the lines, having been split on \r\n, are simply concatenated together as-is, not restoring the \r\n that was there beforehand (to be clear, we need the bytes 0d0a to be in the final filestream).

I tried a few different hacks, but I'm not very familiar with Lwt_stream, and couldn't quite figure out how best to get it to interpose the streams I need. Any hints on how to make it work?

@wokalski
Copy link

@sgrove it seems like you handled this and other issues in your fork. Maybe you could PR it if it's been working for you so far?

@emillon
Copy link
Contributor

emillon commented Jan 31, 2018

Right, sorry for not answering before. As mentioned in #16 I'm not super convinced about the current API, and this package will probably go through significant changes, but that shouldn't prevent it from being correct :) If you have a working patch I'm happy to merge it. Thanks!

@xguerin
Copy link

xguerin commented Jul 31, 2020

This has become very relevant for me. I'll see if I can address that issue.

@emillon
Copy link
Contributor

emillon commented Aug 3, 2020

I'm not a maintainer anymore, but if this package does not work, you might want to try https://github.com/dinosaure/multipart_form (not released yet).

@xguerin
Copy link

xguerin commented Aug 10, 2020

Thanks. I decided for a direct API call instead, but I'll take a look if I decide to use multipart in the future.

@lindig
Copy link

lindig commented Jan 24, 2021

I also suspect that handling of \r\n is not correct: rgrinberg/opium#258 (comment)

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

No branches or pull requests

5 participants