Add caching support and remove access control. Fixes #65 #64 #49 #17

[davidbarratt]

I have fixed everything. :)

Despite the number of changes, I don't think this is actually a breaking change (it's either a minor or a patch):

• The way this library is used has not changed
• The cross-origin interaction browsers have has not changed
• Caching is now fixed

Fixes #65 #64 #49 #17

[barryvdh]

How are other libraries handling this? Is this the only one which actively blocks the requests?

[davidbarratt]

How are other libraries handling this? Is this the only one which actively blocks the requests?

I have no idea, but a simple request shouldn't fail, otherwise you break cachability.

The simple request will not be read without the proper CORS headers.

If it's a non-simple request, the browser wont even send the request if it fails the preflight.

[davidbarratt]

I suppose it's possible that someone was using this library to block simple requests, but that's a misuse of CORS (and a misuse of this library, imho). The purpose is to be more permissive, not less

[davidbarratt]

All of the requests that are simple can be preformed with HTML (anchors, forms, etc.) which you cannot prevent because they will be same-origin when executed. A better way to prevent those would be to use the Referer header. I think using this library to attempt to block simple requests, may give the user a false sense of security.

[barryvdh]

Yeah I think I agree, although it could warrant a new semver release.

[davidbarratt]

Yeah I think I agree, although it could warrant a new semver release.

As long as it gets into Drupal before v9, it doesn't matter to me, but maybe @wimleers can help with that. :)

[davidbarratt]

or maybe it can go in a minor release of Drupal... I'm not sure. :/

[barryvdh]

First of all, thanks for the effort. But not sure if having a big PR like this is the best way. CORS is tricky and I want to make sure we don't break anything. I'm gonna look at this, and possibly cherry-pick a few commits, if that's okay with you.

[davidbarratt]

No problem. :)

My problem was that I couldn't remove the the need to Vary the origin (#64) without also removing the access control (#65). Since the access control requires that the response be varried by the origin all the time (#49). Doing this removed the need for more helpful error messages (#17).

I'm not sure how to untangle that, so if have some ideas, go for it. :)

[davidbarratt]

I suppose this could be done in reverse... so in this order:

1. Always set Vary: Origin #49 Always set the Vary (do not add any new config, since the way it is now, this is always needed)
2. Access control is unnecessary #65 Remove the access control, since this isn't necessary in the first place. Also resolves More helpful error response #17
3. Only Vary the request when necessary #64 Add better cacheability (i.e. partially reverse Always set Vary: Origin #49)

Does that sound reasonable?

[barryvdh]

See #70 also, which includes #69

[barryvdh]

Yeah I was thinking about just modifying the existing response, but not sure how normal apps handle OPTIONS requests.

[davidbarratt]

Yeah I was thinking about just modifying the existing response, but not sure how normal apps handle OPTIONS requests.

I don't think they normally do, so I think catching would be correct, but should probably test in some apps.

[davidbarratt]

Yeah I was thinking about just modifying the existing response, but not sure how normal apps handle OPTIONS requests.

We could kick the can on the preflight by first checking if it's an OPTIONS request and then adding Vary: Access-Control-Request-Method to all of those requests. Then we can check to see if it has the header and if it does, takeover the request...

Though I think it would be better to add the heaqers to all OPTIONS requests (if possible).

[barryvdh]

So if this all covered with the latest version now?

[davidbarratt]

So if this all covered with the latest version now?

Looks like it to me! :) Thank you so much!