chimurai / http-proxy-middleware

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

proxy needs to be last middleware #248

Open leveneg opened 6 years ago

leveneg commented 6 years ago

Perhaps I'm using the library incorrectly, or unaware of its limitations- but I recently found that I've had to put http-proxy-middleware as the last middleware in my express config.

Expected behavior

If possible, http-proxy-middleware should call the next() middleware. If not, the behavior should be documented.

Actual behavior

middlewares that follow http-proxy-middleware are not executed

Setup

app.use('*', [
  (req, res, next) => {
    console.log('I do get executed');
    next();
  },

  proxy({ ...proxyOptions }),

  (req, res, next) => {
    console.log('I do not get executed');
    next();
  }
]);

proxy middleware configuration

var apiProxy = proxy({
  target:'http://www.example.org',
  changeOrigin:true,
  secure: false
});

server mounting

const https = require('https');
const express = require('express');
const app = express();
const certs = require('./getCertOptions')();
const middlewares = require('./getExpressMiddlewares')();

app.use('*', middlewares);

https.createServer({ certs }, app).listen(5443);
chimurai commented 6 years ago

Currently next() is only called when nothing is proxied. (The opposite of what you expected)

https://github.com/chimurai/http-proxy-middleware/blob/66396d482aa7e68125f14ce0dd1c82bde2bce0a5/lib/index.js#L43


Can you patch it locally to call next() at all times? And see if it works for your setup?

I'll have to investigate what kind of side-effects this changes might have.

leveneg commented 6 years ago

@chimurai doing so does work for me. That said, my other middlewares don't modify request headers, which I imagine is where making this change could be tricky.

I absolutely understand if guaranteeing next() is problematic here, but if that's the case noting it in README or something would be really helpful :)

bannier commented 6 years ago

@chimurai do you know why next() isn't called ?

NoraGithub commented 6 years ago

Why not return a promise or directly next()? It would make other middleware to work abnormally.

dobesv commented 6 years ago

Middleware doesn't call next if it handles the request. This is the case here. This isn't a bug. Otherwise it would call next and the next handler would return 404 Not Found or whatever.

dobesv commented 6 years ago

It will call next if you set it up to only proxy certain paths, and the request doesn't match one of those paths.

vinit-sarvade commented 5 years ago

@chimurai @leveneg when the option: selfHandleResponse is set, then the onProxyRes can have the next as argument that if required can be called for post proxy middleware.

aral commented 5 years ago

Just hit this also with the following use case:

  1. I’m proxying an app (Hugo)
  2. If a URL does not exist there, I want to serve it from an older version of the site

But the proxy middleware doesn’t call next() on 404s so other middleware can’t try and handle the request.

Thoughts?

leveneg commented 5 years ago

@aral using the onProxyRes option might be a good fit:

onProxyRes: (proxyRes, req, res) => {
  if (proxyRes.statusCode === 200) {
    // don't do anything if response successful
    return;
  }

  if (proxyRes.path.includes('VX') {
    // don't do anything if we're already returning from version fallback
    return;
  }

  // otherwise, redirect to versioned instance
  res.redirect('/VX/...');
},

... or something like that?

aral commented 5 years ago

@leveneg Thanks, Grant. I did try fiddling with onProxyRes but hadn’t thought to try a redirect. With cascading archives, given that the idea is that the path doesn’t change, I don’t think this would help in my case. When I get a moment, I will try and add support for calling next() on 404 to http-proxy-middleware. (I‘ve deferred the feature in question for the time being as it isn’t essential to the current release but a nice to have.) Appreciate your help :)

subiron commented 5 years ago

I have another case, I want to add cache layer on top of proxy. flow looks like this: Check if url is in the cache store if so then return response body from cache if not forward request to proxy after response from proxy put its body into the cache. <- because of current implementation I'm unable to do this.

edit: found workaround for my case here https://github.com/chimurai/http-proxy-middleware/issues/97#issuecomment-423257695 also I see that I have missed @vinit-sarvade comment in this thread. still, imho this should behave as most of the others middlewares and call next() by default allowing to override all aspects of the response and be

hpl002 commented 3 years ago

Been a while!

Any updates on this. I too was a bit surprised to see that my subsequent middleware suddenly failed. Would be nice to at least have the argument available in the case of option.selfHandleResponse.

Closing by pending issue provided that there might be more progress here..

One dirty workaround that might work for some is to create a separate express instance which only handles proxying. This instance can then be called in the middleware of the outer instance.

My usecase simply being response validation.

PaulMaly commented 3 years ago

@aral @hpl002 Hi guys, I'm pretty sure it should work:

    app.use((req, res, next) => {
        createProxyMiddleware(path,  {
            selfHandleResponse: true,
            onProxyRes(proxyRes, req, res) {
                if (proxyRes.statusCode === 404) {
                    return next();
                } else {
                    let body = new Buffer('');
                    proxyRes.on('data', (data) => body = Buffer.concat([body, data]));
                    proxyRes.on('end', () => res.end(body));
                }
            }
        })(req, res, next);
    });

What do you think about this solution? The weak point is that HPM will be created on each request.

chriscalo commented 1 year ago

Riffing on @PaulMaly's solution, and through some indirection with a WeakMap, I was able to find a way to call the next() function from onProxyRes().

const app = express();
app.use(proxyWithFallthrough(address1));
app.use(proxyWithFallthrough(address2));
app.listen(PORT);

function proxyWithFallthrough(address) {
  // makes it possible to call the `next()` function from `onProxyRes()`
  const reqNextMap = new WeakMap();

  const proxyHandle = createProxyMiddleware({
    target: address,
    selfHandleResponse: true,
    onProxyRes(proxyRes, req, res) {
      if (proxyRes.statusCode === 404) {
        const next = reqNextMap.get(req);
        return next();
      } else {
        proxyRes.pipe(res);
      }
    },
  });

  return function handle(req, res, next) {
    reqNextMap.set(req, next);
    return proxyHandle(req, res, next);
  };
}
Ashkar2023 commented 4 weeks ago

Middleware doesn't call next if it handles the request. This is the case here. This isn't a bug. Otherwise it would call next and the next handler would return 404 Not Found or whatever.

the next will call the next middleware in the chain. in that case calling next() would just left the request hanging.