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

Document CORS safelist exceptions #621

Merged
merged 5 commits into from Nov 21, 2017

Conversation

3 participants
@estark37
Contributor

estark37 commented Oct 26, 2017

As discussed in #567, browsers have
allowed various cross-origin requests with non-safelisted Content-Type header
values to be sent without CORS preflights. These have occurred either by
accident (and now can't be reversed for compatibility reasons) or because of
design constraints (requests that are implemented outside of the web platform
layer). These CORS exceptions are believed to be safe, but the spec should
document them so that servers know to expect them.

I've added a note about the Content-Type exceptions, but haven't added them to
the safelist, because doing so would imply that web content can trigger
requests with these Content-Type headers and arbitrary bodies. We don't want to
allow fully attacker-controlled requests with these headers, but rather just
want to document the current state where web content can trigger the requests
but not control the headers or bodies.


Preview | Diff

Document CORS safelist exceptions
As discussed in #567, browsers have
allowed various cross-origin requests with non-safelisted Content-Type header
values to be sent without CORS preflights. These have occurred either by
accident (and now can't be reversed for compatibility reasons) or because of
design constraints (requests that are implemented outside of the web platform
layer). These CORS exceptions are believed to be safe, but the spec should
document them so that servers know to expect them.

I've added a note about the Content-Type exceptions, but haven't added them to
the safelist, because doing so would imply that web content can triggers
requests with these Content-Type headers and arbitrary bodies. We don't want to
allow fully attacker-controlled requests with these headers, but rather just
want to document the current state where web content can trigger the requests
but not control the headers or bodies.
@estark37

This comment has been minimized.

Show comment
Hide comment
@estark37

estark37 Oct 26, 2017

Contributor

@annevk I don't know (a) if we are ready to take this approach yet (discussion on #567 is not totally satisfyingly resolved) and (b) if this is the right way to document the existing CORS exceptions. But I thought I'd throw something together as a strawman for discussion.

Contributor

estark37 commented Oct 26, 2017

@annevk I don't know (a) if we are ready to take this approach yet (discussion on #567 is not totally satisfyingly resolved) and (b) if this is the right way to document the existing CORS exceptions. But I thought I'd throw something together as a strawman for discussion.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Oct 27, 2017

Member

Thanks for starting this. I agree that the discussion isn't quite resolved, but we might as well document this meanwhile and discourage further such deviations.

I think this text is in part normative due to the requirement put on servers (mostly to be aware of it) and therefore I'd suggest we create a new "CORS protocol exceptions" section within the "CORS protocol" section after the "Examples" section, so as 3.2.7., with this text. And perhaps the note you have now could become a simple pointer to that section.

Also, instead of framing it on behalf of browsers, I would suggest saying it's specifications that caused this. And also caution new specifications to not go further down this path (or at least not do so without discussion).

Member

annevk commented Oct 27, 2017

Thanks for starting this. I agree that the discussion isn't quite resolved, but we might as well document this meanwhile and discourage further such deviations.

I think this text is in part normative due to the requirement put on servers (mostly to be aware of it) and therefore I'd suggest we create a new "CORS protocol exceptions" section within the "CORS protocol" section after the "Examples" section, so as 3.2.7., with this text. And perhaps the note you have now could become a simple pointer to that section.

Also, instead of framing it on behalf of browsers, I would suggest saying it's specifications that caused this. And also caution new specifications to not go further down this path (or at least not do so without discussion).

@estark37

This comment has been minimized.

Show comment
Hide comment
@estark37

estark37 Oct 27, 2017

Contributor

@annevk addressed your suggestions, PTAL. Do we want to expand more on what should be involved in introducing a new exception (e.g. consideration for how much the attacker controls the request body and other headers, the uniqueness of the Content-Type value, etc.)?

Contributor

estark37 commented Oct 27, 2017

@annevk addressed your suggestions, PTAL. Do we want to expand more on what should be involved in introducing a new exception (e.g. consideration for how much the attacker controls the request body and other headers, the uniqueness of the Content-Type value, etc.)?

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Oct 31, 2017

Member

Perhaps we should simply encourage them to file an issue at https://github.com/whatwg/fetch/issues/new?

Member

annevk commented Oct 31, 2017

Perhaps we should simply encourage them to file an issue at https://github.com/whatwg/fetch/issues/new?

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Oct 31, 2017

Member

Would be nice if @whatwg/security could have a quick look at this as well.

Member

annevk commented Oct 31, 2017

Would be nice if @whatwg/security could have a quick look at this as well.

@annevk

I'd also prefer it if Content-Type was always followed by the word "header". LGTM with those nits.

Show outdated Hide outdated fetch.bs Outdated
@estark37

This comment has been minimized.

Show comment
Hide comment
@estark37

estark37 Nov 3, 2017

Contributor

Added "header" to "Content-Type", added an "and", and added a pointer to file an issue for new exceptions. Also wanted to give a heads-up to @igrigorik and @juliatuttle that I'm including the Reporting API content-type in this.

Contributor

estark37 commented Nov 3, 2017

Added "header" to "Content-Type", added an "and", and added a pointer to file an issue for new exceptions. Also wanted to give a heads-up to @igrigorik and @juliatuttle that I'm including the Reporting API content-type in this.

@annevk annevk requested a review from mikewest Nov 6, 2017

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Nov 6, 2017

Member

Thanks, I'd really like review from one more person before landing this. Another thing that might be good here is to have non-normative reference for each of the mentioned MIME types, in case folks want to read up on what they can expect.

Member

annevk commented Nov 6, 2017

Thanks, I'd really like review from one more person before landing this. Another thing that might be good here is to have non-normative reference for each of the mentioned MIME types, in case folks want to read up on what they can expect.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Nov 6, 2017

Member

Also, I saw on Twitter that the motivation for adding this might be to remove CORS preflights from all reporting infrastructure?

Member

annevk commented Nov 6, 2017

Also, I saw on Twitter that the motivation for adding this might be to remove CORS preflights from all reporting infrastructure?

@estark37

This comment has been minimized.

Show comment
Hide comment
@estark37

estark37 Nov 6, 2017

Contributor

Also, I saw on Twitter that the motivation for adding this might be to remove CORS preflights from all reporting infrastructure?

The motivation for adding this is to document the existing exceptions. The only thing which currently preflights (in Chrome) but I'd like to remove is Expect-CT, because it's done as part of certificate verification and in my mind is similar to OCSP requests. But I can take that Content-Type out of this commit and save it for a separate discussion if you'd like.

Contributor

estark37 commented Nov 6, 2017

Also, I saw on Twitter that the motivation for adding this might be to remove CORS preflights from all reporting infrastructure?

The motivation for adding this is to document the existing exceptions. The only thing which currently preflights (in Chrome) but I'd like to remove is Expect-CT, because it's done as part of certificate verification and in my mind is similar to OCSP requests. But I can take that Content-Type out of this commit and save it for a separate discussion if you'd like.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Nov 6, 2017

Member

I personally think it's fine, but I wanted it to be clear for other people who might follow along.

Member

annevk commented Nov 6, 2017

I personally think it's fine, but I wanted it to be clear for other people who might follow along.

@mikewest

LGTM % one addition.

Given that the reporting requests can/will contain data from multiple origins (e.g. reports.google.com might receive reports from events on youtube.com, google.com, docs.google.com, etc), are completely controlled by the browser as opposed to the page, and are uncredentialed, preflights do not add significant value. Documenting these shipping exceptions seems pretty reasonable to me.

Anne's suggestion that we add links to explanatory documents when possible makes sense to me as well.

Show outdated Hide outdated fetch.bs Outdated
@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Nov 7, 2017

Member

(I have a somewhat unrelated question. Are all of these reports JSON? Should we consider updating https://html.spec.whatwg.org/#json-mime-type so user agents can show more of these as JSON when you'd browse to them directly (admittedly a rather unlikely scenario). I'm somewhat surprised they don't all use the +json convention.)

Member

annevk commented Nov 7, 2017

(I have a somewhat unrelated question. Are all of these reports JSON? Should we consider updating https://html.spec.whatwg.org/#json-mime-type so user agents can show more of these as JSON when you'd browse to them directly (admittedly a rather unlikely scenario). I'm somewhat surprised they don't all use the +json convention.)

@mikewest

This comment has been minimized.

Show comment
Hide comment
@mikewest

mikewest Nov 13, 2017

Member

I think all of these reports are JSON. OCSP request bodies are not.

I think I'd be fine with adding something like +json to the MIME type if that would be helpful. My main concern is to distinguish these from other kinds of JSON requests that the server might be expecting in order to mitigate the risk of CSRF, which meant getting them off application/json directly. But application/report+json seems less likely to be a problem, even for less well-behaved applications.

Member

mikewest commented Nov 13, 2017

I think all of these reports are JSON. OCSP request bodies are not.

I think I'd be fine with adding something like +json to the MIME type if that would be helpful. My main concern is to distinguish these from other kinds of JSON requests that the server might be expecting in order to mitigate the risk of CSRF, which meant getting them off application/json directly. But application/report+json seems less likely to be a problem, even for less well-behaved applications.

@mikewest

This comment has been minimized.

Show comment
Hide comment
@mikewest

mikewest Nov 13, 2017

Member

We talked about this in the WebAppSec meeting at TPAC last week. My sense of the room was that folks were fine with adding it to Fetch. @estark37, WDYT?

I asked folks in the room from other vendors (@dveditz, @johnwilander, @patrickkettner, et al) to skim through their codebases to see if they had similar mechanisms (and/or to align with these MIME types for existing mechanisms).

Member

mikewest commented Nov 13, 2017

We talked about this in the WebAppSec meeting at TPAC last week. My sense of the room was that folks were fine with adding it to Fetch. @estark37, WDYT?

I asked folks in the room from other vendors (@dveditz, @johnwilander, @patrickkettner, et al) to skim through their codebases to see if they had similar mechanisms (and/or to align with these MIME types for existing mechanisms).

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Nov 13, 2017

Member

@mikewest if we can still change these I think using +json would be a lot better. Otherwise your generic JSON end point has to know about all kinds of MIME types.

Member

annevk commented Nov 13, 2017

@mikewest if we can still change these I think using +json would be a lot better. Otherwise your generic JSON end point has to know about all kinds of MIME types.

@estark37

This comment has been minimized.

Show comment
Hide comment
@estark37

estark37 Nov 18, 2017

Contributor

Sorry for dropping this on the floor! I added references and application/xss-auditor-report (though I couldn't find anything explanatory for it, @mikewest do you know of anything?) I'm not sure how to make these into informative references though?

Contributor

estark37 commented Nov 18, 2017

Sorry for dropping this on the floor! I added references and application/xss-auditor-report (though I couldn't find anything explanatory for it, @mikewest do you know of anything?) I'm not sure how to make these into informative references though?

@mikewest

This comment has been minimized.

Show comment
Hide comment
@mikewest

mikewest Nov 20, 2017

Member

@mikewest if we can still change these I think using +json would be a lot better. Otherwise your generic JSON end point has to know about all kinds of MIME types.

I'd suggest we land this patch as-is to reflect the status quo, and come back and update it if/when we are able to update the MIME types browsers are sending.

application/xss-auditor-report (though I couldn't find anything explanatory for it, @mikewest do you know of anything?)

Nope. We've done a bad job documenting this. When we move it into the reporting API, we'll need to write up the format (or add some metrics and look at usage to see if anyone cares about it at all). I'd suggest we come back and link it then.

Member

mikewest commented Nov 20, 2017

@mikewest if we can still change these I think using +json would be a lot better. Otherwise your generic JSON end point has to know about all kinds of MIME types.

I'd suggest we land this patch as-is to reflect the status quo, and come back and update it if/when we are able to update the MIME types browsers are sending.

application/xss-auditor-report (though I couldn't find anything explanatory for it, @mikewest do you know of anything?)

Nope. We've done a bad job documenting this. When we move it into the reporting API, we'll need to write up the format (or add some metrics and look at usage to see if anyone cares about it at all). I'd suggest we come back and link it then.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Nov 20, 2017

Member

@mikewest is there a follow-up issue for changing the MIME type that I can subscribe to? And make sure that results in a change to Fetch if made?

@estark37 you made them non-normative already (by omitting a ! in front of the letters). However, the dominant style is to put all references at the end of the paragraph after the period. Perhaps that would read weird as the connection between the value and the reference is no longer as clear though. What do you think about making this a list?

Member

annevk commented Nov 20, 2017

@mikewest is there a follow-up issue for changing the MIME type that I can subscribe to? And make sure that results in a change to Fetch if made?

@estark37 you made them non-normative already (by omitting a ! in front of the letters). However, the dominant style is to put all references at the end of the paragraph after the period. Perhaps that would read weird as the connection between the value and the reference is no longer as clear though. What do you think about making this a list?

@estark37

This comment has been minimized.

Show comment
Hide comment
@estark37

estark37 Nov 20, 2017

Contributor

What do you think about making this a list?

Sure, done.

Contributor

estark37 commented Nov 20, 2017

What do you think about making this a list?

Sure, done.

@annevk annevk merged commit 860ab86 into whatwg:master Nov 21, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Nov 21, 2017

Member

Thanks a lot!

Member

annevk commented Nov 21, 2017

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment