New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 11 users
Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Feb 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment
CSP reports are of type "ping", not "CSP report"
Project Member Reported by rob@robwu.nl, Aug 14 2016 Back to list
In 6a32a203bbae04ef29a42f3f364ec00a0adf9179 I introduced RESOURCE_TYPE_CSP_REPORT that was mapped from RequestContextCSPReport, and assumed that this would get CSP reports to be classified as "other" in the webRequest API.

But it gets thrown in the "ping" bin instead.

Let's make this "other" and add a test to make sure that it's really "other".
 
Comment 1 by ch...@chrisn.me.uk, Aug 14 2016
I agree that "ping" is something of a misnomer - is there a technical reason why RESOURCE_TYPE_CSP_REPORT couldn't be exposed to extensions under its own resource type string? It'd help us with more accurately classifying requests, and I'm sure it'd be a good signpost to adblock extension authors trying to work out whether specific "other" requests should be blocked or not that this one really shouldn't, given that it's security-related.
Comment 2 by rob@robwu.nl, Aug 14 2016
Cc: battelli@chromium.org
There are no technical reasons against including a specific type, it's just that I haven't seen any use case for it yet. Firefox supports TYPE_CSP_REPORT, and the only addon that I could find that makes use of it is NoScript.
Comment 3 by rob@robwu.nl, Aug 14 2016
Cc: -battelli@chromium.org
(did not mean to cc)
CSP reports can have security implications (seem my Usenix'13 paper). I think in general it could be useful for extensions to know that the type of a request is CSP_REPORT rather than OTHER.

Comment 5 by rob@robwu.nl, Aug 17 2016
Cc: rdevlin....@chromium.org mpear...@chromium.org
Devlin, WDYT of adding a csp_report type (instead of other / ping)?

Mark, the resource type is used for histograms, and fixing this bug will affect the counters: the numbers for RESOURCE_TYPE_PING will drop, and RESOURCE_TYPE_CSP_REPORT will become non-zero.

Before:
- <a ping> and beacons -> RESOURCE_TYPE_PING
- CSP -> RESOURCE_TYPE_PING
- [nothing] -> RESOURCE_TYPE_CSP_REPORT

After:
- <a ping> and beacons -> RESOURCE_TYPE_PING
- CSP -> RESOURCE_TYPE_CSP_REPORT

Should I update the histograms again? I hope not, but just asking out of caution.
This only affects the SB2.ResourceTypes2 histogram (and various suffixes of that histogram, right)?

If so, I think keeping the same histogram is probably okay if you don't want to bother changing the names.  RESOURCE_TYPE_PING is such a small percent of all those histograms (at most 3%) I think, and you're not changing the total counts emitted to the histogram, that I doubt anyone besides you`all who care about CSP reports will notice the shift from PING to CSP_REPORT as the change rolls out.

In other words, I doubt using the same histograms will cause problems in the future.

That said, it's still best practice to change the names of histograms when you make any change to the semantics.

Cc: esprehn@chromium.org ojan@chromium.org
@5 I have no objections to adding a csp_report type - it seems reasonable to me, and I don't like lumping stuff into "other" when we can help it.

ojan, esprehn - any complaints in exposing this to extensions?  (Trying to avoid another beacon/ping debacle. :))
What does "other" get used for besides this?
In theory, RESOURCE_TYPE_MEDIA and RESOURCE_TYPE_PREFETCH also get bundled in, and other resource types have a defined type.  In practice, I make no guarantees, because this bug proves that we don't do a perfect job of categorizing requests.
Comment 10 by ch...@chrisn.me.uk, Aug 17 2016
Following what Devlin said in #7, it'd be great if Chromium could achieve what Mozilla recently have with their interpretation of the webRequest API: they have 19 distinct resource types [1], and an apparent determination to minimise the amount of stuff that ends up in the "other" bucket [2].

If there's confusion over what ends up in which bucket and it's not immediately obvious from the webRequest source code, I could put together some tests to find out.

[1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/WebRequest/ResourceType
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1209983
Comment 11 by rob@robwu.nl, Sep 29 2016
Labels: -M-54 M-55
Repeating the question of Devlin:

> ojan, esprehn - any complaints in exposing this to extensions?  (Trying to avoid another beacon/ping debacle. :))
Comment 12 by rob@robwu.nl, Oct 9 2016
Labels: -M-55 M-56
Not getting any reply to the above questions, so I submitted the CL here: https://codereview.chromium.org/2402233002/

If you have any objections, now would be a good time to step in.
From the CL:

On 2016/10/12 09:27:48, robwu wrote:
> On 2016/10/11 23:14:15, esprehn wrote:
> > I don't really think we should continue making these extension categories more
> > specific. It's not useful, and it introduces some funny stuff. For example if
> an
> > extension decides to allow CSP reports, but block ping, then pages can just
> > intentionally cause CSP violations to send requests to the server that
> > circumvent the extension blocking. This is the same for categorizing
> sendBeacon
> > as ping, if the page blocks that I'd just use CSP, or size policy reports, or
> > some other platform thing where the browser will send automated reports for
> you.
> 
> Theoretically, that may happen. But in practice, I find it a bit far-fetched
> given the relative number of such extension users compared to the number of user
> agents that may not support CSP reports. If your sketch ever becomes reality,
> then extensions can just start to block csp_report requests, and we are no worse
> off than now. On the other hand, if extensions somehow misbelieve that blocking
> ping requests is a good idea because of "privacy", then they do harm by
> withelding CSP reports from the website.
> 
> For your argument, it does not matter whether the CSP report is of type
> "csp_report" or "other", so I have submitted a patch set that changes it to
> csp_other.

I agree with Rob on this one.  Whether or not we provide the csp_report type, extensions can choose to block them, if they really want to - either by conditionally (or unconditionally) blocking type "other" or through other, hackier means.  The idea that the extension API could be misused is not a reason to make it less useful, and providing this extra information decreases the need for extensions to guess and potentially get it wrong.

In general, every extension API can be misued.  We should design them to make using them the right way easy, and if there are plenty of reasons to want to monitor csp reports (or, in general, the types of various requests).
> Firefox supports TYPE_CSP_REPORT, and the only addon that I could find that makes use of it is NoScript.

I am revising how uBlock Origin ("uBO") deals with "ping" requests for the next version of uBO, and I just stumbled onto the issue here. The new version won't blanket-block "ping" requests anymore, but will filter them just like any other network requests.

However, two special cases:

- "Tabless" ping network requests will still be blanket blocked, with the assumption that they are used as some sort of replacement for hyperlink-auditing. I don't know for sure but I am assuming "tabless" ping requests are rare, if any.

- "Spurious" CSP report: CSP report fired as a result of uBO trying to internally redirect a blocked network requests to a data:-based URI. I had to add code to uBO to handle this special case: if there are internal redirections by uBO on a page, and if the page fires a ping request for CSP-report purpose (because it does not allow data:-based resources in its CSP), uBO will block it as a "spurious" CSP report.

The above CSP-report issue is easily handled on Firefox because it supports a specific type for CSP-report. For Chromium, the handling is slightly more complicated, because I have to hook onto onBeforeSendHeaders (which would not be required otherwise) to look for ping request which content type ends with "/csp-report".

So in case it matters, it definitely would help to use a specific type for CSP-report-related network requests.
Comment 15 by rob@robwu.nl, Oct 17 2016
#14
"Tabless" pings should not receive a blanket block either, because tabless pings are a result of sending a request upon unload. These requests can easily be replaced with synchronous XHR upon unload (which results in a worse user experience). If users/extensions block "ping" (navigator.sendBeacon) requests en masse, then website owners will just stop using sendBeacon and return to XMLHttpRequest. The end result is then 1) you still don't block "ping"s and 2) users (including those without extensions) have to experience a lag when the tab closes.*


You do show a concrete example of why we should have a specific identifier for csp reports: It aids the development of accurate and better performing extensions.
> "Tabless" pings should not receive a blanket block either

Ok, I will fix this.
Comment 17 Deleted
Comment 18 by ch...@chrisn.me.uk, Oct 19 2016
As the original requester for this change, I fully support everything Rob, Devlin and Raymond have said, both here and on the CR. The only suggestion I have, based on Rob's suggestion to Elliott on the CR, is to change the resource type string from "csp_other" back to "csp_report": "csp_other" is needlessly vague, and as a consumer of the webRequest API I'd find that naming quite confusing. If it's a CSP report, it'd be best to call it a CSP report. (I didn't see an updated patch set with "csp_other" as claimed on the CR, but I thought it'd be worth mentioning, seeing as it was being talked about.)
@esprehn, any comments here?  Lingering concerns?
I don't think there's value in continuing to make these request types more specific. Note that whatever capabilities csp or ping have, we're going to bring it all to fetch. Which means pages will be able to trigger the same requests whenever they want for whatever purpose they want. Those will all just be tagged as generic fetch, since we don't know why the page is keeping the request alive. Extensions need to look at the urls if they want to do reasonable filtering.
Comment 21 by rob@robwu.nl, Oct 27 2016
#18 (chris)
csp_other is a typo. It'll be csp_report.


#20 (esprehn)

> I don't think there's value in continuing to make these request types more specific.

Based on what? Multiple use cases have been posted in this bug, so there is clearly some demand for it.

> Note that whatever capabilities csp or ping have, we're going to bring it all to fetch.

I think that you are referring to https://github.com/whatwg/fetch/issues/184. This is still under discussion and not part of the standard. Moreover, the possibility that fetch may get the ability to send detached requests does not mean that adding csp_report is not useful. I don't even have to think hard to come up with unique characteristics: CSP reports can be generated when JS is disabled, whereas the fetch API does require JS.

If/when this comes to fetch, and extension developers have a need for identifying such requests, then they can open a new feature request here.

> Extensions need to look at the urls if they want to do reasonable filtering.

I agree, and extension developers do know this (see comment 17 for instance).
But sometimes the URL is not sufficiently discerning, and more info is needed (e.g. request type as seen by Chrome).


Are you strongly opposed to this feature, and if so can you say what's harmful about it?
If there is no harm, then I will land the patch since there is demand for the feature and supporting it is inexpensive.
Comment 22 by rob@robwu.nl, Nov 29 2016
Labels: -M-56 M-57
Devlin - over a month has passed without any further discussion. Can you put your final decision (LG or not lg) on the patch? https://codereview.chromium.org/2402233002/
Comment 23 by rob@robwu.nl, Dec 21 2016
Devlin, see previous comment ^
Project Member Comment 25 by bugdroid1@chromium.org, Feb 14 2017
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/226d1bedd1dd5ccc7b2c8a67c72470a4c88d179d

commit 226d1bedd1dd5ccc7b2c8a67c72470a4c88d179d
Author: xlai <xlai@chromium.org>
Date: Tue Feb 14 15:47:16 2017

Revert of Tag CSP violation reports as CSP reports, not as ping (patchset #2 id:20001 of https://codereview.chromium.org/2402233002/ )

Reason for revert:
Suspecting CL that cause virtual/stable/http/tests/navigation/image-load-in-unload-handler.html to fail on WebKit Linux Trusty ASAN:

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty%20ASAN/builds/1117

Original issue's description:
> Tag CSP violation reports as CSP reports, not as ping
>
> BUG= 637577 
> TEST=./browser_tests --gtest_filter=ExtensionWebRequestApiTest.WebRequestTypes
>
> Review-Url: https://codereview.chromium.org/2402233002
> Cr-Commit-Position: refs/heads/master@{#450329}
> Committed: https://chromium.googlesource.com/chromium/src/+/ac2c8350c76da73e48d8ed582c85fd8f572b54d5

TBR=esprehn@chromium.org,mkwst@chromium.org,rdevlin.cronin@chromium.org,rob@robwu.nl
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 637577 

Review-Url: https://codereview.chromium.org/2694833003
Cr-Commit-Position: refs/heads/master@{#450366}

[delete] https://crrev.com/90790ebcd0632f1860a89179643ef89fcf34c270/chrome/test/data/extensions/api_test/webrequest/csp/violation.html
[delete] https://crrev.com/90790ebcd0632f1860a89179643ef89fcf34c270/chrome/test/data/extensions/api_test/webrequest/csp/violation.html.mock-http-headers
[modify] https://crrev.com/226d1bedd1dd5ccc7b2c8a67c72470a4c88d179d/chrome/test/data/extensions/api_test/webrequest/test_types.js
[modify] https://crrev.com/226d1bedd1dd5ccc7b2c8a67c72470a4c88d179d/extensions/browser/api/web_request/web_request_api_helpers.cc
[modify] https://crrev.com/226d1bedd1dd5ccc7b2c8a67c72470a4c88d179d/extensions/common/api/web_request.json
[modify] https://crrev.com/226d1bedd1dd5ccc7b2c8a67c72470a4c88d179d/third_party/WebKit/Source/core/loader/PingLoader.cpp

Project Member Comment 26 by bugdroid1@chromium.org, Feb 20 2017
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9eaef2f85417dfc5fbe6df92ca1ccbacaeefad92

commit 9eaef2f85417dfc5fbe6df92ca1ccbacaeefad92
Author: rob <rob@robwu.nl>
Date: Mon Feb 20 16:39:26 2017

Tag CSP violation reports as CSP reports, not as ping

BUG= 637577 
TEST=./browser_tests --gtest_filter=ExtensionWebRequestApiTest.WebRequestTypes

Review-Url: https://codereview.chromium.org/2402233002
Cr-Original-Commit-Position: refs/heads/master@{#450329}
Committed: https://chromium.googlesource.com/chromium/src/+/ac2c8350c76da73e48d8ed582c85fd8f572b54d5
Review-Url: https://codereview.chromium.org/2402233002
Cr-Commit-Position: refs/heads/master@{#451637}

[add] https://crrev.com/9eaef2f85417dfc5fbe6df92ca1ccbacaeefad92/chrome/test/data/extensions/api_test/webrequest/csp/violation.html
[add] https://crrev.com/9eaef2f85417dfc5fbe6df92ca1ccbacaeefad92/chrome/test/data/extensions/api_test/webrequest/csp/violation.html.mock-http-headers
[modify] https://crrev.com/9eaef2f85417dfc5fbe6df92ca1ccbacaeefad92/chrome/test/data/extensions/api_test/webrequest/test_types.js
[modify] https://crrev.com/9eaef2f85417dfc5fbe6df92ca1ccbacaeefad92/extensions/browser/api/web_request/web_request_resource_type.cc
[modify] https://crrev.com/9eaef2f85417dfc5fbe6df92ca1ccbacaeefad92/extensions/browser/api/web_request/web_request_resource_type.h
[modify] https://crrev.com/9eaef2f85417dfc5fbe6df92ca1ccbacaeefad92/extensions/common/api/web_request.json
[modify] https://crrev.com/9eaef2f85417dfc5fbe6df92ca1ccbacaeefad92/third_party/WebKit/Source/core/loader/PingLoader.cpp

Comment 27 by rob@robwu.nl, Feb 28 2017
Labels: -M-57 M-58
Status: Fixed
This feature is available starting from Chrome 58.
Sign in to add a comment