Relax origin header checking and add more CORS support

[jaswilli]

• Do not send CORS headers for origins that are not whitelisted but otherwise allow the response to proceed normally. This enforces CORS for the browser but does not blow up non-CORS requests.

Very much open to discussion. See #6642 (comment)

[sebgie]

I have looked at both PRs and I really like having a separate CORS module. Using the express CORS module makes a lot of sense and adding pre-flight will be useful in the future (currently we only allow non-pre-flight requests). Big 👍 for separating concerns!

The PR unfortunately disables all arrangements we had in place for disallowing requests from (unwanted) clients? I don't know if this is intentional or an oversight? Similar to Googles Maps API we wanted to only allow requests from certain origins. Removing these checks will allow to use client side JS to fetch content from any blog?

Non CORS requests don't carry an origin header and are allowed by default.

[jaswilli]

Removing these checks will allow to use client side JS to fetch content from any blog?

If I'm understanding what you're asking the answer is no, because if the client-side JS is coming from a non-trusted domain the response won't have an Access-Control-Allow-Origin header so it won't be allowed.

[jaswilli]

Here's a good way to test the behavior:

• Checkout More CORS support #6646
• Run ghost locally
• Go to a third-party web site (I used imgur.com because it has jquery and is non-https)
• Open the dev console and run $.getJSON('http://127.0.0.1:2368/ghost/api/v0.1/tags?limit=all&client_id=ghost-frontend&client_secret=[your secret]')
• See that the request failed for not having CORS headers

(You can also test that the client_trusted_domain works by adding http://imgur.com into the database if you feel like it.)

[sebgie]

You are right, that's what I love about Jason PRs :-D.

I tried to recollect my memories if there was another reason for doing that but to no avail. It might had to do with trying to be smarter than the standards which is always a bad thing.

Anyway, 👍 for merging! Would you like to combine the 2 PRs and/or put closing or refs in the commit message?

[jaswilli]

I think two distinct commits are beneficial because both changesets are working and it shows the evolution of the behavior.

I'll "ref" this PR from #6646 unless someone else feels strongly about squashing both of these together.

[jaswilli]

Actually, scratch that. Amending this commit was a mistake since I had this rebased onto #6646.

New plan: I'm going to close #6646 and cherry-pick its commit onto this PR. A single PR with the two commits is probably less confusing history anyway.

[jaswilli]

@sebgie Ok, this is all sorted out, rebased and ready to go.

[sebgie]

Sorry it took me so long to get back to this issue.

I have tested it and it works great 👍. There is one thing left I noticed while testing. Previously it was possible to only add the hostname (without schema) to client_trusted_domains. In your example imgur.com. After these changes the full url with schema http://imgur.com is needed.

Is there a reason for doing this? It would break the installations of people who are currently using the API form another domain.

[jaswilli]

It's because the scheme is part of the origin. When allowing cross-origin requests the ability to say https://example.com/ is ok but http://example.com/ is not is necessary.

If we want to make it the default to create a trusted domain for both http and https we should probably handle that in the UI (and still allow people to specify one or the other).

[sebgie]

I see ...

But this is still going to break every existing API integration. Can we remove that for now and come up with a plan to do a migration for existing users after the release? I would really like to get this into 0.7.9?

Would it make sense to add a HTTP and HTTPS field to the trusted domain table to and not store two lines per domain by default? Or can we think about it like our approach to SSL on the frontend and support HTTP + HTTPS by default and have a forceSSL column?

[jaswilli]

But this is still going to break every existing API integration. Can we remove that for now and come up with a plan to do a migration for existing users after the release? I would really like to get this into 0.7.9?

Since this is a beta feature that exists only in labs and requires manually inserting records into the database, I think I'd be okay with noting the change in the release notes and updating the documentation.

Would it make sense to add a HTTP and HTTPS field to the trusted domain table to and not store two lines per domain by default?

I think simple is going to be better (i.e., one row per origin without any fancy http vs. https antics). Anything we can do to make the origin checking code simple is going to pay off long-term in regards to bugs and tears shed dealing with them.

@ErisDS thoughts?

[ErisDS]

This is a beta feature. If we agree that the current implementation is broken, and that this is the correct way to fix it, then I am in favour of just fixing it with release notes.

My only concern is that there's no easy transition here. E.g. Upgrading Ghost & changing the trusted domain value have to be done independently, so there is always going to be a point (even if it's seconds) where it is broken. If there was a solution with a better transition path that would be interesting to me.

