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

Rewrite host for proxied requests #83

Closed
wants to merge 1 commit into from

Conversation

robmadole
Copy link

@robmadole robmadole commented Jun 1, 2018

A request to curl -v https://fly.io will set the Host header to fly.io.

When a proxy rewrites the URL from https://endpoint.com to https://origin.com it should also rewrite the Host header from endpoint.com to origin.com if the header is set to begin with.

For sites that use virtual hosts (checking the "Host" header) this will be necessary.

@@ -3,12 +3,22 @@ import { expect } from 'chai'
import * as proxy from '@fly/proxy'

const origin = "https://fly.io/proxy/"
const req = new Request("https://wat.com/path/to/thing", { headers: { "host": "wut.com" } })
const req = new Request("https://wat.com/path/to/thing", { headers: { "host": "wat.com" } })
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This threw me for a loop at first. But I think the host value here is incorrect for a typical request.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That wat/wut distinction is probably too silly. It was meant to test that the breq host header matched the one defined in the header options, not the hostname from the URL.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the silliness doesn't bother me ☺️

It was more the "this is kinda atypical. Unless I'm doing some curl hackery how would a request be like this?"

})

it('includes explicit host header if different than origin', () => {
const breq = proxy.buildProxyRequest(origin, { headers: { "host": "wut.com" } }, req)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure that if an explicit host is given that it takes precedence over a naive rewrite.

@mrkurt
Copy link
Member

mrkurt commented Jun 1, 2018

Host headers need a ton more docs, but I think this change is breaking some of the things people want to do with the proxy feature. The proxy builder does a couple of different things with headers:

  1. It passes any headers visitors send (the default)
  2. It lets you specify hard coded header values
  3. It lets you strip headers entirely

Lots of apps have a bunch of hostnames, but one shared origin. So the default is to just copy the host header from the initial request:

// sends all traffic to an Amazon ELB, `Host` header passes through from visitor request
const origin = proxy("https://elb1298.amazonaws.com")

Then what you're trying to do — always send a known host header to origins that route with it. You can see a more complete example of this pattern in our Glitch example app:

// sends all traffic to an Amazon ELB, always sends "example.com" as the host header
const origin = proxy("https://elb1298.amazonaws.com", { headers: { host: "example.com"}})

And then way more rare, no host header at all. Usually you'd strip out x-forwarded-host, since some origins don't like that:

// sends all traffic to an Amazon ELB, never sends a host header
const origin = proxy("https://elb1298.amazonaws.com", { headers: { host: false}})

Am I understanding what you're needing to do correctly? I'm definitely going to copy this response into our docs. 😁

@robmadole
Copy link
Author

Perhaps it's the fact that the way this behaves doesn't work as I would expect by default.

Slightly modified example:

const origin = proxy("http://httpbin.org")
// I would expect this to send Host: httpbin.org but it ain't

A super simple app example:

import proxy from '@fly/proxy'

fly.http.respondWith(async (req) => {
  const resp = await proxy('http://httpbin.org')(req)
  // This will always 404 Not Found

  return resp
})

It's this one ☝️that threw me for quite a loop. Because I make a request to https://example.com which is proxied to httpbin.org. It sends Host: example.com and since this is a Heroku app the Host header is used to route.

I would expect the proxy to rewrite the Host header if it's going to rewrite the URL. Maybe this is a bad assumption. Anyway, I killed a good chunk of time on this before I finally took a close look at the request headers.


These work now as I would expect 👇 :

const origin = proxy("https://elb1298.amazonaws.com", { headers: { host: "example.com"}})
// I would expect this to always send Host: example.com

and

const origin = proxy("https://elb1298.amazonaws.com", { headers: { host: false}})
// This would never send a Host header

@mrkurt
Copy link
Member

mrkurt commented Jun 1, 2018

There is a reasonable argument for setting the host header to what's in the origin URL by default, and then giving another option for passing host header through from the visitor request. My hangup with that is it's the only header that would behave that way, but it's also the only header that we can infer from a URL.

Let me think about this some more.

@robmadole
Copy link
Author

No worries. Do with it what you like. I can work around it by being explicit.

@OKNoah
Copy link

OKNoah commented Jun 3, 2018

Shamefully, it took me some time to figure out I needed to change the Host header value to not get a 404. Folks can easily write their own proxy, the point of a helper function like @fly/proxy should be to take care of that, I think.

Fly.io draws in Javascript devs. But folks can use Express, Koa or React-Router for years and not be HTTP experts, or even familiar with fetch, Request, Response, API's, etc.

@robmadole
Copy link
Author

Yeah it took me a while too to figure it out.

@mrkurt mrkurt closed this in 306ea2d Jun 4, 2018
@mrkurt
Copy link
Member

mrkurt commented Jun 4, 2018

Ok, I went looking at nginx and other proxy configs, and it seems like what you were thinking is a good default. I added an option to forward the end user's host header as well.

@robmadole
Copy link
Author

Thanks, @mrkurt !

@mrkurt
Copy link
Member

mrkurt commented Jun 5, 2018

Now available if you're using 0.33.2 (this doesn't need any changes on the server side, so you can use it now).

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

Successfully merging this pull request may close these issues.

3 participants