Skip to content

Commit

Permalink
[Security Solutions][Detection Engine] Fixes "undefined" crash for au…
Browse files Browse the repository at this point in the history
…thor field by adding a migration for it (elastic#107230)

## Summary

Fixes elastic#106233

During an earlier upgrade/fix to our system to add defaults to our types, we overlooked the "author" field which wasn't part of the original rules. Users upgrading might get errors such as:

```
params invalid: Invalid value "undefined" supplied to "author"
```

This fixes that issue by adding a migration for the `author` field for `7.14.1`.

See elastic#106233 for test instructions or manually remove your author field before upgrading your release and then upgrade and this should be fixed on upgrade.

### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

# Conflicts:
#	x-pack/plugins/alerting/server/saved_objects/migrations.test.ts
  • Loading branch information
FrankHassanabad committed Aug 11, 2021
1 parent 82081b0 commit 8ad1ab2
Show file tree
Hide file tree
Showing 2 changed files with 200 additions and 0 deletions.
165 changes: 165 additions & 0 deletions x-pack/plugins/alerting/server/saved_objects/migrations.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1026,6 +1026,171 @@ describe('7.13.0', () => {
},
},
});

expect(migration713(alert, migrationContext)).toEqual({
...alert,
attributes: {
...alert.attributes,
params: {
anomalyThreshold: 20,
machineLearningJobId: ['my_job_id', 'my_other_job_id'],
exceptionsList: [],
riskScoreMapping: [],
severityMapping: [],
threat: [],
},
},
});
});
});

describe('7.14.1', () => {
test('security solution author field is migrated to array if it is undefined', () => {
const migration7141 = getMigrations(encryptedSavedObjectsSetup)['7.14.1'];
const alert = getMockData({
alertTypeId: 'siem.signals',
params: {},
});

expect(migration7141(alert, migrationContext)).toEqual({
...alert,
attributes: {
...alert.attributes,
params: {
author: [],
},
},
});
});

test('security solution author field does not override existing values if they exist', () => {
const migration7141 = getMigrations(encryptedSavedObjectsSetup)['7.14.1'];
const alert = getMockData({
alertTypeId: 'siem.signals',
params: {
note: 'some note',
author: ['author 1'],
},
});

expect(migration7141(alert, migrationContext)).toEqual({
...alert,
attributes: {
...alert.attributes,
params: {
note: 'some note',
author: ['author 1'],
},
},
});
});
});

describe('handles errors during migrations', () => {
beforeEach(() => {
jest.resetAllMocks();
encryptedSavedObjectsSetup.createMigration.mockImplementation(() => () => {
throw new Error(`Can't migrate!`);
});
});
describe('7.10.0 throws if migration fails', () => {
test('should show the proper exception', () => {
const migration710 = getMigrations(encryptedSavedObjectsSetup)['7.10.0'];
const alert = getMockData({
consumer: 'alerting',
});
expect(() => {
migration710(alert, migrationContext);
}).toThrowError(`Can't migrate!`);
expect(migrationContext.log.error).toHaveBeenCalledWith(
`encryptedSavedObject 7.10.0 migration failed for alert ${alert.id} with error: Can't migrate!`,
{
migrations: {
alertDocument: {
...alert,
attributes: {
...alert.attributes,
},
},
},
}
);
});
});

describe('7.11.0 throws if migration fails', () => {
test('should show the proper exception', () => {
const migration711 = getMigrations(encryptedSavedObjectsSetup)['7.11.0'];
const alert = getMockData({
consumer: 'alerting',
});
expect(() => {
migration711(alert, migrationContext);
}).toThrowError(`Can't migrate!`);
expect(migrationContext.log.error).toHaveBeenCalledWith(
`encryptedSavedObject 7.11.0 migration failed for alert ${alert.id} with error: Can't migrate!`,
{
migrations: {
alertDocument: {
...alert,
attributes: {
...alert.attributes,
},
},
},
}
);
});
});

describe('7.11.2 throws if migration fails', () => {
test('should show the proper exception', () => {
const migration7112 = getMigrations(encryptedSavedObjectsSetup)['7.11.2'];
const alert = getMockData({
consumer: 'alerting',
});
expect(() => {
migration7112(alert, migrationContext);
}).toThrowError(`Can't migrate!`);
expect(migrationContext.log.error).toHaveBeenCalledWith(
`encryptedSavedObject 7.11.2 migration failed for alert ${alert.id} with error: Can't migrate!`,
{
migrations: {
alertDocument: {
...alert,
attributes: {
...alert.attributes,
},
},
},
}
);
});
});

describe('7.13.0 throws if migration fails', () => {
test('should show the proper exception', () => {
const migration7130 = getMigrations(encryptedSavedObjectsSetup)['7.13.0'];
const alert = getMockData({
consumer: 'alerting',
});
expect(() => {
migration7130(alert, migrationContext);
}).toThrowError(`Can't migrate!`);
expect(migrationContext.log.error).toHaveBeenCalledWith(
`encryptedSavedObject 7.13.0 migration failed for alert ${alert.id} with error: Can't migrate!`,
{
migrations: {
alertDocument: {
...alert,
attributes: {
...alert.attributes,
},
},
},
}
);
});
});
});

Expand Down
35 changes: 35 additions & 0 deletions x-pack/plugins/alerting/server/saved_objects/migrations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,11 +73,18 @@ export function getMigrations(
pipeMigrations(removeNullsFromSecurityRules)
);

const migrationSecurityRules714 = createEsoMigration(
encryptedSavedObjects,
(doc): doc is SavedObjectUnsanitizedDoc<RawAlert> => isSecuritySolutionRule(doc),
pipeMigrations(removeNullAuthorFromSecurityRules)
);

return {
'7.10.0': executeMigrationWithErrorHandling(migrationWhenRBACWasIntroduced, '7.10.0'),
'7.11.0': executeMigrationWithErrorHandling(migrationAlertUpdatedAtAndNotifyWhen, '7.11.0'),
'7.11.2': executeMigrationWithErrorHandling(migrationActions7112, '7.11.2'),
'7.13.0': executeMigrationWithErrorHandling(migrationSecurityRules713, '7.13.0'),
'7.14.1': executeMigrationWithErrorHandling(migrationSecurityRules714, '7.14.1'),
};
}

Expand Down Expand Up @@ -420,6 +427,34 @@ function removeNullsFromSecurityRules(
};
}

/**
* The author field was introduced later and was not part of the original rules. We overlooked
* the filling in the author field as an empty array in an earlier upgrade routine from
* 'removeNullsFromSecurityRules' during the 7.13.0 upgrade. Since we don't change earlier migrations,
* but rather only move forward with the "arrow of time" we are going to upgrade and fix
* it if it is missing for anyone in 7.14.0 and above release. Earlier releases if we want to fix them,
* would have to be modified as a "7.13.1", etc... if we want to fix it there.
* @param doc The document that is not migrated and contains a "null" or "undefined" author field
* @returns The document with the author field fleshed in.
*/
function removeNullAuthorFromSecurityRules(
doc: SavedObjectUnsanitizedDoc<RawAlert>
): SavedObjectUnsanitizedDoc<RawAlert> {
const {
attributes: { params },
} = doc;
return {
...doc,
attributes: {
...doc.attributes,
params: {
...params,
author: params.author != null ? params.author : [],
},
},
};
}

function pipeMigrations(...migrations: AlertMigration[]): AlertMigration {
return (doc: SavedObjectUnsanitizedDoc<RawAlert>) =>
migrations.reduce((migratedDoc, nextMigration) => nextMigration(migratedDoc), doc);
Expand Down

0 comments on commit 8ad1ab2

Please sign in to comment.