FIX: Don't 403 error when origin is on CORS-disallowed (fixes #519) #527
Conversation
kinglozzer
force-pushed
the
519-cors-verification
branch
from
110394a to
fb6295b
Compare
| $response->addHeader('Access-Control-Allow-Origin', $origin); | ||
| // If specific allowed origins are set, the response headers will vary by request origin, so use the | ||
| // Vary header to tell browsers/CDNs that | ||
| $response->addHeader('Vary', 'Origin'); |
There was a problem hiding this comment.
This is new, based on the advice here: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Origin#cors_and_caching
|
bump |
|
The issue attached to this PR is in our "backlog with PRs" column and will be refined in a future backlog refinement session. We're still pretty busy getting the CMS 5 release ready so we haven't had a chance to look at this properly yet. |
There was a problem hiding this comment.
Looking at the the original issue this was to allow for use case where there is no Origin header being sent.
It seems like you can almost already do that by update the config on the private static $cors['Allow-Origin'] to include a * in there to allow this? validateOrigin() will return true if it encounters a * -- however $origin needs to be non-empty which is what prevents this now
This PR seems like it's completely disabling Origin validation though, based on the discussion in the original issue it looks like it's assumption that doing origin validation at this point is wrong? I'm not so sure it is? Seems like we're better of keeping this existing origin validation.
I think all we really need to do in this PR is instead update validateOrigin() to return true if origin is empty but there's a * in the static config
|
@kinglozzer Let me know if you're going to continue with this PR, otherwise I'll close this one for now |
|
Not likely to have time soon 😔 |
This will no longer output a 403 error if origin is invalid - CORS headers are to protect things client side, not server side, so there’s no security benefit in triggering this error.
The
Access-Control-Allow-Originis now only output if the origin is valid. For valid origins, that matches the current behaviour. For invalid origins, this is a behavioural change (it used to 403) but this new behaviour will correctly trigger preflight errors:One other minor behavioural change here - if developer has opted to allow all origins, this will now explicitly output
Access-Control-Allow-Origin: *instead ofAccess-Control-Allow-Origin: <current origin>Issue