925101 - Remove legacy signons.txt files

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Justin Dolske [:Dolske]]

Up until Firefox 3.5, password manager used a signons.txt (*) text file for storing passwords. Bug 288040 switched to sqlite storage, and added code to import data from users' .txt files to the new DB. We didn't delete the .txt files to allow users the possibility of downgrading from Firefox 3.5 to Firefox 3.0 without dataloss.

Nothing else uses those files, and bug 717490 removes a bunch of related legacy code. We should delete the old files, since it's well past time to support downgrading to Firefox 3.0!

* - note that exact file name has changed over time, and is pref-controlled. By default we'd expect a signons.txt, signons2.txt, or signons3.txt in the user's profile. But the pref could make use of an alternate name (or location?). See code in the now-removed storage-Legacy.js module (removed by bug 717490, so it's in the diffs there).

Comment 1

[AKSHAY NAUTIYAL]

want to work on this how do i go about it?

Comment 2

[Matthew N. [:MattN]]

Hello and thanks for helping out.

1) We'll want to initiate the deletion code from nsBrowserGlue's _migrateUI function[1]. Note that this code should only run once per user so in order to repeatedly test your changes you will have to keep decrementing the pref browser.migration.version in about:config to its value prior to your patch.
Put the code directly in that function for now and if it turns out to be too much for there it shouldn't be hard to move later.
2) For the most part you can port the logic from the former _removeOldSignonsFiles[2] but you should probably switch to using OS.File[3] for off-main-thread I/O once you know it's working.

Let me know if you have any questions

[1] https://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js?rev=70e1e02b917d#1472
[2] https://hg.mozilla.org/mozilla-central/rev/00955d61cc94#l15.619
[3] https://developer.mozilla.org/en-US/docs/JavaScript_OS.File/OS.File_for_the_main_thread#OS.File.remove%28%29

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Hey AKSHAY, please let us know if you're still interested in contributing. In the mean time, I will release this back into the "unassigned" pool for someone else to pick up.

Comment 4

[Nathan Yee [:nyee]]

I would like to attempt to fix this bug. Matthew, I've tried following your instructions but I'm not sure exactly what you mean by deletion code in _migrateUI. Additionally, I tried searching for _removeOldSignonsFiles on MXR but got no results back. Perhaps it would be best to update our instructions regarding this bug?

Comment 5

[Matthew N. [:MattN]]

Hello Nathan,

(In reply to Nathan Yee [:nyee] from comment #4)
> I would like to attempt to fix this bug. Matthew, I've tried following your
> instructions but I'm not sure exactly what you mean by deletion code in
> _migrateUI. 

See [1] in comment 2. It links to the migrateUI function where you would call into the function you add.

> Additionally, I tried searching for _removeOldSignonsFiles on
> MXR but got no results back. Perhaps it would be best to update our
> instructions regarding this bug?

The instructions are still up to date. You should follow the links in the footnotes of comment 2 for the relevant code.

Comment 6

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Nathan, do you still plan to work on this bug? If you do, please coordinate with MattN.

Comment 7

[Ray Lin[:ralin]]

Hi, MattN. I'd like to work on this bug. 

I have few questions:
1. Instead of changing migration.version every time, is it make sense to temporarily disable these lines of code[1][2]?
2. What is the expected result or how to verify it?

Thanks :)

[1] https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/browser/components/nsBrowserGlue.js#2165
[2] https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/browser/components/nsBrowserGlue.js#1810-1811

Comment 8

[Matthew N. [:MattN]]

Hello ralin,

Glad to have you taking this. 

