Bug 104578 - [GTK][WK2] New API to detect display/execution of insecure content
Summary: [GTK][WK2] New API to detect display/execution of insecure content
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mario Sanchez Prada
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2012-12-10 11:28 PST by Mario Sanchez Prada
Modified: 2012-12-12 04:02 PST (History)
9 users (show)

See Also:


Attachments
Patch proposal + Unit tests (22.27 KB, patch)
2012-12-10 13:54 PST, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch proposal + Unit tests (22.10 KB, patch)
2012-12-11 02:14 PST, Mario Sanchez Prada
mrobinson: review+
mrobinson: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mario Sanchez Prada 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 Mario Sanchez Prada 2012-12-10 13:54:39 PST
Created attachment 178630 [details]
Patch proposal + Unit tests

Here comes a patch proposal. Asking for review
Comment 2 WebKit Review Bot 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 Mario Sanchez Prada 2012-12-11 02:14:06 PST
Created attachment 178754 [details]
Patch proposal + Unit tests

Second patch after getting some feedback from Carlos in the hackfest
Comment 4 Martin Robinson 2012-12-12 02:40:24 PST
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.
Comment 5 Mario Sanchez Prada 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])
> 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 Mario Sanchez Prada 2012-12-12 04:02:30 PST
Committed r137450: <http://trac.webkit.org/changeset/137450>