Bug 61946 – FrameLoaderClient::didRunInsecureContent - no way to distinguish between blocked/run cases.

• Component: WebKit API
• Status: RESOLVED FIXED

[Thomas Sepez]

FrameLoader::checkIfRunInsecureContent() calls m_client->didRunInsecureContent with the same arguments in both the case where the insecure content was actually run, and in the case where it is blocked from running by security policy.  There needs to be a way to distinguish the two cases so that clients can create appropriate notifications.

Note that FrameLoader::checkIfDisplayInsecurContent() has the same issue with its call to m_client->didDisplayInsecureContent().

A good solution would be to add didBlockRunningInsecureContent() and didBlockDisplayingInsecureContent() methods, and call one or the other, thus restoring the meaning of the original methods.  Adding a flag seems less desirable since the name of the existing method is now misleading and would more accurately be called "didEncounterInsecureContent()".

[Adam Barth]

By the way, another approach to doing this is what we do for allowPlugins, which is that we call the client with the WebCore::Setting as an argument.

[Thomas Sepez]

Created attachment 95926 [details]
First cut at changes to get buy-in before investing in tests.

Particularly interested in knowing if I got all the places that need to change.

[Adam Barth]

Comment on attachment 95926 [details]
First cut at changes to get buy-in before investing in tests.

View in context: https://bugs.webkit.org/attachment.cgi?id=95926&action=review

> Source/WebCore/loader/EmptyClients.h:371
> +    virtual void didBlockDisplayingInsecureContent() { }
> +    virtual void didBlockRunningInsecureContent() { }

We probably want the offending URL, right?  Also, for didBlockRunningInsecureContent we probably want the SecurityOrigin, as in the did run case.

> Source/WebCore/loader/FrameLoader.cpp:1125
> +        m_client->didDisplayInsecureContent();
> +    } else
> +        m_client->didBlockDisplayingInsecureContent();

One thing we discussed is whether we should combine these into one call.  Another question is whether we should call back into the client to make the final "go/no-go" decision, like we do for allowPlugins.  That gives the client maximum flexibility.  It can use the Setting to provide a default, but can still tweak things on a load-by-load basis if needed.  Also, the client then knows whether the load was blocked because it makes the final decision.

[Alexey Proskuryakov]

I still don't quite understand the use case. If didRunInsecureContent() is called, the browser hides its https lock icon. What are the didBlock notifications good for? Will the browser say "bwa-ha-ha, go use a different browser if you really want to use this site"? Or "dice a coin to decide whether to disable security content policy for this page, and then reload it"?

A broken site is just a broken site. When a CSS stylesheet is served with an incorrect Content-Type, we don't ask the user whether to load it - we just ignore it. Should content security policies be different?

+        String message = "The page at " + m_frame->document()->url().string() + " ran insecure content from " + url.string() + ".\n";
+        m_frame->domWindow()->console()->addMessage(HTMLMessageSource, LogMessageType, WarningMessageLevel, message, 1, String());

That might be too chatty, not sure. But more importantly, this is too vague. What should a developer do when seeing this message? Does this only mean that their code potentially won't work in other browsers? We generally don't log things like that.

[Thomas Sepez]

We're doing something stronger than just hiding the https lock icon.  We're going to break pages, by default, that try to include scripts on https pages from non-https sources.  And yes, there needs to be a way to disable it, along the lines of the "Toss a coin" scenario you indicated, at least in the short term until the top sites get their act together.

I'd like to do what you suggest along the lines of your CSS analogy, but the carnage will be too high.

The chatty addMessage() messages pre-dated this change.  I think they should go, too.

[Adam Barth]

> The chatty addMessage() messages pre-dated this change.  I think they should go, too.

Please don't remove those messages.  They're vey useful to developers.  We have many interactions with developers where we ask them to look for these messages and it usually results in them understanding the issue much better.

w.r.t. to the use case, we'd like to try blocking mixed content by default and then given users the option to override the default in various scenarios.  To know when to offer the override, the embedder needs to know when the mixed content was blocked.

[Thomas Sepez]

I think Adam's right about doing this along the lines of allowPlugin() calls.

[Adam Barth]

To pour a few more words over the use case, we want to eventually block mixed content without an opt-out.  The info-bar approach is an incremental step towards that end which will give us a signal for how much more evangelism is required.  There's also a related discussion on moz-security about whether browser vendors can act in coordination to block mixed content.

[Thomas Sepez]

Created attachment 96130 [details]
Patch to call into permissions client upon insecure content.

Implements this along the lines of Adam's suggestion.

[WebKit Review Bot]

Attachment 96130 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/chromium/src/FrameLoaderClientImpl.h:210:  The parameter name "url" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/ChangeLog:5:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/chromium/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/chromium/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 5 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Thomas Sepez]

Created attachment 96131 [details]
Patch to call into permissions client on insecure content

Fix blasted tabs.

[Thomas Sepez]

Created attachment 96134 [details]
Call into permissions client on insecure content.

[WebKit Review Bot]

Attachment 96134 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/chromium/src/FrameLoaderClientImpl.h:210:  insecure_url is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Thomas Sepez]

Created attachment 96137 [details]
Call into permissions client on insecure content.

[Adam Barth]

Comment on attachment 96137 [details]
Call into permissions client on insecure content.

View in context: https://bugs.webkit.org/attachment.cgi?id=96137&action=review

We could add a Chromium WebKit API test to show that the overrides work.  You can look at how we test the other allow methods, if you want some inspiration.

> Source/WebCore/loader/FrameLoader.cpp:1119
> +    bool allowedPerSettings = settings && settings->allowDisplayOfInsecureContent();
> +    bool allowed = m_client->allowDisplayingInsecureContent(allowedPerSettings);

I'd just inline the settings && settings->allowDisplayOfInsecureContent() expression into the client call and get rid of the temporary, but that's just a style nit.

> Source/WebCore/loader/FrameLoader.cpp:1126
> -    m_client->didDisplayInsecureContent();
> +    if (allowed)
> +        m_client->didDisplayInsecureContent();

I'd send the URL through too.

> Source/WebCore/loader/FrameLoaderClient.h:301
> +        virtual bool allowDisplayingInsecureContent(bool enabledPerSettings) { return enabledPerSettings; }

No URL here?

[Thomas Sepez]

re: no URL in didDisplayInsecureContent().  Predates this CL, I'd like to leave it for now, as I believe changing that API will ripple through all the platforms as in the first patch I submitted.

re: tests.  Got a pointer to a layout test for one of the other allow options?  I didn't kick out anything with a few minutes of grepping.

[Adam Barth]

> re: tests.  Got a pointer to a layout test for one of the other allow options?  I didn't kick out anything with a few minutes of grepping.

http://trac.webkit.org/changeset/87860

[Thomas Sepez]

Created attachment 96447 [details]
insecure content policy calls into client, plus new tests

[Adam Barth]

Comment on attachment 96447 [details]
insecure content policy calls into client, plus new tests

Fantastic.

[WebKit Review Bot]

Comment on attachment 96447 [details]
insecure content policy calls into client, plus new tests

Clearing flags on attachment: 96447

Committed r88377: <http://trac.webkit.org/changeset/88377>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.