http-party / node-http-proxy

A full-featured http proxy for node.js
https://github.com/http-party/node-http-proxy
Other
13.89k stars 1.97k forks source link

Unable to proxy multi-part form #760

Closed todkap closed 5 years ago

todkap commented 9 years ago

Multiple attempts to proxy multi-part using both the browser and curl.

Might be something really small but cannot seem to figure it out. The curl command sends the expect header and the browser does not. However, both do not proxy the request. Might be easier to see the first issue with the curl command and then narrow the issue down for the browser.

Curl command curl -v --upload-file ./cam_espn.jpg http://127.0.0.1:3000/todkap%40us.ibm.com/cam_espn5.jpg

Output todkapmacbookpro:bluemix_objectstorage_bug todd$ curl -v --upload-file ./cam_espn.jpg http://127.0.0.1:3000/todkap%40us.ibm.com/cam_espn5.jpg

Node script todkapmacbookpro:bluemix_objectstorage_bug todd$ cat app.js

var https  = require('https');
var url = require("url");
var express = require('express');
var request = require("request");
var httpProxy = require("http-proxy");
var app = express()

var multer  = require('multer')
app.use(multer({ inMemory: true}))

app.use(require('connect-restreamer')());

//
// Create a proxy server with custom application logic
//
var proxy = httpProxy.createProxyServer({});

// Append Swift security token to all proxied outbound calls
proxy.on('proxyReq', function(proxiedReq, req, res, options) {
    console.log('RAW request headers from client', JSON.stringify(req.headers, true, 2));
    if(!req.header('expect')){
        proxiedReq.setHeader('TestHeader', foo);

    }else{
        res.setHeader("ProxyRequestEventFoundExpect", "true");
    }
});

// Proxy logic..
app.use(function (clientRequest, clientResponse, next){
    console.log('Logging inbound request %s %s', clientRequest.method, clientRequest.url);
    console.log("request body", clientRequest.body);
    console.log("request files", clientRequest.files);

    var proxyURL = "www.google.com" ;

    var options = {target : proxyURL};
    if(!clientRequest.header('expect')){
        proxy.web(  clientRequest, clientResponse, options  );
    }else{
        console.log("request had an expect in it... skip proxy call");
        clientResponse.writeContinue();
        clientResponse.setHeader("SkipProxyWeb", "true");
        next();
    }
})

//
// // Listen for the `proxyRes` event on `proxy`.
// //
proxy.on('proxyRes', function (proxyRes, req, res) {
    console.log("attempting to log the response in proxyRes");
    console.log('RAW Response from the target', JSON.stringify(proxyRes.headers, true, 2));
});

var server = app.listen(3000, function () {
    var host = server.address().address
    var port = server.address().port
    console.log('Example app listening at http://%s:%s', host, port)
})

Server side log Logging inbound request PUT /todkap%40us.ibm.com/cam_espn5.jpg request body {} request files {} request had an expect in it... skip proxy call

todkap commented 9 years ago

Main areas of confusion are.. 1) How do deal with 100 Continue when it appears that express supports this inherently. 2) Do you need to have a mutli-part form parser (such as multer) for simple proxy scenarios or can I just populate the request body and pass this in the buffer for the proxy API call 3) Can I still use the connect-restreamer?

There are lots of docs out there via google searches but most of the content predates the express connect redesign that happened last year.

jcrugzz commented 9 years ago

@todkap

  1. Check the node docs
  2. If the server you are proxying to is handling the actual form data (not your proxy server), you only need to parse it with that destination server. You can avoid the dance with the multi-part parser in the proxy itself
  3. If 2 is true, then there would be no need for connect-restreamer.

Otherwise the code looks like it would behave appropriately but your test case did not even test if it did.

todkap commented 9 years ago

Let me simplify this scenario a bit then since I think we are headed in the right direction (note i layered in multer and connect-restreamer after hitting many issues).

Here is the updated code. I have removed the checks for the continue since as you note, Node should be handling this for me. The moment I try to do this with a request that has an expect header, I get an immediate exception about unable to set headers. There must be some strange timing window here (note this is just a single command line client so no threading issues should exist).

Console Output Example app listening at http://0.0.0.0:3000 Logging inbound request PUT /todkap%40us.ibm.com/camespn5.jpg request body undefined request files undefined RAW request headers from client { "user-agent": "curl/7.37.1", "host": "127.0.0.1:3000", "accept": "/_", "content-length": "197616", "expect": "100-continue" }

