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.

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

[Security solution][Detection Engine] Fixes "Cannot read property 'kibana_siem_app_url' of null error from actions #75844

Merged

Conversation

FrankHassanabad
Copy link
Contributor

@FrankHassanabad FrankHassanabad commented Aug 25, 2020

Summary

Found in 7.9.0, if you post a rule with an action that has a missing "meta" then you are going to get errors in your UI that look something like:

An error occurred during rule execution: message: "Cannot read property 'kibana_siem_app_url' of null"
name: "Unusual Windows Remote User" id: "1cc27e7e-d7c7-4f6a-b918-8c272fc6b1a3"
rule id: "1781d055-5c66-4adf-9e93-fc0fa69550c9" signals index: ".siem-signals-default"

This fixes the accidental referencing of the null/undefined property and adds both integration and unit tests in that area of code.

If you have an action id handy you can manually test this by editing the json file of:

test_cases/queries/action_without_meta.json

to have your action id and then posting it like so:

./post_rule.sh ./rules/test_cases/queries/action_without_meta.json

Checklist

Delete any items that are not applicable to this PR.

@FrankHassanabad FrankHassanabad requested review from a team as code owners August 25, 2020 05:41
@FrankHassanabad FrankHassanabad changed the title [Security solution][Detection Engine]Fixes runtime error with meta when meta is missing [Security solution][Detection Engine] Fixes "Cannot read property 'kibana_siem_app_url' of null error from actions Aug 25, 2020
@FrankHassanabad FrankHassanabad self-assigned this Aug 25, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@@ -79,6 +79,84 @@ describe('rules_notification_alert_type', () => {
);
});

