Skip to content
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

Look for Firefox in HKCU as well as HKLM (fix #73) #74

Merged
merged 6 commits into from
Apr 15, 2021
Merged

Look for Firefox in HKCU as well as HKLM (fix #73) #74

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
07f5163
Look for Firefox in HKCU as well as HKLM
mccreery Mar 15, 2021
b01ef4b
Rename result to resultKey
mccreery Apr 8, 2021
4db9036
Modify registry test to fail on access to non-HKLM
mccreery Apr 8, 2021
c7f3c86
Add test for prefer HKCU registry hive over HKLM
mccreery Apr 8, 2021
d42d4f7
Use consistent hive keys with Winreg library
mccreery Apr 14, 2021
d53b2e2
Clarify test comments for modified case
mccreery Apr 15, 2021
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
58 changes: 29 additions & 29 deletions lib/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,41 +74,41 @@ function normalizeBinary (binaryPath, platform, arch) {
// Windows binary finding
var appName = normalizeBinary.appNames[app + " on windows"];

// this is used when reading the registry goes wrong.
function fallBack () {
resolve(getPathToExe(Winreg.HKCU, appName).catch(function() {
return getPathToExe(Winreg.HKLM, appName);
}).catch(function() {
// Neither registry hive has the correct keys
var programFilesVar = "ProgramFiles";
if (arch === "(64)") {
console.warn("You are using 32-bit version of Firefox on 64-bit versions of the Windows.\nSome features may not work correctly in this version. You should upgrade Firefox to the latest 64-bit version now!")
programFilesVar = "ProgramFiles(x86)";
}
resolve(path.join(process.env[programFilesVar], appName, "firefox.exe"));
}
return path.join(process.env[programFilesVar], appName, "firefox.exe");
}));
});
}

// Returns a promise to get Firefox's PathToExe from a registry hive
function getPathToExe(hive, appName) {
const rootKey = path.join("\\Software\\Mozilla\\", appName);

return getRegistryValue(hive, rootKey, "CurrentVersion").then(function(version) {
return getRegistryValue(hive, path.join(rootKey, version, "Main"), "PathToExe");
});
}

var rootKey = "\\Software\\Mozilla\\";
rootKey = path.join(rootKey, appName);

return when.promise(function(resolve, reject) {
var versionKey = new Winreg({
hive: Winreg.HKLM,
key: rootKey
});
versionKey.get("CurrentVersion", function(err, key) {
var isOk = key && !err;
return isOk ? resolve(key.value) : reject();
});
}).then(function(version) {
var mainKey = new Winreg({
hive: Winreg.HKLM,
key: path.join(rootKey, version, "Main")
});
mainKey.get("PathToExe", function(err, key) {
if (err) {
fallBack();
return;
}
resolve(key.value);
});
}, fallBack);
// Returns a promise to get a single registry value
function getRegistryValue(hive, key, name) {
return when.promise(function(resolve, reject) {
const registry = new Winreg({ hive, key });

registry.get(name, function(error, resultKey) {
if (resultKey && !error) {
resolve(resultKey.value);
} else {
reject(error);
}
});
});
}

Expand Down
63 changes: 58 additions & 5 deletions test/run/test.utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,67 @@ describe("lib/utils", function () {
return;
}

// see ./mock-winreg.js
var expected = "fake\\binary\\path";

// see ./mock-winreg.js
// Only mock keys in HKLM (local machine) hive
Copy link
Member

Choose a reason for hiding this comment

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

I suppose that you changed the implementation to veify that the logic still works as intended when there is a HKLM-only registry key (i.e. without HKCU)? Can you clarify that intent in the comment, since it isn't obvious.

Copy link
Contributor Author

@mccreery mccreery Apr 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

That's right. If you have all the correct keys in HKLM and some missing in HKCU the intended behaviour (as I see it) is to ignore HKCU and fall back on HKLM for all the keys. I also edited the comment above clarifying this. The code fails any access to HKCU to mimic the missing keys.

Copy link
Member

Choose a reason for hiding this comment

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

I meant in a code comment, not here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha, I thought it was a strange ask :P Will do.

var winreg = function(options) {
this.get = function(_, fn) {
if (options.hive === winreg.HKLM) {
fn(null, {value: expected});
} else {
fn("Failed", null);
}
};
};
winreg.HKLM = "HKLM";

var binary = sandbox.require("../../lib/utils", {
requires: {"winreg": function() {
this.get = function(_, fn) {
requires: { winreg }
}).normalizeBinary;

var promises = [
[null, "windows", "x86"],
[null, "windows", "x86_64"]
].map(function(args) {
var promise = binary.apply(binary, args);
return promise.then(function(actual) {
expect(actual).to.be.equal(expected);
});
});
all(promises).then(done.bind(null, null), done);
});

it("normalizeBinary() prefers HKCU registry hive over HKLM on Windows", function(done) {
// Skip this test for now, to get Travis running.
if (!/win/i.test(os.platform)) {
done();
return;
}

// Provide different paths depending on hive
var expected = "fake\\binary\\path";
var oldPath = "fake\\old\\binary\\path";

// see ./mock-winreg.js
// Only mock keys in HKLM (local machine) hive
var winreg = function(options) {
this.get = function(_, fn) {
if (options.hive === winreg.HKCU) {
fn(null, {value: expected});
};
}}
} else if (options.hive === winreg.HKLM) {
fn(null, {value: oldPath});
} else {
fn("Failed", null);
}
};
};
// Differentiate hives
winreg.HKLM = "HKLM";
winreg.HKCU = "HKCU";

var binary = sandbox.require("../../lib/utils", {
requires: { winreg }
}).normalizeBinary;

var promises = [
Expand Down