[actions] new action parameter escaping breaks existing templates

[pmuellr]

We changed the way action parameters have their mustache variables escaped in PR #83919 .

Unfortunately, this appears to break some existing alerts.

Eg, it appears monitoring provides a context variable with a markdown link like [link text](url-here). In the past, the HTML escaping generally would have done nothing with this, but now if you use this in an email, we'll do markdown escaping so the user will see the actual markdown text, instead of the rendered link.

We'll need to look through all the alerts to see what else is broken, figure out how to resolve this.

[elasticmachine]

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

[pmuellr]

Here's an example of where the escaping is not going to work right:

kibana/x-pack/plugins/monitoring/server/alerts/missing_monitoring_data_alert.ts

Lines 173 to 174 in 450d297

	const action = `[${fullActionText}](${globalStateLink})`;
	const internalShortMessage = i18n.translate(

kibana/x-pack/plugins/monitoring/server/alerts/missing_monitoring_data_alert.ts

Lines 196 to 205 in 450d297

	instance.scheduleActions('default', {
	internalShortMessage,
	internalFullMessage: Globals.app.isCloud ? internalShortMessage : internalFullMessage,
	state: AlertingDefaults.ALERT_STATE.firing,
	nodes: firingNodes.map((state) => `node: ${state.nodeName}`).toString(),
	count: firingCount,
	clusterName: cluster.clusterName,
	action,
	actionPlain: shortActionText,
	});

on line 173, the action variable is set to a markdown link. That variable is then referenced in the context variables in the invocation of instance.scheduleActions() on line 193.

There's a couple of problems here, relating to context variables containing markup for a particular connector:

• they are only useful for connectors that accept that markup; markup should only be in the mustache template itself, not in the context variables. since the mustache template is associated with the connector, but the context variables are intended to be used with any connector
• the variable values are escaped depending on what connector is using them; some connectors may escape some of the characters, and others not escape the same characters
• to work around including markup in variables, you could "triple brace" the variable in the mustache template, but then the actual data within the markup won't be escaped. Eg, a variable with the value [${text)](${url}), where text contains the string ]() will end up rendering as []() (value-of-url-variable), which is not what you want. text and url need to be escaped, but the markup []() should NOT be escaped, and the only way to do that is to not have the markup in the context variable.

[pmuellr]

Looking through references to AlertInstance::scheduleActions(), it looks like the following modules use the pattern of markup text mixed with variable content, and in fact, the exact same pattern of using a variable action of the following form, and exposing it as one of the context variables:

    const action = `[${fullActionText}](${globalStateLink})`;

I think this is all of the monitoring alerts. Looking through the rest of the alerts, I didn't directly see any markup in use, but it's hard to tell due to the indirection used to populate the context variables in solution-specific libraries.

• x-pack/plugins/monitoring/server/alerts/ccr_read_exceptions_alert.ts
• x-pack/plugins/monitoring/server/alerts/cluster_health_alert.ts
• x-pack/plugins/monitoring/server/alerts/cpu_usage_alert.ts
• x-pack/plugins/monitoring/server/alerts/disk_usage_alert.ts
• x-pack/plugins/monitoring/server/alerts/elasticsearch_version_mismatch_alert.ts
• x-pack/plugins/monitoring/server/alerts/kibana_version_mismatch_alert.ts
• x-pack/plugins/monitoring/server/alerts/license_expiration_alert.ts
• x-pack/plugins/monitoring/server/alerts/logstash_version_mismatch_alert.ts
• x-pack/plugins/monitoring/server/alerts/memory_usage_alert.ts
• x-pack/plugins/monitoring/server/alerts/missing_monitoring_data_alert.ts
• x-pack/plugins/monitoring/server/alerts/nodes_changed_alert.ts
• x-pack/plugins/monitoring/server/alerts/thread_pool_rejections_alert_base.ts

One simple fix, for monitoring, may be to just change the action variable content to be the URL, and not the markdown link. It obviously won't be as pretty as a markdown link, but the URL should be link-ified by the markdown processor, so functionally would be the same as the link.

[pmuellr]

Other potential fixes:

1. Change the action variable to just be the url, not a markdown-formatted link (from comment above)
2. revert PRs [actions] expand object context variables as JSON #85903 and then Allow action types to perform their own mustache variable escaping in parameter templates #83919; the first PR is based on the second (a revert on just the second one will be painful), and the second is the one that changed the way the escaping works; plan on shipping in 7.12 instead
3. add a "migration" to change the existing templates in existing alerts to use "triple brace" references where needed, eg {{{context.action}}}

The problem with the 3rd option is noted in a comment above. This will end up NOT escaping the text or url in the markdown link, and so could end up not rendering correctly in the end.

[pmuellr]

The original monitoring issue that was reported for this is here: issue #84819

In a conversation, it sounds like changing the action variable to a plain URL instead of Markdown link will be the direction for now.

I think leave this open for a bit longer, in case other problems are noticed, or if the suggested change doesn't work for some reason.

[pmuellr]

Also checked with security and observability folks re: context variables with formatting markup, doesn't sound like any of the other alerts are doing this.

[pmuellr]

The only broken alerts here seem to be the observability ones - having the links "not rendered" (ie, the markdown formatting is rendered literally) is going to be acceptable for now, with a long term goal of reorganizing the context variables and templates.

So, going to close this one out. Any further discussion on the monitoring alerts can go here: #84819