[Security Solution] Exceptions flyout refreshes when it regains focus, causing configured fields to get cleared #166550

peluja1012 · 2023-09-14T23:47:24Z

Describe the bug:
The Exception flyout refreshes and clears all configured fields when the browser loses and then regains focus.

Kibana/Elasticsearch Stack version:
8.10

Browser and Browser OS versions:
Chrome, MS Edge (originally reported)

Steps to reproduce:

Click on "Add Rule Exception" either from an Alert from from the Rule Details page
Add some values to the form. For example, title, and fields with AND conditions.
Click on another tab on your browser so that the Exception flyout loses focus.
Click on the main tab again so that the Exception flyout regains focus.
Notice how the Exceptions flyout reloads, causing all configured fields to get cleared. You may have to repeat Steps 3 and 4 a couple of times before you see the unwanted behavior.

Screenshots (if relevant):

Screen.Recording.2023-09-14.at.5.19.44.PM.mov

elasticmachine · 2023-09-14T23:47:27Z

Pinging @elastic/security-solution (Team: SecuritySolution)

elasticmachine · 2023-09-15T00:28:45Z

Pinging @elastic/security-detections-response (Team:Detections and Resp)

peluja1012 · 2023-10-27T22:58:13Z

This bug is probably related to this other one, where you can see that additional calls to the /api/detection_engine/rules API are made whenever the flyout regains focus. #170081

peluja1012 · 2023-11-07T21:18:42Z

Another related and possible duplicate bug: #166759

rylnd · 2023-11-07T22:41:26Z

@e40pud since I'm on SDH today I'm investing this one, as it relates to a few tickets that came through. I will update here with my findings.

rylnd · 2023-11-09T00:08:17Z

Alright, so I've traced the cause, but the solution is going to be more involved since it involves changing caching logic.

What's happening is that we have several useQuery hooks that are used within the RuleDetailsPage component that have a TTL of 5 minutes. Between that and the refetchOnWindowFocus being true by default, after a certain period of time has elapsed, focusing the tab will cause those queries to be fetched again, causing the RuleDetailsPage component to rerender.

The following diff will "fix" the problem by always caching those queries:

diff --git a/x-pack/plugins/security_solution/public/common/components/ml/hooks/use_fetch_jobs_summary_query.ts b/x-pack/plugins/security_solution/public/common/components/ml/hooks/use_fetch_jobs_summary_query.ts
index f2958de5a9a..477fa8fe838 100644
--- a/x-pack/plugins/security_solution/public/common/components/ml/hooks/use_fetch_jobs_summary_query.ts
+++ b/x-pack/plugins/security_solution/public/common/components/ml/hooks/use_fetch_jobs_summary_query.ts
@@ -24,7 +24,7 @@ export const useFetchJobsSummaryQuery = (
     async ({ signal }) => getJobsSummary({ signal, ...queryArgs }),
     {
       refetchIntervalInBackground: false,
-      staleTime: ONE_MINUTE * 5,
+      staleTime: Infinity,
       retry: false,
       ...options,
     }
diff --git a/x-pack/plugins/security_solution/public/common/components/ml_popover/hooks/use_fetch_modules_query.ts b/x-pack/plugins/security_solution/public/common/components/ml_popover/hooks/use_fetch_modules_query.ts
index 810801e96a5..4b8146a76c5 100644
--- a/x-pack/plugins/security_solution/public/common/components/ml_popover/hooks/use_fetch_modules_query.ts
+++ b/x-pack/plugins/security_solution/public/common/components/ml_popover/hooks/use_fetch_modules_query.ts
@@ -23,7 +23,7 @@ export const useFetchModulesQuery = (
     async ({ signal }) => getModules({ signal, ...queryArgs }),
     {
       refetchIntervalInBackground: false,
-      staleTime: ONE_MINUTE * 5,
+      staleTime: Infinity,
       retry: false,
       ...options,
     }
diff --git a/x-pack/plugins/security_solution/public/common/components/ml_popover/hooks/use_fetch_recognizer_query.ts b/x-pack/plugins/security_solution/public/common/components/ml_popover/hooks/use_fetch_recognizer_query.ts
index bd17b82cc56..f98799a482e 100644
--- a/x-pack/plugins/security_solution/public/common/components/ml_popover/hooks/use_fetch_recognizer_query.ts
+++ b/x-pack/plugins/security_solution/public/common/components/ml_popover/hooks/use_fetch_recognizer_query.ts
@@ -23,7 +23,7 @@ export const useFetchRecognizerQuery = (
     async ({ signal }) => checkRecognizer({ signal, ...queryArgs }),
     {
       refetchIntervalInBackground: false,
-      staleTime: ONE_MINUTE * 5,
+      staleTime: Infinity,
       retry: false,
       ...options,
     }

with these changes, I was able to re-focus the exceptions window after 10 minutes and not rerender/lose data. (Similarly, if you want to reliably reproduce the issue, change one of the above hooks to staleTime: 0, which will cause you to lose data after any refocus).

Next steps

Can we simply address the issue by setting refetchOnWindowFocus: false for the above queries, or is it more nuanced than that? My understanding is that this should prevent the immediate issue, but if those queries are refetched via some other mechanism the issue will still occur. This leads to:
Can we fix these hooks/component to rerender not based on query time but on actual contents?

In investigating the rerenders, they appear to be due to the query's internal state having its dataFetchedAt field updated, as opposed to actual security data having changed. Is this the expected behavior?

e40pud · 2023-11-09T14:39:26Z

After thinking a bit about this issue, here are couple thoughts:

Since we cannot guarantee that some queries won't be refetched we should fix the exception page to make sure that we preserve the state even between re-renders. I see that exception name and comment are still present after re-rendering. This way we will at least make sure the data loss does not happen.
I was thinking that we probably want to set refetchOnWindowFocus: false to all our queries and if it is really needed to be true then we should discuss and think how to handle that individually. Or it can to be false based on different conditions, for example if we show flyout then main page should not be refetching queries on focus change.

The UI looks buggy with refetchOnWindowFocus: true, since we re-render page there is always some jumping of the content. I tried to disable refetchOnWindowFocus in whole security solution just to see the difference and UI behaves much better.

refetchOnWindowFocus: true

refetch-on-focus-true.mov

vs refetchOnWindowFocus: false

refetch-on-focus-false.mov

We most probably cannot just blindly disable refetchOnWindowFocus everywhere, but maybe we could walk through queries and make sure that we really need that option to be set to true.

rylnd · 2023-11-09T16:33:05Z

That all makes sense to me, @e40pud! Let's see if we can address this at the exceptions component itself, and bring the refetchOnWindowFocus discussion to the larger D&R team.

e40pud · 2023-12-06T10:34:41Z

To fix the state preservation properly would involve some refactoring of exception flyout components. This will take some time and will not fit FF phase.

Discussed with @yctercero and we will do a quick fix which matches current behaviour of the EditExceptionFlyout component where we avoid losing exception conditions state due to not doing conditional rendering of the ExceptionsConditions (the component which keeps conditions state). We will need to move conditions state to the parent component (AddExceptionFlyout and ExceptionsConditions) instead.

Refactoring will be prioritised in the next release https://github.com/elastic/security-team/issues/8197

…, causing configured fields to get cleared (elastic#166550)

…, causing configured fields to get cleared (#166550) (#172666) ## Summary Addresses #166550 These changes fix the issue where user can experience data loss while adding rule exceptions. The main issue is that we keep conditions state inside the child component `ExceptionsConditions` which is rendered conditionally based on `isLoading` flag. This flag changes when `useQuery` data gets stale and refetching is triggered. In this case we would remove `ExceptionsConditions` component while loading data and re-create it after. Since conditions state stored inside `ExceptionsConditions` we will lose it. This is a quick fix to make sure our users are not frustrated. The state refactoring will come separately in the next release when we are going to address the main issue elastic/security-team#8197 To reproduce: 1. Open "add rule exception" flyout 2. Add exception conditions 3. Wait for 5 minutes (to avoid waiting this long you can use [this approach](#166550 (comment))) 4. Remove focus from the page (by switching to another app or navigating to a different tab in a browser) 5. Focus on the page again When you are back to the page all exception conditions should still be there. --------- Co-authored-by: Vitalii Dmyterko <[email protected]>

…, causing configured fields to get cleared (elastic#166550) (elastic#172666) ## Summary Addresses elastic#166550 These changes fix the issue where user can experience data loss while adding rule exceptions. The main issue is that we keep conditions state inside the child component `ExceptionsConditions` which is rendered conditionally based on `isLoading` flag. This flag changes when `useQuery` data gets stale and refetching is triggered. In this case we would remove `ExceptionsConditions` component while loading data and re-create it after. Since conditions state stored inside `ExceptionsConditions` we will lose it. This is a quick fix to make sure our users are not frustrated. The state refactoring will come separately in the next release when we are going to address the main issue elastic/security-team#8197 To reproduce: 1. Open "add rule exception" flyout 2. Add exception conditions 3. Wait for 5 minutes (to avoid waiting this long you can use [this approach](elastic#166550 (comment))) 4. Remove focus from the page (by switching to another app or navigating to a different tab in a browser) 5. Focus on the page again When you are back to the page all exception conditions should still be there. --------- Co-authored-by: Vitalii Dmyterko <[email protected]> (cherry picked from commit a0631cf)

…s focus, causing configured fields to get cleared (#166550) (#172666) (#173131) # Backport This will backport the following commits from `main` to `8.12`: - [[Security Solution] Exceptions flyout refreshes when it regains focus, causing configured fields to get cleared (#166550) (#172666)](#172666)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Ievgen Sorokopud <[email protected]>

e40pud · 2023-12-12T11:14:25Z

@MadameSheema the fix was merged in both 8.12 and main (8.13)

cybersecdiva · 2024-01-13T00:06:46Z

Tested in 8.12.0 BC6

Describe the bug:
The Exception flyout refreshes and clears all configured fields when the browser loses and then regains focus.

Kibana/Elasticsearch Stack version:
8.12.0

Server OS version:
N/A

Browser and Browser OS versions:

Firefox (Mac OS, Linux, and Windows)
Chrome (Mac OS, Linux, and Windows)
Edge

Steps to reproduce:

Navigate to Security -> Alerts
Select an Alert and More Actions -> Add Rule Exception
Add rule exception name
In the form, add a value OR condition value
Click on another tab on your browser so that the Exception flyout loses focus
Click on the main tab again so that the Exception flyout regains focus

Current behavior:
Condition state remains for exceptions flyout after refresh and focus regain and configured fields do not clear

Expected behavior:
Condition state remains for exceptions flyout after refresh and focus regain and configured fields do not clear

Screenshots (if relevant):

Screenshot of Rule Exception in 8.12.0 after refresh of tab and focus regain:

Screen share recordings:

Screen recording of behavior in 8.10.0

exceptionsflyout8.mp4

Screen recording of behavior in 8.12.0

exceptionlist.8.12.mp4

Errors in browser console (if relevant):
N/A

Provide logs and/or server output (if relevant):
N/A

Any additional context (logs, chat logs, magical formulas, etc.):
Behavior was tested on original 8.10.0 and on 8.12.0 deployment on browsers Firefox, Chrome, and Microsoft Edge and configured fields are not cleared
Validation ✅ for 8.12.0 BC6 fixed

@yctercero @e40pud FYI QA Validated fix ✅ Please confirm if expected behavior to align with your updated fix

@MadameSheema @vgomez-el FYI QA Validation of fix in 8.12.0 BC6

vgomez-el · 2024-01-15T11:36:35Z

I am closing this bug since it has been validated properly by @cybersecdiva on 8.12.0 BC6

peluja1012 added bug Fixes for quality problems that affect the customer experience triage_needed Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. labels Sep 14, 2023

peluja1012 added Team:Detections and Resp Security Detection Response Team Team:Detection Engine Security Solution Detection Engine Area labels Sep 15, 2023

peluja1012 added the impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. label Sep 15, 2023

yctercero added consider-next and removed triage_needed labels Oct 13, 2023

yctercero assigned e40pud Oct 27, 2023

yctercero added the v8.12.0 label Oct 27, 2023

peluja1012 added the sdh-linked label Nov 7, 2023

rylnd self-assigned this Nov 7, 2023

e40pud added a commit to e40pud/kibana that referenced this issue Dec 6, 2023

[Security Solution] Exceptions flyout refreshes when it regains focus…

f1f4eef

…, causing configured fields to get cleared (elastic#166550)

e40pud mentioned this issue Dec 6, 2023

[Security Solution] Exceptions flyout refreshes when it regains focus, causing configured fields to get cleared (#166550) #172666

Merged

e40pud added the fixed label Dec 12, 2023

cybersecdiva added the QA:Validated Issue has been validated by QA label Jan 13, 2024

vgomez-el closed this as completed Jan 15, 2024

github-project-automation bot added this to Security Bug Audit Aug 28, 2024

github-project-automation bot moved this to Done in Security Bug Audit Aug 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Exceptions flyout refreshes when it regains focus, causing configured fields to get cleared #166550

[Security Solution] Exceptions flyout refreshes when it regains focus, causing configured fields to get cleared #166550

peluja1012 commented Sep 14, 2023 •

edited

Loading

elasticmachine commented Sep 14, 2023

elasticmachine commented Sep 15, 2023

peluja1012 commented Oct 27, 2023

peluja1012 commented Nov 7, 2023

rylnd commented Nov 7, 2023

rylnd commented Nov 9, 2023

e40pud commented Nov 9, 2023

rylnd commented Nov 9, 2023

e40pud commented Dec 6, 2023

e40pud commented Dec 12, 2023 •

edited

Loading

cybersecdiva commented Jan 13, 2024 •

edited

Loading

vgomez-el commented Jan 15, 2024

[Security Solution] Exceptions flyout refreshes when it regains focus, causing configured fields to get cleared #166550

[Security Solution] Exceptions flyout refreshes when it regains focus, causing configured fields to get cleared #166550

Comments

peluja1012 commented Sep 14, 2023 • edited Loading

elasticmachine commented Sep 14, 2023

elasticmachine commented Sep 15, 2023

peluja1012 commented Oct 27, 2023

peluja1012 commented Nov 7, 2023

rylnd commented Nov 7, 2023

rylnd commented Nov 9, 2023

Next steps

e40pud commented Nov 9, 2023

rylnd commented Nov 9, 2023

e40pud commented Dec 6, 2023

e40pud commented Dec 12, 2023 • edited Loading

cybersecdiva commented Jan 13, 2024 • edited Loading

Tested in 8.12.0 BC6

Screen share recordings:

vgomez-el commented Jan 15, 2024

peluja1012 commented Sep 14, 2023 •

edited

Loading

e40pud commented Dec 12, 2023 •

edited

Loading

cybersecdiva commented Jan 13, 2024 •

edited

Loading