Re: [PATCH] gitk: add user-defined custom commands
- From
- Tim Wiederhake <twied@gmx.net>
- Date
- Aug 10, 2026, 19:28 UTC
- Message-ID
- <f3b73531581a6f94410d863339c5683ae8d63e0d.camel@gmx.net>
- In-Reply-To
- <xmqq7bm1d1au.fsf@gitster.g>
On Fri, 2026-08-07 at 15:16 -0700, Junio C Hamano wrote:
Show 57 quoted lines
> "Tim Wiederhake via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > + set len [string length $cmd_template]
> > + for {set i 0} {$i < $len} {incr i} {
> > + if {[string index $cmd_template $i] eq "%" && $i + 1 <
> > $len} {
> > + set next [string index $cmd_template [expr {$i + 1}]]
> > + if {!$blame_computed && ($next eq "b" || $next eq
> > "l")} {
> > + set blame [get_blame_origin]
> > + set blame_id [lindex $blame 0]
> > + set blame_line [lindex $blame 1]
> > + set blame_computed 1
> > + }
> > + switch -- $next {
> > + "%" { append cmd "%" }
> > + "i" { append cmd $id }
> > + "t" { append cmd [lindex $commitinfo($id) 0] }
> > + "a" { append cmd [lindex $commitinfo($id) 1] }
> > + "d" { append cmd [lindex $commitinfo($id) 2] }
> > + "c" { append cmd [lindex $commitinfo($id) 3] }
> > + "D" { append cmd [lindex $commitinfo($id) 4] }
> > + "m" { append cmd [lindex $commitinfo($id) 5] }
> > + "M" { if {[info exists markedid]} { append cmd
> > $markedid } }
> > + "b" { append cmd $blame_id }
> > + "f" { append cmd [get_diff_file] }
> > + "l" { append cmd $blame_line }
> > + default { append cmd "%" $next }
> > + }
> > + incr i
> > + } else {
> > + append cmd [string index $cmd_template $i]
> > + }
> > + }
> > +
> > + if {[catch {exec sh -c $cmd 2>@1} output]} {
>
> What do various members of $commitinfo field have? I presume that
> title and message are pretty much free text under control of anybody
> who can write to the repository and entice you to run this command,
> so running with "sh -c $cmd" would require $cmd to be quoting the
> payload properly, or you'd be opening yourself to be an arbitrary
> command execution, no? With template "echo '%t'" you thought you
> are just printing the title but if the title has "title?'; echo no'"
> in
> it, wouldn't cmd end up being
>
> echo 'title?'; echo no''
>
> and a more creative type can use something other than "echo no", to
> have a process run under your name and do more interesting things,
> right?
>
> Note that I no longer speak Tcl (even though I admit I used to), so
> if there is some "magic" that makes use of $cmd in {exec sh -c $cmd}
> safe, the above may be missing the mark by a mile.You are right, the code is vulnerable to shell injection. The first version was calling the command directly, so no escaping was necessary.
I added the "sh -c" to facilitate process forking ("&") and simple exit code manipulation ("command && exit 42"). But by now I honestly am not sure anymore on what the best solution is: calling the command directly and have the user write a wrapper script if necessary; or add code to properly escape all data read from commits and retain the convenience.
Opinions?
Tim