Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1164)

Issue 2013573007: Catch CSP violations in InProcessBrowserTest

Created:
4 years, 7 months ago by wychen
Modified:
4 years, 4 months ago
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/a/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Catch CSP violations in InProcessBrowserTest Content Security Policy violations fail the test by default. When testing CSP specifically, this can be overridden. BUG=81636, 616181, 638050

Patch Set 1 #

Patch Set 2 : only catch CSP msgs #

Patch Set 3 : merge csp turning-on, remove 'unsafe-eval' #

Patch Set 4 : default style-src #

Patch Set 5 : allow chrome resources and eval #

Patch Set 6 : rebase, writelist tests #

Patch Set 7 : rebase #

Patch Set 8 : whitelist more #

Total comments: 17

Patch Set 9 : rebase #

Patch Set 10 : use stack #

Patch Set 11 : address comments #

Patch Set 12 : whitelist more #

Patch Set 13 : rebase #

Patch Set 14 : Use scoped listener #

Patch Set 15 : rebase, and Use ConsoleObserverDelegate instead of log #

Patch Set 16 : set up and check inside RunTestOnMainThreadLoop #

Patch Set 17 : misc #

Patch Set 18 : double check test is run #

Unified diffs Side-by-side diffs Delta from patch set Stats (+49 lines, -3 lines) Patch
M chrome/browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/extensions/api/notifications/notifications_apitest.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/extensions/api/web_request/web_request_apitest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/extensions/content_security_policy_apitest.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/extensions/extension_resource_request_policy_apitest.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/translate/translate_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M chrome/test/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -0 lines 0 comments Download
M chrome/test/base/in_process_browser_test.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 3 chunks +10 lines, -0 lines 0 comments Download
M chrome/test/base/in_process_browser_test.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 6 chunks +28 lines, -3 lines 0 comments Download

Messages

