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

Changing headers should be an option #34

Closed
VanCoding opened this issue Apr 19, 2011 · 37 comments
Closed

Changing headers should be an option #34

VanCoding opened this issue Apr 19, 2011 · 37 comments

Comments

@VanCoding
Copy link

changing the headers "location" & "origin" when proxying websocket request should be an option, because this is important to make the proxying behave like "not happened".

@Marak
Copy link
Contributor

Marak commented Apr 19, 2011

What happens if you modify request.headers directly before they get proxied out?

@VanCoding
Copy link
Author

I think you understood me wrong.
I mean the code in the file "node-http-proxy.js" at line 599 and 600. The headers are going to be changed there. It doesn´t matter what headers are incoming. They are changed. That´s the problem.

It would be better if i could specify if I want to keep the original headers or change them.

@indexzero
Copy link
Contributor

Here? https://github.com/nodejitsu/node-http-proxy/blob/master/lib/node-http-proxy.js#L589

I'm not sure if this is by design due to the websocket protocol or not. I will reread that part of the spec and follow-up on this.

@Marak
Copy link
Contributor

Marak commented Apr 22, 2011

@indexzero - I think I'm running into a related issue here without websockets.

I think the disconnect is in the fact that http-proxy acts as a proxy server by default. This means that the receiving server can tell the request has been proxied. In other words, it's not transparent. The proxy should act in this mode by default, but we should probably expose an API option to enable "transparent" proxying. I'm not sure if there is a better name for that.

Here is a more clear illustration of the problem:

 Start a proxy server on local host that points to blog.nodejitsu.com

 A request comes into [ proxy server ] for: http://localhost

 [ proxy server ] proxies this request to [ blog.nodejitsu.com ]

 [ blog.nodejitsu.com ] receives the request and attempts to route the request based on headers.host

 The headers.host files reads "localhost", [ blog.nodejitsu.com ] fails to respond with the correct request

Does that make any sense?

@andyichr
Copy link

I ran into an issue with this recently. In my client (Google Chrome 10), I was getting this while attempting to make a connection to a websocket server behind node-http-proxy:

Error during WebSocket handshake: origin mismatch: http://127.0.0.1:8071 != http://127.0.0.1

It turns out this was due to the issue mentioned here of changing the request headers. After this patch:

--- /tmp/orig
+++ lib/node-http-proxy.js
@@ -586,8 +586,8 @@
remoteHost = options.host + (options.port - 80 === 0 ? '' : ':' + options.port);

// Change headers

  • req.headers.host = remoteHost;
  • req.headers.origin = 'http://' + options.host;
  • //req.headers.host = remoteHost;
  • //req.headers.origin = 'http://' + options.host;

