Issue 2191143002: Opt-in stripping of line numbers from console messages of layout tests.

Created:
4 years, 4 months ago by Łukasz Anforowicz

Modified:
4 years, 4 months ago

Reviewers:
caseq, jochen (gone - plz use gerrit), Mike West

CC:
alexmos, blink-reviews, caseq, chromium-reviews, dgozman, jochen+watch_chromium.org, Mike West, mlamouri+watch-test-runner_chromium.org, site-isolation-reviews_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@csp-reporting-ipcs

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Opt-in stripping of line numbers from console messages of layout tests. BUG=611232

Patch Set 3 : Rebasing... #

Created: 4 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+34 lines, -8 lines)			Patch
M	components/test_runner/layout_test_runtime_flags.h	View		1 chunk	+4 lines, -0 lines	0 comments	Download
M	components/test_runner/layout_test_runtime_flags.cc	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	components/test_runner/test_runner.h	View		2 chunks	+5 lines, -0 lines	0 comments	Download
M	components/test_runner/test_runner.cc	View		5 chunks	+18 lines, -0 lines	0 comments	Download
M	components/test_runner/web_frame_test_client.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/LayoutTests/FlagExpectations/site-per-process	View	1 2	1 chunk	+0 lines, -3 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/frame-src-child-frame-navigates-to-blocked-origin.html	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/frame-src-child-frame-navigates-to-blocked-origin-expected.txt	View		1 chunk	+4 lines, -4 lines	0 comments	Download

Depends on Patchset:

Issue 2190183002 Patch 120001

Messages

Total messages: 11 (3 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Łukasz Anforowicz

+dgozman@ and caseq@ to CC as FYI, because this CL is slightly related to https://crbug.com/614413 ...

4 years, 4 months ago (2016-07-29 23:36:18 UTC) #3

caseq

https://codereview.chromium.org/2191143002/diff/20001/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/frame-src-child-frame-navigates-to-blocked-origin.html File third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/frame-src-child-frame-navigates-to-blocked-origin.html (right): https://codereview.chromium.org/2191143002/diff/20001/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/frame-src-child-frame-navigates-to-blocked-origin.html#newcode9 third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/frame-src-child-frame-navigates-to-blocked-origin.html:9: testRunner.stripLineNumbersFromConsoleMessages(); Can you elaborate why the line numbers vary ...

4 years, 4 months ago (2016-07-30 00:01:46 UTC) #5

Łukasz Anforowicz

4 years, 4 months ago (2016-07-30 00:36:24 UTC) #6

jochen (gone - plz use gerrit)

doesn't this mean that the new error message will make it more difficult for developers ...

4 years, 4 months ago (2016-08-01 12:15:33 UTC) #7

Łukasz Anforowicz

+site-isolation-reviews@ and +mkwst@ for interesting (IMO) discussion about some CSP scenarios disclosing line numbers cross-site.

4 years, 4 months ago (2016-08-01 18:10:17 UTC) #8

Łukasz Anforowicz

On 2016/08/01 12:15:33, jochen wrote: > doesn't this mean that the new error message will ...

4 years, 4 months ago (2016-08-01 18:13:31 UTC) #9

On 2016/08/01 12:15:33, jochen wrote:
> doesn't this mean that the new error message will make it more difficult for
> developers to figure out what went wrong?

Short answer is: yes - the error message without line numbers makes it indeed
more difficult.

The real answer is much longer though :-)  Let me try to split it up into
sections.

We are not discussing desirability of https://codereview.chromium.org/2190183002
================================================================================

Right now (i.e. before the current CL under review +
https://codereview.chromium.org/2190183002 land), when frame-src CSP violation
is detected at a time when OOPIFs are enabled and the child frame is in a
different process than the parent frame, then 1) the navigation is blocked, but
2) there is no console message, 3) there is no "securitypolicyviolation" event
raised in the parent document, 4) there is no report sent to the server
specified in "report-uri" CSP directive.  So,
1) https://codereview.chromium.org/2190183002 is a strict improvement (by fixing
2+3+4 except for the line numbers thing)
2) in other words - we just need to discuss whether loosing line numbers is
okay.

