GNOME.org
 

[epiphany/mcatanzaro/password-manager-frames] Password manager should work in iframes

commit 9fdf4b7482bfd2d761cec93440391e769f691078
Author: Michael Catanzaro <mcatanzaro igalia com>
Date:   Wed Apr 24 23:01:10 2019 -0500

    Password manager should work in iframes
    
    Currently the password manager does not work on reddit.com because we
    run Ephy JS only in the main frame, never in iframes, but the login form
    on reddit is an iframe. What a shame! Let's change this.
    
    It requires a new webkit_frame_get_id() API to be able to track frames
    across the process boundary, and also a new
    form-controls-associated-for-frame signal since sadly we can't add new
    parameters to signals without breaking API.
    
    Note this only works if the frame's origin is the same as the main
    frame's origin. If the iframe has a different security origin, then the
    password will not be saved at all. This is because the UI process
    ensures the save request matches the page's origin to prevent a
    compromised web process from overwriting passwords for other origins.

 embed/ephy-embed-shell.c                 | 13 +++--
 embed/ephy-web-extension-proxy.c         |  8 +--
 embed/ephy-web-extension-proxy.h         |  4 +-
 embed/web-extension/ephy-web-extension.c | 88 +++++++++++++++++++++++---------
 embed/web-extension/resources/js/ephy.js | 32 +++++++-----
 meson.build                              |  2 +-
 6 files changed, 100 insertions(+), 47 deletions(-)
---
diff --git a/embed/ephy-embed-shell.c b/embed/ephy-embed-shell.c
index 18700b136..b34c2bb57 100644
--- a/embed/ephy-embed-shell.c
+++ b/embed/ephy-embed-shell.c
@@ -346,6 +346,7 @@ typedef struct {
   char *origin;
   gint32 promise_id;
   guint64 page_id;
+  guint64 frame_id;
 } PasswordManagerData;
 
 static void
@@ -374,7 +375,7 @@ password_manager_query_finished_cb (GList               *records,
                                                                                    data->page_id,
                                                                                    data->origin);
   if (proxy)
-    ephy_web_extension_proxy_password_query_response (proxy, username, password, data->promise_id, 
data->page_id);
+    ephy_web_extension_proxy_password_query_response (proxy, username, password, data->promise_id, 
data->frame_id);
 
   password_manager_data_free (data);
 }
@@ -419,11 +420,13 @@ web_extension_password_manager_query_received_cb (WebKitUserContentManager *mana
   g_autofree char *password_field = property_to_string_or_null (value, "passwordField");
   gint32 promise_id = property_to_int32 (value, "promiseID");
   guint64 page_id = property_to_uint64 (value, "pageID");
+  guint64 frame_id = property_to_uint64 (value, "frameID");
 
   PasswordManagerData *data = g_new (PasswordManagerData, 1);
   data->shell = g_object_ref (shell);
   data->promise_id = promise_id;
   data->page_id = page_id;
+  data->frame_id = frame_id;
   data->origin = g_steal_pointer (&origin);
 
   ephy_password_manager_query (priv->password_manager,
@@ -510,7 +513,10 @@ web_extension_password_manager_save_real (EphyEmbedShell *shell,
 
   /* This also sanity checks that a page isn't saving websites for
    * other origins. Remember the request comes from the untrusted web
-   * process and we have to make sure it's not being evil here.
+   * process and we have to make sure it's not being evil here. This
+   * could also happen even without malice if the origin of a subframe
+   * doesn't match the origin of the main frame (in which case we'll
+   * refuse to save the password).
    */
   view = ephy_embed_shell_get_view_for_page_id (shell, page_id, origin);
   if (!view)
@@ -566,13 +572,14 @@ web_extension_password_manager_query_usernames_received_cb (WebKitUserContentMan
   g_autofree char *origin = property_to_string_or_null (value, "origin");
   gint32 promise_id = property_to_int32 (value, "promiseID");
   guint64 page_id = property_to_uint64 (value, "pageID");
+  guint64 frame_id = property_to_uint64 (value, "frameID");
 
   GList *usernames;
   usernames = ephy_password_manager_get_usernames_for_origin (priv->password_manager, origin);
 
   EphyWebExtensionProxy *proxy = ephy_embed_shell_get_extension_proxy_for_page_id (shell, page_id, origin);
   if (proxy)
-    ephy_web_extension_proxy_password_query_usernames_response (proxy, usernames, promise_id, page_id);
+    ephy_web_extension_proxy_password_query_usernames_response (proxy, usernames, promise_id, frame_id);
 }
 
 static void
diff --git a/embed/ephy-web-extension-proxy.c b/embed/ephy-web-extension-proxy.c
index 441491407..a52a98ee3 100644
--- a/embed/ephy-web-extension-proxy.c
+++ b/embed/ephy-web-extension-proxy.c
@@ -298,7 +298,7 @@ void
 ephy_web_extension_proxy_password_query_usernames_response (EphyWebExtensionProxy *web_extension,
                                                             GList                 *users,
                                                             gint32                 promise_id,
-                                                            guint64                page_id)
+                                                            guint64                frame_id)
 {
   if (!web_extension->proxy)
     return;
@@ -310,7 +310,7 @@ ephy_web_extension_proxy_password_query_usernames_response (EphyWebExtensionProx
 
   g_dbus_proxy_call (web_extension->proxy,
                      "PasswordQueryUsernamesResponse",
-                     g_variant_new ("(asit)", &builder, promise_id, page_id),
+                     g_variant_new ("(asit)", &builder, promise_id, frame_id),
                      G_DBUS_CALL_FLAGS_NONE,
                      -1,
                      web_extension->cancellable,
@@ -322,14 +322,14 @@ ephy_web_extension_proxy_password_query_response (EphyWebExtensionProxy *web_ext
                                                   const char            *username,
                                                   const char            *password,
                                                   gint32                 promise_id,
-                                                  guint64                page_id)
+                                                  guint64                frame_id)
 {
   if (!web_extension->proxy)
     return;
 
   g_dbus_proxy_call (web_extension->proxy,
                      "PasswordQueryResponse",
-                     g_variant_new ("(ssit)", username ?: "", password ?: "", promise_id, page_id),
+                     g_variant_new ("(ssit)", username ?: "", password ?: "", promise_id, frame_id),
                      G_DBUS_CALL_FLAGS_NONE,
                      -1,
                      web_extension->cancellable,
diff --git a/embed/ephy-web-extension-proxy.h b/embed/ephy-web-extension-proxy.h
index 10652d41f..58c8358cd 100644
--- a/embed/ephy-web-extension-proxy.h
+++ b/embed/ephy-web-extension-proxy.h
@@ -45,10 +45,10 @@ void                   ephy_web_extension_proxy_history_clear
 void                   ephy_web_extension_proxy_password_query_usernames_response         
(EphyWebExtensionProxy *web_extension,
                                                                                            GList             
    *users,
                                                                                            gint32            
     promise_id,
-                                                                                           guint64           
     page_id);
+                                                                                           guint64           
     frame_id);
 void                   ephy_web_extension_proxy_password_query_response                   
(EphyWebExtensionProxy *web_extension,
                                                                                            const char        
    *username,
                                                                                            const char        
    *password,
                                                                                            gint32            
     promise_id,
-                                                                                           guint64           
     page_id);
+                                                                                           guint64           
     frame_id);
 G_END_DECLS
diff --git a/embed/web-extension/ephy-web-extension.c b/embed/web-extension/ephy-web-extension.c
index 9595e6514..b48d44f78 100644
--- a/embed/web-extension/ephy-web-extension.c
+++ b/embed/web-extension/ephy-web-extension.c
@@ -57,6 +57,8 @@ struct _EphyWebExtension {
   WebKitScriptWorld *script_world;
 
   gboolean is_private_profile;
+
+  GHashTable *frames_map;
 };
 
 static const char introspection_xml[] =
@@ -87,12 +89,12 @@ static const char introspection_xml[] =
   "    <arg type='s' name='username' direction='in'/>"
   "    <arg type='s' name='password' direction='in'/>"
   "    <arg type='i' name='promise_id' direction='in'/>"
-  "    <arg type='t' name='page_id' direction='in'/>"
+  "    <arg type='t' name='frame_id' direction='in'/>"
   "  </method>"
   "  <method name='PasswordQueryUsernamesResponse'>"
   "    <arg type='as' name='users' direction='in'/>"
   "    <arg type='i' name='promise_id' direction='in'/>"
-  "    <arg type='t' name='page_id' direction='in'/>"
+  "    <arg type='t' name='frame_id' direction='in'/>"
   "  </method>"
   " </interface>"
   "</node>";
@@ -197,8 +199,8 @@ static void
 web_page_will_submit_form (WebKitWebPage            *web_page,
                            WebKitDOMHTMLFormElement *dom_form,
                            WebKitFormSubmissionStep  step,
-                           WebKitFrame              *frame,
                            WebKitFrame              *source_frame,
+                           WebKitFrame              *target_frame,
                            GPtrArray                *text_field_names,
                            GPtrArray                *text_field_values)
 {
@@ -222,34 +224,56 @@ web_page_will_submit_form (WebKitWebPage            *web_page,
   extension = ephy_web_extension_get ();
   js_context = webkit_frame_get_js_context_for_script_world (source_frame, extension->script_world);
   js_ephy = jsc_context_get_value (js_context, "Ephy");
-  js_form = webkit_frame_get_js_value_for_dom_object_in_script_world (frame, WEBKIT_DOM_OBJECT (dom_form), 
extension->script_world);
+  js_form = webkit_frame_get_js_value_for_dom_object_in_script_world (source_frame, WEBKIT_DOM_OBJECT 
(dom_form), extension->script_world);
   js_result = jsc_value_object_invoke_method (js_ephy,
                                               "handleFormSubmission",
                                               G_TYPE_UINT64, webkit_web_page_get_id (web_page),
+                                              G_TYPE_UINT64, webkit_frame_get_id (source_frame),
                                               JSC_TYPE_VALUE, js_form,
                                               G_TYPE_NONE);
 }
 
 static char *
-sensitive_form_message_serializer (guint64  page_id,
+sensitive_form_message_serializer (guint64  frame_id,
                                    gboolean is_insecure_action)
 {
   GVariant *variant;
   char *message;
 
-  variant = g_variant_new ("(tb)", page_id, is_insecure_action);
+  variant = g_variant_new ("(tb)", frame_id, is_insecure_action);
   message = g_variant_print (variant, FALSE);
   g_variant_unref (variant);
 
   return message;
 }
 
+static gboolean
+remove_if_value_matches_user_data (gpointer key,
+                                   gpointer value,
+                                   gpointer user_data)
+{
+  return value == user_data;
+}
+
+static void
+frame_destroyed_notify (EphyWebExtension *extension,
+                        GObject          *where_the_object_was)
+{
+  /* This is awkward. We can't just remove the frame from the table
+   * directly since we don't have any way to get its ID except by
+   * checking every entry in the map....
+   */
+  g_hash_table_foreach_remove (extension->frames_map,
+                               remove_if_value_matches_user_data,
+                               where_the_object_was);
+}
+
 static void
 web_page_form_controls_associated (WebKitWebPage    *web_page,
                                    GPtrArray        *elements,
+                                   WebKitFrame      *frame,
                                    EphyWebExtension *extension)
 {
-  WebKitFrame *frame;
   g_autoptr(GPtrArray) form_controls = NULL;
   g_autoptr(JSCContext) js_context = NULL;
   g_autoptr(JSCValue) js_ephy = NULL;
@@ -257,7 +281,6 @@ web_page_form_controls_associated (WebKitWebPage    *web_page,
   g_autoptr(JSCValue) js_result = NULL;
   guint i;
 
-  frame = webkit_web_page_get_main_frame (web_page);
   js_context = webkit_frame_get_js_context_for_script_world (frame, extension->script_world);
 
   form_controls = g_ptr_array_new_with_free_func (g_object_unref);
@@ -276,9 +299,15 @@ web_page_form_controls_associated (WebKitWebPage    *web_page,
   js_result = jsc_value_object_invoke_method (js_ephy,
                                               "formControlsAssociated",
                                               G_TYPE_UINT64, webkit_web_page_get_id (web_page),
+                                              G_TYPE_UINT64, webkit_frame_get_id (frame),
                                               G_TYPE_PTR_ARRAY, form_controls,
                                               JSC_TYPE_VALUE, js_serializer,
                                               G_TYPE_NONE);
+
+  if (!g_hash_table_contains (extension->frames_map, GINT_TO_POINTER (webkit_frame_get_id (frame)))) {
+    g_hash_table_insert (extension->frames_map, GINT_TO_POINTER (webkit_frame_get_id (frame)), frame);
+    g_object_weak_ref (G_OBJECT (frame), (GWeakNotify)frame_destroyed_notify, extension);
+  }
 }
 
 static gboolean
@@ -295,6 +324,9 @@ web_page_context_menu (WebKitWebPage          *web_page,
   g_autoptr(JSCValue) js_value = NULL;
 
   extension = ephy_web_extension_get ();
+  /* FIXME: this is wrong, see https://gitlab.gnome.org/GNOME/epiphany/issues/442
+   * We need a way to get the right frame to use here.
+   */
   frame = webkit_web_page_get_main_frame (web_page);
   js_context = webkit_frame_get_js_context_for_script_world (frame, extension->script_world);
 
@@ -383,24 +415,22 @@ ephy_web_extension_page_created_cb (EphyWebExtension *extension,
   g_signal_connect (web_page, "will-submit-form",
                     G_CALLBACK (web_page_will_submit_form),
                     extension);
-  g_signal_connect (web_page, "form-controls-associated",
+  g_signal_connect (web_page, "form-controls-associated-for-frame",
                     G_CALLBACK (web_page_form_controls_associated),
                     extension);
 }
 
 static JSCValue *
-get_password_manager (EphyWebExtension *self, guint64 page_id)
+get_password_manager (EphyWebExtension *self, guint64 frame_id)
 {
-  WebKitWebPage *page;
   WebKitFrame *frame;
   g_autoptr(JSCContext) js_context = NULL;
   g_autoptr(JSCValue) js_ephy = NULL;
 
-  page = webkit_web_extension_get_page (self->extension, page_id);
-  if (page == NULL)
+  frame = g_hash_table_lookup (self->frames_map, GINT_TO_POINTER (frame_id));
+  if (!frame)
     return NULL;
 
-  frame = webkit_web_page_get_main_frame (page);
   js_context = webkit_frame_get_js_context_for_script_world (frame, self->script_world);
   js_ephy = jsc_context_get_value (js_context, "Ephy");
 
@@ -482,13 +512,13 @@ handle_method_call (GDBusConnection       *connection,
     g_autoptr(JSCValue) ret = NULL;
     g_autoptr(JSCValue) password_manager = NULL;
     gint32 promise_id;
-    guint64 page_id;
+    guint64 frame_id;
 
     users = g_variant_get_strv (g_variant_get_child_value (parameters, 0), NULL);
     g_variant_get_child (parameters, 1, "i", &promise_id);
-    g_variant_get_child (parameters, 2, "t", &page_id);
+    g_variant_get_child (parameters, 2, "t", &frame_id);
 
-    password_manager = get_password_manager (extension, page_id);
+    password_manager = get_password_manager (extension, frame_id);
     if (password_manager != NULL)
       ret = jsc_value_object_invoke_method (password_manager, "_onQueryUsernamesResponse",
                                             G_TYPE_STRV, users, G_TYPE_INT, promise_id, G_TYPE_NONE);
@@ -496,12 +526,12 @@ handle_method_call (GDBusConnection       *connection,
     const char *username;
     const char *password;
     gint32 promise_id;
-    guint64 page_id;
+    guint64 frame_id;
     g_autoptr(JSCValue) ret = NULL;
     g_autoptr(JSCValue) password_manager = NULL;
 
-    g_variant_get (parameters, "(&s&sit)", &username, &password, &promise_id, &page_id);
-    password_manager = get_password_manager (extension, page_id);
+    g_variant_get (parameters, "(&s&sit)", &username, &password, &promise_id, &frame_id);
+    password_manager = get_password_manager (extension, frame_id);
     if (password_manager != NULL)
       ret = jsc_value_object_invoke_method (password_manager, "_onQueryResponse",
                                             G_TYPE_STRING, username,
@@ -516,6 +546,12 @@ static const GDBusInterfaceVTable interface_vtable = {
   NULL
 };
 
+static void
+drop_frame_weak_ref (gpointer key, gpointer value, gpointer extension)
+{
+  g_object_weak_unref (G_OBJECT (value), (GWeakNotify)frame_destroyed_notify, extension);
+}
+
 static void
 ephy_web_extension_dispose (GObject *object)
 {
@@ -534,6 +570,11 @@ ephy_web_extension_dispose (GObject *object)
   g_clear_object (&extension->dbus_connection);
   g_clear_object (&extension->extension);
 
+  if (extension->frames_map) {
+    g_hash_table_foreach (extension->frames_map, drop_frame_weak_ref, extension);
+    g_clear_pointer (&extension->frames_map, g_hash_table_unref);
+  }
+
   G_OBJECT_CLASS (ephy_web_extension_parent_class)->dispose (object);
 }
 
@@ -687,9 +728,6 @@ window_object_cleared_cb (WebKitScriptWorld *world,
   g_autoptr(JSCValue) js_function = NULL;
   g_autoptr(JSCValue) result = NULL;
 
-  if (!webkit_frame_is_main_frame (frame))
-    return;
-
   js_context = webkit_frame_get_js_context_for_script_world (frame, world);
   jsc_context_push_exception_handler (js_context, (JSCExceptionHandler)js_exception_handler, NULL, NULL);
 
@@ -746,6 +784,7 @@ window_object_cleared_cb (WebKitScriptWorld *world,
     g_autoptr(JSCValue) js_password_manager_ctor = jsc_value_object_get_property (js_ephy, 
"PasswordManager");
     g_autoptr(JSCValue) js_password_manager = jsc_value_constructor_call (js_password_manager_ctor,
                                                                           G_TYPE_UINT64, 
webkit_web_page_get_id (page),
+                                                                          G_TYPE_UINT64, webkit_frame_get_id 
(frame),
                                                                           G_TYPE_NONE);
     jsc_value_object_set_property (js_ephy, "passwordManager", js_password_manager);
 
@@ -828,4 +867,7 @@ ephy_web_extension_initialize (EphyWebExtension   *extension,
                                      extension);
 
   extension->uri_tester = ephy_uri_tester_new (adblock_data_dir);
+
+  extension->frames_map = g_hash_table_new_full (g_direct_hash, g_direct_equal,
+                                                 NULL, NULL);
 }
diff --git a/embed/web-extension/resources/js/ephy.js b/embed/web-extension/resources/js/ephy.js
index d3b42e6cc..a59b3eb23 100644
--- a/embed/web-extension/resources/js/ephy.js
+++ b/embed/web-extension/resources/js/ephy.js
@@ -248,33 +248,34 @@ Ephy.PreFillUserMenu = class PreFillUserMenu
     }
 }
 
-Ephy.formControlsAssociated = function(pageID, forms, serializer)
+Ephy.formControlsAssociated = function(pageID, frameID, forms, serializer)
 {
     Ephy.formManagers = [];
 
     for (let i = 0; i < forms.length; i++) {
         if (!(forms[i] instanceof HTMLFormElement))
             continue;
-        let formManager = new Ephy.FormManager(pageID, forms[i]);
+        let formManager = new Ephy.FormManager(pageID, frameID, forms[i]);
         formManager.handleSensitiveElement(serializer);
         formManager.preFillForms();
         Ephy.formManagers.push(formManager);
     }
 }
 
-Ephy.handleFormSubmission = function(pageID, form)
+Ephy.handleFormSubmission = function(pageID, frameID, form)
 {
+    // FIXME: Find out: is it really possible to have multiple frames with same window object???
     let formManager = null;
     for (let i = 0; i < Ephy.formManagers.length; i++) {
         let manager = Ephy.formManagers[i];
-        if (manager.pageID() == pageID && manager.form() == form) {
+        if (manager.frameID() == frameID && manager.form() == form) {
             formManager = manager;
             break;
         }
     }
 
     if (!formManager) {
-        formManager = new Ephy.FormManager(pageID, form);
+        formManager = new Ephy.FormManager(pageID, frameID, form);
         Ephy.formManagers.push(formManager);
     }
 
@@ -311,9 +312,10 @@ Ephy.hasModifiedForms = function()
 
 Ephy.PasswordManager = class PasswordManager
 {
-    constructor(pageID)
+    constructor(pageID, frameID)
     {
         this._pageID = pageID;
+        this._frameID = frameID;
         this._pendingPromises = [];
         this._promiseCounter = 0;
     }
@@ -343,7 +345,7 @@ Ephy.PasswordManager = class PasswordManager
             let promiseID = this._promiseCounter++;
             window.webkit.messageHandlers.passwordManagerQuery.postMessage({
                 origin, targetOrigin, username, usernameField, passwordField, promiseID,
-                pageID: this._pageID,
+                pageID: this._pageID, frameID: this._frameID
             });
             this._pendingPromises.push({promiseID, resolver});
         });
@@ -353,15 +355,16 @@ Ephy.PasswordManager = class PasswordManager
     {
         window.webkit.messageHandlers.passwordManagerSave.postMessage({
             origin, targetOrigin, username, password, usernameField, passwordField, isNew,
-            pageID: this._pageID,
+            pageID: this._pageID
         });
     }
 
+    // FIXME: Why is pageID a parameter here?
     requestSave(origin, targetOrigin, username, password, usernameField, passwordField, isNew, pageID)
     {
         window.webkit.messageHandlers.passwordManagerRequestSave.postMessage({
             origin, targetOrigin, username, password, usernameField, passwordField, isNew,
-            pageID,
+            pageID
         });
     }
 
@@ -377,7 +380,7 @@ Ephy.PasswordManager = class PasswordManager
         return new Promise((resolver, reject) => {
             let promiseID = this._promiseCounter++;
             window.webkit.messageHandlers.passwordManagerQueryUsernames.postMessage({
-                origin, promiseID, pageID: this._pageID,
+                origin, promiseID, pageID: this._pageID, frameID: this._frameID
             });
             this._pendingPromises.push({promiseID, resolver});
         });
@@ -386,9 +389,10 @@ Ephy.PasswordManager = class PasswordManager
 
 Ephy.FormManager = class FormManager
 {
-    constructor(pageID, form)
+    constructor(pageID, frameID, form)
     {
         this._pageID = pageID;
+        this._frameID = frameID;
         this._form = form;
         this._sensitiveElementMessageSerializer = null;
         this._preFillUserMenu = null;
@@ -397,9 +401,9 @@ Ephy.FormManager = class FormManager
 
     // Public
 
-    pageID()
+    frameID()
     {
-        return this._pageID;
+        return this._frameID;
     }
 
     form()
@@ -559,7 +563,7 @@ Ephy.FormManager = class FormManager
         let url = new URL(this._form.action);
         // Warning: we do not whitelist localhost because it could be redirected by DNS.
         let isInsecureAction = url.protocol == 'http:' && url.hostname != "127.0.0.1" && url.hostname != 
"::1";
-        
window.webkit.messageHandlers.sensitiveFormFocused.postMessage(this._sensitiveElementMessageSerializer(this._pageID,
 isInsecureAction));
+        
window.webkit.messageHandlers.sensitiveFormFocused.postMessage(this._sensitiveElementMessageSerializer(this._frameID,
 isInsecureAction));
     }
 
     _findPasswordFields()
diff --git a/meson.build b/meson.build
index 3ca2fd1a5..680d314da 100644
--- a/meson.build
+++ b/meson.build
@@ -78,7 +78,7 @@ config_h = declare_dependency(
 glib_requirement = '>= 2.56.0'
 gtk_requirement = '>= 3.24.0'
 nettle_requirement = '>= 3.4'
-webkitgtk_requirement = '>= 2.24.1'
+webkitgtk_requirement = '>= 2.25.1'
 
 cairo_dep = dependency('cairo', version: '>= 1.2')
 gcr_dep = dependency('gcr-3', version: '>= 3.5.5')
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]