outgoing = {
host: options.host,

the error stops and the websocket connections work fine.

@VanCoding
Copy link
Author

This is why I posted this issue. I had the same problem.

@indexzero
Copy link
Contributor

The websocket support was originally written by @donnerjack13589 and later updated by @davglass. Maybe they can chime in on the reasoning behind changing the outgoing headers.

I will kill those lines in v0.5.1 (coming this week) if there are no objections since both @flashfan and @andyichr have seen this fix their problems.

@jfis
Copy link

jfis commented May 5, 2011

If you're going to kill those lines, please have a look a little further down. There is code to replace the host and origin which wouldn't need to happen. Also, this section currently does not support ports other than 80.

@frank06
Copy link

frank06 commented May 11, 2011

Updates on this? I'm getting an origin mismatch when proxying https => http

@ghost ghost assigned olauzon May 11, 2011
@Marak
Copy link
Contributor

Marak commented May 11, 2011

@olauzon can you take a look at this ticket? I'd like to get this resolved this week if possible.

@olauzon
Copy link
Contributor

olauzon commented May 11, 2011

@Marak definitely, will investigate.

@Marak
Copy link
Contributor

Marak commented May 11, 2011

Thanks @olauzon! Feel free to post your status in this thread.

@indexzero
Copy link
Contributor

Great! Should be a pretty simple fix in there. Maybe we can summon @miksago or @donnerjack13589 to help shed some light on the underlying websocket spec and why these headers need to be changed (or don't)

@donnerjack13589 wrote the original implementation here, it's like 4am in Russia tho so he might not be around

@ThisIsMissEm
Copy link

So what's the issue? Most websocket implementations will be reading the headers of origin and host, as well as the various sec- ones. If you're proxying websockets and adding, say, a "x-" to the front of these headers, then the websocket implementations will break. I think it's probably best not to modify headers on websocket requests.

As for the specification, I don't think it defines as to how proxies should interact with headers, @donnerjack13589 may know otherwise. I might also ask the guys I work with as to whether they can shed light, but they're in BST / GMT timezone.

@indexzero
Copy link
Contributor

@miksago Thanks. The problem is that currently node-http-proxy rewrites the origin and host headers for WebSocket requests. This is causing some unexpected behavior for users over HTTPS and in other cross-domain proxying scenarios.

Based on what you said, I guess we should just not be rewriting these headers at all?
Code: https://github.com/nodejitsu/node-http-proxy/blob/master/lib/node-http-proxy.js#L589

@VanCoding
Copy link
Author

I still think it would be best, if ther was an option.
In some cases, it is necessary to rewrite them.

For example when someone tries to forward wss:// to an internal ws:// server. Then we would have an origin mismatch.

@jfis
Copy link

jfis commented May 13, 2011

how would wss to ws work?
is that common for a proxy to handle all the encoding/decoding?

that isn't to say changing headers is never necessary

the code to change headers and replace host and origin in the handshake is very deliberate
the coder(s) must have thought it was necessary
would be nice to hear why

code i'm referring to:

// Change headers
req.headers.host   = remoteHost;
req.headers.origin = 'http://' + options.host;

...

request.socket.on('data', handshake = function(data) {
  // Handshaking

  // Ok, kind of harmfull part of code
  // Socket.IO is sending hash at the end of handshake
  // If protocol = 76
  // But we need to replace 'host' and 'origin' in response
  // So we split data to printable data and to non-printable
  // (Non-printable will come after double-CRLF)
  var sdata = data.toString();

  // Get Printable
  sdata = sdata.substr(0, sdata.search(CRLF + CRLF));

  // Get Non-Printable
  data = data.slice(Buffer.byteLength(sdata), data.length);

  // Replace host and origin
  sdata = sdata.replace(remoteHost, options.host)
               .replace(remoteHost, options.host);

@indexzero
Copy link
Contributor

@jfis Again, this is a symptom of a larger problem. I am working on something in the experimental branch, but I'm not sure if we can actually write a websocket proxy on top of the existing http.Agent APIs without event emitter leaks.

@jfis
Copy link

jfis commented May 13, 2011

@indexzero i don't see how changing the headers and replacing host and origin in the handshake relates to http.Agent but I'll take your word for it.

I do see the problem with http.Agent in the other thread (#50) though.

@indexzero
Copy link
Contributor

@jfis They are only related in that they both have to do with websockets. If this gets fixed, that is still a problem and websockets is still broken.

@jfis
Copy link

jfis commented May 13, 2011

@indexzero :) ok gotcha.

@indexzero
Copy link
Contributor

This is fixed in 9c6c4b9. WebSockets (ws://) and Secure WebSockets (wss://) should be 100% now. Added new functional tests, but if anyone wants to do a benchmark that would be awesome.

@frank06
Copy link

frank06 commented May 21, 2011

just double-checking: it's currently not possible to proxy wss => ws and ws => wss, correct?

could we add this to the docs?

@indexzero
Copy link
Contributor

This is actually possible. See the docs under "Proxying HTTPS to HTTP" ... this also works for websockets

@frank06
Copy link

frank06 commented May 22, 2011

In a wss => ws scenario. In order for the non-secure websocket backend to accept the origin, we'd need to declare ws as the protocol. Failing to do so results in an "Origin mismatch" (that's what I get, wss != ws)

https://github.com/nodejitsu/node-http-proxy/blob/master/lib/node-http-proxy.js#L632

All possible values for the protocol in req.headers.origin become: http or https (when changeOrigin == true), or wss (passed along from the secure proxy server, when changeOrigin == false). Never ws. Or am I missing something?

@frank06
Copy link

frank06 commented May 22, 2011

Hmm maybe not Origin, but location. This is what I get in the console: Error during WebSocket handshake: location mismatch: wss://localhost/socket.io/websocket != ws://localhost/socket.io/websocket

@jfis
Copy link

jfis commented May 22, 2011

also, as noted somewhere above,
remoteHost is replaced by options.host during the handshake
options.host does not have a port
remoteHost includes the port for ports != 80
https://github.com/nodejitsu/node-http-proxy/blob/master/lib/node-http-proxy.js#L710

@indexzero indexzero reopened this May 23, 2011
@indexzero
Copy link
Contributor

@frank06 Can you provide sample code to reproduce the problem?

@indexzero
Copy link
Contributor

@frank06 @jfis This is fixed in 028d204 and published in v0.5.9. Let me know if you run into any issues with this.

@jfis
Copy link

jfis commented May 23, 2011

  1. websocket tests point to home.devjitsu.com :)

  2. The section I mentioned before, the handshake, has 2 issues.

if (typeof reverseProxy.socket !== 'undefined') {
reverseProxy.socket.on('data', function handshake (data) {
  //
  // Ok, kind of harmfull part of code. Socket.IO sends a hash
  // at the end of handshake if protocol === 76, but we need
  // to replace 'host' and 'origin' in response so we split
  // data to printable data and to non-printable. (Non-printable
  // will come after double-CRLF).
  //
  var sdata = data.toString();

  // Get the Printable data
  sdata = sdata.substr(0, sdata.search(CRLF + CRLF));

  // Get the Non-Printable data
  data = data.slice(Buffer.byteLength(sdata), data.length);

  //
  // Replace the host and origin headers in the Printable data
  //
  sdata = sdata.replace(remoteHost, options.host)
               .replace(remoteHost, options.host);

It tries to replace remoteHost with options.host during the handshake.
remoteHost contains the port but options.host does not.

  var remoteHost   = options.host + (options.port - 80 === 0 ? '' : ':' + options.port);

This replacement only needs to happen when the newly introduced changeOrigin parameter is true.
Also, since options.host does not contain the port, an origin mismatch will occur for ports != 80.

fail conditions:

  1. changeOrigin == true + options.port != 80

wasteful conditions:

  1. changeOrigin == false //remoteHost is never found in sdata so the 4 lines in question are a waste.

to demonstrate
run this gist:
https://gist.github.com/986377
then open in chrome:
http://otherlocalhost:8000
http://otherlocalhost:8001

@indexzero indexzero reopened this May 23, 2011
@frank06
Copy link

frank06 commented May 23, 2011

0.5.9 does seem to work for me in localhost! I want to fully test it tomorrow. Traffic is being smoothly proxyed from https/443 to http/4431 (including a successful websocket handshake)

@indexzero
Copy link
Contributor

@olauzon ... Any progress on this issue today? Ping me if you're blocked.

@ghost ghost assigned indutny May 30, 2011
@Marak
Copy link
Contributor

Marak commented Jun 8, 2011

Ping. Is this still an issue?

@indexzero
Copy link
Contributor

Yes. @olauzon do you want to take a look?

@lezhangxyz
Copy link

I wanted to tack on this issue I had that's possibly related:
http://stackoverflow.com/questions/6444280/heroku-no-such-app-error-with-node-js-node-http-proxy-module

The host wasn't being modified by http-proxy, the repercussion being that Heroku couldn't redirect you to the correct app. I had to set the req.headers.host = appHost manually to get it to correctly route. I'm running http-proxy 0.5.10.

Would be useful to note in the readme that transparent proxies aren't enabled by default.

@lezhangxyz
Copy link

I looked into the source a little and there -is- an option to change the host header by setting the option changeOrigin: true when you create the proxy server. However, I tried setting it during instantiation and there doesn't seem to be any effect.

var proxy = new httpProxy.HttpProxy({changeOrigin: true});

This could definitely be an error somewhere on my part though.

@indexzero
Copy link
Contributor

@lezhang If that code sample fixed your issue, this is not related to node-http-proxy, but how you were using it. That is, the fix was to manually set the header on the incoming request (i.e. the IncomingRequest was missing the Host header to begin with, and thennode-http-proxy correctly proxied the request).

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

No branches or pull requests

10 participants