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

Are report uploads supposed to send CORS preflights? #41

Closed
estark37 opened this Issue May 24, 2017 · 15 comments

Comments

Projects
None yet
8 participants
@estark37
Copy link

commented May 24, 2017

http://wicg.github.io/reporting/#try-delivery sets the request's mode to "cors", but does not set the unsafe-request flag or the use-CORS-preflight flag. So I might be misreading Fetch, but I think that means that CORS preflights are skipped?

Because reports have a non-CORS-whitelisted header (Content-Type: application/report), it seems like they should use preflights.

Are reports intended to send CORS preflights? If not, what's the justification?

cc @annevk @mikewest

@mikewest

This comment has been minimized.

Copy link
Member

commented May 26, 2017

http://wicg.github.io/reporting/#try-delivery sets the request's mode to "cors", but does not set the unsafe-request flag or the use-CORS-preflight flag.

I think we're ok here: the content-type header is not a "CORS-safelisted request header", so a preflight should be triggered by step 12 of https://fetch.spec.whatwg.org/#main-fetch.

The intent for the Reporting API was to do The Right Thing(tm) with regard to CORS. That said, I don't think it currently has enough information to set a reasonable Origin header. We can't set a client, as we probably don't actually have a browsing context at the time we're generating a reporting request. We do have origins for both reports and clients, so we ought to be able to set the request's origin directly in https://wicg.github.io/reporting/#try-delivery. That seems like a real bug.

@estark37

This comment has been minimized.

Copy link
Author

commented May 26, 2017

I think we're ok here: the content-type header is not a "CORS-safelisted request header", so a preflight should be triggered by step 12 of https://fetch.spec.whatwg.org/#main-fetch.

But Step 12 only triggers a preflight if one of the use-CORS-preflight or unsafe-request flags are set, right? Which don't seem to be set by the Reporting API, unless they are set by default or something?

@mikewest

This comment has been minimized.

Copy link
Member

commented May 26, 2017

But Step 12 only triggers a preflight if one of the use-CORS-preflight or unsafe-request flags are set, right? Which don't seem to be set by the Reporting API, unless they are set by default or something

Hrm. I think you're right. I've been misreading that for a while, apparently. :)

We should either be setting the unsafe-request flag in Reporting, or justifying why we don't set it. I agree with you.

@annevk

This comment has been minimized.

Copy link
Member

commented Jun 26, 2017

(We should maybe consider changing the setup in Fetch to make it harder to do the wrong thing. The reason the unsafe-request flag exists is so that EventSource can use Last-Event-ID without preflight. I think that might be the only use case.)

@RByers

This comment has been minimized.

Copy link
Collaborator

commented Oct 3, 2017

Mike, this is one of the main issues blocking shipping Reporting API, right? Are you the right person to drive it, or do you want to assign it to someone else?

@dcreager dcreager self-assigned this Jun 8, 2018

@dcreager

This comment has been minimized.

Copy link
Member

commented Jun 14, 2018

We do have origins for both reports and clients, so we ought to be able to set the request's origin directly in https://wicg.github.io/reporting/#try-delivery

We'd want this to be the origin of the reports in the upload, right? (i.e., the origin of the requests that the reports describe) That, in turn, would mean that each report upload could only contain reports that have the same origin. We couldn't batch reports about multiple different origins together into a single upload if they happen to use the same upload URLs. (I think we've talked about that being a good thing to prohibit regardless of the CORS discussion.)

@dcreager

This comment has been minimized.

Copy link
Member

commented Jun 29, 2018

Are reports intended to send CORS preflights? If not, what's the justification?

Having thought about this some more, I'd like to revisit this question. Short version is that I think we can have Reporting use no-cors for its report uploads, and remove application/report from the safelist exception in Fetch. @mikewest / @estark37, can you check my work?

CORS preflights for report uploads would:

  1. allow the collector to control whether the report upload's response content is delivered to cross-origin web content
  2. require the user agent to ask permission from the collector before uploading reports about an origin

But for (1), report uploads have no response content, and even if they did, those responses are already never delivered to web content (even if a CORS preflight response would tell the browser it was allowed).

For (2), I don't think the collector can rely on CORS preflights to prevent uploads about origins it doesn't care about. For instance, we want Reporting to work with non-browser user agents (which will typically not implement CORS at all). So the collector will have to filter reports that it receives anyway.

If we change report uploads to use no-cors, then:

  • we avoid preflights without having to use the safelist exception
  • might get back an opaque response, but that's okay because we're never going to look in it

Thoughts?

@domenic

This comment has been minimized.

Copy link

commented Jun 29, 2018

We should stop trying to bypass the same origin policy, and the protections like CORS, for browser-initiated fetches. Although it may be technically doable or even secure, it sets a bad precedent for the ecosystem, that browser engineers continually try to bypass the security infrastructure we have in place when they go to write standards. no-cors is supposed to be treated as legacy, and we've been trying to stick to that when adding new features to the web platform (e.g. module scripts).

@yoavweiss

This comment has been minimized.

Copy link

commented Jul 6, 2018

I don't think that the "bad precedent" argument is very strong, as preflights have a real cost and sometimes it makes sense to avoid them.

At the same time, I'm not sure that bypassing preflights here is the right call:

  • Since reports are not critical requests, adding an extra RTT for a preflight doesn't seem awful.
  • Since they do send POST requests with non-form content type, one can imagine non-CORS ready intranet servers where this may trigger unwanted behavior.

@mikewest and @estark - do you agree with the above?

@dcreager - Is there a significant cost to adding preflight support to the collection infrastructure? Any use-cases which adding a preflight will somehow hurt?

@roryhewitt

This comment has been minimized.

Copy link

commented Jul 6, 2018

@yoavweiss As long as the preflight response includes a suitable Access-Control-Max-Age value (e.g. 86400 seconds == 1 day), so it gets cached in the browser, then it's only one additional preflight request per day per URL... So yeah, it's an overhead, but barely.

@dcreager

This comment has been minimized.

Copy link
Member

commented Jul 6, 2018

Is there a significant cost to adding preflight support to the collection infrastructure? Any use-cases which adding a preflight will somehow hurt?

Nah, it'll be easy enough to have the collectors respond to preflight requests. My main concern was that we also have to support non-browser clients, which don't typically implement CORS. So while the collectors can reply to preflights to make browser uploads work, the collection infrastructure will still have to verify that incoming report uploads are for origins that it's willing to collect, since it can't assume that every user agent will have already used CORS to verify that.

@dcreager dcreager closed this Jul 6, 2018

@dcreager dcreager reopened this Jul 6, 2018

@dcreager

This comment has been minimized.

Copy link
Member

commented Jul 6, 2018

[I fat-fingered the UI, so the previous comment is edited from any email notifications you might have just received]

@estark37

This comment has been minimized.

Copy link
Author

commented Jul 6, 2018

So while the collectors can reply to preflights to make browser uploads work, the collection infrastructure will still have to verify that incoming report uploads are for origins that it's willing to collect, since it can't assume that every user agent will have already used CORS to verify that.

That doesn't diminish the value of CORS, though. The point of sending the preflight is to ensure that web content can't trigger unexpected requests to servers that can exploit or break those servers. Non-web clients might do whatever they want, and servers have to be prepared for that, but the point of CORS is to give servers guarantees about what web content can do.

@dcreager

This comment has been minimized.

Copy link
Member

commented Jul 6, 2018

Got it! In that case #97 has the updated spec text that turns on preflights for report uploads.

@dcreager

This comment has been minimized.

Copy link
Member

commented Jul 9, 2018

Closed by #97

@dcreager dcreager closed this Jul 9, 2018

dcreager added a commit to dcreager/fetch that referenced this issue Jul 9, 2018

Remove Reporting API from CORS exceptions
As of w3c/reporting#41, the Reporting spec sends CORS preflights for
report uploads if the origin of the collector is different than the
origin of the reports in the upload.  That means we can remove Reporting
from the CORS protocol exception list.

aarongable pushed a commit to chromium/chromium that referenced this issue Jul 10, 2018

Reporting: Uploads should only contain a single origin
Per w3c/reporting#41, a reporting upload
should only contain reports about a single origin.  This will allow us
to send a CORS preflight for that origin, if it differs from the origin
of the collector receiving the reports.

This patch also removes the notion of an endpoint being "pending".  In
the spec, a "pending endpoint" is one that has been taken out of the
rotation because of too many failures; we're handling this with a
per-endpoint BackoffEntry.  And now that we're creating separate
uploads for each origin that uses a collector, we don't want to penalize
any of them by serializing their uploads.

Bug: 860802
Change-Id: I45cf905bd9ec3491e61aa0567c6dc0a19e957313
Reviewed-on: https://chromium-review.googlesource.com/1128599
Commit-Queue: Douglas Creager <dcreager@chromium.org>
Reviewed-by: Misha Efimov <mef@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573815}

aarongable pushed a commit to chromium/chromium that referenced this issue Jul 13, 2018

Reporting: Send CORS preflight before uploading reports
Per w3c/reporting#41, we're supposed to send a
CORS preflight request before uploading reports, if the origin of the
reports is different than the origin of the collector.

There is existing CORS preflight code in both Blink and the Network
Service.  Code in //net isn't allowed to depend on either of those;
instead of using a delegate to handle the inverted dependency, we just
send out the preflight request by hand, just like the Expect-CT code
does.

Bug: 860802
Change-Id: Ib20db54d3d2597d9fbacfe356e61cc6d3bc9d5fc
Reviewed-on: https://chromium-review.googlesource.com/1128600
Commit-Queue: Douglas Creager <dcreager@chromium.org>
Reviewed-by: Misha Efimov <mef@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574836}

dcreager added a commit to dcreager/fetch that referenced this issue Jul 23, 2018

Remove Reporting API from CORS exceptions
As of w3c/reporting#41, the Reporting spec sends CORS preflights for
report uploads if the origin of the collector is different than the
origin of the reports in the upload.  That means we can remove Reporting
from the CORS protocol exception list.

annevk added a commit to whatwg/fetch that referenced this issue Jul 31, 2018

Remove Reporting API from CORS exceptions
As of w3c/reporting#41, the Reporting spec sends CORS preflights for report uploads if the origin of the collector is different than the origin of the reports in the upload.  That means we can remove Reporting from the CORS protocol exception list.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.