Total messages: 37 (22 generated)
wychen
PTAL
4 years, 6 months ago (2016-05-28 00:32:17 UTC) #4
Avi (use Gerrit)
This looks reasonable but is out of my expertise. Will stamp when tsepez gives his ...
4 years, 6 months ago (2016-05-28 00:51:07 UTC) #5
Paweł Hajdan Jr.
https://codereview.chromium.org/2013573007/diff/140001/chrome/test/base/in_process_browser_test.cc File chrome/test/base/in_process_browser_test.cc (right): https://codereview.chromium.org/2013573007/diff/140001/chrome/test/base/in_process_browser_test.cc#newcode106 chrome/test/base/in_process_browser_test.cc:106: str.find("Content Security Policy") != std::string::npos; It seems fragile to ...
4 years, 6 months ago (2016-05-30 08:43:47 UTC) #6
wychen
+mkwst@ for CSP notification. https://codereview.chromium.org/2013573007/diff/140001/chrome/test/base/in_process_browser_test.cc File chrome/test/base/in_process_browser_test.cc (right): https://codereview.chromium.org/2013573007/diff/140001/chrome/test/base/in_process_browser_test.cc#newcode106 chrome/test/base/in_process_browser_test.cc:106: str.find("Content Security Policy") != std::string::npos; ...
4 years, 6 months ago (2016-05-31 08:11:27 UTC) #8
Mike West
https://codereview.chromium.org/2013573007/diff/140001/chrome/test/base/in_process_browser_test.cc File chrome/test/base/in_process_browser_test.cc (right): https://codereview.chromium.org/2013573007/diff/140001/chrome/test/base/in_process_browser_test.cc#newcode106 chrome/test/base/in_process_browser_test.cc:106: str.find("Content Security Policy") != std::string::npos; On 2016/05/31 at 08:11:27, ...
4 years, 6 months ago (2016-05-31 09:20:00 UTC) #9
Tom Sepez
https://codereview.chromium.org/2013573007/diff/140001/chrome/test/base/in_process_browser_test.cc File chrome/test/base/in_process_browser_test.cc (right): https://codereview.chromium.org/2013573007/diff/140001/chrome/test/base/in_process_browser_test.cc#newcode93 chrome/test/base/in_process_browser_test.cc:93: nit: |static| redundant within namespace {} https://codereview.chromium.org/2013573007/diff/140001/chrome/test/base/in_process_browser_test.cc#newcode106 chrome/test/base/in_process_browser_test.cc:106: str.find("Content ...
4 years, 6 months ago (2016-05-31 16:21:38 UTC) #10
wychen
https://codereview.chromium.org/2013573007/diff/140001/chrome/test/base/in_process_browser_test.cc File chrome/test/base/in_process_browser_test.cc (right): https://codereview.chromium.org/2013573007/diff/140001/chrome/test/base/in_process_browser_test.cc#newcode106 chrome/test/base/in_process_browser_test.cc:106: str.find("Content Security Policy") != std::string::npos; On 2016/05/31 16:21:38, Tom ...
4 years, 6 months ago (2016-05-31 18:17:45 UTC) #11
asargent_no_longer_on_chrome
FWIW I like the suggestion of having some way for your test code to listen ...
4 years, 6 months ago (2016-05-31 19:24:11 UTC) #12
Dan Beam
https://codereview.chromium.org/2013573007/diff/140001/chrome/test/base/in_process_browser_test.cc File chrome/test/base/in_process_browser_test.cc (right): https://codereview.chromium.org/2013573007/diff/140001/chrome/test/base/in_process_browser_test.cc#newcode96 chrome/test/base/in_process_browser_test.cc:96: bool CSPLogHandler(int severity, this will conflict (potentially) with JS-based ...
4 years, 6 months ago (2016-06-07 01:18:30 UTC) #14
wychen
https://codereview.chromium.org/2013573007/diff/140001/chrome/browser/extensions/api/notifications/notifications_apitest.cc File chrome/browser/extensions/api/notifications/notifications_apitest.cc (right): https://codereview.chromium.org/2013573007/diff/140001/chrome/browser/extensions/api/notifications/notifications_apitest.cc#newcode122 chrome/browser/extensions/api/notifications/notifications_apitest.cc:122: SetIgnoreCSPErrorMessages(true); On 2016/05/31 19:24:11, Antony Sargent wrote: > Please ...
4 years, 6 months ago (2016-06-08 05:41:57 UTC) #16
asargent_no_longer_on_chrome
chrome/browser/extensions/* lgtm
4 years, 6 months ago (2016-06-08 15:51:06 UTC) #17
mdjones
distiller lgtm https://codereview.chromium.org/2013573007/diff/140001/chrome/test/base/in_process_browser_test.h File chrome/test/base/in_process_browser_test.h (right): https://codereview.chromium.org/2013573007/diff/140001/chrome/test/base/in_process_browser_test.h#newcode254 chrome/test/base/in_process_browser_test.h:254: nit: extra line.
4 years, 6 months ago (2016-06-08 16:57:04 UTC) #18
Tom Sepez
Nice. Thanks. LGTM.
4 years, 6 months ago (2016-06-08 17:26:25 UTC) #19
Avi (use Gerrit)
lgtm stamp then
4 years, 5 months ago (2016-07-11 19:41:52 UTC) #26
brettw
4 years, 4 months ago (2016-08-23 23:43:39 UTC) #37
Popping up to a higher level, I've heard PlzNavigate is moving to the browser
process. I don't know the timeline for this, but when that's done, it seems like
the right thing is to have that code assert on failure when the tests are
running.

I agree with Antony that trying to hook logging and inspecting the console
messages as a way to get notified about CSP violations seems like a hack. I
don't see any follow up that point. I think that should really be the way we go
here.

If this is important and the PlzNavigate moving it to the browser process is
going to take a while, it seems like a better thing to do is plumb a proper
failure message about CSP failures to the browser process and through the
various delegates.

If such a call ended up in WebContentsObserver, you could install an override in
the test you want and then CHECK inside there. It looks like the current console
messages end up in WebContentsDelegate of which there is only one and it will
not be possible to override from a test as well.

Powered by Google App Engine
This is Rietveld 408576698