|
|
Created:
5 years, 11 months ago by nhu Modified:
5 years, 10 months ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow 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}
Patch Set 1 #Patch Set 2 : Add unit tests #
Total comments: 10
Patch Set 3 : fix coding sytle and reorder unit tests #
Total comments: 4
Patch Set 4 : fix nits #
Messages
Total messages: 20 (4 generated)
ningxin.hu@intel.com changed reviewers: + jam@chromium.org
Please take a look. Thanks!
ningxin.hu@intel.com changed reviewers: + darin@chromium.org
Ping jam or darin. Could you please take a look?
jam@chromium.org changed reviewers: + creis@chromium.org - darin@chromium.org
Charlie would be a better review than me.
Thanks, jam! Charlie, could you please take a look at this CL? Thanks!
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.
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!
Uploaded patch set 2. Please take another look. Thanks!
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.
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?
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
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.
Thanks, still LGTM.
Thanks for review. Could you please help land?
The CQ bit was checked by creis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/855883002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3d426436e10135a99f5e54fe1135767eb55712db Cr-Commit-Position: refs/heads/master@{#313051} |