Backslash quote in json will crash the trigger parser

[chmouel]

On a git repo which is plugged to a triggers, i have made a standard git revert of a commit to submit to a PR.

The title of git revert would look like this :

    Revert "Detect tkn version"

This will shows up like this in the webhook json payload :

    "title": "Revert \"Detect tkn version\"",

but this title will crash trigger's parser :

{"level":"error","logger":"eventlistener","caller":"sink/sink.go:173","msg":"couldn't unmarshal json: invalid character 'D' after object key:value pair","knative.dev/controller":"eventlistener","/triggers-eventid":"ff46j","/trigger":"github-listene
r","stacktrace":"github.com/tektoncd/triggers/pkg/sink.Sink.processTrigger\n\t/opt/app-root/src/go/src/github.com/tektoncd/triggers/pkg/sink/sink.go:173\ngithub.com/tektoncd/triggers/pkg/sink.Sink.HandleEvent.func1\n\t/opt/app-root/src/go/src/githu
b.com/tektoncd/triggers/pkg/sink/sink.go:94"}

Changing the title and removing the quote made triggers accepting the json payload and go on with its life...

/label bug

[khrm]

/kind bug

[dibyom]

@chmouel what version of Triggers are you running?

[khrm]

@dibyom
He is running version 0.6.1.
I can recreate this on v0.8.1.
/assign

[khrm]

Error in v0.8.1

{"level":"error","logger":"eventlistener","caller":"sink/sink.go:266","msg":"problem creating obj: &errors.errorString{s:\"couldn't unmarshal json: invalid character 'R' after object key:value pair\"}","knative.dev/controller":"eventlistener","/triggers-eventid":"9dvkl","/trigger":"foo-trig","stacktrace":"github.com/tektoncd/triggers/pkg/sink.Sink.CreateResources\n\tgithub.com/tektoncd/triggers/pkg/sink/sink.go:266\ngithub.com/tektoncd/triggers/pkg/sink.Sink.processTrigger\n\tgithub.com/tektoncd/triggers/pkg/sink/sink.go:183\ngithub.com/tektoncd/triggers/pkg/sink.Sink.HandleEvent.func1\n\tgithub.com/tektoncd/triggers/pkg/sink/sink.go:94"}
{"level":"error","logger":"eventlistener","caller":"sink/sink.go:184","msg":"couldn't unmarshal json: invalid character 'R' after object key:value pair","knative.dev/controller":"eventlistener","/triggers-eventid":"9dvkl","/trigger":"foo-trig","stacktrace":"github.com/tektoncd/triggers/pkg/sink.Sink.processTrigger\n\tgithub.com/tektoncd/triggers/pkg/sink/sink.go:184\ngithub.com/tektoncd/triggers/pkg/sink.Sink.HandleEvent.func1\n\tgithub.com/tektoncd/triggers/pkg/sink/sink.go:94"}

[dibyom]

yeah looks like the error is while creating the resource, the actual extraction in binding seems to work.

specifically here: https://github.com/tektoncd/triggers/blob/master/pkg/resources/create.go#L69

[dibyom]

I was looking at #772 and it looks like the same issue. I think this is happening because we are doing some overly simplistic escaping here: https://github.com/tektoncd/triggers/blob/v0.8.1/pkg/template/resource.go#L117 (related #257 )

@khrm let me know if your findings are similar!

[khrm]

Yes, it looks similar. I reached the same conclusion just now. In this case, that escaping breaks the body.

[khrm]

Strangely enough, if I set a param directly with backlash, it works but if that arrives from an event body, it doesn't. Will look into this day after tomorrow.

[bigkevmcd]

Discussed at yesterday's (2020-10-27) Triggers call, to be able to drop the quoting of strings, we need to allow folks to retain the behaviour for at least a minor release or two, adding an annotation to TriggerTemplates that require the previous behaviour feels like the best option.

Simply, the issue is caused by quoting strings that are being inserted at unknown positions in a byte slice, short of introducing a way to indicate that we want a specific replacement to be quoted, we have no context for the replacement (to know if it should be quoted or not).