-
Notifications
You must be signed in to change notification settings - Fork 341
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Allow * for Access-Control-Allow-Headers and Access-Control-Allow-Methods #251
Comments
I'd have to defer to the mozilla security team. |
Just as a note, it is possible to use:
But
And Firefox 45:
This is because we already have developers suggesting adding this header via the Apache config:
https://www.google.co.uk/search?q=%22Header+set+Access-Control-Allow-Origin%22 Which would mean that a Simple CORS request (e.g. from a malicious website), could get content from the victim website (e.g. get the members profile page, and if the user is logged in, we can now retrieve their details, and maybe a CSRF token as well). So maybe we do allow wildcards in:
But like |
Thanks, @craigfrancis, I concur. |
Hey Anne, Feetgun (footguns?) are bad, but if a security mechanism is too complex or We can't protect everyone, but we can protect many, by making CORS simpler FWIW, there are many examples out on the internet (some good, some bad) of |
@craigfrancis @mozfreddyb if you use credentials you need to include another header too, so that example wouldn't magically pass the CORS check unless you also included that other header. @roryhewitt do use credentials with CORS? FWIW, I don't really disagree with your position, but as I mentioned elsewhere I'm not really the gatekeeper here. As for reference implementations, I'm not sure we have the bandwidth to maintain that, or are you volunteering? I'm personally also a little wary of writing both the specification and implementation as that leads to tunnel vision issues. |
@annevk, yep, the example was just to show the similarities for those 3 headers, and how we already have a wildcard on As to specification and implementation, I'm not in a position to do this either. |
@craigfrancis, One thing I suspect is that users like to be able to specify
since it means that they don't need to worry about supplying the Vary: Origin header (for correct browser/proxy caching). So maybe in their code, they include a check for Origin, and if it's on their 'safe' list, they respond with:
rather than
|
Are you saying that if Access-Control-Allow-Headers: * is specified for the request, then the server cannot include Access-Control-Allow-Credentials: true in the response? Or that if Access-Control-Allow-Credentials: true is specified in the response, then the browser must throw an error? I assumed that forbidden headers would be disallowed anyway :) Sorry if that wasn't clear. To clarify, I would update https://fetch.spec.whatwg.org/#cors-preflight-fetch to have something like the following: 5.7.7.2: Let headerNames be the result of parsing 5.7.7.6: If headerNames is not set to Does that make sense? |
@annevk, I'm more than willing to try my hand at creating a reference implementation, although as you say, tunnel-vision may occur. I was thinking more of providing some examples of good/bad implementations, including possible server responses. |
@roryhewitt, in summary, don't worry about the security side when adding wildcard support to:
In regards to your second comment, if Or in more detail; today, if a server responds with:
The browsers will normally handle this as you would expect (as a wildcard), but they will reject it when requesting a resource with For any website that wants to allow this behaviour (which is where the security risk comes in), they will need to replace the wildcard with a proper Origin, and provide the
So if we used the same logic with all 3 of these headers, then a response that contains the following should be fine:
You can try this yourself with the following JS, which will only work if the full Origin/Credentials headers are sent from the remote website:
|
I really like the idea that we allow When That's consistent with how |
@sicking, I'm fine with adding * support for all three of these headers (if One thought: A webserver admin might look at this and say "I don't want to On Tue, Mar 22, 2016 at 11:26 AM, Jonas Sicking notifications@github.com
Rory Hewitt |
@roryhewitt I'm happy to add more examples to the specification. New issue or PR for those would be appreciated. I also like the idea of extending wildcard support for the no-credentials scenario. @tyoshino, any concerns with that? |
@annevk & @craigfrancis, what about allowing I understand that we can't have credentialed requests with Unless I'm missing something security-wise, allowing As far as @annevk, OK, I will create an issue/PR for each new example. Also, is there any way to get W3C to update the current CORS spec they have to note that they are no longer updating it, and that it's been taken over by WHATWG? |
FWIW, no need to have an issue/PR per example, one for all is fine too. Getting W3C to acknowledge reality is going rather slow, see #204 (comment) for progress on that. As for |
@annevk, fair point. My concern is that if we don't allow |
I think |
To clarify. I think I think
I don't think this is an accurate characterization. All functionality is already there. What we're debating here is making certain things easier. |
Hmmm. If allowing What I mean is that it's great that we're discussing introducing new functionality to make CORS more usable, but it's a shame that we're also limiting it to a subset of applications. If there is a valid reason to do this (actual quantifiable security risk), that makes sense. I just don't see the specific risk vector here. Basically, what specific security issue is introduced by allowing this? |
I can't think of any problems with I still prefer that we start with it only being available on non-credentialed requests, just so all 3 headers work in the same way. This would be much easier for browsers to accept and implement (because you're matching how it works already). Whereas if you allowed it on credentialed requests as well, I think we will need a lot more people to check this over (just to make sure we haven't missed anything). |
As with any security feature, if it's used correctly there's no problems. The judgement call here is how easy it is to use incorrectly and what problems occur if used incorrectly. I already mentioned one way that it can be used incorrectly here. |
@craigfrancis I guess I'm fine with allowing My concern is that if we don't allow it for credentialed request now, then it will never be allowed for credentialed requests - in the absence of lots of user requests to add it (see my final paragraph below), no-one will ever get round to doing further research (or whatever) to decide whether to allow it. So it will sit there in the Dev queue as "another feature that we should consider", until it withers and dies... So I think it makes sense to make a final decision now to either allow it on all requests or only on non-credentialed requests, even if that might delay the implementation whilst more people weigh in on the security risks. As everyone looking at this thread can figure out I feel strongly that we should allow it on all requests :). I think the benefit it provides, in terms of simplicity of use outweighs the possibility of security risk. Others disagree. Disagreement is good. That being said, I see that @sicking has raised a specific concern about allowing it on credentialed requests:
I'm not sure I understand how allowing XHR to send any headers opens up to CSRF attacks (@sicking can you elaborate for me?), but in any case, as he explicitly points out, many of the concerns that he has (indeed, that we all have) are 'just' about the ways in which incorrect implementation can open security holes - basically, we're concerned about footguns (feetgun?). Personally, I think that allowing Finally, do we have some feedback method from users (as opposed to browser manufacturers) aside from raising issues here? For instance, I have made the contention that a relatively large percentage of requests are credentialed (even if it's still a minority). That's based on my experience, which is a) limited and b) different from other peoples. But do we have any idea of whether I'm correct, or completely mistaken? I guess it's not possible to poll users, but it would be interesting to know. |
Well, we could ask Chromium folks such as @foolip and @mikewest to set up measurements and find out to figure out how much CORS credentialed requests there are vs non-credentialed. The attack @sicking mentioned is that if you have As for including more information in the specification, there's at least one open issue to that effect, #206, and I am certainly open to that as I already mentioned elsewhere. It'll happen quicker with specific PRs or issues. |
@annevk, so the attack @sicking mentioned is only an issue if @sicking, I hope it doesn't sound like I'm attacking you - when I get the 'bit between my teeth', I tend to start trying to poke holes and cause trouble. If any of this comes off that way, my apologies. |
The criteria for adding new features to the web should be to either
But in both cases a requirement is: Don't make it too easy to have security issues. Generally speaking, it should be easier to do the safe thing, than to do the unsafe thing. I don't think any of the features discussed lately matches the first bullet. I think supporting Adding features to credential-less requests is generally quite safe since it essentially only allows things that curl already allows, and so exposes the website to very little risk. In order to add features for requests with credentials, I think it needs to be shown that web developers will understand the implications of those features and not opt in to the wrong thing. Adobe's crossdomain.xml feature was a good example of what happens if we make it too easy to opt in to lots of functionality for requests with credentials. After that feature was released, lots of websites ended up with security problems. I have not heard anything similar for CORS. |
@sicking, surely curl can also do credentialed requests? It can certainly pass cookies... I think we basically agree on the problem, but I am not convinced that the security risks are either too great or too difficult to document to outweigh the usability benefits. As it is, web developers can do all kinds of stupid things. We can't stop them - all we can do is document the right way to implement CORS. If we restrict new features to non-credentialed requests only, I think they will just find other ways to screw up, trying to implement workarounds. So I tend to think that we should provide comprehensive documentation (examples, scenarios, do's and don'ts) and allow them maximum flexibility, in terms of functionality.. The trouble with trying really hard to avoid a footgun, is that you often end up with no gun. Which is no good if you're faced with a bear. |
Yes, curl can pass credentials. But only the credentials of the person invoking curl. If website A makes a CORS-with-credentials request to website B, then it's the users cookies to website B that are sent. That is something that the webdeveloper of website A can't accomplish with curl. It's pretty clear that we're not in a "no gun" situation here. All we're talking about here are APIs for making things that are already possible easier. I.e. these proposals are just grease for existing guns. I feel like we're just going in circles at this point. Unless new information is presented I will stay silent. |
Yea. This change wouldn't relax forbidden headers / methods. @sicking What are you proposing? To extend the forbidden header name list for cross origin fetching by adding |
I'm not proposing that we move I'm suggesting that We should figure out what syntax should be used if a website really want to allow all headers, including the |
Oh, got it. Thanks Jonas for elaboration. So, the step 6 of
will be modified e.g. like as follows: A non-wildcarded header is a header whose name is one of
...
Note that it's guaranteed that headerName is not any of the non-wildcarded header at the point we're modifying preflight cache. |
Requiring I'm going to assume we'll have agreement on this. I don't quite have the bandwidth right now to update the specification, so if anyone else wants to take a crack it, go ahead, but I'll try to get to it soonish. |
I'm fine with the requirement. |
See the result of the measurement at https://bugs.chromium.org/p/chromium/issues/detail?id=602925#c6. About 30% of XHRs are both cross-origin and withCredentials set on Chrome. Note that:
|
The above comment was reporting the fact too roughly. I'd like to correct. (The number of pages Chrome users visited that used sync or async XHRs that are both cross-origin and with withCredentials set) / ((The number of pages Chrome users visited that used async XHRs) + (The number of pages Chrome users visited that used sync XHRs)) = ~30% |
Enable Access-Control-Expose-Headers, Access-Control-Allow-Methods, and Access-Control-Allow-Headers to use a wildcard, with the same restriction as placed upon wildcards in Access-Control-Allow-Origin. Namely, it can only be used for requests where the credentials mode is "omit". The Authorization header still needs to be explicitly listed by Access-Control-Allow-Headers even with the wildcard. This also makes the CORS cache wildcard-aware and updates some of the terminology around CORS caches to share more concepts. Fixes #251 and fixes #252.
my solution:
|
This has almost certainly been discussed before, but would it be possible to allow * (allow-all) as a separate value for the Access-Control-Allow-Headers CORS response header?
This would allow all non-simple headers passed in the request to be added to the browser's preflight cache. This si currently possible by simply mirroring back the value of the Access-Control-Request-Headers request header, but this would be much simpler. The browser would need to track the request headers passed and add them all to their preflight cache (rather than simply parse them out from the Access-Control-Allow-Headers response header, assuming that's what they currently do), but that's not too hard to do.
So the spec would become the following:
Access-Control-Allow-Headers = "Access-Control-Allow-Headers" ":" #field-name | "*"
The text was updated successfully, but these errors were encountered: