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

[SIEM] Fixes escape bug for filterQuery #43030

Merged
merged 11 commits into from
Sep 6, 2019

Conversation

stephmilovic
Copy link
Contributor

@stephmilovic stephmilovic commented Aug 9, 2019

Summary

@spong noticed a bug where when searching fields with escaped values, nothing would get returned: #42866

After playing with the query, I noticed Elasticsearch does not enjoy getting escaped values on strings.

This does not have matches:

"match_phrase": {
  "event.action": "Registry value set \\(rule\\: RegistryEvent\\)"
}

This has matches

"match_phrase": {
  "event.action": "Registry value set (rule: RegistryEvent)"
}

I fixed the bug by surrounding all strings in returned in escapeQueryValue with ". This way we only need to escape " and it eliminates the bugs we were seeing.

To test:
Check the following timeline links return results:

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@stephmilovic stephmilovic added Team:SIEM release_note:skip Skip the PR/issue when compiling release notes labels Aug 9, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/siem

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@FrankHassanabad FrankHassanabad changed the title [SIEM] fix escape bug for filterQuery [SIEM] Fixes escape bug for filterQuery Aug 9, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@FrankHassanabad FrankHassanabad left a comment

Choose a reason for hiding this comment

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

Tested it out and played with it and it fixes the issue.

If there are changes required of the downstream libs and a new PR for the downstream lib is created just tack it on optionally I would say.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@stephmilovic
Copy link
Contributor Author

update: @XavierM and i need to pair on this further

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@spong spong added the v7.5.0 label Sep 4, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@XavierM XavierM left a comment

Choose a reason for hiding this comment

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

I tested locally, and I agreed that match_phrase will do the job, no need to be stubburn and use only used match in dsl

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@stephmilovic stephmilovic merged commit d909457 into elastic:master Sep 6, 2019
@stephmilovic stephmilovic deleted the query-builder-esc branch September 6, 2019 14:31
stephmilovic added a commit to stephmilovic/kibana that referenced this pull request Sep 6, 2019
stephmilovic added a commit to stephmilovic/kibana that referenced this pull request Sep 6, 2019
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 6, 2019
…ete-for-distance_feature

* 'master' of github.com:elastic/kibana:
  [SIEM] Fixes escape bug for filterQuery  (elastic#43030)
  Export saved objects based on search criteria (elastic#44723)
  refactor(webhook-whitelisting): Removed unneeded schema config (elastic#44974)
  [APM] Make number of x ticks responsive to the plot width (elastic#44870)
  [ML] Single metric viewer: Fix top nav refresh behaviour. (elastic#44860)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:SIEM v7.4.0 v7.5.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants