Leaking headers?

[sgress454]

Leaking headers?

[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...

[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.

[sgress454]

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

[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

[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.

[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

[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