Fix OkHttp header validation crashes from non-ASCII characters on Android#155
Closed
pavelzeman wants to merge 2 commits intomattermost:masterfrom
Closed
Fix OkHttp header validation crashes from non-ASCII characters on Android#155pavelzeman wants to merge 2 commits intomattermost:masterfrom
pavelzeman wants to merge 2 commits intomattermost:masterfrom
Conversation
Fix two Android crashes caused by non-ASCII characters in HTTP headers: 1. CompressedResponseSizeInterceptor: Use Locale.US for formatting X-Speed-Mbps header value. On Arabic-locale devices, String.format() produces Arabic-Indic digits (U+0660 etc.) which OkHttp rejects as invalid header characters. 2. BearerTokenInterceptor: Sanitize bearer token to ASCII printable characters before setting the Authorization header. Corrupted tokens with control characters (e.g. 0x02) cause IllegalArgumentException. Sentry: MATTERMOST-MOBILE-ANDROID-AZ02 https://mattermost-mr.sentry.io/issues/7194769442/ Sentry: MATTERMOST-MOBILE-ANDROID-AYPW https://mattermost-mr.sentry.io/issues/7077385609/ Co-authored-by: Claude <claude@anthropic.com>
Add standalone test-runner with 7 tests covering both interceptor fixes: CompressedResponseSizeInterceptor (AZ02): - brokenInterceptor_failsWithArabicLocale: proves locale-dependent formatting crashes - fixedInterceptor_succeedsWithArabicLocale: proves Locale.US fix works - fixedInterceptor_succeedsWithUSLocale: regression test BearerTokenInterceptor (AYPW): - brokenInterceptor_failsWithControlCharInToken: proves corrupt tokens crash - fixedInterceptor_succeedsWithControlCharInToken: proves sanitization works - fixedInterceptor_skipsHeaderWhenTokenIsAllControlChars: edge case - fixedInterceptor_passesCleanTokenUnmodified: regression test Run with: cd test-runner && ./gradlew test Co-authored-by: Claude <claude@anthropic.com>
This was referenced Mar 20, 2026
📝 WalkthroughWalkthroughThe pull request addresses locale and encoding issues in HTTP interceptors by sanitizing bearer tokens and enforcing locale-independent numeric formatting. It also establishes a new test runner module with comprehensive test suites validating these fixes using MockWebServer and multiple locale scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can disable the changed files summary in the walkthrough.Disable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
test-runner/gradle/wrapper/gradle-wrapper.properties (1)
1-7: Add distribution checksum pinning for Gradle wrapper integrity.
validateDistributionUrl=truehelps, but it does not pin the downloaded ZIP contents. AddingdistributionSha256Sumprevents silent tampering of the wrapper distribution.Proposed hardening change
distributionBase=GRADLE_USER_HOME distributionPath=wrapper/dists distributionUrl=https\://services.gradle.org/distributions/gradle-8.5-bin.zip +distributionSha256Sum=9d926787066a081739e8200858338b4a821a33ca9db09dd4a41026 networkTimeout=10000 validateDistributionUrl=true zipStoreBase=GRADLE_USER_HOME zipStorePath=wrapper/dists🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-runner/gradle/wrapper/gradle-wrapper.properties` around lines 1 - 7, Add a pinned SHA-256 checksum for the Gradle distribution to harden the wrapper: update the gradle-wrapper.properties to include a distributionSha256Sum entry that matches the SHA-256 of the file referenced by distributionUrl, keeping the existing distributionUrl and validateDistributionUrl keys; ensure the checksum string uses lowercase hex and is the exact 64-character SHA-256 for the gradle-8.5-bin.zip so the wrapper validates the downloaded ZIP contents.test-runner/src/test/kotlin/com/mattermost/networkclient/BearerTokenSanitizationTest.kt (1)
53-156: Make test cleanup deterministic using try-with-resources patterns.Lines 75, 104, 130, and 155 call
server.shutdown()only after assertions succeed. If any assertion fails,MockWebServerstays open and causes port conflicts with follow-up tests. Additionally, Response objects created at lines 94, 123, and 149 are never closed, leaking OkHttp connections.MockWebServer implements AutoCloseable and Response requires explicit closure per OkHttp documentation. Wrap both with Kotlin's
.use {}:Refactor pattern (apply to all four tests)
- val server = MockWebServer() - server.enqueue(MockResponse().setBody("ok")) - server.start() - - val client = OkHttpClient.Builder() - .addInterceptor(FixedTokenInterceptor(corruptToken)) - .build() - - val request = Request.Builder() - .url(server.url("/test")) - .build() - - val response = client.newCall(request).execute() - Assert.assertEquals(200, response.code) - - val recordedRequest = server.takeRequest() - val authHeader = recordedRequest.getHeader("Authorization") - Assert.assertEquals("Bearer abcdef", authHeader) - - server.shutdown() + MockWebServer().use { server -> + server.enqueue(MockResponse().setBody("ok")) + server.start() + + val client = OkHttpClient.Builder() + .addInterceptor(FixedTokenInterceptor(corruptToken)) + .build() + + val request = Request.Builder() + .url(server.url("/test")) + .build() + + client.newCall(request).execute().use { response -> + Assert.assertEquals(200, response.code) + } + + val recordedRequest = server.takeRequest() + val authHeader = recordedRequest.getHeader("Authorization") + Assert.assertEquals("Bearer abcdef", authHeader) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-runner/src/test/kotlin/com/mattermost/networkclient/BearerTokenSanitizationTest.kt` around lines 53 - 156, Tests fixedInterceptor_succeedsWithControlCharInToken, fixedInterceptor_skipsHeaderWhenTokenIsAllControlChars, fixedInterceptor_passesCleanTokenUnmodified (and the earlier failing test) leak MockWebServer and Response objects; wrap MockWebServer and any Response returned by client.newCall(request).execute() in Kotlin's use {} to ensure deterministic closing (MockWebServer implements AutoCloseable and Response must be closed). Locate usages of MockWebServer and Response in those test functions and replace manual server.start()/server.shutdown() and unclosed responses with the try-with-resources pattern (e.g., server.use { ... } and response.use { ... }) so the server is always closed and responses are always closed even when assertions fail.test-runner/src/test/kotlin/com/mattermost/networkclient/CompressedResponseSizeInterceptorTest.kt (2)
27-43: Test doubles are drifting from production interceptor logic.
BrokenInterceptor/FixedInterceptorcurrently only readContent-Length(Line [33], Line [54]), but productionandroid/src/main/java/com/mattermost/networkclient/interceptors/CompressedResponseSizeInterceptor.ktalso handles lowercasecontent-lengthand body-byte fallback. This reduces fidelity and can miss regressions unrelated to locale formatting.Refactor direction
- class FixedInterceptor : Interceptor { - override fun intercept(chain: Interceptor.Chain): Response { - ... - } - } + // Use production interceptor directly for fixed-path tests. + // Keep only a minimal local BrokenInterceptor for the regression reproduction.Also applies to: 48-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-runner/src/test/kotlin/com/mattermost/networkclient/CompressedResponseSizeInterceptorTest.kt` around lines 27 - 43, BrokenInterceptor and FixedInterceptor read only "Content-Length" and miss production behaviour; update their intercept(chain: Interceptor.Chain): Response logic to mirror CompressedResponseSizeInterceptor by checking both "Content-Length" and "content-length" headers, falling back to response.body()?.contentLength() when header is absent or not parseable to compute compressedSize, and format the X-Speed-Mbps header in a locale-independent way (matching production formatting strategy) before returning response.newBuilder().header(...).build().
89-96: Avoid asserting exact OkHttp exception text.Line [95] hard-codes
"Unexpected char"; that message is implementation-detail brittle across OkHttp versions. Prefer asserting exception type only (or a looser invariant like non-ASCII header rejection context).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-runner/src/test/kotlin/com/mattermost/networkclient/CompressedResponseSizeInterceptorTest.kt` around lines 89 - 96, The test in CompressedResponseSizeInterceptorTest (around the client.newCall(request).execute() block) asserts on OkHttp's exact error text ("Unexpected char"), which is brittle; change the catch block to only assert the exception type (IllegalArgumentException) or a looser invariant such as that e.message is non-null and mentions header/character rejection, rather than matching the exact phrase; update the Assert.assertTrue(...) call to either remove the message check and leave Assert.assertNotNull(e) / Assert.assertTrue(e is IllegalArgumentException) or assert a more general substring like "header" or "char" if needed to keep intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@test-runner/src/test/kotlin/com/mattermost/networkclient/CompressedResponseSizeInterceptorTest.kt`:
- Around line 73-77: Wrap each MockWebServer instantiation and each Response
from client.newCall(request).execute() in Kotlin use blocks to ensure they are
always closed even on assertion failures: replace raw MockWebServer() ->
MockWebServer().use { server -> ... } around the test body (the variable named
server) and call .use { response -> ... } on the result of
client.newCall(request).execute() (the Response object) so server.shutdown() and
response.close() happen automatically; update the three test methods in
CompressedResponseSizeInterceptorTest that currently create server and call
client.newCall(request).execute() without use to this pattern.
- Around line 68-72: Tests mutate the JVM global Locale via
Locale.setDefault(...) in CompressedResponseSizeInterceptorTest and restoring it
in try-finally, which races under parallel test execution; add a class-level
lock (e.g., a private val LOCALE_LOCK in the test class companion object) and
wrap each Locale.setDefault(...) and its corresponding restore inside
synchronized(LOCALE_LOCK) blocks, or alternatively disable parallel execution
for this test class, to ensure the locale mutation in the test methods (the
try-finally blocks around Locale.setDefault and Locale.getDefault()) cannot
interleave with other tests.
---
Nitpick comments:
In `@test-runner/gradle/wrapper/gradle-wrapper.properties`:
- Around line 1-7: Add a pinned SHA-256 checksum for the Gradle distribution to
harden the wrapper: update the gradle-wrapper.properties to include a
distributionSha256Sum entry that matches the SHA-256 of the file referenced by
distributionUrl, keeping the existing distributionUrl and
validateDistributionUrl keys; ensure the checksum string uses lowercase hex and
is the exact 64-character SHA-256 for the gradle-8.5-bin.zip so the wrapper
validates the downloaded ZIP contents.
In
`@test-runner/src/test/kotlin/com/mattermost/networkclient/BearerTokenSanitizationTest.kt`:
- Around line 53-156: Tests fixedInterceptor_succeedsWithControlCharInToken,
fixedInterceptor_skipsHeaderWhenTokenIsAllControlChars,
fixedInterceptor_passesCleanTokenUnmodified (and the earlier failing test) leak
MockWebServer and Response objects; wrap MockWebServer and any Response returned
by client.newCall(request).execute() in Kotlin's use {} to ensure deterministic
closing (MockWebServer implements AutoCloseable and Response must be closed).
Locate usages of MockWebServer and Response in those test functions and replace
manual server.start()/server.shutdown() and unclosed responses with the
try-with-resources pattern (e.g., server.use { ... } and response.use { ... })
so the server is always closed and responses are always closed even when
assertions fail.
In
`@test-runner/src/test/kotlin/com/mattermost/networkclient/CompressedResponseSizeInterceptorTest.kt`:
- Around line 27-43: BrokenInterceptor and FixedInterceptor read only
"Content-Length" and miss production behaviour; update their intercept(chain:
Interceptor.Chain): Response logic to mirror CompressedResponseSizeInterceptor
by checking both "Content-Length" and "content-length" headers, falling back to
response.body()?.contentLength() when header is absent or not parseable to
compute compressedSize, and format the X-Speed-Mbps header in a
locale-independent way (matching production formatting strategy) before
returning response.newBuilder().header(...).build().
- Around line 89-96: The test in CompressedResponseSizeInterceptorTest (around
the client.newCall(request).execute() block) asserts on OkHttp's exact error
text ("Unexpected char"), which is brittle; change the catch block to only
assert the exception type (IllegalArgumentException) or a looser invariant such
as that e.message is non-null and mentions header/character rejection, rather
than matching the exact phrase; update the Assert.assertTrue(...) call to either
remove the message check and leave Assert.assertNotNull(e) / Assert.assertTrue(e
is IllegalArgumentException) or assert a more general substring like "header" or
"char" if needed to keep intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b2a99ad6-1f33-4636-9cf3-4d18ed1d0afb
⛔ Files ignored due to path filters (1)
test-runner/gradle/wrapper/gradle-wrapper.jaris excluded by!**/*.jar
📒 Files selected for processing (9)
android/src/main/java/com/mattermost/networkclient/interceptors/BearerTokenInterceptor.ktandroid/src/main/java/com/mattermost/networkclient/interceptors/CompressedResponseSizeInterceptor.kttest-runner/build.gradle.ktstest-runner/gradle/wrapper/gradle-wrapper.propertiestest-runner/gradlewtest-runner/gradlew.battest-runner/settings.gradle.ktstest-runner/src/test/kotlin/com/mattermost/networkclient/BearerTokenSanitizationTest.kttest-runner/src/test/kotlin/com/mattermost/networkclient/CompressedResponseSizeInterceptorTest.kt
Comment on lines
+68
to
+72
| val originalLocale = Locale.getDefault() | ||
| try { | ||
| // Set Arabic locale — causes String.format to use Arabic-Indic digits | ||
| Locale.setDefault(Locale("ar")) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "CompressedResponseSizeInterceptorTest.kt"Repository: mattermost/react-native-network-client
Length of output: 181
🏁 Script executed:
cat -n ./test-runner/src/test/kotlin/com/mattermost/networkclient/CompressedResponseSizeInterceptorTest.ktRepository: mattermost/react-native-network-client
Length of output: 7077
🏁 Script executed:
find . -type f \( -name "pom.xml" -o -name "build.gradle*" -o -name "junit.properties" -o -name "junit-platform.properties" \) | head -20Repository: mattermost/react-native-network-client
Length of output: 201
🏁 Script executed:
cat ./test-runner/build.gradle.ktsRepository: mattermost/react-native-network-client
Length of output: 451
🏁 Script executed:
find ./test-runner -type f -name "gradle.properties" -o -name "settings.gradle*"Repository: mattermost/react-native-network-client
Length of output: 115
🏁 Script executed:
cat ./test-runner/settings.gradle.ktsRepository: mattermost/react-native-network-client
Length of output: 120
🏁 Script executed:
rg -l "parallel" ./test-runner/ 2>/dev/null || echo "No parallel configuration found"Repository: mattermost/react-native-network-client
Length of output: 113
🏁 Script executed:
# Check for any system property configurations or CI configurations that might enable parallel execution
find . -type f \( -name "*.md" -o -name ".github" -o -name "*.yml" -o -name "*.yaml" \) | grep -E "(CI|workflow|config|README)" | head -10Repository: mattermost/react-native-network-client
Length of output: 157
🏁 Script executed:
cat ./.circleci/config.yml | grep -A 5 -B 5 "test\|parallel" || echo "No test configuration found"Repository: mattermost/react-native-network-client
Length of output: 928
🏁 Script executed:
# Check gradle test configuration in the test-runner build file
rg "test\s*\{" ./test-runner/build.gradle.kts -A 10Repository: mattermost/react-native-network-client
Length of output: 165
🏁 Script executed:
# Check if there's any reference to parallel execution in docs or comments
rg "parallel|concurrent" ./test-runner/ --type kotlin --type java 2>/dev/null | head -20Repository: mattermost/react-native-network-client
Length of output: 64
Guard global locale mutation against parallel test execution.
Lines 71 and 109 mutate JVM-global locale via Locale.setDefault(). The try-finally pattern restores locale per test, but this is insufficient for concurrent test execution: if tests run in parallel, a race condition allows one thread to restore an incorrect locale state, causing cross-test interference. Add a class-level lock around both Locale.setDefault(...) sections, or disable test parallelism for this class.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@test-runner/src/test/kotlin/com/mattermost/networkclient/CompressedResponseSizeInterceptorTest.kt`
around lines 68 - 72, Tests mutate the JVM global Locale via
Locale.setDefault(...) in CompressedResponseSizeInterceptorTest and restoring it
in try-finally, which races under parallel test execution; add a class-level
lock (e.g., a private val LOCALE_LOCK in the test class companion object) and
wrap each Locale.setDefault(...) and its corresponding restore inside
synchronized(LOCALE_LOCK) blocks, or alternatively disable parallel execution
for this test class, to ensure the locale mutation in the test methods (the
try-finally blocks around Locale.setDefault and Locale.getDefault()) cannot
interleave with other tests.
Comment on lines
+73
to
+77
| val server = MockWebServer() | ||
| server.enqueue(MockResponse() | ||
| .setBody("hello") | ||
| .setHeader("Content-Length", "5")) | ||
| server.start() |
There was a problem hiding this comment.
Ensure server and response resources are always closed, even on assertion failures.
In the three test methods at lines 73–99, 111–137, and 145–164, MockWebServer instances are not wrapped in use blocks, and Response objects from client.newCall(request).execute() are never explicitly closed. If an assertion fails before server.shutdown() is reached (at lines 99, 137, or 164), both the server and response resources leak, potentially causing socket exhaustion and test flakiness.
Use use blocks to guarantee cleanup:
Suggested cleanup pattern
MockWebServer().use { server ->
server.enqueue(
MockResponse()
.setBody("hello")
.setHeader("Content-Length", "5")
)
server.start()
client.newCall(request).execute().use { response ->
val speedHeader = response.header("X-Speed-Mbps")
Assert.assertNotNull("X-Speed-Mbps header should be present", speedHeader)
Assert.assertTrue(
"Header value should contain only ASCII: $speedHeader",
speedHeader!!.all { it.code in 0x20..0x7E }
)
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@test-runner/src/test/kotlin/com/mattermost/networkclient/CompressedResponseSizeInterceptorTest.kt`
around lines 73 - 77, Wrap each MockWebServer instantiation and each Response
from client.newCall(request).execute() in Kotlin use blocks to ensure they are
always closed even on assertion failures: replace raw MockWebServer() ->
MockWebServer().use { server -> ... } around the test body (the variable named
server) and call .use { response -> ... } on the result of
client.newCall(request).execute() (the Response object) so server.shutdown() and
response.close() happen automatically; update the three test methods in
CompressedResponseSizeInterceptorTest that currently create server and call
client.newCall(request).execute() without use to this pattern.
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix two Android crashes caused by non-ASCII characters in HTTP header values that OkHttp rejects.
Ticket Link
Release Note
Root Cause
AZ02 — CompressedResponseSizeInterceptor (1,375 events, 640 users)
"%.4f".format(speedMbps)uses the system locale for number formatting. On devices with Arabic locale, this produces Arabic-Indic digits (٠١٢٣٤٥٦٧٨٩ — U+0660 range) in theX-Speed-Mbpsheader value. OkHttp validates that header values contain only ASCII characters and throwsIllegalArgumentException.Sentry title:
Unexpected char 0x660 at 0 in X-Speed-Mbps value: ٠٫٠١٨٩AYPW — BearerTokenInterceptor (20 events, 14 users)
Bearer token retrieved from storage contains non-ASCII control characters (e.g.
0x02). When set as theAuthorizationheader value, OkHttp rejects it.Sentry title:
Unexpected char 0x02 at 9 in Authorization valueChanges
CompressedResponseSizeInterceptor.kt: UseString.format(Locale.US, ...)instead of Kotlin'sString.format()to ensure ASCII digits regardless of device locale.BearerTokenInterceptor.kt: Sanitize bearer token to ASCII printable characters (0x20-0x7E) before setting the Authorization header. Skip the header entirely if the sanitized token is empty.Customer Impact (current, before this fix)
These crashes are happening right now in production on the current release (v2.36.4 / v2.38.0):
CompressedResponseSizeInterceptor, so the app is essentially unusable for these users.Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
Chores