Dissemble the app_mode_loader.

Previously the app_mode_loader was packaged into a .app bundle. But
because the Info.plist is modified as part of creating app shim
shortcuts, only the executable was code signed. This left the Info.plist
as an un-sealed resource. Under stricter code signing verification rules
required for macOS notarization, an .app bundle cannot have an un-sealed
Info.plist.

To work around this, place the un-bundled executable in the Framework's
Helpers directory and store the Info.plist file under Resources. When
building app shims at runtime, the .app bundle structure will be created
manually (rather than copying the whole pre-built bundle).

Created app shims will still not be validly signed .app bundles, but the
components carried within Chrome will pass validation.

Test: Browser menu > More tools > Create shortcut creates an app
      shortcut that launches.

Bug: 960886, 850199
Change-Id: Ia4521ee436bc3288084004e247fd9765fb93816a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1609886Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: ccameron <ccameron@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#659223}

chrome/BUILD.gn

@@ -904,6 +904,7 @@ if (is_win) {
 
   bundle_data("chrome_framework_helpers") {
     sources = [
+      "$root_out_dir/app_mode_loader",
       "$root_out_dir/chrome_crashpad_handler",
     ]
 
@@ -912,6 +913,7 @@ if (is_win) {
     ]
 
     public_deps = [
+      "//chrome/app_shim:app_mode_loader",
       "//components/crash/content/app:chrome_crashpad_handler",
     ]
 
@@ -924,8 +926,6 @@ if (is_win) {
 
   bundle_data("chrome_framework_resources") {
     sources = [
-      "$root_out_dir/app_mode_loader.app",
-
       # This image is used to badge the lock icon in the
       # authentication dialogs, such as those used for installation
       # from disk image and Keystone promotion (if so enabled).  It
@@ -943,7 +943,7 @@ if (is_win) {
 
     public_deps = [
       ":packed_resources",
-      "//chrome/app_shim:app_mode_loader",
+      "//chrome/app_shim:app_mode_loader_plist_bundle_data",
     ]
 
     if (is_chrome_branded) {

chrome/app_shim/BUILD.gn

@@ -34,7 +34,7 @@ source_set("app_shim") {
   ]
 }
 
-tweak_info_plist("app_mode_loader_plist") {
+tweak_info_plist("tweak_app_mode_loader_plist") {
   info_plist = "app_mode-Info.plist"
   args = [
     "--breakpad=0",
@@ -43,13 +43,30 @@ tweak_info_plist("app_mode_loader_plist") {
   ]
 }
 
-mac_app_bundle("app_mode_loader") {
-  extra_configs = [ "//build/config/compiler:wexit_time_destructors" ]
+mac_info_plist("app_mode_loader_plist") {
+  info_plist_target = ":tweak_app_mode_loader_plist"
+
+  executable_name = "app_mode_loader"
 
-  info_plist_target = ":app_mode_loader_plist"
   extra_substitutions = [
     "APP_MODE_APP_BUNDLE_ID=$chrome_mac_bundle_id.app.@APP_MODE_SHORTCUT_ID@",
   ]
+}
+
+bundle_data("app_mode_loader_plist_bundle_data") {
+  sources = get_target_outputs(":app_mode_loader_plist")
+
+  outputs = [
+    "{{bundle_resources_dir}}/app_mode-Info.plist",
+  ]
+
+  public_deps = [
+    ":app_mode_loader_plist",
+  ]
+}
+
+executable("app_mode_loader") {
+  configs += [ "//build/config/compiler:wexit_time_destructors" ]
 
   sources = [
     "app_mode_loader_mac.mm",

chrome/browser/web_applications/components/web_app_shortcut_mac.h

@@ -124,7 +124,8 @@ class WebAppShortcutCreator {
   std::string GetInternalBundleIdentifier() const;
 
   // Copies the app loader template into a temporary directory and fills in all
-  // relevant information.
+  // relevant information. This works around a Finder bug where the app's icon
+  // doesn't properly update.
   bool BuildShortcut(const base::FilePath& staging_path) const;
 
   // Builds a shortcut and copies it to the specified app paths. Populates

chrome/browser/web_applications/components/web_app_shortcut_mac.mm

@@ -19,6 +19,7 @@
 #include "base/files/file_enumerator.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
+#include "base/mac/bundle_locations.h"
 #include "base/mac/foundation_util.h"
 #import "base/mac/launch_services_util.h"
 #include "base/mac/mac_util.h"
@@ -427,11 +428,6 @@ void LaunchShimOnFileThread(web_app::LaunchShimUpdateBehavior update_behavior,
       base::BindOnce(std::move(launched_callback), base::Process()));
 }
 
-base::FilePath GetAppLoaderPath() {
-  return base::mac::PathForFrameworkBundleResource(
-      base::mac::NSToCFCast(@"app_mode_loader.app"));
-}
-
 base::FilePath GetLocalizableAppShortcutsSubdirName() {
   static const char kChromiumAppDirName[] = "Chromium Apps.localized";
   static const char kChromeAppDirName[] = "Chrome Apps.localized";
@@ -713,11 +709,62 @@ base::FilePath WebAppShortcutCreator::GetFallbackBasename() const {
 
 bool WebAppShortcutCreator::BuildShortcut(
     const base::FilePath& staging_path) const {
-  // Update the app's plist and icon in a temp directory. This works around
-  // a Finder bug where the app's icon doesn't properly update.
-  if (!base::CopyDirectory(GetAppLoaderPath(), staging_path, true)) {
-    LOG(ERROR) << "Copying app to staging path: " << staging_path.value()
-               << " failed.";
+  if (!base::DirectoryExists(staging_path.DirName())) {
+    LOG(ERROR) << "Staging path directory does not exit: "
+               << staging_path.DirName();
+    return false;
+  }
+
+  const base::FilePath framework_bundle_path = base::mac::FrameworkBundlePath();
+
+  const base::FilePath executable_path =
+      framework_bundle_path.Append("Helpers").Append("app_mode_loader");
+  const base::FilePath plist_path =
+      framework_bundle_path.Append("Resources").Append("app_mode-Info.plist");
+
+  const base::FilePath destination_contents_path =
+      staging_path.Append("Contents");
+  const base::FilePath destination_executable_path =
+      destination_contents_path.Append("MacOS");
+
+  // First create the .app bundle directory structure.
+  // Use NSFileManager so that the permissions can be set appropriately. The
+  // base::CreateDirectory() routine forces mode 0700.
+  NSError* error = nil;
+  if (![[NSFileManager defaultManager]
+                 createDirectoryAtURL:base::mac::FilePathToNSURL(
+                                          destination_executable_path)
+          withIntermediateDirectories:YES
+                           attributes:@{
+                             NSFilePosixPermissions : @(0755)
+                           }
+                                error:&error]) {
+    LOG(ERROR) << "Failed to create destination executable path: "
+               << destination_executable_path
+               << ", error=" << base::SysNSStringToUTF8([error description]);
+    return false;
+  }
+
+  // Copy the executable file.
+  if (!base::CopyFile(executable_path, destination_executable_path.Append(
+                                           executable_path.BaseName()))) {
+    LOG(ERROR) << "Failed to copy executable: " << executable_path;
+    return false;
+  }
+
+  // Copy the Info.plist.
+  if (!base::CopyFile(plist_path,
+                      destination_contents_path.Append("Info.plist"))) {
+    LOG(ERROR) << "Failed to copy plist: " << plist_path;
+    return false;
+  }
+
+  // Write the PkgInfo file.
+  constexpr char kPkgInfoData[] = "APPL????";
+  constexpr size_t kPkgInfoDataSize = base::size(kPkgInfoData) - 1;
+  if (base::WriteFile(destination_contents_path.Append("PkgInfo"), kPkgInfoData,
+                      kPkgInfoDataSize) != kPkgInfoDataSize) {
+    LOG(ERROR) << "Failed to write PkgInfo file: " << destination_contents_path;
     return false;
   }
 

chrome/installer/mac/signing/signing.py

@@ -75,8 +75,7 @@ def get_parts(config):
                 verify_options=VerifyOptions.DEEP),
         'app-mode-app':
             CodeSignedProduct(
-                '{.framework_dir}/Resources/app_mode_loader.app/Contents/MacOS/app_mode_loader'
-                .format(config),
+                '{.framework_dir}/Helpers/app_mode_loader'.format(config),
                 'app_mode_loader',
                 options=config.codesign_options_helpers,
                 verify_options=VerifyOptions.IGNORE_RESOURCES),
@@ -224,25 +223,10 @@ def sign_chrome(paths, config):
     # signing the entire framework is equivalent to signing the Current version.
     # https://developer.apple.com/library/content/technotes/tn2206/_index.html#//apple_ref/doc/uid/DTS40007919-CH1-TNTAG13
     for name, part in parts.items():
-        if name in ('app', 'framework', 'app-mode-app'):
+        if name in ('app', 'framework'):
             continue
         sign_part(paths, config, part)
 
-    # The app mode loader bundle is modified dynamically at runtime. Just sign
-    # the executable, which shouldn't change. In order to do this, the
-    # executable needs to be moved out of the bundle, signed, and then moved
-    # back in. The resulting bundle's signature won't validate normally, but if
-    # the executable file is verified in isolation or with --ignore-resources,
-    # it will.
-    app_mode_app = copy.copy(parts['app-mode-app'])
-    orig_app_mode_path = os.path.join(paths.work, app_mode_app.path)
-    temp_app_mode_path = os.path.join(paths.work,
-                                      os.path.basename(app_mode_app.path))
-    commands.move_file(orig_app_mode_path, temp_app_mode_path)
-    app_mode_app.path = temp_app_mode_path
-    sign_part(paths, config, app_mode_app)
-    commands.move_file(temp_app_mode_path, orig_app_mode_path)
-
     # Sign the framework bundle.
     sign_part(paths, config, parts['framework'])
 

chrome/installer/mac/signing/signing_test.py

@@ -223,30 +223,8 @@ class TestSignChrome(unittest.TestCase):
 
         signing.sign_chrome(self.paths, config)
 
-        # Test that the two file moves are for app_mode_loader.
-        self.assertEqual(kwargs['move_file'].mock_calls, [
-            mock.call.move_file(
-                '$W/App Product.app/Contents/Versions/99.0.9999.99/Product Framework.framework/Resources/app_mode_loader.app/Contents/MacOS/app_mode_loader',
-                '$W/app_mode_loader'),
-            mock.call.move_file(
-                '$W/app_mode_loader',
-                '$W/App Product.app/Contents/Versions/99.0.9999.99/Product Framework.framework/Resources/app_mode_loader.app/Contents/MacOS/app_mode_loader'
-            ),
-        ])
-
-        # Find the signing step between the two moves.
-        moved_app_loader_index = manager.mock_calls.index(
-            mock.call.move_file(*kwargs['move_file'].mock_calls[0][1]))
-        sign_app_mode = manager.mock_calls[moved_app_loader_index + 1]
-        self.assertEqual('$W/app_mode_loader', sign_app_mode[1][2].path)
-
-        # Make sure app_mode_loader is only signed that once.
-        other_sign_app_modes = [
-            call for call in kwargs['sign_part'].mock_calls
-            if call[1][2].identifier == 'app_mode_loader'
-        ]
-        self.assertEqual(1, len(other_sign_app_modes))
-        self.assertEqual(sign_app_mode[1], other_sign_app_modes[0][1])
+        # No files should be moved.
+        self.assertEqual(0, kwargs['move_file'].call_count)
 
         # Test that the provisioning profile is copied.
         self.assertEqual(kwargs['copy_files'].mock_calls, [
@@ -255,11 +233,17 @@ class TestSignChrome(unittest.TestCase):
                 '$W/App Product.app/Contents/embedded.provisionprofile')
         ])
 
+        # Ensure that all the parts are signed.
+        signed_paths = [
+            call[1][2].path for call in kwargs['sign_part'].mock_calls
+        ]
+        self.assertEqual(
+            set([p.path for p in signing.get_parts(config).values()]),
+            set(signed_paths))
+
         # Make sure that the framework and the app are the last two parts that
         # are signed.
-        last_two_sign_parts = kwargs['sign_part'].mock_calls[-2:]
-        last_two_part_paths = [call[1][2].path for call in last_two_sign_parts]
-        self.assertEqual(last_two_part_paths, [
+        self.assertEqual(signed_paths[-2:], [
             'App Product.app/Contents/Versions/99.0.9999.99/Product Framework.framework',
             'App Product.app'
         ])