Skip to content

Commit 477b1ba

Browse files
didiergarciadidiergarcia
authored
Feature/settings verification (#137)
* Verify that we handle bad settings input. * Remove unused imports. * Add test for invalid inputs to fetchSettings() function
1 parent a762652 commit 477b1ba

File tree

3 files changed

+141
-21
lines changed

3 files changed

+141
-21
lines changed

‎core/src/main/java/com/segment/analytics/kotlin/core/Settings.kt‎

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -86,19 +86,7 @@ suspend fun Analytics.checkSettings() {
8686

8787
withContext(networkIODispatcher) {
8888
log("Fetching settings on ${Thread.currentThread().name}")
89-
val settingsObj: Settings? = try {
90-
val connection = HTTPClient(writeKey).settings(cdnHost)
91-
val settingsString =
92-
connection.inputStream?.bufferedReader()?.use(BufferedReader::readText) ?: ""
93-
log("Fetched Settings: $settingsString")
94-
LenientJson.decodeFromString(settingsString)
95-
} catch (ex: Exception) {
96-
Analytics.segmentLog(
97-
"${ex.message}: failed to fetch settings",
98-
kind = LogFilterKind.ERROR
99-
)
100-
null
101-
}
89+
val settingsObj: Settings? = fetchSettings(writeKey, cdnHost)
10290

10391
withContext(analyticsDispatcher) {
10492
settingsObj?.let {
@@ -112,4 +100,21 @@ suspend fun Analytics.checkSettings() {
112100
store.dispatch(System.ToggleRunningAction(running = true), System::class)
113101
}
114102
}
103+
}
104+
105+
internal fun Analytics.fetchSettings(
106+
writeKey: String,
107+
cdnHost: String
108+
): Settings? = try {
109+
val connection = HTTPClient(writeKey).settings(cdnHost)
110+
val settingsString =
111+
connection.inputStream?.bufferedReader()?.use(BufferedReader::readText) ?: ""
112+
log("Fetched Settings: $settingsString")
113+
LenientJson.decodeFromString(settingsString)
114+
} catch (ex: Exception) {
115+
Analytics.segmentLog(
116+
"${ex.message}: failed to fetch settings",
117+
kind = LogFilterKind.ERROR
118+
)
119+
null
115120
}

‎core/src/test/kotlin/com/segment/analytics/kotlin/core/SettingsTests.kt‎

Lines changed: 118 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,7 @@ import kotlinx.coroutines.test.runTest
1313
import kotlinx.serialization.Serializable
1414
import kotlinx.serialization.json.buildJsonObject
1515
import kotlinx.serialization.json.put
16-
import org.junit.jupiter.api.Assertions.assertEquals
17-
import org.junit.jupiter.api.Assertions.assertFalse
18-
import org.junit.jupiter.api.Assertions.assertNotNull
19-
import org.junit.jupiter.api.Assertions.assertTrue
16+
import org.junit.jupiter.api.Assertions.*
2017
import org.junit.jupiter.api.BeforeEach
2118
import org.junit.jupiter.api.Test
2219
import java.util.concurrent.atomic.AtomicInteger
@@ -178,4 +175,121 @@ class SettingsTests {
178175
analytics.track("track", buildJsonObject { put("direct", true) })
179176
assertEquals(1, eventCounter.get())
180177
}
178+
179+
@Test
180+
fun `fetchSettings returns null when Settings string is invalid`() {
181+
// Null on invalid JSON
182+
mockHTTPClient("")
183+
var settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1")
184+
assertNull(settings)
185+
186+
// Null on invalid JSON
187+
mockHTTPClient("hello")
188+
settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1")
189+
assertNull(settings)
190+
191+
// Null on invalid JSON
192+
mockHTTPClient("#! /bin/sh")
193+
settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1")
194+
assertNull(settings)
195+
196+
// Null on invalid JSON
197+
mockHTTPClient("<!DOCTYPE html>")
198+
settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1")
199+
assertNull(settings)
200+
201+
// Null on invalid JSON
202+
mockHTTPClient("true")
203+
settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1")
204+
assertNull(settings)
205+
206+
// Null on invalid JSON
207+
mockHTTPClient("[]")
208+
settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1")
209+
assertNull(settings)
210+
211+
// Null on invalid JSON
212+
mockHTTPClient("}{")
213+
settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1")
214+
assertNull(settings)
215+
216+
// Null on invalid JSON
217+
mockHTTPClient("{{{{}}}}")
218+
settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1")
219+
assertNull(settings)
220+
221+
// Null on invalid JSON
222+
mockHTTPClient("{null:\"bar\"}")
223+
settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1")
224+
assertNull(settings)
225+
}
226+
227+
@Test
228+
fun `fetchSettings returns null when parameters are invalid`() {
229+
mockHTTPClient("{\"integrations\":{}, \"plan\":{}, \"edgeFunction\": {}, \"middlewareSettings\": {}}")
230+
231+
// empty host
232+
var settings = analytics.fetchSettings("foo", "")
233+
assertNull(settings)
234+
235+
// not a host name
236+
settings = analytics.fetchSettings("foo", "http://blah")
237+
assertNull(settings)
238+
239+
// emoji
240+
settings = analytics.fetchSettings("foo", "😃")
241+
assertNull(settings)
242+
}
243+
244+
@Test
245+
fun `fetchSettings returns null when Settings string is null for known properties`() {
246+
// Null if integrations is null
247+
mockHTTPClient("{\"integrations\":null}")
248+
var settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1")
249+
assertNull(settings)
250+
251+
// Null if plan is null
252+
mockHTTPClient("{\"plan\":null}")
253+
settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1")
254+
assertNull(settings)
255+
256+
// Null if edgeFunction is null
257+
mockHTTPClient("{\"edgeFunction\":null}")
258+
settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1")
259+
assertNull(settings)
260+
261+
// Null if middlewareSettings is null
262+
mockHTTPClient("{\"middlewareSettings\":null}")
263+
settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1")
264+
assertNull(settings)
265+
}
266+
267+
@Test
268+
fun `known Settings property types must match json type`() {
269+
270+
// integrations must be a JSON object
271+
mockHTTPClient("{\"integrations\":{}}")
272+
var settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1")
273+
assertNotNull(settings)
274+
275+
// Null if integrations is a number
276+
mockHTTPClient("{\"integrations\":123}")
277+
settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1")
278+
assertNull(settings)
279+
280+
// Null if integrations is a string
281+
mockHTTPClient("{\"integrations\":\"foo\"}")
282+
settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1")
283+
assertNull(settings)
284+
285+
// Null if integrations is an array
286+
mockHTTPClient("{\"integrations\":[\"foo\"]}")
287+
settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1")
288+
assertNull(settings)
289+
290+
// Null if integrations is an emoji (UTF-8 string)
291+
mockHTTPClient("{\"integrations\": 😃}")
292+
settings = analytics.fetchSettings("foo", "cdn-settings.segment.com/v1")
293+
assertNull(settings)
294+
}
181295
}

‎core/src/test/kotlin/com/segment/analytics/kotlin/core/utils/Mocks.kt‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -42,13 +42,14 @@ fun spyStore(scope: TestScope, dispatcher: TestDispatcher): Store {
4242
every { store getProperty "updateQueue" } propertyType CoroutineContext::class returns dispatcher
4343
return store
4444
}
45+
val settingsDefault = """
46+
{"integrations":{"Segment.io":{"apiKey":"1vNgUqwJeCHmqgI9S1sOm9UHCyfYqbaQ"}},"plan":{},"edgeFunction":{}}
47+
""".trimIndent()
4548

46-
fun mockHTTPClient() {
49+
fun mockHTTPClient(settings: String = settingsDefault) {
4750
mockkConstructor(HTTPClient::class)
4851
val settingsStream = ByteArrayInputStream(
49-
"""
50-
{"integrations":{"Segment.io":{"apiKey":"1vNgUqwJeCHmqgI9S1sOm9UHCyfYqbaQ"}},"plan":{},"edgeFunction":{}}
51-
""".trimIndent().toByteArray()
52+
settings.toByteArray()
5253
)
5354
val httpConnection: HttpURLConnection = mockk()
5455
val connection = object : Connection(httpConnection, settingsStream, null) {}

0 commit comments

Comments
 (0)