-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[Security Solution] [AI Assistant] Replace polynomial regular expression with constant time string manipulation #209314
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
Changes from all commits
8f59b95
b03396a
23a2503
edcefc1
196a01d
791b965
b3fec5d
3778d7d
af99911
647fb00
4e5663e
a399f4f
dd972ff
79ea623
92f9d52
4ddac67
5ffbcdf
55ef292
fcfaccd
86e9ef3
e5482f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| /* | ||
| * 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 { removeContentReferences } from './utils'; | ||
|
|
||
| describe('utils', () => { | ||
| it.each([ | ||
| ['this has no content references', 'this has no content references'], | ||
| [ | ||
| 'The sky is blue{reference(1234)} and the grass is green{reference(4321)}', | ||
| 'The sky is blue and the grass is green', | ||
| ], | ||
| ['', ''], | ||
| ['{reference(1234)}', ''], | ||
| [' {reference(1234)} ', ' '], | ||
| ['{reference(1234', '{reference(1234'], | ||
| ['{reference(1234)', '{reference(1234)'], | ||
| ['{reference(1234)}{reference(1234)}{reference(1234)}', ''], | ||
| ['{reference(1234)}reference(1234)}{reference(1234)}', 'reference(1234)}'], | ||
| ])('removesContentReferences from "%s"', (input: string, expected: string) => { | ||
| const result = removeContentReferences(input); | ||
|
|
||
| expect(result).toEqual(expected); | ||
| }); | ||
|
|
||
| // https://github.com/elastic/kibana/security/code-scanning/539 | ||
| it('removesContentReferences does not run in polynomial time', () => { | ||
| const input = `${'{reference('.repeat(100000)}x${')'.repeat(100000)}`; | ||
| const startTime = performance.now(); // Start timing | ||
|
|
||
| removeContentReferences(input); | ||
|
|
||
| const endTime = performance.now(); // End timing | ||
| const executionTime = endTime - startTime; // Time in milliseconds | ||
|
|
||
| expect(executionTime).toBeLessThan(1000); // Assert under 1 second | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Executes in < 1ms so this won't become a flakey test.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
😂 auto reply from CI: Jokes aside, to test this test locally, I restored the original implementation of export const removeContentReferences = (content: string) => {
return content.replaceAll(/\{reference\(.*?\)\}/g, '');
};and re-ran the test. It failed with: After restoring the PR version of |
||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| /* | ||
| * 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 { | ||
| BEDROCK_SYSTEM_PROMPT, | ||
| DEFAULT_SYSTEM_PROMPT, | ||
| GEMINI_SYSTEM_PROMPT, | ||
| STRUCTURED_SYSTEM_PROMPT, | ||
| } from './prompts'; | ||
|
|
||
| describe('prompts', () => { | ||
| it.each([ | ||
| [DEFAULT_SYSTEM_PROMPT, '{include_citations_prompt_placeholder}', 1], | ||
| [GEMINI_SYSTEM_PROMPT, '{include_citations_prompt_placeholder}', 1], | ||
| [BEDROCK_SYSTEM_PROMPT, '{include_citations_prompt_placeholder}', 1], | ||
| [STRUCTURED_SYSTEM_PROMPT, '{include_citations_prompt_placeholder}', 1], | ||
| [DEFAULT_SYSTEM_PROMPT, 'You are a security analyst', 1], | ||
| [GEMINI_SYSTEM_PROMPT, 'You are an assistant', 1], | ||
| [BEDROCK_SYSTEM_PROMPT, 'You are a security analyst', 1], | ||
| ])( | ||
| '"%s" contains "%s" %s times', | ||
| (prompt: string, containedString: string, expectedCount: number) => { | ||
| const regex = new RegExp(containedString, 'g'); | ||
| expect((prompt.match(regex) || []).length).toBe(expectedCount); | ||
| } | ||
| ); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,7 +11,8 @@ import { EuiLink } from '@elastic/eui'; | |
| import type { ContentReferenceNode } from '../content_reference_parser'; | ||
| import { PopoverReference } from './popover_reference'; | ||
| import { SECURITY_ALERTS_PAGE_REFERENCE_LABEL } from './translations'; | ||
| import { useKibana } from '../../../../common/lib/kibana'; | ||
| import { useNavigateToAlertsPageWithFilters } from '../../../../common/hooks/use_navigate_to_alerts_page_with_filters'; | ||
| import { FILTER_OPEN, FILTER_ACKNOWLEDGED } from '../../../../../common/types'; | ||
|
|
||
| interface Props { | ||
| contentReferenceNode: ContentReferenceNode; | ||
|
|
@@ -22,17 +23,22 @@ export const SecurityAlertsPageReference: React.FC<Props> = ({ | |
| contentReferenceNode, | ||
| securityAlertsPageContentReference, | ||
| }) => { | ||
| const { navigateToApp } = useKibana().services.application; | ||
| const openAlertsPageWithFilters = useNavigateToAlertsPageWithFilters(); | ||
|
|
||
| const onClick = useCallback( | ||
| (e: React.MouseEvent) => { | ||
| e.preventDefault(); | ||
| navigateToApp('security', { | ||
| path: `alerts`, | ||
| openInNewTab: true, | ||
| }); | ||
| openAlertsPageWithFilters( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for adding these filters! |
||
| { | ||
| selectedOptions: [FILTER_OPEN, FILTER_ACKNOWLEDGED], | ||
| fieldName: 'kibana.alert.workflow_status', | ||
| persist: false, | ||
| }, | ||
| true, | ||
| '(global:(timerange:(fromStr:now-24h,kind:relative,toStr:now)))' | ||
| ); | ||
| }, | ||
| [navigateToApp] | ||
| [openAlertsPageWithFilters] | ||
| ); | ||
|
|
||
| return ( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I observed the following unexpected output while desk testing:
I attempted to reproduce it via a test case, but the following test passes:
The unreplaced references are still visible when the conversation is re-opened, and when the
Show citationstoggle is clicked, as illustrated by the animated gif below:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
observation: The poem contains a single entry
Knowledge base entry: Favorite Color, however that KB entry does not appear to be related to the poem:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So there are 2 problems here: