Re: BUG: git-gui no longer executes hook scripts

[Junio C Hamano <gitster@pobox.com>]

Mark Levedahl <mlevedahl@gmail.com> writes:

> The commit titled "Work around Tcl's default |PATH| lookup",|aae9560,
> adds checking on all commands to be executed to assure these are on
> the PATH.

commit aae9560a355d4ab91385e49eae62fade2ddd27ef
Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Date:   Wed Nov 23 09:31:06 2022 +0100

    Work around Tcl's default `PATH` lookup
    
    As per https://www.tcl.tk/man/tcl8.6/TclCmd/exec.html#M23, Tcl's `exec`
    function goes out of its way to imitate the highly dangerous path lookup
    of `cmd.exe`, but _of course_ only on Windows:
    
            If a directory name was not specified as part of the application
            name, the following directories are automatically searched in
            order when attempting to locate the application:

In other words, if somebody tries to run ".git/hooks/pre-commit",
because a directory name _is_ given (i.e. ".git/hooks/" in this case),
the path lookup is *not* done.  Which is what I would expect, and then
"oh, only on Windows to match what cmd.exe does, the current directory
is early in the search order" should not be a problem.

    To avoid that, Git GUI already has the `_which` function that does not
    imitate that dangerous practice when looking up executables in the
    search path.
    
Sounds good, but ...

diff --git a/git-gui.sh b/git-gui.sh
index b0eb5a6ae4..cb92bba1c4 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -121,6 +121,62 @@ proc _which {what args} {
 	return {}
 }
 
+proc sanitize_command_line {command_line from_index} {
+	set i $from_index
+	while {$i < [llength $command_line]} {
+		set cmd [lindex $command_line $i]
+		if {[file pathtype $cmd] ne "absolute"} {
+			set fullpath [_which $cmd]
+			if {$fullpath eq ""} {
+				throw {NOT-FOUND} "$cmd not found in PATH"
+			}
+			lset command_line $i $fullpath

Shouldn't this "is it absolute" check with "$cmd" also check if $cmd
has either forward or backward slash in it?  I do not know about the
Windows cmd.exe convention, but with Unix background, I would be
surprised if dir/cmd gave by end users ran "C:\program
files\dir\cmd" (unless I happened to be in the "C:\program files\"
folder, that is).

Checking the use of _which with fixed arguments, it is used to spawn
git, gitk, nice, sh; and _which finding where they appear on the
search path does sound sane.  But _which does not seem to have the "if
given a command with directory separator, the search path does not
matter.  The caller means it is relative to the $cwd" logic at all,
so it seems it is the callers responsibility to make sure it does
not pass things like ".git/hooks/pre-commit" to it.

+		}
+
+		# handle piped commands, e.g. `exec A | B`
+		for {incr i} {$i < [llength $command_line]} {incr i} {
+			if {[lindex $command_line $i] eq "|"} {
+				incr i
+				break
+			}
+		}
+	}
+	return $command_line
+}
+
+# Override `exec` to avoid unsafe PATH lookup
+
+rename exec real_exec
+
+proc exec {args} {
+	# skip options
+	for {set i 0} {$i < [llength $args]} {incr i} {
+		set arg [lindex $args $i]
+		if {$arg eq "--"} {
+			incr i
+			break
+		}
+		if {[string range $arg 0 0] ne "-"} {
+			break
+		}
+	}
+	set args [sanitize_command_line $args $i]
+	uplevel 1 real_exec $args
+}
+
+# Override `open` to avoid unsafe PATH lookup
+
+rename open real_open
+
+proc open {args} {
+	set arg0 [lindex $args 0]
+	if {[string range $arg0 0 0] eq "|"} {
+		set command_line [string trim [string range $arg0 1 end]]
+		lset args 0 "| [sanitize_command_line $command_line 0]"
+	}
+	uplevel 1 real_open $args
+}
+
 ######################################################################
 ##
 ## locate our library