fix: silently ignore empty proxy host values (#1099)

smithy-lang · Jun 5, 2024 · 95d9b34 · 95d9b34
1 parent 6099a1c
commit 95d9b34
Show file tree

Hide file tree

Showing 4 changed files with 25 additions and 5 deletions.
diff --git a/.changes/7c1e4ec8-d975-498d-affa-ce470d268785.json b/.changes/7c1e4ec8-d975-498d-affa-ce470d268785.json
@@ -0,0 +1,8 @@
+{
+    "id": "7c1e4ec8-d975-498d-affa-ce470d268785",
+    "type": "bugfix",
+    "description": "Silently ignore empty/blank proxy host values from environment variables or system properties instead of throwing exceptions",
+    "issues": [
+        "smithy-lang/smithy-kotlin#1098"
+    ]
+}
diff --git a/...-slf4j2/jvm/src/aws/smithy/kotlin/runtime/telemetry/logging/slf4j/Slf4j1xLoggerAdapter.kt b/...-slf4j2/jvm/src/aws/smithy/kotlin/runtime/telemetry/logging/slf4j/Slf4j1xLoggerAdapter.kt
@@ -57,14 +57,18 @@ private class Slf4j1xLogRecordBuilderAdapter(
         }
 
         if (kvp.isNotEmpty()) {
-            val prevCtx = MDC.getCopyOfContextMap()
+            val prevCtx: Map<String, String>? = MDC.getCopyOfContextMap()
             try {
                 kvp.forEach { (k, v) ->
                     MDC.put(k, v.toString())
                 }
                 logMethod(cause, message)
             } finally {
-                MDC.setContextMap(prevCtx)
+                if (prevCtx == null) {
+                    MDC.clear()
+                } else {
+                    MDC.setContextMap(prevCtx)
+                }
             }
         } else {
             logMethod(cause, message)

diff --git a/.../http-client/common/src/aws/smithy/kotlin/runtime/http/engine/EnvironmentProxySelector.kt b/.../http-client/common/src/aws/smithy/kotlin/runtime/http/engine/EnvironmentProxySelector.kt
@@ -54,8 +54,8 @@ private fun resolveProxyByProperty(provider: PropertyProvider, scheme: Scheme):
     val hostPropName = "${scheme.protocolName}.proxyHost"
     val hostPortPropName = "${scheme.protocolName}.proxyPort"
 
-    val proxyHostProp = provider.getProperty(hostPropName)
-    val proxyPortProp = provider.getProperty(hostPortPropName)
+    val proxyHostProp = provider.getProperty(hostPropName).takeUnless { it.isNullOrBlank() }
+    val proxyPortProp = provider.getProperty(hostPortPropName).takeUnless { it.isNullOrBlank() }
 
     return proxyHostProp?.let { hostName ->
         // we don't support connecting to the proxy over TLS, we expect engines would support
@@ -84,7 +84,7 @@ private fun resolveProxyByEnvironment(provider: EnvironmentProvider, scheme: Sch
     // lowercase takes precedence: https://about.gitlab.com/blog/2021/01/27/we-need-to-talk-no-proxy/
     listOf("${scheme.protocolName.lowercase()}_proxy", "${scheme.protocolName.uppercase()}_PROXY")
         .firstNotNullOfOrNull { envVar ->
-            provider.getenv(envVar)?.let { proxyUrlString ->
+            provider.getenv(envVar).takeUnless { it.isNullOrBlank() }?.let { proxyUrlString ->
                 val url = try {
                     Url.parse(proxyUrlString)
                 } catch (e: Exception) {

diff --git a/...-client/common/test/aws/smithy/kotlin/runtime/http/engine/EnvironmentProxySelectorTest.kt b/...-client/common/test/aws/smithy/kotlin/runtime/http/engine/EnvironmentProxySelectorTest.kt
@@ -49,6 +49,14 @@ class EnvironmentProxySelectorTest {
     private val tests = listOf(
         // no props
         TestCase(ProxyConfig.Direct),
+        TestCase(ProxyConfig.Direct, "http://aws.amazon.com", env = mapOf("http_proxy" to "")),
+        TestCase(ProxyConfig.Direct, "http://aws.amazon.com", env = mapOf("http_proxy" to " ")),
+        TestCase(ProxyConfig.Direct, "http://aws.amazon.com", props = mapOf("http.proxyHost" to "")),
+        TestCase(ProxyConfig.Direct, "http://aws.amazon.com", props = mapOf("http.proxyHost" to " ")),
+        TestCase(ProxyConfig.Direct, env = mapOf("https_proxy" to "")),
+        TestCase(ProxyConfig.Direct, env = mapOf("https_proxy" to " ")),
+        TestCase(ProxyConfig.Direct, props = mapOf("https.proxyHost" to "")),
+        TestCase(ProxyConfig.Direct, props = mapOf("https.proxyHost" to " ")),
 
         // no proxy
         TestCase(ProxyConfig.Direct, env = mapOf("no_proxy" to "aws.amazon.com") + httpsProxyEnv),