Bug 104578

Summary: [GTK][WK2] New API to detect display/execution of insecure content
Product: WebKit Reporter: Mario Sanchez Prada <mario@webkit.org>
Component: WebKit GtkAssignee: Mario Sanchez Prada <mario@webkit.org>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth@webkit.org, cgarcia@igalia.com, dpranke@chromium.org, gns@gnome.org, gyuyoung.kim@samsung.com, levin@chromium.org, mrobinson@webkit.org, rakuco@webkit.org, webkit.review.bot@gmail.com
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch proposal + Unit tests
none
Patch proposal + Unit tests mrobinson: review+, mrobinson: commit‑queue-

Description From 2012-12-10 11:28:40 PST
We need to expose this to applications so they can notify the user in some ways when content from untrusted sources (e.g. from http) has been loaded from a document coming through a trusted connection (https).
------- Comment #1 From 2012-12-10 13:54:39 PST -------
Created an attachment (id=178630) [details]
Patch proposal + Unit tests

Here comes a patch proposal. Asking for review
------- Comment #2 From 2012-12-10 13:56:03 PST -------
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 From 2012-12-11 02:14:06 PST -------
Created an attachment (id=178754) [details]
Patch proposal + Unit tests

Second patch after getting some feedback from Carlos in the hackfest
------- Comment #4 From 2012-12-12 02:40:24 PST -------
(From update of attachment 178754 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=178754&action=review

Looks good, as long as cgarcia is okay with the fact that WEBKIT_INSECURE_CONTENT_RUN is not called WEBKIT_INSECURE_CONTENT_EVENT_RUN and WEBKIT_INSECURE_CONTENT_DISPLAYED is not called WEBKIT_INSECURE_CONTENT_EVENT_DISPLAYED. I don't think they are too long, but it's not a huge deal.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1245
> +     * This signal is emitted when insecure content has been detected
> +     * in a page loaded through a secure connection. This typically
> +     * means that an external resource from an unstrusted source
> +     * (e.g. non HTTPS) has been run or displayed.
> +     *

You might want to try to more clearly describe the situation as "mixing of HTTPS and non-HTTPS content"

> Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:92
> +    static void insecure_content_detected_callback(WebKitWebView* webView, WebKitInsecureContentEvent event, InsecureContentTest* test)

Please use WebKit naming conventions here.

> Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:148
> +        char* contents;
> +        gsize length;
> +
> +        g_file_get_contents(pathToFile.get(), &contents, &length, 0);

Okay. Super nit, but wouldn't it make more sense to "glue" the declarations with the statement that uses them?

> Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:157
> +        gsize length;
> +
> +        g_file_get_contents(pathToFile.get(), &contents, &length, 0);

Same here.

> Tools/Scripts/webkitpy/style/checker.py:184
> +      "-readability/enum_casing",
>        "-whitespace/parens"]),
> +
> +    ([# The GTK+ API use upper case, underscore separated, words in
> +      # certain types of enums (e.g. signals, properties).
> +      "Source/WebKit2/UIProcess/API/gtk"],
> +     ["-readability/enum_casing"]),

I think maybe this is a bit too broad. We want to detect enum style violations in these files too. Perhaps just turn webkitWebViewInsecureContentDetected into webkitWebViewInsecureContentRun and webkitWebViewInsecureContentDisplayed.
------- Comment #5 From 2012-12-12 03:58:56 PST -------
Thanks for the review Martin. Will address those issues right before landing.

(In reply to comment #4)
> (From update of attachment 178754 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178754&action=review
> 
> Looks good, as long as cgarcia is okay with the fact that WEBKIT_INSECURE_CONTENT_RUN is not called 
> WEBKIT_INSECURE_CONTENT_EVENT_RUN and WEBKIT_INSECURE_CONTENT_DISPLAYED is not called 
> WEBKIT_INSECURE_CONTENT_EVENT_DISPLAYED. I don't think they are too long, but it's not a huge deal.

Yes, Carlos is fine with that too.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1245
> > +     * This signal is emitted when insecure content has been detected
> > +     * in a page loaded through a secure connection. This typically
> > +     * means that an external resource from an unstrusted source
> > +     * (e.g. non HTTPS) has been run or displayed.
> > +     *
> 
> You might want to try to more clearly describe the situation as "mixing of HTTPS and non-HTTPS content"

Ok.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:92
> > +    static void insecure_content_detected_callback(WebKitWebView* webView, WebKitInsecureContentEvent event, InsecureContentTest* test)
> 
> Please use WebKit naming conventions here.

Oops! Sorry.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:148
> > +        char* contents;
> > +        gsize length;
> > +
> > +        g_file_get_contents(pathToFile.get(), &contents, &length, 0);
> 
> Okay. Super nit, but wouldn't it make more sense to "glue" the declarations with the statement that uses them?

Ok.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:157
> > +        gsize length;
> > +
> > +        g_file_get_contents(pathToFile.get(), &contents, &length, 0);
> 
> Same here.

Ok.

> > Tools/Scripts/webkitpy/style/checker.py:184
> > +      "-readability/enum_casing",
> >        "-whitespace/parens"]),
> > +
> > +    ([# The GTK+ API use upper case, underscore separated, words in
> > +      # certain types of enums (e.g. signals, properties).
> > +      "Source/WebKit2/UIProcess/API/gtk"],
> > +     ["-readability/enum_casing"]),
> 
> I think maybe this is a bit too broad. We want to detect enum style violations in these files too. 
> Perhaps just turn webkitWebViewInsecureContentDetected into webkitWebViewInsecureContentRun
> and webkitWebViewInsecureContentDisplayed.

I think there's no need to change anything in this regard after the clarifications during the hackfest, so I'll leave it as it is now.
------- Comment #6 From 2012-12-12 04:02:30 PST -------
Committed r137450: <http://trac.webkit.org/changeset/137450>