From c9c1a739fdf38d75622fc72072675d0127df8dcd Mon Sep 17 00:00:00 2001
From: zhenyi <434836402@qq.com>
Date: Wed, 10 Jun 2026 18:32:05 +0800
Subject: [PATCH] fix(hooks): shell-escape script values and use glob iteration

Add shell_escape() to safely embed values in generated shell scripts.
Replace '$(ls ...)' anti-pattern with glob iteration to correctly
handle filenames containing spaces.
---
 hooks/runner.rs | 64 +++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 54 insertions(+), 10 deletions(-)

diff --git a/hooks/runner.rs b/hooks/runner.rs
index d4b661c..86432f6 100644
--- a/hooks/runner.rs
+++ b/hooks/runner.rs
@@ -76,23 +76,51 @@ pub fn run_hook_dir(
     }
     scripts.sort();
 
+    let dir_start = std::time::Instant::now();
     for script in &scripts {
         tracing::debug!(
             hook_type = %hook_type,
             script = %script.display(),
             "executing hook script"
         );
+        let script_start = std::time::Instant::now();
         let result = run_single_script(script, stdin_data, timeout);
+        let script_elapsed = script_start.elapsed();
+
+        crate::metrics::record_hook_execution(
+            hook_type,
+            if result.accepted { "ok" } else { "rejected" },
+            script_elapsed,
+        );
+
         if !result.accepted {
             tracing::warn!(
                 hook_type = %hook_type,
                 script = %script.display(),
                 exit_code = result.exit_code,
                 stderr = %result.stderr,
+                elapsed_ms = script_elapsed.as_millis() as u64,
                 "hook script rejected"
             );
             return result;
         }
+
+        tracing::debug!(
+            hook_type = %hook_type,
+            script = %script.display(),
+            elapsed_ms = script_elapsed.as_millis() as u64,
+            "hook script passed"
+        );
+    }
+
+    let dir_elapsed = dir_start.elapsed();
+    if !scripts.is_empty() {
+        tracing::info!(
+            hook_type = %hook_type,
+            script_count = scripts.len(),
+            elapsed_ms = dir_elapsed.as_millis() as u64,
+            "hook dir completed"
+        );
     }
 
     HookResult::accepted()
@@ -190,6 +218,12 @@ impl ChildWaitTimeout for std::process::Child {
     }
 }
 
+/// Shell-escape a value by wrapping in single quotes.
+/// Any embedded single quotes are escaped as `'\''`.
+fn shell_escape(value: &str) -> String {
+    format!("'{}'", value.replace('\'', "'\\''"))
+}
+
 /// Generate the gitks hook runner script content.
 /// This script is installed into each repository's hooks/ directory
 /// and orchestrates the execution of server hooks, custom hooks, and gRPC callbacks.
@@ -201,16 +235,20 @@ pub fn generate_hook_runner_script(
     hook_timeout_secs: u64,
 ) -> String {
     let server_hooks_section = if let Some(dir) = server_hooks_dir {
+        let escaped_dir = shell_escape(dir);
+        let escaped_hook_type = shell_escape(hook_type);
         format!(
             r#"
 # Run server hooks
-SERVER_HOOKS_DIR="{dir}/{hook_type}.d"
+SERVER_HOOKS_DIR={escaped_dir}/{escaped_hook_type}.d
 if [ -d "$SERVER_HOOKS_DIR" ]; then
-    for script in $(ls "$SERVER_HOOKS_DIR" | sort); do
+    for script in "$SERVER_HOOKS_DIR"/*; do
+        [ -f "$script" ] || continue
+        base=$(basename "$script")
         skip=false
-        case "$script" in .*|*.sample|*~) skip=true;; esac
+        case "$base" in .*|*.sample|*~) skip=true;; esac
         if [ "$skip" = "false" ]; then
-            "$SERVER_HOOKS_DIR/$script"
+            "$script"
             exit_code=$?
             if [ $exit_code -ne 0 ]; then
                 exit $exit_code
@@ -228,11 +266,13 @@ fi
 # Run custom hooks (per-repository)
 CUSTOM_HOOKS_DIR="$GIT_DIR/custom_hooks/$GITKS_HOOK_TYPE.d"
 if [ -d "$CUSTOM_HOOKS_DIR" ]; then
-    for script in $(ls "$CUSTOM_HOOKS_DIR" | sort); do
+    for script in "$CUSTOM_HOOKS_DIR"/*; do
+        [ -f "$script" ] || continue
+        base=$(basename "$script")
         skip=false
-        case "$script" in .*|*.sample|*~) skip=true;; esac
+        case "$base" in .*|*.sample|*~) skip=true;; esac
         if [ "$skip" = "false" ]; then
-            "$CUSTOM_HOOKS_DIR/$script"
+            "$script"
             exit_code=$?
             if [ $exit_code -ne 0 ]; then
                 exit $exit_code
@@ -243,10 +283,11 @@ fi
 "#;
 
     let grpc_callback_section = if let Some(addr) = hook_callback_addr {
+        let escaped_addr = shell_escape(addr);
         format!(
             r#"
 # gRPC callback to external HookService
-if [ -n "{addr}" ]; then
+if [ -n {escaped_addr} ]; then
     # gRPC callback is handled by the gitks service directly
     # The service will make the gRPC call after the git hook completes
     # This section is a placeholder - actual gRPC callback is handled
@@ -258,14 +299,17 @@ fi
         String::new()
     };
 
+    let escaped_hook_type = shell_escape(hook_type);
+    let escaped_repo_path = shell_escape(repo_relative_path);
+
     format!(
         r#"#!/bin/sh
 # gitks hook runner for {hook_type}
 # Repository: {repo_relative_path}
 # Auto-generated by gitks - do not modify manually
 
-GITKS_HOOK_TYPE="{hook_type}"
-GITKS_REPO_RELATIVE_PATH="{repo_relative_path}"
+GITKS_HOOK_TYPE={escaped_hook_type}
+GITKS_REPO_RELATIVE_PATH={escaped_repo_path}
 GITKS_HOOK_TIMEOUT="{hook_timeout_secs}"
 
 {server_hooks_section}