Line numbers in the securitypolicyviolation event
=================================================

Below I try to argue that we cannot possibly preserve "line numbers" (i.e.
sourceFile, lineNumber, columnNumber properties of SecurityPolicyViolationEvent
- https://www.w3.org/TR/CSP2/#securitypolicyviolationevent-interface) when
raising CSP violation event.

When child frame is navigated by setting window.location, then the "line
numbers" indicate a location within the child (e.g. line 7 in
third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/resources/frame-that-navigates-itself.html).

The spec says the user agent must "Fire a violation event at the protected
resource’s Document" (https://www.w3.org/TR/CSP2/#violation-reports).

But, (to enforce site isolation) we don't want to disclose "line numbers"
cross-origin (and this is what would happen when putting child's "line numbers"
into an event raised in a cross-site parent frame).

So - I'll drop line number from the IPC messages in
https://codereview.chromium.org/2190183002/ (no line number is desirable / by
design here).

Line numbers in the external report
===================================

This is still XSS IMO - we would report "line numbers" of a child frame to
"report-uri" controlled by a (cross-origin) parent frame.  Therefore, I think it
is desirable to loose line numbers in this case.

So - I'll drop line number from the IPC messages in
https://codereview.chromium.org/2190183002/ (no line number is desirable / by
design here).

Line numbers in the console message
===================================

We could possibly preserve line numbers here if we did the reporting using the
child frame.  This will still cause a difference between OOPIF-vs-nonOOPIF case
- a different console window will get the console message (so - the developer
inspecting a page for CSP violations can still miss some OOPIF-originated
messages if he/she looks at the usual console).  <irony>OTOH, we would pass the
layout test :-P</irony>

Additionally, ContentSecurityPolicy object that detects the violation doesn't
curently know which child frame caused the violation.  Not sure what to think of
trying to plumb through |childFrame| [aka |contextFrame|?] via
FrameLoader::shouldContinueForNavigationPolicy ->
ContentSecurityPolicy::allowChildFrameFromSource ->
CSPDirectiveList::allowChildFrameFromSource -> ... ->
ContentSecurityPolicy::reportViolation(... contextFrame ...).  I am not sure if
we also need to cover FrameFetchContext::canRequestInternal ... ->
ContentSecurityPolicy::allowChildFrameFromSource -> ... code path.

Value of regression testing
===========================

Finally, there is value in being able to enable
frame-src-child-frame-navigates-to-blocked-origin.html layout tests (value in
being able to detect regressions in OOPIF/site-per-process processing of the
scenario covered by this test).

At the same time, we would still have coverage for line numbers being present in
non-OOPIF scenarios (i.e. we would still have line numbers in
frame-src-blocked-expected.txt and frame-src-cross-origin-load-expected.txt in
"Refused to frame..." message).

testRunner.stripLineNumbersFromConsoleMessages is a gentler alternative to
testRunner.setDumpConsoleMessages
============================================================================================================

There are other (sometimes more clear-cut) cases when line numbers are
irrelevant in console messages.

1. Line numbers are a known source of flakiness - they sometimes don't appear in
layout test depending on the timing of a test (e.g. see
https://codereview.chromium.org/1882893002;  according to dcheng@ this is a
known, recurring source of flakiness)

2. In some OOPIF cases line numbers are stripped on purpose.

So - testRunner.stripLineNumbersFromConsoleMessages addition to test_runner is
valuable in general I think (OTOH, not sure about this CSP-specific case).

So - what do we do next? :-)
============================

I am not sure what to do next.  If you have suggestions - I am all ears :-)

Maybe we can keep the associated CSP bug open + add a TODO in the layout test?

jochen (gone - plz use gerrit)

4 years, 4 months ago (2016-08-02 15:24:53 UTC) #11

Marking Mike as reviewer to get his attention

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages