BUG Fix cors breaking if referer header is present #132
Conversation
| } | ||
|
|
||
| // Check referer | ||
| $referer = $request->getHeader('Referer'); |
There was a problem hiding this comment.
I would say this should be a IE/Edge only fallback, not a "fallback because whenever"
There was a problem hiding this comment.
Why would it be? Firefox also has this issue in some conditions, and none of the other solutions I've seen around the web target IE.
E.g. juxt/yada#195
Referer isn't less trusted than Origin.
There was a problem hiding this comment.
Also, user-agent targeting is bad practice. Read up https://webaim.org/blog/user-agent-string-history/ :D
There was a problem hiding this comment.
I'm well aware user-agent targeting is bad :)
I wasn't aware that Firefox showed the same behaviour sometimes. It makes sense to fully fall back in that case. it just feels a bit like a security issue, because it opens up the whole reason the CORS header was added.
There was a problem hiding this comment.
Yes, older firefox wouldn't send origin on HTTP POST. My feeling is that if we errored on missing Origin when we had a valid Referer would be a bug on our part.
There was a problem hiding this comment.
Fair enough, as the change is a "hack" that breaks a part of security anyway. It just makes me uncomfortable ;)
There was a problem hiding this comment.
I stand with that it is a potential security issue, but also understand it's the only feasible solution at this moment.
There was a problem hiding this comment.
Fair enough, as the change is a "hack" that breaks a part of security anyway. It just makes me uncomfortable ;)
What part is broken?
There was a problem hiding this comment.
Basically, the way CORS is supposed to work. I'm not saying the code itself is broken :)
There was a problem hiding this comment.
The origin header is just harder to spoof on browsers than the referrer header, while neither are enforced when using something like curl
https://stackoverflow.com/q/21058183/695842
Two of the top answers resonate the same thing: don't rely on CORS for security
Remember: CORS is not security. Do not rely on CORS to secure your site. If you are serving protected data, use cookies or OAuth tokens or something other than the Origin header to secure that data. The Access-Control-Allow-Origin header in CORS only dictates which origins should be allowed to make cross-origin requests. Don't rely on it for anything more.
| { | ||
| $response = HTTPResponse::create(); | ||
| $corsConfig = Config::inst()->get(self::class, 'cors'); | ||
| if ($corsConfig['Enabled']) { |
There was a problem hiding this comment.
this changes the workflow of the CORS headers...
from the original ticket which raised this concern #66 (comment)
Also adds a check for the pre-flight OPTIONS request, and if CORS is enabled, adds the appropriate headers to the response object - both for the OPTIONS response and for the actual data response.
There was a problem hiding this comment.
It's the same workflow; In which situation is it different?
There was a problem hiding this comment.
ok, I needed to double check this :)
I overlooked the second === 'OPTIONS' that was removed above
There was a problem hiding this comment.
Yeah the original code was hard to follow, hence the refactor. :)
| * @config | ||
| * @var array | ||
| */ | ||
| private static $fixed_types = []; |
There was a problem hiding this comment.
Oh right, it was an error where it was searching for an undefined config. Probably not related to this PR and can be removed.
There was a problem hiding this comment.
I've reverted this part.
There was a problem hiding this comment.
oh ok, I had a quick search for it just now, seems harmless to be in here and could've been left in :)
BUG Prevent un-extendable config by shifting defaults into PHP BUG Fix tests not checking cors port ENHANCEMENT Clean up Controller::index() method and make lovely Fixes silverstripe#118
tractorcow
force-pushed
the
pulls/1.0/fix-config-controller-imports-cors-ie-gross-code
branch
from
dabde7e to
a62dabc
Compare
|
I've shifted some of the namespace cleanup into a separate commit. |
flamerohr
deleted the
pulls/1.0/fix-config-controller-imports-cors-ie-gross-code
branch
…s-a-wrap Set user themes on TinyMCEConfig before overwriting with admin themes
BUG Prevent un-extendable config by shifting defaults into PHP
BUG Remove dependency on Doctrine module breaking with --prefer-dist
BUG Fix tests not checking cors port
ENHANCEMENT Clean up Controller::index() method and make lovely
ENHANCEMENT Optimise all imports
Fixes #118