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

Use X-Forwarded-Proto header as FastbootRequest protocol #161

Open
ryanto opened this issue Jul 22, 2017 · 7 comments
Open

Use X-Forwarded-Proto header as FastbootRequest protocol #161

ryanto opened this issue Jul 22, 2017 · 7 comments

Comments

@ryanto
Copy link
Contributor

ryanto commented Jul 22, 2017

I'm not sure if this is the right repo to open this issue, please let me know if it should be moved.

Currently the fastboot.request.protocol is used when determining the ember data host to use for ED requests made from fastboot. See: https://github.com/ember-fastboot/ember-cli-fastboot/blob/5ecfe8e90ff07f2318eccecedce19c083b58bd21/fastboot/initializers/ajax.js#L9

It's common for load balancers to do SSL termination before sending a request to the fastboot app server. When this happens the protocol would be HTTP, but the LB would also add an X-Forwarded-Proto header to the request.

This creates a problem because the fastboot-app-server thinks the protocol is HTTP, so when the fastboot run Ember app runs an Ember data query it will request data from http://${host}, which will most likely 301 to https://${host}. Since Ember data is running in fastboot/node, it will not follow redirects, and this leads to Ember data ending up with an empty request that throws an error.

One suggestion is to run the fastboot app server using only HTTPS and not have the LB terminate SSL. While this will fix the problem, not all of us have control over our load balancers. Currently, heroku does SSL termination before forwarding the request, so if you're running fastboot-app-server on heroku you'll always end up with an HTTP protocol + X-Forwarded-Proto: https header.

My suggestion would be to have fastboot.request.protocol check for an X-Forwarded-Proto header in the request and use that. I think this would lead to less surprises.

cc @kratiahuja @krasnoukhov since I know y'all were discussing this earlier today.

@ryanto ryanto changed the title Use X-Forwarded-Proto http header in FastbootRequest protocol Use X-Forwarded-Prot header as FastbootRequest protocol Jul 22, 2017
@ryanto ryanto changed the title Use X-Forwarded-Prot header as FastbootRequest protocol Use X-Forwarded-Proto header as FastbootRequest protocol Jul 22, 2017
@kratiahuja
Copy link
Contributor

@ryanto As discussed in ember-fastboot/ember-cli-fastboot#454 (comment) , you can have fastboot-app-server run under https via defining a beforeMiddleware hook. As mentioned, I am not convinced that this needs to be a requirement in fastboot or fastboot-app-server but something that consumers can easily configure.

@ryanto
Copy link
Contributor Author

ryanto commented Jul 22, 2017

The fix in that issue won't work if fastboot.request.host doesn't allow HTTP traffic.

For example, imagine this sort of request...

Browser -> HTTPS -> LB -> HTTP -> Fastboot app server -> Ember data running in fastboot now does a GET http://$host/posts, which 301s to https://$host/posts and causes ED to throw an exception.

Another way to put it... based on current code Ember data will use HTTPS requests in the browser, but will use HTTP requests in fastboot if SSL termination is done before the fastboot-app-server. I think this is a not so obvious gotcha that will bite folks.

It also makes "deploying fastboot to heroku" require some hacks. I think a good goal is to make the heroku deploy story as easy as possible.

I guess my main argument would boil down to: If fastboot is going to use the request to determine how ember data behaves at runtime, and that request could pass through a load balancer, then fastboot should respect common load balancer header modifications.

@ryanto
Copy link
Contributor Author

ryanto commented Jul 22, 2017

Btw, here's what we're currently doing to code around this issue.

// app/adapters/application.js

import DS from 'ember-data';
import Ember from 'ember';
import config from 'my-app/config/environment';

export default DS.JSONAPIAdapter.extend({
  fastboot: Ember.inject.service(),

  host: Ember.computed('fastboot.isFastBoot', 'fastboot.request.host', function() {
    let fastboot = this.get('fastboot');

    if (fastboot.get('isFastBoot') && config.environment === 'production') {
      let host = fastboot.get('request.host');
      // Force any production fastboot request made by Ember data to use HTTPS
      return `https://${host}`;
    }
  }),
});

@sivakumar-kailasam
Copy link

We had a few caveats similar to this hosting the new ember api docs on heroku with fastboot. Since the ember website proxies /api/ to our app on heroku we had trouble because fastboot doesn't honor the X-Forwarded-Host as well. We're handling the protocol & host via a custom beforeMiddleware in fastboot-app-server.

@kratiahuja may be fastboot can use the X-Forwarded-Proto and X-Forwarded-Host header values when users opt-in via the fastboot config on an ember app?

@lucacorti
Copy link

@sivakumar-kailasam Can you share your workaround for getting FastBoot to work with x-forwarded-host? Deploying a FastBoot app behind a reverse proxy, which I guess is what an heroku setup looks like, looks broken ATM.

@sivakumar-kailasam
Copy link

@lucacorti check out https://github.com/ember-learn/ember-api-docs/blob/master/bin/ember-fastboot#L41. The main ember website is running on nginx and we proxy requests for the API docs to an ember app running on heroku. It has some caveats but it addresses most important usecases for us.

@lucacorti
Copy link

lucacorti commented Feb 5, 2018

@sivakumar-kailasam Thanks, I ended up basically rewriting the host header in a similar way to support deployment behind a reverse proxy setting X-Forwarded-Host. I also opened an issue (#184) for this because at least the standard proxying headers should be handled by simple configuration. This is a very common, if not the most common, deployment scenario.

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

5 participants