Allow universal access from file if flag is set and url is file scheme.

Allow universal access from file if flag is set and url is file scheme.

BUG=449075
TEST=content_unittests --gtest_filter=NavigationControllerTest.IsInPageNavigation

Committed: https://crrev.com/3d426436e10135a99f5e54fe1135767eb55712db
Cr-Commit-Position: refs/heads/master@{#313051}

[nhu, 2015-01-19 09:41:58]

ningxin.hu@intel.com changed reviewers:
+ jam@chromium.org

[nhu, 2015-01-19 09:41:59]

Please take a look. Thanks!

[nhu, 2015-01-20 00:34:36]

ningxin.hu@intel.com changed reviewers:
+ darin@chromium.org

[nhu, 2015-01-20 00:34:36]

Ping jam or darin. Could you please take a look?

[jam, 2015-01-20 17:59:23]

jam@chromium.org changed reviewers:
+ creis@chromium.org
- darin@chromium.org

[jam, 2015-01-20 17:59:23]

Charlie would be a better review than me.

[nhu, 2015-01-21 00:29:58]

Thanks, jam!

Charlie, could you please take a look at this CL? Thanks!

[Charlie Reis, 2015-01-21 19:49:16]

Is this for use in test_shell?  I noticed this comment in render_view_impl.cc:

  // By default, allow_universal_access_from_file_urls is set to false and thus
  // we mitigate attacks from local HTML files by not granting file:// URLs
  // universal access. Only test shell will enable this.

I'd be concerned about adding broader support for
allow_universal_access_from_file_urls outside test shell, since it's quite
insecure.

If it's just for test shell, then this is probably fine.  Can you add tests for
this case (with and without the switch) in
navigation_controller_impl_unittest.cc, near IsInPageNavigation?  That test
shows how to watch for the kill, and you can use
base::CommandLine::ForCurrentProcess()->AppendSwitch to trigger the mode that
allows it.  That will help us track why this is necessary.

[nhu, 2015-01-22 02:51:15]

On 2015/01/21 19:49:16, Charlie Reis wrote:
> Is this for use in test_shell?  I noticed this comment in render_view_impl.cc:
> 
>   // By default, allow_universal_access_from_file_urls is set to false and
thus
>   // we mitigate attacks from local HTML files by not granting file:// URLs
>   // universal access. Only test shell will enable this.
> 
> I'd be concerned about adding broader support for
> allow_universal_access_from_file_urls outside test shell, since it's quite
> insecure.
> 

Content shell testing is one use case.
Other content embedders (e.g. Crosswalk) allow uses to set
allow_universal_access_from_file_urls for some use cases at their own risk.

This patch doesn't add broader support of this flag into Chromium/Chrome.

> If it's just for test shell, then this is probably fine.  Can you add tests
for
> this case (with and without the switch) in
> navigation_controller_impl_unittest.cc, near IsInPageNavigation?  That test
> shows how to watch for the kill, and you can use
> base::CommandLine::ForCurrentProcess()->AppendSwitch to trigger the mode that
> allows it.  That will help us track why this is necessary.

Thanks for your suggestions. The unit tests have been added. Please take a look.
Thanks!

[nhu, 2015-01-22 02:51:47]

Uploaded patch set 2. Please take another look. Thanks!

[Charlie Reis, 2015-01-23 06:53:37]

Thanks for adding the tests.  Just a few comments.

https://codereview.chromium.org/855883002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/855883002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl.cc:134:
existing_url.SchemeIs(url::kFileScheme)) ;
nit: No space before semicolon.

https://codereview.chromium.org/855883002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/855883002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_unittest.cc:3209: // Test
allow_universal_access_from_file_urls flag
nit: End with period.

https://codereview.chromium.org/855883002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_unittest.cc:3211:
main_test_rfh()->GetRenderViewHost()->GetWebkitPreferences();
test_rvh()  (here and below)

https://codereview.chromium.org/855883002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_unittest.cc:3218: // Don't
hornor allow_universal_access_from_file_urls if existing URL is
honor

https://codereview.chromium.org/855883002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_unittest.cc:3224: // Allow
in page navigation if existing URL is file scheme.
Move this case above the bad_msg_count case, so that we can ensure that
bad_msg_count is 0 when it works.

[nhu, 2015-01-23 13:41:13]

Thanks for review. Address the issues by uploading a new patch set. Please take
another look. Thanks!

https://codereview.chromium.org/855883002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/855883002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl.cc:134:
existing_url.SchemeIs(url::kFileScheme)) ;
On 2015/01/23 06:53:37, Charlie Reis (slow until 1-29) wrote:
> nit: No space before semicolon.

Done.

https://codereview.chromium.org/855883002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/855883002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_unittest.cc:3209: // Test
allow_universal_access_from_file_urls flag
On 2015/01/23 06:53:37, Charlie Reis (slow until 1-29) wrote:
> nit: End with period.

Done.

https://codereview.chromium.org/855883002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_unittest.cc:3211:
main_test_rfh()->GetRenderViewHost()->GetWebkitPreferences();
On 2015/01/23 06:53:37, Charlie Reis (slow until 1-29) wrote:
> test_rvh()  (here and below)

Done.

https://codereview.chromium.org/855883002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_unittest.cc:3218: // Don't
hornor allow_universal_access_from_file_urls if existing URL is
On 2015/01/23 06:53:37, Charlie Reis (slow until 1-29) wrote:
> honor

Done.

https://codereview.chromium.org/855883002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_unittest.cc:3224: // Allow
in page navigation if existing URL is file scheme.
On 2015/01/23 06:53:37, Charlie Reis (slow until 1-29) wrote:
> Move this case above the bad_msg_count case, so that we can ensure that
> bad_msg_count is 0 when it works.

It makes sense. I reordered them. Does it look better?

[Charlie Reis, 2015-01-23 22:58:30]

Thanks, LGTM with nits.

https://codereview.chromium.org/855883002/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/855883002/diff/40001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_unittest.cc:3207:
EXPECT_EQ(true, prefs.allow_universal_access_from_file_urls);
nit: EXPECT_TRUE

https://codereview.chromium.org/855883002/diff/40001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_unittest.cc:3226:
EXPECT_EQ(false, prefs.allow_universal_access_from_file_urls);
nit: EXPECT_FALSE

[nhu, 2015-01-26 01:11:25]

Thanks for the review! Landed a new patch set with nits fixed. Please take
another look.

https://codereview.chromium.org/855883002/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_impl_unittest.cc (right):

https://codereview.chromium.org/855883002/diff/40001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_unittest.cc:3207:
EXPECT_EQ(true, prefs.allow_universal_access_from_file_urls);
On 2015/01/23 22:58:30, Charlie Reis (slow until 1-29) wrote:
> nit: EXPECT_TRUE

Done.

https://codereview.chromium.org/855883002/diff/40001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl_unittest.cc:3226:
EXPECT_EQ(false, prefs.allow_universal_access_from_file_urls);
On 2015/01/23 22:58:30, Charlie Reis (slow until 1-29) wrote:
> nit: EXPECT_FALSE

Done.

[Charlie Reis, 2015-01-26 01:16:50]

Thanks, still LGTM.

[nhu, 2015-01-26 04:59:01]

Thanks for review. Could you please help land?

[Charlie Reis, 2015-01-26 07:37:40]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2015-01-26 07:38:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/855883002/60001

[commit-bot: I haz the power, 2015-01-26 09:11:25]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-01-26 09:12:35]

Patchset 4 (id:??) landed as
https://crrev.com/3d426436e10135a99f5e54fe1135767eb55712db
Cr-Commit-Position: refs/heads/master@{#313051}