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

[Michael Catanzaro <mcatanzaro src gnome org>]

commit 9b238587c6287a36310008370a4bcdcc9c863a21
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.
    
    Note also I found a minor bug here in our will-submit-form callback: the
    target_frame parameter was named source_frame. That was not good, but
    fortunately it wasn't being used for anything, so this didn't matter.
    
    Probably fixes #25 and probably also fixes #26

 embed/ephy-embed-shell.c                 | 18 +++++--
 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, 105 insertions(+), 47 deletions(-)
---
diff --git a/embed/ephy-embed-shell.c b/embed/ephy-embed-shell.c
index 4fc745453..5ecf14373 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,6 +420,7 @@ 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");
 
   if (!origin || !target_origin || !password_field)
     return;
@@ -427,7 +429,12 @@ web_extension_password_manager_query_received_cb (WebKitUserContentManager *mana
   data->shell = g_object_ref (shell);
   data->promise_id = promise_id;
   data->page_id = page_id;
+<<<<<<< HEAD
   data->origin = g_strdup (origin);
+=======
+  data->frame_id = frame_id;
+  data->origin = g_steal_pointer (&origin);
+>>>>>>> Password manager should work in iframes
 
   ephy_password_manager_query (priv->password_manager,
                                NULL,
@@ -517,7 +524,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)
@@ -573,6 +583,8 @@ 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;
 
   if (!origin)
@@ -582,7 +594,7 @@ web_extension_password_manager_query_usernames_received_cb (WebKitUserContentMan
 
   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 54c91a7e9..5d1aa5864 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 *
-password_form_message_serializer (guint64  page_id,
+password_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 55556abeb..440cdf3ac 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.handlePasswordForms(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;
     }
@@ -345,7 +347,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});
         });
@@ -355,15 +357,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
         });
     }
 
@@ -379,7 +382,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});
         });
@@ -388,9 +391,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._passwordFormMessageSerializer = null;
         this._preFillUserMenu = null;
@@ -399,9 +403,9 @@ Ephy.FormManager = class FormManager
 
     // Public
 
-    pageID()
+    frameID()
     {
-        return this._pageID;
+        return this._frameID;
     }
 
     form()
@@ -561,7 +565,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.passwordFormFocused.postMessage(this._passwordFormMessageSerializer(this._pageID,
 isInsecureAction));
+        
window.webkit.messageHandlers.passwordFormFocused.postMessage(this._passwordFormMessageSerializer(this._frameID,
 isInsecureAction));
     }
 
     _findPasswordFields()
diff --git a/meson.build b/meson.build
index 9fba53214..27ec3f7ec 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')