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

proxy needs to be last middleware #248

Open
leveneg opened this issue Mar 9, 2018 · 14 comments
Open

proxy needs to be last middleware #248

leveneg opened this issue Mar 9, 2018 · 14 comments

Comments

@leveneg
Copy link

leveneg commented Mar 9, 2018

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();
  }
]);
  • http-proxy-middleware: 0.17.4
  • server: express + 4.16.2

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

chimurai commented Mar 9, 2018

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


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

leveneg commented Mar 9, 2018

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

bannier commented May 12, 2018

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

@NoraGithub
Copy link

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

@dobesv
Copy link

dobesv commented Aug 25, 2018

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

dobesv commented Aug 25, 2018

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

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

aral commented Jul 5, 2019

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

leveneg commented Jul 8, 2019

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

aral commented Jul 8, 2019

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

subiron commented Nov 19, 2019

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
#97 (comment)
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
Copy link

hpl002 commented Jan 6, 2021

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

PaulMaly commented Oct 4, 2021

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

chriscalo commented Apr 16, 2023

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);
  };
}

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

No branches or pull requests