fix: quote paths in the generated hook shim - #1509
Conversation
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; both previously reported regressions are fixed by limiting escaping to the auto-detected executable path. Reviews (2): Last reviewed commit: "fix: quote the auto-detected executable ..." | Re-trigger Greptile |
| elif test -n {{ shellescape .LefthookPath }} | ||
| then | ||
| {{ .LefthookPath }} "$@" | ||
| {{ shellescape .LefthookPath }} "$@" |
There was a problem hiding this comment.
| {{- if .Rc}} | ||
| {{/* Load rc file, which may export ENV variables */}} | ||
| [ -f {{.Rc}} ] && . {{.Rc}} | ||
| [ -f {{ shellescape .Rc }} ] && . {{ shellescape .Rc }} |
There was a problem hiding this comment.
When lefthook installs a hook it bakes the path to the running binary (os.Executable) into the generated shim. That path was interpolated unquoted, so a binary living under a directory with a space (common on macOS: iCloud Drive, Google Drive, any folder with a space) was word-split by the generated sh, every branch of call_lefthook failed, and the shim printed "Can't find lefthook in PATH". Because that branch exits 0, the commit still succeeded, so the hook was silently and permanently skipped with no signal. Escape that path with POSIX single quotes via a shellescape template function. The user cannot quote it themselves since it is detected at install time. The `lefthook` and `rc` config values are intentionally left unquoted: `lefthook` is documented as an executable path or command (for example `bundle exec lefthook`), and `rc` is documented with tilde and environment-variable expansion (for example `~/.lefthookrc`). Both rely on the shell interpreting them, and the docs make quoting a spaced path there the user's responsibility. Fixes evilmartians#1488
33bbc87 to
0b511ef
Compare
|
Good catch. The first revision single-quoted all three interpolated values, which was wrong: I narrowed the fix to only |
Closes #1488
Context
When
lefthook installgenerates a hook, it bakes the path to the running binary (os.Executable(), the.LefthookPathCurrenttemplate value) into the shim. That path was interpolated unquoted, so a binary living under a directory with a space (common on macOS: iCloud Drive, Google Drive, any folder with a space in its name) is word-split by the generatedsh, every branch ofcall_lefthook()fails, and the shim printsCan't find lefthook in PATH. Because that terminal branch exits0(unlessAssertLefthookInstalledis set), the commit still succeeds, so the hook is silently and permanently skipped with no signal. Unlike the config values, this path is detected at install time, so the user cannot quote it themselves.Reproduced with the issue's steps (a lefthook binary living under
/tmp/my project/bin):elif /tmp/my project/bin/lefthook -h ...; committing printsCan't find lefthook in PATHand succeeds with the hook never running.elif '/tmp/my project/bin/lefthook' -h ...; committing runs the hook.Changes
shellescapetemplate function (POSIX single quotes,'\''escaping) and applied it to the two.LefthookPathCurrentinterpolations inhook.tmpl.lefthookandrcconfig values unquoted on purpose:lefthookis documented as an executable path or command (e.g.bundle exec lefthook,LEFTHOOK_VERBOSE=1 lefthook), andrcis documented with tilde and environment-variable expansion (e.g.~/.lefthookrc,"${XDG_CONFIG_HOME:-$HOME/.config}/lefthookrc"). Those rely on the shell interpreting them, and the docs make quoting a spaced path there the user's responsibility. Escaping them would turn commands into nonexistent filenames and stop documented expansions from resolving.internal/templates/templates_test.go: table-driven tests that the auto-detected path is single-quoted, and that a configured command, an env-prefixed command, and tilde/env-expansion rc paths are passed through unquoted.Verified locally:
go test ./internal/...andgolangci-lint run(pinned v2.11.4) pass. No config structs changed, soschema.jsonis untouched.