Repository: adnanh/webhook (go)

Issue:
Log exec.LookPath errors
The error returned by `exec.LookPath` is never surfaced to the user.  Without that detail, the user can't tell the difference between a non-existent path and a permissions issue.  We should log the underlying error message.

**However**, the existing logic tries to lookup the `h.ExecuteCommand` and, upon failure, attempts `filepath.Join(h.CommandWorkingDirectory, h.ExecuteCommand)`.  It can be hard to tell which one the user intended to execute and which error message to show.  We need to update this logic to be more predictable.

My a proposed replacement logic is below.  The primary difference is that if the ExecCmd is an absolute path, webhook won't try prepending the CWD.  This may break some setups, but it makes path lookups more explicit and consistent.  Thoughts, @adnanh?

```go
var lookpath string
if filepath.IsAbs(h.ExecuteCommand) || h.CommandWorkingDirectory == "" {
    lookpath = h.ExecuteCommand
} else {
    lookpath = filepath.Join(h.CommandWorkingDirectory, h.ExecuteCommand)
}

cmdPath, err := exec.LookPath(lookpath)
if err != nil {
    log.Printf("[%s] error in %s", rid, err)
// ...
```

Related to #456

Pull request description:
Log stdlib error on failed exec.LookPath
The error returned by `exec.LookPath` was never surfaced to the user.
Without that detail, the user can't tell the difference between a
non-existent path and a permissions issue.

Additionally, when `ExecuteCommand` is an absolute path, we were still
attempting to prepend the `CommandWorkingDirectory` if the `ExecuteCommand`
was not found, which made it difficult to know which path the user
intended to execute.

This commit simplifies the logic to avoid multiple attempts with
`ExecuteCommand` is an absolute path and changes the error message from:

    error locating command: '/path/to/file'

to:

    error in exec: "/path/to/file": stat /path/to/file: no such file or directory
    error in exec: "/path/to/file": permission denied

Fixes #457

Patch:
diff --git a/webhook.go b/webhook.go
index 6a258c4..26872ed 100644
--- a/webhook.go
+++ b/webhook.go
@@ -581,16 +581,16 @@ func handleHook(h *hook.Hook, rid string, headers, query, payload *map[string]in
 	var errors []error
 
 	// check the command exists
-	cmdPath, err := exec.LookPath(h.ExecuteCommand)
-	if err != nil {
-		// give a last chance, maybe is a relative path
-		relativeToCwd := filepath.Join(h.CommandWorkingDirectory, h.ExecuteCommand)
-		// check the command exists
-		cmdPath, err = exec.LookPath(relativeToCwd)
+	var lookpath string
+	if filepath.IsAbs(h.ExecuteCommand) || h.CommandWorkingDirectory == "" {
+		lookpath = h.ExecuteCommand
+	} else {
+		lookpath = filepath.Join(h.CommandWorkingDirectory, h.ExecuteCommand)
 	}
 
+	cmdPath, err := exec.LookPath(lookpath)
 	if err != nil {
-		log.Printf("[%s] error locating command: '%s'", rid, h.ExecuteCommand)
+		log.Printf("[%s] error in %s", rid, err)
 
 		// check if parameters specified in execute-command by mistake
 		if strings.IndexByte(h.ExecuteCommand, ' ') != -1 {


Test patch:
diff --git a/webhook_test.go b/webhook_test.go
index 4671c24..353d22d 100644
--- a/webhook_test.go
+++ b/webhook_test.go
@@ -720,7 +720,7 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00
 
 	// Check logs
 	{"static params should pass", "static-params-ok", nil, "POST", nil, "application/json", `{}`, http.StatusOK, "arg: passed\n", `(?s)command output: arg: passed`},
-	{"command with space logs warning", "warn-on-space", nil, "POST", nil, "application/json", `{}`, http.StatusInternalServerError, "Error occurred while executing the hook's command. Please check your logs for more details.", `(?s)error locating command.*use 'pass[-]arguments[-]to[-]command' to specify args`},
+	{"command with space logs warning", "warn-on-space", nil, "POST", nil, "application/json", `{}`, http.StatusInternalServerError, "Error occurred while executing the hook's command. Please check your logs for more details.", `(?s)error in exec:.*use 'pass[-]arguments[-]to[-]command' to specify args`},
 	{"unsupported content type error", "github", nil, "POST", map[string]string{"Content-Type": "nonexistent/format"}, "application/json", `{}`, http.StatusBadRequest, `Hook rules were not satisfied.`, `(?s)error parsing body payload due to unsupported content type header:`},
 }