Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1362433002:
Fix for "chrome://" links in PDFs. (Closed)
|
Created:
5 years, 3 months ago by paulmeyer Modified:
5 years, 2 months ago 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. |
DescriptionThis 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. #Messages
Total messages: 41 (17 generated)
Patchset #1 (id:1) has been deleted
paulmeyer@chromium.org changed reviewers: + creis@chromium.org
+creis@ for general review.
Sorry for the delay; yesterday was a deadline. Thanks for looking into this. It seems like a reasonable approach if we can confirm it does the right thing. This will need tests at two levels: ChildProcessSecurityPolicy has a pretty comprehensive set of unit tests, and we should add cases for the new method (and how it interacts with GrantScheme). We should also add a test that tries to follow allowed and disallowed chrome:// URLs in a PDF, to show this fixes the problem. (We should also run try jobs to confirm.) Some comments and questions below, which might help simplify the change. https://codereview.chromium.org/1362433002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/chrome_extension_web_contents_observer.cc (right): https://codereview.chromium.org/1362433002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:39: // Some extensions use "chrome://favicon/" and "chrome://extension-icon" URLs. Why are these cases handled here vs chrome://resources in extension_web_contents_observer.cc? I don't understand the difference. https://codereview.chromium.org/1362433002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:46: extension->location() == Manifest::COMPONENT)) { This block is subtle in terms of which types of extensions have this access. The comment above should elaborate on why these and not others. https://codereview.chromium.org/1362433002/diff/20001/content/browser/child_p... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/1362433002/diff/20001/content/browser/child_p... content/browser/child_process_security_policy_impl.cc:108: void RevokeSchemeHost(const std::string& scheme, const std::string& host) { We shouldn't introduce new methods until they're needed. https://codereview.chromium.org/1362433002/diff/20001/content/browser/child_p... content/browser/child_process_security_policy_impl.cc:284: // granted or revoked. This seems overly complicated if we don't have any code that does revocation. It seems easier to just have a set of GURLs or Origins (e.g., chrome://favicon), similar to FileSet. https://codereview.chromium.org/1362433002/diff/20001/content/browser/child_p... content/browser/child_process_security_policy_impl.cc:288: // scheme in |scheme_host_polcy_| will be respected first, followed by the This also seems overly complicated. If we aren't doing revocation, then there's no reason to check the schemes+hosts before checking schemes. If a full scheme is granted, that should imply that all hosts in it are granted. https://codereview.chromium.org/1362433002/diff/20001/content/browser/child_p... File content/browser/child_process_security_policy_impl.h (right): https://codereview.chromium.org/1362433002/diff/20001/content/browser/child_p... content/browser/child_process_security_policy_impl.h:65: const std::string& host) override; Style nit: "For function declarations and definitions, put each argument on a separate line unless the whole declaration fits on one line." https://codereview.chromium.org/1362433002/diff/20001/content/public/browser/... File content/public/browser/child_process_security_policy.h (right): https://codereview.chromium.org/1362433002/diff/20001/content/public/browser/... content/public/browser/child_process_security_policy.h:135: // provided scheme and host. Maybe we should be using url::Origins here, rather than pairs of strings? (GURL is also an option, but I'd prefer not to allow granting of individual URLs with paths unless we need to.) On that note, perhaps it should be called GrantOrigin. https://codereview.chromium.org/1362433002/diff/20001/content/public/browser/... content/public/browser/child_process_security_policy.h:137: const std::string& host) = 0; Same style nit. https://codereview.chromium.org/1362433002/diff/20001/extensions/browser/exte... File extensions/browser/extension_web_contents_observer.cc (right): https://codereview.chromium.org/1362433002/diff/20001/extensions/browser/exte... extensions/browser/extension_web_contents_observer.cc:80: // Some extensions use "chrome://resources/" URLs. Again, I'm not sure why this is handled separately from chrome://resources, especially with a subtle check guarding each of them. Seems easy for one check to get modified without the other. https://codereview.chromium.org/1362433002/diff/20001/extensions/browser/exte... extensions/browser/extension_web_contents_observer.cc:91: // Some extensions use "file://" URLs. nit: No need for quotes (or churn on this line).
Patchset #2 (id:40001) has been deleted
I'll work on adding tests now. https://codereview.chromium.org/1362433002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/chrome_extension_web_contents_observer.cc (right): https://codereview.chromium.org/1362433002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:39: // Some extensions use "chrome://favicon/" and "chrome://extension-icon" URLs. On 2015/09/22 17:38:15, Charlie Reis wrote: > Why are these cases handled here vs chrome://resources in > extension_web_contents_observer.cc? I don't understand the difference. The reason was because the host constants are from different layers. content::kChromeUIResourcesHost is in the content layer, while chrome::kChromeUIFaviconHost and chrome::kChromeUIExtensionIconHost are in the chrome layer (so I moved them out here). However, all of these permissions are really only needed in Chrome, so I can move it all to the chrome layer. https://codereview.chromium.org/1362433002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:46: extension->location() == Manifest::COMPONENT)) { On 2015/09/22 17:38:15, Charlie Reis wrote: > This block is subtle in terms of which types of extensions have this access. > The comment above should elaborate on why these and not others. Done. https://codereview.chromium.org/1362433002/diff/20001/content/browser/child_p... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/1362433002/diff/20001/content/browser/child_p... content/browser/child_process_security_policy_impl.cc:108: void RevokeSchemeHost(const std::string& scheme, const std::string& host) { On 2015/09/22 17:38:15, Charlie Reis wrote: > We shouldn't introduce new methods until they're needed. I was thinking the same thing, though RevokeScheme is exactly the same situation (it is not currently used). Should I also remove RevokeScheme? https://codereview.chromium.org/1362433002/diff/20001/content/browser/child_p... content/browser/child_process_security_policy_impl.cc:284: // granted or revoked. On 2015/09/22 17:38:15, Charlie Reis wrote: > This seems overly complicated if we don't have any code that does revocation. > > It seems easier to just have a set of GURLs or Origins (e.g., chrome://favicon), > similar to FileSet. Okay, I'll use a set of origins. https://codereview.chromium.org/1362433002/diff/20001/content/browser/child_p... content/browser/child_process_security_policy_impl.cc:288: // scheme in |scheme_host_polcy_| will be respected first, followed by the On 2015/09/22 17:38:15, Charlie Reis wrote: > This also seems overly complicated. If we aren't doing revocation, then there's > no reason to check the schemes+hosts before checking schemes. If a full scheme > is granted, that should imply that all hosts in it are granted. Done. https://codereview.chromium.org/1362433002/diff/20001/content/browser/child_p... File content/browser/child_process_security_policy_impl.h (right): https://codereview.chromium.org/1362433002/diff/20001/content/browser/child_p... content/browser/child_process_security_policy_impl.h:65: const std::string& host) override; On 2015/09/22 17:38:15, Charlie Reis wrote: > Style nit: "For function declarations and definitions, put each argument on a > separate line unless the whole declaration fits on one line." Done. https://codereview.chromium.org/1362433002/diff/20001/content/public/browser/... File content/public/browser/child_process_security_policy.h (right): https://codereview.chromium.org/1362433002/diff/20001/content/public/browser/... content/public/browser/child_process_security_policy.h:135: // provided scheme and host. On 2015/09/22 17:38:15, Charlie Reis wrote: > Maybe we should be using url::Origins here, rather than pairs of strings? (GURL > is also an option, but I'd prefer not to allow granting of individual URLs with > paths unless we need to.) > > On that note, perhaps it should be called GrantOrigin. Done and done. https://codereview.chromium.org/1362433002/diff/20001/content/public/browser/... content/public/browser/child_process_security_policy.h:137: const std::string& host) = 0; On 2015/09/22 17:38:15, Charlie Reis wrote: > Same style nit. Done. https://codereview.chromium.org/1362433002/diff/20001/extensions/browser/exte... File extensions/browser/extension_web_contents_observer.cc (right): https://codereview.chromium.org/1362433002/diff/20001/extensions/browser/exte... extensions/browser/extension_web_contents_observer.cc:80: // Some extensions use "chrome://resources/" URLs. On 2015/09/22 17:38:15, Charlie Reis wrote: > Again, I'm not sure why this is handled separately from chrome://resources, > especially with a subtle check guarding each of them. Seems easy for one check > to get modified without the other. See other comment. I'll move this out to the chrome layer. https://codereview.chromium.org/1362433002/diff/20001/extensions/browser/exte... extensions/browser/extension_web_contents_observer.cc:91: // Some extensions use "file://" URLs. On 2015/09/22 17:38:15, Charlie Reis wrote: > nit: No need for quotes (or churn on this line). Done.
Thanks. Just a few quick comments before I have to go, but I'll take another look tomorrow. https://codereview.chromium.org/1362433002/diff/20001/content/browser/child_p... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/1362433002/diff/20001/content/browser/child_p... content/browser/child_process_security_policy_impl.cc:108: void RevokeSchemeHost(const std::string& scheme, const std::string& host) { On 2015/09/22 22:13:57, Paul Meyer wrote: > On 2015/09/22 17:38:15, Charlie Reis wrote: > > We shouldn't introduce new methods until they're needed. > > I was thinking the same thing, though RevokeScheme is exactly the same situation > (it is not currently used). Should I also remove RevokeScheme? Not in this CL. We can probably remove it as dead code separately. https://codereview.chromium.org/1362433002/diff/20001/content/public/browser/... File content/public/browser/child_process_security_policy.h (right): https://codereview.chromium.org/1362433002/diff/20001/content/public/browser/... content/public/browser/child_process_security_policy.h:135: // provided scheme and host. On 2015/09/22 22:13:57, Paul Meyer wrote: > On 2015/09/22 17:38:15, Charlie Reis wrote: > > Maybe we should be using url::Origins here, rather than pairs of strings? > (GURL > > is also an option, but I'd prefer not to allow granting of individual URLs > with > > paths unless we need to.) > > > > On that note, perhaps it should be called GrantOrigin. > > Done and done. Please use url::Origins rather than strings (same in the impl file).
https://codereview.chromium.org/1362433002/diff/20001/content/browser/child_p... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/1362433002/diff/20001/content/browser/child_p... content/browser/child_process_security_policy_impl.cc:108: void RevokeSchemeHost(const std::string& scheme, const std::string& host) { On 2015/09/23 00:07:15, Charlie Reis wrote: > On 2015/09/22 22:13:57, Paul Meyer wrote: > > On 2015/09/22 17:38:15, Charlie Reis wrote: > > > We shouldn't introduce new methods until they're needed. > > > > I was thinking the same thing, though RevokeScheme is exactly the same > situation > > (it is not currently used). Should I also remove RevokeScheme? > > Not in this CL. We can probably remove it as dead code separately. Acknowledged. https://codereview.chromium.org/1362433002/diff/20001/content/public/browser/... File content/public/browser/child_process_security_policy.h (right): https://codereview.chromium.org/1362433002/diff/20001/content/public/browser/... content/public/browser/child_process_security_policy.h:135: // provided scheme and host. On 2015/09/23 00:07:15, Charlie Reis wrote: > On 2015/09/22 22:13:57, Paul Meyer wrote: > > On 2015/09/22 17:38:15, Charlie Reis wrote: > > > Maybe we should be using url::Origins here, rather than pairs of strings? > > (GURL > > > is also an option, but I'd prefer not to allow granting of individual URLs > > with > > > paths unless we need to.) > > > > > > On that note, perhaps it should be called GrantOrigin. > > > > Done and done. > > Please use url::Origins rather than strings (same in the impl file). Sorry, I misunderstood. Now using url::Origin.
[+kalman,rdevlin.cronin to CC] nit: Let's also list bug 226927 in the CL description, since this resolves that TODO as well. Thanks, this looks much better. I see you mention on the bug that existing tests pass with this, so hopefully it won't regress the chrome.tabs behavior. (It would have been fine to run try jobs with these patch sets, even before you added the new tests-- that would help us confirm the change itself isn't a problem.) I think I'm happy with the change now-- I'll approve once the tests are added. https://codereview.chromium.org/1362433002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/chrome_extension_web_contents_observer.cc (right): https://codereview.chromium.org/1362433002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:50: url::Origin origin = url::Origin::UnsafelyCreateOriginWithoutNormalization( Sounds like that's not a preferred way to create Origins, apart from IPC messages. Why not create a GURL from a string of the scheme and host, and then convert that to Origin, similar to below? https://codereview.chromium.org/1362433002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:56: // Extensions, legacy packaged apps, and platform apps are allowed to use nit: component platform apps
rdevlin.cronin@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Looks pretty good, just a few nits. https://codereview.chromium.org/1362433002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/chrome_extension_web_contents_observer.cc (right): https://codereview.chromium.org/1362433002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:43: Manifest::Type type = extension->GetType(); won't need this. https://codereview.chromium.org/1362433002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:48: if (type == Manifest::TYPE_EXTENSION && extension->is_extension() https://codereview.chromium.org/1362433002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:49: extension->location() == Manifest::COMPONENT) { Manifest::IsComponentLocation(extension->location()) https://codereview.chromium.org/1362433002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:52: content::ChildProcessSecurityPolicy::GetInstance()->GrantOrigin( This would be less verbose if we cached it. https://codereview.chromium.org/1362433002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:60: if (type == Manifest::TYPE_EXTENSION || is_extension(), analogous for below https://codereview.chromium.org/1362433002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:70: ExtensionWebContentsObserver::RenderViewCreated(render_view_host); I think I'd prefer to do this initialization first.
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
PTAL https://codereview.chromium.org/1362433002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/chrome_extension_web_contents_observer.cc (right): https://codereview.chromium.org/1362433002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:43: Manifest::Type type = extension->GetType(); On 2015/09/23 23:14:17, Devlin wrote: > won't need this. Done. https://codereview.chromium.org/1362433002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:48: if (type == Manifest::TYPE_EXTENSION && On 2015/09/23 23:14:17, Devlin wrote: > extension->is_extension() Done. https://codereview.chromium.org/1362433002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:49: extension->location() == Manifest::COMPONENT) { On 2015/09/23 23:14:17, Devlin wrote: > Manifest::IsComponentLocation(extension->location()) Done. https://codereview.chromium.org/1362433002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:52: content::ChildProcessSecurityPolicy::GetInstance()->GrantOrigin( On 2015/09/23 23:14:17, Devlin wrote: > This would be less verbose if we cached it. Done. https://codereview.chromium.org/1362433002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:56: // Extensions, legacy packaged apps, and platform apps are allowed to use On 2015/09/23 22:57:14, Charlie Reis wrote: > nit: component platform apps Done. https://codereview.chromium.org/1362433002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:60: if (type == Manifest::TYPE_EXTENSION || On 2015/09/23 23:14:17, Devlin wrote: > is_extension(), analogous for below Done. https://codereview.chromium.org/1362433002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:70: ExtensionWebContentsObserver::RenderViewCreated(render_view_host); On 2015/09/23 23:14:17, Devlin wrote: > I think I'd prefer to do this initialization first. Done.
PTAL https://codereview.chromium.org/1362433002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/chrome_extension_web_contents_observer.cc (right): https://codereview.chromium.org/1362433002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:48: if (type == Manifest::TYPE_EXTENSION && On 2015/09/23 23:14:17, Devlin wrote: > extension->is_extension() Done. https://codereview.chromium.org/1362433002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:52: content::ChildProcessSecurityPolicy::GetInstance()->GrantOrigin( On 2015/09/23 23:14:17, Devlin wrote: > This would be less verbose if we cached it. Done. https://codereview.chromium.org/1362433002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:56: // Extensions, legacy packaged apps, and platform apps are allowed to use On 2015/09/23 22:57:14, Charlie Reis wrote: > nit: component platform apps Done. https://codereview.chromium.org/1362433002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:70: ExtensionWebContentsObserver::RenderViewCreated(render_view_host); On 2015/09/23 23:14:17, Devlin wrote: > I think I'd prefer to do this initialization first. Done.
extensions lgtm https://codereview.chromium.org/1362433002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/chrome_extension_web_contents_observer.cc (right): https://codereview.chromium.org/1362433002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:52: base::SStringPrintf(&origin_url, "%s://%s/", content::kChromeUIScheme, nit: Why SStringPrintf, instead of just StringPrintf? https://codereview.chromium.org/1362433002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:52: base::SStringPrintf(&origin_url, "%s://%s/", content::kChromeUIScheme, nit: Also kStandardSchemeSeparator https://codereview.chromium.org/1362433002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:52: base::SStringPrintf(&origin_url, "%s://%s/", content::kChromeUIScheme, And, both those said, I actually wouldn't be opposed to just adding a chrome::kChromeUIResourcesURL, but I'll defer to other people's opinions there.
Thanks for the tests! LGTM with nits. https://codereview.chromium.org/1362433002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/chrome_extension_web_contents_observer.cc (right): https://codereview.chromium.org/1362433002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:52: base::SStringPrintf(&origin_url, "%s://%s/", content::kChromeUIScheme, On 2015/09/29 19:06:30, Devlin wrote: > And, both those said, I actually wouldn't be opposed to just adding a > chrome::kChromeUIResourcesURL, but I'll defer to other people's opinions there. I'd be happy with that, similar to kChromeUINetworkViewCacheURL and kChromeUINetworkViewCacheHost. https://codereview.chromium.org/1362433002/diff/140001/chrome/browser/pdf/pdf... File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/1362433002/diff/140001/chrome/browser/pdf/pdf... chrome/browser/pdf/pdf_extension_test.cc:403: rph->FilterURL(true, &invalid_link_url); Hmm, is there no way to attempt to request the resource instead? This is better than nothing, but it doesn't guarantee that we're actually calling FilterURL in the cases that matter. https://codereview.chromium.org/1362433002/diff/140001/chrome/browser/pdf/pdf... chrome/browser/pdf/pdf_extension_test.cc:406: ASSERT_EQ(valid_link_url, unfiltered_valid_link_url); nit: Use EXPECT_EQ for test expectations (so that the test keeps running) and ASSERT_EQ for mandatory preconditions (if the test can't proceed sensibly). https://codereview.chromium.org/1362433002/diff/140001/chrome/browser/pdf/pdf... chrome/browser/pdf/pdf_extension_test.cc:407: ASSERT_EQ(invalid_link_url, GURL("about:blank")); nit: The arguments should be (expected, actual), so flip the order. https://codereview.chromium.org/1362433002/diff/140001/content/browser/child_... File content/browser/child_process_security_policy_unittest.cc (right): https://codereview.chromium.org/1362433002/diff/140001/content/browser/child_... content/browser/child_process_security_policy_unittest.cc:713: EXPECT_FALSE(p->CanCommitURL(kRendererID, url_foo1)); nit: Let's also include CanRequestURL calls, similar to StandardSchemesTest, etc.
paulmeyer@chromium.org changed reviewers: + raymes@chromium.org
+raymes@ for OWNER review of pdf_extension_test.cc https://codereview.chromium.org/1362433002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/chrome_extension_web_contents_observer.cc (right): https://codereview.chromium.org/1362433002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:52: base::SStringPrintf(&origin_url, "%s://%s/", content::kChromeUIScheme, On 2015/09/29 22:15:53, Charlie Reis wrote: > On 2015/09/29 19:06:30, Devlin wrote: > > And, both those said, I actually wouldn't be opposed to just adding a > > chrome::kChromeUIResourcesURL, but I'll defer to other people's opinions > there. > > I'd be happy with that, similar to kChromeUINetworkViewCacheURL and > kChromeUINetworkViewCacheHost. Done. https://codereview.chromium.org/1362433002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:52: base::SStringPrintf(&origin_url, "%s://%s/", content::kChromeUIScheme, On 2015/09/29 19:06:30, Devlin wrote: > And, both those said, I actually wouldn't be opposed to just adding a > chrome::kChromeUIResourcesURL, but I'll defer to other people's opinions there. Done. https://codereview.chromium.org/1362433002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:52: base::SStringPrintf(&origin_url, "%s://%s/", content::kChromeUIScheme, On 2015/09/29 19:06:30, Devlin wrote: > nit: Also kStandardSchemeSeparator Acknowledged. https://codereview.chromium.org/1362433002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/chrome_extension_web_contents_observer.cc:52: base::SStringPrintf(&origin_url, "%s://%s/", content::kChromeUIScheme, On 2015/09/29 19:06:30, Devlin wrote: > nit: Why SStringPrintf, instead of just StringPrintf? No particular reason, but I'm taking it out anyways. https://codereview.chromium.org/1362433002/diff/140001/chrome/browser/pdf/pdf... File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/1362433002/diff/140001/chrome/browser/pdf/pdf... chrome/browser/pdf/pdf_extension_test.cc:403: rph->FilterURL(true, &invalid_link_url); On 2015/09/29 22:15:53, Charlie Reis wrote: > Hmm, is there no way to attempt to request the resource instead? This is better > than nothing, but it doesn't guarantee that we're actually calling FilterURL in > the cases that matter. The issue behind this bug is that context menus allowed link URLs to go through that shouldn't. This only happened because of a problem within FilterURL, so I am testing that. I originally created a PDF with these links on it, and was going to open context menus on them both and check the links that the menus picked up, but it turned out that writing the test that way was really finicky, and the context menus often didn't act as if they'd been opened on links even if they were opened in exactly the right spot. I feel like a test like that would also be really fragile in the future, because it relies upon the links being at set coordinates that could change easily if the layout of the pdf viewer ever changed. I think this test should be sufficient for testing against the regression of this particular bug, and is also very reliable. https://codereview.chromium.org/1362433002/diff/140001/chrome/browser/pdf/pdf... chrome/browser/pdf/pdf_extension_test.cc:406: ASSERT_EQ(valid_link_url, unfiltered_valid_link_url); On 2015/09/29 22:15:53, Charlie Reis wrote: > nit: Use EXPECT_EQ for test expectations (so that the test keeps running) and > ASSERT_EQ for mandatory preconditions (if the test can't proceed sensibly). Done. https://codereview.chromium.org/1362433002/diff/140001/chrome/browser/pdf/pdf... chrome/browser/pdf/pdf_extension_test.cc:407: ASSERT_EQ(invalid_link_url, GURL("about:blank")); On 2015/09/29 22:15:53, Charlie Reis wrote: > nit: The arguments should be (expected, actual), so flip the order. Done. https://codereview.chromium.org/1362433002/diff/140001/content/browser/child_... File content/browser/child_process_security_policy_unittest.cc (right): https://codereview.chromium.org/1362433002/diff/140001/content/browser/child_... content/browser/child_process_security_policy_unittest.cc:713: EXPECT_FALSE(p->CanCommitURL(kRendererID, url_foo1)); On 2015/09/29 22:15:53, Charlie Reis wrote: > nit: Let's also include CanRequestURL calls, similar to StandardSchemesTest, > etc. Done.
The CQ bit was checked by paulmeyer@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by paulmeyer@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
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_comp...)
https://chromiumcodereview.appspot.com/1362433002/diff/140001/chrome/browser/... File chrome/browser/pdf/pdf_extension_test.cc (right): https://chromiumcodereview.appspot.com/1362433002/diff/140001/chrome/browser/... chrome/browser/pdf/pdf_extension_test.cc:403: rph->FilterURL(true, &invalid_link_url); On 2015/09/30 14:21:31, Paul Meyer wrote: > On 2015/09/29 22:15:53, Charlie Reis wrote: > > Hmm, is there no way to attempt to request the resource instead? This is > better > > than nothing, but it doesn't guarantee that we're actually calling FilterURL > in > > the cases that matter. > > The issue behind this bug is that context menus allowed link URLs to go through > that shouldn't. This only happened because of a problem within FilterURL, so I > am testing that. > > I originally created a PDF with these links on it, and was going to open context > menus on them both and check the links that the menus picked up, but it turned > out that writing the test that way was really finicky, and the context menus > often didn't act as if they'd been opened on links even if they were opened in > exactly the right spot. I feel like a test like that would also be really > fragile in the future, because it relies upon the links being at set coordinates > that could change easily if the layout of the pdf viewer ever changed. > > I think this test should be sufficient for testing against the regression of > this particular bug, and is also very reliable. Acknowledged.
The CQ bit was checked by paulmeyer@chromium.org to run a CQ dry run
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
Patchset #7 (id:200001) has been deleted
The CQ bit was checked by paulmeyer@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by paulmeyer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1362433002/#ps220001 (title: "Small fix.")
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
Message was sent while issue was closed.
Committed patchset #7 (id:220001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/1eefa26e1795192c5a347a1e1e7a99e88c47f9c4 Cr-Commit-Position: refs/heads/master@{#351705} |
