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

Fixed binary data from form-data not getting converted to base64 #9

Merged
merged 2 commits into from
Jun 13, 2023
Merged

Fixed binary data from form-data not getting converted to base64 #9

merged 2 commits into from
Jun 13, 2023

Conversation

danniehansen
Copy link
Contributor

This PR resolves brefphp/bref#1537 by not converting request body to UTF8 send form-data requests as base64. It utilize https://www.npmjs.com/package/body-parser as a middleware to get the raw request body as a buffer.

I'm not too sure of the implementation. I feels a bit hackish looking at the raw body to detect form-data requests. Additionally, form-data requests may contain content that can perfectly well be displayed in UTF8 (such as regular form-data or CSV file uploads).

Investigation with a AWS demo setup showed that even CSV uploads gets received in the Lambda as base64 encoded.

I'm wondering if it may be better to simply always send as base64. Unless of course AWS has some documentation hidden some where on how they detect if it should send base64 or not.

Looking for feedback.

@danniehansen
Copy link
Contributor Author

I'm wondering if we could use something like https://stackoverflow.com/questions/75108373/how-to-check-if-a-node-js-buffer-contains-valid-utf-8 to detect valid UTF8 request body, and then only base64 encode in cases where it's not valid UTF8 🤔

@playable-dannie
Copy link

@mnapoli Sorry, not to be pushy. But any chance you could give me some feedback on the MR? Then i can help adjust anything that needs adjusting this weekend 👍

@mnapoli
Copy link
Member

mnapoli commented May 31, 2023

Hey all, sorry for not checking this sooner, I left that aside because of the [WIP] in the title.

I'm 👍 with the approach, thanks for implementing that!

I think the tests need to be slightly adjusted, and if you are able, adding one test to cover this would be awesome. If you can't, I'd rather merge this anyway, just let me know!

@playable-dannie
Copy link

Thank you @mnapoli 👍 Appreciate it.

I'll check the tests this weekend and implement a test covering this use-case 🙌

…buffers & added test for binary file uploads
@danniehansen
Copy link
Contributor Author

@mnapoli Sorry for the delay. Got thrown into a moving chaos for a weeks time. I've resolved the issue with the unit tests (covering GET requests properly) and introduced a test coverting form-data file uploads containing binary data (simple CSV).

@danniehansen danniehansen changed the title [WIP] Fixed binary data from form-data not getting converted to base64 Fixed binary data from form-data not getting converted to base64 Jun 8, 2023
@mnapoli
Copy link
Member

mnapoli commented Jun 13, 2023

Awesome thanks!

@mnapoli mnapoli merged commit 897663d into brefphp:main Jun 13, 2023
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

Successfully merging this pull request may close these issues.

File uploads get's corrupted in local development images
3 participants