Added support for trusting proxies w/ HTTPS #535

Merged
merged 7 commits into from Oct 17, 2016

Projects

None yet

4 participants

@Nickster28
Contributor

(Thanks to http://jaketrent.com/post/https-redirect-node-heroku/)
When hosting on Heroku, it turns out the request.secure will always be false, even if the client requests over HTTPS. Instead, Heroku adds an HTTP header 'x-forwarded-proto' specifying the protocol used ('http' or 'https'). For those on Heroku, this additional check will allow the HTTPs check to work. For those not on Heroku (where this header doesn't exist), it won't do anything.

A followup PR to #454.

@facebook-github-bot
Collaborator

By analyzing the blame information on this pull request, we identified @drew-gross, @flovilmart and @Zertz to be potential reviewers.

@ghost
ghost commented Sep 18, 2016

@Nickster28 updated the pull request - view changes

@ghost ghost added the CLA Signed label Sep 18, 2016
@Nickster28 Nickster28 changed the title from Added support for HTTPS + proxies to Added support for trusting proxies w/ HTTPS Sep 18, 2016
@ghost ghost added the CLA Signed label Sep 18, 2016
@drew-gross

This looks better! Just a couple small suggestions.

Parse-Dashboard/app.js
@@ -48,11 +48,16 @@ function checkIfIconsExistForApps(apps, iconsFolder) {
}
}
-module.exports = function(config, allowInsecureHTTP) {
+module.exports = function(config, allowInsecureHTTP, trustProxy) {
@drew-gross
drew-gross Sep 18, 2016 Collaborator

Instead of adding a new parameter here, can you make this part of the config parameter?

@Nickster28
Nickster28 Oct 2, 2016 Contributor

Done.

Parse-Dashboard/index.js
@@ -105,8 +107,8 @@ p.then(config => {
const app = express();
- if (allowInsecureHTTP) app.enable('trust proxy');
- app.use(mountPath, parseDashboard(config.data, allowInsecureHTTP));
+ if (allowInsecureHTTP || trustProxy) app.enable('trust proxy');
@drew-gross
drew-gross Sep 18, 2016 Collaborator

Can you have the dashboard throw an error if both allowInsecureHTTP and trustProxy are set? We want to nudge people away from using allowInsecureHTTP.

@Nickster28
Nickster28 Oct 2, 2016 Contributor

Done.

README.md
@@ -215,6 +215,25 @@ In order to securely deploy the dashboard without leaking your apps master key,
The deployed dashboard detects if you are using a secure connection. If you are deploying the dashboard behind a load balancer or proxy that does early SSL termination, then the app won't be able to detect that the connection is secure. In this case, you can start the dashboard with the `--allowInsecureHTTP=1` option. You will then be responsible for ensureing that your proxy or load balancer only allows HTTPS.
+Alternatively, if you are behind a front-facing proxy and want to rely on the X-Forwarded-* headers for the client's connection and IP address, you can start the dashboard with the `--trustProxy=1` option (or set the PARSE_DASHBOARD_TRUST_PROXY config var to 1). This is useful for hosting on services like Heroku, where you can trust the provided proxy headers. For Heroku in particular, setting this option allows the dashboard to correctly determine whether you're using HTTP or HTTPS. You can also turn on this setting when using the dashboard as [express](https://github.com/expressjs/express) middleware:
@drew-gross
drew-gross Sep 18, 2016 Collaborator

Nice docs! I think this should be default method for most people. Can you make this the "normal" suggestion for what to do if you have a proxy/SSL terminator? And de-emphasize the --allowInsecureHTTP option.

@Nickster28
Nickster28 Oct 2, 2016 Contributor

Done.

@Nickster28
Contributor

Thanks for the feedback Drew! I'll put those changes in as soon as I can. My laptop had to take an unexpected trip to the shop this week ( 😥 ) but I'll try to update the PR this weekend or next week, once I get it back.

@gimdongwoo
Contributor

Very good!
I hope to merge fast.
Thank you @Nickster28

@ghost ghost added the CLA Signed label Sep 25, 2016
@Nickster28 Nickster28 Pull request feedback to combine trustProxy into dashboard config obj…
…ect and de-emphasize allowInsecureHTTP in favor of trustProxy
db8f85b
@facebook-github-bot
Collaborator

@Nickster28 updated the pull request - view changes

@drew-gross drew-gross merged commit dba402d into ParsePlatform:master Oct 17, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Nickster28 Nickster28 deleted the unknown repository branch Oct 17, 2016
@georgeloh georgeloh added a commit to georgeloh/parse-dashboard that referenced this pull request Oct 19, 2016
@georgeloh georgeloh Merge branch 'master' into aws_master
* master:
  Added support for trusting proxies w/ HTTPS (#535)
  Fix calendar month/current selection and clicking (#555)
  Fix text in CONTRIBUTING.md (#533)
  Adds an "ends with" filter for string queries (#540)
  Fix documentation link and open link in new browser tab. (#534)
ba7245a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment