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

feat: try to proxy body even after body-parser middleware #492

Merged
merged 9 commits into from
Apr 24, 2021

Conversation

midgleyc
Copy link
Contributor

If a body parser middleware is added before the proxy is configured, proxying POST requests with bodies fails. This fixes that and provides a test, as requested by @chimurai in #460.

Code from #460 by @sarumont and from #320 by @repl-chris used -- thanks to both!

Fixes #90
Fixes #320
Fixes #417

@ghost
Copy link

ghost commented Dec 31, 2020

Congratulations 🎉. DeepCode analyzed your code in 3.34 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

@coveralls
Copy link

coveralls commented Dec 31, 2020

Coverage Status

Coverage decreased (-0.9%) to 84.516% when pulling 71bef4d on midgleyc:body-parser-test into 1704875 on chimurai:master.

@chimurai
Copy link
Owner

chimurai commented Apr 18, 2021

Thanks @midgleyc for this PR. (Sorry for late reply)

Could you separate this.fixBody function to external handler? Similar to responseInterceptor's implementation. https://github.com/chimurai/http-proxy-middleware/tree/master/src/handlers

And maybe rename fixBody to fixRequestBody, to avoid ambiguity between request body and response body.

This'll make fixRequestBody optional, since not everyone is having this issue. And it'll prevent bloat in the core functionality.

It's usage will be like this:

const { createProxyMiddleware, fixRequestBody } = require('http-proxy-middleware');

const proxy = createProxyMiddleware({
  /**
   * Fix bodyParser
   **/
  onProxyReq: fixRequestBody,
});

Planning to work on a preset-like functionality where something like recommended contains a collection of handlers for the common of use-cases.

@midgleyc
Copy link
Contributor Author

@chimurai Done, how does it look now?

Copy link
Owner

@chimurai chimurai left a comment

Choose a reason for hiding this comment

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

test/e2e/_utils.ts Outdated Show resolved Hide resolved
@midgleyc midgleyc requested a review from chimurai April 18, 2021 17:34
@midgleyc
Copy link
Contributor Author

Changes made.

You should be able to make changes to my branch directly, if you'd like.

Copy link
Owner

@chimurai chimurai left a comment

Choose a reason for hiding this comment

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

lgtm

@chimurai chimurai merged commit 397f39f into chimurai:master Apr 24, 2021
@midgleyc midgleyc deleted the body-parser-test branch April 24, 2021 13:03
@chimurai
Copy link
Owner

Thanks @midgleyc !

@midgleyc
Copy link
Contributor Author

You're welcome, thanks for merging :)

@dhurtrci
Copy link

dhurtrci commented Aug 23, 2024

This workaround doesn't seem to work for my case.

In later versions, the proxyReq etc. are set inside the on property:


on: {
                        /**
                        * Fix bodyParser
                        **/
                        proxyReq: fixRequestBody
                    }

In my case I have an https proxy agent as well in the options (without that I don't even need to try the workaround!). Unfortunately I need that as my server runs inside company network and needs to go via proxy to the target.

In such a set-up, I get an error in the fixRequestBody method (where it sets the content type header), and is says (something like) 'cannot change headers after they are sent to the client'.

Specifically the error is:



Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client
    at new NodeError (node:internal/errors:406:5)
    at ClientRequest.setHeader (node:_http_outgoing:652:11)
    at writeBody (D:\API_Mapping_Server\api-mapping-express-frontend-server\dist\PathMapping.js:22:18)

Basically, it seems that the proxyReq isn't able to modify the headers, which seems to largely defeat the point of this 'hook' (in my case at least).....

@chimurai
Copy link
Owner

@dhurtrci Can you please open a new issue and provide a reproduction so it can be investigated?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants