From 1c779a0d7566bcf9b1ab4fb648944408613f8f33 Mon Sep 17 00:00:00 2001 From: Cameron Moore Date: Tue, 24 Dec 2019 14:51:49 -0600 Subject: [PATCH] Update multipart form data logic All form values are simply added to the payload map without processing. JSON parsing of values happens later. --- README.md | 23 +++--------- docs/Hook-Examples.md | 48 +++++++----------------- test/hooks.json.tmpl | 24 ++++++++++++ test/hooks.yaml.tmpl | 15 ++++++++ webhook.go | 21 ++++++++--- webhook_test.go | 85 ++++++++++++++++++++++++++++--------------- 6 files changed, 129 insertions(+), 87 deletions(-) diff --git a/README.md b/README.md index 72f3d01..083c36f 100644 --- a/README.md +++ b/README.md @@ -78,27 +78,16 @@ By performing a simple HTTP GET or POST request to that endpoint, your specified However, hook defined like that could pose a security threat to your system, because anyone who knows your endpoint, can send a request and execute your command. To prevent that, you can use the `"trigger-rule"` property for your hook, to specify the exact circumstances under which the hook would be triggered. For example, you can use them to add a secret that you must supply as a parameter in order to successfully trigger the hook. Please check out the [Hook rules page](docs/Hook-Rules.md) for detailed list of available rules and their usage. ## Multipart Form Data - [webhook][w] provides limited support the parsing of multipart form data. -Upon receiving a `multipart/form-data` request, [webhook][w] steps through the named parts and parses all parts that match one of the following criteria: +Multipart form data can contain two types of parts: values and files. +All form _values_ are automatically added to the `payload` scope. +Use the `parse-parameters-as-json` settings to parse a given value as JSON. +All files are ignored unless they match one of the following criteria: -1. The `Content-Type` header is a supported type (only JSON is supported so far). +1. The `Content-Type` header is `application/json`. 1. The part is named in the `parse-parameters-as-json` setting. -The named part will be accessible from the `payload` source by referencing the name of the part. -For example, given a named part of "command", a match rule could look similar to the following: - -```json -{ - "match": { - "type": "value", - "parameter": { - "source": "payload", - "name": "command.event", - }, - "value": "media.play" -} -``` +In either case, the given file part will be parsed as JSON and added to the `payload` map. ## Templates [webhook][w] can parse the `hooks.json` input file as a Go template when given the `-template` [CLI parameter](docs/Webhook-Parameters.md). See the [Templates page](docs/Templates.md) for more details on template usage. diff --git a/docs/Hook-Examples.md b/docs/Hook-Examples.md index 1a95745..23896a7 100644 --- a/docs/Hook-Examples.md +++ b/docs/Hook-Examples.md @@ -479,39 +479,7 @@ Given the following payload: Example of a [Plex Media Server webhook](https://support.plex.tv/articles/115002267687-webhooks/). The Plex Media Server will send two parts: payload and thumb. - -```json -[ - { - "id": "plex", - "execute-command": "play-command.sh", - "trigger-rule": - { - "match": - { - "type": "value", - "parameter": { - "source": "payload" - "name": "payload.event", - }, - "value": "media.play" - } - } - } -] -``` - -Each part of a multipart form data body will have a `Content-Disposition` header. -An example header: - -``` -Content-Disposition: form-data; name="payload"; filename="payload.json" -``` - -We key off of the `name` attribute in the `Content-Disposition` value. - -If the named part is JSON but has an incorrect `Content-Type`, -use the `parse-parameters-as-json` setting to force it to be parsed as JSON: +We only care about the payload part. ```json [ @@ -530,8 +498,8 @@ use the `parse-parameters-as-json` setting to force it to be parsed as JSON: { "type": "value", "parameter": { - "source": "payload" - "name": "payload.event", + "source": "payload", + "name": "payload.event" }, "value": "media.play" } @@ -540,3 +508,13 @@ use the `parse-parameters-as-json` setting to force it to be parsed as JSON: ] ``` + +Each part of a multipart form data body will have a `Content-Disposition` header. +Some example headers: + +``` +Content-Disposition: form-data; name="payload" +Content-Disposition: form-data; name="thumb"; filename="thumb.jpg" +``` + +We key off of the `name` attribute in the `Content-Disposition` value. diff --git a/test/hooks.json.tmpl b/test/hooks.json.tmpl index 9661472..40c6de5 100644 --- a/test/hooks.json.tmpl +++ b/test/hooks.json.tmpl @@ -167,6 +167,30 @@ ], } }, + { + "id": "plex", + "execute-command": "{{ .Hookecho }}", + "command-working-directory": "/", + "response-message": "success", + "parse-parameters-as-json": [ + { + "source": "payload", + "name": "payload" + } + ], + "trigger-rule": + { + "match": + { + "type": "value", + "parameter": { + "source": "payload", + "name": "payload.event" + }, + "value": "media.play" + } + } + }, { "id": "capture-command-output-on-success-not-by-default", "pass-arguments-to-command": [ diff --git a/test/hooks.yaml.tmpl b/test/hooks.yaml.tmpl index cf90471..bfeacc2 100644 --- a/test/hooks.yaml.tmpl +++ b/test/hooks.yaml.tmpl @@ -95,6 +95,21 @@ name: "app.messages.message.#text" value: "Hello!!" +- id: plex + trigger-rule: + match: + type: value + parameter: + source: payload + name: payload.event + value: media.play + parse-parameters-as-json: + - source: payload + name: payload + execute-command: '{{ .Hookecho }}' + response-message: success + command-working-directory: / + - id: capture-command-output-on-success-not-by-default pass-arguments-to-command: - source: string diff --git a/webhook.go b/webhook.go index 7418c6d..ca53a7d 100644 --- a/webhook.go +++ b/webhook.go @@ -294,6 +294,17 @@ func hookHandler(w http.ResponseWriter, r *http.Request) { 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{}) + } + + // 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 @@ -304,7 +315,7 @@ func hookHandler(w http.ResponseWriter, r *http.Request) { } } - // FIXME(moorereason): we need to support multiple parts + // TODO(moorereason): we need to support multiple parts // with the same name instead of just processing the first // one. Will need #215 resolved first. @@ -320,14 +331,14 @@ func hookHandler(w http.ResponseWriter, r *http.Request) { } if parseAsJSON { - log.Printf("[%s] parsing multipart part %q as JSON\n", rid, k) + 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 part: %+v\n", rid, err) + 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 part.") + fmt.Fprint(w, "Error occurred while parsing multipart form file.") return } @@ -337,7 +348,7 @@ func hookHandler(w http.ResponseWriter, r *http.Request) { var part map[string]interface{} err = decoder.Decode(&part) if err != nil { - log.Printf("[%s] error parsing JSON payload part: %+v\n", rid, err) + log.Printf("[%s] error parsing JSON payload file: %+v\n", rid, err) } if payload == nil { diff --git a/webhook_test.go b/webhook_test.go index adcfd3c..f7ba36b 100644 --- a/webhook_test.go +++ b/webhook_test.go @@ -75,7 +75,7 @@ func TestWebhook(t *testing.T) { defer cleanupConfigFn() for _, tt := range hookHandlerTests { - t.Run(tt.desc, func(t *testing.T) { + t.Run(tt.desc+"@"+hookTmpl, func(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"} @@ -106,13 +106,7 @@ func TestWebhook(t *testing.T) { var res *http.Response - if tt.urlencoded == true { - req.Header.Add("Content-Type", "application/x-www-form-urlencoded") - } else { - if req.Header.Get("Content-Type") == "" { - req.Header.Add("Content-Type", "application/json") - } - } + req.Header.Add("Content-Type", tt.contentType) client := &http.Client{} res, err = client.Do(req) @@ -127,7 +121,7 @@ func TestWebhook(t *testing.T) { } 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) + t.Errorf("failed %q (id: %s):\nexpected status: %#v, response: %s\ngot status: %#v, response: %s\ncommand output:\n%s\n", tt.desc, tt.id, tt.respStatus, tt.respBody, res.StatusCode, body, b) } if tt.logMatch == "" { @@ -293,11 +287,11 @@ func webhookEnv() (env []string) { } var hookHandlerTests = []struct { - desc string - id string - headers map[string]string - body string - urlencoded bool + desc string + id string + headers map[string]string + contentType string + body string respStatus int respBody string @@ -307,6 +301,7 @@ var hookHandlerTests = []struct { "github", "github", map[string]string{"X-Hub-Signature": "f68df0375d7b03e3eb29b4cf9f9ec12e08f42ff8"}, + "application/json", `{ "after":"1481a2de7b2a7d02428ad93446ab166be7793fbb", "before":"17c497ccc7cca9c2f735aa07e9e3813060ce9a6a", @@ -451,7 +446,6 @@ var hookHandlerTests = []struct { "watchers":1 } }`, - false, http.StatusOK, `arg: 1481a2de7b2a7d02428ad93446ab166be7793fbb lolwut@noway.biz env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 @@ -462,8 +456,8 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 "bitbucket", // bitbucket sends their payload using uriencoded params. "bitbucket", 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"}`, - true, http.StatusOK, `success`, ``, @@ -472,6 +466,7 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 "gitlab", "gitlab", map[string]string{"X-Gitlab-Event": "Push Hook"}, + "application/json", `{ "object_kind": "push", "before": "95790bf891e76fee5e1747ab589903a6a1f80f22", @@ -514,7 +509,6 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 ], "total_commits_count": 4 }`, - false, http.StatusOK, `arg: b6568db1bc1dcd7f8b4d5a946b0b91f9dacd7327 John Smith john@example.com `, @@ -524,6 +518,7 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 "xml", "xml", map[string]string{"Content-Type": "application/xml"}, + "application/xml", ` @@ -533,15 +528,46 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 Hello!! `, - false, http.StatusOK, `success`, ``, }, + { + "multipart", + "plex", + nil, + "multipart/form-data; boundary=xxx", + `--xxx +Content-Disposition: form-data; name="payload" + +{ + "event": "media.play", + "user": true, + "owner": true, + "Account": { + "id": 1, + "thumb": "https://plex.tv/users/1022b120ffbaa/avatar?c=1465525047", + "title": "elan" + } +} + +--xxx +Content-Disposition: form-data; name="thumb"; filename="thumb.jpg" +Content-Type: application/octet-stream +Content-Transfer-Encoding: binary + +binary data +--xxx--`, + http.StatusOK, + `success`, + ``, + }, + { "missing-cmd-arg", // missing head_commit.author.email "github", map[string]string{"X-Hub-Signature": "ab03955b9377f530aa298b1b6d273ae9a47e1e40"}, + "application/json", `{ "head_commit":{ "added":[ @@ -571,7 +597,6 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 }, "ref":"refs/heads/master" }`, - false, http.StatusOK, `arg: 1481a2de7b2a7d02428ad93446ab166be7793fbb lolwut@noway.biz env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 @@ -583,6 +608,7 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 "missing-env-arg", // missing head_commit.timestamp "github", map[string]string{"X-Hub-Signature": "2cf8b878cb6b74a25090a140fa4a474be04b97fa"}, + "application/json", `{ "head_commit":{ "added":[ @@ -611,7 +637,6 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 }, "ref":"refs/heads/master" }`, - false, http.StatusOK, `arg: 1481a2de7b2a7d02428ad93446ab166be7793fbb lolwut@noway.biz `, @@ -619,24 +644,24 @@ env: HOOK_head_commit.timestamp=2013-03-12T08:14:29-07:00 }, // test with custom return code - {"empty payload", "github", nil, `{}`, false, http.StatusBadRequest, `Hook rules were not satisfied.`, ``}, + {"empty payload", "github", 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, `{}`, false, http.StatusOK, `Hook rules were not satisfied.`, ``}, + {"empty payload", "bitbucket", 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, `{}`, false, http.StatusOK, `Hook rules were not satisfied.`, ``}, + {"empty payload", "gitlab", 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, `{}`, 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 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 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 + {"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 `, ``}, // 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:`}, + {"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:`}, } // buffer provides a concurrency-safe bytes.Buffer to tests above.