Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kumar303 merged 9 commits into from
Aug 1, 2017
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
cc10673
fix: Prevent --keep-profile-changes on default Firefox profiles
rpl Jul 17, 2017
faac3d5
chore: fixed flow error and removed eslint-disable-line leftover
rpl Jul 18, 2017
953a644
chore: compute fake absolute profile path to fix failure on windows CI
rpl Jul 18, 2017
ee9ea08
chore: remove redundant asserion
rpl Jul 18, 2017
4aad0aa
chore: fix test descriptions
rpl Jul 18, 2017
3c1c0a3
chore: added test assertion messages
rpl Jul 18, 2017
52c86ba
chore: tweak error message
rpl Jul 18, 2017
0d0d5d8
chore: change indexOf into includes
rpl Jul 18, 2017
896a757
chore: cover unexpected error while searching the profiles.ini file w…
rpl Jul 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 95 additions & 0 deletions src/firefox/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,90 @@ export async function run(
}


// isDefaultProfile types and implementation.

const DEFAULT_PROFILES_NAMES = [
Copy link
Contributor

@saintsebastian saintsebastian Jul 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@rpl rpl Jul 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

'default',
'dev-edition-default',
];

export type IsDefaultProfileFn = (
profilePathOrName: string,
ProfileFinder?: typeof FirefoxProfile.Finder,
fsStat?: typeof fs.stat,
) => Promise<boolean>;

/*
* 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,
fsStat?: typeof fs.stat = fs.stat,
): Promise<boolean> {
if (DEFAULT_PROFILES_NAMES.includes(profilePathOrName)) {
return true;
}

const baseProfileDir = ProfileFinder.locateUserDirectory();
const profilesIniPath = path.join(baseProfileDir, 'profiles.ini');
try {
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;
}

// Re-throw any unexpected exception.
throw error;
Copy link
Contributor

@kumar303 kumar303 Jul 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@rpl rpl Jul 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

// 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.includes(profile.Name) ||
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 = {|
Expand Down Expand Up @@ -238,6 +322,7 @@ export function configureProfile(
export type UseProfileParams = {
app?: PreferencesAppName,
configureThisProfile?: ConfigureProfileFn,
isFirefoxDefaultProfile?: IsDefaultProfileFn,
customPrefs?: FirefoxPreferences,
};

Expand All @@ -248,9 +333,19 @@ export async function useProfile(
{
app,
configureThisProfile = configureProfile,
isFirefoxDefaultProfile = isDefaultProfile,
customPrefs = {},
}: UseProfileParams = {},
): Promise<FirefoxProfile> {
const isForbiddenProfile = await isFirefoxDefaultProfile(profilePath);
Copy link
Contributor

@saintsebastian saintsebastian Jul 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@rpl rpl Jul 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@kumar303 kumar303 Jul 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (isForbiddenProfile) {
throw new UsageError(
'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});
return await configureThisProfile(profile, {app, customPrefs});
}
Expand Down
210 changes: 210 additions & 0 deletions tests/unit/test-firefox/test.firefox.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,38 @@ 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: string, profilesDefs: Array<Object>
): Promise<void> {
let content = '';

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

await fs.writeFile(path.join(dirPath, 'profiles.ini'), content);
}

describe('firefox', () => {

describe('run', () => {
Expand Down Expand Up @@ -263,6 +295,160 @@ 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 it 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
);
assert.equal(
isDefault, true,
'Manually configured default profile'
);

const isNotDefault = await firefox.isDefaultProfile(
'unkown-profile-name', FakeProfileFinder
);
assert.equal(
isNotDefault, false,
'Unknown profile name'
);
});
});

it('allows profile path if it is not listed as default in profiles.ini',
async () => {
return withTempDir(async (tmpDir) => {
const profilesDirPath = tmpDir.path();
const FakeProfileFinder = createFakeProfileFinder(profilesDirPath);
const absProfilePath = path.join(
profilesDirPath,
'fake-manually-default-profile'
);

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: absProfilePath,
Default: 1,
},
]);

const isFirefoxDefaultPath = await firefox.isDefaultProfile(
path.join(profilesDirPath, 'fake-default-profile'),
FakeProfileFinder
);
assert.equal(
isFirefoxDefaultPath, true,
'Firefox default profile'
);

const isDevEditionDefaultPath = await firefox.isDefaultProfile(
path.join(profilesDirPath, 'fake-devedition-default-profile'),
FakeProfileFinder
);
assert.equal(
isDevEditionDefaultPath, true,
'Firefox DevEdition default profile'
);

const isManuallyDefault = await firefox.isDefaultProfile(
absProfilePath,
FakeProfileFinder
);
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'
);
});
});

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);
});
});

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,
Copy link
Contributor

@kumar303 kumar303 Aug 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

fakeFsStat
);
} catch (error) {
exception = error;
}

assert.match(exception && exception.message, /Fake fs stat error/);
});
}
);

});

describe('createProfile', () => {

it('resolves with a profile object', () => {
Expand Down Expand Up @@ -304,6 +490,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,
/Cannot use --keep-profile-changes on a default profile/
);
});

it('resolves to a FirefoxProfile instance', () => withBaseProfile(
(baseProfile) => {
Expand Down