http.js:689 throw new Error('Can\'t set headers after they are sent.'); ^ Error: Can't set headers after they are sent. at ClientRequest.OutgoingMessage.setHeader (http.js:689:11) at ProxyServer.app.use.proxyURL (/Users/todd/Documents/workspace/mobilecloudsamples/bluemix_objectstorage_bug/app.js:17:13) at ProxyServer.emit (/Users/todd/Documents/workspace/mobilecloudsamples/bluemix_objectstorage_bug/node_modules/http-proxy/node_modules/eventemitter3/index.js:75:35) at ClientRequest. (/Users/todd/Documents/workspace/mobilecloudsamples/bluemix_objectstorage_bug/node_modules/http-proxy/lib/http-proxy/passes/web-incoming.js:113:27) at ClientRequest.emit (events.js:117:20) at http.js:1762:9 at process._tickCallback (node.js:419:13)

thanks! todd

todd kaplinger STSM, Mobile Cloud Platform Architect (IBM MobileFirst for iOS) ibm master inventor

From: Jarrett Cruger notifications@github.com To: nodejitsu/node-http-proxy node-http-proxy@noreply.github.com Cc: Todd Kaplinger/Durham/IBM@IBMUS Date: 01/02/2015 11:21 AM Subject: Re: [node-http-proxy] Unable to proxy multi-part form (#760)

@todkap

  1. Check the node docs
  2. If the server you are proxying to is handling the actual form data (not your proxy server), you only need to parse it with that destination server. You can avoid the dance with the multi-part parser in the proxy itself
  3. If 2 is true, then there would be no need for connect-restreamer. Otherwise the code looks like it would behave appropriately but your test case did not even test if it did. — Reply to this email directly or view it on GitHub.
jcrugzz commented 9 years ago

Well it seems because the middleware stack is still executing, even if it does send the 100 response automatically you need to handle the case because it won't stop the middleware chain. I also have no idea where the error is coming from in your code so I can't point out exactly whats wrong. But you are probably doing a res.setHeader after you have already sent the response or the http-proxy logic was given a request and response where the response had already been replied to (this is kind of my feeling with the middleware chain still executing). I'd keep digging into this problem and open an issue if you find a bug in http-proxy but this just seems to be an implementation issue atm. Stackoverflow will be a good place to post next if you can't figure it out :). Good luck!

todkap commented 9 years ago

I do not mind going to stack overflow. There must be something strange here since the failure to set the header is on the "REQUEST* not the response and is happening in htis method. Do you not have testcases for multi-part form?

// Append Swift security token to all proxied outbound calls proxy.on('proxyReq', function(proxiedReq, req, res, options) { console.log('RAW request headers from client', JSON.stringify(req.headers, true, 2)); proxiedReq.setHeader('TestHeader', 'foo'); });

Note I am not doing anything special in this code, I have pretty much removed all of the app logic with the failure happening prior to the actual proxy call.

thanks! todd

todd kaplinger STSM, Mobile Cloud Platform Architect (IBM MobileFirst for iOS) ibm master inventor

From: Jarrett Cruger notifications@github.com To: nodejitsu/node-http-proxy node-http-proxy@noreply.github.com Cc: Todd Kaplinger/Durham/IBM@IBMUS Date: 01/02/2015 11:47 AM Subject: Re: [node-http-proxy] Unable to proxy multi-part form (#760)

Well it seems because the middleware stack is still executing, even if it does send the 100 response automatically you need to handle the case because it won't stop the middleware chain. I also have no idea where the error is coming from in your code so I can't point out exactly whats wrong. But you are probably doing a res.setHeader after you have already sent the response or the http-proxy logic was given a request and response where the response had already been replied to (this is kind of my feeling with the middleware chain still executing). I'd keep digging into this problem and open an issue if you find a bug in http-proxy but this just seems to be an implementation issue atm. Stackoverflow will be a good place to post next if you can't figure it out :). Good luck! — Reply to this email directly or view it on GitHub.

jcrugzz commented 9 years ago

@todkap Ahh ok. So it seems that when you don't handle the write-continue case when the client sends a 100 request, it must fast-laned even when proxied where the .on('socket') event doesn't even allow headers to be set. Im looking to see where this happens in node core as this is kind of interesting.

But my recommendation here is to put back the logic you had detecting the expect 100 request which was the problem I was inferring previously. I mistakenly made you change the code so that the 100 request was being proxied (because of how middleware works I guess or the writeContinue not happening automatically). You want to proxy just the payload and have your proxy-app tell the client to send the payload so that it can be proxied to the proper destination. I believe that is what is causing the setHeader error.

jcrugzz commented 9 years ago

Also we do not have multi-part form tests (though I would love to!) But in this case it shouldn't make a difference as we will proxy any binary data that comes down the pipe as we just simply make a new request and use the .pipe() method internally. It should be because of the writeContinue dance as we do not handle specially handle expect 100 requests innately.

todkap commented 9 years ago

I was able to get the browser to work successfully for me (issue with the back end server and how it handles PUT versus POST). I am not super concerned about the 100 Continue but if you have time to investigate that would be fine with me :)

thanks! todd

todd kaplinger STSM, Mobile Cloud Platform Architect (IBM MobileFirst for iOS) ibm master inventor

From: Jarrett Cruger notifications@github.com To: nodejitsu/node-http-proxy node-http-proxy@noreply.github.com Cc: Todd Kaplinger/Durham/IBM@IBMUS Date: 01/02/2015 12:35 PM Subject: Re: [node-http-proxy] Unable to proxy multi-part form (#760)

Also we do not have multi-part form tests (though I would love to!) But in this case it shouldn't make a difference as we will proxy any binary data that comes down the pipe as we just simply make a new request and use the .pipe() method internally. It should be because of the writeContinue dance as we do not handle specially handle expect 100 requests innately. — Reply to this email directly or view it on GitHub.

conartist6 commented 8 years ago

I ran across this issue with ember-cli and had a bit of trouble decoding what has been said here: for anyone who runs across this, here is my decoding:

When sending a multipart/form-data POST the browser may not send the form data right away. It instead sends what I'll call a request-for-continue, consisting of just the headers that will be sent and an extra header: expect: 100-continue

To avoid large amount of unintelligible data being sent, the server must respond with an HTTP 100 acknowledging that it will be capable of decoding the multipart/form-data.

Since express.js does support this type of decoding, part of its functionality consists of automagically responding to this request with the requisite HTTP 100 response.

The problem is that express still allows middleware to operate on the request-for-continue after it has been responded to, and thus the proxy forwards the request-for-continue along. This is bad.

The solution is to insert a bit of middleware that runs before the http-proxy in the middleware stack and which does NOT continue through the stack if it detects the request is a request-for-continue. The solution is flawed in a proxying sense as the user is no longer protected from the expense of sending multipart form data to a server which cannot decode it.

RangerMauve commented 7 years ago

Is there an example of the middleware that needs to be made and how it gets integrated with http-proxy?

RangerMauve commented 7 years ago

Disregard that, apparently it works out of the box with the default config. Awesome software, by the way. Big thanks to the creators and collaborators for making something this easy to use.

websitesca commented 6 years ago

Another solution I found was to handle the http.Server event checkContinue. This, according to the docs, prevents the request event from being emitted so that you don't get this dual stream thing happening where both write headers. This had me really confused, my server would just die on ERR_HTTP_HEADERS_SENT and not respond to any more requests. This seemed to fix the problem. I think it would be helpful if there was either some documentation about this or if somehow node-http-proxy knew about the Expect:100-continue case.

var server = this.server = httpServerModule.createServer();
server.on('request', function (req, res) {
  requestHandler(req, res, proxy, controller);
})
server.on('checkContinue', function(req, res) {
  res.writeContinue();
});
skehlet commented 6 years ago

I ran into this problem too. The following sample code shows the problem: when the client sends an Expect HTTP header (like curl does with a big enough request (> 1024 chars?)), and you don't stop it from getting passed through to node-http-proxy, you can no longer modify the HTTP headers on proxyReq, because some special handling of HTTP requests with Expect headers in Node core (I think?) has already sent them to the backend:

const express = require('express');
const app = express();
const httpProxy = require('http-proxy');
const proxy = httpProxy.createProxyServer({});

// Invoke me with an 'Expect: 100-continue' HTTP header to test.
// You can call me with `curl` like:
// dd if=/dev/urandom bs=1024 count=1 | base64 > 1024-bytes.txt
// curl -vv http://localhost:4080/search -d @1024-bytes.txt

app.use(function (req, res, next) {
    // The following line is important. If we don't remove any Expect HTTP headers,
    // we'll have problems in the on `proxyReq` handler below.
    delete req.headers.expect;
    proxy.web(req, res, {target : 'http://www.google.com'}, next);
});

proxy.on('proxyReq', function (proxyReq) {
    try {
        // The following line errors with:
        // Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client
        // unless you remove any `expect` HTTP headers above, before handing to http-proxy
        proxyReq.setHeader('X-Special-Proxy-Header', 'foobar');
    } catch (err) {
        console.log('ERROR:', err);
        proxyReq.abort();
    }
});

const server = app.listen(4080, function () {
    const port = server.address().port;
    console.log('node-http-proxy/issues/760 demo server listening on port', port);
});

That delete req.headers.expect; line is important. I don't know whose responsibility it should be to deal with this, but the above is able to workaround this problem for now.

As a secondary note, since we're losing out on the purpose of the Expect header (having the server confirm it will accept data of the given content-length), you should probably make sure your proxy's body-parser's limit matches your proxied server's limit, so it can reject too-large requests.

mlegenhausen commented 6 years ago

@skehlet thanks for your work around.

If you don't want to use a wrapper service you can use the 'start' event to remove the expect header before proxyReq handles the request.

proxy.on('start', (req) => delete req.headers.expect);