Change wildcard source expression matching to conform latest spec

Change wildcard source expression matching to conform latest spec

This patch changes wildcard source expression matching to require
schemes other than http/https/ws/wss be explicitly present
in the source list to match, per
https://w3c.github.io/webappsec-csp/#match-url-to-source-expression

This also updates CSP injected in inline_login_ui.cc to explicitly allow
`chrome:` as in `connect-src * chrome:` to fix failing tests.

This also changes Content Security Policy in manifests of Feedback and
PDF Viewer extensions to allow objects from `file:`.

BUG=611314
R=mkwst@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/39d12f5665910578d2e18251a0225a7aebe6964b
Cr-Commit-Position: refs/heads/master@{#410566}

[Sergey Shekyan, 2016-08-04 06:19:09]

Description was changed from

==========
Change wildcard source expression matching to conform latest spec

This patch changes wildcard source expression matching to require
schemes other than http/https/ws/wss be explicitly present
in the source list to match, per
https://w3c.github.io/webappsec-csp/#match-url-to-source-expression

This also updates CSP injected in inline_login_ui.cc to explicitly allow
`chrome:` as in `connect-src * chrome:` to fix failing tests.

This also changes Content Security Policy in manifests of Feedback and
PDF Viewer extensions to allow objects from `file:`.

BUG=611314
R=mkwst@chromium.org
==========

to

==========
Change wildcard source expression matching to conform latest spec

This patch changes wildcard source expression matching to require
schemes other than http/https/ws/wss be explicitly present
in the source list to match, per
https://w3c.github.io/webappsec-csp/#match-url-to-source-expression

This also updates CSP injected in inline_login_ui.cc to explicitly allow
`chrome:` as in `connect-src * chrome:` to fix failing tests.

This also changes Content Security Policy in manifests of Feedback and
PDF Viewer extensions to allow objects from `file:`.

BUG=611314
R=mkwst@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Sergey Shekyan, 2016-08-04 06:23:50]

The CQ bit was checked by shekyan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-04 06:24:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sergey Shekyan, 2016-08-04 06:24:51]

Mike, I'd be happy to write tests for extensions, but can you please suggest on
where is it better be tested?

[commit-bot: I haz the power, 2016-08-04 07:38:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-04 07:38:12]

Dry run: This issue passed the CQ dry run.

[Mike West, 2016-08-04 11:23:59]

mkwst@chromium.org changed reviewers:
+ jam@chromium.org, jochen@chromium.org

[Mike West, 2016-08-04 11:24:00]

+jochen@, jam@, who might have a good idea where to land a test for PDFium.

Relanding the actual change LGTM.

[jam, 2016-08-04 18:12:45]

Description was changed from

==========
Change wildcard source expression matching to conform latest spec

This patch changes wildcard source expression matching to require
schemes other than http/https/ws/wss be explicitly present
in the source list to match, per
https://w3c.github.io/webappsec-csp/#match-url-to-source-expression

This also updates CSP injected in inline_login_ui.cc to explicitly allow
`chrome:` as in `connect-src * chrome:` to fix failing tests.

This also changes Content Security Policy in manifests of Feedback and
PDF Viewer extensions to allow objects from `file:`.

BUG=611314
R=mkwst@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Change wildcard source expression matching to conform latest spec

This patch changes wildcard source expression matching to require
schemes other than http/https/ws/wss be explicitly present
in the source list to match, per
https://w3c.github.io/webappsec-csp/#match-url-to-source-expression

This also updates CSP injected in inline_login_ui.cc to explicitly allow
`chrome:` as in `connect-src * chrome:` to fix failing tests.

This also changes Content Security Policy in manifests of Feedback and
PDF Viewer extensions to allow objects from `file:`.

BUG=611314
R=mkwst@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[jam, 2016-08-04 18:12:45]

jam@chromium.org changed reviewers:
+ thestig@chromium.org
- jam@chromium.org

[jam, 2016-08-04 18:13:15]

On 2016/08/04 11:24:00, Mike West wrote:
> +jochen@, jam@, who might have a good idea where to land a test for PDFium.

Adding thestig.

I'm not sure what kind of test you're looking for (i.e. unit? browser_test?) so
hard to say.

> 
> Relanding the actual change LGTM.

