Allow action types to perform their own mustache variable escaping in parameter templates #83919

pmuellr · 2020-11-20T14:24:34Z

resolves #79371
resolves #62928

In this PR, we allow action types to determine how to escape the
variables used in their parameters, when rendered as mustache
templates. Prior to this, action parameters were recursively
rendered as mustache templates using the default mustache
templating, by the alerts library. The default mustache
templating used html escaping.

Action types opt-in to the new capability via a new optional
method in the action type, renderParameterTemplates(). If not
provided, the previous recursive rendering is done, but now with
no escaping at all.

For #62928, changed the mustache template rendering to be
replaced with the error message, if an error occurred,
so at least you can now see that an error occurred. Useful
to diagnose problems with invalid mustache templates.

Unit or functional tests were updated or added to match the most common scenarios

Release note

Improves the escaping done by the mustache templating engine, which previously always performed HTML escaping. Escaping is now off for most action parameters, except those that need per-action escaping, including the Slack, Email and Webhook action parameters.

resolves elastic#79371 resolves elastic#62928 In this PR, we allow action types to determine how to escape the variables used in their parameters, when rendered as mustache templates. Prior to this, action parameters were recursively rendered as mustache templates using the default mustache templating, by the alerts library. The default mustache templating used html escaping. Action types opt-in to the new capability via a new optional method in the action type, `renderParameterTemplates()`. If not provided, the previous recursive rendering is done, but now with no escaping at all. For elastic#62928, changed the mustache template rendering to be replaced with the error message, if an error occurred, so at least you can now see that an error occurred. Useful to diagnose problems with invalid mustache templates.

pmuellr · 2020-12-08T17:48:05Z

x-pack/plugins/actions/server/builtin_action_types/email.test.ts

