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 853 forks source link

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

Closed mattbaileyuk closed 2 months ago

mattbaileyuk commented 2 months ago

Checks

Describe the bug (be clear and concise)

The recently-published 3.0.1 of http-proxy-middleware includes a change to the logger (https://github.com/chimurai/http-proxy-middleware/pull/1001) which attempts to build a valid URL object using parts coming into proxyRes.

However, we're seeing this blow up in some of our unit tests:

Node server exiting due to exception: TypeError [ERR_INVALID_URL]: Invalid URL
    at new NodeError (node:internal/errors:405:5)
    at new URL (node:internal/url:676:13)
    at ProxyServer.<anonymous> (/Users/matt/myapplication/node_modules/http-proxy-middleware/dist/plugins/default/logger-plugin.js:35:24)
    at ProxyServer.emit (/Users/matt/myapplication/node_modules/eventemitter3/index.js:204:33)
    at OverriddenClientRequest.<anonymous> (/Users/matt/myapplication/node_modules/http-proxy/lib/http-proxy/passes/web-incoming.js:173:27)
    at OverriddenClientRequest.emit (node:events:517:28)
    at respond (/Users/matt/myapplication/node_modules/nock/lib/playback_interceptor.js:307:11)
    at Timeout.cb [as _onTimeout] (/Users/matt/myapplication/node_modules/nock/lib/common.js:605:9)
    at listOnTimeout (node:internal/timers:569:17)
    at process.processTimers (node:internal/timers:512:7) {
  input: 'undefined//undefined/management/users',
  code: 'ERR_INVALID_URL'
}

The change does not cope with parts of req being missing in proxyRes, such as req.protocol and req.host as observed here. In 3.0.0 this would just get included in the log message as undefined//undefined/management/users and now it breaks http-proxy-middleware, so we have a breaking change here, even if it's just for more usual behaviour.

I think this is because nock isn't returning all the bits of req that this code expects (no protocol or hostname but does send pathname) - or not in the right format, as I'm unsure if proxyRes.req is being built or just copied in from the incoming message. I think the code could be updated to follow what was done with the port and make it conditional, maybe defaulting to http://localhost to make the URL valid.

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
2. 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.

Expected behavior (be clear and concise)

The logger can cope with bits of req not being present when trying to build a valid URL and does not just blow up (especially if you don't even have logging enabled, as in our case)

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

Our CommonJS module requires `http-proxy-middleware`, pulling in `^3.0.0` in `package.json`

What http-proxy-middleware configuration are you using?

const proxyOptions = {
      target: 'https://' + hostname + path,
      changeOrigin: true,
      on: {
        proxyReq: fixRequestBody
      },
      ws: true,
      secure: true
    }

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

```shell
Node v18.20.4

Additional context (optional)

No response

chimurai commented 2 months ago

Thanks for reporting the issue @mattbaileyuk

I released v3.0.2 with a fix.

Please let me know if this fixes the issue you're facing.

Not sure what's happening with nock. I'll investigate the issue further when I have time.

Sorry for the inconvenience.

mattbaileyuk commented 2 months ago

@chimurai Thanks for responding and publishing a fix so quickly šŸ™‚ All looks good at our end with 3.0.2

I added an onProxyRes function which logged out the inbound proxyRes.req object in response to the call to the nock endpoint; I could see that there was an options object on it which had protocol and host (so proxyRes.req.options.protocol instead of proxyRes.req.protocol), so the information is there but not where you would expect.

chimurai commented 2 months ago

Happy to hear the fix works šŸ‘

Thank you for doing additional investigation. This is really helpful.

Would it be possible to provide a minimal reproduction with nock to further investigate the issue and help finding a proper solution?

VadBary commented 2 weeks ago

Hi @chimurai , this fix doesn't work in case if router option is set but target option is not set.