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

fix(proxy): avoid decoding request body as utf8 #440

Merged
merged 6 commits into from
Jul 24, 2023

Conversation

Ngob
Copy link
Contributor

@Ngob Ngob commented Jul 13, 2023

πŸ”— Linked issue

#413

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Resolves #413

This pull-request add:

  • A test to test if the proxy preserve the PDF file when sending the file to the upstream service. I added a PDF file into the test/ressource directory
  • A fix that stream the request body to the upstream service. It allow better memory usage and prevent any conversion on the body

This PR consists into 2 commit:

  • The first commit does add the test. In this commit you can see the test failing
  • The second commit does add the fix. In this commit, you can see the test not failing anymore

Let me know if I need to update the PR.
Any comments would be appreciated.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly (does not apply here).

@Ngob Ngob marked this pull request as ready for review July 13, 2023 15:45
@Hebilicious Hebilicious added enhancement New feature or request proxy labels Jul 14, 2023 — with Volta.net
@codecov
Copy link

codecov bot commented Jul 14, 2023

Codecov Report

Merging #440 (1e115b6) into main (5c4521f) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #440      +/-   ##
==========================================
+ Coverage   78.23%   78.36%   +0.13%     
==========================================
  Files          26       26              
  Lines        2775     2792      +17     
  Branches      407      407              
==========================================
+ Hits         2171     2188      +17     
  Misses        604      604              
Impacted Files Coverage Ξ”
src/utils/body.ts 95.13% <100.00%> (+0.49%) ⬆️
src/utils/proxy.ts 81.51% <100.00%> (ΓΈ)

@Ngob Ngob force-pushed the fix/proxy_prevent_binary_file_corruption branch from 5634f51 to 7a3d817 Compare July 14, 2023 08:47
@Ngob Ngob force-pushed the fix/proxy_prevent_binary_file_corruption branch from 7a3d817 to f85b99e Compare July 14, 2023 08:59
@Ngob
Copy link
Contributor Author

Ngob commented Jul 14, 2023

Hello,

I added another test to improve the coverage. It test if additionnals headers are being proxifyed along with request's headers
I also tested on node version 16, 18 and 19 and added the "duplex: 'half'" to make it work using version 18/19. The pipeline should be fixed now.

Thanks for the review

@pi0
Copy link
Member

pi0 commented Jul 17, 2023

Hi and thanks for the PR. I think it is a good idea to support incoming request body streaming but it only works on Node.js and we have an ongoing draft for it (#374). Is there any chance we might be able to fix readRawBody directly? (it is supposed to properly read "binary" request body into a Buffer if there is any possible bug with it..)

@Ngob
Copy link
Contributor Author

Ngob commented Jul 21, 2023

Hi and thanks for the PR. I think it is a good idea to support incoming request body streaming but it only works on Node.js and we have an ongoing draft for it (#374). Is there any chance we might be able to fix readRawBody directly? (it is supposed to properly read "binary" request body into a Buffer if there is any possible bug with it..)

Yes I can do
So what I need to do is to restore the usage of readRawBody and fix the method when using binaries?

@pi0 pi0 changed the title fix(proxy): prevent corruption for binary file fix(proxy): avoid decoding request body as utf-8 Jul 24, 2023
@pi0 pi0 changed the title fix(proxy): avoid decoding request body as utf-8 fix(proxy): avoid decoding request body as utf8 Jul 24, 2023
@pi0
Copy link
Member

pi0 commented Jul 24, 2023

Thanks for helping on this by providing reproduction, explanations and fixtures ❀️

I have made a refactor to simplify fix (We need to pass false as second argument to readRawBody to make it reading body as a stream).

Using proxy for incoming requests seems a good idea too! Leveraging event.request introduced in #454 we can implement this feature in #374 (an experimental flag at first)

@pi0 pi0 merged commit ed3ae90 into unjs:main Jul 24, 2023
@Ngob
Copy link
Contributor Author

Ngob commented Jul 31, 2023

Thanks a lot for the followup!

@pi0 pi0 mentioned this pull request Aug 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request proxy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Corrupted binary file when using proxyRequest
3 participants