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

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

Open
2 tasks done
mnadeem opened this issue Jul 6, 2024 · 8 comments
Open
2 tasks done
Labels

Comments

@mnadeem
Copy link

mnadeem commented Jul 6, 2024

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?

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

same here

@danmichaelo
Copy link

danmichaelo commented Jul 11, 2024

@chimurai mentioned in #985 (comment) 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
Copy link
Owner

dupe of #1000

Indeed related to change in #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
Copy link

Netail commented Jul 29, 2024

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

mnadeem commented Jul 30, 2024

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

@chimurai
Copy link
Owner

chimurai commented Jul 30, 2024

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

@Netail
Copy link

Netail commented Jul 30, 2024

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

@chimurai
Copy link
Owner

chimurai commented Aug 4, 2024

The legacy behaviour is on L30:

/**
* Incorrect usage confirmed: https://github.com/expressjs/express/issues/4854#issuecomment-1066171160
* Temporary restore req.url patch for {@link src/legacy/create-proxy-middleware.ts legacyCreateProxyMiddleware()}
* FIXME: remove this patch in future release
*/
if ((this.middleware as unknown as any).__LEGACY_HTTP_PROXY_MIDDLEWARE__) {
req.url = (req as unknown as any).originalUrl || req.url;
}

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants