Resolve hosts when checking against host deny list (#496) (#499)

* Resolve hosts when checking against host deny list Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> * Stub isHostInDenyList() for notification core unit tests Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> * Stub the correct isHostInDenylist function Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> * Use Before annotation instead for mocking setup Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> * Test switching one of the ChimeDestinationTests to use mockk instead of EasyMock Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> * Switch to BeforeEach for setup and remove unneeded unit test Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com> * Change CustomWebhookDestinationTest and SlackDestinationTests to use BeforeEach as well for consistency Signed-off-by: Mohammad Qureshi <47198598+qreshi@users.noreply.github.com>
opensearch-project · Aug 5, 2022 · 1d598f9 · 1d598f9
1 parent e238e09
commit 1d598f9
Show file tree

Hide file tree

Showing 6 changed files with 65 additions and 20 deletions.
diff --git a/notifications/core-spi/build.gradle b/notifications/core-spi/build.gradle
@@ -53,12 +53,21 @@ dependencies {
     implementation "com.github.seancfoley:ipaddress:5.3.3"
 
     testImplementation(
-            'org.junit.jupiter:junit-jupiter-api:5.6.2',
+            "io.mockk:mockk:1.11.0",
+            "io.mockk:mockk-common:1.11.0",
+            "io.mockk:mockk-dsl:1.11.0",
+            "io.mockk:mockk-dsl-jvm:1.11.0",
+            "io.mockk:mockk-agent-api:1.11.0",
+            "io.mockk:mockk-agent-common:1.11.0",
+            "io.mockk:mockk-agent-jvm:1.11.0",
+            "org.junit.jupiter:junit-jupiter-api:5.6.2",
             "org.junit.jupiter:junit-jupiter-params:5.6.2",
-            'org.mockito:mockito-junit-jupiter:3.10.0',
+            "org.mockito:mockito-junit-jupiter:3.10.0",
     )
     testRuntimeOnly('org.junit.jupiter:junit-jupiter-engine:5.6.2')
     testImplementation "org.jetbrains.kotlin:kotlin-test:${kotlin_version}"
+    testImplementation "org.jetbrains.kotlin:kotlin-reflect:${kotlin_version}" // required by mockk
+    testImplementation "net.bytebuddy:byte-buddy-agent:1.12.7"
     testImplementation "org.opensearch.test:framework:${opensearch_version}"
 }
 

diff --git a/...ions/core-spi/src/main/kotlin/org/opensearch/notifications/spi/utils/ValidationHelpers.kt b/...ions/core-spi/src/main/kotlin/org/opensearch/notifications/spi/utils/ValidationHelpers.kt
@@ -10,6 +10,7 @@ import org.apache.http.client.methods.HttpPatch
 import org.apache.http.client.methods.HttpPost
 import org.apache.http.client.methods.HttpPut
 import org.opensearch.common.Strings
+import java.net.InetAddress
 import java.net.URL
 
 fun validateUrl(urlString: String) {
@@ -30,7 +31,7 @@ fun isValidUrl(urlString: String): Boolean {
 fun isHostInDenylist(urlString: String, hostDenyList: List<String>): Boolean {
     val url = URL(urlString)
     if (url.host != null) {
-        val ipStr = IPAddressString(url.host)
+        val ipStr = IPAddressString(InetAddress.getByName(url.host).hostAddress)
         for (network in hostDenyList) {
             val netStr = IPAddressString(network)
             if (netStr.contains(ipStr)) {

diff --git a/...core-spi/src/test/kotlin/org/opensearch/notifications/spi/utils/ValidationHelpersTests.kt b/...core-spi/src/test/kotlin/org/opensearch/notifications/spi/utils/ValidationHelpersTests.kt
@@ -5,12 +5,15 @@
 
 package org.opensearch.notifications.spi.utils
 
+import io.mockk.every
+import io.mockk.mockkStatic
 import org.junit.jupiter.api.Assertions.assertEquals
 import org.junit.jupiter.api.Test
+import java.net.InetAddress
 
 internal class ValidationHelpersTests {
 
-    private val hostDentyList = listOf(
+    private val hostDenyList = listOf(
         "127.0.0.0/8",
         "10.0.0.0/8",
         "172.16.0.0/12",
@@ -31,15 +34,19 @@ internal class ValidationHelpersTests {
             "9.9.9.9"
         )
         for (ip in ips) {
-            assertEquals(true, isHostInDenylist("https://$ip", hostDentyList))
+            assertEquals(true, isHostInDenylist("https://$ip", hostDenyList))
         }
     }
 
     @Test
-    fun `test url in denylist`() {
-        val urls = listOf("https://www.amazon.com", "https://mytest.com", "https://mytest.com")
-        for (url in urls) {
-            assertEquals(false, isHostInDenylist(url, hostDentyList))
-        }
+    fun `test hostname gets resolved to ip for denylist`() {
+        val invalidHost = "invalid.com"
+        mockkStatic(InetAddress::class)
+        every { InetAddress.getByName(invalidHost).hostAddress } returns "10.0.0.1" // 10.0.0.0/8
+        assertEquals(true, isHostInDenylist("https://$invalidHost", hostDenyList))
+
+        val validHost = "valid.com"
+        every { InetAddress.getByName(validHost).hostAddress } returns "174.12.0.0"
+        assertEquals(false, isHostInDenylist("https://$validHost", hostDenyList))
     }
 }
diff --git a/...e/src/test/kotlin/org/opensearch/notifications/core/destinations/ChimeDestinationTests.kt b/...e/src/test/kotlin/org/opensearch/notifications/core/destinations/ChimeDestinationTests.kt
@@ -5,6 +5,9 @@
 
 package org.opensearch.notifications.core.destinations
 
+import io.mockk.every
+import io.mockk.mockk
+import io.mockk.mockkStatic
 import org.apache.http.client.methods.CloseableHttpResponse
 import org.apache.http.client.methods.HttpPost
 import org.apache.http.entity.StringEntity
@@ -13,6 +16,7 @@ import org.apache.http.message.BasicStatusLine
 import org.easymock.EasyMock
 import org.junit.jupiter.api.Assertions
 import org.junit.jupiter.api.Assertions.assertEquals
+import org.junit.jupiter.api.BeforeEach
 import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.assertThrows
 import org.junit.jupiter.params.ParameterizedTest
@@ -43,23 +47,27 @@ internal class ChimeDestinationTests {
             )
     }
 
+    @BeforeEach
+    fun setup() {
+        // Stubbing isHostInDenylist() so it doesn't attempt to resolve hosts that don't exist in the unit tests
+        mockkStatic("org.opensearch.notifications.spi.utils.ValidationHelpersKt")
+        every { org.opensearch.notifications.spi.utils.isHostInDenylist(any(), any()) } returns false
+    }
+
     @Test
     fun `test chime message null entity response`() {
-        val mockHttpClient: CloseableHttpClient = EasyMock.createMock(CloseableHttpClient::class.java)
+        val mockHttpClient = mockk<CloseableHttpClient>()
 
         // The DestinationHttpClient replaces a null entity with "{}".
         val expectedWebhookResponse = DestinationMessageResponse(RestStatus.OK.status, "{}")
         // TODO replace EasyMock in all UTs with mockk which fits Kotlin better
-        val httpResponse: CloseableHttpResponse = EasyMock.createMock(CloseableHttpResponse::class.java)
-        EasyMock.expect(mockHttpClient.execute(EasyMock.anyObject(HttpPost::class.java))).andReturn(httpResponse)
+        val httpResponse = mockk<CloseableHttpResponse>()
+        every { mockHttpClient.execute(any<HttpPost>()) } returns httpResponse
 
-        val mockStatusLine: BasicStatusLine = EasyMock.createMock(BasicStatusLine::class.java)
-        EasyMock.expect(httpResponse.statusLine).andReturn(mockStatusLine)
-        EasyMock.expect(httpResponse.entity).andReturn(null).anyTimes()
-        EasyMock.expect(mockStatusLine.statusCode).andReturn(RestStatus.OK.status)
-        EasyMock.replay(mockHttpClient)
-        EasyMock.replay(httpResponse)
-        EasyMock.replay(mockStatusLine)
+        val mockStatusLine = mockk<BasicStatusLine>()
+        every { httpResponse.statusLine } returns mockStatusLine
+        every { httpResponse.entity } returns null
+        every { mockStatusLine.statusCode } returns RestStatus.OK.status
 
         val httpClient = DestinationHttpClient(mockHttpClient)
         val webhookDestinationTransport = WebhookDestinationTransport(httpClient)

diff --git a/...st/kotlin/org/opensearch/notifications/core/destinations/CustomWebhookDestinationTests.kt b/...st/kotlin/org/opensearch/notifications/core/destinations/CustomWebhookDestinationTests.kt
@@ -5,6 +5,8 @@
 
 package org.opensearch.notifications.core.destinations
 
+import io.mockk.every
+import io.mockk.mockkStatic
 import org.apache.http.client.methods.CloseableHttpResponse
 import org.apache.http.client.methods.HttpPatch
 import org.apache.http.client.methods.HttpPost
@@ -16,6 +18,7 @@ import org.apache.http.message.BasicStatusLine
 import org.easymock.EasyMock
 import org.junit.jupiter.api.Assertions
 import org.junit.jupiter.api.Assertions.assertEquals
+import org.junit.jupiter.api.BeforeEach
 import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.assertThrows
 import org.junit.jupiter.params.ParameterizedTest
@@ -53,6 +56,13 @@ internal class CustomWebhookDestinationTests {
             )
     }
 
+    @BeforeEach
+    fun setup() {
+        // Stubbing isHostInDenylist() so it doesn't attempt to resolve hosts that don't exist in the unit tests
+        mockkStatic("org.opensearch.notifications.spi.utils.ValidationHelpersKt")
+        every { org.opensearch.notifications.spi.utils.isHostInDenylist(any(), any()) } returns false
+    }
+
     @ParameterizedTest(name = "method {0} should return corresponding type of Http request object {1}")
     @MethodSource("methodToHttpRequestType")
     fun `test custom webhook message null entity response`(method: String, expectedHttpClass: Class<HttpUriRequest>) {

diff --git a/...e/src/test/kotlin/org/opensearch/notifications/core/destinations/SlackDestinationTests.kt b/...e/src/test/kotlin/org/opensearch/notifications/core/destinations/SlackDestinationTests.kt
@@ -5,6 +5,8 @@
 
 package org.opensearch.notifications.core.destinations
 
+import io.mockk.every
+import io.mockk.mockkStatic
 import org.apache.http.client.methods.CloseableHttpResponse
 import org.apache.http.client.methods.HttpPost
 import org.apache.http.entity.StringEntity
@@ -13,6 +15,7 @@ import org.apache.http.message.BasicStatusLine
 import org.easymock.EasyMock
 import org.junit.jupiter.api.Assertions
 import org.junit.jupiter.api.Assertions.assertEquals
+import org.junit.jupiter.api.BeforeEach
 import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.assertThrows
 import org.junit.jupiter.params.ParameterizedTest
@@ -44,6 +47,13 @@ internal class SlackDestinationTests {
             )
     }
 
+    @BeforeEach
+    fun setup() {
+        // Stubbing isHostInDenylist() so it doesn't attempt to resolve hosts that don't exist in the unit tests
+        mockkStatic("org.opensearch.notifications.spi.utils.ValidationHelpersKt")
+        every { org.opensearch.notifications.spi.utils.isHostInDenylist(any(), any()) } returns false
+    }
+
     @Test
     fun `test Slack message null entity response`() {
         val mockHttpClient: CloseableHttpClient = EasyMock.createMock(CloseableHttpClient::class.java)