[Actions] Ability to declare unescaped messageVariables

[cnasikas]

PR #83139 introduced a new way of declaring messageVariables. The type of messageVariables changed from ActionVariable[] to ActionsVariables.

Security solution was providing the parameters as:

return [
    { name: 'state.signals_count', description: 'state.signals_count' },
    { name: '{context.results_link}', description: 'context.results_link' },
    { name: 'context.rule_id', description: 'context.rule_id' },
  ];

With the new formating, it passes them as:

return {
      state: [{ name: 'signals_count', description: 'state.signals_count' }],
      params: [],
      context: [
        { name: 'results_link', description: 'context.results_link' },
        { name: 'rule_id', description: 'context.rule_id' },
      ],
    };

Now, the context., state., and params. are being prefixed automatically by the ActionTypeForm component. So { name: 'rule_id', description: 'context.rule_id' } will become { name: 'context.rule_id', description: 'context.rule_id' }. Because the logic of prefixing was moved inside the component we had to remove the prefixes ourselves. This created a problem with unescaped variables. Before {context.results_link} was transformed to {{{context.results_link}}} (unescaped version). Now if we do {results_link} it will be transformed to {{context.{result_link}}}.

This will create a problem with URLs as they will be malformed.

We would like to have a way to declare which variables should be surrounded with {{{}}} (unescaped) and which not. Maybe ActionVariable could change to:

interface ActionVariable {
   name: string;
   description: string;
   unescaped/raw?: boolean;
}

Thank you a lot!

[elasticmachine]

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

[pmuellr]

I'm just starting to look at another "escaping" issue, which I think will change things a bit as to how much escaping really needs to happen. Issue #79371

I'll have to dig into this issue a bit more, hopefully whatever we do in the 79731 issue will be appropriate for this issue as well.

[pmuellr]

I'm a little confused by this:

return [
    { name: 'state.signals_count', description: 'state.signals_count' },
    { name: '{context.results_link}', description: 'context.results_link' },
    { name: '{context.rule_id}', description: 'context.rule_id' },
  ];

At first I thought perhaps {context.results_link} was some indirection that siem was dealing with internally, but now I'm thinking the idea is that when this variable is inserted with the usual {{var-name-here}} from the variables list, that this will end up rendering it as {{{context.results_link}}}. If so, that smells like a bad pattern to begin with, but I can certainly see why you'd want to do this :-)

Per previously-referenced issue #79371, I'm hoping we can basically be in a position where you can always just use {{}} and we'll do the escaping as-needed, in the actions themselves. URLs will be a good test case for this. We'd ideally like to never escape them anywhere (I think), but we may need to augment the variable definitions, kinda like you suggest, to enforce that. Eg, for slack, escaping is hard, because there are no rules I can find on how it's done, so I was planning on, as a last resort, "escaping" by turning all variable references in to back-tic'd string values, and somehow escaping or removing any embedded back-tics IN the string. That would be awful for URLs though - "here's a link, but it's not clickable".

[pmuellr]

I have a draft PR of a change to the way mustache variables are escaped, here: #83919

It seems like the right thing to do, to fix the issue it's trying to fix, not sure it will help in this case though.

The new escaping is really just for webhook json, slack and email message bodies - I think for the other actions, no escaping is needed at all, and that's the new default. So the question is, would the new escaping for the three actions ^^^ end up mangling a URL. All the new escaping, per format-type (basically action), is here: https://github.com/elastic/kibana/blob/4588f996764c607f2b685ebb1c7a6d246cb1f892/x-pack/plugins/actions/server/lib/mustache_renderer.ts

Both the markdown and slack escaping will do escaping on _, so I think that means they will likely mangle URLs.

Which means we need another mechanism.

@cnasikas's suggestion of adding this in the action variable definition seems to make sense to me. I guess the new flag would be an indicator of whether the variables list would paste in {{var-name}} or {{{var-name}}}, so could be called something super explicit like tripleBraceVariable: boolean. unescaped feels odd, since the default is presumably false, and it makes sense, but I kinda hate having "negative" sounding property names. More name suggestions please!

[mikecote]

@pmuellr how about noEscape, it's used by handlebars to indicate compiling the template without escaping (Handlebars.compile(source, { noEscape: true })). We could use it per action variable to indicate no escaping the value.

[pmuellr]

noEscape is a negative, which I don't like for "properties", especially given the default value of false.

noEscape: false WHAT DOES IT MEAN??? :-)

I kinda think it should reflect what we're actually going to do here - use triple braces instead of double braces - to make it super-clear. Also keep in mind, this is something only alert developers need to deal with, in terms of sprinkling it over their context variable definitions - not something users would ever see.

[mikecote]

I kinda think it should reflect what we're actually going to do here - use triple braces instead of double braces - to make it super-clear.

How about useWithTripleBracesInTemplates? Since the ActionVariables[] are set at the alert type registry, I figured if we mention triple braces we should also mention templates since it's not clear otherwise (since action variables don't have to be just for templates)

[pmuellr]

useWithTripleBracesInTemplates is super-wordy, but extremely precise :-) 👍 unless there's a better suggestion.