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

Add validation check for doc level query name during monitor creation #1506

Merged
merged 3 commits into from
Apr 12, 2024
Merged

Add validation check for doc level query name during monitor creation #1506

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
135bae5
add validation check for doc level query name during monitor creation
jowg-amazon Apr 5, 2024
cd50847
change to 0-256 chars
jowg-amazon Apr 5, 2024
b490cf7
change validation message and move validation loc
jowg-amazon Apr 8, 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
21 changes: 21 additions & 0 deletions alerting/src/main/kotlin/org/opensearch/alerting/resthandler/RestIndexMonitorAction.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import org.opensearch.commons.alerting.action.AlertingActions
import org.opensearch.commons.alerting.action.IndexMonitorRequest
import org.opensearch.commons.alerting.action.IndexMonitorResponse
import org.opensearch.commons.alerting.model.BucketLevelTrigger
import org.opensearch.commons.alerting.model.DocLevelMonitorInput
import org.opensearch.commons.alerting.model.DocumentLevelTrigger
import org.opensearch.commons.alerting.model.Monitor
import org.opensearch.commons.alerting.model.QueryLevelTrigger
Expand Down Expand Up @@ -46,6 +47,11 @@ private val log = LogManager.getLogger(RestIndexMonitorAction::class.java)
*/
class RestIndexMonitorAction : BaseRestHandler() {

// allowed characters [- : , ( ) [ ] ' _]
private val allowedChars = "-:,\\(\\)\\[\\]\'_"
// regex to restrict string to alphanumeric and allowed chars, must be between 0 - 256 characters
val regex = "[\\w\\s$allowedChars]{0,256}"

override fun getName(): String {
return "index_monitor_action"
}
Expand Down Expand Up @@ -116,9 +122,11 @@ class RestIndexMonitorAction : BaseRestHandler() {
if (it !is DocumentLevelTrigger) {
throw IllegalArgumentException("Illegal trigger type, ${it.javaClass.name}, for document level monitor")
}
validateDocLevelQueryName(monitor)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like this is iterating through each trigger configured for the monitor, and then validating all of the query names during each iteration. Since queries are at the monitor level, not the trigger level, would it make sense to move this validation check out of the forEach loop?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes agreed, moved the validation outside the forEach loop

}
}
}

val seqNo = request.paramAsLong(IF_SEQ_NO, SequenceNumbers.UNASSIGNED_SEQ_NO)
val primaryTerm = request.paramAsLong(IF_PRIMARY_TERM, SequenceNumbers.UNASSIGNED_PRIMARY_TERM)
val refreshPolicy = if (request.hasParam(REFRESH)) {
Expand All @@ -133,6 +141,19 @@ class RestIndexMonitorAction : BaseRestHandler() {
}
}

private fun validateDocLevelQueryName(monitor: Monitor) {
monitor.inputs.filterIsInstance<DocLevelMonitorInput>().forEach { docLevelMonitorInput ->
docLevelMonitorInput.queries.forEach { dlq ->
if (!dlq.name.matches(Regex(regex))) {
throw IllegalArgumentException(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to confirm, does this end up throwing an IllegalArgumentException, or does it get wrapped in an AlertingException somewhere else? I believe IllegalArgumentException results in a 500 error, which we want to avoid.

Copy link
Collaborator

@AWSHurneyt AWSHurneyt Apr 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Never mind, I see in your integ test that the expected error status is RestStatus.BAD_REQUEST, which is a 400 error; so the IllegalArgumentException must be getting caught, and wrapped somewhere else.

"Doc level query name, ${dlq.name}, may only contain alphanumeric values and " +
Copy link
Collaborator

@AWSHurneyt AWSHurneyt Apr 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We shouldn't return user input in error messages; so could we reword this error message to leave out ${dlq.name}?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reworded the error message to remove dlq.name

"these special characters: ${allowedChars.replace("\\","")}"
)
}
}
}
}

private fun validateDataSources(monitor: Monitor) { // Data Sources will currently be supported only at transport layer.
if (monitor.dataSources != null) {
if (
Expand Down
44 changes: 44 additions & 0 deletions alerting/src/test/kotlin/org/opensearch/alerting/resthandler/MonitorRestApiIT.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ import org.opensearch.alerting.randomAnomalyDetector
import org.opensearch.alerting.randomAnomalyDetectorWithUser
import org.opensearch.alerting.randomBucketLevelMonitor
import org.opensearch.alerting.randomBucketLevelTrigger
import org.opensearch.alerting.randomDocLevelMonitorInput
import org.opensearch.alerting.randomDocLevelQuery
import org.opensearch.alerting.randomDocumentLevelMonitor
import org.opensearch.alerting.randomDocumentLevelTrigger
import org.opensearch.alerting.randomQueryLevelMonitor
Expand Down Expand Up @@ -1285,6 +1287,48 @@ class MonitorRestApiIT : AlertingRestTestCase() {
}
}

fun `test creating and updating a document monitor with invalid query name`() {
// creating a monitor with an invalid query name
val invalidQueryName = "Invalid @ query ! name"
val queries = listOf(randomDocLevelQuery(name = invalidQueryName))
val randomDocLevelMonitorInput = randomDocLevelMonitorInput(queries = queries)
val inputs = listOf(randomDocLevelMonitorInput)
val trigger = randomDocumentLevelTrigger()
var monitor = randomDocumentLevelMonitor(inputs = inputs, triggers = listOf(trigger))

try {
client().makeRequest("POST", ALERTING_BASE_URI, emptyMap(), monitor.toHttpEntity())
fail("Doc level monitor with invalid query name should be rejected")
} catch (e: ResponseException) {
assertEquals("Unexpected status", RestStatus.BAD_REQUEST, e.response.restStatus())
val expectedMessage = "Doc level query name, $invalidQueryName, may only contain alphanumeric values"
e.message?.let { assertTrue(it.contains(expectedMessage)) }
}

// successfully creating monitor with valid query name
val testIndex = createTestIndex()
val docQuery = DocLevelQuery(query = "test_field:\"us-west-2\"", name = "valid name", fields = listOf())
val docLevelInput = DocLevelMonitorInput("description", listOf(testIndex), listOf(docQuery))

monitor = createMonitor(randomDocumentLevelMonitor(inputs = listOf(docLevelInput), triggers = listOf(trigger)))

// updating monitor with invalid query name
val updatedDocQuery = DocLevelQuery(query = "test_field:\"us-west-2\"", name = invalidQueryName, fields = listOf())
val updatedDocLevelInput = DocLevelMonitorInput("description", listOf(testIndex), listOf(updatedDocQuery))

try {
client().makeRequest(
"PUT", monitor.relativeUrl(),
emptyMap(), monitor.copy(inputs = listOf(updatedDocLevelInput)).toHttpEntity()
)
fail("Doc level monitor with invalid query name should be rejected")
} catch (e: ResponseException) {
assertEquals("Unexpected status", RestStatus.BAD_REQUEST, e.response.restStatus())
val expectedMessage = "Doc level query name, $invalidQueryName, may only contain alphanumeric values"
e.message?.let { assertTrue(it.contains(expectedMessage)) }
}
}

/**
* This use case is needed by the frontend plugin for displaying alert counts on the Monitors list page.
* https://github.com/opensearch-project/alerting-dashboards-plugin/blob/main/server/services/MonitorService.js#L235
Expand Down
Loading