[NewEV] Fix Mac Binary Path Resolution

- Fixed resolution of tildes (~) in file paths (realpath doesn't handle
  that apparently).
  - Also expanding environment variables, but only if they are at the
    beginning of the path.
  - Env var and tilde expansion is done inside a loop to handle cases
    where an env var points to another one or to the tilde.
- Fixed extraction of product name and version signals by making sure
  the file path used there was pointing to the app bundle and not the
  end binary.
  - Resolution of the bundle path is done by walking up the path and
    loading it via [NSBundle bundleWithPath:] and testing to see if
    its infoDictionary has values or not (which is what we need in our
    use-case to get a product name and version out).

Unit tests were added and are making use of a test bundle (added to
test data).

Bug: b:231326198
Change-Id: I24b8d8de5f93386e8c6a0e3f47a8e16d80f5f63d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3909752
Commit-Queue: Sebastien Lalancette <seblalancette@chromium.org>
Reviewed-by: Hamda Mare <hmare@google.com>
Cr-Commit-Position: refs/heads/main@{#1054304}

chrome/services/system_signals/BUILD.gn

@@ -50,7 +50,7 @@ source_set("system_signals") {
 
     sources += [ "linux/linux_system_signals_service.cc" ]
 
-    deps += [ "//components/device_signals/core/system_signals/linux" ]
+    deps += [ "//components/device_signals/core/system_signals/posix" ]
   }
 }
 

chrome/services/system_signals/linux/linux_system_signals_service.cc

@@ -9,8 +9,8 @@
 #include "components/device_signals/core/common/common_types.h"
 #include "components/device_signals/core/system_signals/executable_metadata_service.h"
 #include "components/device_signals/core/system_signals/file_system_service.h"
-#include "components/device_signals/core/system_signals/linux/linux_platform_delegate.h"
 #include "components/device_signals/core/system_signals/platform_delegate.h"
+#include "components/device_signals/core/system_signals/posix/posix_platform_delegate.h"
 
 namespace system_signals {
 
@@ -19,9 +19,9 @@ LinuxSystemSignalsService::LinuxSystemSignalsService(
     : LinuxSystemSignalsService(
           std::move(receiver),
           device_signals::FileSystemService::Create(
-              std::make_unique<device_signals::LinuxPlatformDelegate>(),
+              std::make_unique<device_signals::PosixPlatformDelegate>(),
               device_signals::ExecutableMetadataService::Create(
-                  std::make_unique<device_signals::LinuxPlatformDelegate>()))) {
+                  std::make_unique<device_signals::PosixPlatformDelegate>()))) {
 }
 
 LinuxSystemSignalsService::LinuxSystemSignalsService(

components/device_signals/core/system_signals/BUILD.gn

@@ -88,4 +88,9 @@ source_set("unit_tests") {
   if (is_mac) {
     deps += [ "//components/device_signals/core/system_signals/mac:unit_tests" ]
   }
+
+  if (is_mac || is_linux) {
+    deps +=
+        [ "//components/device_signals/core/system_signals/posix:unit_tests" ]
+  }
 }

components/device_signals/core/system_signals/mac/BUILD.gn

@@ -8,12 +8,14 @@ static_library("mac") {
   sources = [ "mac_platform_delegate.mm" ]
 
   public_deps = [
-    "//components/device_signals/core/common",
     "//components/device_signals/core/system_signals",
     "//components/device_signals/core/system_signals/posix",
   ]
 
-  deps = [ "//base" ]
+  deps = [
+    "//base",
+    "//components/device_signals/core/common",
+  ]
 
   frameworks = [ "Foundation.framework" ]
 }
@@ -28,6 +30,7 @@ source_set("unit_tests") {
     "//components/device_signals/core/common",
     "//components/device_signals/core/system_signals",
     "//components/device_signals/core/system_signals:test_support",
+    "//components/device_signals/test:test_support",
     "//testing/gmock",
     "//testing/gtest",
   ]

components/device_signals/core/system_signals/mac/mac_platform_delegate.h

@@ -17,14 +17,11 @@ class MacPlatformDelegate : public PosixPlatformDelegate {
   // PlatformDelegate:
   bool ResolveFilePath(const base::FilePath& file_path,
                        base::FilePath* resolved_file_path) override;
+  absl::optional<ProductMetadata> GetProductMetadata(
+      const base::FilePath& file_path) override;
   absl::optional<std::vector<std::string>>
   GetSigningCertificatesPublicKeyHashes(
       const base::FilePath& file_path) override;
-
-  // Verifies if `file_path` points to an app bundle and then returns the
-  // executable path for the file inside the bundle. If `file_path` does not
-  // point to a bundle, it is returned as-is.
-  base::FilePath GetBinaryFilePath(const base::FilePath& file_path);
 };
 
 }  // namespace device_signals

components/device_signals/core/system_signals/mac/mac_platform_delegate.mm

@@ -12,42 +12,82 @@
 
 namespace device_signals {
 
+namespace {
+
+// Verifies if `file_path` points to an app bundle and then returns the
+// executable path for the file inside the bundle. If `file_path` does not
+// point to a bundle, it is returned as-is.
+base::FilePath GetBinaryFilePath(const base::FilePath& file_path) {
+  // Try to load the path into a bundle.
+  NSBundle* bundle =
+      [NSBundle bundleWithPath:base::mac::FilePathToNSString(file_path)];
+  if (bundle) {
+    NSString* executable_path = bundle.executablePath;
+    if (executable_path) {
+      return base::mac::NSStringToFilePath(executable_path);
+    }
+  }
+
+  return file_path;
+}
+
+// Attempts to parse out the bundle path from a binary absolute path. Returns
+// absl::nullopt if there is no bundle path in `file_path`. Since bundle paths
+// are solely used to get information about the bundle, the "path X is a bundle"
+// heuristic is based on whether bundle information is available at path X.
+absl::optional<base::FilePath> GetBundleFilePath(
+    const base::FilePath& file_path) {
+  @autoreleasepool {
+    base::FilePath current_path = file_path;
+    do {
+      NSString* current_path_string =
+          base::mac::FilePathToNSString(current_path);
+      NSBundle* bundle = [NSBundle bundleWithPath:current_path_string];
+      if (bundle.infoDictionary.count > 0) {
+        // Current path points to a bundle that has metadata.
+        return current_path;
+      }
+      current_path = current_path.DirName();
+    } while (current_path.GetComponents().size() > 1);
+
+    return absl::nullopt;
+  }
+}
+
+}  // namespace
+
 MacPlatformDelegate::MacPlatformDelegate() = default;
 
 MacPlatformDelegate::~MacPlatformDelegate() = default;
 
 bool MacPlatformDelegate::ResolveFilePath(const base::FilePath& file_path,
                                           base::FilePath* resolved_file_path) {
-  if (!PosixPlatformDelegate::ResolveFilePath(file_path, resolved_file_path)) {
+  base::FilePath temp_file_path;
+  if (!PosixPlatformDelegate::ResolveFilePath(file_path, &temp_file_path)) {
     return false;
   }
 
   // Since `file_path` might point to an app bundle, resolve that path to point
-  // to the binary too.
-  *resolved_file_path = GetBinaryFilePath(*resolved_file_path);
+  // to the binary too. This step is an optimization which will help keep the
+  // "isRunning" logic platform agnostic.
+  *resolved_file_path = GetBinaryFilePath(temp_file_path);
   return true;
 }
 
+absl::optional<PlatformDelegate::ProductMetadata>
+MacPlatformDelegate::GetProductMetadata(const base::FilePath& file_path) {
+  // The implementation in BasePlatformDelegate requires that the given path
+  // points to a bundle.
+  auto bundle_path = GetBundleFilePath(file_path);
+  return BasePlatformDelegate::GetProductMetadata(
+      bundle_path.value_or(file_path));
+}
+
 absl::optional<std::vector<std::string>>
 MacPlatformDelegate::GetSigningCertificatesPublicKeyHashes(
     const base::FilePath& file_path) {
   // TODO(b:231326198): Implement.
   return absl::nullopt;
 }
 
-base::FilePath MacPlatformDelegate::GetBinaryFilePath(
-    const base::FilePath& file_path) {
-  // Try to load the path into a bundle.
-  NSBundle* bundle =
-      [NSBundle bundleWithPath:base::mac::FilePathToNSString(file_path)];
-  if (bundle) {
-    NSString* executable_path = bundle.executablePath;
-    if (executable_path) {
-      return base::mac::NSStringToFilePath(executable_path);
-    }
-  }
-
-  return file_path;
-}
-
 }  // namespace device_signals

components/device_signals/core/system_signals/mac/mac_platform_delegate_unittest.mm

@@ -7,48 +7,79 @@
 #import <Foundation/Foundation.h>
 
 #include "base/files/file_path.h"
+#include "base/files/file_util.h"
 #import "base/mac/foundation_util.h"
+#include "components/device_signals/test/test_constants.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace device_signals {
 
 class MacPlatformDelegateTest : public testing::Test {
  protected:
+  MacPlatformDelegateTest()
+      : home_dir_(base::GetHomeDir()),
+        bundle_path_(test::GetTestBundlePath()),
+        binary_path_(test::GetTestBundleBinaryPath()) {}
+
+  const base::FilePath home_dir_;
+  const base::FilePath bundle_path_;
+  const base::FilePath binary_path_;
   MacPlatformDelegate platform_delegate_;
 };
 
 TEST_F(MacPlatformDelegateTest, ResolvingBundlePath) {
-  // The NSBundle object corresponds to the bundle directory containing the
-  // current executable.
-  // https://developer.apple.com/documentation/foundation/nsbundle
-  NSBundle* main_bundle = [NSBundle mainBundle];
-  NSString* bundle_binary_string = main_bundle.executablePath;
-  if (!main_bundle || !bundle_binary_string) {
-    // This test can only run if a mainBundle is accessible. Not having a main
-    // bundle for the test runner should not be considered a failure case
-    // though.
-    return;
-  }
-
-  base::FilePath bundle_path =
-      base::mac::NSStringToFilePath(main_bundle.bundlePath);
-  base::FilePath binary_path =
-      base::mac::NSStringToFilePath(bundle_binary_string);
-
-  // `GetBinaryFilePath` should return the bundle's main executable.
-  EXPECT_EQ(platform_delegate_.GetBinaryFilePath(bundle_path), binary_path);
-
-  // `ResolveFilePath` should also point to the bundle's main executable.
+  // Should point to the bundle's main executable.
   base::FilePath resolved_file_path;
   ASSERT_TRUE(
-      platform_delegate_.ResolveFilePath(bundle_path, &resolved_file_path));
-  EXPECT_EQ(resolved_file_path, binary_path);
+      platform_delegate_.ResolveFilePath(bundle_path_, &resolved_file_path));
+  EXPECT_EQ(resolved_file_path, binary_path_);
+}
+
+TEST_F(MacPlatformDelegateTest, ResolveFilePath_Absolute) {
+  base::FilePath resolved_file_path;
+  EXPECT_TRUE(
+      platform_delegate_.ResolveFilePath(binary_path_, &resolved_file_path));
+  EXPECT_EQ(resolved_file_path, binary_path_);
+}
+
+TEST_F(MacPlatformDelegateTest, ResolveFilePath_PathTraversal) {
+  auto bundle_base_name = bundle_path_.BaseName();
+  auto file_path_with_traversal = bundle_path_.DirName();
+  auto bundle_dir_base_name = file_path_with_traversal.BaseName();
+  file_path_with_traversal = file_path_with_traversal.Append("..")
+                                 .Append(bundle_dir_base_name)
+                                 .Append(bundle_base_name);
+
+  base::FilePath resolved_file_path;
+  EXPECT_TRUE(platform_delegate_.ResolveFilePath(file_path_with_traversal,
+                                                 &resolved_file_path));
+
+  // Final resolved path should point to the binary directly.
+  EXPECT_EQ(resolved_file_path, binary_path_);
 }
 
-TEST_F(MacPlatformDelegateTest, GetBinaryFilePath_BundleNotFound) {
+TEST_F(MacPlatformDelegateTest, ResolveFilePath_BundleNotFound) {
   base::FilePath file_path =
       base::FilePath::FromUTF8Unsafe("/some/file/path/no/bundle");
-  EXPECT_EQ(platform_delegate_.GetBinaryFilePath(file_path), file_path);
+  base::FilePath resolved_file_path;
+  EXPECT_FALSE(
+      platform_delegate_.ResolveFilePath(file_path, &resolved_file_path));
+}
+
+TEST_F(MacPlatformDelegateTest, GetProductMetadata_Success) {
+  auto product_metadata = platform_delegate_.GetProductMetadata(bundle_path_);
+
+  ASSERT_TRUE(product_metadata);
+  ASSERT_TRUE(product_metadata->name);
+  EXPECT_EQ(product_metadata->name.value(), test::GetTestBundleProductName());
+  ASSERT_TRUE(product_metadata->version);
+  EXPECT_EQ(product_metadata->version.value(),
+            test::GetTestBundleProductVersion());
+
+  // Metadata values should be the same between a bundle and its binary.
+  auto binary_product_metadata =
+      platform_delegate_.GetProductMetadata(binary_path_);
+  EXPECT_EQ(binary_product_metadata, product_metadata);
 }
 
 }  // namespace device_signals

components/device_signals/core/system_signals/platform_delegate.cc

@@ -29,6 +29,11 @@ PlatformDelegate::ProductMetadata& PlatformDelegate::ProductMetadata::operator=(
 
 PlatformDelegate::ProductMetadata::~ProductMetadata() = default;
 
+bool PlatformDelegate::ProductMetadata::operator==(
+    const ProductMetadata& other) const {
+  return name == other.name && version == other.version;
+}
+
 absl::optional<PlatformDelegate::ProductMetadata>
 PlatformDelegate::GetProductMetadata(const base::FilePath& file_path) {
   return absl::nullopt;

components/device_signals/core/system_signals/platform_delegate.h

@@ -60,6 +60,8 @@ class PlatformDelegate {
 
     absl::optional<std::string> name = absl::nullopt;
     absl::optional<std::string> version = absl::nullopt;
+
+    bool operator==(const ProductMetadata& other) const;
   };
 
   // Returns product metadata for a given `file_path`.

components/device_signals/core/system_signals/posix/BUILD.gn

@@ -14,3 +14,17 @@ static_library("posix") {
 
   deps = [ "//base" ]
 }
+
+source_set("unit_tests") {
+  testonly = true
+  sources = [ "posix_platform_delegate_unittest.cc" ]
+
+  deps = [
+    ":posix",
+    "//base",
+    "//components/device_signals/core/common",
+    "//components/device_signals/test:test_support",
+    "//testing/gmock",
+    "//testing/gtest",
+  ]
+}