content/public/browser/content_browser_client.h - Issue 1905033002: PlzNavigate: Move navigation-level mixed content checks to the browser.

Unified Diff: content/public/browser/content_browser_client.h

Issue 1905033002: PlzNavigate: Move navigation-level mixed content checks to the browser. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@console-security-message

Patch Set: Addressed all jam@ latest comments. Created 3 years, 11 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

Index: content/public/browser/content_browser_client.h

diff --git a/content/public/browser/content_browser_client.h b/content/public/browser/content_browser_client.h

index 18976390c16b3ae327bedc94cea83011874450a0..f91855cac18e3519f81fc4f588d3290d291b186e 100644

--- a/content/public/browser/content_browser_client.h

+++ b/content/public/browser/content_browser_client.h

@@ -762,6 +762,16 @@ class CONTENT_EXPORT ContentBrowserClient {

// protected content.

virtual std::unique_ptr<media::CdmFactory> CreateCdmFactory();

+ // Reports that passive mixed content was found at the specified WebContents.

+ virtual void PassiveInsecureContentFound(const GURL& resource_url);

+ // Checks if running of mixed-content is allowed for the specified

+ // WebContents/tab.

+ virtual bool ShouldAllowRunningInsecureContent(bool allowed_per_settings,

jam 2017/01/09 21:15:46 this method should be on WebContentsDelegate. Cont

carlosk 2017/01/10 19:13:11 Understood and thanks for this explanation. But n

Understood and thanks for this explanation. But now I'm also kinda lost in the sea of 72 existing subclasses of WCD... I know I need to have it implemented for: * The default implementation, replacing ContentBrowserClient's implementation. ->>> Would be in WebContentsDelegate itself. * User facing WebContents in Chrome, replacing ChromeContentBrowserClient's implementation. ->>> It seems ::Browser (chrome/browser/ui/browser.h) would be the right substitute? * Supporting layout tests within content_shell, replacing LayoutTestContentBrowserClient's implementation. ->>> As I can't find any WCD subclass under content/shell/browser/layout_test/ I think I'd have to create a new one to hold that implementation. * And now that I am thinking about this, it seems active mixed content should also work for user facing WebContents in content_shell itself, and not be always blocked. That would be as if I had overridden it in ShellContentBrowserClient. ->>> It seems content::Shell could implement this? As I couldn't find evidence of settings or flags under content/shell/ I'm assuming they don't exist and I'd simply return allowed_per_settings. And shouldn't PassiveInsecureContentFound be moved too? It seems what you described also applies to it. Final sanity check: are you sure mixed content checks only applies to user visible WebContents? I see WCD subclasses for things like extensions, headless, prerender and more, and they make me wonder...

jam 2017/01/10 19:28:37 right. browser is the one that's currently adding

right. browser is the one that's currently adding the MixedContentSettings tab helper which the current implementation depends on.

content/shell/browser/shell.h is the WebContentsDelegate for content_shell (note a WCD is always required)

yep SG

these don't get tabhelpers, i.e. the MixedContentSettings class that you added wouldn't run. so ChromeContentBrowserClient::ShouldAllowRunningInsecureContent will crash with a null allowed_per_settings you bring up a good point though. What's the current behavior for extensions, prerender etc? we should maintain that behavior. if the renderer is doing it for all types of RenderFrames, which it appears to, then we can keep this method on ContentBrowserClient. But then you'll need to get the tab helper added for extensions, prerender etc...

carlosk 2017/01/10 20:03:59 There is logic that is specific to layout tests th

There is logic that is specific to layout tests that shouldn't apply to content_shell running by itself (see LayoutTestContentBrowserClient's implementation). As test support is built in (and not in a separate target) it might be possible to consolidate both in Shell.

I will move both then.

Then I'll keep the current behavior by changing the default return value I I am currently using: instead of always returning false I'll return allowed_per_settings' value. This will solve the content_shell case I mentioned above and will also match the renderer behavior (see FrameLoaderClientImpl::allowRunningInsecureContent)

carlosk 2017/01/11 03:32:21 Done.

On 2017/01/10 20:03:59, carlosk wrote: > On 2017/01/10 19:28:37, jam wrote: > > On 2017/01/10 19:13:11, carlosk wrote: > > > On 2017/01/09 21:15:46, jam wrote: > > > > this method should be on WebContentsDelegate. ContentBrowserClient is > > > generally > > > > used for: > > > > 1) embedder calls when there's no more specific delegate interface like > WCD > > > > 2) web contents delegate methods that are common for all WebContents' used > > in > > > > the browser. in this case, this method only makes sense for tabs that the > > user > > > > sees (and the implementation depends on a class that is only added for > > browser > > > > tabs). so then it can be implemented on chrome's Browser class only. > > > > > > Understood and thanks for this explanation. > > > > > > But now I'm also kinda lost in the sea of 72 existing subclasses of WCD... I > > > know I need to have it implemented for: > > > > > > * The default implementation, replacing ContentBrowserClient's > implementation. > > > ->>> Would be in WebContentsDelegate itself. > > > > > > * User facing WebContents in Chrome, replacing ChromeContentBrowserClient's > > > implementation. > > > ->>> It seems ::Browser (chrome/browser/ui/browser.h) would be the right > > > substitute? > > > > right. browser is the one that's currently adding the MixedContentSettings tab > > helper which the current implementation depends on. > > > > > > > > * Supporting layout tests within content_shell, replacing > > > LayoutTestContentBrowserClient's implementation. > > > ->>> As I can't find any WCD subclass under > content/shell/browser/layout_test/ > > I > > > think I'd have to create a new one to hold that implementation. > > > > content/shell/browser/shell.h is the WebContentsDelegate for content_shell > (note > > a WCD is always required) > > There is logic that is specific to layout tests that shouldn't apply to > content_shell running by itself (see LayoutTestContentBrowserClient's > implementation). As test support is built in (and not in a separate target) it > might be possible to consolidate both in Shell. > > > > * And now that I am thinking about this, it seems active mixed content > should > > > also work for user facing WebContents in content_shell itself, and not be > > always > > > blocked. That would be as if I had overridden it in > ShellContentBrowserClient. > > > ->>> It seems content::Shell could implement this? As I couldn't find > evidence > > > of settings or flags under content/shell/ I'm assuming they don't exist and > > I'd > > > simply return allowed_per_settings. > > > > > > > > > And shouldn't PassiveInsecureContentFound be moved too? It seems what you > > > described also applies to it. > > > > yep SG > > I will move both then. > > > > > > > > > > Final sanity check: are you sure mixed content checks only applies to user > > > visible WebContents? I see WCD subclasses for things like extensions, > > headless, > > > prerender and more, and they make me wonder... > > > > these don't get tabhelpers, i.e. the MixedContentSettings class that you > added > > wouldn't run. so ChromeContentBrowserClient::ShouldAllowRunningInsecureContent > > will crash with a null allowed_per_settings > > > > > > you bring up a good point though. What's the current behavior for extensions, > > prerender etc? we should maintain that behavior. if the renderer is doing it > for > > all types of RenderFrames, which it appears to, then we can keep this method > on > > ContentBrowserClient. But then you'll need to get the tab helper added for > > extensions, prerender etc... > > Then I'll keep the current behavior by changing the default return value I I am > currently using: instead of always returning false I'll return > allowed_per_settings' value. This will solve the content_shell case I mentioned > above and will also match the renderer behavior (see > FrameLoaderClientImpl::allowRunningInsecureContent)

Done.

+ const url::Origin& origin,

+ const GURL& resource_url,

+ WebContents* web_contents);

// Populates |mappings| with all files that need to be mapped before launching

// a child process.

#if defined(OS_ANDROID)

« content/common/navigation_params.h ('K') | « content/common/resource_request.h ('k') | content/public/browser/content_browser_client.cc » ('j') | net/url_request/url_request.h » ('J')