From 135bae57a5114e3837a5030f84d5f95a0a55c166 Mon Sep 17 00:00:00 2001 From: Joanne Wang Date: Fri, 5 Apr 2024 11:52:57 -0700 Subject: [PATCH 1/3] add validation check for doc level query name during monitor creation Signed-off-by: Joanne Wang --- .../resthandler/RestIndexMonitorAction.kt | 21 +++++++++ .../alerting/resthandler/MonitorRestApiIT.kt | 44 +++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/alerting/src/main/kotlin/org/opensearch/alerting/resthandler/RestIndexMonitorAction.kt b/alerting/src/main/kotlin/org/opensearch/alerting/resthandler/RestIndexMonitorAction.kt index 5e4a8c155..958445f30 100644 --- a/alerting/src/main/kotlin/org/opensearch/alerting/resthandler/RestIndexMonitorAction.kt +++ b/alerting/src/main/kotlin/org/opensearch/alerting/resthandler/RestIndexMonitorAction.kt @@ -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 @@ -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 1 - 256 characters + val regex = "[\\w\\s$allowedChars]{1,256}" + override fun getName(): String { return "index_monitor_action" } @@ -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) } } } + 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)) { @@ -133,6 +141,19 @@ class RestIndexMonitorAction : BaseRestHandler() { } } + private fun validateDocLevelQueryName(monitor: Monitor) { + monitor.inputs.filterIsInstance().forEach { docLevelMonitorInput -> + docLevelMonitorInput.queries.forEach { dlq -> + if (!dlq.name.matches(Regex(regex))) { + throw IllegalArgumentException( + "Doc level query name, ${dlq.name}, may only contain alphanumeric values and " + + "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 ( diff --git a/alerting/src/test/kotlin/org/opensearch/alerting/resthandler/MonitorRestApiIT.kt b/alerting/src/test/kotlin/org/opensearch/alerting/resthandler/MonitorRestApiIT.kt index 25e8d319d..a298b4b3f 100644 --- a/alerting/src/test/kotlin/org/opensearch/alerting/resthandler/MonitorRestApiIT.kt +++ b/alerting/src/test/kotlin/org/opensearch/alerting/resthandler/MonitorRestApiIT.kt @@ -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 @@ -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 From cd50847aead2639e4ff375d371b810ffe1d27ab4 Mon Sep 17 00:00:00 2001 From: Joanne Wang Date: Fri, 5 Apr 2024 12:01:32 -0700 Subject: [PATCH 2/3] change to 0-256 chars Signed-off-by: Joanne Wang --- .../opensearch/alerting/resthandler/RestIndexMonitorAction.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/alerting/src/main/kotlin/org/opensearch/alerting/resthandler/RestIndexMonitorAction.kt b/alerting/src/main/kotlin/org/opensearch/alerting/resthandler/RestIndexMonitorAction.kt index 958445f30..edc834a40 100644 --- a/alerting/src/main/kotlin/org/opensearch/alerting/resthandler/RestIndexMonitorAction.kt +++ b/alerting/src/main/kotlin/org/opensearch/alerting/resthandler/RestIndexMonitorAction.kt @@ -49,8 +49,8 @@ class RestIndexMonitorAction : BaseRestHandler() { // allowed characters [- : , ( ) [ ] ' _] private val allowedChars = "-:,\\(\\)\\[\\]\'_" - // regex to restrict string to alphanumeric and allowed chars, must be between 1 - 256 characters - val regex = "[\\w\\s$allowedChars]{1,256}" + // 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" From b490cf7a299c461963aa1008bee67b4a331b4d1b Mon Sep 17 00:00:00 2001 From: Joanne Wang Date: Mon, 8 Apr 2024 15:23:38 -0700 Subject: [PATCH 3/3] change validation message and move validation loc Signed-off-by: Joanne Wang --- .../resthandler/RestIndexMonitorAction.kt | 15 ++++++--------- .../alerting/resthandler/MonitorRestApiIT.kt | 11 +++++++---- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/alerting/src/main/kotlin/org/opensearch/alerting/resthandler/RestIndexMonitorAction.kt b/alerting/src/main/kotlin/org/opensearch/alerting/resthandler/RestIndexMonitorAction.kt index edc834a40..03b5e09be 100644 --- a/alerting/src/main/kotlin/org/opensearch/alerting/resthandler/RestIndexMonitorAction.kt +++ b/alerting/src/main/kotlin/org/opensearch/alerting/resthandler/RestIndexMonitorAction.kt @@ -21,6 +21,8 @@ import org.opensearch.commons.alerting.model.DocumentLevelTrigger import org.opensearch.commons.alerting.model.Monitor import org.opensearch.commons.alerting.model.QueryLevelTrigger import org.opensearch.commons.alerting.model.ScheduledJob +import org.opensearch.commons.utils.getInvalidNameChars +import org.opensearch.commons.utils.isValidName import org.opensearch.core.rest.RestStatus import org.opensearch.core.xcontent.ToXContent import org.opensearch.core.xcontent.XContentParser.Token @@ -47,11 +49,6 @@ 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" } @@ -122,8 +119,8 @@ class RestIndexMonitorAction : BaseRestHandler() { if (it !is DocumentLevelTrigger) { throw IllegalArgumentException("Illegal trigger type, ${it.javaClass.name}, for document level monitor") } - validateDocLevelQueryName(monitor) } + validateDocLevelQueryName(monitor) } } @@ -144,10 +141,10 @@ class RestIndexMonitorAction : BaseRestHandler() { private fun validateDocLevelQueryName(monitor: Monitor) { monitor.inputs.filterIsInstance().forEach { docLevelMonitorInput -> docLevelMonitorInput.queries.forEach { dlq -> - if (!dlq.name.matches(Regex(regex))) { + if (!isValidName(dlq.name)) { throw IllegalArgumentException( - "Doc level query name, ${dlq.name}, may only contain alphanumeric values and " + - "these special characters: ${allowedChars.replace("\\","")}" + "Doc level query name may not start with [_, +, -], contain '..', or contain: " + + getInvalidNameChars().replace("\\", "") ) } } diff --git a/alerting/src/test/kotlin/org/opensearch/alerting/resthandler/MonitorRestApiIT.kt b/alerting/src/test/kotlin/org/opensearch/alerting/resthandler/MonitorRestApiIT.kt index a298b4b3f..79c871a97 100644 --- a/alerting/src/test/kotlin/org/opensearch/alerting/resthandler/MonitorRestApiIT.kt +++ b/alerting/src/test/kotlin/org/opensearch/alerting/resthandler/MonitorRestApiIT.kt @@ -50,6 +50,7 @@ import org.opensearch.commons.alerting.model.Monitor import org.opensearch.commons.alerting.model.QueryLevelTrigger import org.opensearch.commons.alerting.model.ScheduledJob import org.opensearch.commons.alerting.model.SearchInput +import org.opensearch.commons.utils.getInvalidNameChars import org.opensearch.core.common.bytes.BytesReference import org.opensearch.core.rest.RestStatus import org.opensearch.core.xcontent.ToXContent @@ -1289,7 +1290,7 @@ 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 invalidQueryName = "_Invalid .. query ! n>ame" val queries = listOf(randomDocLevelQuery(name = invalidQueryName)) val randomDocLevelMonitorInput = randomDocLevelMonitorInput(queries = queries) val inputs = listOf(randomDocLevelMonitorInput) @@ -1301,13 +1302,14 @@ class MonitorRestApiIT : AlertingRestTestCase() { 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" + val expectedMessage = "Doc level query name may not start with [_, +, -], contain '..', or contain: " + + getInvalidNameChars().replace("\\", "") 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 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))) @@ -1324,7 +1326,8 @@ class MonitorRestApiIT : AlertingRestTestCase() { 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" + val expectedMessage = "Doc level query name may not start with [_, +, -], contain '..', or contain: " + + getInvalidNameChars().replace("\\", "") e.message?.let { assertTrue(it.contains(expectedMessage)) } } }