(In reply to Ray Lin[:ralin] from comment #7)
> 1. Instead of changing migration.version every time, is it make sense to
> temporarily disable these lines of code[1][2]?

Sure, that's fine for your own testing.

> 2. What is the expected result or how to verify it?

Old signon txt files in the profile directory should be deleted. See _removeOldSignonsFiles (linked in comment 5) which specified multiple pref names that can specify filenames. We should delete the file(s) specified by any of those prefs and call clearUserPref on all of the pref names. We should probably delete from the default filenames even if the prefs don't exist but we should ensure we don't follow symlinks (only delete the symlink itself) and make sure we don't delete these passwords files that may lie outside the profile directory (if the prefs even allow that?).

At a high level, all we're doing is deleting some old files containing passwords that have since been migrated to other storage mechanisms (sqlite or JSON).

Thanks

Comment 9

[Ray Lin[:ralin]]

Attachment: MozReview Request: Bug 925101 - Remove legacy signons.txt files. r?dolske

Review commit: https://reviewboard.mozilla.org/r/47437/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47437/

Comment 10

[Ray Lin[:ralin]]

Comment on attachment 8742722 [details]
MozReview Request: Bug 925101 - Remove legacy signons.txt files. r?dolske

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47437/diff/1-2/

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 8742722 [details]
MozReview Request: Bug 925101 - Remove legacy signons.txt files. r?dolske

https://reviewboard.mozilla.org/r/47437/#review44385

Hello Ray, thanks for starting this.

It would probably be good to move the main logic to a helper method in toolkit/components/passwordmgr/LoginHelper.jsm, lazily load that module at the top of nsBrowserGlue and then call it from a new migration `if`-block here. That will make it much easier to test. Can you write a simple xpcshell-test in /toolkit/components/passwordmgr/test/unit/ (see test_module_LoginImport.js for an example but you would want something much simpler).

::: browser/components/nsBrowserGlue.js:1929
(Diff revision 2)
>      if (currentUIVersion < 14) {
> +      const profileDir = OS.Constants.Path.profileDir;
> +      const sep = "winGetDrive" in OS.Path ? "\\" : "/";

You need to bump `UI_VERSION` and add a new `if (currentUIVersion < N) {…}` block otherwise the migration won't happen for users who already upgraded to UI version 14.

::: browser/components/nsBrowserGlue.js:1943
(Diff revision 2)
> +
>        // DOM Storage doesn't specially handle about: pages anymore.
> -      let path = OS.Path.join(OS.Constants.Path.profileDir,
> -                              "chromeappsstore.sqlite");
> -      OS.File.remove(path);
> +      toDeletes.add(OS.Path.join(profileDir, "chromeappsstore.sqlite"));
> +
> +      // Remove legacy signon files.
> +      defaultSignonFilePrefs.forEach((val, pref) => {

This comment seems out of place since the code immediately below it isn't deleting and the code above is just as relevant as the code immediately below.

::: browser/components/nsBrowserGlue.js:1947
(Diff revision 2)
> +          let signonFile = Services.prefs.getCharPref(pref);
> +
> +          toDeletes.add(OS.Path.normalize(OS.Path.join(profileDir, signonFile)));
> +          Services.prefs.clearUserPref(pref);

I think this is wrong as I believe the prefs had a "signon." prefix.

::: browser/components/nsBrowserGlue.js:1954
(Diff revision 2)
> +          toDeletes.add(OS.Path.normalize(OS.Path.join(profileDir, signonFile)));
> +          Services.prefs.clearUserPref(pref);
> +        } catch (e) {}
> +      });
> +
> +      toDeletes.forEach(file => {

Nit: for…of is generally simpler to read that .forEach so that should be preferred unless having a separate function scope is useful for some case. The same applies to the forEach above

::: browser/components/nsBrowserGlue.js:1955
(Diff revision 2)
> +          Services.prefs.clearUserPref(pref);
> +        } catch (e) {}
> +      });
> +
> +      toDeletes.forEach(file => {
> +        if (file.indexOf(`${profileDir}${sep}`) === 0) {

What is this line doing? It feels wrong to me but that may just be because I don't understand. Please add a comment if you're leaving it.

Generally you shouldn't need to manually handle separators and OS.Path.join should do it for you.

::: browser/components/nsBrowserGlue.js:1956
(Diff revision 2)
> +        } catch (e) {}
> +      });
> +
> +      toDeletes.forEach(file => {
> +        if (file.indexOf(`${profileDir}${sep}`) === 0) {
> +          OS.File.remove(file);

Did you double-check that this doesn't follow symlinks?

Could you please flag Justin Dolske for the next review as he knows more about the txt file behaviour.

Comment 12

[Ray Lin[:ralin]]

https://reviewboard.mozilla.org/r/47437/#review44385

> What is this line doing? It feels wrong to me but that may just be because I don't understand. Please add a comment if you're leaving it.
> 
> Generally you shouldn't need to manually handle separators and OS.Path.join should do it for you.

This line checks the file is located in profile directory or not, in order not to delete the file outside. Do we need this check?

MattN, thank you so much for all the helpful comments :) I have clearer direction to go now!

I'll fix these issues and ask Justin to review next time. Thanks.

Comment 13

[Ray Lin[:ralin]]

Hi MattN,

I got a problem on Mac:

For testing, I created a symbolic link with command: ln -s.
OS.File.stat().isSymLink always return false even the file is symbolic link, meanwhile, FileUtils.jsm works correctly.
Is it a bug or my misuse? otherwise, we need to import FileUtils additionally to deal with symbolic link.

Thanks!

Comment 14

[Ray Lin[:ralin]]

Comment on attachment 8742722 [details]
MozReview Request: Bug 925101 - Remove legacy signons.txt files. r?dolske

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47437/diff/2-3/

Comment 15

[Justin Dolske [:Dolske]]

Comment on attachment 8742722 [details]
MozReview Request: Bug 925101 - Remove legacy signons.txt files. r?dolske

https://reviewboard.mozilla.org/r/47437/#review47361

Sorry for the delay -- looks good to me, nice code! One trivial fix to xpcshell.ini needed, but I don't need to review it again.

::: toolkit/components/passwordmgr/LoginHelper.jsm:472
(Diff revision 3)
> +      toDeletes.add(Path.join(profileDir, val));
> +
> +      try {
> +        let signonFile = Services.prefs.getCharPref(pref);
> +
> +        toDeletes.add(Path.normalize(Path.join(profileDir, signonFile)));

Is there a particular reason to use normalize here? I don't think it hurts, but was just wondering why you're using it. FWIW the old signons-Legacy.js didn't do anything special (unless the old nsIFile code did this internally, but I don't think so).

::: toolkit/components/passwordmgr/test/unit/test_module_LoginHelper.js:55
(Diff revision 3)
> +    { file: "signons.txt" },
> +    { file: "signons2.txt" },
> +    { file: "singons.txt", pref: "signon.SignonFileName" },
> +    { file: "customized2.txt", pref: "signon.SignonFileName2" },
> +    { file: "customized3.txt", pref: "signon.SignonFileName3" }
> +  ]];

Oh, good, the last one tests the case of a file (signons3.txt) being deleted even when it doesn't exist. Maybe worth a comment noting that, since it's kind of a subtle point.

::: toolkit/components/passwordmgr/test/unit/xpcshell.ini:11
(Diff revision 3)
>  
>  # Test JSON file access and import from SQLite, not applicable to Android.
>  [test_module_LoginImport.js]
>  skip-if = os == "android"
>  [test_module_LoginStore.js]
> +[test_module_LoginHelper.js]

You've made the new test skipped on Android (which is fine, since I don't think we ever shipped an Android version using the old .txt storage, and in any case _migrateUI doesn't run there.)

But you've also implicitly removed the skip-if for test_module_LoginStore.js... The skip-if instructions apply to the last test in [brackets].

So please add the extra skip-if so that existing test doesn't start running on Android.

Comment 16

[Matthew N. [:MattN]]

(In reply to Ray Lin[:ralin] from comment #13)
> I got a problem on Mac:
> 
> For testing, I created a symbolic link with command: ln -s.
> OS.File.stat().isSymLink always return false even the file is symbolic link,
> meanwhile, FileUtils.jsm works correctly.
> Is it a bug or my misuse? otherwise, we need to import FileUtils
> additionally to deal with symbolic link.

Honestly, I don't know.

Also, can you please rename test_module_LoginHelper.js to test_removeLegacySignonFiles.js?

Comment 17

[Ray Lin[:ralin]]

Comment on attachment 8742722 [details]
MozReview Request: Bug 925101 - Remove legacy signons.txt files. r?dolske

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47437/diff/3-4/

Comment 18

[Justin Dolske [:Dolske]]

(In reply to Ray Lin[:ralin] from comment #13)

> For testing, I created a symbolic link with command: ln -s.
> OS.File.stat().isSymLink always return false even the file is symbolic link,
> meanwhile, FileUtils.jsm works correctly.

Hmm, this wouldn't perhaps be a result of that Path.normalize(), would it? Does that attempt to return the actual location of a symlink?

> Is it a bug or my misuse? otherwise, we need to import FileUtils
> additionally to deal with symbolic link.

If you're actually passing in a symlink as the leaf node, that sounds like a bug. But I'd assume you don't need to check, I'd expect removing it to just delete the symlink, and not the file it actually points to. (And even if it didn't, I don't think it greatly matters here.)

Comment 19

[Justin Dolske [:Dolske]]

https://reviewboard.mozilla.org/r/47437/#review47631

Comment 20

[Justin Dolske [:Dolske]]

Note that your test seems to be failing on Try -- https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0f5f6ae5442

Comment 21

[Ray Lin[:ralin]]

Comment on attachment 8742722 [details]
MozReview Request: Bug 925101 - Remove legacy signons.txt files. r?dolske

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47437/diff/4-5/

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ff2bd098e489

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ff2bd098e489