forceAdminSSL may need to redirect to another domain

[gimelfarb]

This is an enhancement to the current handling, where a 301 redirect is issued to "https://" URL using the same hostname as configured in the ".url" property of the config. This behavior may not always be appropriate.

Take Heroku for example. The SSL endpoint for a custom domain is a premium feature. So for free accounts custom domain (e.g. "www.myblog.com") is available via HTTP only. However, HTTPS is available via the default "https://myblog.herokuapp.com", using Heroku's domain for your app.

Since it is not desirable to access Ghost Admin via HTTP connection, the "http://www.myblog.com/ghost" should either be disallowed or redirect to "https://myblog.herokuapp.com".

Proposed features:

1. Add an ability to specify a redirect to different domain than the one in default .url property for Admin access
2. Add an ability to block access to /ghost folder for non-secure connections altogether (e.g. 401 Unauthorized response)

These are for when forceAdminSSL: true is set.

[gimelfarb]

Implemented in PR #1809 .

[ErisDS]

This issue has gone into a special 0.4.1 milestone for a maintenance release sometime within the next month. Master will continue to be work in progress for the active milestone 0.5. A special 0.4-maintenance branch has been created. The workflow for these fixes is that PRs for 0.4.1 should be made to that branch rather than master and then I will merge it back into master.

Unfortunately, it's not possible to change open PRs to go to a different branch, a brand new PR has to be created & the old one closed.There were already many PRs open for the 0.4.1 issues before the branch was created, and I am not asking or requiring anyone with an already open PR to master to do this - I am able to manually merge the changes into the right branch. However, I will appreciate anyone who has a few minutes to save me the job 👍

[sebgie]

From #1809:

I'll try to structure what I wrote above. Adding a second domain adds some complexity we should think about. At the moment we have two configuration options to force the use of SSL.

• url: https://my-ghost-blog.com

If the scheme of url is https:// all traffic to frontend and admin is forced to use SSL and all requests sent using HTTP will be redirected to HTTPS. If the scheme is http:// and SSL is available HTTP and HTTPS requests are answered.

• forceAdminSSL

The forceAdminSSL switch will force the use of SSL on all admin pages (/ghost/*). If it is set to true all requests sent using HTTP will be redirected to HTTPS. In the answer above I was suggesting to add a separate switch (redirectToSSL: true) to enable/disable the redirect and send a 403 forbidden response if the redirect is disabled. After reading your response I think that it would make sense to change the behavior of forceAdminSSL and have all possible combinations covered with one configuration entry.

• forceAdminSSL: false default, SSL is not forced
• forceAdminSSL: true eq to forceAdminSSL: {redirect: true}
• forceAdminSSL: {redirect: true} SSL is forced and all HTTP traffic is redirected to HTTPS
• forceAdminSSL: {redirect: false} SSL is forced and all HTTP traffic to /ghost/* returns 403 forbidden

Now the tricky part. I would suggest using urlSSL as config name in config.js. I think that it is not necessary to force single pages to SSL? In my opinion it is enough to distinguish between frontend and admin? That would allow the following options and need implementation in Ghost:

• url: https://: all traffic should be HTTPS; set url to the SSL domain
• url: http://: HTTP and HTTPS traffic is allowed; add logic to use either url or urlSSL for frontend requests
• forceAdminSSL: false: add logic to use either url or urlSSL for admin requests
• forceAdminSSL: true: redirect all traffic to urlSSL and add logic to use urlSSL where needed
• forceAdminSSL: {redirect: false}: don't redirect and add logic to use urlSSL where needed

Have I forgotten anything?

[gimelfarb]

@ErisDS @sebgie As I looked into it deeper, I wanted to get your opinion. Implementing urlSSL properly would require quite a bit refactoring. That certainly reduces the reward part of the risk/reward ratio for this enhancement.

Basically, the proper implementation would change all the places where config().url is referenced to use either url or urlSSL, depending on what the current connection type is. That way, once redirected to urlSSL, all the absolute links point to SSL variant of the blog domain.

In order to minimize code changes, one could use [Object.defineProperty'](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/defineProperty) to turn config().urlinto a _getter function_, which would return SSL or non-SSL value depending on the current context. The problem is that there are number of places in the code where theconfig().urlis now cached - i.e.server/config/paths.jsandserver/config/theme.js`. Technically those would have to be refactored as well to use the getter, rather than the cached value.

So to me that started to look like a bigger change than warranted for just forcing Admin portion to SSL. What do you think? Am I totally off here?

I reckon it would suffice to keep this contained to checkSSL() method, and just use urlSSL, if it is defined, to redirect to SSL for /ghost/*. There, as of now, most links are relative and should work fine.

For me personally, the most important case is really just denying access, via forceAdminSSL: {redirect: false}. And automatic redirect would be an icing on the cake, and will work fine for most cases. As of now, I can manage and access Admin panel via the other domain without having had to configure anything in the config.js, which tells me it will work after the redirect without doing any other code modification.

Thoughts?

[hswolff]

The problem is that there are number of places in the code where the config().url is now cached - i.e. server/config/paths.js and server/config/theme.js.

FYI #1858 just got merged in which cleaned up some of those issues. I haven't really investigated thoroughly but just wanted to alert you that some of the pain points may have been mitigated.

[gimelfarb]

@hswolff Perfect timing! 👍 Yeah, looks like the config() part has been cleaned up and improved, I'm reading through the changes to all the details. With these changes, the only place I see now that caches URL is theme.update(settings, configUrl) in server/config/theme.js. This is obviously used by the themes ({{@blog.url}}). We may be able to patch it throughinitViews() in server/middleware/index.js) ...

Ok, so the current idea is to modify createUrl in server/config/url.js to pick either SSL or non-SSL base URL based on whether current request is HTTPS or HTTP. Of course, that means injecting current request context somehow, so that hbs helper functions can see it. I haven't looked at hbs before deeply, so I am not sure yet how that will be achieved ... Anyone wants to pitch in their expert opinion?

P.S. This is a good example where in multi-threaded frameworks thread-local variables can come to the rescue. In Node.js this will have to be more inventive.

[gimelfarb]

I think I figured it out. Just need to test more, and probably add a bunch of unit tests for the new expected flows.

[gimelfarb]

Finally managed to find time to finish this. Creating unit tests took a while, but I have now added support for 'forking' new instance of Ghost during unit tests to be able to test various configuration options (not just single static configuration from the default config.js file).

As I was merging today there were a few conflicts, since code has moved on since I started on this. I believe I have accounted for the new changes, such as /tags/ URLs, and tested locally. Looks like it's working fine, and uses the urlSSL option if connecting over HTTPS.

[sebgie]

@gimelfarb thank you for updating and your endurance :-).

I have tested it locally and it is imo good to merge 👍.

[gimelfarb]

No worries. Glad I was able to contribute!