Description was changed from ========== PlzNavigate|CSP Use the SourceLocation in violation reports. The SourceLocation struct ...
3 years, 9 months ago
(2017-03-21 12:33:20 UTC)
#1
Description was changed from
==========
PlzNavigate|CSP Use the SourceLocation in violation reports.
The SourceLocation struct is available during a navigation thanks to this CL:
https://codereview.chromium.org/2720763002
This patch makes use of it for CSP. It fixes several test where the line
number in console messages was missing.
BUG=690946
==========
to
==========
PlzNavigate|CSP Use the SourceLocation in violation reports.
The SourceLocation struct is available during a navigation thanks to this CL:
https://codereview.chromium.org/2720763002
This patch makes use of it for CSP. It fixes several test where the line
number in console messages was missing.
BUG=690946
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
arthursonzogni
Description was changed from ========== PlzNavigate|CSP Use the SourceLocation in violation reports. The SourceLocation struct ...
3 years, 9 months ago
(2017-03-21 12:35:33 UTC)
#2
Description was changed from
==========
PlzNavigate|CSP Use the SourceLocation in violation reports.
The SourceLocation struct is available during a navigation thanks to this CL:
https://codereview.chromium.org/2720763002
This patch makes use of it for CSP. It fixes several test where the line
number in console messages was missing.
BUG=690946
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
to
==========
PlzNavigate|CSP Use the SourceLocation in violation reports.
The SourceLocation struct is available during a navigation thanks to this CL:
https://codereview.chromium.org/2720763002
This patch makes use of it for CSP. It fixes several test where the line
number in console messages was missing.
BUG=690946
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel
==========
arthursonzogni
The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run
3 years, 9 months ago
(2017-03-21 12:51:58 UTC)
#3
Description was changed from ========== PlzNavigate|CSP Use the SourceLocation in violation reports. The SourceLocation struct ...
3 years, 9 months ago
(2017-03-21 16:03:12 UTC)
#13
Description was changed from
==========
PlzNavigate|CSP Use the SourceLocation in violation reports.
The SourceLocation struct is available during a navigation thanks to this CL:
https://codereview.chromium.org/2720763002
This patch makes use of it for CSP. It fixes several test where the line
number in console messages was missing.
BUG=690946
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel
==========
to
==========
PlzNavigate & CSP. Use the SourceLocation in violation reports.
The SourceLocation struct is available during a navigation thanks to this CL:
https://codereview.chromium.org/2720763002
This patch makes use of it for CSP. It fixes several test where the line
number in console messages was missing.
BUG=690946
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel
==========
Hi Camille and Mike, Please can you take a look? Thanks! Camille: * content/ Mike: ...
3 years, 9 months ago
(2017-03-21 16:20:38 UTC)
#15
Hi Camille and Mike,
Please can you take a look? Thanks!
Camille:
* content/
Mike:
* third_party/WebKit/public/{web, platform}/
* content/common/frame_messages.h
https://codereview.chromium.org/2761153003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/ancestor_throttle.cc (right):
https://codereview.chromium.org/2761153003/diff/60001/content/browser/frame_h...
content/browser/frame_host/ancestor_throttle.cc:192: if
(parent->EnforceCsp(CSPDirective::FrameSrc, url, is_redirect,
While I added the SourceLocation argument to this function, I choose to change
the "IsAllowedByCsp" name, because it was not clear that this function produces
side-effects.
It is probably unrelated to this CL though, let me know what do you think.
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
3 years, 9 months ago
(2017-03-21 17:34:32 UTC)
#16
3 years, 9 months ago
(2017-03-21 17:34:33 UTC)
#17
Dry run: This issue passed the CQ dry run.
Mike West
LGTM % the rename, which I think needs a little more discussion. https://codereview.chromium.org/2761153003/diff/60001/content/browser/frame_host/ancestor_throttle.cc File content/browser/frame_host/ancestor_throttle.cc ...
3 years, 9 months ago
(2017-03-22 08:57:38 UTC)
#18
LGTM % the rename, which I think needs a little more discussion.
https://codereview.chromium.org/2761153003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/ancestor_throttle.cc (right):
https://codereview.chromium.org/2761153003/diff/60001/content/browser/frame_h...
content/browser/frame_host/ancestor_throttle.cc:192: if
(parent->EnforceCsp(CSPDirective::FrameSrc, url, is_redirect,
On 2017/03/21 at 16:20:38, arthursonzogni wrote:
> While I added the SourceLocation argument to this function, I choose to change
the "IsAllowedByCsp" name, because it was not clear that this function produces
side-effects.
> It is probably unrelated to this CL though, let me know what do you think.
It seems odd to diverge from Blink's implementation. There we do something like
`policy->allowRequest(...)` and pass in an enum that governs whether or not
we're just checking the result or doing reporting. I'd suggest splitting this
change out into a separate patch so we have a chance to discuss the model in a
little more detail.
arthursonzogni
I applied your suggestion. Thanks for the review! https://codereview.chromium.org/2761153003/diff/60001/content/browser/frame_host/ancestor_throttle.cc File content/browser/frame_host/ancestor_throttle.cc (right): https://codereview.chromium.org/2761153003/diff/60001/content/browser/frame_host/ancestor_throttle.cc#newcode192 content/browser/frame_host/ancestor_throttle.cc:192: if ...
3 years, 9 months ago
(2017-03-22 09:38:35 UTC)
#19
I applied your suggestion.
Thanks for the review!
https://codereview.chromium.org/2761153003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/ancestor_throttle.cc (right):
https://codereview.chromium.org/2761153003/diff/60001/content/browser/frame_h...
content/browser/frame_host/ancestor_throttle.cc:192: if
(parent->EnforceCsp(CSPDirective::FrameSrc, url, is_redirect,
On 2017/03/22 08:57:38, Mike West wrote:
> On 2017/03/21 at 16:20:38, arthursonzogni wrote:
> > While I added the SourceLocation argument to this function, I choose to
change
> the "IsAllowedByCsp" name, because it was not clear that this function
produces
> side-effects.
> > It is probably unrelated to this CL though, let me know what do you think.
>
> It seems odd to diverge from Blink's implementation. There we do something
like
> `policy->allowRequest(...)` and pass in an enum that governs whether or not
> we're just checking the result or doing reporting. I'd suggest splitting this
> change out into a separate patch so we have a chance to discuss the model in a
> little more detail.
Okay, let's do this!
arthursonzogni
The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run
3 years, 9 months ago
(2017-03-27 12:09:01 UTC)
#20
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-clang/builds/64056) ios-simulator on ...
3 years, 8 months ago
(2017-03-28 12:35:48 UTC)
#27
Description was changed from ========== PlzNavigate & CSP. Use the SourceLocation in violation reports. The ...
3 years, 8 months ago
(2017-03-28 15:17:49 UTC)
#33
Description was changed from
==========
PlzNavigate & CSP. Use the SourceLocation in violation reports.
The SourceLocation struct is available during a navigation thanks to this CL:
https://codereview.chromium.org/2720763002
This patch makes use of it for CSP. It fixes several test where the line
number in console messages was missing.
BUG=690946
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel
==========
to
==========
PlzNavigate & CSP. Use the SourceLocation in violation reports.
The SourceLocation struct is available during a navigation thanks to this CL:
https://codereview.chromium.org/2720763002
This patch makes use of it for CSP. It fixes several test where the line
number in console messages was missing.
BUG=690946,705098
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel
==========
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
3 years, 8 months ago
(2017-03-28 16:13:19 UTC)
#34
Hi Alex, Please can you take a this CL? (as an OWNER of content) Thanks!
3 years, 8 months ago
(2017-03-29 09:32:55 UTC)
#37
Hi Alex,
Please can you take a this CL? (as an OWNER of content)
Thanks!
alexmos
content/ LGTM, and a couple of minor clarification questions about test expectations. https://codereview.chromium.org/2761153003/diff/200001/content/common/content_security_policy/csp_context.h File content/common/content_security_policy/csp_context.h ...
3 years, 8 months ago
(2017-03-29 23:27:32 UTC)
#38
3 years, 8 months ago
(2017-03-30 11:26:48 UTC)
#42
Dry run: This issue passed the CQ dry run.
arthursonzogni
Thanks for the review! https://codereview.chromium.org/2761153003/diff/200001/content/common/content_security_policy/csp_context.h File content/common/content_security_policy/csp_context.h (right): https://codereview.chromium.org/2761153003/diff/200001/content/common/content_security_policy/csp_context.h#newcode23 content/common/content_security_policy/csp_context.h:23: // and what is the ...
3 years, 8 months ago
(2017-03-30 11:36:27 UTC)
#43
Thanks for the review!
https://codereview.chromium.org/2761153003/diff/200001/content/common/content...
File content/common/content_security_policy/csp_context.h (right):
https://codereview.chromium.org/2761153003/diff/200001/content/common/content...
content/common/content_security_policy/csp_context.h:23: // and what is the set
of scheme that bypasses the CSP. Its main implementation
On 2017/03/29 23:27:32, alexmos wrote:
> nit: "set of schemes that bypass the CSP"
Done.
https://codereview.chromium.org/2761153003/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation
(right):
https://codereview.chromium.org/2761153003/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation:10:
crbug.com/690946
http/tests/security/contentSecurityPolicy/frame-src-cross-origin-load.html [
Failure ]
On 2017/03/29 23:27:32, alexmos wrote:
> I'm curious why this test is still not working?
Hum, I don't know. I will investigate.
It looks like the SourceLocation that is captured here:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/LocalFrame...
doesn't contain the data.
The goal of this CL is to transmit the SourceLocation to the ReportViolation
function. I think I can do another CL that will fix the issue.
https://codereview.chromium.org/2761153003/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/FlagExpectations/enable-browser-side-navigation:19:
Bug(none)
http/tests/security/contentSecurityPolicy/1.1/child-src/frame-redirect-blocked.html
[ Failure ]
On 2017/03/29 23:27:32, alexmos wrote:
> I thought the expectations in this file are only applied when running with
> --enable-browser-side-navigation, so then I'm a little confused on why we
still
> need these?
The thing is that the current test expectations doesn't match what we get with
PlzNavigate. What we have with PlzNavigate is better. The test expectations
should be updated, but we can't, since the source location is missing without
PlzNavigate whenever there is a redirect.
It would be nice to be able to provide different test expectations for the
LayoutTest depending on the experiments that are running.
https://codereview.chromium.org/2761153003/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/nonces/style-enforce-blocked.php
(right):
https://codereview.chromium.org/2761153003/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/nonces/style-enforce-blocked.php:14:
assert_equals(e.lineNumber, 20);
On 2017/03/29 23:27:32, alexmos wrote:
> This doesn't seem related to this CL and is already changed on ToT. You
> probably need to rebase? Also looks like the other two tests doing this were
> deleted in r460353.
Yes, sorry for the confusion. I wanted to add
https://codereview.chromium.org/2785463002/
as dependent patchset, but apparently, it fails.
I will rebase.
arthursonzogni
The CQ bit was checked by arthursonzogni@chromium.org
3 years, 8 months ago
(2017-03-30 11:51:28 UTC)
#44
CQ is committing da patch. Bot data: {"patchset_id": 240001, "attempt_start_ts": 1490874688634510, "parent_rev": "e504e898649ac4eb5d9bbf371dd4e6c57034cb19", "commit_rev": "8bec3f2a2edf7217f83d10fe929816e698c64a35"}
3 years, 8 months ago
(2017-03-30 11:56:00 UTC)
#47
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1490874688634510,
"parent_rev": "e504e898649ac4eb5d9bbf371dd4e6c57034cb19", "commit_rev":
"8bec3f2a2edf7217f83d10fe929816e698c64a35"}
commit-bot: I haz the power
Description was changed from ========== PlzNavigate & CSP. Use the SourceLocation in violation reports. The ...
3 years, 8 months ago
(2017-03-30 11:56:50 UTC)
#48
Message was sent while issue was closed.
Description was changed from
==========
PlzNavigate & CSP. Use the SourceLocation in violation reports.
The SourceLocation struct is available during a navigation thanks to this CL:
https://codereview.chromium.org/2720763002
This patch makes use of it for CSP. It fixes several test where the line
number in console messages was missing.
BUG=690946,705098
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel
==========
to
==========
PlzNavigate & CSP. Use the SourceLocation in violation reports.
The SourceLocation struct is available during a navigation thanks to this CL:
https://codereview.chromium.org/2720763002
This patch makes use of it for CSP. It fixes several test where the line
number in console messages was missing.
BUG=690946,705098
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel
Review-Url: https://codereview.chromium.org/2761153003
Cr-Commit-Position: refs/heads/master@{#460727}
Committed:
https://chromium.googlesource.com/chromium/src/+/8bec3f2a2edf7217f83d10fe9298...
==========
commit-bot: I haz the power
Committed patchset #8 (id:240001) as https://chromium.googlesource.com/chromium/src/+/8bec3f2a2edf7217f83d10fe929816e698c64a35
3 years, 8 months ago
(2017-03-30 11:56:51 UTC)
#49
Issue 2761153003: PlzNavigate & CSP. Use the SourceLocation in violation reports.
(Closed)
Created 3 years, 9 months ago by arthursonzogni
Modified 3 years, 8 months ago
Reviewers: Mike West, clamy, alexmos
Base URL:
Comments: 11
Chromium Code Reviews
Issue
2761153003:
PlzNavigate & CSP. Use the SourceLocation in violation reports. (Closed)
