After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 786818 - EphyPasswordManager has hostname variables that contain security origins
EphyPasswordManager has hostname variables that contain security origins
Status: RESOLVED FIXED
Product: epiphany
Classification: Core
Component: Passwords, Cookies, & Certificates
3.25.x
Other Linux
: Normal normal
: ---
Assigned To: Epiphany Maintainers
Epiphany Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-08-25 21:10 UTC by Michael Catanzaro
Modified: 2017-08-27 17:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
password-manager: Handle security origins only (no more URIs) (44.78 KB, patch)
2017-08-26 21:17 UTC, Gabriel Ivașcu
committed Details | Review

Description Michael Catanzaro 2017-08-25 21:10:03 UTC
It's very important to use accurate terminology in our code, to avoid future programming mistakes. Previously we had a bunch of problems with "uri" variables in the form auth code that actually wanted to receive hostnames. This caused bugs when I modified the code and passed actual URIs instead of hostnames to parameters named uri, because what else was I going to do? :) So it's bad that EphyPasswordManager stores security origins in hostname variables. We should consistently rename these variables to origin.
Comment 1 Gabriel Ivașcu 2017-08-26 21:17:18 UTC
Created attachment 358493 [details] [review]
password-manager: Handle security origins only (no more URIs)

Change the API of EphyPasswordManager to handle security origins only,
thus clearing possible confusions about whether to call functions with
URIs or origins. Also fix bad variable naming: "hostname" -> "origin".
Comment 2 Michael Catanzaro 2017-08-26 22:15:41 UTC
Review of attachment 358493 [details] [review]:

Well that was more work than I expected. Thanks.

::: lib/sync/ephy-password-record.c
@@ +196,1 @@
     g_param_spec_string ("hostname",

We should change the property name too, right?

@@ +205,2 @@
   obj_properties[PROP_TARGET_ORIGIN] =
     g_param_spec_string ("formSubmitURL",

And here too.

@@ +274,1 @@
                                              "formSubmitURL", target_origin,

Ditto.
Comment 3 Alexander Mikhaylenko 2017-08-27 05:49:49 UTC
Michael Catanzaro: We can't change the property names, they have to same as in Firefox for sync to work.
Comment 4 Gabriel Ivașcu 2017-08-27 11:13:10 UTC
(In reply to Alexander Mikhaylenko from comment #3)
> Michael Catanzaro: We can't change the property names, they have to same as
> in Firefox for sync to work.

Exactly. We need to match the format exposed here: https://mozilla-services.readthedocs.io/en/latest/sync/objectformats.html#passwords

GObject serialization works with properties and their values, so changing the property names will break the format expected on the sync storage server, thus making Firefox most probably not recognize the objects and ignore them when syncing.
Comment 5 Michael Catanzaro 2017-08-27 17:20:44 UTC
Ah that's pretty cool. I forgot about that. OK then.
Comment 6 Gabriel Ivașcu 2017-08-27 17:41:30 UTC
Attachment 358493 [details] pushed as 3960972 - password-manager: Handle security origins only (no more URIs)