From d2e315d9c61a3e7a9752cd727e8b72f7de5a90aa Mon Sep 17 00:00:00 2001 From: Cameron Moore Date: Wed, 18 Nov 2015 12:00:47 -0600 Subject: [PATCH] Fix environment and argument passing Two issues are addressed in this commit: 1. Instead of only sending the predefined environment arguments, this commit appends the arguments to the existing OS environment. Fixes #53. 2. If an argument is not found in the payload, allow the command to run and pass in an empty string as a placeholder. Fixes #54. Additionally, I replaced `hook.ErrInvalidPayloadSignature` with a new `SignatureError` type so that we can embed the signature in the error. --- hook/hook.go | 17 +++++++--- hook/hook_test.go | 2 +- test/hooks.json.tmpl | 8 ++--- webhook.go | 10 +++--- webhook_test.go | 77 +++++++++++++++++++++++++++++++++++++++++++- 5 files changed, 97 insertions(+), 17 deletions(-) diff --git a/hook/hook.go b/hook/hook.go index cefc158..e01d078 100644 --- a/hook/hook.go +++ b/hook/hook.go @@ -5,7 +5,6 @@ import ( "crypto/sha1" "encoding/hex" "encoding/json" - "errors" "fmt" "io/ioutil" "reflect" @@ -31,8 +30,17 @@ const ( EnvNamespace string = "HOOK_" ) -// ErrInvalidPayloadSignature describes an invalid payload signature. -var ErrInvalidPayloadSignature = errors.New("invalid payload signature") +// SignatureError describes an invalid payload signature passed to Hook. +type SignatureError struct { + Signature string +} + +func (e *SignatureError) Error() string { + if e == nil { + return "" + } + return fmt.Sprintf("invalid payload signature %s", e.Signature) +} // ArgumentError describes an invalid argument passed to Hook. type ArgumentError struct { @@ -84,7 +92,7 @@ func CheckPayloadSignature(payload []byte, secret string, signature string) (str expectedMAC := hex.EncodeToString(mac.Sum(nil)) if !hmac.Equal([]byte(signature), []byte(expectedMAC)) { - err = ErrInvalidPayloadSignature + return expectedMAC, &SignatureError{expectedMAC} } return expectedMAC, err } @@ -302,6 +310,7 @@ func (h *Hook) ExtractCommandArguments(headers, query, payload *map[string]inter if arg, ok := h.PassArgumentsToCommand[i].Get(headers, query, payload); ok { args = append(args, arg) } else { + args = append(args, "") return args, &ArgumentError{h.PassArgumentsToCommand[i]} } } diff --git a/hook/hook_test.go b/hook/hook_test.go index 932395a..f4d5b8f 100644 --- a/hook/hook_test.go +++ b/hook/hook_test.go @@ -123,7 +123,7 @@ var hookExtractCommandArgumentsTests = []struct { }{ {"test", []Argument{Argument{"header", "a"}}, &map[string]interface{}{"a": "z"}, nil, nil, []string{"test", "z"}, true}, // failures - {"fail", []Argument{Argument{"payload", "a"}}, &map[string]interface{}{"a": "z"}, nil, nil, []string{"fail"}, false}, + {"fail", []Argument{Argument{"payload", "a"}}, &map[string]interface{}{"a": "z"}, nil, nil, []string{"fail", ""}, false}, } func TestHookExtractCommandArguments(t *testing.T) { diff --git a/test/hooks.json.tmpl b/test/hooks.json.tmpl index e2f696b..1839a18 100644 --- a/test/hooks.json.tmpl +++ b/test/hooks.json.tmpl @@ -8,7 +8,7 @@ [ { "source": "payload", - "name": "pusher.email" + "name": "head_commit.timestamp" } ], "pass-arguments-to-command": @@ -19,11 +19,7 @@ }, { "source": "payload", - "name": "pusher.name" - }, - { - "source": "payload", - "name": "pusher.email" + "name": "head_commit.author.email" } ], "trigger-rule": diff --git a/webhook.go b/webhook.go index ef2ca65..a116c90 100644 --- a/webhook.go +++ b/webhook.go @@ -238,16 +238,16 @@ func handleHook(h *hook.Hook, headers, query, payload *map[string]interface{}, b cmd.Args, err = h.ExtractCommandArguments(headers, query, payload) if err != nil { log.Printf("error extracting command arguments: %s", err) - return "" } - cmd.Env, err = h.ExtractCommandArgumentsForEnv(headers, query, payload) + var envs []string + envs, err = h.ExtractCommandArgumentsForEnv(headers, query, payload) if err != nil { - log.Printf("error extracting command arguments: %s", err) - return "" + log.Printf("error extracting command arguments for environment: %s", err) } + cmd.Env = append(os.Environ(), envs...) - log.Printf("executing %s (%s) with arguments %s and environment %s using %s as cwd\n", h.ExecuteCommand, cmd.Path, cmd.Args, cmd.Env, cmd.Dir) + log.Printf("executing %s (%s) with arguments %q and environment %s using %s as cwd\n", h.ExecuteCommand, cmd.Path, cmd.Args, envs, cmd.Dir) out, err := cmd.CombinedOutput() diff --git a/webhook_test.go b/webhook_test.go index 74b1c2f..80cf0c3 100644 --- a/webhook_test.go +++ b/webhook_test.go @@ -372,7 +372,7 @@ var hookHandlerTests = []struct { }`, false, http.StatusOK, - `{"output":"arg: 1481a2de7b2a7d02428ad93446ab166be7793fbb Garen Torikian lolwut@noway.biz\nenv: HOOK_pusher.email=lolwut@noway.biz\n"}`, + `{"output":"arg: 1481a2de7b2a7d02428ad93446ab166be7793fbb lolwut@noway.biz\nenv: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00\n"}`, }, { "bitbucket", // bitbucket sends their payload using uriencoded params. @@ -434,5 +434,80 @@ var hookHandlerTests = []struct { `{"message":"success","output":"arg: b6568db1bc1dcd7f8b4d5a946b0b91f9dacd7327 John Smith john@example.com\n"}`, }, + { + "missing-cmd-arg", // missing head_commit.author.email + "github", + map[string]string{"X-Hub-Signature": "ab03955b9377f530aa298b1b6d273ae9a47e1e40"}, + `{ + "head_commit":{ + "added":[ + "words/madame-bovary.txt" + ], + "author":{ + "email":"lolwut@noway.biz", + "name":"Garen Torikian", + "username":"octokitty" + }, + "committer":{ + "email":"lolwut@noway.biz", + "name":"Garen Torikian", + "username":"octokitty" + }, + "distinct":true, + "id":"1481a2de7b2a7d02428ad93446ab166be7793fbb", + "message":"Rename madame-bovary.txt to words/madame-bovary.txt", + "modified":[ + + ], + "removed":[ + "madame-bovary.txt" + ], + "timestamp":"2013-03-12T08:14:29-07:00", + "url":"https://github.com/octokitty/testing/commit/1481a2de7b2a7d02428ad93446ab166be7793fbb" + }, + "ref":"refs/heads/master" + }`, + false, + http.StatusOK, + `{"output":"arg: 1481a2de7b2a7d02428ad93446ab166be7793fbb lolwut@noway.biz\nenv: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00\n"}`, + }, + + { + "missing-env-arg", // missing head_commit.timestamp + "github", + map[string]string{"X-Hub-Signature": "2cf8b878cb6b74a25090a140fa4a474be04b97fa"}, + `{ + "head_commit":{ + "added":[ + "words/madame-bovary.txt" + ], + "author":{ + "email":"lolwut@noway.biz", + "name":"Garen Torikian", + "username":"octokitty" + }, + "committer":{ + "email":"lolwut@noway.biz", + "name":"Garen Torikian", + "username":"octokitty" + }, + "distinct":true, + "id":"1481a2de7b2a7d02428ad93446ab166be7793fbb", + "message":"Rename madame-bovary.txt to words/madame-bovary.txt", + "modified":[ + + ], + "removed":[ + "madame-bovary.txt" + ], + "url":"https://github.com/octokitty/testing/commit/1481a2de7b2a7d02428ad93446ab166be7793fbb" + }, + "ref":"refs/heads/master" + }`, + false, + http.StatusOK, + `{"output":"arg: 1481a2de7b2a7d02428ad93446ab166be7793fbb lolwut@noway.biz\n"}`, + }, + {"empty payload", "github", nil, `{}`, false, http.StatusOK, `Hook rules were not satisfied.`}, }