Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IE11 does not add CORS header when using the same domain #118

Closed
1 task
Firesphere opened this issue Sep 12, 2017 · 16 comments
Closed
1 task

IE11 does not add CORS header when using the same domain #118

Firesphere opened this issue Sep 12, 2017 · 16 comments

Comments

@Firesphere
Copy link

@Firesphere Firesphere commented Sep 12, 2017

IE11 and Edge do not add the CORS Origin header, even when explicitly set, when the domain is the same as the domain graphql (asset admin) tries to talk to.

This breaks the CMS when CORS is enabled. Which means the GraphQL API becomes a lot less useful to anyone who wants to serve it to other domains or standalone apps, and still support IE for it's content authors.

Explicitly setting the Origin header in the asset admin, does not resolve the problem, as IE still does not actually add it.

Background is here:
https://connect.microsoft.com/IE/feedback/details/781303/origin-header-is-not-added-to-cors-requests-to-same-domain-but-different-port
Although the issue applies to the same port as well as a different port

PRs:

@Firesphere
Copy link
Author

@Firesphere Firesphere commented Sep 12, 2017

So far, the only working solution I've found is very dirty, it's changing the else in the Controller from line 184 onward to:

else {
                /**
                 * IE 11 and Edge fix. When CORS is enabled but we are on the same domain,
                 * IE refuses to send the Origin header, causing a lot of pain.
                 * Note, when it's an actual CORS request, IE does add the header, so we only end up here
                 * only when it is not an actual CORS request
                 * @link https://connect.microsoft.com/IE/feedback/details/781303/origin-header-is-not-added-to-cors-requests-to-same-domain-but-different-port
                 * This fault also happens when the port is the same!
                 */
                $isIE = strpos($request->getHeader('User-Agent'), 'Trident') !== false;
                $isEdge = strpos($request->getHeader('User-Agent'), 'Edge') !== false;
                if (($isEdge || $isIE) && in_array('*', $allowedOrigins, true)) {
                    $response->addHeader('Access-Control-Allow-Origin', $origin);
                } else {
                    // No Origin header present in Request.
                    return $this->httpError(403, "Access Forbidden");
                }
            }

@chillu
Copy link
Member

@chillu chillu commented Sep 13, 2017

Can reproduce - here's IE11 on Windows 8, with the following YAML on a stock SS4:

SilverStripe\GraphQL\Controller:
  cors:
    Enabled: true
    Allow-Origin: '*'
    Allow-Headers: 'Authorization, Content-Type'
    Allow-Methods:  'GET, POST, OPTIONS'
    Max-Age: 86400

image

Although the issue applies to the same port as well as a different port

That's a big assumption, and not specifically mentioned in the issue or comments there.

@chillu
Copy link
Member

@chillu chillu commented Sep 13, 2017

This Stackoverflow post gives a bit of context on the origin behaviour.

We've ruled out setting the Origin header directly in the Apollo network layer, since that's not supported by browsers for security reasons - see https://developer.mozilla.org/en-US/docs/Web/Security/Same-origin_policy#Changing_origin

I've confirmed that my Browserstack tests is putting the site in the "Internet Zone", rather than a "Trusted Zone" that might prevent the Origin header from being sent.

Looking at the headers, this should be considered a simple request in CORS speak, not requiring a preflight OPTIONS call. We're not setting any funky headers manually here, and it's a POST request

@Firesphere
Copy link
Author

@Firesphere Firesphere commented Sep 14, 2017

That's a big assumption, and not specifically mentioned in the issue or comments there.\

It's not an assumption, it's an observation. I see it happen on my Windows 10 IE11 and Edge

@chillu
Copy link
Member

@chillu chillu commented Sep 14, 2017

Apollo uses isomorphic-fetch as a polyfill for the WHATWG fetch API in IE11. Assuming that JS libraries can't really interfere with user agent stuff like setting Origin headers, I don't think that's a factor - but worth mentioning.

One of the effects here is that removing credentials: 'same-origin' from buildApolloClient.jsdoesn't change anything in IE11 - see https://github.com/github/fetch/pull/56#issuecomment-68835992. In Chrome, it means the fetch defaults toomit`, which doesn't send cookies. IE11 sends cookies regardless. From https://github.com/github/fetch:

If you have trouble making a request to another domain (a different subdomain or port number also constitutes another domain), please familiarize yourself with all the intricacies and limitations of CORS requests. Because CORS requires participation of the server by implementing specific HTTP response headers, it is often nontrivial to set up or debug. CORS is exclusively handled by the browser's internal mechanisms which this polyfill cannot influence.

@chillu
Copy link
Member

@chillu chillu commented Sep 14, 2017

This script executed on the same domain (4.vagrant) with cookies set results in the same behaviour (no Origin header). That's about as basic as it gets. Changing the host to something else does include the Origin header.

function reqListener () {
  console.log(this.responseText);
}

var formData = '...';

var oReq = new XMLHttpRequest();
oReq.addEventListener("load", reqListener);
oReq.open("POST", "http://4.vagrant/graphql");
oReq.setRequestHeader("Content-Type", "application/x-www-form-urlencoded;charset=UTF-8");
oReq.send(formData);

I officially give up on this now :D At first I thought Simon's patch as a security hole (allowing elevated access simply by changing a user agent), but that's not the case - the server has already expressed that every origin can access the resource.

The problem with Simon's patch is if you want to allow same domain and whitelist specific domains. It'll still deny the response, because in_array('*', $allowedOrigins, true) === false. So this effectively makes the CORS settings useless on IE.

