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

Change doc level query name validation #630

Merged
merged 6 commits into from
Apr 12, 2024
Merged

Change doc level query name validation #630

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6ed40de
change validation and tests
jowg-amazon Apr 5, 2024
150413f
change regex to 0-256 chars
jowg-amazon Apr 5, 2024
aa2b6c8
moved validation to common utils and fix test
jowg-amazon Apr 8, 2024
272513e
remove TODO
jowg-amazon Apr 10, 2024
5c8888f
ensure name is at lest 1 char
jowg-amazon Apr 12, 2024
6904220
change error message
jowg-amazon Apr 12, 2024
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: 12 additions & 6 deletions src/main/kotlin/org/opensearch/commons/alerting/model/DocLevelQuery.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ data class DocLevelQuery(

init {
// Ensure the name and tags have valid characters
validateQuery(name)
validateQueryName(name)
for (tag in tags) {
validateQuery(tag)
validateQueryTag(tag)
}
}

Expand Down Expand Up @@ -80,6 +80,7 @@ data class DocLevelQuery(
const val QUERY_FIELD_NAMES_FIELD = "query_field_names"
const val NO_ID = ""
val INVALID_CHARACTERS: List<String> = listOf(" ", "[", "]", "{", "}", "(", ")")
val QUERY_NAME_REGEX = "^.{0,256}$".toRegex() // regex to restrict string length 256 chars
Copy link

Choose a reason for hiding this comment

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

Doubt: How did we decide to cap the length at 256?

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

Copy link
Collaborator

Choose a reason for hiding this comment

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

Another comment - this regex differs from the one in alerting: https://github.com/opensearch-project/alerting/pull/1506/files#diff-c8b7d067c22deb1581efc7a7286769d1c24d1a96146f8a4ecb92ef0dec16c44aR53

What's the reason for a difference?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I chose 256 as the cap based on security analytics front end validation for a rule name in hopes of remaining somewhat consistent:
https://github.com/opensearch-project/security-analytics-dashboards-plugin/blob/3e01e1631fa8aaff4e0968c68424a81abff03981/public/utils/validation.ts#L14

I had to change the length to 0-256 because when security analytics creates findings from bucket level monitors, it will create a doc level query with an empty name.
https://github.com/opensearch-project/security-analytics/blob/0507239054d238dac1e9cf53cfde488aeb4be1c2/src/main/java/org/opensearch/securityanalytics/findings/FindingsService.java#L302

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The regex is different from alerting and security analytics because when security analytics converts the prepackaged sigma rules into doc level queries, there are some existing prepackaged rules that have some of the special characters like ( so we can't include all the validation at the doc level query level.


@JvmStatic
@Throws(IOException::class)
Expand All @@ -100,7 +101,7 @@ data class DocLevelQuery(
QUERY_ID_FIELD -> id = xcp.text()
NAME_FIELD -> {
name = xcp.text()
validateQuery(name)
validateQueryName(name)
}

QUERY_FIELD -> query = xcp.text()
Expand All @@ -112,7 +113,7 @@ data class DocLevelQuery(
)
while (xcp.nextToken() != XContentParser.Token.END_ARRAY) {
val tag = xcp.text()
validateQuery(tag)
validateQueryTag(tag)
tags.add(tag)
}
}
Expand Down Expand Up @@ -160,15 +161,20 @@ data class DocLevelQuery(
}

// TODO: add test for this
Copy link

Choose a reason for hiding this comment

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

Do you want to address this as part of this PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This PR modifies the existing test in DocLevelMonitorInputTests, should we add more testing for this? If not I will remove this comment.

common-utils/src/test/kotlin/org/opensearch/commons/alerting/model/DocLevelMonitorInputTests.kt

Line 45 in 150413f

fun `test create Doc Level Query with invalid name length`() {

Copy link

Choose a reason for hiding this comment

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

If there is no test for the part of the code that the TODO was added for, then let's add one?

Copy link
Collaborator Author

@jowg-amazon jowg-amazon Apr 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

These two unit tests should be sufficient to test the two validation functions. I think the TODO may have been left in after the tests were created. These two tests create an invalid doc level query so that validation for a doc level query name and doc level query tag exceptions are caught.

  1. common-utils/src/test/kotlin/org/opensearch/commons/alerting/model/DocLevelMonitorInputTests.kt

    Line 45 in aa2b6c8

    fun `test create Doc Level Query with invalid name length`() {

  2. common-utils/src/test/kotlin/org/opensearch/commons/alerting/model/DocLevelMonitorInputTests.kt

    Line 65 in aa2b6c8

    fun `test create Doc Level Query with invalid characters for tags`() {

Will take out the TODO comment

private fun validateQuery(stringVal: String) {
private fun validateQueryTag(stringVal: String) {
for (inValidChar in INVALID_CHARACTERS) {
if (stringVal.contains(inValidChar)) {
throw IllegalArgumentException(
"They query name or tag, $stringVal, contains an invalid character: [' ','[',']','{','}','(',')']"
"The query tag, $stringVal, contains an invalid character: [' ','[',']','{','}','(',')']"
)
}
}
}
private fun validateQueryName(stringVal: String) {
if (!stringVal.matches(QUERY_NAME_REGEX)) {
throw IllegalArgumentException("The query name, $stringVal, can be max 256 characters.")
}
}
}

// constructor for java plugins' convenience to optionally avoid passing empty list for 'fieldsBeingQueried' field
Expand Down
13 changes: 9 additions & 4 deletions src/test/kotlin/org/opensearch/commons/alerting/model/DocLevelMonitorInputTests.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,19 @@ class DocLevelMonitorInputTests {
}

@Test
fun `test create Doc Level Query with invalid characters for name`() {
val badString = "query with space"
fun `test create Doc Level Query with invalid name length`() {
val stringBuilder = StringBuilder()
repeat(256) {
stringBuilder.append("a")
}
val badString = stringBuilder.toString() + "b".repeat(256)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor: Looks like this is testing a string of length 512. Would be better to test at the limit (257) imo

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed testing to 257 chars


try {
randomDocLevelQuery(name = badString)
Assertions.fail("Expecting an illegal argument exception")
} catch (e: IllegalArgumentException) {
Assertions.assertEquals(
"They query name or tag, $badString, contains an invalid character: [' ','[',']','{','}','(',')']",
"The query name, $badString, can be max 256 characters.",
e.message
)
}
Expand All @@ -64,7 +69,7 @@ class DocLevelMonitorInputTests {
Assertions.fail("Expecting an illegal argument exception")
} catch (e: IllegalArgumentException) {
Assertions.assertEquals(
"They query name or tag, $badString, contains an invalid character: [' ','[',']','{','}','(',')']",
"The query tag, $badString, contains an invalid character: [' ','[',']','{','}','(',')']",
e.message
)
}
Expand Down
Loading