Fix OkHttp header validation crashes from non-ASCII characters on Android#160
Fix OkHttp header validation crashes from non-ASCII characters on Android#160
Conversation
Willyfrog
left a comment
Willyfrog
left a comment
There was a problem hiding this comment.
LGTM, added a nitpick to prevent headaches when debugging issues in the future
android/src/main/java/com/mattermost/networkclient/interceptors/BearerTokenInterceptor.kt
Outdated
Show resolved
Hide resolved
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis change addresses locale-dependency issues in HTTP header formatting and validation. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
test-runner/build.gradle.kts (1)
15-21: Optional:sourceSetsconfiguration is redundant.The
src/test/kotlinpath is the default convention for Kotlin JVM projects, so this explicit configuration can be removed without affecting behaviour.♻️ Suggested simplification
-sourceSets { - test { - kotlin { - srcDir("src/test/kotlin") - } - } -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-runner/build.gradle.kts` around lines 15 - 21, The explicit sourceSets configuration for test (the block referencing sourceSets { test { kotlin { srcDir("src/test/kotlin") } } }) is redundant because Kotlin JVM uses src/test/kotlin by convention; remove the entire sourceSets test kotlin srcDir block from build.gradle.kts to simplify the file and rely on default source layout.test-runner/src/test/kotlin/com/mattermost/networkclient/CompressedResponseSizeInterceptorTest.kt (1)
88-106: Consider shutting down OkHttpClient after tests.
OkHttpClientmaintains a connection pool and dispatcher thread pool that should be shut down to release resources. While this won't cause test failures, it can lead to resource leaks and slower test execution if many tests accumulate.♻️ Suggested improvement
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 - ) + try { + // 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 { + client.dispatcher.executorService.shutdown() + client.connectionPool.evictAll() }🤖 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 88 - 106, The test creates an OkHttpClient (variable client) with BrokenInterceptor but never shuts it down, leaking the dispatcher and connection pool; wrap the client.newCall(...).execute() call in a try/finally (or add a finally block) and in the finally call the OkHttp shutdown steps on the client: shut down client.dispatcher.executorService and evict connections from client.connectionPool (and close client.cache if present) so BrokenInterceptor test cleans up resources after execution.test-runner/src/test/kotlin/com/mattermost/networkclient/BearerTokenSanitizationTest.kt (1)
99-100: Use boundedtakeRequestcalls to avoid hanging CI tests.
MockWebServer.takeRequest()blocks indefinitely. On failure, these tests can hang instead of failing fast.Suggested patch
import org.junit.Assert import org.junit.Test +import java.util.concurrent.TimeUnit @@ - val recordedRequest = server.takeRequest() - val authHeader = recordedRequest.getHeader("Authorization") + val recordedRequest = server.takeRequest(2, TimeUnit.SECONDS) + Assert.assertNotNull("Expected request to reach MockWebServer", recordedRequest) + val authHeader = recordedRequest!!.getHeader("Authorization") Assert.assertEquals("Bearer abcdef", authHeader) @@ - val recordedRequest = server.takeRequest() - Assert.assertNull(recordedRequest.getHeader("Authorization")) + val recordedRequest = server.takeRequest(2, TimeUnit.SECONDS) + Assert.assertNotNull("Expected request to reach MockWebServer", recordedRequest) + Assert.assertNull(recordedRequest!!.getHeader("Authorization")) @@ - val recordedRequest = server.takeRequest() - Assert.assertEquals("Bearer $cleanToken", recordedRequest.getHeader("Authorization")) + val recordedRequest = server.takeRequest(2, TimeUnit.SECONDS) + Assert.assertNotNull("Expected request to reach MockWebServer", recordedRequest) + Assert.assertEquals("Bearer $cleanToken", recordedRequest!!.getHeader("Authorization"))Also applies to: 126-127, 151-152
🤖 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 99 - 100, The tests currently call server.takeRequest() which can block indefinitely; replace those calls to use the bounded version (server.takeRequest with a timeout) so tests fail fast instead of hanging — update the occurrences where recordedRequest is assigned from server.takeRequest (and subsequent uses of authHeader) in BearerTokenSanitizationTest.kt (the recordedRequest variable and the server.takeRequest calls around the Authorization header assertions) to call the timed takeRequest overload and assert non-null/handle timeout if it returns null.
🤖 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/BearerTokenSanitizationTest.kt`:
- Around line 36-47: Replace the test-only FixedTokenInterceptor with the real
production interceptor
com.mattermost.networkclient.interceptors.BearerTokenInterceptor by wiring it
into the test with a mocked token source/provider (or a stubbed function) so the
test exercises its actual sanitization logic; locate where FixedTokenInterceptor
is used and instantiate BearerTokenInterceptor (or call its constructor that
accepts the token provider) with a mock that returns the unsafe token, then
verify outgoing requests contain the sanitized "Authorization: Bearer ..."
header as before.
---
Nitpick comments:
In `@test-runner/build.gradle.kts`:
- Around line 15-21: The explicit sourceSets configuration for test (the block
referencing sourceSets { test { kotlin { srcDir("src/test/kotlin") } } }) is
redundant because Kotlin JVM uses src/test/kotlin by convention; remove the
entire sourceSets test kotlin srcDir block from build.gradle.kts to simplify the
file and rely on default source layout.
In
`@test-runner/src/test/kotlin/com/mattermost/networkclient/BearerTokenSanitizationTest.kt`:
- Around line 99-100: The tests currently call server.takeRequest() which can
block indefinitely; replace those calls to use the bounded version
(server.takeRequest with a timeout) so tests fail fast instead of hanging —
update the occurrences where recordedRequest is assigned from server.takeRequest
(and subsequent uses of authHeader) in BearerTokenSanitizationTest.kt (the
recordedRequest variable and the server.takeRequest calls around the
Authorization header assertions) to call the timed takeRequest overload and
assert non-null/handle timeout if it returns null.
In
`@test-runner/src/test/kotlin/com/mattermost/networkclient/CompressedResponseSizeInterceptorTest.kt`:
- Around line 88-106: The test creates an OkHttpClient (variable client) with
BrokenInterceptor but never shuts it down, leaking the dispatcher and connection
pool; wrap the client.newCall(...).execute() call in a try/finally (or add a
finally block) and in the finally call the OkHttp shutdown steps on the client:
shut down client.dispatcher.executorService and evict connections from
client.connectionPool (and close client.cache if present) so BrokenInterceptor
test cleans up resources after execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1f7d8420-ead6-4e0c-a6ed-dd776d138d5f
⛔ 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
test-runner/src/test/kotlin/com/mattermost/networkclient/BearerTokenSanitizationTest.kt
Outdated
Show resolved
Hide resolved
pavelzeman
dismissed
coderabbitai[bot]’s stale review
All findings addressed: resource leak fix (use{} blocks), locale thread-safety (LOCALE_LOCK), and production interceptor duplication is by design (standalone JVM test-runner can't compile Android/RN dependencies).
larkox
left a comment
larkox
left a comment
There was a problem hiding this comment.
I would love some security eyes on this PR. Probably I am too cautious, but if someone from @esarafianou team can take a look to see whether what we are doing is correct, I would feel more comfortable.
Also, similar to the other PR. I love we adding tests to the native side, but not sure if we are running these tests on CI. A great improvement if we start doing it.
pavelzeman
force-pushed
the
fix/sentry-android-header-validation
branch
from
9eb212d to
40da9fb
Compare
|
Leaving open a little longer, so that @esarafianou or @edgarbellot can have a look. |
esarafianou
left a comment
esarafianou
left a comment
There was a problem hiding this comment.
The changes in CompressedResponseSizeInterceptor.kt for MATTERMOST-MOBILE-ANDROID-AZ02 look good.
I don't think that the BearerTokenInterceptor.kt changes address the MATTERMOST-MOBILE-ANDROID-AYPW crash. Looking at the Sentry stack trace, the crash happens in ExtensionsKt.applyHeaders at line 138, called from NetworkClient.buildRequest at line 446. That's the per-request headers path during request construction.
Request construction and interceptor execution are two separate phases. When buildRequest runs, it calls applyHeaders, which calls OkHttp's addHeader() and OkHttp validates the header value immediately. If the value contains a non-ASCII character like 0x02, it throws IllegalArgumentException and the request never finishes being built. BearerTokenInterceptor is an OkHttp interceptor, which means it only runs after the request is fully constructed and handed to the interceptor chain for execution. It never gets a chance to sanitize anything because the crash has already happened during construction.
The fix should go in applyHeaders in Extensions.kt, but I don't think it should sanitize the header value. This library is a transport layer, it shouldn't silently modify credentials or decide what a good enough token looks like. Instead, applyHeaders should catch the IllegalArgumentException from addHeader, skip the invalid header, and let the request proceed without it. This turns an unhandled crash into a failed request that the JS layer can handle gracefully.
I'd be also worth opening a follow up to investigate why the JS layer is sending a token with 0x02 in upload request headers. This library is just the transport layer, so the real fix should be upstream wherever the token gets corrupted.
cc. @larkox, @Willyfrog tagging you for visibility since you're our mobile experts
…Http crashes On devices with Arabic locale, String.format() produces Arabic-Indic digits (U+0660 range) in the X-Speed-Mbps header value. OkHttp rejects non-ASCII characters in header values with IllegalArgumentException. Use String.format(Locale.US, ...) to ensure ASCII digits regardless of device locale. Fixes: MATTERMOST-MOBILE-ANDROID-AZ02 Co-authored-by: Claude <claude@anthropic.com>
OkHttp validates header values during request construction and throws IllegalArgumentException for non-ASCII characters. When the JS layer passes a corrupted token (e.g. containing 0x02 control characters), the crash happens in applyHeaders during addHeader() — before any interceptor gets a chance to run. Catch the exception per-header, log a warning, skip the invalid header, and let the request proceed. The server will reject it with a 4xx that the JS layer can handle gracefully. This library is a transport layer and should not silently modify credentials. Fixes: MATTERMOST-MOBILE-ANDROID-AYPW Co-authored-by: Claude <claude@anthropic.com>
pavelzeman
force-pushed
the
fix/sentry-android-header-validation
branch
from
40da9fb to
67c4b2a
Compare
pavelzeman
left a comment
pavelzeman
left a comment
There was a problem hiding this comment.
Thanks @esarafianou — great catch on the interceptor vs request construction timing. You're absolutely right: the crash happens in applyHeaders during addHeader(), well before any interceptor in the chain gets a chance to run. The BearerTokenInterceptor fix was addressing the wrong phase entirely.
Reworked the PR based on your feedback:
- Reverted
BearerTokenInterceptor.ktto its original state - Moved the fix to
applyHeaders()inExtensions.kt— catchesIllegalArgumentExceptionper-header, logs a warning, skips the invalid header, and lets the request proceed - No sanitization — as you noted, this is a transport layer and shouldn't silently modify credentials. The server will reject with a 4xx that JS can handle
- Updated tests to validate the new approach (catch-and-skip in applyHeaders instead of token sanitization)
We're also investigating upstream why the JS layer is sending a token with 0x02 — will open a follow-up issue once we have findings.
pavelzeman
left a comment
pavelzeman
left a comment
There was a problem hiding this comment.
Thanks @esarafianou — great catch on the interceptor vs request construction timing. You're absolutely right: the crash happens in applyHeaders during addHeader(), well before any interceptor in the chain gets a chance to run. The BearerTokenInterceptor fix was addressing the wrong phase entirely.
Reworked the PR based on your feedback:
- Reverted
BearerTokenInterceptor.ktto its original state - Moved the fix to
applyHeaders()inExtensions.kt— catchesIllegalArgumentExceptionper-header, logs a warning, skips the invalid header, and lets the request proceed - No sanitization — as you noted, this is a transport layer and shouldn't silently modify credentials. The server will reject with a 4xx that JS can handle
- Updated tests to validate the new approach (catch-and-skip in applyHeaders instead of token sanitization)
We're also investigating upstream why the JS layer is sending a token with 0x02 — will open a follow-up issue once we have findings.
5 participants
Summary
Fixes two OkHttp
IllegalArgumentExceptioncrashes on Android where header values contain characters that fail OkHttp's ASCII validation.1. CompressedResponseSizeInterceptor — Arabic locale digits (MATTERMOST-MOBILE-ANDROID-AZ02)
On devices with Arabic system locale,
String.format("%.4f", value)produces Arabic-Indic digits (U+0660–U+0669) in theX-Speed-Mbpsheader. OkHttp rejects these as non-ASCII.Fix: Use
String.format(Locale.US, "%.4f", value)to ensure ASCII digits regardless of device locale.2. applyHeaders — corrupted token with control characters (MATTERMOST-MOBILE-ANDROID-AYPW)
When the JS layer passes a token containing non-ASCII control characters (e.g.
0x02), OkHttp throwsIllegalArgumentExceptionduringaddHeader()inapplyHeaders()at request construction time — before any interceptor runs.Fix: Catch
IllegalArgumentExceptionper-header inapplyHeaders(), log a warning, skip the invalid header, and let the request proceed. The server will respond with a 4xx that the JS layer can handle gracefully. As a transport layer, this library should not silently modify or sanitize credentials.A follow-up investigation into why the JS layer sends corrupted tokens is warranted.
Release Note