it('should resolve results_link when meta is undefined to use "/app/security"', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I always go back and forth with the wording - I find myself getting confused sometimes (might just be me). In tests when we say x is undefined should we be explicitly be checking that for example, meta: undefined or are we testing that the property itself is not there.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it's one in the same. 🤯

Copy link
Contributor Author

@FrankHassanabad FrankHassanabad Aug 26, 2020

Choose a reason for hiding this comment

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

It's technically not the same but we treat it a lot of the times as the same.

For example:

const a = { b: 1 }
Object.getOwnPropertyDescriptor(a, 'b')
{value: 1, writable: true, enumerable: true, configurable: true}

a.b = undefined;
Object.getOwnPropertyDescriptor(a, 'b')
{value: undefined, writable: true, enumerable: true, configurable: true}

delete a.b;
Object.getOwnPropertyDescriptor(a, 'b')
undefined

Technically you can see that the key is no longer there compared to if I just set it to undefined.

You can do this too:

a.b = undefined;
Object.keys(a)
["b"]

And really the key is still there. But for testing and stuff we might not be as strict when we could be. I sometimes try to use delete to be more exact or do a type of omit.

I am lazy and sometimes just use the wording of undefined to mean the same thing as for the most part it's treated the same unless/until someone is doing Object.keys() or enumerating against it. We do the same thing a lot of times within TypeScript using something as an undefined to indicate a value does not exist regardless of if the value was deleted or turned into undefined. So, some small subtle differences.

@yctercero
Copy link
Contributor

yctercero commented Aug 26, 2020

Thanks for adding all these great tests! The code itself looks great. I had a question about expected behavior. Here's what I did, let me know if it's all expected.

  • Created a rule with an action (via UI)
  • Checked that it ran and actions worked 👍
  • Used API to patch rule to set meta: {}
  • Received an auth error
[error][plugins][taskManager][taskManager] Task actions:.slack "[ID_HERE_I_REMOVED]" failed: [security_exception] missing authentication credentials for REST request [/_security/user/_has_privileges], with { header={ WWW-Authenticate={ 0="Bearer realm=\"security\"" & 1="ApiKey" & 2="Basic realm=\"security\" charset=\"UTF-8\"" } } } ::  
...
  • Rule no longer running
  • Turned rule on then off (via UI) -> now rule and action running as expected

May be easier to zoom tomorrow, I may be missing something. If this is all known/expected, then LGTM. Was no longer seeing the error mentioned in the ticket.

@FrankHassanabad
Copy link
Contributor Author

The auth error you're seeing is probably because while the rule is running if you update it, then the existing running rule has access to an API key that is now invalidated. At least that's we have seen before. You can wait another 5 minutes (for example) and the second run should work out ok. I think the alerting team knows about this.

However, if you were to test a second time and see the same issue consistently (even between rule runs) then we would have a bigger problem.

Copy link
Contributor

@yctercero yctercero left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed replies. It seems then that it is a known issue that I encountered.

I tested:

  • creating a rule w/ an action via api with meta: {} and it worked great 👍
  • creating a rule w/ action via UI 👍
  • updating rule created via UI to have meta: {} and resulted in known issue of needing to enable/disable rule to get working again (unrelated to the issue addressed here)

LGTM!

@FrankHassanabad
Copy link
Contributor Author

@elasticmachine merge upstream

@FrankHassanabad FrankHassanabad merged commit d6c45a2 into elastic:master Aug 26, 2020
@FrankHassanabad FrankHassanabad deleted the fix-action-api-breakage branch August 26, 2020 15:01
FrankHassanabad added a commit to FrankHassanabad/kibana that referenced this pull request Aug 26, 2020
## Summary

Found in 7.9.0, if you post a rule with an action that has a missing "meta" then you are going to get errors in your UI that look something like:

```ts
An error occurred during rule execution: message: "Cannot read property 'kibana_siem_app_url' of null"
name: "Unusual Windows Remote User" id: "1cc27e7e-d7c7-4f6a-b918-8c272fc6b1a3"
rule id: "1781d055-5c66-4adf-9e93-fc0fa69550c9" signals index: ".siem-signals-default"
```

This fixes the accidental referencing of the null/undefined property and adds both integration and unit tests in that area of code.

If you have an action id handy you can manually test this by editing the json file of:

```ts
test_cases/queries/action_without_meta.json
```

to have your action id and then posting it like so:

```ts
./post_rule.sh ./rules/test_cases/queries/action_without_meta.json
```

### Checklist

Delete any items that are not applicable to this PR.

- [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
FrankHassanabad added a commit to FrankHassanabad/kibana that referenced this pull request Aug 26, 2020
## Summary

Found in 7.9.0, if you post a rule with an action that has a missing "meta" then you are going to get errors in your UI that look something like:

```ts
An error occurred during rule execution: message: "Cannot read property 'kibana_siem_app_url' of null"
name: "Unusual Windows Remote User" id: "1cc27e7e-d7c7-4f6a-b918-8c272fc6b1a3"
rule id: "1781d055-5c66-4adf-9e93-fc0fa69550c9" signals index: ".siem-signals-default"
```

This fixes the accidental referencing of the null/undefined property and adds both integration and unit tests in that area of code.

If you have an action id handy you can manually test this by editing the json file of:

```ts
test_cases/queries/action_without_meta.json
```

to have your action id and then posting it like so:

```ts
./post_rule.sh ./rules/test_cases/queries/action_without_meta.json
```

### Checklist

Delete any items that are not applicable to this PR.

- [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
FrankHassanabad added a commit that referenced this pull request Aug 26, 2020
## Summary

Found in 7.9.0, if you post a rule with an action that has a missing "meta" then you are going to get errors in your UI that look something like:

```ts
An error occurred during rule execution: message: "Cannot read property 'kibana_siem_app_url' of null"
name: "Unusual Windows Remote User" id: "1cc27e7e-d7c7-4f6a-b918-8c272fc6b1a3"
rule id: "1781d055-5c66-4adf-9e93-fc0fa69550c9" signals index: ".siem-signals-default"
```

This fixes the accidental referencing of the null/undefined property and adds both integration and unit tests in that area of code.

If you have an action id handy you can manually test this by editing the json file of:

```ts
test_cases/queries/action_without_meta.json
```

to have your action id and then posting it like so:

```ts
./post_rule.sh ./rules/test_cases/queries/action_without_meta.json
```

### Checklist

Delete any items that are not applicable to this PR.

- [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
FrankHassanabad added a commit that referenced this pull request Aug 26, 2020
## Summary

Found in 7.9.0, if you post a rule with an action that has a missing "meta" then you are going to get errors in your UI that look something like:

```ts
An error occurred during rule execution: message: "Cannot read property 'kibana_siem_app_url' of null"
name: "Unusual Windows Remote User" id: "1cc27e7e-d7c7-4f6a-b918-8c272fc6b1a3"
rule id: "1781d055-5c66-4adf-9e93-fc0fa69550c9" signals index: ".siem-signals-default"
```

This fixes the accidental referencing of the null/undefined property and adds both integration and unit tests in that area of code.

If you have an action id handy you can manually test this by editing the json file of:

```ts
test_cases/queries/action_without_meta.json
```

to have your action id and then posting it like so:

```ts
./post_rule.sh ./rules/test_cases/queries/action_without_meta.json
```

### Checklist

Delete any items that are not applicable to this PR.

- [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

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@MindyRS MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021
@elasticmachine
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:fix Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SIEM v7.9.1 v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants