GNOME Bugzilla – Bug 666808
Padlock displayed incorrectly on SSL pages with non-SSL content
Last modified: 2014-08-27 12:53:49 UTC
On a https:// page, padlock icon is shown incorrectly in case some of the page resources are loaded via http://.
Created attachment 280460 [details] [review] ephy-web-view: remove unneeded/unused security levels Once upon a time, we used these security levels to indicate how strong the crypto was. Nowadays the crypto is either good enough or it isn't, like in other modern browsers, so we can get rid of these levels. Also, there's no difference between INSECURE and UNKNOWN.
Created attachment 280461 [details] [review] Do not display secure lock on pages with mixed content Since an attacker can modify some contents of these pages, we should display a broken lock instead of a secure lock, following the convention of major browsers. This is patch is ready for review, but should not be pushed yet as I need to add additional UI to indicate what is going on, since the user will be confused when he clicks on the broken lock icon and the cert viewer shows that the cert is valid. (Planning to use a popover for this, like Firefox does.)
Review of attachment 280460 [details] [review]: Ok. It makes sense.
Review of attachment 280461 [details] [review]: ::: embed/ephy-web-view.c @@ +1873,3 @@ +{ + EphyWebView *view = EPHY_WEB_VIEW (web_view); + EphyWebViewPrivate *priv = view->priv; This is unused ::: embed/ephy-web-view.h @@ +53,3 @@ EPHY_WEB_VIEW_STATE_IS_INSECURE, EPHY_WEB_VIEW_STATE_IS_BROKEN, + EPHY_WEB_VIEW_STATE_IS_PARTIALLY_SECURE, Why don't we use EPHY_WEB_VIEW_STATE_MIXES_CONTENT or something like that to make it clearer what partial security means? ::: src/ephy-window.c @@ +753,3 @@ + /* Mixed content detected */ + state = EPHY_LOCATION_LOCK_STATE_INSECURE; + show_lock = TRUE; So, in case of mixed content we show the same lock icon to open the certificate?
(In reply to comment #4) > ::: embed/ephy-web-view.h > @@ +53,3 @@ > EPHY_WEB_VIEW_STATE_IS_INSECURE, > EPHY_WEB_VIEW_STATE_IS_BROKEN, > + EPHY_WEB_VIEW_STATE_IS_PARTIALLY_SECURE, > > Why don't we use EPHY_WEB_VIEW_STATE_MIXES_CONTENT or something like that to > make it clearer what partial security means? I chose IS_PARTIALLY_SECURE so that I could get the word "secure" in there. How about this naming scheme, or something similar: EPHY_WEB_VIEW_SECURITY_LEVEL_INSECURE, EPHY_WEB_VIEW_SECURITY_LEVEL_BROKEN, EPHY_WEB_VIEW_SECURITY_LEVEL_MIXED_CONTENT, EPHY_WEB_VIEW_SECURITY_LEVEL_SECURE > So, in case of mixed content we show the same lock icon to open the > certificate? A broken lock icon, but I'm thinking we might want to do a warning icon instead, since that is what Firefox does and I think GNOME has a similar one.
Created attachment 280556 [details] [review] ephy-web-view: improve security levels Once upon a time, we used these security levels to indicate how strong the crypto was. Nowadays the crypto is either good enough or it isn't, like in other modern browsers, so we can get rid of these levels. Use a better naming convention, for improved clarity. Also, there's no difference between INSECURE and UNKNOWN. To avoid any possible confusion, let's call this NONE.
Created attachment 280557 [details] [review] ephy-location-entry: Improve security levels
Created attachment 280558 [details] [review] Do not display secure lock on pages with mixed content Since an attacker can modify some contents of these pages, we should display a broken lock instead of a secure lock, following the convention of major browsers.
Created attachment 280559 [details] [review] Do not display secure lock on pages with mixed content Since an attacker can modify some contents of these pages, we should display a warning icon instead of a secure lock, following the convention of major browsers.
Created attachment 280756 [details] [review] Display security status in a popover Instead of bringing up the certificate details dialog when clicking on the lock icon in the address bar, instead bring up a popover that displays information about the security status of this site, including a button to open the certificate dialog. First draft
Review of attachment 280756 [details] [review]: Please, add the new .c file to the POTFILES.in.
(In reply to comment #11) > Please, add the new .c file to the POTFILES.in. Thanks, I don't understand why it's so hard for me to remember to do this....
Created attachment 280757 [details] [review] Display security status in a popover
Created attachment 280848 [details] [review] Display security status in a popover
Created attachment 282944 [details] [review] ephy-web-view: improve security levels Rebased
Created attachment 282945 [details] [review] ephy-location-entry: Improve security levels Rebased
Created attachment 282946 [details] [review] Do not display secure lock on pages with mixed content Rebased
Created attachment 282947 [details] [review] Display security status in a popover Tried to make it look better; notably, no longer displays the issuer.
Created attachment 282948 [details] [review] Open certificate popover if title box lock clicked We show two locks in the header bar: the lock in the location entry, and the lock in the title box. Previously, only the lock in the location entry was actually functional, but clicking the lock in the title box ought to work, too, since you should not need to open the location entry to perform any actions. Arguably the lock should be removed from the location entry so that this functionality only exists in one place, but this patch leaves both locks intact.
Test on this page (a secure site), https://www.ssllabs.com/ssltest/viewMyClient.html (for mixed content), and https://www.gentoo.org/ ("insecure" because the chain is unordered). There is one big problem remaining: if you visit a site with mixed content, visit another page, then return with the back or forward button, the WebKitWebView's insecure-content-detected signal will not be emitted.
Created attachment 282950 [details] [review] Display security status in a popover Forgot to destroy the popover when it is closed
Created attachment 282951 [details] [review] Open certificate popover if title box lock clicked
Review of attachment 282944 [details] [review]: This looks good to me, but I wonder if we should move the enum to somewhere in lib, and use that enum from everywhere, instead of defining another one for the location entry
Review of attachment 282945 [details] [review]: I would merge this and the previous commit into a single commit that unifies the enums somewhere in lib, if possible. ::: lib/widgets/ephy-location-entry.c @@ +1644,3 @@ + switch (state) { + case EPHY_LOCATION_LOCK_STATE_NO_SECURITY: + /* Fall through, but this icon should not be displayed... */ Why not breaking the switch then?
Review of attachment 282946 [details] [review]: The commit message is a bit confusing, it sounds to me like nothing is shown for mixed content. ::: lib/widgets/ephy-location-entry.c @@ +1644,2 @@ + icon_name = ephy_location_lock_state_to_icon_name (state); + if (strlen (icon_name) == 0) A NULL check would be better here, I'd say. @@ +1704,3 @@ + switch (state) { + case EPHY_LOCATION_LOCK_STATE_NO_SECURITY: + result = ""; Use NULL instead of "" ::: src/ephy-title-box.c @@ +730,3 @@ + g_object_set (priv->lock_image, + "icon-name", ephy_location_lock_state_to_icon_name (state), + NULL); Why do you need to move this?
Review of attachment 282950 [details] [review]: ::: lib/widgets/ephy-certificate-popover.c @@ +70,3 @@ + + g_clear_pointer (&priv->address, g_free); + g_clear_pointer (&priv->hostname, g_free); You don't need this, the properties are construct only, so this will only be called once. @@ +86,3 @@ + EphyCertificatePopoverPrivate *priv = popover->priv; + + g_clear_object (&priv->certificate); Ditto. @@ +218,3 @@ + priv->tls_errors); + gtk_window_set_destroy_with_parent (GTK_WINDOW (dialog), TRUE); + g_signal_connect (GTK_DIALOG (dialog), "response", g_signal_connect receives a gpointer, you don't need the GTK_DIALOG cast. @@ +295,3 @@ + "The state of the lock icon", + EPHY_TYPE_LOCATION_LOCK_STATE, + 0, Use the enum name here better. @@ +322,3 @@ + gtk_widget_set_margin_top (certificate_button, 5); + gtk_widget_set_receives_default (certificate_button, FALSE); + g_signal_connect (GTK_BUTTON (certificate_button), "clicked", You don't need the GTK_BUTTON cast here either. @@ +362,3 @@ + NULL)); + + return popover; You don't need the local variable ::: src/ephy-window.c @@ +3160,3 @@ + gtk_popover_set_pointing_to (GTK_POPOVER (certificate_popover), &lock_position); + g_signal_connect_swapped (GTK_POPOVER (certificate_popover), "closed", + G_CALLBACK (gtk_widget_destroy), certificate_popover); Why swapped? you are passing the instance as user_data. You can use g_signal_connect I think, and you don't need the GTK_POPOVER cast.
Review of attachment 282951 [details] [review]: I really like the idea of not having to switch to entry mode just to click the lock icon. ::: src/ephy-title-box.c @@ +371,3 @@ LOG ("button-press-event title-box %p event %p", title_box, event); + lock_allocation = g_new (GtkAllocation, 1); Use a stack allocated GtkAllocation. @@ +477,3 @@ + G_TYPE_NONE, + 1, + GDK_TYPE_RECTANGLE); Use GDK_TYPE_RECTANGLE | G_SIGNAL_TYPE_STATIC_SCOPE to avoid copies of the rectangle.
(In reply to comment #24) > ::: lib/widgets/ephy-location-entry.c > @@ +1644,3 @@ > + switch (state) { > + case EPHY_LOCATION_LOCK_STATE_NO_SECURITY: > + /* Fall through, but this icon should not be displayed... */ > > Why not breaking the switch then? Because priv->lock_gicon is unreffed at the start of that function, so it needs to be left in a valid state. We could change that unref to a g_clear_object(), but it seems easiest to just leave the icon always valid? I guess it doesn't really matter either way.
(In reply to comment #25) > Why do you need to move this? I guess I moved it by mistake. .(In reply to comment #27) > Review of attachment 282951 [details] [review]: > > I really like the idea of not having to switch to entry mode just to click the > lock icon. The question is: do we want the lock icon in the location entry at all? It's the only thing in the location entry besides the address right now. > Use GDK_TYPE_RECTANGLE | G_SIGNAL_TYPE_STATIC_SCOPE to avoid copies of the > rectangle. Oooooh, cool. (In reply to comment #26) > @@ +218,3 @@ > + priv->tls_errors); > + gtk_window_set_destroy_with_parent (GTK_WINDOW (dialog), TRUE); > + g_signal_connect (GTK_DIALOG (dialog), "response", > > g_signal_connect receives a gpointer, you don't need the GTK_DIALOG cast. This is a habit I developed running Tartan over gnome-games code; it warns if the type does not match exactly. I'll avoid that in Epiphany code since it's indeed unnecessary.
(In reply to comment #28) > Because priv->lock_gicon is unreffed at the start of that function, so it needs > to be left in a valid state. We could change that unref to a g_clear_object(), > but it seems easiest to just leave the icon always valid? I guess it doesn't > really matter either way. Actually, that code is removed in the next patch (which adds ephy_location_lock_state_to_icon_name()), so it doesn't matter.
Created attachment 283056 [details] [review] EphyLocationController: remove unused properties
Created attachment 283057 [details] [review] Merge EphyWebViewSecurityLevel and EphyLocationLockState EphyWebViewSecurityLevel has lots of unused members. Once they're removed, they correspond one-to-one with EphyLocationLockState. Merge these so we don't have to convert between two identical enums.
Created attachment 283058 [details] [review] Remove various show_lock properties EphyTitleBox and EphyLocationEntry now know to show the lock whenever the security level is not EPHY_SECURITY_LEVEL_NO_SECURITY.
Created attachment 283059 [details] [review] Display warning icon if mixed content is detected Since an attacker can modify some contents of these pages, we should display a warning icon instead of a secure lock, following the convention of major browsers.
Created attachment 283060 [details] [review] Display security status in a popover Instead of bringing up the certificate details dialog when clicking on the lock icon in the address bar, instead bring up a popover that displays information about the security status of this site, including a button to open the certificate dialog.
Created attachment 283061 [details] [review] Open certificate popover if title box lock clicked We show two locks in the header bar: the lock in the location entry, and the lock in the title box. Previously, only the lock in the location entry was actually functional, but clicking the lock in the title box ought to work, too, since you should not need to open the location entry to perform any actions. Arguably the lock should be removed from the location entry so that this functionality only exists in one place, but this patch leaves both locks intact.
Created attachment 283062 [details] [review] EphyLocationController: remove unused properties
Created attachment 283063 [details] [review] Merge EphyWebViewSecurityLevel and EphyLocationLockState
Created attachment 283064 [details] [review] Remove various show_lock properties
Created attachment 283065 [details] [review] Display warning icon if mixed content is detected
Created attachment 283066 [details] [review] Display security status in a popover
Created attachment 283067 [details] [review] Open certificate popover if title box lock clicked
Review of attachment 283065 [details] [review]: Fix my comments below before pushing, please. ::: lib/ephy-security-levels.c @@ +33,3 @@ +ephy_security_level_to_icon_name (EphySecurityLevel level) +{ + const char *result; This doesn't seem to follow the coding style for new files ::: lib/ephy-security-levels.h @@ +39,3 @@ } EphySecurityLevel; +const char *ephy_security_level_to_icon_name (EphySecurityLevel level); Remove the extra spaces there between char and * and name and ( ::: lib/widgets/ephy-location-entry.c @@ +1601,1 @@ + g_return_if_fail (security_level != EPHY_SECURITY_LEVEL_NO_SECURITY); Use a g_assert here instead.
Review of attachment 283066 [details] [review]: Fix my comments here as well before pushing ::: lib/widgets/ephy-certificate-popover.h @@ +45,3 @@ +struct _EphyCertificatePopover +{ + GtkPopover parent_object; The header file should also use a 2 space indentation. ::: po/POTFILES.in @@ +14,3 @@ embed/ephy-find-toolbar.c embed/ephy-web-view.c +lib/ephy-certificate-popover.c lib/widgets/ephy-certificate-popover.c
Attachment 283062 [details] pushed as 7afd509 - EphyLocationController: remove unused properties Attachment 283063 [details] pushed as d8ec235 - Merge EphyWebViewSecurityLevel and EphyLocationLockState Attachment 283064 [details] pushed as 487ff82 - Remove various show_lock properties Attachment 283065 [details] pushed as 6b59dd4 - Display warning icon if mixed content is detected Attachment 283066 [details] pushed as 7c5686d - Display security status in a popover Attachment 283067 [details] pushed as 564a695 - Open certificate popover if title box lock clicked
(In reply to comment #44) > Review of attachment 283066 [details] [review]: > > Fix my comments here as well before pushing Should all be fixed, thanks!
(In reply to comment #45) > Attachment 283067 [details] pushed as 564a695 - Open certificate popover if title box > lock clicked So, you pushed this one unreviewed, please only push patches marked as accepted-commit_now. I wanted to discuss it before, because even though I like the idea of not having to go to the location entry to click on the lock icon, I'm not sure about having it in both places. Also, in the header bar it's not that obvious that the icon is clickable.
Another advantage of making the lock icon clikable in the title mode is that it could be accessed also in app mode, however it doesn't seem to work with your patch.
(In reply to comment #47) > So, you pushed this one unreviewed, please only push patches marked as > accepted-commit_now. Ah, I know better than to do that, I just didn't notice that that one's attachment status was different, sorry. :/ > I wanted to discuss it before, because even though I like > the idea of not having to go to the location entry to click on the lock icon, > I'm not sure about having it in both places. Also, in the header bar it's not > that obvious that the icon is clickable. I agree. I'd favor removing it from the location entry, but you're right that it's more obviously clickable in the location entry. I think the reason is that the color changes from gray to black when you hover over it.
(In reply to comment #48) > Another advantage of making the lock icon clikable in the title mode is that it > could be accessed also in app mode, however it doesn't seem to work with your > patch. Looks like the page URL is only displayed for non-HTTPS sites....
(In reply to comment #50) > Looks like the page URL is only displayed for non-HTTPS sites.... Whoops, I was launching my non-jhbuild version. I see the problem: just the lock is missing....
Created attachment 283513 [details] [review] ephy-title-box: allow clicking lock in app mode
(In reply to comment #52) > Created an attachment (id=283513) [details] [review] > ephy-title-box: allow clicking lock in app mode This would be a nice one to land.
Comment on attachment 283513 [details] [review] ephy-title-box: allow clicking lock in app mode Ok, we can land this for now. I wonder if we should actually allow to switch to location in app mode, but with the entry in read only mode. That would also allow to copy the current URL.
I don't know if a read-only entry would not be confusing :/
Attachment 283513 [details] pushed as a8a3680 - ephy-title-box: allow clicking lock in app mode