Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2402233002:
Tag CSP violation reports as CSP reports, not as ping (Closed)
|
Created:
4 years, 2 months ago by robwu Modified:
3 years, 10 months ago CC:
blink-reviews, chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org, gavinp+loader_chromium.org, Nate Chapin, loading-reviews_chromium.org, tyoshino+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTag 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}
Committed: https://chromium.googlesource.com/chromium/src/+/9eaef2f85417dfc5fbe6df92ca1ccbacaeefad92
Patch Set 1 #Patch Set 2 : Change resourceType "other" to "csp_report" #Patch Set 3 : Rebase on patch for crbug.com/129353 #
Messages
Total messages: 40 (22 generated)
The CQ bit was checked by rob@robwu.nl to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rob@robwu.nl changed reviewers: + esprehn@chromium.org, mkwst@chromium.org, rdevlin.cronin@chromium.org
rob@robwu.nl changed required reviewers: + mkwst@chromium.org, rdevlin.cronin@chromium.org
Mike: PTAL at the PingLoader change. Devlin: PTAL at the tests. Elliott: FYI
So it looks like this lumps CSP reports in with "other" in the API itself. Is there a reason to do that instead of adding a csp_report type? (Or just for simplicity?) If we do go with that, it'd be good to specify it in the CL description, since right now it sounds like they will be classified separately.
On 2016/10/10 14:53:03, Devlin wrote: > So it looks like this lumps CSP reports in with "other" in the API itself. Is > there a reason to do that instead of adding a csp_report type? (Or just for > simplicity?) If we do go with that, it'd be good to specify it in the CL > description, since right now it sounds like they will be classified separately. Oops. You're right. I wrote this patch over a month ago and forgot to change the type while waiting for feedback on the bug. I will change it to "csp_report" unless anyone strongly objects.
LGTM.
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.
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. Devlin - Can you LG if you want the patch to land despite Elliott's feedback, or wait with LGing if you want to extend the discussion? (shouldn't we be discussing this on the bug?)
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. > > Devlin - Can you LG if you want the patch to land despite Elliott's feedback, or > wait with LGing if you want to extend the discussion? (shouldn't we be > discussing this on the bug?) Yes, we should be discussing this on the bug. I've moved the conversation there. I'd like to reach at some consensus (or at least acceptance) before landing.
Devlin, can you approve this CL? There has not been any discussion on the bug for the past three months.
On 2017/02/06 15:09:38, robwu wrote: > Devlin, can you approve this CL? There has not been any discussion on the bug > for the past three months. lgtm
The CQ bit was checked by rob@robwu.nl
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/2402233002/#ps20001 (title: "Change resourceType "other" to "csp_report"")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487065502765820,
"parent_rev": "9e87351607a343f906b1866ea41767434986ebbe", "commit_rev":
"ac2c8350c76da73e48d8ed582c85fd8f572b54d5"}
Message was sent while issue was closed.
Description was changed from ========== Tag CSP violation reports as CSP reports, not as ping BUG=637577 TEST=./browser_tests --gtest_filter=ExtensionWebRequestApiTest.WebRequestTypes ========== to ========== 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/+/ac2c8350c76da73e48d8ed582c85... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/ac2c8350c76da73e48d8ed582c85...
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2694833003/ by xlai@chromium.org. The reason for reverting is: 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....
Message was sent while issue was closed.
Description was changed from ========== 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/+/ac2c8350c76da73e48d8ed582c85... ========== to ========== 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/+/ac2c8350c76da73e48d8ed582c85... ==========
On 2017/02/14 15:44:38, xlai (Olivia) wrote: > A revert of this CL (patchset #2 id:20001) has been created in > https://codereview.chromium.org/2694833003/ by mailto:xlai@chromium.org. > > The reason for reverting is: 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.... My CL only changes the the label of CSP/XSS violation reports. The failing test tests the logic of PingLoader::loadImage, which is not affected by my patch. I will submit the CL again in a few days since the revert seems unnecessary.
The CQ bit was checked by rob@robwu.nl
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rob@robwu.nl to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rob@robwu.nl
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/2402233002/#ps40001 (title: "Rebase on patch for crbug.com/129353")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by rob@robwu.nl
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487608560442030,
"parent_rev": "dbc456d4d716612c740256fc91f928983da98cc7", "commit_rev":
"9eaef2f85417dfc5fbe6df92ca1ccbacaeefad92"}
Message was sent while issue was closed.
Description was changed from ========== 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/+/ac2c8350c76da73e48d8ed582c85... ========== to ========== 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/+/ac2c8350c76da73e48d8ed582c85... Review-Url: https://codereview.chromium.org/2402233002 Cr-Commit-Position: refs/heads/master@{#451637} Committed: https://chromium.googlesource.com/chromium/src/+/9eaef2f85417dfc5fbe6df92ca1c... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/9eaef2f85417dfc5fbe6df92ca1c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
