From a99abd4e6fba5b40efa83bcd64b9d59451abe993 Mon Sep 17 00:00:00 2001 From: Adnan Hajdarevic Date: Mon, 2 Dec 2019 18:49:24 +0100 Subject: [PATCH 1/3] Fix invalid assumption in code that multipart forms can be parsed in the same way as urlencoded forms. Refactored code to use switch-case statement over the `Content-Type` header and log unsupported content types instead of silently failing. Also made the `x-www-form-urlencoded` content type handler more specific (as opposed to the previous code which looked for `form` occurence in the value), as we need to use different logic for multipart forms, which we'll hopefully implement soon. The issue with multipart forms that we have to handle first is that the files are being written to temporary files, and as such, for async hooks webhook cannot guarantee they'll be available after we close the request; that, and the fact that we don't have code that will properly serialize and pass such Golang objects to the script, as there are several fields which might be interesting to the end user. --- webhook.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/webhook.go b/webhook.go index 16cbcbe..502d798 100644 --- a/webhook.go +++ b/webhook.go @@ -240,7 +240,8 @@ func hookHandler(w http.ResponseWriter, r *http.Request) { contentType = matchedHook.IncomingPayloadContentType } - if strings.Contains(contentType, "json") { + switch { + case strings.Contains(contentType, "json"): decoder := json.NewDecoder(strings.NewReader(string(body))) decoder.UseNumber() @@ -249,13 +250,15 @@ func hookHandler(w http.ResponseWriter, r *http.Request) { if err != nil { log.Printf("[%s] error parsing JSON payload %+v\n", rid, err) } - } else if strings.Contains(contentType, "form") { + case strings.Contains(contentType, "x-www-form-urlencoded"): fd, err := url.ParseQuery(string(body)) if err != nil { log.Printf("[%s] error parsing form payload %+v\n", rid, err) } else { payload = valuesToMap(fd) } + default: + log.Printf("[%s] error parsing body payload due to unsupported content type header: %s\n", rid, contentType) } // handle hook @@ -272,7 +275,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.Print(msg) + log.Println(msg) w.WriteHeader(http.StatusInternalServerError) fmt.Fprint(w, "Error occurred while evaluating hook rules.") return From 1110f82443d1177eb77d29f5f70a11319748b7b6 Mon Sep 17 00:00:00 2001 From: Adnan Hajdarevic Date: Mon, 2 Dec 2019 19:01:20 +0100 Subject: [PATCH 2/3] Add test for unsupported content type error message. --- webhook_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/webhook_test.go b/webhook_test.go index a1f8fc4..6f46527 100644 --- a/webhook_test.go +++ b/webhook_test.go @@ -617,6 +617,7 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 // 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`}, + {"unsupported content type error", "github", map[string]string{"Content-Type": "nonexistent/format"}, `{}`, false, http.StatusBadRequest, `Hook rules were not satisfied.`, `(?s)error parsing body payload due to unsupported content type header:`}, } // buffer provides a concurrency-safe bytes.Buffer to tests above. From ce186487f4402ccd594b6bbb4b46e999b896a063 Mon Sep 17 00:00:00 2001 From: Adnan Hajdarevic Date: Mon, 2 Dec 2019 19:03:38 +0100 Subject: [PATCH 3/3] Format the file using `go fmt`. --- webhook.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/webhook.go b/webhook.go index 502d798..7e37aef 100644 --- a/webhook.go +++ b/webhook.go @@ -341,8 +341,8 @@ func handleHook(h *hook.Hook, rid string, headers, query, payload *map[string]in // 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) + // 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) }