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

Issue 1362433002: Fix for "chrome://" links in PDFs. (Closed)

Created:
5 years, 3 months ago by paulmeyer
Modified:
5 years, 2 months ago
Reviewers:
Charlie Reis, raymes, Devlin
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, darin-cc_chromium.org, jam, extensions-reviews_chromium.org, not at google - send to devlin, Devlin
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

This patch implements a mechanism for more granular link URL permissions (filtering on scheme/host). This fixes the bug that allowed PDFs to have working links to any "chrome://" URLs. BUG=528505, 226927 Committed: https://crrev.com/1eefa26e1795192c5a347a1e1e7a99e88c47f9c4 Cr-Commit-Position: refs/heads/master@{#351705}

Patch Set 1 #

Total comments: 24

Patch Set 2 : Addressed comments by creis@. #

Patch Set 3 : Now using url::Origin. #

Total comments: 12

Patch Set 4 : Added tests. Addressed comments. #

Total comments: 17

Patch Set 5 : Addressed final nits. #

Patch Set 6 : Rebased. #

Patch Set 7 : Small fix. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+151 lines, -25 lines) Patch
M chrome/browser/extensions/chrome_extension_web_contents_observer.cc View 1 2 3 4 5 6 2 chunks +31 lines, -0 lines 0 comments Download
M chrome/browser/pdf/pdf_extension_test.cc View 1 2 3 4 5 4 chunks +38 lines, -8 lines 0 comments Download
M content/browser/child_process_security_policy_impl.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/child_process_security_policy_impl.cc View 1 2 6 chunks +32 lines, -4 lines 0 comments Download
M content/browser/child_process_security_policy_unittest.cc View 1 2 3 4 2 chunks +41 lines, -0 lines 0 comments Download
M content/public/browser/child_process_security_policy.h View 1 2 2 chunks +5 lines, -0 lines 0 comments Download
M content/public/common/url_constants.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M content/public/common/url_constants.cc View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M extensions/browser/extension_web_contents_observer.cc View 1 2 chunks +1 line, -13 lines 0 comments Download

Messages

Total messages: 41 (17 generated)
paulmeyer
+creis@ for general review.
5 years, 3 months ago (2015-09-21 15:08:04 UTC) #3
Charlie Reis
Sorry for the delay; yesterday was a deadline. Thanks for looking into this. It seems ...
5 years, 3 months ago (2015-09-22 17:38:16 UTC) #4
paulmeyer
I'll work on adding tests now. https://codereview.chromium.org/1362433002/diff/20001/chrome/browser/extensions/chrome_extension_web_contents_observer.cc File chrome/browser/extensions/chrome_extension_web_contents_observer.cc (right): https://codereview.chromium.org/1362433002/diff/20001/chrome/browser/extensions/chrome_extension_web_contents_observer.cc#newcode39 chrome/browser/extensions/chrome_extension_web_contents_observer.cc:39: // Some extensions ...
5 years, 3 months ago (2015-09-22 22:13:57 UTC) #6
Charlie Reis
Thanks. Just a few quick comments before I have to go, but I'll take another ...
5 years, 3 months ago (2015-09-23 00:07:15 UTC) #7
paulmeyer
https://codereview.chromium.org/1362433002/diff/20001/content/browser/child_process_security_policy_impl.cc File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/1362433002/diff/20001/content/browser/child_process_security_policy_impl.cc#newcode108 content/browser/child_process_security_policy_impl.cc:108: void RevokeSchemeHost(const std::string& scheme, const std::string& host) { On ...
5 years, 2 months ago (2015-09-23 17:03:54 UTC) #8
Charlie Reis
[+kalman,rdevlin.cronin to CC] nit: Let's also list bug 226927 in the CL description, since this ...
5 years, 2 months ago (2015-09-23 22:57:13 UTC) #9
Devlin
Looks pretty good, just a few nits. https://codereview.chromium.org/1362433002/diff/80001/chrome/browser/extensions/chrome_extension_web_contents_observer.cc File chrome/browser/extensions/chrome_extension_web_contents_observer.cc (right): https://codereview.chromium.org/1362433002/diff/80001/chrome/browser/extensions/chrome_extension_web_contents_observer.cc#newcode43 chrome/browser/extensions/chrome_extension_web_contents_observer.cc:43: Manifest::Type type ...
5 years, 2 months ago (2015-09-23 23:14:17 UTC) #11
paulmeyer
PTAL https://codereview.chromium.org/1362433002/diff/80001/chrome/browser/extensions/chrome_extension_web_contents_observer.cc File chrome/browser/extensions/chrome_extension_web_contents_observer.cc (right): https://codereview.chromium.org/1362433002/diff/80001/chrome/browser/extensions/chrome_extension_web_contents_observer.cc#newcode43 chrome/browser/extensions/chrome_extension_web_contents_observer.cc:43: Manifest::Type type = extension->GetType(); On 2015/09/23 23:14:17, Devlin ...
5 years, 2 months ago (2015-09-29 17:24:56 UTC) #14
paulmeyer
PTAL https://codereview.chromium.org/1362433002/diff/80001/chrome/browser/extensions/chrome_extension_web_contents_observer.cc File chrome/browser/extensions/chrome_extension_web_contents_observer.cc (right): https://codereview.chromium.org/1362433002/diff/80001/chrome/browser/extensions/chrome_extension_web_contents_observer.cc#newcode48 chrome/browser/extensions/chrome_extension_web_contents_observer.cc:48: if (type == Manifest::TYPE_EXTENSION && On 2015/09/23 23:14:17, ...
5 years, 2 months ago (2015-09-29 17:24:56 UTC) #15
Devlin
extensions lgtm https://codereview.chromium.org/1362433002/diff/140001/chrome/browser/extensions/chrome_extension_web_contents_observer.cc File chrome/browser/extensions/chrome_extension_web_contents_observer.cc (right): https://codereview.chromium.org/1362433002/diff/140001/chrome/browser/extensions/chrome_extension_web_contents_observer.cc#newcode52 chrome/browser/extensions/chrome_extension_web_contents_observer.cc:52: base::SStringPrintf(&origin_url, "%s://%s/", content::kChromeUIScheme, nit: Why SStringPrintf, instead ...
5 years, 2 months ago (2015-09-29 19:06:30 UTC) #16
Charlie Reis
Thanks for the tests! LGTM with nits. https://codereview.chromium.org/1362433002/diff/140001/chrome/browser/extensions/chrome_extension_web_contents_observer.cc File chrome/browser/extensions/chrome_extension_web_contents_observer.cc (right): https://codereview.chromium.org/1362433002/diff/140001/chrome/browser/extensions/chrome_extension_web_contents_observer.cc#newcode52 chrome/browser/extensions/chrome_extension_web_contents_observer.cc:52: base::SStringPrintf(&origin_url, "%s://%s/", ...
5 years, 2 months ago (2015-09-29 22:15:53 UTC) #17
paulmeyer
+raymes@ for OWNER review of pdf_extension_test.cc https://codereview.chromium.org/1362433002/diff/140001/chrome/browser/extensions/chrome_extension_web_contents_observer.cc File chrome/browser/extensions/chrome_extension_web_contents_observer.cc (right): https://codereview.chromium.org/1362433002/diff/140001/chrome/browser/extensions/chrome_extension_web_contents_observer.cc#newcode52 chrome/browser/extensions/chrome_extension_web_contents_observer.cc:52: base::SStringPrintf(&origin_url, "%s://%s/", content::kChromeUIScheme, ...
5 years, 2 months ago (2015-09-30 14:21:31 UTC) #19
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1362433002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1362433002/160001
5 years, 2 months ago (2015-09-30 14:27:32 UTC) #21
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/109874)
5 years, 2 months ago (2015-09-30 15:54:34 UTC) #23
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1362433002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1362433002/180001
5 years, 2 months ago (2015-09-30 18:27:36 UTC) #25
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_compile_dbg_ng/builds/89668)
5 years, 2 months ago (2015-09-30 18:48:28 UTC) #27
Charlie Reis
https://chromiumcodereview.appspot.com/1362433002/diff/140001/chrome/browser/pdf/pdf_extension_test.cc File chrome/browser/pdf/pdf_extension_test.cc (right): https://chromiumcodereview.appspot.com/1362433002/diff/140001/chrome/browser/pdf/pdf_extension_test.cc#newcode403 chrome/browser/pdf/pdf_extension_test.cc:403: rph->FilterURL(true, &invalid_link_url); On 2015/09/30 14:21:31, Paul Meyer wrote: > ...
5 years, 2 months ago (2015-09-30 20:39:17 UTC) #28
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1362433002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1362433002/200001
5 years, 2 months ago (2015-09-30 22:04:09 UTC) #30
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1362433002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1362433002/220001
5 years, 2 months ago (2015-09-30 22:11:27 UTC) #33
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years, 2 months ago (2015-09-30 23:48:14 UTC) #35
raymes
lgtm
5 years, 2 months ago (2015-10-01 01:40:36 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1362433002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1362433002/220001
5 years, 2 months ago (2015-10-01 02:05:00 UTC) #39
commit-bot: I haz the power
Committed patchset #7 (id:220001)
5 years, 2 months ago (2015-10-01 02:11:21 UTC) #40
commit-bot: I haz the power
5 years, 2 months ago (2015-10-01 02:12:07 UTC) #41
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/1eefa26e1795192c5a347a1e1e7a99e88c47f9c4
Cr-Commit-Position: refs/heads/master@{#351705}

Powered by Google App Engine
This is Rietveld 408576698