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

onProxyReq not fired #483

Closed
sberney opened this issue Nov 10, 2020 · 8 comments
Closed

onProxyReq not fired #483

sberney opened this issue Nov 10, 2020 · 8 comments

Comments

@sberney
Copy link

sberney commented Nov 10, 2020

Is this a bug report?

Yes.

Expected behavior

onProxyReq runs, allowing me to modify the request.

Actual behavior

onProxyReq is not run when a body is included in the request message, when C#'s HttpClient is used to issue the request.

The setup here is a little non-standard. I have a C# client that uses HttpClient. It makes a request which goes to a node server running http-proxy-middleware.

HttpClient is adding an Expect header for some reason -- something it is entitled to do, but which browser don't do. Usually, an Expect header indicates that we're doing a 100-continue type hand signal. Microsoft's implementation of HttpClient is presumably correct. Developers are not supposed to be explicitly modifying the Expect header.

When I issue a POST with no body, onProxyReq is fired. But when I issue a POST with a body, onProxyReq is never fired. I traced this to one of the latest commits in node-http-proxy -- link. The problem is fixed if that change is undone.

I don't see any reason why they changed this upstream.

It's not clear to me what actions I can take to make http-proxy-middleware proxy my requests that have bodies. I can't modify the implementation or usage of HttpClient, only the server.

I have sizable headers, but the request body itself can be as short as "{}" in order to trigger this bug.

Setup

  • http-proxy-middleware: 1.0.6
  • http-proxy-middleware configuration
app.use(`/mypath`, createProxyMiddleware({

  target: 'http://foobar.com',
  changeOrigin: true,
  headers: {
    'X-Custom-Header': 'foobar'
  },
  preserveHeaderKeyCase: true,
  logLevel: 'debug'
  onProxyReq(proxyReq, req, res) {
    console.log('onProxyReq was fired');
  }
}));
  • server: express 4.17.1

client info

.NET 4.7.2's HttpClient

target server info

Target agnostic -- I've pointed it at an express server that just prints out information about requests that it receives.

Reproducible Demo

No demo at the moment.

@chimurai
Copy link
Owner

Probably related to #40 (comment) .

Feel free to re-open if that's not the case

@sberney
Copy link
Author

sberney commented Nov 11, 2020

This is not related, there is no other middleware.

Although I have been wondering if a way to fix it may be to add middleware, for example by installing body-parser and putting it at the top, exactly the opposite of #40. I saw that work arounds for "related issues" involved pipeline actions before and after onProxyReq. If the express server acted as an endpoint, maybe it would handle Expect-100's. Then I would just need to try to extract the data out of body-parser and put it back into the request before it goes out. But body-parser isn't installed. The only middleware I'm using is morgan() and disabling it doesn't help, and re-ordering the middleware doesn't help.

I investigated more on what I mentioned, about reverting the change in node-http-proxy. It turns out, while I thought that "fixed" things, it simply fixes one part of things. It is looking like http-proxy does not transparently proxy 100's, and falls into the camp of proxies where it's best if 100's are not issued. So after undoing that change, I just run into more, older issues.

@chimurai
Copy link
Owner

Looks like it's indeed an upstream lib issue.
The change you were talking about is related to a security fix (http-party/node-http-proxy#1447)

Nothing I can really do from HPM perspective. onProxyReq is just a alias for http-proxy's .on('proxyReq') listener.

@alwint3r
Copy link

I know it's been months. But there's a workaround here http-party/node-http-proxy#1219, but I'm not sure if it's the right solution. Regardless, is there any way for us to access the http-proxy directly from HPM?

@sberney
Copy link
Author

sberney commented Feb 15, 2021

I forget how it's related to this precise issue, and exactly what that codebase contained, but I did get my proxy server running. I had to check for the Expect header and strip it; use body-parser in the raw mode to then later copy the body onto the proxied request. I also had to run node 14, and start the server using the extra options argument to http(s).createServer(options, app), specifying the node-14-only max-header size property to be much larger.

I seem to have moved past this/worked around it, but I really don't remember more details; I apologize.

Edit: I remember also possibly specifying older versions of the libraries involved, but not if that was included in the final solution.

@radiorz
Copy link

radiorz commented Nov 17, 2021

follow the solution in http-party/node-http-proxy#1219 , We should delete the req.headers.expect and add it back in onProxyReq function and it works!!!!
before:

 if ((req.headers || {}).expect) {
            req.__expectHeader = req.headers.expect;
            delete req['headers'].expect;
        }

onProxyReq

  if (req.__expectHeader) {
            proxyReq.setHeader('Expect', req.__expectHeader)
        }

@saki82
Copy link

saki82 commented Aug 22, 2022

@radiorz
Where exactly did you get access to the original request object and executed this block?

  if ((req.headers || {}).expect) {
             req.__expectHeader = req.headers.expect;
             delete req['headers'].expect;
         }

As we are using http-proxy-middleware, this is (basically) my current usage:

export function setupProxies(app, r) {
        app.use(r.url, createProxyMiddleware(r.proxyOptions));
    };
}

And in my main file i call it like:

import express from 'express';

const app = express();
setupProxies(app, ROUTES);

I tried deleting the header like so:

app.use(deleteExpectHeader);

var deleteExpectHeader = function (req: Request, res: Response) {
    if (req.headers.expect) {
        delete req.headers.expect;
    }
};

No luck with this. Any help is much appreciated.
Thanks, Sakis

@saki82
Copy link

saki82 commented Aug 22, 2022

Answered my own question: I had to call the "deleteExceptHeader" middleware before the "setupProxies".

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