Leaking headers?

[sgress454]

Let me preface this by saying that this module works great and seems generally well-thought-out, and it's not at all clear to me what the best practice for exposing Access-Control-* headers is in every situation (or even if there is a consensus on best practice), but it seems to me that the code leaks information in a couple of ways:

1. If the origin setting is set to a string (a "fixed" origin), then the Access-Control-Allow-Origin header is always returned with that value, regardless of the request origin.
2. All of the other Access-Control-* headers are always returned regardless of whether the request origin is in the whitelist.

I'm fairly certain that an AJAX request from a non-whitelisted origin will not have access to these headers (that is, the browser will not provide them in the response), but you can still see them by, for example, looking in the "Network" tab of the Chrome developer console.

I don't see the rationale for exposing the criteria for acceptance to a requestor that doesn't meet those criteria. For example, if you had an admin app on some subdomain ("xyzadmin.myapp.com") that you didn't want to be made public, and you have your main app ("myapp.com") accepting cross-origin requests only from that domain, it doesn't seem right that by making an AJAX request to myapp.com from any random domain you could get the Access-Control-Allow-Origin header with the value xyzadmin.myapp.com and thus learn about that existence of the admin server. Similarly, if someone made an AJAX PUT request to some route and it was rejected for not matching the whitelist of available methods, why would you want that person to be given the list of available methods that are allowed?

What's even more curious to me is that in this module's implementation, specifying the origin setting as an array always does what I would consider the best behavior, which is to not send the Access-Control-Allow-Origin header at all if the requesting origin doesn't match the whitelist. It does this even if the array only has one item.

Absent a good reason to send back all this information, I'd propose that any request whose origin doesn't match the whitelist should be responded to without any Access-Control-* headers at all, and I'd be happy to submit a patch to make things work that way. I'd even go so far as to say that requests that do match the origin whitelist should only be given as much information as is necessary to satisfy the browser (that is, instead of Access-Control-Allow-Methods showing all the allowable methods, only reflect the one that was actually used in the request), but I'm not quite as bullish on that point.

I'm also fully prepared to hear that I don't know what I'm talking about and that there is a perfectly reasonable explanation for the current behavior. I wouldn't be surprised if it's hidden in the W3C CORS spec, clear as mud.

[dougwilson]

Hi @sgress454 thanks for this long post :) ! I am helping out on this module now and going through the issues. I need to get more familiar with the implementation here to provide insight, but the general answer is any deviation from the W3C CORS spec is a bug :)

[jamesjjk]

@dougwilson any update on this?

[dougwilson]

I have not had time to dig in much. If you have any insight, that would be awesome!

[jamesjjk]

Hey @dougwilson sure, we noticed line 67 of lib/index.js (master) has a conditional to check if the options.origin parameter is a string. However the isOriginAllowed method seemingly does the same check (line 27). ideally as I understand the spec indicates not to return the origin header at all. which you do by setting it false. However it would require verification.

else if (isString(options.origin)) {
      // fixed origin
      headers.push([{
        key: 'Access-Control-Allow-Origin',
        value: options.origin
      }]);
      headers.push([{
        key: 'Vary',
        value: 'Origin'
      }]);
    } else {
      isAllowed = isOriginAllowed(requestOrigin, options.origin);
      console.log('isAllowed', isAllowed);
      // reflect origin
      headers.push([{
        key: 'Access-Control-Allow-Origin',
        value: isAllowed ? requestOrigin : false
      }]);
      headers.push([{
        key: 'Vary',
        value: 'Origin'
      }]);
    }


function isOriginAllowed(origin, allowedOrigin) {
    if (Array.isArray(allowedOrigin)) {
      for (var i = 0; i < allowedOrigin.length; ++i) {
        if (isOriginAllowed(origin, allowedOrigin[i])) {
          return true;
        }
      }
      return false;
    } else if (isString(allowedOrigin)) {
      return origin === allowedOrigin;
    } else if (allowedOrigin instanceof RegExp) {
      return allowedOrigin.test(origin);
    } else {
      return !!allowedOrigin;
    }
  }

[dougwilson]

which you do by setting it false

Sorry, I'm not sure if you really followed the thread above; I didn't write the original code, so replying to me with "you do ..." when you're taking about the code makes it sound like (a) I did and (b) I am fully aware of how it works. The reason I'm asking for help is because I haven't fully looked over the code as I said.

I would suggest maybe helping by putting together a pull request. I'm not really following what you're saying, perhaps because there is an assumption here that I understand the code within the module fully, which is why I'm just trying to re-iterate that I don't :) I can try to put together a fix if you can be more specific on what needs to change where in the code. I see you pointed to a line, but then I don't really follow the rest of what you're saying needs to be changed if you can help out there.

[sgress454]

@dougwilson I'll make a PR out of the patched version we've been using in Sails; the massive post above was meant to elicit feedback from the community as to whether my expectations for CORS headers were correct. It sounds like at least one person thinks so :P

[dougwilson]

Yes, if this is operating contary to the CORS spec, then it should be changed. If the CORS spec says the way this works now it should not change. If the CORS spec does not say either way, add it as an optional behavior change.

I haven't really studied how this module is working specifically, which is what I was going to look into. Just back up the pull request with a reference to what the spec says is all.

I hope that makes sense.

[jamesjjk]

I noticed there are a few more problems such as returning the access-control-allow-credentials even when the request is not a valid origin. Also not case sensitive.

[sgress454]

@dougwilson Actually, not sure how I missed this in the CORS doc before (other than my eyes glazing over every time I look at it), but this section clearly states that if no origin header is present in the request, or if the header doesn't match the whitelist, then the server should not set any additional headers in the response. Same goes for failed checks in the preflight. I'll link to that section in the PR.

[dougwilson]

Yea, @sgress454 , that was reported in #114 though the user declined to make a pull request.

[sgress454]

Alrighty, working on it.

[sgress454]

Wow, time flies. I actually got the code for this done pretty quick, but ran out of gas re-working all the tests. Plugging away...

[dijonkitchen]

I've come across these findings as well. Any updates @sgress454 ?

[amscher]

I have come across it as well, would be great to have an update @sgress454

[sgress454]

Thanks for the bump y'all. Will revisit this week. Again, the delay was in getting all the tests in order.

[msageryd]

I'm about to choose a cors library. This seems to be a go-to lib. But I'm a little weary due to this issue being open for three years.

Is this "leak" a verified problem?
Is is a very bad thing?
If it were very bad I'd hope more people would have chimed in at this issue.

[jub0bs]

@sgress454

I don't see the rationale for exposing the criteria for acceptance to a requestor that doesn't meet those criteria. For example, if you had an admin app on some subdomain ("xyzadmin.myapp.com") that you didn't want to be made public, and you have your main app ("myapp.com") accepting cross-origin requests only from that domain, it doesn't seem right that by making an AJAX request to myapp.com from any random domain you could get the Access-Control-Allow-Origin header with the value xyzadmin.myapp.com and thus learn about that existence of the admin server.

The current spec for CORS can be found in the Fetch standard, which is less prescriptive than the original W3C standard was about how servers ought to implement CORS.

The last paragraph in https://fetch.spec.whatwg.org/#cors-protocol-and-http-caches gives one use case for unconditionally including Access-Control-Allow-Origin in responses:

if Access-Control-Allow-Origin is set to * or a static origin for a particular resource, then configure the server to always send Access-Control-Allow-Origin in responses for the resource — for non-CORS requests as well as CORS requests — and do not use Vary.

[jub0bs]

Related: whatwg/fetch#1588