GNOME Bugzilla – Bug 608687
Migration of stored passwords doesn't work
Last modified: 2013-08-20 07:16:32 UTC
Supposedly, Epiphany (2.29.6) was supposed to migrate my old saved passwords to the keyring. I find myself having to supply passwords for sites which I'm sure I had one saved; so this doesn't appear to have worked for me.
OK, let's start with this: - Can you tell me what number do you have in the file ~/.gnome2/epiphany/.migrated ? - If you have a '4' all your all passwords should have been migrated. To check this, install 'seahorse' and look for URLs like: www.urlofthesite.com/blah?form%5username=something&form%5password=somethingelse If you click those your login and password should be stored there.
Indeed, it does look like they have been migrated. Nonetheless, autofill doesn't appear to be working for sites that I see listed in seahorse; and I even continue to get the "save password?" bar for these sites.
(In reply to comment #2) > Indeed, it does look like they have been migrated. > > Nonetheless, autofill doesn't appear to be working for sites that I see listed > in seahorse; and I even continue to get the "save password?" bar for these > sites. Can you tell me some of the sites that do not work for you so that I can test it myself? If you delete some data from keyring (write the password somewhere else! ;)) and save it again, does it work from that point on?
AFAICT, autofill from a migrated password hasn't worked for me on *any* site. For example, ohloh.net. I clearly had a migrated password stored for this one; but it wasn't autofilled. Epiphany *did* prompt to save the password when I entered it; and on subsequent visits the password *was* autofilled. The key that Epiphany added appears to store the full URL in the "Server" field for the key; whereas the migrated one only stores the server name. This difference is mirrored in the "Description" field.
That is to say, while I have encountered sites where the "save password?" bar doesn't appear upon login, I have not encountered an instance where the presence of a migrated password appears to be suppressing activation of the bar. Rather: * autofill from migrated passwords never happens; and * the activation of the "save password?" bar happens regardless of the presence of a migrated password.
(In reply to comment #4) > AFAICT, autofill from a migrated password hasn't worked for me on *any* site. > > For example, ohloh.net. I clearly had a migrated password stored for this one; > but it wasn't autofilled. Epiphany *did* prompt to save the password when I > entered it; and on subsequent visits the password *was* autofilled. > > The key that Epiphany added appears to store the full URL in the "Server" field > for the key; whereas the migrated one only stores the server name. This > difference is mirrored in the "Description" field. So you are telling that for 'foo.com' one will have simply foo.com as the Server, but the other will have foo.com/?form_name...etc? This seems very strange, since we use the same function to store the auth data in migration and normal use, and both create a URL like the latter, not the former. Or am I misunderstanding?
(In reply to comment #6) > So you are telling that for 'foo.com' one will have simply foo.com as the > Server, but the other will have foo.com/?form_name...etc? Almost. In the latter case, the URI scheme is included; i.e.: http://foo.com/?form_name New keys look like that. Migrated ones just look like "foo.com".
At what point was the migration code added? I was running 2.28.x for some time before upgrading to 2.29.6; possibly some older version of the code is responsible for my password migration?
I can confirm this behavior too.
I'm seeing this too. It also means that for some sites (such as amazon.co.uk) where I can login on different parts of the site, I now have two passwords saved (three if you count the imported, unused password).
Now that I think of it, I like the new behavior better. It allows having several passwords for different parts of the same website. However it also means the migration code is completely useless.
Long long explanation that serve me as notes: Passwords are stored like this over normal usage: === ephy-web-view.c @store_password(): uri = soup_uri_new (webkit_web_view_get_uri (web_view)); _ephy_profile_store_form_auth_data (uri, name_field_name, password_field_name, name_field_value, password_field_value); === Which actually does this: === ephy-profile-migration.c @_ephy_profile_store_form_auth_data(): fake_uri = soup_uri_new (uri); fake_uri_str = soup_uri_to_string (fake_uri, FALSE); gnome_keyring_set_network_password (NULL, username, NULL, // This is the domain (host) fake_uri_str, // This is the server ^^^^^^^^^^^^ NULL, // This is the object This means that we STORE as the URL of the page, like http://site.com/login. === === And we query like this: ephy-web-view.c @pre_fill_form(): uri = soup_uri_new (webkit_web_view_get_uri (WEBKIT_WEB_VIEW (view))); char *uri_str = soup_uri_to_string (uri, FALSE); _ephy_profile_query_form_auth_data (uri_str, ^^^^^^^ ephy-profile-migration.c @_ephy_profile_query_form_auth_data(): key = soup_uri_new (uri_str); key_str = soup_uri_to_string (key, FALSE); gnome_keyring_find_network_password (NULL, NULL, // This is the domain (host) key_str, // This is the server ^^^^^^^ NULL, // This is the object This means that we QUERY for the URL of the page, like http://site.com/login, we don't use any other criteria when looking, only the server parameter. === === On migration we do this: gnome_keyring_set_network_password_sync (NULL, username, // Doesn't matter realm, // This is the domain (host or realm) uri->host, // This is the server NULL, // This is the object As you can see, here we store the host, so we store "site.com". The other option is that we do this, where u is the SoupURI for the full URL of the stored password, eg http://site.com/login: _ephy_profile_store_form_auth_data (u, form_username, form_password, username, password); This means that after a migration we have this as an example set: http://site.com/login, user, pass (from normal use) site.com, user, pass (from migration) The possible/proposed solution would be to: 1. Check for url, if none found, check for host (this on ephy-web-view or ephy-profile-migration, the first one makes more sense to me) 2. Fix the migration code to /always/ use the full url, not the host Agree? Comments?
(In reply to comment #12) > The possible/proposed solution would be to: > 1. Check for url, if none found, check for host (this on ephy-web-view or > ephy-profile-migration, the first one makes more sense to me) > 2. Fix the migration code to /always/ use the full url, not the host > > Agree? Comments? I’m not entirely sure, but AFAICT Gecko doesn’t store the full URL of the site. In your example, if you go to http://site.com/login and enter a login/password, it would be registered in signons.sqlite as the login/password pair for http://site.com/, and it will be used for whatever URL that starts with http://site.com/. Therefore, if you extract the full URL in the migration code, the said login will not be used with webkit for http://site.com/login. Unless I’m completely wrong about this, the only solution I can think of is to keep the migrated passwords in a specific database that goes per site instead of per URL, and to look in both databases for a password to propose.
I just tested with epiphany 2.22.3 + xulrunner 1.9.0.16 from Debian lenny. I went to somesite.com/login, and entered a login and a password. Here are the contents of the created signons3.txt file: #2e . http://somesite.com login MDIEEPblahblahblahblahblaaaaAAAAAhhhAhahaaaahahahahaaaaaablah== *pass MDIEEPblehblehblehblehbleeeeEEEEEhhhEheheeeeheheheheeeeeebleh== http://somesite.com --- . With the latest version of iceweasel the same things are stored in an sqlite database. Again, without the full URL. So I think it is correct for the migration code to only store the host. As suggested by Diego on IRC, the correct fix is to do a fallback query on the host when the URL is not found.
(In reply to comment #12) > On migration we do this: > > gnome_keyring_set_network_password_sync (NULL, > username, // Doesn't matter > realm, // This is the domain (host > or realm) > uri->host, // This is the server > NULL, // This is the object This is the path for the old migration, the new one uses _ephy_profile_store_form_auth_data to store the data, which should do the right thing. Previously the form passwords were ignored, so there shouldn't even be any problem of having duplicated entries for them. Are you ever going through the new path in the migration process?
Created attachment 155454 [details] [review] ephy-web-view: rework pre_fill_form() pre_fill_form() doesn't really require or takes advantage of calling ephy_embed_single_get_form_auth(). Nothing will really happen unless _ephy_profile_query_form_auth_data() returns a result, and the form username/password info is available from the JSObjectRef elements that are already passed. Bug #608687
Created attachment 155455 [details] [review] Add _ephy_profile_query_host_auth_data This function does the same as _ephy_profile_query_form_auth_data() but instead of querying with full details for the username/password it only uses the host, this is part of a workaround for a bug in gecko passwords migration but can serve the purpose of domain-wide passwords. Bug #608687
Created attachment 155456 [details] [review] Query for hostname if full url returns no passwords Currently we check for passwords matching "http://site.com/login", but due to the way gecko stored passwords we have a lot of migrated passwords in the form of "site.com" or "http://site.com", so we need to check for that. This change should also gives us the option to store fallback site-wide passwords, if we ever care for something like that. Summing up, we currently ask first for "http://site.com/login" and if no results are found we ask for "site.com". Bug #608687
Comment on attachment 155455 [details] [review] Add _ephy_profile_query_host_auth_data Ideally I think the workaround should happen transparently instead of adding this function, but since the query is async I see no good way of doing that so... Btw, either I'm still high because of my flu or we left a return value in the query functions from the time they were sync in our testing; now it's obviously useless (much more obviously in your function!). Can you remove that?
Comment on attachment 155456 [details] [review] Query for hostname if full url returns no passwords Duplicating host when you have the full URI is not ideal, you should probably use a SoupURI here or something (maybe the query functions should get one too, dunno). But we can improve this later, not too much time for iterations between hard code freeze.
Comment on attachment 155454 [details] [review] ephy-web-view: rework pre_fill_form() It does take advantage of it. You don't want to query the daemon all the time that you have a candidate form, so this just queries the local hash table which should be really fast. If there's a hit, then you query the daemon to get the actual auth values.
Created attachment 155516 [details] [review] Add _ephy_profile_query_host_auth_data This function does the same as _ephy_profile_query_form_auth_data() but instead of querying with full details for the username/password it only uses the host, this is part of a workaround for a bug in gecko passwords migration but can serve the purpose of domain-wide passwords. Bug #608687
Created attachment 155517 [details] [review] ephy-profile-migration: queries doesn't return anything Bug #608687
Created attachment 155518 [details] [review] Query for hostname if full url returns no passwords Currently we check for passwords matching "http://site.com/login", but due to the way gecko stored passwords we have a lot of migrated passwords in the form of "site.com" or "http://site.com", so we need to check for that. This change should also gives us the option to store fallback site-wide passwords, if we ever care for something like that. Summing up, we currently ask first for "http://site.com/login" and if no results are found we ask for "site.com". Bug #608687
Created attachment 155519 [details] [review] Handle caching of gecko-migrated passwords Gecko passwords don't have the form fields encoded in the url, nor are complete urls, they are only hostnames. Considering that we allow our caching function to cache them. Bug #608687
Created attachment 155520 [details] [review] Don't rely on cached form field names We query for the host of the uri where we found the form we are going to fill and this can give us more than 1 match, to be completely sure that we are filling the correct form but also that we permit gecko-migrated passwords we check if the cached data has form fields names (gecko passwords don't). Bug #608687
I had to do some changes to ephy-profile-migration to get it to cache gecko passwords, changes should be well isolated but useless (but not broken) if not together. I think. What do you think?
So seems I thought hard code freeze was tomorrow, so there's less rush to get this done. Feel free to nitpick in my rushed up (just a tiny bit) new patchset.
Comment on attachment 155516 [details] [review] Add _ephy_profile_query_host_auth_data Don't see the point in committing the function with bogus return value only to fix it in a follow-up. OK other than that, but change it before committing.
Comment on attachment 155517 [details] [review] ephy-profile-migration: queries doesn't return anything OK taking into account comment for previous patch.
Comment on attachment 155518 [details] [review] Query for hostname if full url returns no passwords Looks exactly the same than before to me, no need to put it up for review twice. Or if there's some really subtle change, better point it out to me ;)
OK, wait a minute. Just doing a query by host is dangerous because there could be confusions with HTTP auth stuff, which is also stored in keyring. If I'm not wrong the gecko migrated logins/passwords *do* have login/password form info, it's just that they were stored with the host instead of the full URI. Can't you simply make sure they are cached correctly (they should already?), and then when checking the hash table, if we fail with full URI, check only with host, and query the daemon with that if it we get a hit. What fails with that plan?
I found the real problem with our migration code: /* The password */ /* The element name has a leading '*' */ if (lines[begin][0] == '*') { if (handle_forms) { form_password = g_strdup (lines[begin++]); } ..... if (handle_forms && username && password && !g_str_equal (form_username, "") && !g_str_equal (form_password, "*")) { // We do the right thing, form fields, full uri. g_free (u); } else if (!handle_forms && username && password) { // We store as HTTP auth, hence only using the server/host. Notice how we make "form_password" the value of lines[begin], which happens to start with a '*'. Now notice what we are checking for in the later if (): !g_str_equal (form_password, "*"). This always fails, that's why all our migrated passwords are host only, because they were migrated as HTTP auth. Sadly we can't do much with the already migrated passwords, although I'm investigating if we can do some magic and delete the old cruft we would be leaving behind. It's wrong™ to leave 100 duplicate logins in everyone's keyring. I think I can delete the old ones safely.
(In reply to comment #33) > I found the real problem with our migration code: > > /* The password */ > /* The element name has a leading '*' */ > if (lines[begin][0] == '*') { > if (handle_forms) { > form_password = g_strdup (lines[begin++]); > } > ..... > if (handle_forms && username && password && > !g_str_equal (form_username, "") && > !g_str_equal (form_password, "*")) { > // We do the right thing, form fields, full uri. > g_free (u); > } else if (!handle_forms && username && password) { > // We store as HTTP auth, hence only using the server/host. > > Notice how we make "form_password" the value of lines[begin], which happens to > start with a '*'. > Now notice what we are checking for in the later if (): !g_str_equal > (form_password, "*"). > This always fails, that's why all our migrated passwords are host only, because > they were migrated as HTTP auth. > It will always fail if form_password is always "*", which only happens for HTTP passwords... that's basically how you tell them apart from the form-based ones.
Epic patches coming. Just a second.
Obsoleting all this stuff.
Created attachment 155641 [details] [review] Add with_path option to _ephy_profile_query_form_auth_data Use this option to optionally remove the /path/ after the hostname. So for example if with_path is FALSE, then the query is: - "http://host.com/?form_elements" instead of: - "http://host.com/login/?form_elements" Also remove the useless GList return type. Bug #608687
Created attachment 155642 [details] [review] Query without path as a second option for passwords When filling or saving a form we are storing/asking using the full url, gecko passwords where migrated with only hostname because of how gecko stores its passwords. To handle that case we query for passwords using only the host and no path in the URL. This means we first check for http://g.com/login (if http://g.com/login was the current url) and then for http://g.com/. Bug #608687
This first two patches implement the idea of doing a second more general/broad query when we don't find specific results for an URL. Like commit log says, this enables us to use "http://host.com/" when "http://host.com/login" is not available *AND* it also respects form fields names.
Created attachment 155643 [details] [review] migration: fix leading * in password field name Password form fields are marked with a *, but this * was not removed when storing the field name so this led to all the passwords field names to be migrated with the leading *. Bug #608687
Created attachment 155644 [details] [review] migration: don't store a trailing ) in realm Realms where exported as "realm)" because of a missing -1. Bug #608687
Created attachment 155645 [details] [review] The epic fix for password migration process This is a considerably touchy patch, but it's stable so far. What this does: - Eliminates the old 'migrators' logic, uses only one big function and directly implements conditions like "if last migration is something before version 5". - Consolidate migrate_passwords and migrate_passwords2 into one single function, handle migration of forms and non forms in one go. - Add remove_badly_migrated_passwords(), this functions queries the keyring for *all* the stored items using the *same* logic of pdm-dialog (that means no non-ephy password is hurt), then it deletes the badly migrated passwords that match any of these: + Passwords without a realm, and without form information + Passwords with a leading '*' in password field name + Passwords with a realm with a trailing ')' This were all migrated like that because of bugs in previous migration code, luckily we can use those bugs to find this passwords. Obviously if there are no buggy passwords none are removed. This is compatible with our webkit-era stored passwords. - Bump migration version to 5 Hopefully this solves the problem forever. Bug #608687
So this 3 last patches are the ones solving the root issue. There were two ugly bugs in the migration code ("migration:" patches), those are fixed. However to fix the already migrated passwords we need to do a bit of work, that what the last patch is for. Xan please check these out (they are all different from the ones before, you'll loved them, totally) and let me know :)
Comment on attachment 155643 [details] [review] migration: fix leading * in password field name This is wrong, since we need to check further below for the '*' character. Just do a +1 on the string when we pass it to store_form_auth_data with a comment saying that we skip the '*' from now on.
Comment on attachment 155644 [details] [review] migration: don't store a trailing ) in realm Looks good to me.
Comment on attachment 155645 [details] [review] The epic fix for password migration process - I don't think you can rely on a leading '*' in the password value to delete stuff, since that's a valid initial character in a password as far as I know. So you could be deleting people's passwords. What you need to do to be totally sure is to check, for each item in the moz prefilo, if it's still stored (bugs and all from the old code) in the keyring; if it is, you delete it and store it again with the bugs fixed (or just fix it in place). This sucks but I don't think there's any other sane way of doing it. - I don't really agree with removing the iterative logic in the migration process. It's much easier to patch-up a previous bug with a new function than refactoring everything to be idempotent with the old process. Besides, the version makes much more sense that way, otherwise it's just a loosely defined integer that you would change... when exactly? IMHO fix the bugs in the code, add a step 5 that knows how to look for items with the buggy values, and fix them. If the user is running step 4 when 5 is defined, there's no need to run 5, since 4 won't have bugs anymore.
Comment on attachment 155641 [details] [review] Add with_path option to _ephy_profile_query_form_auth_data OK, but you could have done two patches ;)
(In reply to comment #47) > (From update of attachment 155641 [details] [review]) > OK, but you could have done two patches ;) But actually this should not be needed...
Comment on attachment 155642 [details] [review] Query without path as a second option for passwords Why can't you simply check the hash table a second time in pre_fill_form? Seems a lot simpler. If you get nothing, get rid of /path/ right there and check again. If that time you get something, query the keyring with that.
Comment on attachment 155644 [details] [review] migration: don't store a trailing ) in realm Attachment 155644 [details] pushed as 2a1a987 - migration: don't store a trailing ) in realm
Comment on attachment 155643 [details] [review] migration: fix leading * in password field name Committed '*' fix with Xan's comments.
Comment on attachment 155642 [details] [review] Query without path as a second option for passwords Second thought: we shouldn't have to do this if we fix migration for good. Marking rejected.
Comment on attachment 155641 [details] [review] Add with_path option to _ephy_profile_query_form_auth_data Second thought: we shouldn't have to do this if we fix migration for good. Marking rejected. (the return value part was committed separately)
Created attachment 155935 [details] [review] Remove path when normalizing urls for keyring query This means that we use the url path when saving or querying for passwords, so providing "http://host.com/login" will store/retrieve "http://host.com/?form...". Bug #608687
Created attachment 155936 [details] [review] embed-single: don't use paths in the form auth cache Modify ephy_embed_single_get_form_auth and ephy_embed_single_add_form_auth to discard the path parts of their uri argument so that queries and saves to the form auth cache are congruent with the actual keyring representation of the stored info. This means that the internal auth cache is no longer using "host" as the key of the hash table, it now uses the fully encoded string (http://host/?form_...). This should round up compatibility with the normalization we are doing in normalize_and_prepare_uri in ephy-profile-migration.c, which is doing the same, removing the path of uris before encoding and storing. Bug #608687
Created attachment 155937 [details] [review] ephy-web-view: adapt to ephy-embed-single API change Don't use uneeded SoupURIs to get the host of uris and use the new ephy-embed-single API correctly. Bug #608687
Created attachment 155938 [details] [review] tests: add test for EphyEmbedSingle Currently it only covers form auth. Bug #608687
Created attachment 155939 [details] [review] fixup for use form auth api correctly Bug #608687
Created attachment 155940 [details] [review] migration: move around normalize_and_prepare_uri To enable it to be called by other functions earlier. A small change to accomodate later ones. Bug #608687
Created attachment 155941 [details] [review] migration: add handle_fix arg to parse_and_decrypt This is to handle find_and_delete in a future commit. Bug #608687
Created attachment 155942 [details] [review] migration: add migrate_passwords_while_fixing migrator Not yet in use, it has to be after api change to parse_and_decrypt Bug #608687
Created attachment 155943 [details] [review] migration: cache uri string value for future use This is used in a later patch. Bug #608687
Created attachment 155944 [details] [review] migration: add find_and_delete function This function is the magic doer of the whole patchset, it looks for all the possible errors in previous passwords and nukes them out of sight. Bug #608687
Created attachment 155945 [details] [review] migration: correctly use handle_forms in parse_and_decrypt The function was excluding either forms or HTTP auth, when it doesn't really needs to do that. This change to the conditional fixes it. Bug #608687
Created attachment 155946 [details] [review] migration: enable version 5, fixing all previous bugs This is the one true fix. Bug #608687
Oh, hai. I just attached 12 patches. Sorry about that... the fix is that long and granular. Well, currently I'm not yet finished, but this is 90%. I think I broke querying of form passwords somehow (in embed-single, meaning it's just a bad host/full url confusion somewhere -not related to migration). Anyway, the migrator function works *well*. I tried it with a clean (meaning unmigrated) profile and it worked fine. I want to test it a bit more with already migrated profiles in this final form but it was working perfectly before the last commit (enabling the migrator, so not really touching the function). So, yeah, enjoy. Let me know what do you think about this so far. I'll try to be around early tomorrow but I'm sick so perhaps I'll be a bit late.
(In reply to comment #66) > > So, yeah, enjoy. Let me know what do you think about this so far. I'll try to > be around early tomorrow but I'm sick so perhaps I'll be a bit late. Given that it's fscking 2:40 am.
Created attachment 156032 [details] [review] embed-single: don't use paths in the form auth cache -- embed-single: don't use paths in the form auth cache Modify ephy_embed_single_get_form_auth and ephy_embed_single_add_form_auth to discard the path parts of their uri argument so that queries and saves to the form auth cache are congruent with the actual keyring representation of the stored info. This means that the internal auth cache is no longer using "host" as the key of the hash table, it now uses the fully encoded string (http://host/?form_...). This should round up compatibility with the normalization we are doing in normalize_and_prepare_uri in ephy-profile-migration.c, which is doing the same, removing the path of uris before encoding and storing. Bug #608687
Created attachment 156033 [details] [review] migration: correctly use handle_forms in parse_and_decrypt saved the previous patch attempt :) -- migration: correctly use handle_forms in parse_and_decrypt The function was excluding either forms or HTTP auth, when it doesn't really needs to do that. This change to the conditional fixes it. Bug #608687
Ok so the full patchset currently works fine when fixing a previous migration, or migrating for the first time itself. Works: - migrate passwords cleanly from stage 0 (never migrated) or 1 (only cookies) - fix previous migrations (from stage 2 and up) - save/retrieve form info from the migrated data (you had x.com in gecko, you now go to x.com and you find it working) Doesn't work: - Auto fill of http auth passwords (working on this, should be trivial) - Moving the old full-path passwords to the new scheme of host only (needs a migrator, not priority since people with this should be a small minority) I'd reaaaaally appreciate a review so we can merge this calmly before monday.
(In reply to comment #70) > > Doesn't work: > - Auto fill of http auth passwords (working on this, should be trivial) Found out why... it's my webkit build. So nothing really left to do.
Created attachment 156513 [details] [review] migration: move around normalize_and_prepare_uri To enable it to be called by other functions earlier. A small change to accomodate later ones. Bug #608687 -- Also obsoletting everything. We splitted the patches and simplified. Now there are only two here.
Created attachment 156514 [details] [review] migration: enable version 5, fixing all previous bugs Add find_and_delete() and migrate_passwords_while_fixing() functions to fix past migrations (2.29.x era) and enable the version 5 of the migrator. This should fix old passwords badly migrated during the 2.29 devel cycle. Bug #608687
Created attachment 156530 [details] [review] migration: enable version 5, fixing all previous bugs Enables the version 5 of the migrator that fixes any previous badly migrated passwords. Also refactors a lot of code regarding the migrators. Bug #608687 -- Got it reduced to this! It reads much better and cleaner now. I also fixed the issues about the migrate_*() functions we talked about earlier, it was easier and simpler than I thought. I think we are much much closer to a final patch now, hopefully :).
I tried out the patch, and now, any new saved passwords no longer show up in the Personal Data tab. They are still saved in the keyring though. Not sure if this is a known problem with the patch or not, as I haven't followed the discussion closely. Anyway, it would be great to have this fixed.
Comment on attachment 156530 [details] [review] migration: enable version 5, fixing all previous bugs I don't think this longer applies, obsoleting the patch. For discussion if you want to keep the bug open, but I think it's already irrelevant.
We have migrated again, now to use libsecret, so this is obsolete.