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.

Sign up for GitHub

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

[Cases] Add guardrails for add and update comment API #161200

Merged
merged 9 commits into from
Jul 6, 2023
Merged

[Cases] Add guardrails for add and update comment API #161200

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
dd10222
add guardrails and tests for post and patch comment
js-jankisalvi Jul 4, 2023
f87deb5
update the docs
js-jankisalvi Jul 4, 2023
c719484
PR feedback
js-jankisalvi Jul 5, 2023
74868c0
[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…
kibanamachine Jul 5, 2023
a3795f6
added path reporter to check validation for commentRequestRt
js-jankisalvi Jul 5, 2023
28004e9
Merge branch 'guardrail-api-comment-characters' of https://github.com…
js-jankisalvi Jul 5, 2023
480adef
added tests for commentRequestRt
js-jankisalvi Jul 5, 2023
75eb73a
Merge branch 'main' into guardrail-api-comment-characters
kibanamachine Jul 6, 2023
3ca8625
lint fix
js-jankisalvi Jul 6, 2023
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
18 changes: 18 additions & 0 deletions x-pack/plugins/cases/common/api/cases/comment/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
BulkGetAttachmentsRequestRt,
BulkGetAttachmentsResponseRt,
} from '.';
import { MAX_COMMENT_LENGTH } from '../../../constants';

describe('Comments', () => {
describe('CommentAttributesBasicRt', () => {
Expand Down Expand Up @@ -340,6 +341,23 @@ describe('Comments', () => {
right: defaultRequest,
});
});

it.skip('throws error when comment is too long', () => {
js-jankisalvi marked this conversation as resolved.
Show resolved Hide resolved
const longComment = 'x'.repeat(MAX_COMMENT_LENGTH + 1);

expect(CommentRequestRt.decode({ ...defaultRequest, comment: longComment })).toMatchObject({
_tag: 'Left',
left: [{
context:[{
actual: {
...defaultRequest,
comment: longComment,
message: "The length of the comment is too long. The maximum length is 30000."
}
}]
}]
});
});
});

