chimurai / http-proxy-middleware

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

http-proxy-middleware v3 adds a '/' before query parameters ending up with 404 #1016

Open mnadeem opened 1 month ago

mnadeem commented 1 month ago

Checks

Describe the bug (be clear and concise)

With the following

app.use(
  createProxyMiddleware('/api/v1/xyz', {
    target: 'https://example.com/api/v1/xyz',
    changeOrigin: true,
  })
);

if a request is being made https://myapp.com/api/v1/xyz?param1=value the proxied request is translating to https://example.com/api/v1/xyz/?param1=value resulting in 404. ( Extra / is being added )

This was not the case with v2 and even with v3 legacy mode.

Step-by-step reproduction instructions

1. Add proxy as described above
2. send the request with query params

Expected behavior (be clear and concise)

No "/" should be added,

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

orx-aim-azure-bff@1.0.0 proxy-app
├── http-proxy-middleware@3.0.0
└─┬ react-scripts@5.0.1
  └─┬ webpack-dev-server@4.15.1
    └── http-proxy-middleware@2.0.6

What http-proxy-middleware configuration are you using?

app.use(
  createProxyMiddleware('/api/v1/xyz', {
    target: 'https://example.com/api/v1/xyz',
    changeOrigin: true,
  })
);

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

```shell
Windows 11, Node 20

Additional context (optional)

No response

emondora commented 1 month ago

same here

danmichaelo commented 1 month ago

@chimurai mentioned in https://github.com/chimurai/http-proxy-middleware/issues/985#issuecomment-2067656422 that this is an upstream issue in the (unmaintained) http-proxy library. I only started seeing it after upgrading http-proxy-middleware from 2.0.6 to 3.0.0 though, which use the same http-proxy version, so that's a bit odd, but perhaps the issue somehow surfaced with #731 ?

Workaround (first attempt):

createProxyMiddleware({
  target: `https://otherhost/api`,
  changeOrigin: true,
  pathRewrite: (path) => path.replace(/\/$/, ""),
});

That doesn't handle URLs with query strings or hashes though.. Second attempt using lookahead to remove /'s followed by either ? or end of line:

createProxyMiddleware({
  target: `https://otherhost/api`,
  changeOrigin: true,
  pathRewrite: (path) => path.replace(/\/(?:(?=\?)|$)/, ""),
});

I have a feeling this one might not be perfect either though :/

chimurai commented 1 month ago

dupe of https://github.com/chimurai/http-proxy-middleware/issues/1000

Indeed related to change in https://github.com/chimurai/http-proxy-middleware/pull/731

Specifically: https://github.com/chimurai/http-proxy-middleware/pull/731/files#diff-07e6ad10bda0df091b737caed42767657cd0bd74a01246a1a0b7ab59c0f6e977L118

trailing slash issue should (ideally) be fixed in http-proxy

Netail commented 1 month ago

trailing slash issue should (ideally) be fixed in http-proxy

The issue is that http-proxy hasn't been updated in 4 years... But this issue doesn't occur in the legacy version, so are you sure it happens in http-proxy

mnadeem commented 1 month ago

Please fix this issue, other wise it will impact usage of latest version

chimurai commented 1 month ago

Hi @mnadeem. It's an open source project. You are welcome fix it upstream or suggest a fix.

Netail commented 1 month ago

I am more wondering why this does occur with the createProxyMiddleware, but not the legacyCreateProxyMiddleware. Trying to investigate this a bit more...

chimurai commented 3 weeks ago

The legacy behaviour is on L30:

https://github.com/chimurai/http-proxy-middleware/blob/897611af512ffaf6add1601585952356fde8aec5/src/http-proxy-middleware.ts#L124-L131

To help investigation, it would be helpful to provide a minimal reproduction of the issue.