fix: Prevent --keep-profile-changes on default Firefox profiles by rpl · Pull Request #1007 · mozilla/web-ext

rpl · 2017-07-17T19:52:17Z

This PR extracts (and expands) the "checks for the Firefox default profile" pieces that we are working on in #968 to fix #1005

additional checks needed to prevent the usage on the --keep-profile-changes option on a default Firefox profile (when specified using the name, a full path or a relative path)
additional unit tests

coveralls · 2017-07-17T19:55:37Z

Coverage decreased (-1.2%) to 98.78% when pulling ea4c214 on rpl:fix/prevent-keep-profile-changes-on-default-profile into 7b390e4 on mozilla:master.

coveralls · 2017-07-18T10:58:57Z

Coverage decreased (-0.3%) to 99.668% when pulling 778757a on rpl:fix/prevent-keep-profile-changes-on-default-profile into 7b390e4 on mozilla:master.

coveralls · 2017-07-18T11:50:57Z

Coverage remained the same at 100.0% when pulling cc10673 on rpl:fix/prevent-keep-profile-changes-on-default-profile into 7b390e4 on mozilla:master.

rpl · 2017-07-18T11:54:18Z

@kumar303 This should be ready to be reviewed now. (I'm going to also test it manually on Windows as I did on Linux in the meantime).

@saintsebastian I've also added you as a reviewer, because I'm touching files that could probably impact some of your open pull requests, and a pair of additional eyes are always appreciated ;-)

saintsebastian

Since I wrote useProfile() this bug is kinda on me, so thanks for fixing it!

saintsebastian · 2017-07-18T15:23:08Z

src/firefox/index.js


+// isDefaultProfile types and implementation.
+
+const DEFAULT_PROFILES_NAMES = [


I'm no expert on firefox profiles, but could that be that those names are misleading? As in I could have created a profile which i called default but don't actually use if as default.

I would prefer to keep this as strict as "you can use a different profile name instead of these ones"

mostly because the incorrect usage can be implemented in a script (like the react-devtools automation script mentioned in #1005) and so the user could be not in charge of that "potentially harmful" decision.

saintsebastian · 2017-07-18T15:27:08Z

src/firefox/index.js

    customPrefs = {},
  }: UseProfileParams = {},
 ): Promise<FirefoxProfile> {
+  const isForbiddenProfile = await isFirefoxDefaultProfile(profilePath);


Kumar proposed option like --allow-web-ext-to-completely-break-my-profile, i think that might be a good idea if we show a warning first, since if users want to do something potentially dangerous that's on them.

@saintsebastian @kumar303 I'm open to implement such option if we decide to go for it, but I vote against it :-)

I thought about it more (especially when reviewing this patch) and I don't know that introducing the --allow-web-ext-to-completely-break-my-profile option is the right thing to do.

It's nice because it gives a clear indication of not only keeping profile changes but also letting web-ext save its own settings. However, it will just be annoying to those who know what they are doing. Plus, there is really no valid use case (other than laziness) that requires someone to use their default profile with web-ext.

saintsebastian · 2017-07-18T15:32:20Z

tests/unit/test-firefox/test.firefox.js

+  for (const [idx, profile] of profilesDefs.entries()) {
+    content += `[Profile${idx}]\n`;
+    for (const k of Object.keys(profile)) {
+      // $FLOW_FIXME: test123


I want to do that very often. Maybe a more descriptive reason for why fixme is needed would make it easier to fix it later.

kumar303

Looking good! Thanks for starting a patch so quickly.

kumar303 · 2017-07-18T15:20:04Z

src/firefox/index.js

+  } catch (error) {
+    if (isErrorWithCode('ENOENT', error)) {
+      // No profiles exist yet, default to false (the default profile name contains a
+      // random generated component).


I'd like to see some debug logging here that shows profilesIniPath and error because there could be a bug upstream in guessing the path to this file.

yeah I agree

kumar303 · 2017-07-18T15:50:01Z

src/firefox/index.js

+  for (const profile of finder.profiles) {
+    // Check if the profile dir path or name is one of the default profiles
+    // defined in the profiles.ini file.
+    if (DEFAULT_PROFILES_NAMES.indexOf(profile.Name) >= 0 ||


Would this work as two if statements?

if (DEFAULT_PROFILES_NAMES.indexOf(profile.Name) >= 0) { return true; } if (profile.Default === '1') { // ... }

It could be less noisy to use .includes(), then it seems simple enough to keep together if it makes sense.

@mstriemer 👍 yeah, I did it mostly for compatibility, but to be fair we are already transpiling this files with babel and it is completely possible that the polyfill is already there and supported across the nodejs versions, I'll change it to use .includes().

@kumar303 if I'm not wrong, we would miss the default profiles when specified by path and not marked as Default in the profiles.ini file

we would miss the default profiles when specified by path and not marked as Default in the profiles.ini file

Oh, right! In that case, should the early return statement be removed?

@kumar303 the early return should ensure that the usage error with the "Cannot use default profile with --keep-profile-changes..." message is logged also when the profiles.ini file doesn't exist yet and the user is using "default" or "dev-edition-default" as the firefox profile parameter.

kumar303 · 2017-07-18T16:00:55Z

src/firefox/index.js

+  const isForbiddenProfile = await isFirefoxDefaultProfile(profilePath);
+  if (isForbiddenProfile) {
+    throw new UsageError(
+      'Using --keep-profile-changes' +


How about this to explain the problem better?

`Cannot use --keep-profile-changes on a default profile ("${profilePath}")` + ' because web-ext will make it insecure and unsuitable for daily use.' + '\nSee https://github.com/mozilla/web-ext/issues/1005'

kumar303 · 2017-07-18T16:46:33Z

src/firefox/index.js

+    }
+
+    // Re-throw any unexpected exception.
+    throw error;


This doesn't appear to have test coverage. The tests still pass if I comment it out.

you are right... "never trust the coverage report" :-) it is not marked as missed (nor executed actually) in the html report generated locally.

kumar303 · 2017-07-18T16:54:46Z

tests/unit/test-firefox/test.firefox.js

+  return FakeProfileFinder;
+}
+
+async function createFakeProfilesIni(dirPath, profilesDefs) { // eslint-disable-line


I was curious about the lint error but when I removed this comment, yarn lint passed just fine. Maybe it's no longer needed?

definitely not needed, it is a leftover from when I was rewriting some of the new tests.

kumar303 · 2017-07-18T17:02:13Z

tests/unit/test-firefox/test.firefox.js

+      async () => {
+        return withTempDir(async (tmpDir) => {
+          const profilesDirPath = tmpDir.path();
+          const FakeProfileFinder = createFakeProfileFinder(profilesDirPath);


Instead of using a fake ProfileFinder, could you just pass in an explicit baseProfileDir value to firefox.isDefaultProfile()? That would bypass ProfileFinder.locateUserDirectory() but continue to use a real ProfileFinder which would provide better test coverage.

we are actually overriding only locateUserDirectory static method (which is only used directly in our helper, because we manually pass it to the created ProfileFinder and so it will never be discovered again), all the other methods are the real ones.

I also remembered right now that I added the sinon spies to add some additional assertion... that I should add :-)

kumar303 · 2017-07-18T17:03:19Z

tests/unit/test-firefox/test.firefox.js

+          const isDefault = await firefox.isDefaultProfile(
+            'manually-set-default', FakeProfileFinder
+          );
+          const isNotDefault = await firefox.isDefaultProfile(


This seems like it should be in a separate unit test (unless it's sharing the same setup?) to make test failures easier to deal with

@kumar303 How about adding some assertion messages where useful instead? (added in 3c1c0a3)

kumar303 · 2017-07-18T17:03:58Z

tests/unit/test-firefox/test.firefox.js

+        });
+      });
+
+    it('allows profile path if is not listed as default in profiles.ini',


nit: allows profile path if it is not ...

kumar303 · 2017-07-18T17:04:17Z

tests/unit/test-firefox/test.firefox.js

+         assert.equal(isDevEditionDefault, true);
+       });
+
+    it('allows profile name if is not listed as default in profiles.ini',


nit: allows profile name if it is not ...

kumar303 · 2017-07-18T17:31:02Z

tests/unit/test-firefox/test.firefox.js

+           );
+           assert.equal(isManuallyDefault, true);
+
+           const isNotDefault = await firefox.isDefaultProfile(


This case looks like it's covered up above. Is there something different about testing it again here?

you are right, it is a leftover of the rewrite

coveralls · 2017-07-18T19:01:35Z

Coverage remained the same at 100.0% when pulling faac3d5 on rpl:fix/prevent-keep-profile-changes-on-default-profile into 7b390e4 on mozilla:master.

coveralls · 2017-07-18T19:11:56Z

Coverage remained the same at 100.0% when pulling ee9ea08 on rpl:fix/prevent-keep-profile-changes-on-default-profile into 7b390e4 on mozilla:master.

coveralls · 2017-07-18T19:15:59Z

Coverage remained the same at 100.0% when pulling ee9ea08 on rpl:fix/prevent-keep-profile-changes-on-default-profile into 7b390e4 on mozilla:master.

coveralls · 2017-07-18T19:25:47Z

Coverage remained the same at 100.0% when pulling 52c86ba on rpl:fix/prevent-keep-profile-changes-on-default-profile into 7b390e4 on mozilla:master.

coveralls · 2017-07-18T19:28:36Z

Coverage remained the same at 100.0% when pulling 0d0d5d8 on rpl:fix/prevent-keep-profile-changes-on-default-profile into 7b390e4 on mozilla:master.

coveralls · 2017-07-18T19:34:27Z

Coverage remained the same at 100.0% when pulling 0d0d5d8 on rpl:fix/prevent-keep-profile-changes-on-default-profile into 7b390e4 on mozilla:master.

…ith a new test case

coveralls · 2017-07-18T20:12:29Z

Coverage remained the same at 100.0% when pulling 896a757 on rpl:fix/prevent-keep-profile-changes-on-default-profile into 7b390e4 on mozilla:master.

rpl · 2017-08-01T16:43:36Z

@kumar303 this should be ready now, it can also help to have a second pair of eyes that check the new behavior locally

kumar303

Thanks, looks great. I had one minor comment but it's not necessary to block this patch on. I tested this out and got the error :)

kumar303 · 2017-08-01T20:04:15Z

tests/unit/test-firefox/test.firefox.js

+           try {
+             await firefox.isDefaultProfile(
+               '/tmp/my-custom-profile-dir',
+               FakeProfileFinder,


I prefer to see optional function parameters passed as an object, like:

firefox.isDefaultProfile('/tmp/my-custom-profile-dir', { profileFinder: FakeProfileFinder, fsStat: fakeFsStat, });

However, this preference was born in a world without Flow so I can see how it's not such a big deal anymore. Anyway, I just wanted to point it out since it would be the first time we have a function in web-ext that does optional params like this.

rpl added in progress type: bug labels Jul 17, 2017

muffinresearch added the state: pull request ready label Jul 17, 2017

rpl force-pushed the fix/prevent-keep-profile-changes-on-default-profile branch from ea4c214 to 778757a Compare July 18, 2017 10:53

fix: Prevent --keep-profile-changes on default Firefox profiles

cc10673

rpl force-pushed the fix/prevent-keep-profile-changes-on-default-profile branch from 778757a to cc10673 Compare July 18, 2017 11:47

rpl requested review from kumar303 and saintsebastian July 18, 2017 11:51

saintsebastian reviewed Jul 18, 2017

View reviewed changes

kumar303 suggested changes Jul 18, 2017

View reviewed changes

chore: fixed flow error and removed eslint-disable-line leftover

faac3d5

rpl added 3 commits July 18, 2017 21:08

chore: compute fake absolute profile path to fix failure on windows CI

953a644

chore: remove redundant asserion

ee9ea08

chore: fix test descriptions

4aad0aa

rpl added 2 commits July 18, 2017 21:20

chore: added test assertion messages

3c1c0a3

chore: tweak error message

52c86ba

chore: change indexOf into includes

0d0d5d8

chore: cover unexpected error while searching the profiles.ini file w…

896a757

…ith a new test case

kumar303 self-requested a review August 1, 2017 16:40

kumar303 approved these changes Aug 1, 2017

View reviewed changes

kumar303 merged commit 532951f into mozilla:master Aug 1, 2017

This was referenced Apr 27, 2018

fix: Can't run with specified profile name and --keep-profile-changes #968

Closed

Using a default profile with --keep-profile-changes will break Firefox #1005

Closed


		// isDefaultProfile types and implementation.

		const DEFAULT_PROFILES_NAMES = [

Conversation

rpl commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jul 17, 2017

Uh oh!

coveralls commented Jul 18, 2017

Uh oh!

coveralls commented Jul 18, 2017

Uh oh!

rpl commented Jul 18, 2017

Uh oh!

saintsebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kumar303 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpl Jul 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jul 18, 2017

Uh oh!

coveralls commented Jul 18, 2017

Uh oh!

rpl commented Jul 17, 2017 •

edited

Loading

rpl Jul 18, 2017 •

edited

Loading