Bug 104578 – [GTK][WK2] New API to detect display/execution of insecure content

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Mario Sanchez Prada]

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).

[Mario Sanchez Prada]

Created attachment 178630 [details]
Patch proposal + Unit tests

Here comes a patch proposal. Asking for review

[WebKit Review Bot]

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

[Mario Sanchez Prada]

Created attachment 178754 [details]
Patch proposal + Unit tests

Second patch after getting some feedback from Carlos in the hackfest

[Martin Robinson]

Comment on attachment 178754 [details]
Patch proposal + Unit tests

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.

[Mario Sanchez Prada]

Thanks for the review Martin. Will address those issues right before landing.

(In reply to comment #4)
> (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.

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.

[Mario Sanchez Prada]

Committed r137450: <http://trac.webkit.org/changeset/137450>