From 3414f34025ed7094187f208d1f60fedde053d62c Mon Sep 17 00:00:00 2001 From: Cameron Moore Date: Thu, 26 Dec 2019 11:11:53 -0600 Subject: [PATCH 1/6] Add per-hook HTTP method restrictions --- docs/Hook-Definition.md | 1 + internal/hook/hook.go | 1 + test/hooks.json.tmpl | 1 + test/hooks.yaml.tmpl | 2 + webhook.go | 364 +++++++++++++++++++++------------------- webhook_test.go | 32 ++-- 6 files changed, 217 insertions(+), 184 deletions(-) diff --git a/docs/Hook-Definition.md b/docs/Hook-Definition.md index fd8836a..7c54a20 100644 --- a/docs/Hook-Definition.md +++ b/docs/Hook-Definition.md @@ -10,6 +10,7 @@ Hooks are defined as JSON objects. Please note that in order to be considered va * `response-headers` - specifies the list of headers in format `{"name": "X-Example-Header", "value": "it works"}` that will be returned in HTTP response for the hook * `success-http-response-code` - specifies the HTTP status code to be returned upon success * `incoming-payload-content-type` - sets the `Content-Type` of the incoming HTTP request (ie. `application/json`); useful when the request lacks a `Content-Type` or sends an erroneous value + * `http-methods` - a list of allowed HTTP methods, such as `POST` and `GET` * `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`. diff --git a/internal/hook/hook.go b/internal/hook/hook.go index 72acb01..0e4d70f 100644 --- a/internal/hook/hook.go +++ b/internal/hook/hook.go @@ -460,6 +460,7 @@ type Hook struct { TriggerRuleMismatchHttpResponseCode int `json:"trigger-rule-mismatch-http-response-code,omitempty"` IncomingPayloadContentType string `json:"incoming-payload-content-type,omitempty"` SuccessHttpResponseCode int `json:"success-http-response-code,omitempty"` + HTTPMethods []string `json:"http-methods"` } // ParseJSONParameters decodes specified arguments to JSON objects and replaces the diff --git a/test/hooks.json.tmpl b/test/hooks.json.tmpl index 40c6de5..e91f663 100644 --- a/test/hooks.json.tmpl +++ b/test/hooks.json.tmpl @@ -3,6 +3,7 @@ "id": "github", "execute-command": "{{ .Hookecho }}", "command-working-directory": "/", + "http-methods": ["POST"], "include-command-output-in-response": true, "trigger-rule-mismatch-http-response-code": 400, "pass-environment-to-command": diff --git a/test/hooks.yaml.tmpl b/test/hooks.yaml.tmpl index bfeacc2..79e8987 100644 --- a/test/hooks.yaml.tmpl +++ b/test/hooks.yaml.tmpl @@ -1,4 +1,6 @@ - id: github + http-methods: + - POST trigger-rule: and: - match: diff --git a/webhook.go b/webhook.go index 6807520..9b34598 100644 --- a/webhook.go +++ b/webhook.go @@ -253,218 +253,236 @@ func hookHandler(w http.ResponseWriter, r *http.Request) { log.Printf("[%s] incoming HTTP request from %s\n", rid, r.RemoteAddr) + id := mux.Vars(r)["id"] + + matchedHook := matchLoadedHook(id) + if matchedHook == nil { + w.WriteHeader(http.StatusNotFound) + fmt.Fprint(w, "Hook not found.") + return + } + + // Check for allowed methods + if len(matchedHook.HTTPMethods) != 0 { + var allowed bool + for i := range matchedHook.HTTPMethods { + if matchedHook.HTTPMethods[i] == r.Method { + allowed = true + break + } + } + + if !allowed { + w.WriteHeader(http.StatusMethodNotAllowed) + return + } + } + + log.Printf("[%s] %s got matched\n", rid, id) + for _, responseHeader := range responseHeaders { w.Header().Set(responseHeader.Name, responseHeader.Value) } - id := mux.Vars(r)["id"] + var ( + body []byte + err error + ) - if matchedHook := matchLoadedHook(id); matchedHook != nil { - log.Printf("[%s] %s got matched\n", rid, id) + // set contentType to IncomingPayloadContentType or header value + contentType := r.Header.Get("Content-Type") + if len(matchedHook.IncomingPayloadContentType) != 0 { + contentType = matchedHook.IncomingPayloadContentType + } - var ( - body []byte - err error - ) + isMultipart := strings.HasPrefix(contentType, "multipart/form-data;") - // set contentType to IncomingPayloadContentType or header value - contentType := r.Header.Get("Content-Type") - if len(matchedHook.IncomingPayloadContentType) != 0 { - contentType = matchedHook.IncomingPayloadContentType + if !isMultipart { + body, err = ioutil.ReadAll(r.Body) + if err != nil { + log.Printf("[%s] error reading the request body: %+v\n", rid, err) + } + } + + // parse headers + headers := valuesToMap(r.Header) + + // parse query variables + query := valuesToMap(r.URL.Query()) + + // parse body + var payload map[string]interface{} + + switch { + case strings.Contains(contentType, "json"): + decoder := json.NewDecoder(bytes.NewReader(body)) + decoder.UseNumber() + + err := decoder.Decode(&payload) + if err != nil { + log.Printf("[%s] error parsing JSON payload %+v\n", rid, err) } - isMultipart := strings.HasPrefix(contentType, "multipart/form-data;") - - if !isMultipart { - body, err = ioutil.ReadAll(r.Body) - if err != nil { - log.Printf("[%s] error reading the request body: %+v\n", rid, err) - } + 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) } - // parse headers - headers := valuesToMap(r.Header) + case strings.Contains(contentType, "xml"): + payload, err = mxj.NewMapXmlReader(bytes.NewReader(body)) + if err != nil { + log.Printf("[%s] error parsing XML payload: %+v\n", rid, err) + } - // parse query variables - query := valuesToMap(r.URL.Query()) + case isMultipart: + err = r.ParseMultipartForm(*maxMultipartMem) + if err != nil { + msg := fmt.Sprintf("[%s] error parsing multipart form: %+v\n", rid, err) + log.Println(msg) + w.WriteHeader(http.StatusInternalServerError) + fmt.Fprint(w, "Error occurred while parsing multipart form.") + return + } - // parse body - var payload map[string]interface{} + for k, v := range r.MultipartForm.Value { + log.Printf("[%s] found multipart form value %q", rid, k) - switch { - case strings.Contains(contentType, "json"): - decoder := json.NewDecoder(bytes.NewReader(body)) - decoder.UseNumber() - - err := decoder.Decode(&payload) - if err != nil { - log.Printf("[%s] error parsing JSON payload %+v\n", rid, err) + if payload == nil { + payload = make(map[string]interface{}) } - 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) - } + // TODO(moorereason): support duplicate, named values + payload[k] = v[0] + } - case strings.Contains(contentType, "xml"): - payload, err = mxj.NewMapXmlReader(bytes.NewReader(body)) - if err != nil { - log.Printf("[%s] error parsing XML payload: %+v\n", rid, err) - } - - case isMultipart: - err = r.ParseMultipartForm(*maxMultipartMem) - if err != nil { - msg := fmt.Sprintf("[%s] error parsing multipart form: %+v\n", rid, err) - log.Println(msg) - w.WriteHeader(http.StatusInternalServerError) - fmt.Fprint(w, "Error occurred while parsing multipart form.") - return - } - - for k, v := range r.MultipartForm.Value { - log.Printf("[%s] found multipart form value %q", rid, k) - - if payload == nil { - payload = make(map[string]interface{}) + for k, v := range r.MultipartForm.File { + // Force parsing as JSON regardless of Content-Type. + var parseAsJSON bool + for _, j := range matchedHook.JSONStringParameters { + if j.Source == "payload" && j.Name == k { + parseAsJSON = true + break } - - // TODO(moorereason): support duplicate, named values - payload[k] = v[0] } - for k, v := range r.MultipartForm.File { - // Force parsing as JSON regardless of Content-Type. - var parseAsJSON bool - for _, j := range matchedHook.JSONStringParameters { - if j.Source == "payload" && j.Name == k { + // TODO(moorereason): we need to support multiple parts + // with the same name instead of just processing the first + // one. Will need #215 resolved first. + + // MIME encoding can contain duplicate headers, so check them + // all. + if !parseAsJSON && len(v[0].Header["Content-Type"]) > 0 { + for _, j := range v[0].Header["Content-Type"] { + if j == "application/json" { parseAsJSON = true break } } - - // TODO(moorereason): we need to support multiple parts - // with the same name instead of just processing the first - // one. Will need #215 resolved first. - - // MIME encoding can contain duplicate headers, so check them - // all. - if !parseAsJSON && len(v[0].Header["Content-Type"]) > 0 { - for _, j := range v[0].Header["Content-Type"] { - if j == "application/json" { - parseAsJSON = true - break - } - } - } - - if parseAsJSON { - log.Printf("[%s] parsing multipart form file %q as JSON\n", rid, k) - - f, err := v[0].Open() - if err != nil { - msg := fmt.Sprintf("[%s] error parsing multipart form file: %+v\n", rid, err) - log.Println(msg) - w.WriteHeader(http.StatusInternalServerError) - fmt.Fprint(w, "Error occurred while parsing multipart form file.") - return - } - - decoder := json.NewDecoder(f) - decoder.UseNumber() - - var part map[string]interface{} - err = decoder.Decode(&part) - if err != nil { - log.Printf("[%s] error parsing JSON payload file: %+v\n", rid, err) - } - - if payload == nil { - payload = make(map[string]interface{}) - } - payload[k] = part - } } - default: - log.Printf("[%s] error parsing body payload due to unsupported content type header: %s\n", rid, contentType) - } - - // handle hook - errors := matchedHook.ParseJSONParameters(&headers, &query, &payload) - for _, err := range errors { - log.Printf("[%s] error parsing JSON parameters: %s\n", rid, err) - } - - var ok bool - - if matchedHook.TriggerRule == nil { - ok = true - } else { - 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.Println(msg) - w.WriteHeader(http.StatusInternalServerError) - fmt.Fprint(w, "Error occurred while evaluating hook rules.") - return - } - } - - if ok { - log.Printf("[%s] %s hook triggered successfully\n", rid, matchedHook.ID) - - for _, responseHeader := range matchedHook.ResponseHeaders { - w.Header().Set(responseHeader.Name, responseHeader.Value) - } - - if matchedHook.CaptureCommandOutput { - response, err := handleHook(matchedHook, rid, &headers, &query, &payload, &body) + if parseAsJSON { + log.Printf("[%s] parsing multipart form file %q as JSON\n", rid, k) + f, err := v[0].Open() if err != nil { + msg := fmt.Sprintf("[%s] error parsing multipart form file: %+v\n", rid, err) + log.Println(msg) w.WriteHeader(http.StatusInternalServerError) - if matchedHook.CaptureCommandOutputOnError { - fmt.Fprint(w, response) - } else { - w.Header().Set("Content-Type", "text/plain; charset=utf-8") - fmt.Fprint(w, "Error occurred while executing the hook's command. Please check your logs for more details.") - } - } else { - // Check if a success return code is configured for the hook - if matchedHook.SuccessHttpResponseCode != 0 { - writeHttpResponseCode(w, rid, matchedHook.ID, matchedHook.SuccessHttpResponseCode) - } + fmt.Fprint(w, "Error occurred while parsing multipart form file.") + return + } + + decoder := json.NewDecoder(f) + decoder.UseNumber() + + var part map[string]interface{} + err = decoder.Decode(&part) + if err != nil { + log.Printf("[%s] error parsing JSON payload file: %+v\n", rid, err) + } + + if payload == nil { + payload = make(map[string]interface{}) + } + payload[k] = part + } + } + + default: + log.Printf("[%s] error parsing body payload due to unsupported content type header: %s\n", rid, contentType) + } + + // handle hook + errors := matchedHook.ParseJSONParameters(&headers, &query, &payload) + for _, err := range errors { + log.Printf("[%s] error parsing JSON parameters: %s\n", rid, err) + } + + var ok bool + + if matchedHook.TriggerRule == nil { + ok = true + } else { + 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.Println(msg) + w.WriteHeader(http.StatusInternalServerError) + fmt.Fprint(w, "Error occurred while evaluating hook rules.") + return + } + } + + if ok { + log.Printf("[%s] %s hook triggered successfully\n", rid, matchedHook.ID) + + for _, responseHeader := range matchedHook.ResponseHeaders { + w.Header().Set(responseHeader.Name, responseHeader.Value) + } + + if matchedHook.CaptureCommandOutput { + response, err := handleHook(matchedHook, rid, &headers, &query, &payload, &body) + + if err != nil { + w.WriteHeader(http.StatusInternalServerError) + if matchedHook.CaptureCommandOutputOnError { fmt.Fprint(w, response) + } else { + w.Header().Set("Content-Type", "text/plain; charset=utf-8") + fmt.Fprint(w, "Error occurred while executing the hook's command. Please check your logs for more details.") } } else { - go handleHook(matchedHook, rid, &headers, &query, &payload, &body) - // Check if a success return code is configured for the hook if matchedHook.SuccessHttpResponseCode != 0 { writeHttpResponseCode(w, rid, matchedHook.ID, matchedHook.SuccessHttpResponseCode) } - - fmt.Fprint(w, matchedHook.ResponseMessage) + fmt.Fprint(w, response) } - return + } else { + go handleHook(matchedHook, rid, &headers, &query, &payload, &body) + + // Check if a success return code is configured for the hook + if matchedHook.SuccessHttpResponseCode != 0 { + writeHttpResponseCode(w, rid, matchedHook.ID, matchedHook.SuccessHttpResponseCode) + } + + fmt.Fprint(w, matchedHook.ResponseMessage) } - - // Check if a return code is configured for the hook - if matchedHook.TriggerRuleMismatchHttpResponseCode != 0 { - writeHttpResponseCode(w, rid, matchedHook.ID, matchedHook.TriggerRuleMismatchHttpResponseCode) - } - - // if none of the hooks got triggered - log.Printf("[%s] %s got matched, but didn't get triggered because the trigger rules were not satisfied\n", rid, matchedHook.ID) - - fmt.Fprint(w, "Hook rules were not satisfied.") - } else { - w.WriteHeader(http.StatusNotFound) - fmt.Fprint(w, "Hook not found.") + return } + + // Check if a return code is configured for the hook + if matchedHook.TriggerRuleMismatchHttpResponseCode != 0 { + writeHttpResponseCode(w, rid, matchedHook.ID, matchedHook.TriggerRuleMismatchHttpResponseCode) + } + + // if none of the hooks got triggered + log.Printf("[%s] %s got matched, but didn't get triggered because the trigger rules were not satisfied\n", rid, matchedHook.ID) + + fmt.Fprint(w, "Hook rules were not satisfied.") } func handleHook(h *hook.Hook, rid string, headers, query, payload *map[string]interface{}, body *[]byte) (string, error) { diff --git a/webhook_test.go b/webhook_test.go index f7ba36b..3e5217c 100644 --- a/webhook_test.go +++ b/webhook_test.go @@ -95,7 +95,7 @@ func TestWebhook(t *testing.T) { url := fmt.Sprintf("http://%s:%s/hooks/%s", ip, port, tt.id) - req, err := http.NewRequest("POST", url, ioutil.NopCloser(strings.NewReader(tt.body))) + req, err := http.NewRequest(tt.method, url, ioutil.NopCloser(strings.NewReader(tt.body))) if err != nil { t.Errorf("New request failed: %s", err) } @@ -289,6 +289,7 @@ func webhookEnv() (env []string) { var hookHandlerTests = []struct { desc string id string + method string headers map[string]string contentType string body string @@ -300,6 +301,7 @@ var hookHandlerTests = []struct { { "github", "github", + "POST", map[string]string{"X-Hub-Signature": "f68df0375d7b03e3eb29b4cf9f9ec12e08f42ff8"}, "application/json", `{ @@ -455,6 +457,7 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 { "bitbucket", // bitbucket sends their payload using uriencoded params. "bitbucket", + "POST", nil, "application/x-www-form-urlencoded", `payload={"canon_url": "https://bitbucket.org","commits": [{"author": "marcus","branch": "master","files": [{"file": "somefile.py","type": "modified"}],"message": "Added some more things to somefile.py\n","node": "620ade18607a","parents": ["702c70160afc"],"raw_author": "Marcus Bertrand ","raw_node": "620ade18607ac42d872b568bb92acaa9a28620e9","revision": null,"size": -1,"timestamp": "2012-05-30 05:58:56","utctimestamp": "2014-11-07 15:19:02+00:00"}],"repository": {"absolute_url": "/webhook/testing/","fork": false,"is_private": true,"name": "Project X","owner": "marcus","scm": "git","slug": "project-x","website": "https://atlassian.com/"},"user": "marcus"}`, @@ -465,6 +468,7 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 { "gitlab", "gitlab", + "POST", map[string]string{"X-Gitlab-Event": "Push Hook"}, "application/json", `{ @@ -517,6 +521,7 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 { "xml", "xml", + "POST", map[string]string{"Content-Type": "application/xml"}, "application/xml", ` @@ -535,6 +540,7 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 { "multipart", "plex", + "POST", nil, "multipart/form-data; boundary=xxx", `--xxx @@ -566,6 +572,7 @@ binary data { "missing-cmd-arg", // missing head_commit.author.email "github", + "POST", map[string]string{"X-Hub-Signature": "ab03955b9377f530aa298b1b6d273ae9a47e1e40"}, "application/json", `{ @@ -607,6 +614,7 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 { "missing-env-arg", // missing head_commit.timestamp "github", + "POST", map[string]string{"X-Hub-Signature": "2cf8b878cb6b74a25090a140fa4a474be04b97fa"}, "application/json", `{ @@ -643,25 +651,27 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 ``, }, + // test with disallowed HTTP method + {"disallowed method", "github", "GET", nil, `{}`, "application/json", http.StatusMethodNotAllowed, ``, ``}, // test with custom return code - {"empty payload", "github", nil, "application/json", `{}`, http.StatusBadRequest, `Hook rules were not satisfied.`, ``}, + {"empty payload", "github", "POST", nil, "application/json", `{}`, http.StatusBadRequest, `Hook rules were not satisfied.`, ``}, // test with custom invalid http code, should default to 200 OK - {"empty payload", "bitbucket", nil, "application/json", `{}`, http.StatusOK, `Hook rules were not satisfied.`, ``}, + {"empty payload", "bitbucket", "POST", nil, "application/json", `{}`, http.StatusOK, `Hook rules were not satisfied.`, ``}, // test with no configured http return code, should default to 200 OK - {"empty payload", "gitlab", nil, "application/json", `{}`, http.StatusOK, `Hook rules were not satisfied.`, ``}, + {"empty payload", "gitlab", "POST", nil, "application/json", `{}`, 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, "application/json", `{}`, http.StatusOK, ``, ``}, - {"capture output on success with flag set", "capture-command-output-on-success-yes-with-flag", nil, "application/json", `{}`, http.StatusOK, `arg: exit=0 + {"don't capture output on success by default", "capture-command-output-on-success-not-by-default", "POST", nil, "application/json", `{}`, http.StatusOK, ``, ``}, + {"capture output on success with flag set", "capture-command-output-on-success-yes-with-flag", "POST", nil, "application/json", `{}`, http.StatusOK, `arg: exit=0 `, ``}, - {"don't capture output on error by default", "capture-command-output-on-error-not-by-default", nil, "application/json", `{}`, 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, "application/json", `{}`, http.StatusInternalServerError, `arg: exit=1 + {"don't capture output on error by default", "capture-command-output-on-error-not-by-default", "POST", nil, "application/json", `{}`, 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", "POST", nil, "application/json", `{}`, http.StatusInternalServerError, `arg: exit=1 `, ``}, // Check logs - {"static params should pass", "static-params-ok", nil, "application/json", `{}`, http.StatusOK, "arg: passed\n", `(?s)command output: arg: passed`}, - {"command with space logs warning", "warn-on-space", nil, "application/json", `{}`, 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"}, "application/json", `{}`, http.StatusBadRequest, `Hook rules were not satisfied.`, `(?s)error parsing body payload due to unsupported content type header:`}, + {"static params should pass", "static-params-ok", "POST", nil, "application/json", `{}`, http.StatusOK, "arg: passed\n", `(?s)command output: arg: passed`}, + {"command with space logs warning", "warn-on-space", "POST", nil, "application/json", `{}`, 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", "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:`}, } // buffer provides a concurrency-safe bytes.Buffer to tests above. From c38778ba6202aeccb55621c7432f5302218423ef Mon Sep 17 00:00:00 2001 From: Cameron Moore Date: Thu, 26 Dec 2019 11:29:33 -0600 Subject: [PATCH 2/6] Add HTTP methods cli parameter Allows to globally restrict HTTP methods. Fixes #248 --- docs/Webhook-Parameters.md | 2 ++ webhook.go | 8 +++++++- webhook_test.go | 36 +++++++++++++++++++++++++----------- 3 files changed, 34 insertions(+), 12 deletions(-) diff --git a/docs/Webhook-Parameters.md b/docs/Webhook-Parameters.md index 7d5beed..b3ba27f 100644 --- a/docs/Webhook-Parameters.md +++ b/docs/Webhook-Parameters.md @@ -13,6 +13,8 @@ Usage of webhook: path to the json file containing defined hooks the webhook should serve, use multiple times to load from different files -hotreload watch hooks file for changes and reload them automatically + -http-methods string + globally restrict allowed HTTP methods; separate methods with comma -ip string ip the webhook should serve hooks on (default "0.0.0.0") -key string diff --git a/webhook.go b/webhook.go index 9b34598..f1dc5da 100644 --- a/webhook.go +++ b/webhook.go @@ -50,6 +50,7 @@ var ( maxMultipartMem = flag.Int64("max-multipart-mem", 1<<20, "maximum memory in bytes for parsing multipart form data before disk caching") setGID = flag.Int("setgid", 0, "set group ID after opening listening port; must be used with setuid") setUID = flag.Int("setuid", 0, "set user ID after opening listening port; must be used with setgid") + httpMethods = flag.String("http-methods", "", "globally restrict allowed HTTP methods; separate methods with comma") responseHeaders hook.ResponseHeaders hooksFiles hook.HooksFiles @@ -203,7 +204,12 @@ func main() { fmt.Fprint(w, "OK") }) - r.HandleFunc(hooksURL, hookHandler) + if *httpMethods == "" { + r.HandleFunc(hooksURL, hookHandler) + } else { + allowed := strings.Split(*httpMethods, ",") + r.HandleFunc(hooksURL, hookHandler).Methods(allowed...) + } addr := fmt.Sprintf("%s:%d", *ip, *port) diff --git a/webhook_test.go b/webhook_test.go index 3e5217c..7bdfab7 100644 --- a/webhook_test.go +++ b/webhook_test.go @@ -79,6 +79,10 @@ func TestWebhook(t *testing.T) { ip, port := serverAddress(t) args := []string{fmt.Sprintf("-hooks=%s", configPath), fmt.Sprintf("-ip=%s", ip), fmt.Sprintf("-port=%s", port), "-verbose"} + if len(tt.cliMethods) != 0 { + args = append(args, "-http-methods="+strings.Join(tt.cliMethods, ",")) + } + // Setup a buffer for capturing webhook logs for later evaluation b := &buffer{} @@ -289,6 +293,7 @@ func webhookEnv() (env []string) { var hookHandlerTests = []struct { desc string id string + cliMethods []string method string headers map[string]string contentType string @@ -301,6 +306,7 @@ var hookHandlerTests = []struct { { "github", "github", + nil, "POST", map[string]string{"X-Hub-Signature": "f68df0375d7b03e3eb29b4cf9f9ec12e08f42ff8"}, "application/json", @@ -457,6 +463,7 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 { "bitbucket", // bitbucket sends their payload using uriencoded params. "bitbucket", + nil, "POST", nil, "application/x-www-form-urlencoded", @@ -468,6 +475,7 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 { "gitlab", "gitlab", + nil, "POST", map[string]string{"X-Gitlab-Event": "Push Hook"}, "application/json", @@ -521,6 +529,7 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 { "xml", "xml", + nil, "POST", map[string]string{"Content-Type": "application/xml"}, "application/xml", @@ -540,6 +549,7 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 { "multipart", "plex", + nil, "POST", nil, "multipart/form-data; boundary=xxx", @@ -572,6 +582,7 @@ binary data { "missing-cmd-arg", // missing head_commit.author.email "github", + nil, "POST", map[string]string{"X-Hub-Signature": "ab03955b9377f530aa298b1b6d273ae9a47e1e40"}, "application/json", @@ -614,6 +625,7 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 { "missing-env-arg", // missing head_commit.timestamp "github", + nil, "POST", map[string]string{"X-Hub-Signature": "2cf8b878cb6b74a25090a140fa4a474be04b97fa"}, "application/json", @@ -651,27 +663,29 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 ``, }, + // test with disallowed global HTTP method + {"global disallowed method", "bitbucket", []string{"POST"}, "GET", nil, `{}`, "application/json", http.StatusMethodNotAllowed, ``, ``}, // test with disallowed HTTP method - {"disallowed method", "github", "GET", nil, `{}`, "application/json", http.StatusMethodNotAllowed, ``, ``}, + {"disallowed method", "github", nil, "GET", nil, `{}`, "application/json", http.StatusMethodNotAllowed, ``, ``}, // test with custom return code - {"empty payload", "github", "POST", nil, "application/json", `{}`, http.StatusBadRequest, `Hook rules were not satisfied.`, ``}, + {"empty payload", "github", nil, "POST", nil, "application/json", `{}`, http.StatusBadRequest, `Hook rules were not satisfied.`, ``}, // test with custom invalid http code, should default to 200 OK - {"empty payload", "bitbucket", "POST", nil, "application/json", `{}`, http.StatusOK, `Hook rules were not satisfied.`, ``}, + {"empty payload", "bitbucket", nil, "POST", nil, "application/json", `{}`, http.StatusOK, `Hook rules were not satisfied.`, ``}, // test with no configured http return code, should default to 200 OK - {"empty payload", "gitlab", "POST", nil, "application/json", `{}`, http.StatusOK, `Hook rules were not satisfied.`, ``}, + {"empty payload", "gitlab", nil, "POST", nil, "application/json", `{}`, 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", "POST", nil, "application/json", `{}`, http.StatusOK, ``, ``}, - {"capture output on success with flag set", "capture-command-output-on-success-yes-with-flag", "POST", nil, "application/json", `{}`, http.StatusOK, `arg: exit=0 + {"don't capture output on success by default", "capture-command-output-on-success-not-by-default", nil, "POST", nil, "application/json", `{}`, http.StatusOK, ``, ``}, + {"capture output on success with flag set", "capture-command-output-on-success-yes-with-flag", nil, "POST", nil, "application/json", `{}`, http.StatusOK, `arg: exit=0 `, ``}, - {"don't capture output on error by default", "capture-command-output-on-error-not-by-default", "POST", nil, "application/json", `{}`, 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", "POST", nil, "application/json", `{}`, http.StatusInternalServerError, `arg: exit=1 + {"don't capture output on error by default", "capture-command-output-on-error-not-by-default", nil, "POST", nil, "application/json", `{}`, 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, "POST", nil, "application/json", `{}`, http.StatusInternalServerError, `arg: exit=1 `, ``}, // Check logs - {"static params should pass", "static-params-ok", "POST", nil, "application/json", `{}`, http.StatusOK, "arg: passed\n", `(?s)command output: arg: passed`}, - {"command with space logs warning", "warn-on-space", "POST", nil, "application/json", `{}`, 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", "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:`}, + {"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)unable to locate command.*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:`}, } // buffer provides a concurrency-safe bytes.Buffer to tests above. From a03e8126157a8cba744f22d6c28cb1e9fd4d163c Mon Sep 17 00:00:00 2001 From: Cameron Moore Date: Thu, 26 Dec 2019 14:51:40 -0600 Subject: [PATCH 3/6] Update HTTP methods to sanitize user input --- internal/hook/hook.go | 3 +-- test/hooks.json.tmpl | 2 +- test/hooks.yaml.tmpl | 2 +- webhook.go | 11 ++++++++++- webhook_test.go | 4 ++-- 5 files changed, 15 insertions(+), 7 deletions(-) diff --git a/internal/hook/hook.go b/internal/hook/hook.go index 0e4d70f..1131eb4 100644 --- a/internal/hook/hook.go +++ b/internal/hook/hook.go @@ -649,8 +649,7 @@ func (h *Hooks) LoadFromFile(path string, asTemplate bool) error { file = buf.Bytes() } - e = yaml.Unmarshal(file, h) - return e + return yaml.Unmarshal(file, h) } // Append appends hooks unless the new hooks contain a hook with an ID that already exists diff --git a/test/hooks.json.tmpl b/test/hooks.json.tmpl index e91f663..0c7fc0c 100644 --- a/test/hooks.json.tmpl +++ b/test/hooks.json.tmpl @@ -3,7 +3,7 @@ "id": "github", "execute-command": "{{ .Hookecho }}", "command-working-directory": "/", - "http-methods": ["POST"], + "http-methods": ["Post "], "include-command-output-in-response": true, "trigger-rule-mismatch-http-response-code": 400, "pass-environment-to-command": diff --git a/test/hooks.yaml.tmpl b/test/hooks.yaml.tmpl index 79e8987..f1e1204 100644 --- a/test/hooks.yaml.tmpl +++ b/test/hooks.yaml.tmpl @@ -1,6 +1,6 @@ - id: github http-methods: - - POST + - "Post " trigger-rule: and: - match: diff --git a/webhook.go b/webhook.go index f1dc5da..5bf6f3d 100644 --- a/webhook.go +++ b/webhook.go @@ -208,6 +208,10 @@ func main() { r.HandleFunc(hooksURL, hookHandler) } else { allowed := strings.Split(*httpMethods, ",") + for i := range allowed { + allowed[i] = strings.TrimSpace(allowed[i]) + } + r.HandleFunc(hooksURL, hookHandler).Methods(allowed...) } @@ -257,7 +261,7 @@ func main() { func hookHandler(w http.ResponseWriter, r *http.Request) { rid := middleware.GetReqID(r.Context()) - log.Printf("[%s] incoming HTTP request from %s\n", rid, r.RemoteAddr) + log.Printf("[%s] incoming HTTP %s request from %s\n", rid, r.Method, r.RemoteAddr) id := mux.Vars(r)["id"] @@ -272,6 +276,10 @@ func hookHandler(w http.ResponseWriter, r *http.Request) { if len(matchedHook.HTTPMethods) != 0 { var allowed bool for i := range matchedHook.HTTPMethods { + // TODO(moorereason): refactor config loading and reloading to + // sanitize these methods once at load time. + matchedHook.HTTPMethods[i] = strings.ToUpper(strings.TrimSpace(matchedHook.HTTPMethods[i])) + if matchedHook.HTTPMethods[i] == r.Method { allowed = true break @@ -280,6 +288,7 @@ func hookHandler(w http.ResponseWriter, r *http.Request) { if !allowed { w.WriteHeader(http.StatusMethodNotAllowed) + log.Printf("[%s] HTTP %s method not implemented for hook %q", rid, r.Method, id) return } } diff --git a/webhook_test.go b/webhook_test.go index 7bdfab7..8b53981 100644 --- a/webhook_test.go +++ b/webhook_test.go @@ -664,9 +664,9 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 }, // test with disallowed global HTTP method - {"global disallowed method", "bitbucket", []string{"POST"}, "GET", nil, `{}`, "application/json", http.StatusMethodNotAllowed, ``, ``}, + {"global disallowed method", "bitbucket", []string{"Post "}, "GET", nil, `{}`, "application/json", http.StatusMethodNotAllowed, ``, ``}, // test with disallowed HTTP method - {"disallowed method", "github", nil, "GET", nil, `{}`, "application/json", http.StatusMethodNotAllowed, ``, ``}, + {"disallowed method", "github", nil, "Get", nil, `{}`, "application/json", http.StatusMethodNotAllowed, ``, ``}, // test with custom return code {"empty payload", "github", nil, "POST", nil, "application/json", `{}`, http.StatusBadRequest, `Hook rules were not satisfied.`, ``}, // test with custom invalid http code, should default to 200 OK From e1249a9ddbdd00dd6cf74cee02351fda5c9f2f8a Mon Sep 17 00:00:00 2001 From: Cameron Moore Date: Thu, 26 Dec 2019 15:17:01 -0600 Subject: [PATCH 4/6] Add global HTTP methods to starting log message --- webhook.go | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/webhook.go b/webhook.go index 5bf6f3d..13b5595 100644 --- a/webhook.go +++ b/webhook.go @@ -204,15 +204,17 @@ func main() { fmt.Fprint(w, "OK") }) + var allowedMethods []string + if *httpMethods == "" { r.HandleFunc(hooksURL, hookHandler) } else { - allowed := strings.Split(*httpMethods, ",") - for i := range allowed { - allowed[i] = strings.TrimSpace(allowed[i]) + allowedMethods = strings.Split(*httpMethods, ",") + for i := range allowedMethods { + allowedMethods[i] = strings.TrimSpace(allowedMethods[i]) } - r.HandleFunc(hooksURL, hookHandler).Methods(allowed...) + r.HandleFunc(hooksURL, hookHandler).Methods(allowedMethods...) } addr := fmt.Sprintf("%s:%d", *ip, *port) @@ -240,7 +242,12 @@ func main() { // Serve HTTP if !*secure { - log.Printf("serving hooks on http://%s%s", addr, hooksURL) + if len(allowedMethods) == 0 { + log.Printf("serving hooks on http://%s%s", addr, hooksURL) + } else { + log.Printf("serving hooks on http://%s%s for %s", addr, hooksURL, strings.Join(allowedMethods, ", ")) + } + log.Print(svr.Serve(ln)) return } @@ -254,7 +261,11 @@ func main() { } svr.TLSNextProto = make(map[string]func(*http.Server, *tls.Conn, http.Handler)) // disable http/2 - log.Printf("serving hooks on https://%s%s", addr, hooksURL) + if len(allowedMethods) == 0 { + log.Printf("serving hooks on https://%s%s", addr, hooksURL) + } else { + log.Printf("serving hooks on https://%s%s for %s", addr, hooksURL, strings.Join(allowedMethods, ", ")) + } log.Print(svr.ServeTLS(ln, *cert, *key)) } From 157f468e0cf3ca17c23877eed2015ffdcb00c5fa Mon Sep 17 00:00:00 2001 From: Cameron Moore Date: Fri, 27 Dec 2019 11:22:04 -0600 Subject: [PATCH 5/6] Refactor cli HTTP methods behavior The CLI HTTP methods option now sets the default allowed methods while allowing an individual hook definition to override the default. --- webhook.go | 64 +++++++++++++++++++++++++----------------------------- 1 file changed, 29 insertions(+), 35 deletions(-) diff --git a/webhook.go b/webhook.go index 13b5595..5df2cfd 100644 --- a/webhook.go +++ b/webhook.go @@ -50,7 +50,7 @@ var ( maxMultipartMem = flag.Int64("max-multipart-mem", 1<<20, "maximum memory in bytes for parsing multipart form data before disk caching") setGID = flag.Int("setgid", 0, "set group ID after opening listening port; must be used with setuid") setUID = flag.Int("setuid", 0, "set user ID after opening listening port; must be used with setgid") - httpMethods = flag.String("http-methods", "", "globally restrict allowed HTTP methods; separate methods with comma") + httpMethods = flag.String("http-methods", "", `set default allowed HTTP methods (ie. "POST"); separate methods with comma`) responseHeaders hook.ResponseHeaders hooksFiles hook.HooksFiles @@ -198,24 +198,16 @@ func main() { r.Use(middleware.Dumper(log.Writer())) } + // Clean up input + *httpMethods = strings.ToUpper(strings.ReplaceAll(*httpMethods, " ", "")) + hooksURL := makeURL(hooksURLPrefix) r.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { fmt.Fprint(w, "OK") }) - var allowedMethods []string - - if *httpMethods == "" { - r.HandleFunc(hooksURL, hookHandler) - } else { - allowedMethods = strings.Split(*httpMethods, ",") - for i := range allowedMethods { - allowedMethods[i] = strings.TrimSpace(allowedMethods[i]) - } - - r.HandleFunc(hooksURL, hookHandler).Methods(allowedMethods...) - } + r.HandleFunc(hooksURL, hookHandler) addr := fmt.Sprintf("%s:%d", *ip, *port) @@ -242,13 +234,9 @@ func main() { // Serve HTTP if !*secure { - if len(allowedMethods) == 0 { - log.Printf("serving hooks on http://%s%s", addr, hooksURL) - } else { - log.Printf("serving hooks on http://%s%s for %s", addr, hooksURL, strings.Join(allowedMethods, ", ")) - } - + log.Printf("serving hooks on http://%s%s", addr, hooksURL) log.Print(svr.Serve(ln)) + return } @@ -261,11 +249,7 @@ func main() { } svr.TLSNextProto = make(map[string]func(*http.Server, *tls.Conn, http.Handler)) // disable http/2 - if len(allowedMethods) == 0 { - log.Printf("serving hooks on https://%s%s", addr, hooksURL) - } else { - log.Printf("serving hooks on https://%s%s for %s", addr, hooksURL, strings.Join(allowedMethods, ", ")) - } + log.Printf("serving hooks on https://%s%s", addr, hooksURL) log.Print(svr.ServeTLS(ln, *cert, *key)) } @@ -284,24 +268,34 @@ func hookHandler(w http.ResponseWriter, r *http.Request) { } // Check for allowed methods - if len(matchedHook.HTTPMethods) != 0 { - var allowed bool + var allowedMethod bool + + switch { + case len(matchedHook.HTTPMethods) != 0: for i := range matchedHook.HTTPMethods { // TODO(moorereason): refactor config loading and reloading to // sanitize these methods once at load time. - matchedHook.HTTPMethods[i] = strings.ToUpper(strings.TrimSpace(matchedHook.HTTPMethods[i])) - - if matchedHook.HTTPMethods[i] == r.Method { - allowed = true + if r.Method == strings.ToUpper(strings.TrimSpace(matchedHook.HTTPMethods[i])) { + allowedMethod = true break } } - - if !allowed { - w.WriteHeader(http.StatusMethodNotAllowed) - log.Printf("[%s] HTTP %s method not implemented for hook %q", rid, r.Method, id) - return + case *httpMethods != "": + for _, v := range strings.Split(*httpMethods, ",") { + if r.Method == v { + allowedMethod = true + break + } } + default: + allowedMethod = true + } + + if !allowedMethod { + w.WriteHeader(http.StatusMethodNotAllowed) + log.Printf("[%s] HTTP %s method not implemented for hook %q", rid, r.Method, id) + + return } log.Printf("[%s] %s got matched\n", rid, id) From 811481298ad89019ca41e99d83a42a64299ffced Mon Sep 17 00:00:00 2001 From: Cameron Moore Date: Sat, 28 Dec 2019 07:09:36 -0600 Subject: [PATCH 6/6] Fix method not allowed log message --- webhook.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/webhook.go b/webhook.go index 5df2cfd..06018af 100644 --- a/webhook.go +++ b/webhook.go @@ -293,7 +293,7 @@ func hookHandler(w http.ResponseWriter, r *http.Request) { if !allowedMethod { w.WriteHeader(http.StatusMethodNotAllowed) - log.Printf("[%s] HTTP %s method not implemented for hook %q", rid, r.Method, id) + log.Printf("[%s] HTTP %s method not allowed for hook %q", rid, r.Method, id) return }