SilverStripe\GraphQL\Controller:
  cors:
    Enabled: true
    Allow-Origin:
      - http://same-host.com
      - http://different-host.com
    Allow-Headers: 'Authorization, Content-Type'
    Allow-Methods:  'GET, POST, OPTIONS'
    Max-Age: 86400

@chillu
Copy link
Member

@chillu chillu commented Sep 14, 2017

A flaw in my logic: I was assuming the Origin header is "trusted", since it's set by the user agent and can't be tampered with via JavaScript. That doesn't stop other HTTP clients making requests though, you can just set the Origin header via curl. So in that regard, it's the same level of trust as the Referer header. Which means Allow-Origin: <some-domains> is really only a suggestion relevant for the browser ecosystem, but not all HTTP clients.

I think we should alter Simon's patch to always send Access-Control-Allow-Origin: * to IE if any Allow-Origin is configured in the YAML file. That needs to be documented pretty clearly both inline and in the README. @Firesphere waddya think?

@Firesphere
Copy link
Author

@Firesphere Firesphere commented Sep 14, 2017

I officially give up on this now :D
😂

You've fairly conclusively written out my discoveries over the last couple of days.

What I think, is we could add a referrer check for IE? As the referrer is correctly set, and if that's in the trusted domain? It's better than nothing, and as IE does set it when the domain is different, it's a catchable tampering of the headers. So the referrer needs to match the allowed domains, if the browser is IE and no origin is set.
If origin is set, match the referrer and the allowed domains maybe even?

@Firesphere
Copy link
Author

@Firesphere Firesphere commented Sep 14, 2017

Also, I can hardly believe we are dealing with exceptions for IE. I mean, it's 2017!

@Firesphere
Copy link
Author

@Firesphere Firesphere commented Sep 14, 2017

Here's an updated version with referral checking in IE11
Manual checking confirms this works, I'm looking at how to unit test this because I'm a tad unsure. cURL request with custom headers probably?

                /**
                 * IE 11 and Edge fix. When CORS is enabled but we are on the same domain,
                 * IE refuses to send the Origin header, causing a lot of pain.
                 * The next best thing, is to validate it's IE or Edge and check the referrer.
                 * This is not the best way to go, but it's something
                 * Note, when it's an actual CORS request, IE does add the header, so we only end up here
                 * only when it is not an actual CORS request
                 * @link https://connect.microsoft.com/IE/feedback/details/781303/origin-header-is-not-added-to-cors-requests-to-same-domain-but-different-port
                 * This fault also happens when the port is the same!
                 */
                $isIE = strpos($request->getHeader('User-Agent'), 'Trident') !== false;
                $isEdge = strpos($request->getHeader('User-Agent'), 'Edge') !== false;
                // The needs parsing to get the base URL
                $refererURL = parse_url($request->getHeader('referer'));
                if (
                    ($isEdge || $isIE) &&
                    // Only use the host, in case of http/https mismatch, which IE ignores as well
                    (in_array($refererURL, $allowedOrigins, true) || in_array('*', $allowedOrigins, true))
                ) {
                    $response->addHeader('Access-Control-Allow-Origin', $origin);
                } else {
                    // No Origin or IE plus valid Referer header present in Request.
                    return $this->httpError(403, "Access Forbidden");
                }

Firesphere added a commit to Firesphere/silverstripe-graphql that referenced this issue Sep 14, 2017
@chillu chillu added this to the Recipe 4.1.0 milestone Nov 1, 2017
@tractorcow tractorcow self-assigned this Jan 17, 2018
@tractorcow
Copy link
Contributor

@tractorcow tractorcow commented Jan 17, 2018

I'm investigating

@Firesphere
Copy link
Author

@Firesphere Firesphere commented Jan 17, 2018

You know where to find me if you have any questions :)

@tractorcow
Copy link
Contributor

@tractorcow tractorcow commented Jan 17, 2018

@Firesphere I found another example of this solution being applied to another project.

juxt/yada#195

With that in mind, it's probably safe enough?

@tractorcow
Copy link
Contributor

@tractorcow tractorcow commented Jan 17, 2018

OWASP seems to think so https://www.owasp.org/index.php/Cross-Site_Request_Forgery_(CSRF)_Prevention_Cheat_Sheet#Checking_the_Origin_Header

If the Origin header is not present, verify the hostname in the Referer header matches the target origin.

@Firesphere
Copy link
Author

@Firesphere Firesphere commented Jan 17, 2018

That seems to be the same form of implementation as my second bit of code on how it could be done. So I'd say, possibly with some added configuration, it's not a too big a security issue

tractorcow pushed a commit to open-sausages/silverstripe-graphql that referenced this issue Jan 18, 2018
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 silverstripe#118
@tractorcow
Copy link
Contributor

@tractorcow tractorcow commented Jan 18, 2018

Fix at #132

Also fixed all the terrible coding style in this module and various other bugs.

@tractorcow tractorcow removed this from the Recipe 4.1.0 milestone Jan 18, 2018
@tractorcow tractorcow added this to the Recipe 4.2.0 milestone Jan 18, 2018
@tractorcow tractorcow removed this from the Recipe 4.2.0 milestone Jan 18, 2018
@tractorcow tractorcow added this to the Recipe 4.0.2 milestone Jan 18, 2018
@tractorcow tractorcow removed their assignment Jan 22, 2018
@flamerohr flamerohr self-assigned this Jan 22, 2018
tractorcow pushed a commit to open-sausages/silverstripe-graphql that referenced this issue Jan 23, 2018
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants