Merge pull request #278 from adnanh/add-exe-extension-to-windows-build

Fix Makefile to include .exe extension for windows builds

.travis.yml

@@ -0,0 +1,21 @@
+language: go
+
+go:
+  - 1.8.x
+  - 1.9.x
+  - tip
+
+os:
+  - linux
+  - osx
+
+matrix:
+  fast_finish: true
+  allow_failures:
+    - go: tip
+
+install:
+  - go get -d -v -t ./...
+
+script:
+  - go test -v -race ./...

Makefile

@@ -1,7 +1,7 @@
-OS = darwin freebsd linux openbsd windows
+OS = darwin freebsd linux openbsd
 ARCHS = 386 arm amd64 arm64
 
-all: build release
+all: build release release-windows
 
 build: deps
 	go build
@@ -18,6 +18,15 @@ release: clean deps
 		done \
 	done
 
+release-windows: clean deps
+	@for arch in $(ARCHS);\
+	do \
+		echo "Building windows-$$arch"; \
+		mkdir -p build/webhook-windows-$$arch/; \
+		GOOS=windows GOARCH=$$arch go build -o build/webhook-windows-$$arch/webhook.exe; \
+		tar cz -C build -f build/webhook-windows-$$arch.tar.gz webhook-windows-$$arch; \
+	done
+
 test: deps
 	go test ./...
 

README.md

@@ -18,7 +18,7 @@ If you use Mattermost or Slack, you can set up an "Outgoing webhook integration"
 Everything else is the responsibility of the command's author.
 
 # Hookdoo
-<a href="https://www.hookdoo.com/?github"><img src="https://my.hookdoo.com/logo/logo-dark-96.png" alt="hookdoo" align="left" /></a>
+<a href="https://www.hookdoo.com/?github"><img src="https://www.hookdoo.com/logo/logo.svg" height="96" alt="hookdoo" align="left" /></a>
 
 If you don't have time to waste configuring, hosting, debugging and maintaining your webhook instance, we offer a __SaaS__ solution that has all of the capabilities webhook provides, plus a lot more, and all that packaged in a nice friendly web interface. If you are interested, find out more at [hookdoo website](https://www.hookdoo.com/?ref=github-webhook-readme). If you have any questions, you can contact us at info@hookdoo.com
 
@@ -45,7 +45,7 @@ Prebuilt binaries for different architectures are available at [GitHub Releases]
 ## Configuration
 Next step is to define some hooks you want [webhook][w] to serve. Begin by creating an empty file named `hooks.json`. This file will contain an array of hooks the [webhook][w] will serve. Check [Hook definition page](docs/Hook-Definition.md) to see the detailed description of what properties a hook can contain, and how to use them.
 
-Let's define a simple hook named `redeploy-webhook` that will run a redeploy script located in `/var/scripts/redeploy.sh`.
+Let's define a simple hook named `redeploy-webhook` that will run a redeploy script located in `/var/scripts/redeploy.sh`. Make sure that your bash script has `#!/bin/sh` shebang on top.
 
 Our `hooks.json` file will now look like this:
 ```json
@@ -92,6 +92,7 @@ Check out [Hook examples page](docs/Hook-Examples.md) for more complex examples
 ### Guides featuring webhook
  - [Webhook & JIRA](https://sites.google.com/site/mrxpalmeiras/notes/jira-webhooks) by [@perfecto25](https://github.com/perfecto25)
  - [Trigger Ansible AWX job runs on SCM (e.g. git) commit](http://jpmens.net/2017/10/23/trigger-awx-job-runs-on-scm-commit/) by [@jpmens](http://mens.de/)
+ - [Deploy using GitHub webhooks](https://davidauthier.wearemd.com/blog/deploy-using-github-webhooks.html) by [@awea](https://davidauthier.wearemd.com)
 
 ## Community Contributions
 See the [webhook-contrib][wc] repository for a collections of tools and helpers related to [webhook][w] that have been contributed by the [webhook][w] community.
@@ -102,7 +103,7 @@ Check out [existing issues](https://github.com/adnanh/webhook/issues) to see if
 # Support active development
 
 ## Sponsors
-## <a href="https://www.digitalocean.com/?ref=webhook"><img src="https://www.digitalocean.com/assets/media/logos-badges/png/DO_Logo_Horizontal_Blue-3db19536.png" alt="DigitalOcean" width="250"/></a>
+## <a href="https://www.digitalocean.com/?ref=webhook"><img src="http://www.hajdarevic.net/DO_Logo_Horizontal_Blue.png" alt="DigitalOcean" width="250"/></a>
 [DigitalOcean](https://www.digitalocean.com/?ref=webhook) is a simple and robust cloud computing platform, designed for developers.
 
 

appveyor.yml

@@ -0,0 +1,15 @@
+clone_folder: C:\gopath\src\github.com\adnanh\webhook
+
+environment:
+  GOPATH: c:\gopath
+
+install:
+  - set PATH=%PATH%;C:\MinGW\bin;%GOPATH%\bin
+  - echo %PATH%
+  - echo %GOPATH%
+  - go version
+  - go env
+  - go get -d -v -t ./...
+
+build_script:
+  - go test -v -race ./...

docs/Hook-Definition.md

@@ -11,12 +11,12 @@ Hooks are defined as JSON objects. Please note that in order to be considered va
  * `include-command-output-in-response` - boolean whether webhook should wait for the command to finish and return the raw output as a response to the hook initiator. If the command fails to execute or encounters any errors while executing the response will result in 500 Internal Server Error HTTP status code, otherwise the 200 OK status code will be returned.
  * `include-command-output-in-response-on-error` - boolean whether webhook should include command stdout & stderror as a response in failed executions. It only works if `include-command-output-in-response` is set to `true`.
  * `parse-parameters-as-json` - specifies the list of arguments that contain JSON strings. These parameters will be decoded by webhook and you can access them like regular objects in rules and `pass-arguments-to-command`.
- * `pass-arguments-to-command` - specifies the list of arguments that will be passed to the command. Check [Referencing request values page](Referencing-Request-Values) to see how to reference the values from the request. If you want to pass a static string value to your command you can specify it as
+ * `pass-arguments-to-command` - specifies the list of arguments that will be passed to the command. Check [Referencing request values page](Referencing-Request-Values.md) to see how to reference the values from the request. If you want to pass a static string value to your command you can specify it as
 `{ "source": "string", "name": "argumentvalue" }`
- * `pass-environment-to-command` - specifies the list of arguments that will be passed to the command as environment variables. If you do not specify the `"envname"` field in the referenced value, the hook will be in format "HOOK_argumentname", otherwise "envname" field will be used as it's name. Check [Referencing request values page](Referencing-Request-Values) to see how to reference the values from the request. If you want to pass a static string value to your command you can specify it as
+ * `pass-environment-to-command` - specifies the list of arguments that will be passed to the command as environment variables. If you do not specify the `"envname"` field in the referenced value, the hook will be in format "HOOK_argumentname", otherwise "envname" field will be used as it's name. Check [Referencing request values page](Referencing-Request-Values.md) to see how to reference the values from the request. If you want to pass a static string value to your command you can specify it as
 `{ "source": "string", "envname": "SOMETHING", "name": "argumentvalue" }`
 * `pass-file-to-command` - specifies a list of entries that will be serialized as a file. Incoming [data](Referencing-Request-Values.md) will be serialized in a request-temporary-file (otherwise parallel calls of the hook would lead to concurrent overwritings of the file). The filename to be addressed within the subsequent script is provided via an environment variable. Use `envname` to specify the name of the environment variable. If `envname` is not provided `HOOK_` and the name used to reference the request value are used. Defining `command-working-directory` will store the file relative to this location, if not provided, the systems temporary file directory will be used.  If `base64decode` is true, the incoming binary data will be base 64 decoded prior to storing it into the file. By default the corresponding file will be removed after the webhook exited.
- * `trigger-rule` - specifies the rule that will be evaluated in order to determine should the hook be triggered. Check [Hook rules page](Hook-Rules) to see the list of valid rules and their usage
+ * `trigger-rule` - specifies the rule that will be evaluated in order to determine should the hook be triggered. Check [Hook rules page](Hook-Rules.md) to see the list of valid rules and their usage
  * `trigger-rule-mismatch-http-response-code` - specifies the HTTP status code to be returned when the trigger rule is not satisfied
 
 ## Examples

docs/Hook-Examples.md

@@ -281,3 +281,44 @@ or in a single line, using https://github.com/jpmens/jo to generate the JSON cod
 jo binary=%filename.zip | curl -H "Content-Type:application/json" -X POST -d @- \
 http://localhost:9000/hooks/test-file-webhook
 </pre>
+
+
+## Incoming Scalr Webhook
+[Guide by @hassanbabaie]
+Scalr makes webhook calls based on an event to a configured webhook endpoint (for example Host Down, Host Up). Webhook endpoints are URLs where Scalr will deliver Webhook notifications.  
+Scalr assigns a unique signing key for every configured webhook endpoint.
+Refer to this URL for information on how to setup the webhook call on the Scalr side: [Scalr Wiki Webhooks](https://scalr-wiki.atlassian.net/wiki/spaces/docs/pages/6193173/Webhooks)
+In order to leverage the Signing Key for addtional authentication/security you must configure the trigger rule with a match type of "scalr-signature".
+
+```json
+[
+    {
+        "id": "redeploy-webhook",
+        "execute-command": "/home/adnan/redeploy-go-webhook.sh",
+        "command-working-directory": "/home/adnan/go",
+        "include-command-output-in-response": true,
+        "trigger-rule": 
+		{
+            "match": 
+			{
+                "type": "scalr-signature",
+                "secret": "Scalr-provided signing key"
+            }
+        },
+        "pass-environment-to-command": 
+		[
+            {
+                "envname": "EVENT_NAME",
+                "source": "payload",
+                "name": "eventName"
+            },
+            {
+                "envname": "SERVER_HOSTNAME",
+                "source": "payload",
+                "name": "data.SCALR_SERVER_HOSTNAME"
+            }
+        ]
+    }
+]
+
+```

docs/Hook-Rules.md

@@ -198,4 +198,20 @@ The IP can be IPv4- or IPv6-formatted, using [CIDR notation](https://en.wikipedi
     "ip-range": "192.168.0.1/24"
   }
 }
+```
+
+### 5. Match scalr-signature
+
+The trigger rule checks the scalr signature and also checks that the request was signed less than 5 minutes before it was received. 
+A unqiue signing key is generated for each webhook endpoint URL you register in Scalr.
+Given the time check make sure that NTP is enabled on both your Scalr and webhook server to prevent any issues
+
+```json
+{
+  "match":
+  {
+    "type": "scalr-signature",
+    "secret": "Scalr-provided signing key"
+  }
+}
 ```

hook/hook.go

