diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml index 636a4b8..01ecc71 100644 --- a/.github/workflows/push.yml +++ b/.github/workflows/push.yml @@ -17,13 +17,11 @@ jobs: - name: Build and test with integration coverage run: | go build -cover -v - mkdir -p coverage - GOCOVERDIR=$(pwd)/coverage go test ./... -v -count=1 - go tool covdata textfmt -i=coverage -o coverage-integration.txt + mkdir -p coverage/integration + GOCOVERDIR="$(pwd)/coverage/integration" go test ./... -v -count=1 - uses: actions/upload-artifact@v3 with: - name: coverage - path: coverage-integration.txt + path: coverage test-cover-build: runs-on: ubuntu-latest steps: @@ -34,26 +32,21 @@ jobs: - name: Build and test with unit coverage run: | go build -v - go test ./... -v -count=1 -coverprofile=coverage-unit.txt + mkdir -p coverage/unit + go test ./... -v -count=1 -cover -args -test.gocoverdir="$PWD/coverage/unit" - uses: actions/upload-artifact@v3 with: - name: coverage - path: coverage-unit.txt + path: coverage coverage: runs-on: ubuntu-latest needs: [test-cover-integration, test-cover-build] steps: + - uses: actions/checkout@v3 - uses: actions/download-artifact@v3 - with: - name: coverage - name: Process coverage - # Coverage is simple text files, so we can combine the integration - # and unit test coverage by simply appending the latter with the - # first line skipped. run: | - cp coverage-integration.txt coverage.txt - tail -n +2 coverage-unit.txt >> coverage.txt + go tool covdata textfmt -i=./artifact/unit,./artifact/integration -o=coverage.txt - name: Upload coverage reports to Codecov uses: codecov/codecov-action@v3 with: - files: ./coverage.txt + file: ./coverage.txt diff --git a/hooks/pre_commit.go b/hooks/pre_commit.go index e2f1ea7..458e3c1 100644 --- a/hooks/pre_commit.go +++ b/hooks/pre_commit.go @@ -1,8 +1,10 @@ package hooks import ( + _ "embed" "fmt" "os" + "os/exec" "path" "strings" @@ -13,6 +15,9 @@ import ( "github.com/dirk/quickhook/tracing" ) +//go:embed pre_commit_git_shim.sh +var PRE_COMMIT_GIT_SHIM string + const PRE_COMMIT_HOOK = "pre-commit" const PRE_COMMIT_MUTATING_HOOK = "pre-commit-mutating" @@ -95,6 +100,13 @@ func (hook *PreCommit) checkResult(result hookResult) bool { } func shimGit() (string, error) { + actualGit, err := exec.LookPath("git") + if err != nil { + return "", err + } + // Trusting that we didn't get a malicious path back from LookPath(). + templated := strings.Replace(PRE_COMMIT_GIT_SHIM, "ACTUAL_GIT", actualGit, 1) + span := tracing.NewSpan("shim-git") defer span.End() @@ -104,12 +116,7 @@ func shimGit() (string, error) { } git := path.Join(dir, "git") - err = os.WriteFile(git, []byte(strings.Join([]string{ - "#!/bin/sh", - "echo \"git is not allowed in parallel hooks (git $@)\"", - "exit 1", - "", - }, "\n")), 0755) + err = os.WriteFile(git, []byte(templated), 0755) if err != nil { return "", err } diff --git a/hooks/pre_commit_git_shim.sh b/hooks/pre_commit_git_shim.sh new file mode 100644 index 0000000..ac8dae8 --- /dev/null +++ b/hooks/pre_commit_git_shim.sh @@ -0,0 +1,19 @@ +#!/bin/sh +set -e +COMMAND=$1 +shift +if + [ "$COMMAND" = "diff" ] || + [ "$COMMAND" = "ls-files" ] || + [ "$COMMAND" = "rev-list" ] || + [ "$COMMAND" = "rev-parse" ] || + [ "$COMMAND" = "show" ] || + [ "$COMMAND" = "status" ]; +then + # The Git executable below will be replaced at runtime when shimming. + ACTUAL_GIT "$COMMAND" "$@" + exit 0 +fi +COMBINED=$(echo "$COMMAND $*" | xargs) +echo "git is not allowed in parallel hooks (git $COMBINED)" +exit 1 diff --git a/hooks/pre_commit_test.go b/hooks/pre_commit_test.go index 80b81c6..02c31cf 100644 --- a/hooks/pre_commit_test.go +++ b/hooks/pre_commit_test.go @@ -3,6 +3,8 @@ package hooks import ( "bytes" "io" + "sort" + "strings" "testing" "github.com/creack/pty" @@ -127,14 +129,34 @@ func TestHandlesDeletedFiles(t *testing.T) { assert.Equal(t, "", output) } -func TestShimsGitToDenyAccess(t *testing.T) { +func TestGitShimAllowsReadonlyAccess(t *testing.T) { tempDir := initGitForPreCommit(t) tempDir.MkdirAll(".quickhook", "pre-commit") tempDir.WriteFile([]string{".quickhook", "pre-commit", "accesses-git"}, "#!/bin/sh \n git status") + output, err := tempDir.ExecQuickhook("hook", "pre-commit") + assert.Nil(t, err) + assert.Empty(t, output) +} + +func TestGitShimDeniesOtherAccess(t *testing.T) { + tempDir := initGitForPreCommit(t) + tempDir.MkdirAll(".quickhook", "pre-commit") + tempDir.WriteFile([]string{".quickhook", "pre-commit", "reset0"}, "#!/bin/sh \n git reset") + tempDir.WriteFile([]string{".quickhook", "pre-commit", "reset1"}, "#!/bin/sh \n git reset --hard") + output, err := tempDir.ExecQuickhook("hook", "pre-commit") assert.Error(t, err) - assert.Equal(t, "accesses-git: git is not allowed in parallel hooks (git status)\n", output) + lines := strings.Split(strings.TrimSpace(output), "\n") + sort.Strings(lines) + assert.Equal( + t, + []string{ + "reset0: git is not allowed in parallel hooks (git reset)", + "reset1: git is not allowed in parallel hooks (git reset --hard)", + }, + lines, + ) } func TestMutatingCanAccessGit(t *testing.T) {