chimurai / http-proxy-middleware

:zap: The one-liner node.js http-proxy middleware for connect, express, next.js and more
MIT License
10.79k stars 852 forks source link

Exception thrown in logger when trying to build URL for message #1056

Open VadBary opened 2 weeks ago

VadBary commented 2 weeks ago

Checks

Describe the bug (be clear and concise)

In fact, the solution for the issue has already been implemented for v3.0.2 But implemented fix doesn't work in case when target option is not set but router option is used instead.

Step-by-step reproduction instructions

1. Send a message to an Express route which then uses `http-proxy-middleware` to send a message on to another endpoint
Have an endpoint which sends a response where the `req` object doesn't include all the properties such as `protocol`. I believe mocking framework like `nock` would do this.
2. Use router option instead of target option.

Expected behavior (be clear and concise)

The logger can cope with bits of req not being present when trying to build a valid URL when router option is used instead of target.

How is http-proxy-middleware used in your project?

We are using `http-proxy-middleware `^3.0.3`with express router for routing.

What http-proxy-middleware configuration are you using?

createProxyMiddleware({
     router: (request) => this.route(request),
     agent,
     secure: false,
     changeOrigin: true,
     followRedirects: true,
     ignorePath: true,
     headers: { Connection: 'keep-alive' },
     plugins: [proxyRequestLoggerPlugin],
     on: {
       error: (err, req, res, target) => this.proxyError(err, req, res, target),
       proxyReq: (proxyReq, req) => {
         this.proxyReq(proxyReq, req);
       },
       proxyRes: (proxyRes, req) => {
         this.proxyRes(proxyRes, req);
       },
     },
   });

What OS/version and node/version are you seeing the problem?

Node v20.15.1

Additional context (optional)

No response

chimurai commented 2 weeks ago

Could you provide a minimal working reproduction for investigation?

VadBary commented 2 weeks ago

Hi @chimurai, my issue is the same like this 1035. I investigated it deeply and know the reason. There is a commit with a fix to resolve the issue, this commit was delivered to v.3.0.2 - fix(logger-plugin): handle undefined protocol and hostname. As part of this commit was added try...catch block to handle the situation when protocol is undefined, in my case, protocol is also undefined... But the solution doesn't work in my case because I don't define target as an option for createProxyMiddleware but set router option instead. If you pay attention on this catch block implementation in logger-plugin.ts target = new URL(options.target as URL); then you will see that options.target is required to build URL but as I mentioned this is possible that target is not set because router option is set instead. In this case, options.target is undefined and target = new URL(undefined as URL); throw a new Error. If avoid using options.target for building URL and use something like that target = new URL(${proxyRes.req.protocol}//${proxyRes.req.host}$proxyRes.req.path}); in catch block then everything would be ok.

chimurai commented 2 weeks ago

Thank you for the analysis.

As you might already have seen in https://github.com/chimurai/http-proxy-middleware/issues/1035#issuecomment-2327374207, I didn't got a reply on the minimal reproduction for further investigation.

Would really appreciate your help if you can provide a minimal example for investigation so the logger can be fixed properly. 🙏

Alternatively opening a PR would also be really helpful