Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(186)

Issue 1917073002: Block webpages from navigating to view-source URLs (Closed)

Created:
4 years, 7 months ago by meacer
Modified:
4 years, 6 months ago
CC:
chromium-reviews, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, jam, darin-cc_chromium.org, Charlie Reis
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Block webpages from navigating to view-source URLs. This CL blocks navigations to view-source URLs initiated by a page via window.location, window.open, A tag, etc. It still allows user initiated navigations such as directly entering the URL, clicking "view page source" or "open in new tab" in the context menu. BUG=247151, 606619 Committed: https://crrev.com/ce6b6603637ee045041ccb49359fbae617d84ba5 Cr-Commit-Position: refs/heads/master@{#397505}

Patch Set 1 #

Total comments: 3

Patch Set 2 : Fix ChildProcessSecurityPolicy and tests #

Total comments: 12

Patch Set 3 : Fix unit test #

Total comments: 2

Patch Set 4 : Add layout and browser tests #

Total comments: 27

Patch Set 5 : creis comments #

Patch Set 6 : Minor comment fix #

Patch Set 7 : Delete layout tests #

Unified diffs Side-by-side diffs Delta from patch set Stats (+227 lines, -105 lines) Patch
M content/browser/browser_side_navigation_browsertest.cc View 1 2 3 4 5 2 chunks +45 lines, -0 lines 0 comments Download
M content/browser/child_process_security_policy_impl.cc View 1 2 chunks +6 lines, -28 lines 0 comments Download
M content/browser/child_process_security_policy_unittest.cc View 1 3 chunks +11 lines, -16 lines 0 comments Download
M content/browser/site_per_process_browsertest.cc View 1 2 3 4 1 chunk +0 lines, -52 lines 0 comments Download
M content/browser/web_contents/web_contents_impl_browsertest.cc View 1 2 3 4 1 chunk +78 lines, -0 lines 0 comments Download
M content/browser/web_contents/web_contents_impl_unittest.cc View 1 2 3 4 1 chunk +16 lines, -9 lines 0 comments Download
M content/public/test/content_browser_test_utils.h View 1 2 3 4 3 chunks +33 lines, -0 lines 0 comments Download
M content/public/test/content_browser_test_utils.cc View 1 2 3 4 2 chunks +29 lines, -0 lines 0 comments Download
M content/renderer/render_thread_impl.cc View 1 2 3 4 1 chunk +4 lines, -0 lines 0 comments Download
M content/test/data/simple_links.html View 1 2 3 4 1 chunk +5 lines, -0 lines 0 comments Download

Messages

Total messages: 29 (11 generated)
meacer
Charlie, can you PTAL? Thanks.
4 years, 7 months ago (2016-04-26 17:45:18 UTC) #4
Charlie Reis
Thanks for putting something together. Since this is a web-visible change, I think we should ...
4 years, 7 months ago (2016-04-27 21:47:03 UTC) #6
meacer
Thanks Charlie, modified some tests but will look into adding more. > The current fix ...
4 years, 7 months ago (2016-05-12 01:14:16 UTC) #7
Charlie Reis
Sorry for the delay reviewing this-- I got hit with an unusually large number of ...
4 years, 7 months ago (2016-05-16 22:22:18 UTC) #8
meacer
> Sorry for the delay reviewing this-- I got hit with an unusually large number ...
4 years, 7 months ago (2016-05-19 01:13:55 UTC) #9
Charlie Reis
On 2016/05/19 01:13:55, Mustafa Emre Acer wrote: > > Sorry for the delay reviewing this-- ...
4 years, 7 months ago (2016-05-23 22:32:16 UTC) #10
meacer
Thanks Charlie, added some more tests. https://codereview.chromium.org/1917073002/diff/20001/content/browser/web_contents/web_contents_impl_unittest.cc File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/1917073002/diff/20001/content/browser/web_contents/web_contents_impl_unittest.cc#newcode417 content/browser/web_contents/web_contents_impl_unittest.cc:417: cont.LoadURL(kGURL, Referrer(), ui::PAGE_TRANSITION_LINK, ...
4 years, 7 months ago (2016-05-23 23:57:47 UTC) #11
Charlie Reis
Thanks! content/ LGTM with nits. +japhet@ for layout test. https://codereview.chromium.org/1917073002/diff/60001/content/browser/browser_side_navigation_browsertest.cc File content/browser/browser_side_navigation_browsertest.cc (right): https://codereview.chromium.org/1917073002/diff/60001/content/browser/browser_side_navigation_browsertest.cc#newcode241 content/browser/browser_side_navigation_browsertest.cc:241: ...
4 years, 7 months ago (2016-05-24 22:54:17 UTC) #13
Nate Chapin
https://codereview.chromium.org/1917073002/diff/60001/third_party/WebKit/LayoutTests/http/tests/security/view-source-navigation-block.html File third_party/WebKit/LayoutTests/http/tests/security/view-source-navigation-block.html (right): https://codereview.chromium.org/1917073002/diff/60001/third_party/WebKit/LayoutTests/http/tests/security/view-source-navigation-block.html#newcode9 third_party/WebKit/LayoutTests/http/tests/security/view-source-navigation-block.html:9: testRunner.notifyDone(); Nit: if (window.testRunner) https://codereview.chromium.org/1917073002/diff/60001/third_party/WebKit/LayoutTests/http/tests/security/view-source-navigation-block.html#newcode18 third_party/WebKit/LayoutTests/http/tests/security/view-source-navigation-block.html:18: }, 1000); On ...
4 years, 6 months ago (2016-05-27 23:57:32 UTC) #14
meacer
https://codereview.chromium.org/1917073002/diff/60001/content/browser/browser_side_navigation_browsertest.cc File content/browser/browser_side_navigation_browsertest.cc (right): https://codereview.chromium.org/1917073002/diff/60001/content/browser/browser_side_navigation_browsertest.cc#newcode241 content/browser/browser_side_navigation_browsertest.cc:241: } On 2016/05/24 22:54:16, Charlie Reis wrote: > Thanks ...
4 years, 6 months ago (2016-05-31 23:57:55 UTC) #15
Charlie Reis
Thanks for the updates! LGTM, and I think it might be safe to land without ...
4 years, 6 months ago (2016-06-01 22:59:51 UTC) #16
Nate Chapin
On 2016/06/01 22:59:51, Charlie Reis wrote: > Thanks for the updates! LGTM, and I think ...
4 years, 6 months ago (2016-06-01 23:09:54 UTC) #17
meacer
On 2016/06/01 23:09:54, Nate Chapin wrote: > On 2016/06/01 22:59:51, Charlie Reis wrote: > > ...
4 years, 6 months ago (2016-06-01 23:47:36 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1917073002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1917073002/120001
4 years, 6 months ago (2016-06-02 00:05:06 UTC) #21
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/239291)
4 years, 6 months ago (2016-06-02 02:04:15 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1917073002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1917073002/120001
4 years, 6 months ago (2016-06-02 18:45:25 UTC) #25
commit-bot: I haz the power
Committed patchset #7 (id:120001)
4 years, 6 months ago (2016-06-02 20:56:21 UTC) #27
commit-bot: I haz the power
4 years, 6 months ago (2016-06-02 20:58:49 UTC) #29
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/ce6b6603637ee045041ccb49359fbae617d84ba5
Cr-Commit-Position: refs/heads/master@{#397505}

Powered by Google App Engine
This is Rietveld 408576698