Merge pull request #396 from charlespierce/third_party_executable

Ensure package binary files are executable on Unix

crates/notion-core/src/distro/package.rs

@@ -38,6 +38,8 @@ cfg_if! {
         use cmdline_words_parser::StrExt;
         use regex::Regex;
         use std::io::{BufRead, BufReader};
+    } else if #[cfg(unix)] {
+        use std::os::unix::fs::PermissionsExt;
     }
 }
 
@@ -413,7 +415,8 @@ impl PackageVersion {
     fn write_config_and_shims(&self, platform_spec: &PlatformSpec) -> Fallible<()> {
         self.package_config(&platform_spec).to_serial().write()?;
         for (bin_name, bin_path) in self.bins.iter() {
-            let loader = self.determine_script_loader(bin_name, bin_path)?;
+            let full_path = bin_full_path(&self.name, &self.version, bin_name, bin_path)?;
+            let loader = determine_script_loader(bin_name, &full_path)?;
             self.bin_config(
                 bin_name.to_string(),
                 bin_path.to_string(),
@@ -424,55 +427,16 @@ impl PackageVersion {
             .write()?;
             // create a link to the shim executable
             shim::create(&bin_name)?;
-        }
-        Ok(())
-    }
-
-    /// On Unix, shebang loaders work correctly, so we don't need to bother storing loader information
-    #[cfg(unix)]
-    fn determine_script_loader(
-        &self,
-        _bin_name: &str,
-        _bin_path: &str,
-    ) -> Fallible<Option<BinLoader>> {
-        Ok(None)
-    }
 
-    /// On Windows, we need to read the executable and try to find a shebang loader
-    /// If it exists, we store the loader in the BinConfig so that the shim can execute it correctly
-    #[cfg(windows)]
-    fn determine_script_loader(
-        &self,
-        bin_name: &str,
-        bin_path: &str,
-    ) -> Fallible<Option<BinLoader>> {
-        let full_path = bin_full_path(&self.name, &self.version, bin_path, |_| {
-            ErrorDetails::DetermineBinaryLoaderError {
-                bin: bin_name.to_string(),
-            }
-        })?;
-        let script =
-            File::open(full_path).with_context(|_| ErrorDetails::DetermineBinaryLoaderError {
-                bin: bin_name.to_string(),
+            // On Unix, ensure the executable file has correct permissions
+            #[cfg(unix)]
+            set_executable_permissions(&full_path).with_context(|_| {
+                ErrorDetails::ExecutablePermissionsError {
+                    bin: bin_name.clone(),
+                }
             })?;
-        if let Some(Ok(first_line)) = BufReader::new(script).lines().next() {
-            // Note: Regex adapted from @zkochan/cmd-shim package used by Yarn
-            // https://github.com/pnpm/cmd-shim/blob/bac160cc554e5157e4c5f5e595af30740be3519a/index.js#L42
-            let re = Regex::new(r#"^#!\s*(?:/usr/bin/env)?\s*(?P<exe>[^ \t]+) ?(?P<args>.*)$"#)
-                .expect("Regex is valid");
-            if let Some(caps) = re.captures(&first_line) {
-                let args = caps["args"]
-                    .to_string()
-                    .parse_cmdline_words()
-                    .map(|word| word.to_string())
-                    .collect();
-                return Ok(Some(BinLoader {
-                    command: caps["exe"].to_string(),
-                    args,
-                }));
-            }
         }
-        Ok(None)
+        Ok(())
     }
 
     /// Uninstall the specified package.
@@ -583,10 +547,8 @@ impl UserTool {
         let bin_path = bin_full_path(
             &bin_config.package,
             &bin_config.version,
+            &bin_config.name,
             &bin_config.path,
-            |_| ErrorDetails::ExecutablePathError {
-                command: bin_config.name.clone(),
-            },
         )?;
 
         // If the user does not have yarn set in the platform for this binary, use the default
@@ -622,21 +584,71 @@ impl UserTool {
     }
 }
 
-fn bin_full_path<P, E>(
+fn bin_full_path<P>(
     package: &str,
     version: &Version,
+    bin_name: &str,
     bin_path: P,
-    error_context: E,
 ) -> Fallible<PathBuf>
 where
     P: AsRef<Path>,
-    E: FnOnce(&io::Error) -> ErrorDetails,
 {
     // canonicalize because path is relative, and sometimes uses '.' char
     path::package_image_dir(package, &version.to_string())?
         .join(bin_path)
         .canonicalize()
-        .with_context(error_context)
+        .with_context(|_| ErrorDetails::ExecutablePathError {
+            command: bin_name.to_string(),
+        })
+}
+
+/// Ensure that a given binary has 'executable' permissions on Unix, otherwise we won't be able to call it
+/// On Windows, this isn't a concern as there is no concept of 'executable' permissions
+#[cfg(unix)]
+fn set_executable_permissions(bin: &Path) -> io::Result<()> {
+    let mut permissions = fs::metadata(bin)?.permissions();
+    let mode = permissions.mode();
+
+    if mode & 0o111 != 0o111 {
+        permissions.set_mode(mode | 0o111);
+        fs::set_permissions(bin, permissions)
+    } else {
+        Ok(())
+    }
+}
+
+/// On Unix, shebang loaders work correctly, so we don't need to bother storing loader information
+#[cfg(unix)]
+fn determine_script_loader(_bin_name: &str, _full_path: &Path) -> Fallible<Option<BinLoader>> {
+    Ok(None)
+}
+
+/// On Windows, we need to read the executable and try to find a shebang loader
+/// If it exists, we store the loader in the BinConfig so that the shim can execute it correctly
+#[cfg(windows)]
+fn determine_script_loader(bin_name: &str, full_path: &Path) -> Fallible<Option<BinLoader>> {
+    let script =
+        File::open(full_path).with_context(|_| ErrorDetails::DetermineBinaryLoaderError {
+            bin: bin_name.to_string(),
+        })?;
+    if let Some(Ok(first_line)) = BufReader::new(script).lines().next() {
+        // Note: Regex adapted from @zkochan/cmd-shim package used by Yarn
+        // https://github.com/pnpm/cmd-shim/blob/bac160cc554e5157e4c5f5e595af30740be3519a/index.js#L42
+        let re = Regex::new(r#"^#!\s*(?:/usr/bin/env)?\s*(?P<exe>[^ \t]+) ?(?P<args>.*)$"#)
+            .expect("Regex is valid");
+        if let Some(caps) = re.captures(&first_line) {
+            let args = caps["args"]
+                .to_string()
+                .parse_cmdline_words()
+                .map(|word| word.to_string())
+                .collect();
+            return Ok(Some(BinLoader {
+                command: caps["exe"].to_string(),
+                args,
+            }));
+        }
+    }
+    Ok(None)
 }
 
 /// Build a package install command using the specified directory and path

crates/notion-core/src/error/details.rs

@@ -104,6 +104,11 @@ pub enum ErrorDetails {
         command: String,
     },
 
+    /// Thrown when verifying the file permissions on an executable fails
+    ExecutablePermissionsError {
+        bin: String,
+    },
+
     /// Thrown when executing a hook command fails
     ExecuteHookError {
         command: String,
@@ -495,12 +500,15 @@ from {}
 Please verify your internet connection and ensure the correct version is specified.",
                 tool, from_url
             ),
-            ErrorDetails::ExecuteHookError { command } => write!(f, "Could not execute hook command: '{}'
-
-Please ensure that the corrent command is specified.", command),
             ErrorDetails::ExecutablePathError { command } => write!(f, "Could not determine path to executable '{}'
 
 {}", command, REPORT_BUG_CTA),
+            ErrorDetails::ExecutablePermissionsError { bin } => write!(f, "Could not verify permissions for executable '{}'
+
+{}", bin, PERMISSIONS_CTA),
+            ErrorDetails::ExecuteHookError { command } => write!(f, "Could not execute hook command: '{}'
+
+Please ensure that the correct command is specified.", command),
             ErrorDetails::HookMultipleFieldsSpecified => write!(f, "Hook configuration includes multiple hook types.
 
 Please include only one of 'bin', 'prefix', or 'template'"),
@@ -835,6 +843,7 @@ impl NotionFail for ErrorDetails {
             ErrorDetails::DetermineBinaryLoaderError { .. } => ExitCode::FileSystemError,
             ErrorDetails::DownloadToolNetworkError { .. } => ExitCode::NetworkError,
             ErrorDetails::ExecutablePathError { .. } => ExitCode::UnknownError,
+            ErrorDetails::ExecutablePermissionsError { .. } => ExitCode::FileSystemError,
             ErrorDetails::ExecuteHookError { .. } => ExitCode::ExecutionFailure,
             ErrorDetails::HookMultipleFieldsSpecified => ExitCode::ConfigurationError,
             ErrorDetails::HookNoFieldsSpecified => ExitCode::ConfigurationError,