Add template support #285
Add template support #285
Conversation
| return payload.Msg, nil | ||
| if r.Method == http.MethodGet { | ||
| buf := new(bytes.Buffer) | ||
| if err := t.Execute(buf, r.URL.Query()); err == nil { |
There was a problem hiding this comment.
perhaps the template population should be timeboxed (context.WithTimeout) (and in a separate goroutine) so a malicious template (infinite loop) doesn't halt the bot
There was a problem hiding this comment.
i've spent some time looking into this and thinking about how it would be done and i'm struggling to think of how we would cancel the t.Execute call if it did end up in a situation where there was some kind of infinite loop in a template. we could call it in a goroutine, and we could use context.WithTimeout to know that we're okay to move on after so many seconds of waiting, but i'm not sure how we would actually stop the t.Execute call from continuing to hang?
i also did some tests to see if it's possible to create some kind of malicious template with an infinite loop, and it turns out that if you create a template that ranges over nothing, like {{ range }}{{ end }} it just fails to execute and returns an error, rather than creating an infinite loop the way for{} would. i think overall these templates should be pretty safe, considering the fact that they provide a separate templating package specifically to prevent code injection when dealing with html, which makes me think these templates should be okay to use even if they come from an untrusted source.
if you still think we should timebox this, i'd be interested in your thoughts about how to kill that t.Execute call.
There was a problem hiding this comment.
Would a recursive template like this be possible? https://play.golang.org/p/uMN1IsGB4xl
It might be useful to look at this issue golang/go#31107
There was a problem hiding this comment.
I realized that my playground example might not be super realistic since playground has some pretty strict timing & memory restrictions. It seems like Execute gives up at some depth (100000 from the source).
However, I came up with something that gets killed by the linux OOM killer when writing to a bytes.Buffer if the timeout is too loose. Might be useful to allocate a limited size buffer. I imagine it's still possible to abuse the memory when executing the template though.
There was a problem hiding this comment.
Would a recursive template like this be possible? https://play.golang.org/p/uMN1IsGB4xl
i tried this on an instance of the bot that i'm currently running, and it did not work. i tried it exactly as you have it here, which takes no input from any fields on the webhook, and i also modified it a bit to take a name field. both ways resulted in the following log line when attempting to hit the webhook:
DummyStatsBackend: Count: webhookbot - HTTPSrv - handle - empty msg
but this was a really cool approach. i'm kind of curious why this works in the playground but not the bot. out of curiosity i also put your example in a file and compiled it and it runs in under a second before exiting, and it prints out a bunch of the Don't you just love recursion? lines.
It might be useful to look at this issue golang/go#31107
thank you for this! i skimmed it a but just now but will have to read through it a bit when i have more time. it looks like there may be some good ideas in there that i can use.
It seems like Execute gives up at some depth (100000 from the source).
perhaps this explains the behavior i mentioned abovened above
However, I came up with something that gets killed by the linux OOM killer when writing to a bytes.Buffer if the timeout is too loose. Might be useful to allocate a limited size buffer. I imagine it's still possible to abuse the memory when executing the template though.
i'll look into this, thanks for the suggestion. i'd be interested to see what you came up with.
also, feel free to ping me on keybase and i can point you to the instance i'm running if you want to poke at it.
This PR adds support for webhooks to have templates. The default template will look and act exactly like the bot's current non-template functionality.
This PR does not currently include any migrations, which will be needed to add the new template column before this gets deployed to an existing instance.
I've also fixed a bug that allowed someone to create multiple webhooks with the same name in a channel. Because of this, we may also need an additional migration that finds any channels with multiple webhooks with the same name, and rename them.
/cc @joshblum @mmaxim