Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cleanup server-log action #53326

Merged
merged 4 commits into from
Jan 2, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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');
});
});
Original file line number Diff line number Diff line change
@@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will remove tabs as well? I think that is probably OK, but some might expect that to be valid whitespace.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true; it's likely to confuse customers, so I think we should probably just let that one pass through untranslated


// replaces control characters in string with ;
export function withoutControlCharacters(s: string): string {
return s.replace(CONTROL_CHAR_PATTERN, ';');
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,21 +10,25 @@ 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

export type ActionParamsType = TypeOf<typeof ParamsSchema>;

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
Expand All @@ -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}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the prefix seems to be causing a test failure in:

params: { message: 'message text here', level: 'info' },
config: {},
secrets: {},
});
expect(mockedLogger.info).toHaveBeenCalledWith('message text here');

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ya, just pushed a commit with the fix

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe 'server-log' should be pulled out as a symbol, as it is also used in the action type name and id?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense

} catch (err) {
const message = i18n.translate('xpack.actions.builtin.serverLog.errorLoggingErrorMessage', {
defaultMessage: 'error logging message',
Expand Down
2 changes: 1 addition & 1 deletion x-pack/legacy/plugins/actions/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ export class Plugin {
private licenseState: LicenseState | null = null;

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$;
}
Expand Down