[Lei Zhang, 2016-08-04 18:21:17]

On 2016/08/04 18:13:15, jam wrote:
> On 2016/08/04 11:24:00, Mike West wrote:
> > +jochen@, jam@, who might have a good idea where to land a test for PDFium.
> 
> I'm not sure what kind of test you're looking for (i.e. unit? browser_test?)
so
> hard to say.

I'm guessing you want a test for the Chrome PDF Viewer to load a PDF off disk?
If yes, try chrome/browser/pdf/pdf_extension_test.cc. Right now all the test
cases there are loading PDFs off disk indirectly via an EmbeddedTestServer.
Should be easy to add a new test case to load off disk directly.

[Lei Zhang, 2016-08-04 18:22:30]

BTW, for future CLs - when relanding, upload the reverted CL as patch set 1, and
the new CL as patch set 2. Then reviewers can easily see the difference.

[jochen (gone - plz use gerrit), 2016-08-05 11:46:07]

lgtm

[Lei Zhang, 2016-08-05 19:04:06]

On 2016/08/04 18:21:17, Lei Zhang wrote:
> I'm guessing you want a test for the Chrome PDF Viewer to load a PDF off disk?
> If yes, try chrome/browser/pdf/pdf_extension_test.cc. Right now all the test
> cases there are loading PDFs off disk indirectly via an EmbeddedTestServer.
> Should be easy to add a new test case to load off disk directly.

And If you would like me to write a load PDF off disk test, please say so.

[Sergey Shekyan, 2016-08-05 19:34:04]

On 2016/08/05 19:04:06, Lei Zhang wrote:
> On 2016/08/04 18:21:17, Lei Zhang wrote:
> > I'm guessing you want a test for the Chrome PDF Viewer to load a PDF off
disk?
> > If yes, try chrome/browser/pdf/pdf_extension_test.cc. Right now all the test
> > cases there are loading PDFs off disk indirectly via an EmbeddedTestServer.
> > Should be easy to add a new test case to load off disk directly.
> 
> And If you would like me to write a load PDF off disk test, please say so.

Thank you for suggestions and the advice. I am traveling now and will be back
tomorrow and hope to write tests down tomorrow. 
I'll let you know if I am stuck.

[Sergey Shekyan, 2016-08-08 17:37:36]

The CQ bit was checked by shekyan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-08 17:38:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sergey Shekyan, 2016-08-08 17:45:24]

I don't think there are any tests around feedback component.

[commit-bot: I haz the power, 2016-08-08 18:55:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-08 18:55:29]

Dry run: This issue passed the CQ dry run.

[Sergey Shekyan, 2016-08-08 23:13:02]

On 2016/08/04 18:21:17, Lei Zhang wrote:
> On 2016/08/04 18:13:15, jam wrote:
> > On 2016/08/04 11:24:00, Mike West wrote:
> > > +jochen@, jam@, who might have a good idea where to land a test for
PDFium.
> > 
> > I'm not sure what kind of test you're looking for (i.e. unit? browser_test?)
> so
> > hard to say.
> 
> I'm guessing you want a test for the Chrome PDF Viewer to load a PDF off disk?
> If yes, try chrome/browser/pdf/pdf_extension_test.cc. Right now all the test
> cases there are loading PDFs off disk indirectly via an EmbeddedTestServer.
> Should be easy to add a new test case to load off disk directly.

Added the test for loading PDF from 'file:'.

[Lei Zhang, 2016-08-08 23:16:25]

Thanks for adding the test.

https://codereview.chromium.org/2209113002/diff/40001/chrome/browser/pdf/pdf_...
File chrome/browser/pdf/pdf_extension_test.cc (right):

https://codereview.chromium.org/2209113002/diff/40001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:470:
ASSERT_TRUE(PathService::Get(base::DIR_SOURCE_ROOT, &test_dir));
You can do PathService::Get(chrome::DIR_TEST_DATA, ...) and skip the next 3
lines.

https://codereview.chromium.org/2209113002/diff/40001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:475: test_dir =
test_dir.AppendASCII("test.pdf");
Please sanity check and make sure the final FilePath exists.

[Sergey Shekyan, 2016-08-08 23:55:42]

