Skip to content

Commit 1e871c1

Browse files
pavelzemanclaude
pavelzeman
and
claude
committed
fix: address CodeRabbit review — resource leaks and locale thread-safety
- Wrap MockWebServer and Response in use{} blocks to prevent resource leaks if assertions fail (socket exhaustion, test flakiness) - Add LOCALE_LOCK companion object and synchronized{} blocks around Locale.setDefault() mutations to prevent races under parallel execution - Applied to both BearerTokenSanitizationTest and CompressedResponseSizeInterceptorTest Co-authored-by: Claude <claude@anthropic.com>
1 parent c33eb1b commit 1e871c1

2 files changed

Lines changed: 155 additions & 147 deletions

File tree

‎test-runner/src/test/kotlin/com/mattermost/networkclient/BearerTokenSanitizationTest.kt‎

Lines changed: 70 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -50,108 +50,106 @@ class BearerTokenSanitizationTest {
5050
fun brokenInterceptor_failsWithControlCharInToken() {
5151
val corruptToken = "abc\u0002def" // Contains 0x02 control character
5252

53-
val server = MockWebServer()
54-
server.enqueue(MockResponse().setBody("ok"))
55-
server.start()
56-
57-
val client = OkHttpClient.Builder()
58-
.addInterceptor(BrokenTokenInterceptor(corruptToken))
59-
.build()
60-
61-
val request = Request.Builder()
62-
.url(server.url("/test"))
63-
.build()
64-
65-
try {
66-
client.newCall(request).execute()
67-
Assert.fail("Expected IllegalArgumentException from OkHttp header validation")
68-
} catch (e: IllegalArgumentException) {
69-
Assert.assertTrue(
70-
"Expected error about unexpected char",
71-
e.message?.contains("Unexpected char") == true
72-
)
73-
}
53+
MockWebServer().use { server ->
54+
server.enqueue(MockResponse().setBody("ok"))
55+
server.start()
56+
57+
val client = OkHttpClient.Builder()
58+
.addInterceptor(BrokenTokenInterceptor(corruptToken))
59+
.build()
7460

75-
server.shutdown()
61+
val request = Request.Builder()
62+
.url(server.url("/test"))
63+
.build()
64+
65+
try {
66+
client.newCall(request).execute().use { /* auto-close */ }
67+
Assert.fail("Expected IllegalArgumentException from OkHttp header validation")
68+
} catch (e: IllegalArgumentException) {
69+
Assert.assertTrue(
70+
"Expected error about unexpected char",
71+
e.message?.contains("Unexpected char") == true
72+
)
73+
}
74+
}
7675
}
7776

7877
@Test
7978
fun fixedInterceptor_succeedsWithControlCharInToken() {
8079
val corruptToken = "abc\u0002def"
8180

82-
val server = MockWebServer()
83-
server.enqueue(MockResponse().setBody("ok"))
84-
server.start()
85-
86-
val client = OkHttpClient.Builder()
87-
.addInterceptor(FixedTokenInterceptor(corruptToken))
88-
.build()
89-
90-
val request = Request.Builder()
91-
.url(server.url("/test"))
92-
.build()
81+
MockWebServer().use { server ->
82+
server.enqueue(MockResponse().setBody("ok"))
83+
server.start()
9384

94-
val response = client.newCall(request).execute()
85+
val client = OkHttpClient.Builder()
86+
.addInterceptor(FixedTokenInterceptor(corruptToken))
87+
.build()
9588

96-
// Should succeed without throwing
97-
Assert.assertEquals(200, response.code)
89+
val request = Request.Builder()
90+
.url(server.url("/test"))
91+
.build()
9892

99-
// Verify the server received the sanitized token
100-
val recordedRequest = server.takeRequest()
101-
val authHeader = recordedRequest.getHeader("Authorization")
102-
Assert.assertEquals("Bearer abcdef", authHeader)
93+
client.newCall(request).execute().use { response ->
94+
// Should succeed without throwing
95+
Assert.assertEquals(200, response.code)
96+
}
10397

104-
server.shutdown()
98+
// Verify the server received the sanitized token
99+
val recordedRequest = server.takeRequest()
100+
val authHeader = recordedRequest.getHeader("Authorization")
101+
Assert.assertEquals("Bearer abcdef", authHeader)
102+
}
105103
}
106104

107105
@Test
108106
fun fixedInterceptor_skipsHeaderWhenTokenIsAllControlChars() {
109107
val allControlChars = "\u0001\u0002\u0003"
110108

111-
val server = MockWebServer()
112-
server.enqueue(MockResponse().setBody("ok"))
113-
server.start()
114-
115-
val client = OkHttpClient.Builder()
116-
.addInterceptor(FixedTokenInterceptor(allControlChars))
117-
.build()
109+
MockWebServer().use { server ->
110+
server.enqueue(MockResponse().setBody("ok"))
111+
server.start()
118112

119-
val request = Request.Builder()
120-
.url(server.url("/test"))
121-
.build()
113+
val client = OkHttpClient.Builder()
114+
.addInterceptor(FixedTokenInterceptor(allControlChars))
115+
.build()
122116

123-
val response = client.newCall(request).execute()
124-
Assert.assertEquals(200, response.code)
117+
val request = Request.Builder()
118+
.url(server.url("/test"))
119+
.build()
125120

126-
// No Authorization header should be set
127-
val recordedRequest = server.takeRequest()
128-
Assert.assertNull(recordedRequest.getHeader("Authorization"))
121+
client.newCall(request).execute().use { response ->
122+
Assert.assertEquals(200, response.code)
123+
}
129124

130-
server.shutdown()
125+
// No Authorization header should be set
126+
val recordedRequest = server.takeRequest()
127+
Assert.assertNull(recordedRequest.getHeader("Authorization"))
128+
}
131129
}
132130

133131
@Test
134132
fun fixedInterceptor_passesCleanTokenUnmodified() {
135133
val cleanToken = "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.valid.token"
136134

137-
val server = MockWebServer()
138-
server.enqueue(MockResponse().setBody("ok"))
139-
server.start()
135+
MockWebServer().use { server ->
136+
server.enqueue(MockResponse().setBody("ok"))
137+
server.start()
140138

141-
val client = OkHttpClient.Builder()
142-
.addInterceptor(FixedTokenInterceptor(cleanToken))
143-
.build()
144-
145-
val request = Request.Builder()
146-
.url(server.url("/test"))
147-
.build()
139+
val client = OkHttpClient.Builder()
140+
.addInterceptor(FixedTokenInterceptor(cleanToken))
141+
.build()
148142

149-
val response = client.newCall(request).execute()
150-
Assert.assertEquals(200, response.code)
143+
val request = Request.Builder()
144+
.url(server.url("/test"))
145+
.build()
151146

152-
val recordedRequest = server.takeRequest()
153-
Assert.assertEquals("Bearer $cleanToken", recordedRequest.getHeader("Authorization"))
147+
client.newCall(request).execute().use { response ->
148+
Assert.assertEquals(200, response.code)
149+
}
154150

155-
server.shutdown()
151+
val recordedRequest = server.takeRequest()
152+
Assert.assertEquals("Bearer $cleanToken", recordedRequest.getHeader("Authorization"))
153+
}
156154
}
157155
}

‎test-runner/src/test/kotlin/com/mattermost/networkclient/CompressedResponseSizeInterceptorTest.kt‎

Lines changed: 85 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,14 @@ import java.util.Locale
2020
*/
2121
class CompressedResponseSizeInterceptorTest {
2222

23+
companion object {
24+
/**
25+
* Lock to prevent parallel tests from racing on Locale.setDefault().
26+
* JVM locale is global state — concurrent mutation causes flaky tests.
27+
*/
28+
private val LOCALE_LOCK = Any()
29+
}
30+
2331
/**
2432
* Interceptor using the BROKEN locale-dependent formatting.
2533
* This is what the code did before the fix.
@@ -65,50 +73,86 @@ class CompressedResponseSizeInterceptorTest {
6573

6674
@Test
6775
fun brokenInterceptor_failsWithArabicLocale() {
68-
val originalLocale = Locale.getDefault()
69-
try {
70-
// Set Arabic locale — causes String.format to use Arabic-Indic digits
71-
Locale.setDefault(Locale("ar"))
72-
73-
val server = MockWebServer()
74-
server.enqueue(MockResponse()
75-
.setBody("hello")
76-
.setHeader("Content-Length", "5"))
77-
server.start()
78-
79-
val client = OkHttpClient.Builder()
80-
.addInterceptor(BrokenInterceptor())
81-
.build()
82-
83-
val request = Request.Builder()
84-
.url(server.url("/test"))
85-
.build()
86-
87-
// The broken interceptor produces Arabic-Indic digits in the header value.
88-
// OkHttp's header validation rejects non-ASCII characters.
76+
synchronized(LOCALE_LOCK) {
77+
val originalLocale = Locale.getDefault()
8978
try {
90-
client.newCall(request).execute()
91-
Assert.fail("Expected IllegalArgumentException from OkHttp header validation")
92-
} catch (e: IllegalArgumentException) {
93-
Assert.assertTrue(
94-
"Expected error about unexpected char in header value",
95-
e.message?.contains("Unexpected char") == true
96-
)
79+
// Set Arabic locale — causes String.format to use Arabic-Indic digits
80+
Locale.setDefault(Locale("ar"))
81+
82+
MockWebServer().use { server ->
83+
server.enqueue(MockResponse()
84+
.setBody("hello")
85+
.setHeader("Content-Length", "5"))
86+
server.start()
87+
88+
val client = OkHttpClient.Builder()
89+
.addInterceptor(BrokenInterceptor())
90+
.build()
91+
92+
val request = Request.Builder()
93+
.url(server.url("/test"))
94+
.build()
95+
96+
// The broken interceptor produces Arabic-Indic digits in the header value.
97+
// OkHttp's header validation rejects non-ASCII characters.
98+
try {
99+
client.newCall(request).execute().use { /* auto-close */ }
100+
Assert.fail("Expected IllegalArgumentException from OkHttp header validation")
101+
} catch (e: IllegalArgumentException) {
102+
Assert.assertTrue(
103+
"Expected error about unexpected char in header value",
104+
e.message?.contains("Unexpected char") == true
105+
)
106+
}
107+
}
108+
} finally {
109+
Locale.setDefault(originalLocale)
97110
}
98-
99-
server.shutdown()
100-
} finally {
101-
Locale.setDefault(originalLocale)
102111
}
103112
}
104113

105114
@Test
106115
fun fixedInterceptor_succeedsWithArabicLocale() {
107-
val originalLocale = Locale.getDefault()
108-
try {
109-
Locale.setDefault(Locale("ar"))
116+
synchronized(LOCALE_LOCK) {
117+
val originalLocale = Locale.getDefault()
118+
try {
119+
Locale.setDefault(Locale("ar"))
120+
121+
MockWebServer().use { server ->
122+
server.enqueue(MockResponse()
123+
.setBody("hello")
124+
.setHeader("Content-Length", "5"))
125+
server.start()
126+
127+
val client = OkHttpClient.Builder()
128+
.addInterceptor(FixedInterceptor())
129+
.build()
130+
131+
val request = Request.Builder()
132+
.url(server.url("/test"))
133+
.build()
134+
135+
client.newCall(request).execute().use { response ->
136+
// Should succeed without throwing
137+
val speedHeader = response.header("X-Speed-Mbps")
138+
Assert.assertNotNull("X-Speed-Mbps header should be present", speedHeader)
139+
140+
// Verify the value contains only ASCII characters
141+
Assert.assertTrue(
142+
"Header value should contain only ASCII: $speedHeader",
143+
speedHeader!!.all { it.code in 0x20..0x7E }
144+
)
145+
}
146+
}
147+
} finally {
148+
Locale.setDefault(originalLocale)
149+
}
150+
}
151+
}
110152

111-
val server = MockWebServer()
153+
@Test
154+
fun fixedInterceptor_succeedsWithUSLocale() {
155+
MockWebServer().use { server ->
112156
server.enqueue(MockResponse()
113157
.setBody("hello")
114158
.setHeader("Content-Length", "5"))
@@ -122,45 +166,11 @@ class CompressedResponseSizeInterceptorTest {
122166
.url(server.url("/test"))
123167
.build()
124168

125-
val response = client.newCall(request).execute()
126-
127-
// Should succeed without throwing
128-
val speedHeader = response.header("X-Speed-Mbps")
129-
Assert.assertNotNull("X-Speed-Mbps header should be present", speedHeader)
130-
131-
// Verify the value contains only ASCII characters
132-
Assert.assertTrue(
133-
"Header value should contain only ASCII: $speedHeader",
134-
speedHeader!!.all { it.code in 0x20..0x7E }
135-
)
136-
137-
server.shutdown()
138-
} finally {
139-
Locale.setDefault(originalLocale)
169+
client.newCall(request).execute().use { response ->
170+
val speedHeader = response.header("X-Speed-Mbps")
171+
Assert.assertNotNull(speedHeader)
172+
Assert.assertTrue(speedHeader!!.all { it.code in 0x20..0x7E })
173+
}
140174
}
141175
}
142-
143-
@Test
144-
fun fixedInterceptor_succeedsWithUSLocale() {
145-
val server = MockWebServer()
146-
server.enqueue(MockResponse()
147-
.setBody("hello")
148-
.setHeader("Content-Length", "5"))
149-
server.start()
150-
151-
val client = OkHttpClient.Builder()
152-
.addInterceptor(FixedInterceptor())
153-
.build()
154-
155-
val request = Request.Builder()
156-
.url(server.url("/test"))
157-
.build()
158-
159-
val response = client.newCall(request).execute()
160-
val speedHeader = response.header("X-Speed-Mbps")
161-
Assert.assertNotNull(speedHeader)
162-
Assert.assertTrue(speedHeader!!.all { it.code in 0x20..0x7E })
163-
164-
server.shutdown()
165-
}
166176
}

0 commit comments

Comments
 (0)