Bug 138127 - Insecure content warnings not emitted when page is restored from page cache
Summary: Insecure content warnings not emitted when page is restored from page cache
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: History (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
: 134882 (view as bug list)
Depends on:
Blocks: 140625
  Show dependency treegraph
 
Reported: 2014-10-27 22:23 PDT by Michael Catanzaro
Modified: 2020-02-19 13:33 PST (History)
21 users (show)

See Also:


Attachments
Patch (13.18 KB, patch)
2014-10-27 22:34 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (13.67 KB, patch)
2014-10-28 15:14 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (24.49 KB, patch)
2015-02-02 15:27 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2014-10-27 22:23:32 PDT
When a page with insecure content is restored from the page cache, it needs to emit the normal insecure content detected events didDisplayInsecureContent or didRunInsecureContent. [1] is a quick test in a browser that displays insecure content warnings (like Epiphany). Just note the insecure content warning, then click any link on that page, then click back and note that there is no longer any warning.

This patch mostly works so I'm posting it for feedback. I added GTK+ test cases to make sure not just that the insecure-content-detected signal fires, but also that this occurs after load-committed, which is what Epiphany expects and I guess is what is probably desired? (The problem is that one of the assertions I added to check this is failing, so I guess this is likely broken, but I need to debug it more.)

[1] https://www.ssllabs.com/ssltest/viewMyClient.html
Comment 1 Michael Catanzaro 2014-10-27 22:34:21 PDT
Created attachment 240529 [details]
Patch
Comment 2 WebKit Commit Bot 2014-10-27 22:36:32 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 3 Michael Catanzaro 2014-10-28 15:14:23 PDT
Created attachment 240574 [details]
Patch

Fixed that.
Comment 4 Brady Eidson 2014-10-30 10:04:15 PDT
Comment on attachment 240574 [details]
Patch

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

This is basically a new feature, and I think it'd be important to test cross platform.

> Source/WebCore/history/CachedFrame.cpp:107
> +    if (frame.didDisplayInsecureContent()) {

How is it okay to check this on the frame when (in the common main-frame case) it is being reused from the previously shown content?

> Source/WebCore/history/CachedFrame.cpp:112
> +    if (frame.didRunInsecureContent()) {

Ditto.

> Source/WebCore/history/CachedFrame.cpp:114
> +        frame.loader().client().didRunInsecureContent(m_document->securityOrigin(), m_url);

"did run insecure content" being overloaded for the page cache restore case seems weird.

> Source/WebCore/loader/FrameLoader.cpp:-1805
> +        dispatchDidCommitLoad();
> +
> +        // CachedPage::restore() emits events (e.g. didDisplayInsecureContent) that are good to have after didCommitLoad.
>          // FIXME: This API should be turned around so that we ground CachedPage into the Page.
>          cachedPage->restore(*m_frame.page());
>  
> -        dispatchDidCommitLoad();

Did you run the WK2 API tests before making this change?  I suspect they will be very relevant.

Also, as is suggested in this code and your comment, FrameLoader is usually incharge of dispatching events and callbacks etc etc.  Yet now some of that logic is being moved into the cached frame restore.  That doesn't seem good.

> Source/WebCore/page/Frame.h:269
> +        void setDidDisplayInsecureContent(bool value = true) { m_didDisplayInsecureContent = value; }
> +        bool didDisplayInsecureContent() const { return m_didDisplayInsecureContent; }
> +        void setDidRunInsecureContent(bool value = true) { m_didRunInsecureContent = value; }
> +        bool didRunInsecureContent() const { return m_didRunInsecureContent; }

Why is Frame the right place for these?  Frames are commonly reused between navigations (and the main frame always is)

I'm not confident the values are kept fully up to date in this patch. It would be much better to attach these values to something that *does* change with navigations.  Document is one example.  DOMWindow is another.  etc etc
Comment 5 Michael Catanzaro 2014-10-30 15:57:18 PDT
Thanks for the review. I'll work on cross-platform tests before I try to address your comments.

(In reply to comment #4)
> This is basically a new feature, and I think it'd be important to test cross
> platform.

I'd call it a minor security bug rather than a feature, since it's unsafe for a port to use these events unless it completely blocks all mixed content.

> > Source/WebCore/history/CachedFrame.cpp:107
> > +    if (frame.didDisplayInsecureContent()) {
> 
> How is it okay to check this on the frame when (in the common main-frame
> case) it is being reused from the previously shown content?

You're right; now loading a single page with insecure content causes unrelated future loads from the page cache to emit these events, like you suspected. That's very bad. I'll add a test for this.

> Did you run the WK2 API tests before making this change?  I suspect they
> will be very relevant.

If we're thinking of the same tests (the Google framework ones), those all passed.
Comment 6 Michael Catanzaro 2015-02-02 15:27:49 PST
Created attachment 245906 [details]
Patch
Comment 7 Michael Catanzaro 2015-02-02 15:30:09 PST
Brady, thanks for the last review and sorry for the delay on this. I think this second attempt addresses your previous comments.

Carlos might want to approve the GTK-specific changes.
Comment 8 Alexey Proskuryakov 2015-11-12 15:58:04 PST
Strangely, the symptom didn't reproduce in Safari when I tested going back to <https://www.ssllabs.com/ssltest/viewMyClient.html> - it just won't go into page cache. I wonder if this is something that we do intentionally.
Comment 9 Michael Catanzaro 2015-11-13 10:09:32 PST
I did consider rejecting the page from the page cache as a potential solution for this (and the equivalent favicon cache issue). The disadvantage is that would obviously make affected sites slower.

OTOH, such sites are broken, and I'm fine with them receiving a performance penalty for that.
Comment 10 Michael Catanzaro 2016-01-02 10:12:27 PST
Comment on attachment 245906 [details]
Patch

I think my patch should store a list of insecure URIs so that they can be reported properly, rather than pretending that the entire frame is insecure.
Comment 11 Michael Catanzaro 2018-06-04 22:17:05 PDT
*** Bug 134882 has been marked as a duplicate of this bug. ***
Comment 12 Michael Catanzaro 2018-06-04 22:17:28 PDT
(In reply to Michael Catanzaro from comment #11)
> *** Bug 134882 has been marked as a duplicate of this bug. ***

This (private) bug has another testcase.
Comment 13 Michael Catanzaro 2020-02-19 13:33:23 PST
Hm, I thought this might be fixed by r206006, but the reproducer in my first comment is still failing so maybe not.