@@ -396,4 +396,37 @@ describe('execute()', () => {
      }
    `);
  });
+
+  test('renders parameter templates as expected', async () => {


This is the first of a new set of tests for action types with optional renderParameterTemplates() methods. They don't exhaustively test the escaping, as that is done in the render-specific code - it only tests that the escaping is basically happening as expected.

pmuellr · 2020-12-08T17:49:24Z

x-pack/plugins/actions/server/builtin_action_types/email.ts

+  variables: Record<string, unknown>
+): ActionParamsType {
+  return {
+    // most of the params need no escaping


good example of how to do the traditional no-escape templating first, then customize the escaping for specific parameters

pmuellr · 2020-12-08T17:52:12Z

x-pack/plugins/actions/server/lib/mustache_renderer.ts

+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+import Mustache from 'mustache';


This module contains all of the specific escaping functions that we currently support. I'm guessing the Slack and Markdown ones may need some tweaking for edge cases - especially Slack where "escaping" is not well defined. But works for all the basic cases I've tried.

pmuellr · 2020-12-08T17:54:14Z

x-pack/plugins/actions/server/mocks.ts

  };
  return mock;
 };

+// this is a default renderer that escapes nothing


Not a real mock :-). But had to do this, as there was so much other code dependent on the templating, which would have required a LOT more non-trivial changes to other unrelated tests. Function tests have been added to test a complete end-to-end of alerts firing actions and capturing the results of the action executions.

pmuellr · 2020-12-08T17:55:44Z

x-pack/plugins/actions/server/plugin.ts

@@ -484,3 +492,17 @@ export class ActionsPlugin implements Plugin<Promise<PluginSetupContract>, Plugi
    }
  }
 }
+
+export function renderActionParameterTemplates<Params extends ActionTypeParams = ActionTypeParams>(


Here's where the action types get to override the new default templating method, which does not do any escaping.

pmuellr · 2020-12-08T17:56:47Z

x-pack/plugins/alerts/server/manual_tests/action_param_templates.sh

@@ -0,0 +1,121 @@
+#!/usr/bin/env bash


This is a handy little test script to play with changing the value of variables (the alert name) via script or ad hoc in the UI, once the script creates the actions and alerts.

pmuellr · 2020-12-08T17:57:50Z

x-pack/plugins/alerts/server/task_runner/transform_action_params.ts

-  // type signature for the customizer, but rather than pollute the customizer
-  // with casts, seemed better to just do it in one place, here.
-  return (result as unknown) as AlertActionParams;
+  // when the list of variables we pass in here changes,


This is where we moved the mustache templating out of alerts, and into actions.

pmuellr · 2020-12-08T17:59:27Z

...erting_api_integration/common/fixtures/plugins/actions_simulators/server/slack_simulation.ts

  return http.createServer((request, response) => {
+    // return the messages that were posted to be remembered


extended the slack and webhook simulators, to capture the data sent in some requests, and allow it to be retrieved. We use this in some new fully-cycle function tests that create actions and alerts and track that the payloads generated by the actions are escaped properly.

elasticmachine · 2020-12-08T17:59:54Z

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

ymao1

Looks good and looks like it's working as expected!

I have been trying to reproduce the webhook failure locally by using random characters and quotes in the alertName and then adding the alertName to the webhook body but have been unable to reproduce the failure. Do you have any suggestions for reproducing the failed action?

YulNaumenko · 2020-12-09T19:41:53Z

Looks good and looks like it's working as expected!

I have been trying to reproduce the webhook failure locally by using random characters and quotes in the alertName and then adding the alertName to the webhook body but have been unable to reproduce the failure. Do you have any suggestions for reproducing the failed action?

I think this test cover the issue case enough. Maybe you can reproduce it by setting 'x"y' as an alert name and then add this param to the Webhook message?

YulNaumenko

LGTM

ymao1 · 2020-12-09T20:01:21Z

I think this test cover the issue case enough. Maybe you can reproduce it by setting 'x"y' as an alert name and then add this param to the Webhook message?

I tried that (something like foo"bar for the alert name and the webhook POST was successful although i did see the quote sign turn into ". I was just looking for a way to replicate the failed webhook POST

pmuellr · 2020-12-10T14:46:46Z

I have been trying to reproduce the webhook failure locally by using random characters and quotes in the alertName and then adding the alertName to the webhook body but have been unable to reproduce the failure. Do you have any suggestions for reproducing the failed action?

I assume you mean replicate the problems we saw before this PR?

I thought originally webhook would be problematic with double quote, but the old code escaped that as an html entity (as you saw). The other problematic character is \n - the character. aka 0x0a. The problem trying to simulate that with the manual test case I added is that there's no easy way to get a 0x0a char in a string in JSON, as that makes the string unparseable (strings in JSON can't contain line breaks explicitly, but can include \n). It needs to come from some place other than a JSON-sourced thing.

I was able to test this by hacking some bits while debugging, but agree it would be good to have an explicit test for this. Just had a thought that we could extend one of our FT test alerts to add a context variable with an embedded 0x0a, and then check that one in the new FT cases I added.

pmuellr · 2020-12-10T21:47:13Z

I was able to test this by hacking some bits while debugging, but agree it would be good to have an explicit test for this. Just had a thought that we could extend one of our FT test alerts to add a context variable with an embedded 0x0a, and then check that one in the new FT cases I added.

Just added commit 651eefc which provides a way to test some of these harder escapable strings, by having the alert inject context variables with escapable strings directly.

ymao1

LGTM! Thanks for adding that test!

pmuellr · 2020-12-14T06:05:16Z

@elasticmachine merge upstream

pmuellr · 2020-12-14T21:46:52Z

@elasticmachine merge upstream

kibanamachine · 2020-12-14T23:56:44Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 589c5d8

Metrics [docs]

Distributable file count

id	before	after	diff
`default`	47145	47907	+762

History

💚 Build #94154 succeeded 9e14968
💚 Build #93877 succeeded 50a80cd
💚 Build #93711 succeeded da7b387
💔 Build #93687 failed 651eefc
💔 Build #93501 failed 4081c28

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

… parameter templates (elastic#83919) resolves elastic#79371 resolves elastic#62928 In this PR, we allow action types to determine how to escape the variables used in their parameters, when rendered as mustache templates. Prior to this, action parameters were recursively rendered as mustache templates using the default mustache templating, by the alerts library. The default mustache templating used html escaping. Action types opt-in to the new capability via a new optional method in the action type, `renderParameterTemplates()`. If not provided, the previous recursive rendering is done, but now with no escaping at all. For elastic#62928, changed the mustache template rendering to be replaced with the error message, if an error occurred, so at least you can now see that an error occurred. Useful to diagnose problems with invalid mustache templates.

… parameter templates (#83919) (#85901) resolves #79371 resolves #62928 In this PR, we allow action types to determine how to escape the variables used in their parameters, when rendered as mustache templates. Prior to this, action parameters were recursively rendered as mustache templates using the default mustache templating, by the alerts library. The default mustache templating used html escaping. Action types opt-in to the new capability via a new optional method in the action type, `renderParameterTemplates()`. If not provided, the previous recursive rendering is done, but now with no escaping at all. For #62928, changed the mustache template rendering to be replaced with the error message, if an error occurred, so at least you can now see that an error occurred. Useful to diagnose problems with invalid mustache templates.

* master: (66 commits) [Alerting] fixes broken Alerting Example plugin (elastic#85774) [APM] Service overview instances table (elastic#85770) [Security Solution] Unskip timeline creation Cypress test (elastic#85871) properly recognize enterprise licenses (elastic#85849) [SecuritySolution][Detections] Adds SavedObject persistence to Signals Migrations (elastic#85690) [TSVB] Fix functional tests flakiness and unskip them (elastic#85388) [Fleet] Change permissions for Fleet enroll role (elastic#85802) Gauge visualization can no longer be clicked to filter on values since Kibana 7.10.0 (elastic#84768) [Security Solution][Detections] Add alert source to detection rule action context (elastic#85488) [Discover] Don't display hide/show button for histogram when there's no time filter (elastic#85424) skip flaky suite (elastic#78553) License checks for alerts plugin (elastic#85649) skip flaky suite (elastic#84992) skip 'query return results valid for scripted field' elastic#78553 Allow action types to perform their own mustache variable escaping in parameter templates (elastic#83919) [ML] More machine learning links in doc_links_service.ts (elastic#85365) Removed Alerting & Event Log deprecated fields that should not be using (elastic#85652) Closes elastic#79995 by adding new tab in transaction details to show related trace logs. (elastic#85859) Fix outdated jest snapshot [Maps] Surface on prem EMS (elastic#85729) ...

pmuellr added the needs_docs label Nov 20, 2020

pmuellr mentioned this pull request Nov 20, 2020

[Actions] Ability to declare unescaped messageVariables #83780

Closed

pmuellr force-pushed the actions/per-type-mustache-escaping-2 branch from 4588f99 to a9ba265 Compare November 24, 2020 01:17

pmuellr force-pushed the actions/per-type-mustache-escaping-2 branch from 2a24ca9 to da2fb13 Compare November 24, 2020 21:42

pmuellr force-pushed the actions/per-type-mustache-escaping-2 branch 5 times, most recently from bb4cf80 to b2ee151 Compare December 8, 2020 14:50

pmuellr force-pushed the actions/per-type-mustache-escaping-2 branch from b2ee151 to 4081c28 Compare December 8, 2020 14:58

pmuellr changed the title ~~[alerts] allow action types to escape their own mustache templates~~ Allow action types to perform their own mustache variable escaping in parameter templates Dec 8, 2020

pmuellr added Feature:Alerting release_note:enhancement Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.11.0 v8.0.0 and removed needs_docs labels Dec 8, 2020

pmuellr commented Dec 8, 2020

View reviewed changes

pmuellr marked this pull request as ready for review December 8, 2020 17:59

pmuellr requested a review from a team as a code owner December 8, 2020 17:59

ymao1 reviewed Dec 9, 2020

View reviewed changes

YulNaumenko approved these changes Dec 9, 2020

View reviewed changes

address PR comment with better function test

651eefc

merge master and fix merge conflicts

da7b387

ymao1 approved these changes Dec 10, 2020

View reviewed changes

kibanamachine and others added 2 commits December 14, 2020 01:05

Merge branch 'master' into actions/per-type-mustache-escaping-2

50a80cd

add more tests, and code to fix those newly broken tests

9e14968

Merge branch 'master' into actions/per-type-mustache-escaping-2

589c5d8

pmuellr merged commit 7873e36 into elastic:master Dec 15, 2020

pmuellr mentioned this pull request Dec 15, 2020

[7.x] Allow action types to perform their own mustache variable escaping in parameter templates (#83919) #85901

Merged

pmuellr added the backported label Dec 15, 2020

pmuellr mentioned this pull request Jan 5, 2021

[actions] new action parameter escaping breaks existing templates #87344

Closed

pmuellr self-assigned this Jan 5, 2021

This was referenced Jan 6, 2021

[Security Solution][Case] Markdown links are not properly shown on external services #87440

Open

[alerts] fix up the docs on mustache escaping #87933

Closed

pmuellr mentioned this pull request Jan 15, 2021

updates doc on action parameter variable mustache escaping #88521

Merged

1 task

pmuellr mentioned this pull request Feb 4, 2021

[Security Solution] webhook action can sends invalid json payload when combined with mustache #88893

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow action types to perform their own mustache variable escaping in parameter templates #83919

Allow action types to perform their own mustache variable escaping in parameter templates #83919

pmuellr commented Nov 20, 2020 •

edited

Loading

pmuellr Dec 8, 2020

pmuellr Dec 8, 2020

pmuellr Dec 8, 2020

pmuellr Dec 8, 2020

pmuellr Dec 8, 2020

pmuellr Dec 8, 2020

pmuellr Dec 8, 2020

pmuellr Dec 8, 2020

elasticmachine commented Dec 8, 2020

ymao1 left a comment

YulNaumenko commented Dec 9, 2020 •

edited

Loading

YulNaumenko left a comment

ymao1 commented Dec 9, 2020

pmuellr commented Dec 10, 2020

pmuellr commented Dec 10, 2020

ymao1 left a comment

pmuellr commented Dec 14, 2020

pmuellr commented Dec 14, 2020

kibanamachine commented Dec 14, 2020

		return http.createServer((request, response) => {
		// return the messages that were posted to be remembered

Allow action types to perform their own mustache variable escaping in parameter templates #83919

Allow action types to perform their own mustache variable escaping in parameter templates #83919

Conversation

pmuellr commented Nov 20, 2020 • edited Loading

Release note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Dec 8, 2020

ymao1 left a comment

Choose a reason for hiding this comment

YulNaumenko commented Dec 9, 2020 • edited Loading

YulNaumenko left a comment

Choose a reason for hiding this comment

ymao1 commented Dec 9, 2020

pmuellr commented Dec 10, 2020

pmuellr commented Dec 10, 2020

ymao1 left a comment

Choose a reason for hiding this comment

pmuellr commented Dec 14, 2020

pmuellr commented Dec 14, 2020

kibanamachine commented Dec 14, 2020

💚 Build Succeeded

Metrics [docs]

Distributable file count

History

pmuellr commented Nov 20, 2020 •

edited

Loading

YulNaumenko commented Dec 9, 2020 •

edited

Loading