From a57159c45a497f2d11f14d4616840706402c61db Mon Sep 17 00:00:00 2001 From: Patrick Mueller Date: Tue, 17 Dec 2019 10:32:39 -0500 Subject: [PATCH 1/3] cleanup server-log action - make the level param optional, defaults to info - change the actions logger "tag" from "alerting" to "actions" - remove control characters from message --- .../lib/string_utils.test.ts | 44 +++++++++++++++++++ .../builtin_action_types/lib/string_utils.ts | 13 ++++++ .../server/builtin_action_types/server_log.ts | 23 ++++++---- .../legacy/plugins/actions/server/plugin.ts | 2 +- 4 files changed, 72 insertions(+), 10 deletions(-) create mode 100644 x-pack/legacy/plugins/actions/server/builtin_action_types/lib/string_utils.test.ts create mode 100644 x-pack/legacy/plugins/actions/server/builtin_action_types/lib/string_utils.ts diff --git a/x-pack/legacy/plugins/actions/server/builtin_action_types/lib/string_utils.test.ts b/x-pack/legacy/plugins/actions/server/builtin_action_types/lib/string_utils.test.ts new file mode 100644 index 0000000000000..1220b6852781b --- /dev/null +++ b/x-pack/legacy/plugins/actions/server/builtin_action_types/lib/string_utils.test.ts @@ -0,0 +1,44 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { withoutControlCharacters } from './string_utils'; + +describe('ensureSingleLineString()', () => { + test('works with plain ole strings', () => { + expect(withoutControlCharacters('')).toEqual(''); + expect(withoutControlCharacters(' a b c ')).toEqual(' a b c '); + }); + + test('works with multiple control characters', () => { + expect(withoutControlCharacters(' \r \n ')).toEqual(' ; ; '); + expect(withoutControlCharacters('\r \n ')).toEqual('; ; '); + expect(withoutControlCharacters(' \r \n')).toEqual(' ; ;'); + expect(withoutControlCharacters('\r \n')).toEqual('; ;'); + }); + + test('works with /00-/1F', () => { + for (let c = 0; c <= 0x1f; c++) { + expect(withoutControlCharacters(String.fromCharCode(c))).toEqual(';'); + } + expect(withoutControlCharacters(String.fromCharCode(0x20))).toEqual(' '); + }); + + test('works with /7F-/9F', () => { + expect(withoutControlCharacters(String.fromCharCode(0x7e))).toEqual('~'); + for (let c = 0x7f; c <= 0x9f; c++) { + expect(withoutControlCharacters(String.fromCharCode(c))).toEqual(';'); + } + const nbsp = String.fromCharCode(0xa0); + expect(withoutControlCharacters(nbsp)).toEqual(nbsp); + }); + + test('works with UCS newlines', () => { + expect(withoutControlCharacters('\u2027')).toEqual('\u2027'); + expect(withoutControlCharacters('\u2028')).toEqual(';'); + expect(withoutControlCharacters('\u2029')).toEqual(';'); + expect(withoutControlCharacters('\u202A')).toEqual('\u202A'); + }); +}); diff --git a/x-pack/legacy/plugins/actions/server/builtin_action_types/lib/string_utils.ts b/x-pack/legacy/plugins/actions/server/builtin_action_types/lib/string_utils.ts new file mode 100644 index 0000000000000..de283bfd935b8 --- /dev/null +++ b/x-pack/legacy/plugins/actions/server/builtin_action_types/lib/string_utils.ts @@ -0,0 +1,13 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +// see: https://en.wikipedia.org/wiki/Unicode_control_characters +const CONTROL_CHAR_PATTERN = /[\x00-\x1F]|[\x7F-\x9F]|[\u2028-\u2029]/g; + +// replaces control characters in string with ; +export function withoutControlCharacters(s: string): string { + return s.replace(CONTROL_CHAR_PATTERN, ';'); +} diff --git a/x-pack/legacy/plugins/actions/server/builtin_action_types/server_log.ts b/x-pack/legacy/plugins/actions/server/builtin_action_types/server_log.ts index c2af29051d8dd..efb7ce4531bfa 100644 --- a/x-pack/legacy/plugins/actions/server/builtin_action_types/server_log.ts +++ b/x-pack/legacy/plugins/actions/server/builtin_action_types/server_log.ts @@ -10,6 +10,7 @@ import { schema, TypeOf } from '@kbn/config-schema'; import { Logger } from '../../../../../../src/core/server'; import { ActionType, ActionTypeExecutorOptions, ActionTypeExecutorResult } from '../types'; +import { withoutControlCharacters } from './lib/string_utils'; // params definition @@ -17,14 +18,17 @@ export type ActionParamsType = TypeOf; const ParamsSchema = schema.object({ message: schema.string(), - level: schema.oneOf([ - schema.literal('trace'), - schema.literal('debug'), - schema.literal('info'), - schema.literal('warn'), - schema.literal('error'), - schema.literal('fatal'), - ]), + level: schema.oneOf( + [ + schema.literal('trace'), + schema.literal('debug'), + schema.literal('info'), + schema.literal('warn'), + schema.literal('error'), + schema.literal('fatal'), + ], + { defaultValue: 'info' } + ), }); // action type definition @@ -48,8 +52,9 @@ async function executor( const actionId = execOptions.actionId; const params = execOptions.params as ActionParamsType; + const sanitizedMessage = withoutControlCharacters(params.message); try { - logger[params.level](params.message); + logger[params.level](`server-log: ${sanitizedMessage}`); } catch (err) { const message = i18n.translate('xpack.actions.builtin.serverLog.errorLoggingErrorMessage', { defaultMessage: 'error logging message', diff --git a/x-pack/legacy/plugins/actions/server/plugin.ts b/x-pack/legacy/plugins/actions/server/plugin.ts index 6a41bf9a8b459..3de0f8222ceb5 100644 --- a/x-pack/legacy/plugins/actions/server/plugin.ts +++ b/x-pack/legacy/plugins/actions/server/plugin.ts @@ -56,7 +56,7 @@ export class Plugin { private defaultKibanaIndex?: string; constructor(initializerContext: ActionsPluginInitializerContext) { - this.logger = initializerContext.logger.get('plugins', 'alerting'); + this.logger = initializerContext.logger.get('plugins', 'actions'); this.config$ = initializerContext.config.create(); this.kibana$ = initializerContext.config.kibana$; } From 9b7facab7679da2e17851fabc880d841616f8519 Mon Sep 17 00:00:00 2001 From: Patrick Mueller Date: Thu, 2 Jan 2020 12:05:29 -0500 Subject: [PATCH 2/3] fix broken jest test --- .../actions/server/builtin_action_types/server_log.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/legacy/plugins/actions/server/builtin_action_types/server_log.test.ts b/x-pack/legacy/plugins/actions/server/builtin_action_types/server_log.test.ts index 76e639c994834..c59ddf97017fd 100644 --- a/x-pack/legacy/plugins/actions/server/builtin_action_types/server_log.test.ts +++ b/x-pack/legacy/plugins/actions/server/builtin_action_types/server_log.test.ts @@ -98,6 +98,6 @@ describe('execute()', () => { config: {}, secrets: {}, }); - expect(mockedLogger.info).toHaveBeenCalledWith('message text here'); + expect(mockedLogger.info).toHaveBeenCalledWith('server-log: message text here'); }); }); From 04c600582cd93b0d3a9c8435b0b3b248d7ba7698 Mon Sep 17 00:00:00 2001 From: Patrick Mueller Date: Thu, 2 Jan 2020 12:22:56 -0500 Subject: [PATCH 3/3] apply fixes suggested in PR review --- .../server/builtin_action_types/lib/string_utils.test.ts | 8 ++++++-- .../server/builtin_action_types/lib/string_utils.ts | 5 +++-- .../actions/server/builtin_action_types/server_log.ts | 6 ++++-- 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/x-pack/legacy/plugins/actions/server/builtin_action_types/lib/string_utils.test.ts b/x-pack/legacy/plugins/actions/server/builtin_action_types/lib/string_utils.test.ts index 1220b6852781b..85e002b805e46 100644 --- a/x-pack/legacy/plugins/actions/server/builtin_action_types/lib/string_utils.test.ts +++ b/x-pack/legacy/plugins/actions/server/builtin_action_types/lib/string_utils.test.ts @@ -19,9 +19,13 @@ describe('ensureSingleLineString()', () => { expect(withoutControlCharacters('\r \n')).toEqual('; ;'); }); - test('works with /00-/1F', () => { + test('works with /00-/1F, except tab', () => { for (let c = 0; c <= 0x1f; c++) { - expect(withoutControlCharacters(String.fromCharCode(c))).toEqual(';'); + if (c === 0x09) { + expect(withoutControlCharacters(String.fromCharCode(c))).toEqual('\t'); + } else { + expect(withoutControlCharacters(String.fromCharCode(c))).toEqual(';'); + } } expect(withoutControlCharacters(String.fromCharCode(0x20))).toEqual(' '); }); diff --git a/x-pack/legacy/plugins/actions/server/builtin_action_types/lib/string_utils.ts b/x-pack/legacy/plugins/actions/server/builtin_action_types/lib/string_utils.ts index de283bfd935b8..d0e3504677307 100644 --- a/x-pack/legacy/plugins/actions/server/builtin_action_types/lib/string_utils.ts +++ b/x-pack/legacy/plugins/actions/server/builtin_action_types/lib/string_utils.ts @@ -5,9 +5,10 @@ */ // see: https://en.wikipedia.org/wiki/Unicode_control_characters -const CONTROL_CHAR_PATTERN = /[\x00-\x1F]|[\x7F-\x9F]|[\u2028-\u2029]/g; +// but don't include tabs (0x09), they're fine +const CONTROL_CHAR_PATTERN = /[\x00-\x08]|[\x0A-\x1F]|[\x7F-\x9F]|[\u2028-\u2029]/g; -// replaces control characters in string with ; +// replaces control characters in string with ;, but leaves tabs export function withoutControlCharacters(s: string): string { return s.replace(CONTROL_CHAR_PATTERN, ';'); } diff --git a/x-pack/legacy/plugins/actions/server/builtin_action_types/server_log.ts b/x-pack/legacy/plugins/actions/server/builtin_action_types/server_log.ts index efb7ce4531bfa..0edf409e4d46c 100644 --- a/x-pack/legacy/plugins/actions/server/builtin_action_types/server_log.ts +++ b/x-pack/legacy/plugins/actions/server/builtin_action_types/server_log.ts @@ -12,6 +12,8 @@ import { Logger } from '../../../../../../src/core/server'; import { ActionType, ActionTypeExecutorOptions, ActionTypeExecutorResult } from '../types'; import { withoutControlCharacters } from './lib/string_utils'; +const ACTION_NAME = 'server-log'; + // params definition export type ActionParamsType = TypeOf; @@ -35,7 +37,7 @@ const ParamsSchema = schema.object({ export function getActionType({ logger }: { logger: Logger }): ActionType { return { id: '.server-log', - name: 'server-log', + name: ACTION_NAME, validate: { params: ParamsSchema, }, @@ -54,7 +56,7 @@ async function executor( const sanitizedMessage = withoutControlCharacters(params.message); try { - logger[params.level](`server-log: ${sanitizedMessage}`); + logger[params.level](`${ACTION_NAME}: ${sanitizedMessage}`); } catch (err) { const message = i18n.translate('xpack.actions.builtin.serverLog.errorLoggingErrorMessage', { defaultMessage: 'error logging message',