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

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

Closed
2 tasks done
mattbaileyuk opened this issue Sep 3, 2024 · 4 comments · Fixed by #1036
Closed
2 tasks done

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

mattbaileyuk opened this issue Sep 3, 2024 · 4 comments · Fixed by #1036

Comments

@mattbaileyuk
Copy link

Checks

Describe the bug (be clear and concise)

The recently-published 3.0.1 of http-proxy-middleware includes a change to the logger (#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
Copy link
Owner

chimurai commented Sep 3, 2024

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
Copy link
Author

@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
Copy link
Owner

chimurai commented Sep 4, 2024

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
Copy link

VadBary commented Nov 7, 2024

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

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

Successfully merging a pull request may close this issue.

3 participants