From cc106739f3250d79f7d1791151291f2aab50c162 Mon Sep 17 00:00:00 2001 From: Luca Greco Date: Mon, 17 Jul 2017 21:44:48 +0200 Subject: [PATCH 1/9] fix: Prevent --keep-profile-changes on default Firefox profiles --- src/firefox/index.js | 90 +++++++++++++ tests/unit/test-firefox/test.firefox.js | 162 ++++++++++++++++++++++++ 2 files changed, 252 insertions(+) diff --git a/src/firefox/index.js b/src/firefox/index.js index 756fe42fd3..6b28aad559 100644 --- a/src/firefox/index.js +++ b/src/firefox/index.js @@ -189,6 +189,86 @@ export async function run( } +// isDefaultProfile types and implementation. + +const DEFAULT_PROFILES_NAMES = [ + 'default', + 'dev-edition-default', +]; + +export type IsDefaultProfileFn = ( + profilePathOrName: string, + ProfileFinder?: typeof FirefoxProfile.Finder, +) => Promise; + +/* + * Tests if a profile is a default Firefox profile (both as a profile name or + * profile path). + * + * Returns a promise that resolves to true if the profile is one of default Firefox profile. + */ +export async function isDefaultProfile( + profilePathOrName: string, + ProfileFinder?: typeof FirefoxProfile.Finder = FirefoxProfile.Finder +): Promise { + if (DEFAULT_PROFILES_NAMES.indexOf(profilePathOrName) >= 0) { + return true; + } + + const baseProfileDir = ProfileFinder.locateUserDirectory(); + const profilesIniPath = path.join(baseProfileDir, 'profiles.ini'); + try { + await fs.stat(profilesIniPath); + } catch (error) { + if (isErrorWithCode('ENOENT', error)) { + // No profiles exist yet, default to false (the default profile name contains a + // random generated component). + return false; + } + + // Re-throw any unexpected exception. + throw error; + } + + // Check for profile dir path. + const finder = new ProfileFinder(baseProfileDir); + const readProfiles = promisify(finder.readProfiles, finder); + + await readProfiles(); + + const normalizedProfileDirPath = path.normalize( + path.join(path.resolve(profilePathOrName), path.sep) + ); + + 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 || + profile.Default === '1') { + let profileFullPath; + + // Check for profile name. + if (profile.Name === profilePathOrName) { + return true; + } + + // Check for profile path. + if (profile.IsRelative === '1') { + profileFullPath = path.join(baseProfileDir, profile.Path, path.sep); + } else { + profileFullPath = path.join(profile.Path, path.sep); + } + + if (path.normalize(profileFullPath) === normalizedProfileDirPath) { + return true; + } + } + } + + // Profile directory not found. + return false; +} + // configureProfile types and implementation. export type ConfigureProfileOptions = {| @@ -238,6 +318,7 @@ export function configureProfile( export type UseProfileParams = { app?: PreferencesAppName, configureThisProfile?: ConfigureProfileFn, + isFirefoxDefaultProfile?: IsDefaultProfileFn, customPrefs?: FirefoxPreferences, }; @@ -248,9 +329,18 @@ export async function useProfile( { app, configureThisProfile = configureProfile, + isFirefoxDefaultProfile = isDefaultProfile, customPrefs = {}, }: UseProfileParams = {}, ): Promise { + const isForbiddenProfile = await isFirefoxDefaultProfile(profilePath); + if (isForbiddenProfile) { + throw new UsageError( + 'Using --keep-profile-changes' + + ` on the Firefox default profile "${profilePath}" is forbidden.` + + '\n(See https://github.com/mozilla/web-ext/issues/1005)' + ); + } const profile = new FirefoxProfile({destinationDirectory: profilePath}); return await configureThisProfile(profile, {app, customPrefs}); } diff --git a/tests/unit/test-firefox/test.firefox.js b/tests/unit/test-firefox/test.firefox.js index 80363fa460..56c89eaf56 100644 --- a/tests/unit/test-firefox/test.firefox.js +++ b/tests/unit/test-firefox/test.firefox.js @@ -35,6 +35,37 @@ function withBaseProfile(callback) { ); } +function createFakeProfileFinder(profilesDirPath) { + const FakeProfileFinder = sinon.spy((...args) => { + const finder = new FirefoxProfile.Finder(...args); + + sinon.spy(finder, 'readProfiles'); + + return finder; + }); + + FakeProfileFinder.locateUserDirectory = sinon.spy(() => { + return profilesDirPath; + }); + + return FakeProfileFinder; +} + +async function createFakeProfilesIni(dirPath, profilesDefs) { // eslint-disable-line + let content = ''; + + for (const [idx, profile] of profilesDefs.entries()) { + content += `[Profile${idx}]\n`; + for (const k of Object.keys(profile)) { + // $FLOW_FIXME: test123 + content += `${k}=${profile[k]}\n`; + } + content += '\n'; + } + + await fs.writeFile(path.join(dirPath, 'profiles.ini'), content); +} + describe('firefox', () => { describe('run', () => { @@ -263,6 +294,113 @@ describe('firefox', () => { }); + describe('isDefaultProfile', () => { + + it('detects common Firefox default profiles specified by name', + async () => { + const isDefault = await firefox.isDefaultProfile('default'); + assert.equal(isDefault, true); + + const isDevEditionDefault = await firefox.isDefaultProfile( + 'dev-edition-default' + ); + assert.equal(isDevEditionDefault, true); + }); + + it('allows profile name if is not listed as default in profiles.ini', + async () => { + return withTempDir(async (tmpDir) => { + const profilesDirPath = tmpDir.path(); + const FakeProfileFinder = createFakeProfileFinder(profilesDirPath); + + await createFakeProfilesIni(profilesDirPath, [ + { + Name: 'manually-set-default', + Path: 'fake-default-profile', + IsRelative: 1, + Default: 1, + }, + ]); + + const isDefault = await firefox.isDefaultProfile( + 'manually-set-default', FakeProfileFinder + ); + const isNotDefault = await firefox.isDefaultProfile( + 'unkown-profile-name', FakeProfileFinder + ); + + assert.equal(isDefault, true); + assert.equal(isNotDefault, false); + }); + }); + + it('allows profile path if is not listed as default in profiles.ini', + async () => { + return withTempDir(async (tmpDir) => { + const profilesDirPath = tmpDir.path(); + const FakeProfileFinder = createFakeProfileFinder(profilesDirPath); + + await createFakeProfilesIni(profilesDirPath, [ + { + Name: 'default', + Path: 'fake-default-profile', + IsRelative: 1, + }, + { + Name: 'dev-edition-default', + Path: 'fake-devedition-default-profile', + IsRelative: 1, + }, + { + Name: 'manually-set-default', + Path: '/tmp/fake-manually-default-profile', + Default: 1, + }, + ]); + + const isFirefoxDefaultPath = await firefox.isDefaultProfile( + path.join(profilesDirPath, 'fake-default-profile'), + FakeProfileFinder + ); + assert.equal(isFirefoxDefaultPath, true); + + const isDevEditionDefaultPath = await firefox.isDefaultProfile( + path.join(profilesDirPath, 'fake-devedition-default-profile'), + FakeProfileFinder + ); + assert.equal(isDevEditionDefaultPath, true); + + const isManuallyDefault = await firefox.isDefaultProfile( + '/tmp/fake-manually-default-profile', + FakeProfileFinder + ); + assert.equal(isManuallyDefault, true); + + const isNotDefault = await firefox.isDefaultProfile( + path.join(profilesDirPath, 'unkown-profile-dir'), + FakeProfileFinder + ); + assert.equal(isNotDefault, false); + }); + }); + + it('allows profile path if there is no profiles.ini file', + async () => { + return withTempDir(async (tmpDir) => { + const profilesDirPath = tmpDir.path(); + const FakeProfileFinder = createFakeProfileFinder(profilesDirPath); + + const isNotDefault = await firefox.isDefaultProfile( + '/tmp/my-custom-profile-dir', + FakeProfileFinder + ); + + assert.equal(isNotDefault, false); + }); + }); + + }); + describe('createProfile', () => { it('resolves with a profile object', () => { @@ -304,6 +442,30 @@ describe('firefox', () => { }); describe('useProfile', () => { + it('rejects to a UsageError when used on a default Firefox profile', + async () => { + const configureThisProfile = sinon.spy( + (profile) => Promise.resolve(profile) + ); + const isFirefoxDefaultProfile = sinon.spy( + () => Promise.resolve(true) + ); + let exception; + + try { + await firefox.useProfile('default', { + configureThisProfile, + isFirefoxDefaultProfile, + }); + } catch (error) { + exception = error; + } + + assert.match( + exception && exception.message, + /Using --keep-profile-changes .* forbidden/ + ); + }); it('resolves to a FirefoxProfile instance', () => withBaseProfile( (baseProfile) => { From faac3d5623dca301b0695fc4b0e4115fcbb6fe4c Mon Sep 17 00:00:00 2001 From: Luca Greco Date: Tue, 18 Jul 2017 20:58:18 +0200 Subject: [PATCH 2/9] chore: fixed flow error and removed eslint-disable-line leftover --- tests/unit/test-firefox/test.firefox.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/unit/test-firefox/test.firefox.js b/tests/unit/test-firefox/test.firefox.js index 56c89eaf56..fe0e418026 100644 --- a/tests/unit/test-firefox/test.firefox.js +++ b/tests/unit/test-firefox/test.firefox.js @@ -51,13 +51,14 @@ function createFakeProfileFinder(profilesDirPath) { return FakeProfileFinder; } -async function createFakeProfilesIni(dirPath, profilesDefs) { // eslint-disable-line +async function createFakeProfilesIni( + dirPath: string, profilesDefs: Array +): Promise { let content = ''; for (const [idx, profile] of profilesDefs.entries()) { content += `[Profile${idx}]\n`; for (const k of Object.keys(profile)) { - // $FLOW_FIXME: test123 content += `${k}=${profile[k]}\n`; } content += '\n'; From 953a64406b5736aa42c3f89e3b9277267f12ce8e Mon Sep 17 00:00:00 2001 From: Luca Greco Date: Tue, 18 Jul 2017 21:08:19 +0200 Subject: [PATCH 3/9] chore: compute fake absolute profile path to fix failure on windows CI --- tests/unit/test-firefox/test.firefox.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/unit/test-firefox/test.firefox.js b/tests/unit/test-firefox/test.firefox.js index fe0e418026..70d1568954 100644 --- a/tests/unit/test-firefox/test.firefox.js +++ b/tests/unit/test-firefox/test.firefox.js @@ -340,6 +340,10 @@ describe('firefox', () => { return withTempDir(async (tmpDir) => { const profilesDirPath = tmpDir.path(); const FakeProfileFinder = createFakeProfileFinder(profilesDirPath); + const absProfilePath = path.join( + profilesDirPath, + 'fake-manually-default-profile' + ); await createFakeProfilesIni(profilesDirPath, [ { @@ -354,7 +358,7 @@ describe('firefox', () => { }, { Name: 'manually-set-default', - Path: '/tmp/fake-manually-default-profile', + Path: absProfilePath, Default: 1, }, ]); @@ -372,7 +376,7 @@ describe('firefox', () => { assert.equal(isDevEditionDefaultPath, true); const isManuallyDefault = await firefox.isDefaultProfile( - '/tmp/fake-manually-default-profile', + absProfilePath, FakeProfileFinder ); assert.equal(isManuallyDefault, true); From ee9ea08865eaa0c2c7cd006d8332f692a0037198 Mon Sep 17 00:00:00 2001 From: Luca Greco Date: Tue, 18 Jul 2017 21:10:10 +0200 Subject: [PATCH 4/9] chore: remove redundant asserion --- tests/unit/test-firefox/test.firefox.js | 6 ------ 1 file changed, 6 deletions(-) diff --git a/tests/unit/test-firefox/test.firefox.js b/tests/unit/test-firefox/test.firefox.js index 70d1568954..e2b5124252 100644 --- a/tests/unit/test-firefox/test.firefox.js +++ b/tests/unit/test-firefox/test.firefox.js @@ -380,12 +380,6 @@ describe('firefox', () => { FakeProfileFinder ); assert.equal(isManuallyDefault, true); - - const isNotDefault = await firefox.isDefaultProfile( - path.join(profilesDirPath, 'unkown-profile-dir'), - FakeProfileFinder - ); - assert.equal(isNotDefault, false); }); }); From 4aad0aa74bcda680c3268793da12129b1b619745 Mon Sep 17 00:00:00 2001 From: Luca Greco Date: Tue, 18 Jul 2017 21:11:50 +0200 Subject: [PATCH 5/9] chore: fix test descriptions --- tests/unit/test-firefox/test.firefox.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/test-firefox/test.firefox.js b/tests/unit/test-firefox/test.firefox.js index e2b5124252..7815915327 100644 --- a/tests/unit/test-firefox/test.firefox.js +++ b/tests/unit/test-firefox/test.firefox.js @@ -308,7 +308,7 @@ describe('firefox', () => { assert.equal(isDevEditionDefault, true); }); - it('allows profile name if is not listed as default in profiles.ini', + it('allows profile name if it is not listed as default in profiles.ini', async () => { return withTempDir(async (tmpDir) => { const profilesDirPath = tmpDir.path(); @@ -335,7 +335,7 @@ describe('firefox', () => { }); }); - it('allows profile path if is not listed as default in profiles.ini', + it('allows profile path if it is not listed as default in profiles.ini', async () => { return withTempDir(async (tmpDir) => { const profilesDirPath = tmpDir.path(); From 3c1c0a3b1f11e8784b7d6da3e18bf328b9db3c2d Mon Sep 17 00:00:00 2001 From: Luca Greco Date: Tue, 18 Jul 2017 21:20:51 +0200 Subject: [PATCH 6/9] chore: added test assertion messages --- tests/unit/test-firefox/test.firefox.js | 36 ++++++++++++++++++++----- 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/tests/unit/test-firefox/test.firefox.js b/tests/unit/test-firefox/test.firefox.js index 7815915327..53f837c5ce 100644 --- a/tests/unit/test-firefox/test.firefox.js +++ b/tests/unit/test-firefox/test.firefox.js @@ -326,12 +326,18 @@ describe('firefox', () => { const isDefault = await firefox.isDefaultProfile( 'manually-set-default', FakeProfileFinder ); + assert.equal( + isDefault, true, + 'Manually configured default profile' + ); + const isNotDefault = await firefox.isDefaultProfile( 'unkown-profile-name', FakeProfileFinder ); - - assert.equal(isDefault, true); - assert.equal(isNotDefault, false); + assert.equal( + isNotDefault, false, + 'Unknown profile name' + ); }); }); @@ -367,19 +373,37 @@ describe('firefox', () => { path.join(profilesDirPath, 'fake-default-profile'), FakeProfileFinder ); - assert.equal(isFirefoxDefaultPath, true); + assert.equal( + isFirefoxDefaultPath, true, + 'Firefox default profile' + ); const isDevEditionDefaultPath = await firefox.isDefaultProfile( path.join(profilesDirPath, 'fake-devedition-default-profile'), FakeProfileFinder ); - assert.equal(isDevEditionDefaultPath, true); + assert.equal( + isDevEditionDefaultPath, true, + 'Firefox DevEdition default profile' + ); const isManuallyDefault = await firefox.isDefaultProfile( absProfilePath, FakeProfileFinder ); - assert.equal(isManuallyDefault, true); + assert.equal( + isManuallyDefault, true, + 'Manually configured default profile' + ); + + const isNotDefault = await firefox.isDefaultProfile( + path.join(profilesDirPath, 'unkown-profile-dir'), + FakeProfileFinder + ); + assert.equal( + isNotDefault, false, + 'Unknown profile path' + ); }); }); From 52c86ba82c2b4a54e61e158abdfc99a1a5b39a19 Mon Sep 17 00:00:00 2001 From: Luca Greco Date: Tue, 18 Jul 2017 21:23:12 +0200 Subject: [PATCH 7/9] chore: tweak error message --- src/firefox/index.js | 7 ++++--- tests/unit/test-firefox/test.firefox.js | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/firefox/index.js b/src/firefox/index.js index 6b28aad559..96eb9c07fc 100644 --- a/src/firefox/index.js +++ b/src/firefox/index.js @@ -336,9 +336,10 @@ export async function useProfile( const isForbiddenProfile = await isFirefoxDefaultProfile(profilePath); if (isForbiddenProfile) { throw new UsageError( - 'Using --keep-profile-changes' + - ` on the Firefox default profile "${profilePath}" is forbidden.` + - '\n(See https://github.com/mozilla/web-ext/issues/1005)' + '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' ); } const profile = new FirefoxProfile({destinationDirectory: profilePath}); diff --git a/tests/unit/test-firefox/test.firefox.js b/tests/unit/test-firefox/test.firefox.js index 53f837c5ce..e804050d5d 100644 --- a/tests/unit/test-firefox/test.firefox.js +++ b/tests/unit/test-firefox/test.firefox.js @@ -486,7 +486,7 @@ describe('firefox', () => { assert.match( exception && exception.message, - /Using --keep-profile-changes .* forbidden/ + /Cannot use --keep-profile-changes on a default profile/ ); }); From 0d0d5d8b952911fb0af2531d4eb05c402f933fd4 Mon Sep 17 00:00:00 2001 From: Luca Greco Date: Tue, 18 Jul 2017 21:27:57 +0200 Subject: [PATCH 8/9] chore: change indexOf into includes --- src/firefox/index.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/firefox/index.js b/src/firefox/index.js index 96eb9c07fc..9b12c5cf2f 100644 --- a/src/firefox/index.js +++ b/src/firefox/index.js @@ -211,7 +211,7 @@ export async function isDefaultProfile( profilePathOrName: string, ProfileFinder?: typeof FirefoxProfile.Finder = FirefoxProfile.Finder ): Promise { - if (DEFAULT_PROFILES_NAMES.indexOf(profilePathOrName) >= 0) { + if (DEFAULT_PROFILES_NAMES.includes(profilePathOrName)) { return true; } @@ -243,7 +243,7 @@ export async function isDefaultProfile( 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 || + if (DEFAULT_PROFILES_NAMES.includes(profile.Name) || profile.Default === '1') { let profileFullPath; From 896a75722bd8e7447fc218fb019c61c09b0454f0 Mon Sep 17 00:00:00 2001 From: Luca Greco Date: Tue, 18 Jul 2017 22:06:59 +0200 Subject: [PATCH 9/9] chore: cover unexpected error while searching the profiles.ini file with a new test case --- src/firefox/index.js | 8 ++++++-- tests/unit/test-firefox/test.firefox.js | 25 +++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/src/firefox/index.js b/src/firefox/index.js index 9b12c5cf2f..c3adff970b 100644 --- a/src/firefox/index.js +++ b/src/firefox/index.js @@ -199,6 +199,7 @@ const DEFAULT_PROFILES_NAMES = [ export type IsDefaultProfileFn = ( profilePathOrName: string, ProfileFinder?: typeof FirefoxProfile.Finder, + fsStat?: typeof fs.stat, ) => Promise; /* @@ -209,7 +210,8 @@ export type IsDefaultProfileFn = ( */ export async function isDefaultProfile( profilePathOrName: string, - ProfileFinder?: typeof FirefoxProfile.Finder = FirefoxProfile.Finder + ProfileFinder?: typeof FirefoxProfile.Finder = FirefoxProfile.Finder, + fsStat?: typeof fs.stat = fs.stat, ): Promise { if (DEFAULT_PROFILES_NAMES.includes(profilePathOrName)) { return true; @@ -218,9 +220,11 @@ export async function isDefaultProfile( const baseProfileDir = ProfileFinder.locateUserDirectory(); const profilesIniPath = path.join(baseProfileDir, 'profiles.ini'); try { - await fs.stat(profilesIniPath); + await fsStat(profilesIniPath); } catch (error) { if (isErrorWithCode('ENOENT', error)) { + log.debug(`profiles.ini not found: ${error}`); + // No profiles exist yet, default to false (the default profile name contains a // random generated component). return false; diff --git a/tests/unit/test-firefox/test.firefox.js b/tests/unit/test-firefox/test.firefox.js index e804050d5d..ed8112ffe5 100644 --- a/tests/unit/test-firefox/test.firefox.js +++ b/tests/unit/test-firefox/test.firefox.js @@ -422,6 +422,31 @@ describe('firefox', () => { }); }); + it('rejects on any unexpected error while looking for profiles.ini', + async () => { + return withTempDir(async (tmpDir) => { + const profilesDirPath = tmpDir.path(); + const FakeProfileFinder = createFakeProfileFinder(profilesDirPath); + const fakeFsStat = sinon.spy(() => { + return Promise.reject(new Error('Fake fs stat error')); + }); + + let exception; + try { + await firefox.isDefaultProfile( + '/tmp/my-custom-profile-dir', + FakeProfileFinder, + fakeFsStat + ); + } catch (error) { + exception = error; + } + + assert.match(exception && exception.message, /Fake fs stat error/); + }); + } + ); + }); describe('createProfile', () => {