describe('CommentRt', () => {
Expand Down
28 changes: 27 additions & 1 deletion x-pack/plugins/cases/common/api/cases/comment/index.ts
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

Let's add some tests for the CommentRequestRt to verify the validation. In general, I think it is nice if we created a strong linked chain of tests. For example, we test the CommentRequestRt with all possible testing scenarios. Then we test the addComment and bulkCreate but there there is no need to check all possible testing scenarios (unless there is new logic on top of it). One is enough to ensure the integration between the function and the schema. The chain now is addComment -> CommentRequestRt. If the tests in CommentRequestRt pass we are sure that addComment will work correctly because we have at least one test that tests the integration between the two. Lastly, with integration tests, we check the integration between the addComment and the route.

This article explains the concept in detail: https://www.jamesshore.com/v2/projects/nullables/testing-without-mocks#sociable-tests

cc @adcoelho

Copy link
Contributor

Choose a reason for hiding this comment

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

Then we test the addComment and bulkCreate but there there is no need to check all possible testing scenarios (unless there is new logic on top of it).

I wondered about this but we need to guarantee somehow that (in this case) CommentRequestRt is being used by addComment and bulkCreate.

Is this what you mean with

because we have at least one test that tests the integration between the two.

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh okay, so just checking if I understood it correctly,

  • all the tests of errors like long comment, empty string etc should be part of CommentRequestRt tests in x-pack/plugins/cases/common/api/cases/comment/index.test.ts
  • where as addComment, update and bulk create just tests the integration with CommentRequestRt with one test
  • and same with api integration tests to test route and post_comment, patch_comment, bulk_create_attachments integration with one test?

Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import { jsonValueRt } from '../../runtime_types';
import { NumberFromString } from '../../saved_object';

import { UserRt } from '../../user';
import { limitedStringSchema } from '../../../schema';
import { MAX_COMMENT_LENGTH } from '../../../constants';

export * from './files';

Expand Down Expand Up @@ -192,7 +194,31 @@ const BasicCommentRequestRt = rt.union([
PersistableStateAttachmentRt,
]);

export const CommentRequestRt = rt.union([BasicCommentRequestRt, ExternalReferenceSORt]);
export const CommentRequestRt = rt.union([
rt.strict({
comment: limitedStringSchema({ fieldName: 'comment', min: 1, max: MAX_COMMENT_LENGTH }),
type: rt.literal(CommentType.user),
owner: rt.string,
}),
AlertCommentRequestRt,
rt.strict({
type: rt.literal(CommentType.actions),
comment: limitedStringSchema({ fieldName: 'comment', min: 1, max: MAX_COMMENT_LENGTH }),
actions: rt.strict({
targets: rt.array(
rt.strict({
hostname: rt.string,
endpointId: rt.string,
})
),
type: rt.string,
}),
owner: rt.string,
}),
ExternalReferenceNoSORt,
ExternalReferenceSORt,
PersistableStateAttachmentRt,
]);

export const CommentRequestWithoutRefsRt = rt.union([
BasicCommentRequestRt,
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/cases/common/constants/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ export const MAX_REPORTERS_FILTER_LENGTH = 100 as const;
export const MAX_TITLE_LENGTH = 160 as const;
export const MAX_CATEGORY_LENGTH = 50 as const;
export const MAX_DESCRIPTION_LENGTH = 30000 as const;
export const MAX_COMMENT_LENGTH = 30000 as const;
export const MAX_LENGTH_PER_TAG = 256 as const;
export const MAX_TAGS_PER_CASE = 200 as const;
export const MAX_DELETE_IDS_LENGTH = 100 as const;
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/cases/docs/openapi/bundled.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5568,6 +5568,7 @@
"comment": {
"description": "The new comment. It is required only when `type` is `user`.",
"type": "string",
"maxLength": 30000,
"example": "A new comment."
},
"owner": {
Expand Down Expand Up @@ -5657,6 +5658,7 @@
"comment": {
"description": "The new comment. It is required only when `type` is `user`.",
"type": "string",
"maxLength": 30000,
"example": "A new comment."
},
"id": {
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/cases/docs/openapi/bundled.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3606,6 +3606,7 @@ components:
comment:
description: The new comment. It is required only when `type` is `user`.
type: string
maxLength: 30000
example: A new comment.
owner:
$ref: '#/components/schemas/owners'
Expand Down Expand Up @@ -3672,6 +3673,7 @@ components:
comment:
description: The new comment. It is required only when `type` is `user`.
type: string
maxLength: 30000
example: A new comment.
id:
type: string
Expand Down
1 change: 1 addition & 0 deletions ...ck/plugins/cases/docs/openapi/components/schemas/add_user_comment_request_properties.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ properties:
comment:
description: The new comment. It is required only when `type` is `user`.
type: string
maxLength: 30000
example: A new comment.
owner:
$ref: 'owners.yaml'
Expand Down
1 change: 1 addition & 0 deletions ...plugins/cases/docs/openapi/components/schemas/update_user_comment_request_properties.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ properties:
comment:
description: The new comment. It is required only when `type` is `user`.
type: string
maxLength: 30000
example: A new comment.
id:
type: string
Expand Down
27 changes: 27 additions & 0 deletions x-pack/plugins/cases/server/client/attachments/add.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import { comment } from '../../mocks';
import { createCasesClientMockArgs } from '../mocks';
import { MAX_COMMENT_LENGTH } from '../../../common/constants';
import { addComment } from './add';

describe('addComment', () => {
Expand All @@ -22,4 +23,30 @@ describe('addComment', () => {
addComment({ comment: { ...comment, foo: 'bar' }, caseId: 'test-case' }, clientArgs)
).rejects.toThrow('invalid keys "foo"');
});

it('should throw an error if the comment length is too long', async () => {
const longComment = 'x'.repeat(MAX_COMMENT_LENGTH + 1);

await expect(
addComment({ comment: { ...comment, comment: longComment }, caseId: 'test-case' }, clientArgs)
).rejects.toThrow(
`Failed while adding a comment to case id: test-case error: Error: The length of the comment is too long. The maximum length is ${MAX_COMMENT_LENGTH}.`
);
});

it('should throw an error if the comment is an empty string', async () => {
await expect(
addComment({ comment: { ...comment, comment: '' }, caseId: 'test-case' }, clientArgs)
).rejects.toThrow(
'Failed while adding a comment to case id: test-case error: Error: The comment field cannot be an empty string.'
);
});

it('should throw an error if the description is a string with empty characters', async () => {
await expect(
addComment({ comment: { ...comment, comment: ' ' }, caseId: 'test-case' }, clientArgs)
).rejects.toThrow(
'Failed while adding a comment to case id: test-case error: Error: The comment field cannot be an empty string.'
);
});
});
78 changes: 77 additions & 1 deletion x-pack/plugins/cases/server/client/attachments/bulk_create.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@
* 2.0.
*/

import { comment } from '../../mocks';
import { comment, actionComment } from '../../mocks';
import { createCasesClientMockArgs } from '../mocks';
import { MAX_COMMENT_LENGTH } from '../../../common/constants';
import { bulkCreate } from './bulk_create';

describe('bulkCreate', () => {
Expand All @@ -22,4 +23,79 @@ describe('bulkCreate', () => {
bulkCreate({ attachments: [{ ...comment, foo: 'bar' }], caseId: 'test-case' }, clientArgs)
).rejects.toThrow('invalid keys "foo"');
});

describe('comments', () => {
it('should throw an error if the comment length is too long', async () => {
const longComment = Array(MAX_COMMENT_LENGTH + 1)
.fill('x')
.toString();

await expect(
bulkCreate(
{ attachments: [{ ...comment, comment: longComment }], caseId: 'test-case' },
clientArgs
)
).rejects.toThrow(
`Failed while bulk creating attachment to case id: test-case error: Error: The length of the comment is too long. The maximum length is ${MAX_COMMENT_LENGTH}.`
);
});

it('should throw an error if the comment is an empty string', async () => {
await expect(
bulkCreate({ attachments: [{ ...comment, comment: '' }], caseId: 'test-case' }, clientArgs)
).rejects.toThrow(
'Failed while bulk creating attachment to case id: test-case error: Error: The comment field cannot be an empty string.'
);
});

it('should throw an error if the description is a string with empty characters', async () => {
await expect(
bulkCreate(
{ attachments: [{ ...comment, comment: ' ' }], caseId: 'test-case' },
clientArgs
)
).rejects.toThrow(
'Failed while bulk creating attachment to case id: test-case error: Error: The comment field cannot be an empty string.'
);
});
});

describe('actions', () => {
it('should throw an error if the comment length is too long', async () => {
const longComment = Array(MAX_COMMENT_LENGTH + 1)
.fill('x')
.toString();

await expect(
bulkCreate(
{ attachments: [{ ...actionComment, comment: longComment }], caseId: 'test-case' },
clientArgs
)
).rejects.toThrow(
`Failed while bulk creating attachment to case id: test-case error: Error: The length of the comment is too long. The maximum length is ${MAX_COMMENT_LENGTH}.`
);
});

it('should throw an error if the comment is an empty string', async () => {
await expect(
bulkCreate(
{ attachments: [{ ...actionComment, comment: '' }], caseId: 'test-case' },
clientArgs
)
).rejects.toThrow(
'Failed while bulk creating attachment to case id: test-case error: Error: The comment field cannot be an empty string.'
);
});

it('should throw an error if the description is a string with empty characters', async () => {
await expect(
bulkCreate(
{ attachments: [{ ...actionComment, comment: ' ' }], caseId: 'test-case' },
clientArgs
)
).rejects.toThrow(
'Failed while bulk creating attachment to case id: test-case error: Error: The comment field cannot be an empty string.'
);
});
});
});
100 changes: 100 additions & 0 deletions x-pack/plugins/cases/server/client/attachments/update.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { comment, actionComment } from '../../mocks';
import { createCasesClientMockArgs } from '../mocks';
import { MAX_COMMENT_LENGTH } from '../../../common/constants';
import { update } from './update';

describe('update', () => {
const clientArgs = createCasesClientMockArgs();

beforeEach(() => {
jest.clearAllMocks();
});

describe('comments', () => {
const updateComment = { ...comment, id: 'comment-id', version: 'WzAsMV0=' };
it('should throw an error if the comment length is too long', async () => {
const longComment = Array(MAX_COMMENT_LENGTH + 1)
.fill('x')
.toString();

await expect(
update(
{ updateRequest: { ...updateComment, comment: longComment }, caseID: 'test-case' },
clientArgs
)
).rejects.toThrow(
`Failed to patch comment case id: test-case: Error: The length of the comment is too long. The maximum length is ${MAX_COMMENT_LENGTH}.`
);
});

it('should throw an error if the comment is an empty string', async () => {
await expect(
update(
{ updateRequest: { ...updateComment, comment: '' }, caseID: 'test-case' },
clientArgs
)
).rejects.toThrow(
'Failed to patch comment case id: test-case: Error: The comment field cannot be an empty string.'
);
});

it('should throw an error if the description is a string with empty characters', async () => {
await expect(
update(
{ updateRequest: { ...updateComment, comment: ' ' }, caseID: 'test-case' },
clientArgs
)
).rejects.toThrow(
'Failed to patch comment case id: test-case: Error: The comment field cannot be an empty string.'
);
});
});

describe('actions', () => {
const updateActionComment = { ...actionComment, id: 'comment-id', version: 'WzAsMV0=' };
Copy link
Contributor

@adcoelho adcoelho Jul 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

what is the difference between updateComment and actionComment?

Edit: I know it is the type 😅 but more specifically, how are they different in practice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think @cnasikas can answer it 😄

Copy link
Member

Choose a reason for hiding this comment

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

If a user isolates a host from the alerts flyout the security solution team adds a comment to a case + some information about the host etc. At the time we did not have the attachment framework so this became a core attachment type.


it('should throw an error if the comment length is too long', async () => {
const longComment = Array(MAX_COMMENT_LENGTH + 1)
.fill('x')
.toString();

await expect(
update(
{ updateRequest: { ...updateActionComment, comment: longComment }, caseID: 'test-case' },
clientArgs
)
).rejects.toThrow(
`Failed to patch comment case id: test-case: Error: The length of the comment is too long. The maximum length is ${MAX_COMMENT_LENGTH}.`
);
});

it('should throw an error if the comment is an empty string', async () => {
await expect(
update(
{ updateRequest: { ...updateActionComment, comment: '' }, caseID: 'test-case' },
clientArgs
)
).rejects.toThrow(
'Failed to patch comment case id: test-case: Error: The comment field cannot be an empty string.'
);
});

it('should throw an error if the description is a string with empty characters', async () => {
await expect(
update(
{ updateRequest: { ...updateActionComment, comment: ' ' }, caseID: 'test-case' },
clientArgs
)
).rejects.toThrow(
'Failed to patch comment case id: test-case: Error: The comment field cannot be an empty string.'
);
});
});
});
16 changes: 16 additions & 0 deletions x-pack/plugins/cases/server/mocks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import type { SavedObject } from '@kbn/core/server';
import type {
CasePostRequest,
CommentAttributes,
CommentRequestActionsType,
CommentRequestAlertType,
CommentRequestUserType,
ConnectorMappings,
Expand Down Expand Up @@ -664,6 +665,21 @@ export const comment: CommentRequestUserType = {
owner: SECURITY_SOLUTION_OWNER,
};

export const actionComment: CommentRequestActionsType = {
type: CommentType.actions,
comment: 'I just isolated the host!',
actions: {
targets: [
{
hostname: 'host1',
endpointId: '001',
},
],
type: 'isolate',
},
owner: 'cases',
};

export const alertComment: CommentRequestAlertType = {
alertId: 'alert-id-1',
index: 'alert-index-1',
Expand Down
Loading