Added support for trusting proxies w/ HTTPS #535
Conversation
…g proxies, and used app.enable('trust proxy') to let Express parse the headers
|
By analyzing the blame information on this pull request, we identified @drew-gross, @flovilmart and @Zertz to be potential reviewers. |
|
@Nickster28 updated the pull request - view changes |
ghost
added
the
Nickster28
changed the title
| @@ -48,11 +48,16 @@ function checkIfIconsExistForApps(apps, iconsFolder) { | |||
| } | |||
| } | |||
|
|
|||
| module.exports = function(config, allowInsecureHTTP) { | |||
| module.exports = function(config, allowInsecureHTTP, trustProxy) { | |||
There was a problem hiding this comment.
Instead of adding a new parameter here, can you make this part of the config parameter?
| @@ -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'); | |||
There was a problem hiding this comment.
Can you have the dashboard throw an error if both allowInsecureHTTP and trustProxy are set? We want to nudge people away from using allowInsecureHTTP.
| @@ -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: | |||
There was a problem hiding this comment.
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.
|
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. |
|
Very good! |
…ect and de-emphasize allowInsecureHTTP in favor of trustProxy
|
@Nickster28 updated the pull request - view changes |
* master: Added support for trusting proxies w/ HTTPS (parse-community#535) Fix calendar month/current selection and clicking (parse-community#555) Fix text in CONTRIBUTING.md (parse-community#533) Adds an "ends with" filter for string queries (parse-community#540) Fix documentation link and open link in new browser tab. (parse-community#534)
(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.