https://codereview.chromium.org/2209113002/diff/40001/chrome/browser/pdf/pdf_...
File chrome/browser/pdf/pdf_extension_test.cc (right):

https://codereview.chromium.org/2209113002/diff/40001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:470:
ASSERT_TRUE(PathService::Get(base::DIR_SOURCE_ROOT, &test_dir));
On 2016/08/08 23:16:24, Lei Zhang wrote:
> You can do PathService::Get(chrome::DIR_TEST_DATA, ...) and skip the next 3
> lines.

I assumed so too, but unfortunately, path set by
PathService::Get(chrome::DIR_TEST_DATA, ...) is like
`file:///path_to_my_chromium_source_code/src/base/test/data/pdf/test.pdf`, where
`base` doesn't exist.

What would you suggest?

https://codereview.chromium.org/2209113002/diff/40001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:475: test_dir =
test_dir.AppendASCII("test.pdf");
On 2016/08/08 23:16:24, Lei Zhang wrote:
> Please sanity check and make sure the final FilePath exists.

Acknowledged.

[Lei Zhang, 2016-08-08 23:57:16]

https://codereview.chromium.org/2209113002/diff/40001/chrome/browser/pdf/pdf_...
File chrome/browser/pdf/pdf_extension_test.cc (right):

https://codereview.chromium.org/2209113002/diff/40001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:470:
ASSERT_TRUE(PathService::Get(base::DIR_SOURCE_ROOT, &test_dir));
On 2016/08/08 23:55:42, Sergey Shekyan wrote:
> On 2016/08/08 23:16:24, Lei Zhang wrote:
> > You can do PathService::Get(chrome::DIR_TEST_DATA, ...) and skip the next 3
> > lines.
> 
> I assumed so too, but unfortunately, path set by
> PathService::Get(chrome::DIR_TEST_DATA, ...) is like
> `file:///path_to_my_chromium_source_code/src/base/test/data/pdf/test.pdf`,
where
> `base` doesn't exist.
> 
> What would you suggest?

Really? Because we use just that in several places above. Did you putin
base::DIR_TEST_DATA instead of chrome::DIR_TEST_DATA?

[Lei Zhang, 2016-08-08 23:57:37]

On 2016/08/08 23:57:16, Lei Zhang wrote:
> Really? Because we use just that in several places above. Did you putin
> base::DIR_TEST_DATA instead of chrome::DIR_TEST_DATA?

s/putin/put in/ :)

[Sergey Shekyan, 2016-08-09 00:00:22]

On 2016/08/08 23:57:37, Lei Zhang wrote:
> On 2016/08/08 23:57:16, Lei Zhang wrote:
> > Really? Because we use just that in several places above. Did you putin
> > base::DIR_TEST_DATA instead of chrome::DIR_TEST_DATA?
> 
> s/putin/put in/ :)

You are right, I missed the `chrome::` part and used `base::` .

[Lei Zhang, 2016-08-09 00:02:20]

On 2016/08/09 00:00:22, Sergey Shekyan wrote:
> You are right, I missed the `chrome::` part and used `base::` .

Great. Looking forward to patch set 4.

[Sergey Shekyan, 2016-08-09 00:07:04]

On 2016/08/09 00:02:20, Lei Zhang wrote:
> On 2016/08/09 00:00:22, Sergey Shekyan wrote:
> > You are right, I missed the `chrome::` part and used `base::` .
> 
> Great. Looking forward to patch set 4.

4 and 5 :)

[Sergey Shekyan, 2016-08-09 00:28:38]

On 2016/08/09 00:07:04, Sergey Shekyan wrote:
> On 2016/08/09 00:02:20, Lei Zhang wrote:
> > On 2016/08/09 00:00:22, Sergey Shekyan wrote:
> > > You are right, I missed the `chrome::` part and used `base::` .
> > 
> > Great. Looking forward to patch set 4.
> 
> 4 and 5 :)
In patch set 5, I renamed `test_dir` to `test_data_dir` to be consistent.

[Lei Zhang, 2016-08-09 00:28:45]

Just double checking locally to make sure the new PDF test fails when the
reverted CL is applied.

https://codereview.chromium.org/2209113002/diff/80001/chrome/browser/pdf/pdf_...
File chrome/browser/pdf/pdf_extension_test.cc (right):

https://codereview.chromium.org/2209113002/diff/80001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:472: test_data_dir =
test_data_dir.AppendASCII("test.pdf");
Maybe make a new base::FilePath test_data_file variable here, so the 2 lines
below can refer to test_data_file and it reads a little bit better.

[Lei Zhang, 2016-08-09 00:32:39]

On 2016/08/09 00:28:45, Lei Zhang wrote:
> Just double checking locally to make sure the new PDF test fails when the
> reverted CL is applied.

Verified locally -> new test case LGTM.

[Sergey Shekyan, 2016-08-09 00:38:28]

https://codereview.chromium.org/2209113002/diff/80001/chrome/browser/pdf/pdf_...
File chrome/browser/pdf/pdf_extension_test.cc (right):

https://codereview.chromium.org/2209113002/diff/80001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:472: test_data_dir =
test_data_dir.AppendASCII("test.pdf");
On 2016/08/09 00:28:45, Lei Zhang wrote:
> Maybe make a new base::FilePath test_data_file variable here, so the 2 lines
> below can refer to test_data_file and it reads a little bit better.

Acknowledged.

https://codereview.chromium.org/2209113002/diff/80001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:472: test_data_dir =
test_data_dir.AppendASCII("test.pdf");
On 2016/08/09 00:28:45, Lei Zhang wrote:
> Maybe make a new base::FilePath test_data_file variable here, so the 2 lines
> below can refer to test_data_file and it reads a little bit better.

Done.

[Sergey Shekyan, 2016-08-09 00:39:31]

The CQ bit was checked by shekyan@gmail.com

[Sergey Shekyan, 2016-08-09 00:39:31]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
mkwst@chromium.org, thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2209113002/#ps100001
(title: "Addressed comment on introducing `test_data_file`")

[commit-bot: I haz the power, 2016-08-09 00:40:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 00:48:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 00:48:35]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[Sergey Shekyan, 2016-08-09 03:15:27]

The CQ bit was checked by shekyan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 03:15:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 03:32:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 03:32:23]

Dry run: This issue passed the CQ dry run.

[Sergey Shekyan, 2016-08-09 03:33:02]

The CQ bit was checked by shekyan@gmail.com

[commit-bot: I haz the power, 2016-08-09 03:33:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 03:39:24]

Description was changed from

==========
Change wildcard source expression matching to conform latest spec

This patch changes wildcard source expression matching to require
schemes other than http/https/ws/wss be explicitly present
in the source list to match, per
https://w3c.github.io/webappsec-csp/#match-url-to-source-expression

This also updates CSP injected in inline_login_ui.cc to explicitly allow
`chrome:` as in `connect-src * chrome:` to fix failing tests.

This also changes Content Security Policy in manifests of Feedback and
PDF Viewer extensions to allow objects from `file:`.

BUG=611314
R=mkwst@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Change wildcard source expression matching to conform latest spec

This patch changes wildcard source expression matching to require
schemes other than http/https/ws/wss be explicitly present
in the source list to match, per
https://w3c.github.io/webappsec-csp/#match-url-to-source-expression

This also updates CSP injected in inline_login_ui.cc to explicitly allow
`chrome:` as in `connect-src * chrome:` to fix failing tests.

This also changes Content Security Policy in manifests of Feedback and
PDF Viewer extensions to allow objects from `file:`.

BUG=611314
R=mkwst@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-08-09 03:39:26]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-09 03:42:03]

Description was changed from

==========
Change wildcard source expression matching to conform latest spec

This patch changes wildcard source expression matching to require
schemes other than http/https/ws/wss be explicitly present
in the source list to match, per
https://w3c.github.io/webappsec-csp/#match-url-to-source-expression

This also updates CSP injected in inline_login_ui.cc to explicitly allow
`chrome:` as in `connect-src * chrome:` to fix failing tests.

This also changes Content Security Policy in manifests of Feedback and
PDF Viewer extensions to allow objects from `file:`.

BUG=611314
R=mkwst@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Change wildcard source expression matching to conform latest spec

This patch changes wildcard source expression matching to require
schemes other than http/https/ws/wss be explicitly present
in the source list to match, per
https://w3c.github.io/webappsec-csp/#match-url-to-source-expression

This also updates CSP injected in inline_login_ui.cc to explicitly allow
`chrome:` as in `connect-src * chrome:` to fix failing tests.

This also changes Content Security Policy in manifests of Feedback and
PDF Viewer extensions to allow objects from `file:`.

BUG=611314
R=mkwst@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/39d12f5665910578d2e18251a0225a7aebe6964b
Cr-Commit-Position: refs/heads/master@{#410566}
==========

[commit-bot: I haz the power, 2016-08-09 03:42:05]

Patchset 6 (id:??) landed as
https://crrev.com/39d12f5665910578d2e18251a0225a7aebe6964b
Cr-Commit-Position: refs/heads/master@{#410566}

[Sergey Shekyan, 2016-08-10 17:03:58]

Description was changed from

==========
Change wildcard source expression matching to conform latest spec

This patch changes wildcard source expression matching to require
schemes other than http/https/ws/wss be explicitly present
in the source list to match, per
https://w3c.github.io/webappsec-csp/#match-url-to-source-expression

This also updates CSP injected in inline_login_ui.cc to explicitly allow
`chrome:` as in `connect-src * chrome:` to fix failing tests.

This also changes Content Security Policy in manifests of Feedback and
PDF Viewer extensions to allow objects from `file:`.

BUG=611314
R=mkwst@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/39d12f5665910578d2e18251a0225a7aebe6964b
Cr-Commit-Position: refs/heads/master@{#410566}
==========

to

==========
Change wildcard source expression matching to conform latest spec

This patch changes wildcard source expression matching to require
schemes other than http/https/ws/wss be explicitly present
in the source list to match, per
https://w3c.github.io/webappsec-csp/#match-url-to-source-expression

This also updates CSP injected in inline_login_ui.cc to explicitly allow
`chrome:` as in `connect-src * chrome:` to fix failing tests.

This also changes Content Security Policy in manifests of Feedback and
PDF Viewer extensions to allow objects from `file:`.

BUG=611314
R=mkwst@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/39d12f5665910578d2e18251a0225a7aebe6964b
Cr-Commit-Position: refs/heads/master@{#410566}
==========

[Sergey Shekyan, 2016-08-10 17:03:58]

shekyan@gmail.com changed reviewers:
+ jww@chromium.org

[Sergey Shekyan, 2016-08-10 17:05:06]

On 2016/08/09 03:42:05, commit-bot: I haz the power wrote:
> Patchset 6 (id:??) landed as
> https://crrev.com/39d12f5665910578d2e18251a0225a7aebe6964b
> Cr-Commit-Position: refs/heads/master@{#410566}

jww@, all other reviewers are OOO, can you please take a look? I am not sure if
I can update already landed CL, or I need to revert and land again with the last
patch set.

[Lei Zhang, 2016-08-10 17:12:51]

On 2016/08/10 17:05:06, Sergey Shekyan wrote:
> On 2016/08/09 03:42:05, commit-bot: I haz the power wrote:
> > Patchset 6 (id:??) landed as
> > https://crrev.com/39d12f5665910578d2e18251a0225a7aebe6964b
> > Cr-Commit-Position: refs/heads/master@{#410566}
> 
> jww@, all other reviewers are OOO, can you please take a look? I am not sure
if
> I can update already landed CL, or I need to revert and land again with the
last
> patch set.

Just start a new CL for the changes in patch set 7.

[jww, 2016-08-10 19:12:10]

On 2016/08/10 17:12:51, Lei Zhang (Soon to be OOO) wrote:
> On 2016/08/10 17:05:06, Sergey Shekyan wrote:
> > On 2016/08/09 03:42:05, commit-bot: I haz the power wrote:
> > > Patchset 6 (id:??) landed as
> > > https://crrev.com/39d12f5665910578d2e18251a0225a7aebe6964b
> > > Cr-Commit-Position: refs/heads/master@{#410566}
> > 
> > jww@, all other reviewers are OOO, can you please take a look? I am not sure
> if
> > I can update already landed CL, or I need to revert and land again with the
> last
> > patch set.
> 
> Just start a new CL for the changes in patch set 7.

Indeed, as thestig@ says, that's the best approach. Happy to take a look at that
patch set once it's up.