@@ -12,6 +12,7 @@ import (
 	"fmt"
 	"io/ioutil"
 	"log"
+	"math"
 	"net"
 	"net/textproto"
 	"os"
@@ -20,6 +21,7 @@ import (
 	"strconv"
 	"strings"
 	"text/template"
+	"time"
 
 	"github.com/ghodss/yaml"
 )
@@ -92,9 +94,7 @@ func (e *ParseError) Error() string {
 
 // CheckPayloadSignature calculates and verifies SHA1 signature of the given payload
 func CheckPayloadSignature(payload []byte, secret string, signature string) (string, error) {
-	if strings.HasPrefix(signature, "sha1=") {
-		signature = signature[5:]
-	}
+	signature = strings.TrimPrefix(signature, "sha1=")
 
 	mac := hmac.New(sha1.New, []byte(secret))
 	_, err := mac.Write(payload)
@@ -111,9 +111,7 @@ func CheckPayloadSignature(payload []byte, secret string, signature string) (str
 
 // CheckPayloadSignature256 calculates and verifies SHA256 signature of the given payload
 func CheckPayloadSignature256(payload []byte, secret string, signature string) (string, error) {
-	if strings.HasPrefix(signature, "sha256=") {
-		signature = signature[7:]
-	}
+	signature = strings.TrimPrefix(signature, "sha256=")
 
 	mac := hmac.New(sha256.New, []byte(secret))
 	_, err := mac.Write(payload)
@@ -128,6 +126,43 @@ func CheckPayloadSignature256(payload []byte, secret string, signature string) (
 	return expectedMAC, err
 }
 
+func CheckScalrSignature(headers map[string]interface{}, body []byte, signingKey string, checkDate bool) (bool, error) {
+	// Check for the signature and date headers
+	if _, ok := headers["X-Signature"]; !ok {
+		return false, nil
+	}
+	if _, ok := headers["Date"]; !ok {
+		return false, nil
+	}
+	providedSignature := headers["X-Signature"].(string)
+	dateHeader := headers["Date"].(string)
+	mac := hmac.New(sha1.New, []byte(signingKey))
+	mac.Write(body)
+	mac.Write([]byte(dateHeader))
+	expectedSignature := hex.EncodeToString(mac.Sum(nil))
+
+	if !hmac.Equal([]byte(providedSignature), []byte(expectedSignature)) {
+		return false, &SignatureError{providedSignature}
+	}
+
+	if !checkDate {
+		return true, nil
+	}
+	// Example format: Fri 08 Sep 2017 11:24:32 UTC
+	date, err := time.Parse("Mon 02 Jan 2006 15:04:05 MST", dateHeader)
+	//date, err := time.Parse(time.RFC1123, dateHeader)
+	if err != nil {
+		return false, err
+	}
+	now := time.Now()
+	delta := math.Abs(now.Sub(date).Seconds())
+
+	if delta > 300 {
+		return false, &SignatureError{"outdated"}
+	}
+	return true, nil
+}
+
 // CheckIPWhitelist makes sure the provided remote address (of the form IP:port) falls within the provided IP range
 // (in CIDR form or a single IP address).
 func CheckIPWhitelist(remoteAddr string, ipRange string) (bool, error) {
@@ -157,7 +192,7 @@ func CheckIPWhitelist(remoteAddr string, ipRange string) (bool, error) {
 
 	ipRange = strings.TrimSpace(ipRange)
 
-	if strings.Index(ipRange, "/") == -1 {
+	if !strings.Contains(ipRange, "/") {
 		ipRange = ipRange + "/32"
 	}
 
@@ -391,6 +426,7 @@ type Hook struct {
 	JSONStringParameters                []Argument      `json:"parse-parameters-as-json,omitempty"`
 	TriggerRule                         *Rules          `json:"trigger-rule,omitempty"`
 	TriggerRuleMismatchHttpResponseCode int             `json:"trigger-rule-mismatch-http-response-code,omitempty"`
+	IncomingPayloadContentType          string          `json:"incoming-payload-content-type,omitempty"`	
 }
 
 // ParseJSONParameters decodes specified arguments to JSON objects and replaces the
@@ -648,7 +684,7 @@ func (r AndRule) Evaluate(headers, query, payload *map[string]interface{}, body
 		}
 
 		res = res && rv
-		if res == false {
+		if !res {
 			return res, nil
 		}
 	}
@@ -670,7 +706,7 @@ func (r OrRule) Evaluate(headers, query, payload *map[string]interface{}, body *
 		}
 
 		res = res || rv
-		if res == true {
+		if res {
 			return res, nil
 		}
 	}
@@ -704,6 +740,7 @@ const (
 	MatchHashSHA1   string = "payload-hash-sha1"
 	MatchHashSHA256 string = "payload-hash-sha256"
 	IPWhitelist     string = "ip-whitelist"
+	ScalrSignature  string = "scalr-signature"
 )
 
 // Evaluate MatchRule will return based on the type
@@ -711,6 +748,9 @@ func (r MatchRule) Evaluate(headers, query, payload *map[string]interface{}, bod
 	if r.Type == IPWhitelist {
 		return CheckIPWhitelist(remoteAddr, r.IPRange)
 	}
+	if r.Type == ScalrSignature {
+		return CheckScalrSignature(*headers, *body, r.Secret, true)
+	}
 
 	if arg, ok := r.Parameter.Get(headers, query, payload); ok {
 		switch r.Type {

hook/hook_test.go

@@ -60,6 +60,54 @@ func TestCheckPayloadSignature256(t *testing.T) {
 	}
 }
 
+var checkScalrSignatureTests = []struct {
+	description       string
+	headers           map[string]interface{}
+	payload           []byte
+	secret            string
+	expectedSignature string
+	ok                bool
+}{
+	{
+		"Valid signature",
+		map[string]interface{}{"Date": "Thu 07 Sep 2017 06:30:04 UTC", "X-Signature": "48e395e38ac48988929167df531eb2da00063a7d"},
+		[]byte(`{"a": "b"}`), "bilFGi4ZVZUdG+C6r0NIM9tuRq6PaG33R3eBUVhLwMAErGBaazvXe4Gq2DcJs5q+",
+		"48e395e38ac48988929167df531eb2da00063a7d", true,
+	},
+	{
+		"Wrong signature",
+		map[string]interface{}{"Date": "Thu 07 Sep 2017 06:30:04 UTC", "X-Signature": "999395e38ac48988929167df531eb2da00063a7d"},
+		[]byte(`{"a": "b"}`), "bilFGi4ZVZUdG+C6r0NIM9tuRq6PaG33R3eBUVhLwMAErGBaazvXe4Gq2DcJs5q+",
+		"48e395e38ac48988929167df531eb2da00063a7d", false,
+	},
+	{
+		"Missing Date header",
+		map[string]interface{}{"X-Signature": "999395e38ac48988929167df531eb2da00063a7d"},
+		[]byte(`{"a": "b"}`), "bilFGi4ZVZUdG+C6r0NIM9tuRq6PaG33R3eBUVhLwMAErGBaazvXe4Gq2DcJs5q+",
+		"48e395e38ac48988929167df531eb2da00063a7d", false,
+	},
+	{
+		"Missing X-Signature header",
+		map[string]interface{}{"Date": "Thu 07 Sep 2017 06:30:04 UTC"},
+		[]byte(`{"a": "b"}`), "bilFGi4ZVZUdG+C6r0NIM9tuRq6PaG33R3eBUVhLwMAErGBaazvXe4Gq2DcJs5q+",
+		"48e395e38ac48988929167df531eb2da00063a7d", false,
+	},
+}
+
+func TestCheckScalrSignature(t *testing.T) {
+	for _, testCase := range checkScalrSignatureTests {
+		valid, err := CheckScalrSignature(testCase.headers, testCase.payload, testCase.secret, false)
+		if valid != testCase.ok {
+			t.Errorf("failed to check scalr signature fot test case: %s\nexpected ok:%#v, got ok:%#v}",
+				testCase.description, testCase.ok, valid)
+		}
+
+		if err != nil && strings.Contains(err.Error(), testCase.expectedSignature) {
+			t.Errorf("error message should not disclose expected mac: %s on test case %s", err, testCase.description)
+		}
+	}
+}
+
 var extractParameterTests = []struct {
 	s      string
 	params interface{}

test/hookecho.go

@@ -5,8 +5,8 @@ package main
 import (
 	"fmt"
 	"os"
-	"strings"
 	"strconv"
+	"strings"
 )
 
 func main() {

test/hooks.json.tmpl

@@ -180,5 +180,29 @@
     "execute-command": "{{ .Hookecho }}",
     "include-command-output-in-response": true,
     "include-command-output-in-response-on-error": true
+  },
+  {
+    "id": "static-params-ok",
+		"execute-command": "{{ .Hookecho }}",
+		"response-message": "success",
+    "include-command-output-in-response": true,
+		"pass-arguments-to-command": [
+      {
+        "source": "string",
+        "name": "passed"
+      }
+    ],
+  },
+  {
+    "id": "warn-on-space",
+		"execute-command": "{{ .Hookecho }} foo",
+		"response-message": "success",
+    "include-command-output-in-response": true,
+		"pass-arguments-to-command": [
+      {
+        "source": "string",
+        "name": "passed"
+      }
+    ],
   }
 ]

test/hooks.yaml.tmpl

@@ -1,4 +1,5 @@
-- trigger-rule:
+- id: github
+  trigger-rule:
     and:
     - match:
         parameter:
@@ -23,9 +24,10 @@
   pass-environment-to-command:
   - source: payload
     name: head_commit.timestamp
-  id: github
   command-working-directory: /
-- trigger-rule:
+
+- id: bitbucket
+  trigger-rule:
     and:
     - match:
         parameter:
@@ -52,9 +54,10 @@
   execute-command: '{{ .Hookecho }}'
   response-message: success
   include-command-output-in-response: false
-  id: bitbucket
   command-working-directory: /
-- trigger-rule:
+
+- id: gitlab
+  trigger-rule:
     match:
       parameter:
         source: payload
@@ -71,25 +74,28 @@
   execute-command: '{{ .Hookecho }}'
   response-message: success
   include-command-output-in-response: true
-  id: gitlab
   command-working-directory: /
+
 - id: capture-command-output-on-success-not-by-default
   pass-arguments-to-command:
   - source: string
     name: exit=0
   execute-command: '{{ .Hookecho }}'
+
 - id: capture-command-output-on-success-yes-with-flag
   pass-arguments-to-command:
   - source: string
     name: exit=0
   execute-command: '{{ .Hookecho }}'
   include-command-output-in-response: true
+
 - id: capture-command-output-on-error-not-by-default
   pass-arguments-to-command:
   - source: string
     name: exit=1
   execute-command: '{{ .Hookecho }}'
   include-command-output-in-response: true
+
 - id: capture-command-output-on-error-yes-with-extra-flag
   pass-arguments-to-command:
   - source: string
@@ -97,3 +103,14 @@
   execute-command: '{{ .Hookecho }}'
   include-command-output-in-response: true
   include-command-output-in-response-on-error: true
+
+- id: static-params-ok
+  execute-command: '{{ .Hookecho }}'
+  include-command-output-in-response: true
+  pass-arguments-to-command:
+  - source: string
+    name: passed
+
+- id: warn-on-space
+  execute-command: '{{ .Hookecho }} foo'
+  include-command-output-in-response: true

webhook.go

@@ -23,7 +23,7 @@ import (
 )
 
 const (
-	version = "2.6.7"
+	version = "2.6.9"
 )
 
 var (
@@ -120,7 +120,7 @@ func main() {
 
 	newHooksFiles := hooksFiles[:0]
 	for _, filePath := range hooksFiles {
-		if _, ok := loadedHooksFromFiles[filePath]; ok == true {
+		if _, ok := loadedHooksFromFiles[filePath]; ok {
 			newHooksFiles = append(newHooksFiles, filePath)
 		}
 	}
@@ -229,7 +229,11 @@ func hookHandler(w http.ResponseWriter, r *http.Request) {
 		// parse body
 		var payload map[string]interface{}
 
+		// set contentType to IncomingPayloadContentType or header value
 		contentType := r.Header.Get("Content-Type")
+		if len(matchedHook.IncomingPayloadContentType) != 0 {
+			contentType = matchedHook.IncomingPayloadContentType
+		}
 
 		if strings.Contains(contentType, "json") {
 			decoder := json.NewDecoder(strings.NewReader(string(body)))
@@ -250,10 +254,9 @@ func hookHandler(w http.ResponseWriter, r *http.Request) {
 		}
 
 		// handle hook
-		if errors := matchedHook.ParseJSONParameters(&headers, &query, &payload); errors != nil {
-			for _, err := range errors {
-				log.Printf("[%s] error parsing JSON parameters: %s\n", rid, err)
-			}
+		errors := matchedHook.ParseJSONParameters(&headers, &query, &payload)
+		for _, err := range errors {
+			log.Printf("[%s] error parsing JSON parameters: %s\n", rid, err)
 		}
 
 		var ok bool
@@ -264,7 +267,7 @@ func hookHandler(w http.ResponseWriter, r *http.Request) {
 			ok, err = matchedHook.TriggerRule.Evaluate(&headers, &query, &payload, &body, r.RemoteAddr)
 			if err != nil {
 				msg := fmt.Sprintf("[%s] error evaluating hook: %s", rid, err)
-				log.Printf(msg)
+				log.Print(msg)
 				w.WriteHeader(http.StatusInternalServerError)
 				fmt.Fprintf(w, "Error occurred while evaluating hook rules.")
 				return
@@ -341,40 +344,37 @@ func handleHook(h *hook.Hook, rid string, headers, query, payload *map[string]in
 	cmd.Dir = h.CommandWorkingDirectory
 
 	cmd.Args, errors = h.ExtractCommandArguments(headers, query, payload)
-	if errors != nil {
-		for _, err := range errors {
-			log.Printf("[%s] error extracting command arguments: %s\n", rid, err)
-		}
+	for _, err := range errors {
+		log.Printf("[%s] error extracting command arguments: %s\n", rid, err)
 	}
 
 	var envs []string
 	envs, errors = h.ExtractCommandArgumentsForEnv(headers, query, payload)
 
-	if errors != nil {
-		for _, err := range errors {
-			log.Printf("[%s] error extracting command arguments for environment: %s\n", rid, err)
-		}
+	for _, err := range errors {
+		log.Printf("[%s] error extracting command arguments for environment: %s\n", rid, err)
 	}
 
 	files, errors := h.ExtractCommandArgumentsForFile(headers, query, payload)
 
-	if errors != nil {
-		for _, err := range errors {
-			log.Printf("[%s] error extracting command arguments for file: %s\n", rid, err)
-		}
+	for _, err := range errors {
+		log.Printf("[%s] error extracting command arguments for file: %s\n", rid, err)
 	}
 
 	for i := range files {
 		tmpfile, err := ioutil.TempFile(h.CommandWorkingDirectory, files[i].EnvName)
 		if err != nil {
 			log.Printf("[%s] error creating temp file [%s]", rid, err)
+			continue
 		}
 		log.Printf("[%s] writing env %s file %s", rid, files[i].EnvName, tmpfile.Name())
 		if _, err := tmpfile.Write(files[i].Data); err != nil {
 			log.Printf("[%s] error writing file %s [%s]", rid, tmpfile.Name(), err)
+			continue
 		}
 		if err := tmpfile.Close(); err != nil {
 			log.Printf("[%s] error closing file %s [%s]", rid, tmpfile.Name(), err)
+			continue
 		}
 
 		files[i].File = tmpfile
@@ -394,10 +394,12 @@ func handleHook(h *hook.Hook, rid string, headers, query, payload *map[string]in
 	}
 
 	for i := range files {
-		log.Printf("[%s] removing file %s\n", rid, files[i].File.Name())
-		err := os.Remove(files[i].File.Name())
-		if err != nil {
-			log.Printf("[%s] error removing file %s [%s]", rid, files[i].File.Name(), err)
+		if files[i].File != nil {
+			log.Printf("[%s] removing file %s\n", rid, files[i].File.Name())
+			err := os.Remove(files[i].File.Name())
+			if err != nil {
+				log.Printf("[%s] error removing file %s [%s]", rid, files[i].File.Name(), err)
+			}
 		}
 	}
 
@@ -431,7 +433,7 @@ func reloadHooks(hooksFilePath string) {
 				}
 			}
 
-			if (matchLoadedHook(hook.ID) != nil && !wasHookIDAlreadyLoaded) || seenHooksIds[hook.ID] == true {
+			if (matchLoadedHook(hook.ID) != nil && !wasHookIDAlreadyLoaded) || seenHooksIds[hook.ID] {
 				log.Printf("error: hook with the id %s has already been loaded!\nplease check your hooks file for duplicate hooks ids!", hook.ID)
 				log.Println("reverting hooks back to the previous configuration")
 				return

webhook_test.go

@@ -13,6 +13,7 @@ import (
 	"regexp"
 	"runtime"
 	"strings"
+	"sync"
 	"testing"
 	"text/template"
 	"time"
@@ -21,15 +22,26 @@ import (
 )
 
 func TestStaticParams(t *testing.T) {
+	// FIXME(moorereason): incorporate this test into TestWebhook.
+	//   Need to be able to execute a binary with a space in the filename.
+	if runtime.GOOS == "windows" {
+		t.Skip("Skipping on Windows")
+	}
 
 	spHeaders := make(map[string]interface{})
 	spHeaders["User-Agent"] = "curl/7.54.0"
 	spHeaders["Accept"] = "*/*"
 
-	// case 1: correct entry
+	// case 2: binary with spaces in its name
+	err := os.Symlink("/bin/echo", "/tmp/with space")
+	if err != nil {
+		t.Fatalf("%v", err)
+	}
+	defer os.Remove("/tmp/with space")
+
 	spHook := &hook.Hook{
-		ID:                      "static-params-ok",
-		ExecuteCommand:          "/bin/echo",
+		ID:                      "static-params-name-space",
+		ExecuteCommand:          "/tmp/with space",
 		CommandWorkingDirectory: "/tmp",
 		ResponseMessage:         "success",
 		CaptureCommandOutput:    true,
@@ -41,159 +53,126 @@ func TestStaticParams(t *testing.T) {
 	b := &bytes.Buffer{}
 	log.SetOutput(b)
 
-	s, err := handleHook(spHook, "test", &spHeaders, &map[string]interface{}{}, &map[string]interface{}{}, &[]byte{})
+	_, err = handleHook(spHook, "test", &spHeaders, &map[string]interface{}{}, &map[string]interface{}{}, &[]byte{})
 	if err != nil {
 		t.Fatalf("Unexpected error: %v\n", err)
 	}
-	matched, _ := regexp.MatchString("(?s).*command output: passed.*static-params-ok", b.String())
-	if !matched {
-		t.Fatalf("Unexpected log output:\n%s", b)
-	}
-
-	// case 2: binary with spaces in its name
-	err = os.Symlink("/bin/echo", "/tmp/with space")
-	if err != nil {
-		t.Fatalf("%v", err)
-	}
-	defer os.Remove("/tmp/with space")
-
-	spHook = &hook.Hook{
-		ID:                      "static-params-name-space",
-		ExecuteCommand:          "/tmp/with space",
-		CommandWorkingDirectory: "/tmp",
-		ResponseMessage:         "success",
-		CaptureCommandOutput:    true,
-		PassArgumentsToCommand: []hook.Argument{
-			hook.Argument{Source: "string", Name: "passed"},
-		},
-	}
-
-	b = &bytes.Buffer{}
-	log.SetOutput(b)
-
-	s, err = handleHook(spHook, "test", &spHeaders, &map[string]interface{}{}, &map[string]interface{}{}, &[]byte{})
-	if err != nil {
-		t.Fatalf("Unexpected error: %v\n", err)
-	}
-	matched, _ = regexp.MatchString("(?s)command output: .*static-params-name-space", b.String())
+	matched, _ := regexp.MatchString("(?s)command output: .*static-params-name-space", b.String())
 	if !matched {
 		t.Fatalf("Unexpected log output:\n%sn", b)
 	}
-
-	// case 3: parameters specified in execute-command
-	spHook = &hook.Hook{
-		ID:                      "static-params-bad",
-		ExecuteCommand:          "/bin/echo success",
-		CommandWorkingDirectory: "/tmp",
-		ResponseMessage:         "success",
-		CaptureCommandOutput:    true,
-		PassArgumentsToCommand: []hook.Argument{
-			hook.Argument{Source: "string", Name: "failed"},
-		},
-	}
-
-	b = &bytes.Buffer{}
-	log.SetOutput(b)
-
-	s, err = handleHook(spHook, "test", &spHeaders, &map[string]interface{}{}, &map[string]interface{}{}, &[]byte{})
-	if err == nil {
-		t.Fatalf("Error expected, but none returned: %s\n", s)
-	}
-	matched, _ = regexp.MatchString("(?s)unable to locate command: ..bin.echo success.*use 'pass-arguments-to-command'", b.String())
-	if !matched {
-		t.Fatalf("Unexpected log output:\n%s\n", b)
-	}
 }
 
 func TestWebhook(t *testing.T) {
 	hookecho, cleanupHookecho := buildHookecho(t)
 	defer cleanupHookecho()
 
+	webhook, cleanupWebhookFn := buildWebhook(t)
+	defer cleanupWebhookFn()
+
 	for _, hookTmpl := range []string{"test/hooks.json.tmpl", "test/hooks.yaml.tmpl"} {
-		config, cleanupConfig := genConfig(t, hookecho, hookTmpl)
-		defer cleanupConfig()
-
-		webhook, cleanupWebhook := buildWebhook(t)
-		defer cleanupWebhook()
-
-		ip, port := serverAddress(t)
-		args := []string{fmt.Sprintf("-hooks=%s", config), fmt.Sprintf("-ip=%s", ip), fmt.Sprintf("-port=%s", port), "-verbose"}
-
-		cmd := exec.Command(webhook, args...)
-		//cmd.Stderr = os.Stderr // uncomment to see verbose output
-		cmd.Env = webhookEnv()
-		cmd.Args[0] = "webhook"
-		if err := cmd.Start(); err != nil {
-			t.Fatalf("failed to start webhook: %s", err)
-		}
-		defer killAndWait(cmd)
-
-		waitForServerReady(t, ip, port)
+		configPath, cleanupConfigFn := genConfig(t, hookecho, hookTmpl)
+		defer cleanupConfigFn()
 
 		for _, tt := range hookHandlerTests {
-			url := fmt.Sprintf("http://%s:%s/hooks/%s", ip, port, tt.id)
+			t.Run(tt.desc, func(t *testing.T) {
 
-			req, err := http.NewRequest("POST", url, ioutil.NopCloser(strings.NewReader(tt.body)))
-			if err != nil {
-				t.Errorf("New request failed: %s", err)
-			}
+				ip, port := serverAddress(t)
+				args := []string{fmt.Sprintf("-hooks=%s", configPath), fmt.Sprintf("-ip=%s", ip), fmt.Sprintf("-port=%s", port), "-verbose"}
 
-			for k, v := range tt.headers {
-				req.Header.Add(k, v)
-			}
+				// Setup a buffer for capturing webhook logs for later evaluation
+				b := &buffer{}
 
-			var res *http.Response
+				cmd := exec.Command(webhook, args...)
+				cmd.Stderr = b
+				cmd.Env = webhookEnv()
+				cmd.Args[0] = "webhook"
+				if err := cmd.Start(); err != nil {
+					t.Fatalf("failed to start webhook: %s", err)
+				}
+				defer killAndWait(cmd)
 
-			if tt.urlencoded == true {
-				req.Header.Add("Content-Type", "application/x-www-form-urlencoded")
-			} else {
-				req.Header.Add("Content-Type", "application/json")
-			}
+				waitForServerReady(t, ip, port)
 
-			client := &http.Client{}
-			res, err = client.Do(req)
-			if err != nil {
-				t.Errorf("client.Do failed: %s", err)
-			}
+				url := fmt.Sprintf("http://%s:%s/hooks/%s", ip, port, tt.id)
 
-			body, err := ioutil.ReadAll(res.Body)
-			res.Body.Close()
-			if err != nil {
-				t.Errorf("POST %q: failed to ready body: %s", tt.desc, err)
-			}
+				req, err := http.NewRequest("POST", url, ioutil.NopCloser(strings.NewReader(tt.body)))
+				if err != nil {
+					t.Errorf("New request failed: %s", err)
+				}
 
-			if res.StatusCode != tt.respStatus || string(body) != tt.respBody {
-				t.Errorf("failed %q (id: %s):\nexpected status: %#v, response: %s\ngot status: %#v, response: %s", tt.desc, tt.id, tt.respStatus, tt.respBody, res.StatusCode, body)
-			}
+				for k, v := range tt.headers {
+					req.Header.Add(k, v)
+				}
+
+				var res *http.Response
+
+				if tt.urlencoded == true {
+					req.Header.Add("Content-Type", "application/x-www-form-urlencoded")
+				} else {
+					req.Header.Add("Content-Type", "application/json")
+				}
+
+				client := &http.Client{}
+				res, err = client.Do(req)
+				if err != nil {
+					t.Errorf("client.Do failed: %s", err)
+				}
+
+				body, err := ioutil.ReadAll(res.Body)
+				res.Body.Close()
+				if err != nil {
+					t.Errorf("POST %q: failed to ready body: %s", tt.desc, err)
+				}
+
+				if res.StatusCode != tt.respStatus || string(body) != tt.respBody {
+					t.Errorf("failed %q (id: %s):\nexpected status: %#v, response: %s\ngot status: %#v, response: %s", tt.desc, tt.id, tt.respStatus, tt.respBody, res.StatusCode, body)
+				}
+
+				if tt.logMatch == "" {
+					return
+				}
+
+				// There's the potential for a race condition below where we
+				// try to read the logs buffer b before the logs have been
+				// flushed by the webhook process. Kill the process to flush
+				// the logs.
+				killAndWait(cmd)
+
+				matched, _ := regexp.MatchString(tt.logMatch, b.String())
+				if !matched {
+					t.Errorf("failed log match for %q (id: %s):\nmatch pattern: %q\ngot:\n%s", tt.desc, tt.id, tt.logMatch, b)
+				}
+			})
 		}
 	}
 }
 
-func buildHookecho(t *testing.T) (bin string, cleanup func()) {
+func buildHookecho(t *testing.T) (binPath string, cleanupFn func()) {
 	tmp, err := ioutil.TempDir("", "hookecho-test-")
 	if err != nil {
 		t.Fatal(err)
 	}
 	defer func() {
-		if cleanup == nil {
+		if cleanupFn == nil {
 			os.RemoveAll(tmp)
 		}
 	}()
 
-	bin = filepath.Join(tmp, "hookecho")
+	binPath = filepath.Join(tmp, "hookecho")
 	if runtime.GOOS == "windows" {
-		bin += ".exe"
+		binPath += ".exe"
 	}
 
-	cmd := exec.Command("go", "build", "-o", bin, "test/hookecho.go")
+	cmd := exec.Command("go", "build", "-o", binPath, "test/hookecho.go")
 	if err := cmd.Run(); err != nil {
 		t.Fatalf("Building hookecho: %v", err)
 	}
 
-	return bin, func() { os.RemoveAll(tmp) }
+	return binPath, func() { os.RemoveAll(tmp) }
 }
 
-func genConfig(t *testing.T, bin string, hookTemplate string) (config string, cleanup func()) {
+func genConfig(t *testing.T, bin string, hookTemplate string) (configPath string, cleanupFn func()) {
 	tmpl := template.Must(template.ParseFiles(hookTemplate))
 
 	tmp, err := ioutil.TempDir("", "webhook-config-")
@@ -201,7 +180,7 @@ func genConfig(t *testing.T, bin string, hookTemplate string) (config string, cl
 		t.Fatal(err)
 	}
 	defer func() {
-		if cleanup == nil {
+		if cleanupFn == nil {
 			os.RemoveAll(tmp)
 		}
 	}()
@@ -215,7 +194,11 @@ func genConfig(t *testing.T, bin string, hookTemplate string) (config string, cl
 	}
 	defer file.Close()
 
-	data := struct{ Hookecho string }{filepath.ToSlash(bin)}
+	data := struct{ Hookecho string }{filepath.FromSlash(bin)}
+	if runtime.GOOS == "windows" {
+		// Simulate escaped backslashes on Windows.
+		data.Hookecho = strings.Replace(data.Hookecho, `\`, `\\`, -1)
+	}
 	if err := tmpl.Execute(file, data); err != nil {
 		t.Fatalf("Executing template: %v", err)
 	}
@@ -223,28 +206,28 @@ func genConfig(t *testing.T, bin string, hookTemplate string) (config string, cl
 	return path, func() { os.RemoveAll(tmp) }
 }
 
-func buildWebhook(t *testing.T) (bin string, cleanup func()) {
+func buildWebhook(t *testing.T) (binPath string, cleanupFn func()) {
 	tmp, err := ioutil.TempDir("", "webhook-test-")
 	if err != nil {
 		t.Fatal(err)
 	}
 	defer func() {
-		if cleanup == nil {
+		if cleanupFn == nil {
 			os.RemoveAll(tmp)
 		}
 	}()
 
-	bin = filepath.Join(tmp, "webhook")
+	binPath = filepath.Join(tmp, "webhook")
 	if runtime.GOOS == "windows" {
-		bin += ".exe"
+		binPath += ".exe"
 	}
 
-	cmd := exec.Command("go", "build", "-o", bin)
+	cmd := exec.Command("go", "build", "-o", binPath)
 	if err := cmd.Run(); err != nil {
 		t.Fatalf("Building webhook: %v", err)
 	}
 
-	return bin, func() { os.RemoveAll(tmp) }
+	return binPath, func() { os.RemoveAll(tmp) }
 }
 
 func serverAddress(t *testing.T) (string, string) {
@@ -288,6 +271,10 @@ func waitForServer(t *testing.T, url string, status int, timeout time.Duration)
 }
 
 func killAndWait(cmd *exec.Cmd) {
+	if cmd == nil || cmd.ProcessState != nil && cmd.ProcessState.Exited() {
+		return
+	}
+
 	cmd.Process.Kill()
 	cmd.Wait()
 }
@@ -313,6 +300,7 @@ var hookHandlerTests = []struct {
 
 	respStatus int
 	respBody   string
+	logMatch   string
 }{
 	{
 		"github",
@@ -467,6 +455,7 @@ var hookHandlerTests = []struct {
 		`arg: 1481a2de7b2a7d02428ad93446ab166be7793fbb lolwut@noway.biz
 env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00
 `,
+		``,
 	},
 	{
 		"bitbucket", // bitbucket sends their payload using uriencoded params.
@@ -476,6 +465,7 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00
 		true,
 		http.StatusOK,
 		`success`,
+		``,
 	},
 	{
 		"gitlab",
@@ -527,6 +517,7 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00
 		http.StatusOK,
 		`arg: b6568db1bc1dcd7f8b4d5a946b0b91f9dacd7327 John Smith john@example.com
 `,
+		``,
 	},
 
 	{
@@ -567,6 +558,7 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00
 		`arg: 1481a2de7b2a7d02428ad93446ab166be7793fbb lolwut@noway.biz
 env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00
 `,
+		``,
 	},
 
 	{
@@ -605,20 +597,55 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00
 		http.StatusOK,
 		`arg: 1481a2de7b2a7d02428ad93446ab166be7793fbb lolwut@noway.biz
 `,
+		``,
 	},
 
 	// test with custom return code
-	{"empty payload", "github", nil, `{}`, false, http.StatusBadRequest, `Hook rules were not satisfied.`},
+	{"empty payload", "github", nil, `{}`, false, http.StatusBadRequest, `Hook rules were not satisfied.`, ``},
 	// test with custom invalid http code, should default to 200 OK
-	{"empty payload", "bitbucket", nil, `{}`, false, http.StatusOK, `Hook rules were not satisfied.`},
+	{"empty payload", "bitbucket", nil, `{}`, false, http.StatusOK, `Hook rules were not satisfied.`, ``},
 	// test with no configured http return code, should default to 200 OK
-	{"empty payload", "gitlab", nil, `{}`, false, http.StatusOK, `Hook rules were not satisfied.`},
+	{"empty payload", "gitlab", nil, `{}`, false, http.StatusOK, `Hook rules were not satisfied.`, ``},
 
 	// test capturing command output
-	{"don't capture output on success by default", "capture-command-output-on-success-not-by-default", nil, `{}`, false, http.StatusOK, ``},
+	{"don't capture output on success by default", "capture-command-output-on-success-not-by-default", nil, `{}`, false, http.StatusOK, ``, ``},
 	{"capture output on success with flag set", "capture-command-output-on-success-yes-with-flag", nil, `{}`, false, http.StatusOK, `arg: exit=0
-`},
-	{"don't capture output on error by default", "capture-command-output-on-error-not-by-default", nil, `{}`, false, http.StatusInternalServerError, `Error occurred while executing the hook's command. Please check your logs for more details.`},
+`, ``},
+	{"don't capture output on error by default", "capture-command-output-on-error-not-by-default", nil, `{}`, false, http.StatusInternalServerError, `Error occurred while executing the hook's command. Please check your logs for more details.`, ``},
 	{"capture output on error with extra flag set", "capture-command-output-on-error-yes-with-extra-flag", nil, `{}`, false, http.StatusInternalServerError, `arg: exit=1
-`},
+`, ``},
+
+	// Check logs
+	{"static params should pass", "static-params-ok", nil, `{}`, false, http.StatusOK, "arg: passed\n", `(?s)command output: arg: passed`},
+	{"command with space logs warning", "warn-on-space", nil, `{}`, false, http.StatusInternalServerError, "Error occurred while executing the hook's command. Please check your logs for more details.", `(?s)unable to locate command.*use 'pass[-]arguments[-]to[-]command' to specify args`},
+}
+
+// buffer provides a concurrency-safe bytes.Buffer to tests above.
+type buffer struct {
+	b bytes.Buffer
+	m sync.Mutex
+}
+
+func (b *buffer) Read(p []byte) (n int, err error) {
+	b.m.Lock()
+	defer b.m.Unlock()
+	return b.b.Read(p)
+}
+
+func (b *buffer) Write(p []byte) (n int, err error) {
+	b.m.Lock()
+	defer b.m.Unlock()
+	return b.b.Write(p)
+}
+
+func (b *buffer) String() string {
+	b.m.Lock()
+	defer b.m.Unlock()
+	return b.b.String()
+}
+
+func (b *buffer) Reset() {
+	b.m.Lock()
+	defer b.m.Unlock()
+	b.b.Reset()
 }