diff --git a/android/src/main/java/com/mattermost/networkclient/Extensions.kt b/android/src/main/java/com/mattermost/networkclient/Extensions.kt index 21ac054f2..7fa1c7ce3 100644 --- a/android/src/main/java/com/mattermost/networkclient/Extensions.kt +++ b/android/src/main/java/com/mattermost/networkclient/Extensions.kt @@ -1,5 +1,6 @@ package com.mattermost.networkclient +import android.util.Log import com.facebook.react.bridge.Arguments import com.facebook.react.bridge.ReadableMap import com.facebook.react.bridge.WritableArray @@ -134,8 +135,17 @@ fun Response.toDownloadMap(path: String): WritableMap { fun Request.Builder.applyHeaders(headers: Map?): Request.Builder { if (headers != null){ for ((k, v) in headers) { - this.removeHeader(k) - this.addHeader(k, v.toString()) + try { + this.removeHeader(k) + this.addHeader(k, v.toString()) + } catch (e: IllegalArgumentException) { + // OkHttp validates header values and rejects non-ASCII characters + // (e.g. control chars like 0x02 in corrupted auth tokens, or + // Arabic-Indic digits from locale-dependent formatting). + // Skip the invalid header and let the request proceed — the server + // will reject it with a 4xx that the JS layer can handle gracefully. + Log.w("NetworkClient", "Skipping header '$k': ${e.message}") + } } } diff --git a/android/src/main/java/com/mattermost/networkclient/interceptors/CompressedResponseSizeInterceptor.kt b/android/src/main/java/com/mattermost/networkclient/interceptors/CompressedResponseSizeInterceptor.kt index 51f03220a..de4948526 100644 --- a/android/src/main/java/com/mattermost/networkclient/interceptors/CompressedResponseSizeInterceptor.kt +++ b/android/src/main/java/com/mattermost/networkclient/interceptors/CompressedResponseSizeInterceptor.kt @@ -3,6 +3,7 @@ package com.mattermost.networkclient.interceptors import okhttp3.Interceptor import okhttp3.Response import okhttp3.ResponseBody.Companion.toResponseBody +import java.util.Locale class CompressedResponseSizeInterceptor: Interceptor { override fun intercept(chain: Interceptor.Chain): Response { @@ -37,7 +38,9 @@ class CompressedResponseSizeInterceptor: Interceptor { .header("X-Compressed-Size", compressedSize.toString()) .header("X-Start-Time", startTime.toString()) .header("X-End-Time", endTime.toString()) - .header("X-Speed-Mbps", "%.4f".format(speedMbps)) // Format to 4 decimal places + // Use Locale.US to ensure ASCII digits in header values. + // System locale formatting (e.g. Arabic) produces non-ASCII digits that OkHttp rejects. + .header("X-Speed-Mbps", String.format(Locale.US, "%.4f", speedMbps)) .build() } } diff --git a/test-runner/src/test/kotlin/com/mattermost/networkclient/ApplyHeadersTest.kt b/test-runner/src/test/kotlin/com/mattermost/networkclient/ApplyHeadersTest.kt new file mode 100644 index 000000000..c6ea53a9b --- /dev/null +++ b/test-runner/src/test/kotlin/com/mattermost/networkclient/ApplyHeadersTest.kt @@ -0,0 +1,177 @@ +package com.mattermost.networkclient + +import okhttp3.OkHttpClient +import okhttp3.Request +import okhttp3.mockwebserver.MockResponse +import okhttp3.mockwebserver.MockWebServer +import okhttp3.Interceptor +import okhttp3.Response +import org.junit.Assert +import org.junit.Test + +/** + * Tests for applyHeaders resilience against invalid header values. + * + * Reproduces MATTERMOST-MOBILE-ANDROID-AYPW: corrupted tokens containing + * non-ASCII control characters (e.g. 0x02) cause OkHttp to throw + * IllegalArgumentException in addHeader() during request construction. + * The fix catches this in applyHeaders and skips the invalid header. + */ +class ApplyHeadersTest { + + /** + * Interceptor that applies headers using OkHttp's addHeader WITHOUT + * any error handling (broken behavior — reproduces the crash). + */ + class BrokenHeaderInterceptor(private val headers: Map) : Interceptor { + override fun intercept(chain: Interceptor.Chain): Response { + val builder = chain.request().newBuilder() + for ((k, v) in headers) { + builder.removeHeader(k) + builder.addHeader(k, v) // Throws on non-ASCII + } + return chain.proceed(builder.build()) + } + } + + /** + * Interceptor that applies headers with the same catch-and-skip pattern + * used in the fixed applyHeaders extension function. + */ + class FixedHeaderInterceptor(private val headers: Map) : Interceptor { + override fun intercept(chain: Interceptor.Chain): Response { + val builder = chain.request().newBuilder() + for ((k, v) in headers) { + try { + builder.removeHeader(k) + builder.addHeader(k, v) + } catch (e: IllegalArgumentException) { + // Skip invalid header, same as the production fix + } + } + return chain.proceed(builder.build()) + } + } + + @Test + fun brokenHeaders_crashWithControlCharInToken() { + val headers = mapOf("Authorization" to "Bearer abc\u0002def") + + MockWebServer().use { server -> + server.enqueue(MockResponse().setBody("ok")) + server.start() + + val client = OkHttpClient.Builder() + .addInterceptor(BrokenHeaderInterceptor(headers)) + .build() + + val request = Request.Builder() + .url(server.url("/test")) + .build() + + try { + client.newCall(request).execute().use { /* auto-close */ } + Assert.fail("Expected IllegalArgumentException from OkHttp header validation") + } catch (e: IllegalArgumentException) { + Assert.assertTrue( + "Expected error about unexpected char", + e.message?.contains("Unexpected char") == true + ) + } + } + } + + @Test + fun fixedHeaders_skipInvalidAndProceed() { + val headers = mapOf( + "Authorization" to "Bearer abc\u0002def", + "X-Custom" to "valid-value" + ) + + MockWebServer().use { server -> + server.enqueue(MockResponse().setBody("ok")) + server.start() + + val client = OkHttpClient.Builder() + .addInterceptor(FixedHeaderInterceptor(headers)) + .build() + + val request = Request.Builder() + .url(server.url("/test")) + .build() + + client.newCall(request).execute().use { response -> + Assert.assertEquals(200, response.code) + } + + // Invalid Authorization header should be skipped, valid header kept + val recorded = server.takeRequest() + Assert.assertNull( + "Invalid Authorization header should be skipped", + recorded.getHeader("Authorization") + ) + Assert.assertEquals("valid-value", recorded.getHeader("X-Custom")) + } + } + + @Test + fun fixedHeaders_cleanHeadersPassThrough() { + val headers = mapOf( + "Authorization" to "Bearer eyJhbGciOiJIUzI1NiJ9.valid.token", + "Content-Type" to "application/json" + ) + + MockWebServer().use { server -> + server.enqueue(MockResponse().setBody("ok")) + server.start() + + val client = OkHttpClient.Builder() + .addInterceptor(FixedHeaderInterceptor(headers)) + .build() + + val request = Request.Builder() + .url(server.url("/test")) + .build() + + client.newCall(request).execute().use { response -> + Assert.assertEquals(200, response.code) + } + + val recorded = server.takeRequest() + Assert.assertEquals( + "Bearer eyJhbGciOiJIUzI1NiJ9.valid.token", + recorded.getHeader("Authorization") + ) + Assert.assertEquals("application/json", recorded.getHeader("Content-Type")) + } + } + + @Test + fun fixedHeaders_allInvalidHeadersSkipped() { + val headers = mapOf( + "X-Bad1" to "value\u0001here", + "X-Bad2" to "\u0003\u0004\u0005" + ) + + MockWebServer().use { server -> + server.enqueue(MockResponse().setBody("ok")) + server.start() + + val client = OkHttpClient.Builder() + .addInterceptor(FixedHeaderInterceptor(headers)) + .build() + + val request = Request.Builder() + .url(server.url("/test")) + .build() + + client.newCall(request).execute().use { response -> + Assert.assertEquals(200, response.code) + } + + val recorded = server.takeRequest() + Assert.assertNull(recorded.getHeader("X-Bad1")) + Assert.assertNull(recorded.getHeader("X-Bad2")) + } + } +} diff --git a/test-runner/src/test/kotlin/com/mattermost/networkclient/CompressedResponseSizeInterceptorTest.kt b/test-runner/src/test/kotlin/com/mattermost/networkclient/CompressedResponseSizeInterceptorTest.kt new file mode 100644 index 000000000..6b00a7682 --- /dev/null +++ b/test-runner/src/test/kotlin/com/mattermost/networkclient/CompressedResponseSizeInterceptorTest.kt @@ -0,0 +1,176 @@ +package com.mattermost.networkclient + +import okhttp3.OkHttpClient +import okhttp3.Request +import okhttp3.mockwebserver.MockResponse +import okhttp3.mockwebserver.MockWebServer +import okhttp3.Interceptor +import okhttp3.Response +import okhttp3.ResponseBody.Companion.toResponseBody +import org.junit.Assert +import org.junit.Test +import java.util.Locale + +/** + * Standalone test for CompressedResponseSizeInterceptor locale behavior. + * + * Reproduces MATTERMOST-MOBILE-ANDROID-AZ02: on Arabic-locale devices, + * String.format() produces Arabic-Indic digits (U+0660 range) in the + * X-Speed-Mbps header, which OkHttp rejects as invalid header characters. + */ +class CompressedResponseSizeInterceptorTest { + + companion object { + /** + * Lock to prevent parallel tests from racing on Locale.setDefault(). + * JVM locale is global state — concurrent mutation causes flaky tests. + */ + private val LOCALE_LOCK = Any() + } + + /** + * Interceptor using the BROKEN locale-dependent formatting. + * This is what the code did before the fix. + */ + class BrokenInterceptor : Interceptor { + override fun intercept(chain: Interceptor.Chain): Response { + val startTime = System.nanoTime() + val response = chain.proceed(chain.request()) + val endTime = System.nanoTime() + val elapsedTimeSeconds = (endTime - startTime) / 1_000_000_000.0 + val compressedSize = response.header("Content-Length")?.toLongOrNull() ?: 0L + val speedMbps = if (elapsedTimeSeconds > 0 && compressedSize > 0) { + (compressedSize * 8 / elapsedTimeSeconds) / 1_000_000.0 + } else { + 0.0 + } + return response.newBuilder() + .header("X-Speed-Mbps", "%.4f".format(speedMbps)) // locale-dependent! + .build() + } + } + + /** + * Interceptor using the FIXED locale-independent formatting. + */ + class FixedInterceptor : Interceptor { + override fun intercept(chain: Interceptor.Chain): Response { + val startTime = System.nanoTime() + val response = chain.proceed(chain.request()) + val endTime = System.nanoTime() + val elapsedTimeSeconds = (endTime - startTime) / 1_000_000_000.0 + val compressedSize = response.header("Content-Length")?.toLongOrNull() ?: 0L + val speedMbps = if (elapsedTimeSeconds > 0 && compressedSize > 0) { + (compressedSize * 8 / elapsedTimeSeconds) / 1_000_000.0 + } else { + 0.0 + } + return response.newBuilder() + .header("X-Speed-Mbps", String.format(Locale.US, "%.4f", speedMbps)) + .build() + } + } + + @Test + fun brokenInterceptor_failsWithArabicLocale() { + synchronized(LOCALE_LOCK) { + val originalLocale = Locale.getDefault() + try { + // Set Arabic locale — causes String.format to use Arabic-Indic digits + Locale.setDefault(Locale("ar")) + + MockWebServer().use { server -> + server.enqueue(MockResponse() + .setBody("hello") + .setHeader("Content-Length", "5")) + server.start() + + val client = OkHttpClient.Builder() + .addInterceptor(BrokenInterceptor()) + .build() + + val request = Request.Builder() + .url(server.url("/test")) + .build() + + // The broken interceptor produces Arabic-Indic digits in the header value. + // OkHttp's header validation rejects non-ASCII characters. + try { + client.newCall(request).execute().use { /* auto-close */ } + Assert.fail("Expected IllegalArgumentException from OkHttp header validation") + } catch (e: IllegalArgumentException) { + Assert.assertTrue( + "Expected error about unexpected char in header value", + e.message?.contains("Unexpected char") == true + ) + } + } + } finally { + Locale.setDefault(originalLocale) + } + } + } + + @Test + fun fixedInterceptor_succeedsWithArabicLocale() { + synchronized(LOCALE_LOCK) { + val originalLocale = Locale.getDefault() + try { + Locale.setDefault(Locale("ar")) + + MockWebServer().use { server -> + server.enqueue(MockResponse() + .setBody("hello") + .setHeader("Content-Length", "5")) + server.start() + + val client = OkHttpClient.Builder() + .addInterceptor(FixedInterceptor()) + .build() + + val request = Request.Builder() + .url(server.url("/test")) + .build() + + client.newCall(request).execute().use { response -> + // Should succeed without throwing + val speedHeader = response.header("X-Speed-Mbps") + Assert.assertNotNull("X-Speed-Mbps header should be present", speedHeader) + + // Verify the value contains only ASCII characters + Assert.assertTrue( + "Header value should contain only ASCII: $speedHeader", + speedHeader!!.all { it.code in 0x20..0x7E } + ) + } + } + } finally { + Locale.setDefault(originalLocale) + } + } + } + + @Test + fun fixedInterceptor_succeedsWithUSLocale() { + MockWebServer().use { server -> + server.enqueue(MockResponse() + .setBody("hello") + .setHeader("Content-Length", "5")) + server.start() + + val client = OkHttpClient.Builder() + .addInterceptor(FixedInterceptor()) + .build() + + val request = Request.Builder() + .url(server.url("/test")) + .build() + + client.newCall(request).execute().use { response -> + val speedHeader = response.header("X-Speed-Mbps") + Assert.assertNotNull(speedHeader) + Assert.assertTrue(speedHeader!!.all { it.code in 0x20..0x7E }) + } + } + } +}