[tor-commits] [tor-browser] 211/311: Bug 1759162 - Fix pkcs11.isModuleInstalled regressions triggered on non-windows builds. r=barret, willdurand a=pascalc

[gitolite role]

This is an automated email from the git hooks/post-receive script.

pierov pushed a commit to branch geckoview-99.0.1-11.0-1
in repository tor-browser.

commit 4c0756a5ad149b69e5882a84cb1c9e49393d6396
Author: Luca Greco <lgreco at mozilla.com>
AuthorDate: Tue Mar 15 20:13:16 2022 +0000

    Bug 1759162 - Fix pkcs11.isModuleInstalled regressions triggered on non-windows builds. r=barret,willdurand a=pascalc
    
    PathUtils.filename throws if the path is not an absolute path, which was likely
    not the case with the OS.Path.basename call used previously.
    
    This internal change of behavior shouldn't be triggering any issue on Windows,
    where the hostInfo.manifest.path seems to be always normalized into an absolute
    path (by computing it as relative to the hostInfo.manifest if it wasn't already
    an absolute path), but it makes browser.pkcs11.isModuleInstalled to regress
    on Linux (and maybe also on MacOS if the pkcs11 manifest files include only
    the library name and not its full path, as it seems to be the case for
    the Belgium eID pkcs11 manifest packaged for Linux).
    
    The result of PathUtils.filename is expected to only include the basename of
    the file (without the full dir path and the file extension) and so to fix
    the regression being triggered on non-windows platform we could use a fake
    absolute url to get the expected result using PathUtils.filename as is.
    
    Differential Revision: https://phabricator.services.mozilla.com/D141097
---
 browser/components/extensions/parent/ext-pkcs11.js | 34 +++++++++++++++-------
 .../test/xpcshell/test_ext_pkcs11_management.js    | 17 +++++++++++
 2 files changed, 40 insertions(+), 11 deletions(-)

diff --git a/browser/components/extensions/parent/ext-pkcs11.js b/browser/components/extensions/parent/ext-pkcs11.js
index db6a6af7c87b0..5f72786cae4a7 100644
--- a/browser/components/extensions/parent/ext-pkcs11.js
+++ b/browser/components/extensions/parent/ext-pkcs11.js
@@ -39,19 +39,31 @@ this.pkcs11 = class extends ExtensionAPI {
         context
       );
       if (hostInfo) {
-        if (AppConstants.platform === "win") {
-          // If the path specified in the manifest is not an abslute path,
-          // translate it relative to manifest's directory.
-          if (!PathUtils.isAbsolute(hostInfo.manifest.path)) {
-            hostInfo.manifest.path = PathUtils.normalize(
-              PathUtils.joinRelative(
-                PathUtils.parent(hostInfo.path),
-                hostInfo.manifest.path
-              )
+        // We don't normalize the absolute path below because
+        // `Path.normalize` throws when the target file doesn't
+        // exist, and that might be the case on non Windows
+        // builds.
+        let absolutePath = PathUtils.isAbsolute(hostInfo.manifest.path)
+          ? hostInfo.manifest.path
+          : PathUtils.joinRelative(
+              PathUtils.parent(hostInfo.path),
+              hostInfo.manifest.path
             );
-          }
+
+        if (AppConstants.platform === "win") {
+          // On Windows, `hostInfo.manifest.path` is expected to be a normalized
+          // absolute path. On other platforms, this path may be relative but we
+          // cannot use `PathUtils.normalize()` on non-absolute paths.
+          absolutePath = PathUtils.normalize(absolutePath);
+          hostInfo.manifest.path = absolutePath;
         }
-        let manifestLib = PathUtils.filename(hostInfo.manifest.path);
+
+        // PathUtils.filename throws if the path is not an absolute path.
+        // The result is expected to be the basename of the file (without
+        // the dir path and the extension) so it is fine to use an absolute
+        // path that may not be normalized (non-Windows platforms).
+        let manifestLib = PathUtils.filename(absolutePath);
+
         if (AppConstants.platform !== "linux") {
           manifestLib = manifestLib.toLowerCase(manifestLib);
         }
diff --git a/browser/components/extensions/test/xpcshell/test_ext_pkcs11_management.js b/browser/components/extensions/test/xpcshell/test_ext_pkcs11_management.js
index 36f66ecebdece..9d6dae0d6be72 100644
--- a/browser/components/extensions/test/xpcshell/test_ext_pkcs11_management.js
+++ b/browser/components/extensions/test/xpcshell/test_ext_pkcs11_management.js
@@ -111,6 +111,17 @@ async function setupManifests(modules) {
 add_task(async function test_pkcs11() {
   async function background() {
     try {
+      const { os } = await browser.runtime.getPlatformInfo();
+      if (os !== "win") {
+        // Expect this call to not throw (explicitly cover regression fixed in Bug 1759162).
+        let isInstalledNonAbsolute = await browser.pkcs11.isModuleInstalled(
+          "testmoduleNonAbsolutePath"
+        );
+        browser.test.assertFalse(
+          isInstalledNonAbsolute,
+          "PKCS#11 module with non absolute path expected to not be installed"
+        );
+      }
       let isInstalled = await browser.pkcs11.isModuleInstalled("testmodule");
       browser.test.assertFalse(
         isInstalled,
@@ -247,6 +258,12 @@ add_task(async function test_pkcs11() {
       path: testmodule,
       id: "pkcs11 at tests.mozilla.org",
     },
+    {
+      name: "testmoduleNonAbsolutePath",
+      description: "PKCS#11 Test Module",
+      path: ctypes.libraryName("pkcs11testmodule"),
+      id: "pkcs11 at tests.mozilla.org",
+    },
     {
       name: "othermodule",
       description: "PKCS#11 Test Module",

-- 
To stop receiving notification emails like this one, please contact
the administrator of this repository.