Webhook silently fails when header contains colon

[brennentsmith]

Hi Pinterest,

We're seeing webhooks fail when headers contain colons - we presume it's the line here:

teletraan/deploy-service/common/src/main/java/com/pinterest/deployservice/handler/WebhookJob.java

Line 70 in a4201f2

headers = Splitter.on(';').trimResults().withKeyValueSeparator(":").split(webhook.getHeaders());

Are the headers supposed to be kv pairs split with colons (as the code suggests), or equals signs (as the docs/tooltips) suggest?

An example header string that repros the behavior: Accept=application/json;Content-Type=application/json;Authorization=tok:747703

Log output:

cat service.log | grep "com.pinterest.deployservice.handler.WebhookJob"
INFO  [2016-11-15 11:29:48,397] com.pinterest.deployservice.handler.WebhookJob: Url after transform is https://api.webhook.com
INFO  [2016-11-15 11:29:48,397] com.pinterest.deployservice.handler.WebhookJob: Header string after transform is Accept=application/json;Content-Type=application/json;Authorization=tok:747703
INFO  [2016-11-15 11:29:48,397] com.pinterest.deployservice.handler.WebhookJob: Body string after transform is {"json":"data"}

There is no other entries in the log, so I presume that it is silently failing somewhere between L66 and L78.

[sbaogang]

Thanks for reporting, will take a look.

If you do not provide headers, will it work ( will it call the hooked backend server at all?)

[brennentsmith]

Hi sbaogang,

Thanks again for the response - we made a local patch changing the keyValueSeperator on L70 to "=" (from ":") and it worked great.

If you want, I'd be happy to provide a PR, though if you already have webhooks setup on your instances, it would run the risk of breaking them.

[sbaogang]

the code in L70 does not like:
Authorization:tok:747703
due to two consecutive colons
"java.lang.IllegalArgumentException: Chunk [Authorization: tok:747703] is not a valid entry"

do you have to use this syntax, any chance you could use
Authorization:tok 747703

[sbaogang]

The help/hint in UI is wrong, the splitter name value pair is :
name1:value1;name2:value2, will change that in the UI, thanks for reporting

[sbaogang]

HTTP headers are in the form of name:value, so it is still prefered. We will change the one-line parse code to handle case such as "name: foo:bar", will have the code fix earlier next week.