Skip to content

Commit

Permalink
[Response Ops][Cases] Add more granular checks before decoding user a…
Browse files Browse the repository at this point in the history 
…ctions (elastic#124896) (elastic#125415)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Christos Nasikas <christos.nasikas@elastic.co>
(cherry picked from commit 97bd50d)

Co-authored-by: Jonathan Buttner <56361221+jonathan-buttner@users.noreply.github.com>
  • Loading branch information
@kibanamachine @jonathan-buttner
kibanamachine and jonathan-buttner authored Feb 16, 2022
1 parent dcb69c2 commit 45f7c3b
Show file tree
Hide file tree
Showing 2 changed files with 153 additions and 31 deletions.
148 changes: 135 additions & 13 deletions x-pack/plugins/cases/server/saved_object_types/migrations/user_actions/alerts.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
SavedObjectMigrationContext,
SavedObjectsMigrationLogger,
} from 'kibana/server';
import { cloneDeep, omit } from 'lodash';
import { cloneDeep, omit, set } from 'lodash';
import { migrationMocks } from 'src/core/server/mocks';
import { removeRuleInformation } from './alerts';

Expand All @@ -22,6 +22,89 @@ describe('alert user actions', () => {
context = migrationMocks.createContext();
});

describe('JSON.stringify throws an error', () => {
let jsonStringifySpy: jest.SpyInstance;
beforeEach(() => {
jsonStringifySpy = jest.spyOn(JSON, 'stringify').mockImplementation(() => {
throw new Error('failed to stringify');
});
});

afterEach(() => {
jsonStringifySpy.mockRestore();
});

it('logs an error when an error is thrown outside of the JSON.parse function', () => {
const doc = {
id: '123',
attributes: {
action: 'create',
action_field: ['comment'],
new_value:
'{"type":"generated_alert","alertId":"4eb4cd05b85bc65c7b9f22b776e0136f970f7538eb0d1b2e6e8c7d35b2e875cb","index":".internal.alerts-security.alerts-default-000001","rule":{"id":"43104810-7875-11ec-abc6-6f72e72f6004","name":"A rule"},"owner":"securitySolution"}',
},
type: 'abc',
references: [],
};

expect(removeRuleInformation(doc, context)).toEqual(doc);

const log = context.log as jest.Mocked<SavedObjectsMigrationLogger>;
expect(log.error.mock.calls[0]).toMatchInlineSnapshot(`
Array [
"Failed to migrate user action alerts with doc id: 123 version: 8.0.0 error: failed to stringify",
Object {
"migrations": Object {
"userAction": Object {
"id": "123",
},
},
},
]
`);
});
});

describe('JSON.parse spy', () => {
let jsonParseSpy: jest.SpyInstance;
beforeEach(() => {
jsonParseSpy = jest.spyOn(JSON, 'parse');
});

afterEach(() => {
jsonParseSpy.mockRestore();
});

it.each([
['update', ['status']],
['update', ['comment']],
[undefined, ['comment']],
['create', ['status']],
['create', []],
])(
'does not modify the document and does not call JSON.parse when action: %s and action_field: %s',
(action, actionField) => {
const doc = {
id: '123',
attributes: {
action,
action_field: actionField,
new_value: 'open',
},
type: 'abc',
references: [],
};

expect(removeRuleInformation(doc, context)).toEqual(doc);

expect(jsonParseSpy).not.toBeCalled();

const log = context.log as jest.Mocked<SavedObjectsMigrationLogger>;
expect(log.error).not.toBeCalled();
}
);
});

it('does not modify non-alert user action', () => {
const doc = {
id: '123',
Expand Down Expand Up @@ -51,18 +134,7 @@ describe('alert user actions', () => {
expect(removeRuleInformation(doc, context)).toEqual(doc);

const log = context.log as jest.Mocked<SavedObjectsMigrationLogger>;
expect(log.error.mock.calls[0]).toMatchInlineSnapshot(`
Array [
"Failed to migrate user action alerts with doc id: 123 version: 8.0.0 error: Unexpected end of JSON input",
Object {
"migrations": Object {
"userAction": Object {
"id": "123",
},
},
},
]
`);
expect(log.error).not.toBeCalled();
});

it('does not modify the document when new_value is null', () => {
Expand Down Expand Up @@ -127,6 +199,27 @@ describe('alert user actions', () => {
ensureRuleFieldsAreNull(newDoc);

expect(docWithoutNewValue(newDoc)).toEqual(docWithoutNewValue(doc));
expectParsedDocsToEqual(newDoc, doc);
});

it('sets the rule fields to null when the alert has an extra field', () => {
const doc = {
id: '123',
attributes: {
action: 'create',
action_field: ['comment'],
new_value:
'{"anExtraField": "some value","type":"alert","alertId":"4eb4cd05b85bc65c7b9f22b776e0136f970f7538eb0d1b2e6e8c7d35b2e875cb","index":".internal.alerts-security.alerts-default-000001","rule":{"id":"43104810-7875-11ec-abc6-6f72e72f6004","name":"A rule"},"owner":"securitySolution"}',
},
type: 'abc',
references: [],
};

const newDoc = removeRuleInformation(doc, context) as SavedObject<{ new_value: string }>;
ensureRuleFieldsAreNull(newDoc);

expect(docWithoutNewValue(newDoc)).toEqual(docWithoutNewValue(doc));
expectParsedDocsToEqual(newDoc, doc);
});

it('sets the rule fields to null for a generated alert', () => {
Expand All @@ -146,6 +239,7 @@ describe('alert user actions', () => {
ensureRuleFieldsAreNull(newDoc);

expect(docWithoutNewValue(newDoc)).toEqual(docWithoutNewValue(doc));
expectParsedDocsToEqual(newDoc, doc);
});

it('preserves the references field', () => {
Expand All @@ -165,10 +259,38 @@ describe('alert user actions', () => {
ensureRuleFieldsAreNull(newDoc);

expect(docWithoutNewValue(newDoc)).toEqual(docWithoutNewValue(doc));
expectParsedDocsToEqual(newDoc, doc);
});
});
});

const expectParsedDocsToEqual = (
nulledRuleDoc: SavedObject<{ new_value: string }>,
originalDoc: SavedObject<{ new_value: string }>
) => {
expect(parseDoc(nulledRuleDoc)).toEqual(injectNullRuleFields(parseDoc(originalDoc)));
};

const parseDoc = (doc: SavedObject<{ new_value: string }>): SavedObject<{ new_value: {} }> => {
const copyOfDoc = cloneDeep(doc);

const decodedNewValue = JSON.parse(doc.attributes.new_value);
set(copyOfDoc, 'attributes.new_value', decodedNewValue);

return copyOfDoc;
};

const injectNullRuleFields = (doc: SavedObject<{ new_value: {} }>) => {
const copyOfDoc = cloneDeep(doc);

set(copyOfDoc, 'attributes.new_value', {
...doc.attributes.new_value,
rule: { id: null, name: null },
});

return copyOfDoc;
};

const docWithoutNewValue = (doc: {}) => {
const copyOfDoc = cloneDeep(doc);
return omit(copyOfDoc, 'attributes.new_value');
Expand Down
36 changes: 18 additions & 18 deletions x-pack/plugins/cases/server/saved_object_types/migrations/user_actions/alerts.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,13 @@ export function removeRuleInformation(
try {
const { new_value, action, action_field } = doc.attributes;

if (!isCreateComment(action, action_field)) {
return originalDocWithReferences;
}

const decodedNewValueData = decodeNewValue(new_value);

if (!isAlertUserAction(action, action_field, decodedNewValueData)) {
if (!isAlertUserAction(decodedNewValueData)) {
return originalDocWithReferences;
}

Expand Down Expand Up @@ -66,15 +70,21 @@ function decodeNewValue(data?: string | null): unknown | null {
return null;
}

return JSON.parse(data);
try {
return JSON.parse(data);
} catch (error) {
return null;
}
}

function isAlertUserAction(
action?: string,
actionFields?: string[],
newValue?: unknown | null
): newValue is AlertCommentOptional {
return isCreateComment(action, actionFields) && isAlertObject(newValue);
function isAlertUserAction(newValue?: unknown | null): newValue is AlertCommentOptional {
const unsafeAlertData = newValue as AlertCommentOptional;

return (
unsafeAlertData !== undefined &&
unsafeAlertData !== null &&
(unsafeAlertData.type === GENERATED_ALERT || unsafeAlertData.type === CommentType.alert)
);
}

function isCreateComment(action?: string, actionFields?: string[]): boolean {
Expand All @@ -89,13 +99,3 @@ function isCreateComment(action?: string, actionFields?: string[]): boolean {
type AlertCommentOptional = Partial<Omit<CommentRequestAlertType, 'type'>> & {
type: CommentType.alert | typeof GENERATED_ALERT;
};

function isAlertObject(data?: unknown | null): boolean {
const unsafeAlertData = data as AlertCommentOptional;

return (
unsafeAlertData !== undefined &&
unsafeAlertData !== null &&
(unsafeAlertData.type === GENERATED_ALERT || unsafeAlertData.type === CommentType.alert)
);
}

0 comments on commit 45f7c3b

Please sign in to comment.