Again though, if we're all happy that this is the right solution, then I say rip this plaster off. I can manually upgrade each user that has this on Ghost(Pro) and update their trusted domain records within seconds (it's not many) and each one of them has had explicit warning that this can break at any time.

[jaswilli]

If there was a solution with a better transition path that would be interesting to me.

I'm definitely not against doing a migration (especially if it's just a one-off like the client secret fix)--just that I don't think we're obligated to jump through any hoops in this situation.

Again though, if we're all happy that this is the right solution, then I say rip this plaster off

I think including the scheme as part of the origin is correct (but willing to discuss if there's dissent). Assuming there's consensus there, I don't know if we've settled on how to handle it.

I'm in favor of doing a simple "one row per origin" solution so we can minimize the drama surrounding figuring out what's what, parsing URLs, and doing partial matches, etc., etc. But that's clearly just my opinion and I'm not sure @sebgie is on board with that.

[ErisDS]

I definitely do not want to do a migration for this. No way.

Buut, would a "transitionable fix" be to assume http for one version? So if the scheme is missing, bolt http on. Tell everyone they have to update their record for 0.7.9 and then do the break in 0.8.0.

I literally just had this idea and I'm not even sure that I like it. Too much effort for something that is meant to break.

[jaswilli]

Buut, would a "transitionable fix" be to assume http for one version? So if the scheme is missing, bolt http on.

I'm not sure we could assume just http. I think we'd have to change the match logic to match the bare domain (i.e., match both http and https). So we'd grab the actual Origin header, run it through url.parse() to yank out the host and then match it against the trusted_client_domain. But...

I literally just had this idea and I'm not even sure that I like it. Too much effort for something that is meant to break.

Exactly. I'm not even sure we should bother.

[sebgie]

I'm aware that this is a labs feature and there is no obligation on our side to keep things working. Given that we are talking about the Public API which was anticipated for a long time I'm a bit hesitant to break it without discussion.

If we agree that this is okay I'm fine with the PR as it is.

[ErisDS]

Yah I'm ok with it. Anyone one Ghost(Pro) can't fix it themselves, but we can ensure it's done at the right time. Anyone who is self hosting should see the notice in the release notes. I've already added notice here: http://support.ghost.org/public-api-beta/

Worth keeping in mind that at some point we need to move & rename the /shared/ghost-url.js file and get that working better - so we're definitely going to do more breaking yet 😁

[ErisDS]

As of this PR, we no longer have the following rules:

origin === configHostname       
|| configHostname === 'my-ghost-blog.com'       
|| origin === url.parse(config.urlSSL ? config.urlSSL : '').hostname

I assume this was done on purpose, but I'm not 100% clear on why they aren't needed anymore?

[jaswilli]

They aren't needed for CORS purposes because if a request is cross-origin and doesn't match a client_trusted_domain or one of the whitelist entries (localhost, local IPs) then the response won't get an Access-Controll-Allow-Origin header and will be denied.

Basically the only thing it was accomplishing was preventing same-origin requests that didn't match what was defined in config.js (i.e., why ngrok proxying didn't work).

[ErisDS]

Then the most common use case for urlSSL is serving the admin panel over HTTPS using the urlSSL hostname, whilst serving the frontend of your blog over HTTP using the url hostname (I think this feature was added mainly for heroku).

In this case, the API would exist on urlSSL, but AJAX API requests in the theme would be from url. As far as I can see this case isn't handled anymore?

This case is roughly the same as the Ghost(Pro) usecase - where the API exists on a different secure URL if you have a custom domain. The custom domain is what is output as config.url as it's the public facing URL in all cases, except the API. There is one custom piece of logic added to Ghost(Pro) to make this work, which I intend to port back, I just need to figure out if I need an extra custom piece for CORS or if there is a more general solution.

I think if the hostname from config.url (and possibly config.urlSSL to handle every possible case but not sure this is really necessary) is added to the whitelisted domains, then both of these cases start working again (but without regressing on the ngrok request problem).

[jaswilli]

If I understand the scenario(s) you're laying out then, yeah, I think we need to make an adjustment. To make cross-origin requests work where the frontend is at a different origin from the API by way of config.js we'll have to add the config URLs into the whitelist (or come up with an alternate solution).

We should also add a test for this.