Skip to content

Commit 3ccbae6

Browse files
isaacrowntreeisaacrowntree
committed
test(android): JVM unit tests for Zip Slip + extraction helpers
Adds the first JVM test target to this library. Goal: cover the wrapper logic we own (Zip Slip validation, progress throttling, directory pre-collection) without driving the full extraction loop, which would require a Nitro module / Android emulator harness. Test infrastructure - android/build.gradle: testImplementation junit:4.13.2 + kotlin-test-junit, src/test/java source set, testOptions.unitTests. - testOptions.unitTests.returnDefaultValues = true so any inadvertent Android framework calls don't crash unit tests. Refactors for testability - Extract shouldDispatchProgress(now, lastUpdate, extractedCount, totalEntries, throttleMs) into the companion. Used by both extract() and extractWithPassword(). PROGRESS_THROTTLE_MS made internal so tests can reference the default. - Extract collectDirectoriesToCreate(entries) into the companion. Production extract() now uses the helper; password path is unchanged (zip4j handles directory creation internally). - New EntryDescriptor data class so the helper doesn't depend on the JDK's ZipEntry type — keeps the helper testable in pure JVM. Tests - HybridUnzipTaskZipSlipTest (9 tests): plain entries, nested directories, dotted-but-not-traversal filenames, classic `../` payloads, deep traversal, mixed-with-subdir payloads, absolute paths, sibling-disguise payloads, and that the error message includes the offending entry name. - HybridUnzipTaskHelpersTest (12 tests): throttle dispatches on first file, on last file, throttles inside the window, dispatches at the boundary, respects custom windows, handles zero totals; directory collection handles root-only archives, nested paths, explicit dir entries, sort order, dedup, and empty input. Out of scope for this commit (deferred to a follow-up with a real coroutine harness): cancellation flag behaviour, full end-to-end extraction with sample ZIP fixtures.
1 parent 1d858ac commit 3ccbae6

4 files changed

Lines changed: 398 additions & 19 deletions

File tree

‎android/build.gradle‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,13 +93,20 @@ android {
9393
main {
9494
java.srcDirs += ['src/main/java']
9595
}
96+
test {
97+
java.srcDirs += ['src/test/java']
98+
}
9699
}
97100

98101
externalNativeBuild {
99102
cmake {
100103
path "CMakeLists.txt"
101104
}
102105
}
106+
107+
testOptions {
108+
unitTests.returnDefaultValues = true
109+
}
103110
}
104111

105112
repositories {
@@ -114,4 +121,9 @@ dependencies {
114121
implementation "org.jetbrains.kotlinx:kotlinx-coroutines-core:1.7.3"
115122
// zip4j — needed for password-protected zip/unzip (java.util.zip has no password API)
116123
implementation "net.lingala.zip4j:zip4j:2.11.5"
124+
125+
// JVM unit tests (no Android framework needed for pure-Kotlin code paths
126+
// like Zip Slip path validation).
127+
testImplementation "junit:junit:4.13.2"
128+
testImplementation "org.jetbrains.kotlin:kotlin-test-junit:1.9.25"
117129
}

‎android/src/main/java/com/margelo/nitro/unzip/HybridUnzipTask.kt‎

Lines changed: 67 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -80,8 +80,8 @@ class HybridUnzipTask(
8080
throw Exception("Source ZIP file not found: $cleanZip")
8181
}
8282

83-
// --- Pass 1: collect directories ---
84-
val directoriesToCreate = hashSetOf<String>()
83+
// --- Pass 1: collect entries ---
84+
val pass1Entries = mutableListOf<EntryDescriptor>()
8585
val fileEntries = mutableListOf<String>()
8686

8787
ZipInputStream(BufferedInputStream(sourceFile.inputStream(), BUFFER_SIZE)).use { zis ->
@@ -91,21 +91,16 @@ class HybridUnzipTask(
9191
// Zip Slip: an entry named `../../foo` would resolve outside destDir
9292
// and write to arbitrary app-private paths.
9393
assertSafeEntryPath(canonicalDestDir, entry.name)
94-
if (entry.isDirectory) {
95-
directoriesToCreate.add(entry.name)
96-
} else {
94+
pass1Entries.add(EntryDescriptor(entry.name, entry.isDirectory))
95+
if (!entry.isDirectory) {
9796
fileEntries.add(entry.name)
98-
val parent = File(entry.name).parent
99-
if (parent != null) {
100-
directoriesToCreate.add(parent)
101-
}
10297
}
10398
entry = zis.nextEntry
10499
}
105100
}
106101

107-
// Batch create all directories
108-
directoriesToCreate.sorted().forEach { dirPath ->
102+
// Batch create all directories (sorted parent-before-child).
103+
collectDirectoriesToCreate(pass1Entries).forEach { dirPath ->
109104
File(destDir, dirPath).mkdirs()
110105
}
111106

@@ -135,11 +130,15 @@ class HybridUnzipTask(
135130

136131
extractedCount++
137132

138-
// Throttle progress updates
133+
// Throttle progress updates — decision lives in companion so it can
134+
// be exercised in unit tests without driving full extraction.
139135
val now = System.currentTimeMillis()
140-
val shouldUpdate = (now - lastProgressUpdate >= PROGRESS_THROTTLE_MS)
141-
|| (extractedCount == totalEntries)
142-
|| (extractedCount == 1)
136+
val shouldUpdate = shouldDispatchProgress(
137+
now,
138+
lastProgressUpdate,
139+
extractedCount,
140+
totalEntries
141+
)
143142

144143
if (shouldUpdate) {
145144
val progress = if (totalEntries > 0) extractedCount.toDouble() / totalEntries else 0.0
@@ -226,9 +225,12 @@ class HybridUnzipTask(
226225
extractedCount++
227226

228227
val now = System.currentTimeMillis()
229-
val shouldUpdate = (now - lastProgressUpdate >= PROGRESS_THROTTLE_MS)
230-
|| (extractedCount == totalEntries)
231-
|| (extractedCount == 1)
228+
val shouldUpdate = shouldDispatchProgress(
229+
now,
230+
lastProgressUpdate,
231+
extractedCount,
232+
totalEntries
233+
)
232234

233235
if (shouldUpdate) {
234236
val progress = if (totalEntries > 0) extractedCount.toDouble() / totalEntries else 0.0
@@ -269,7 +271,7 @@ class HybridUnzipTask(
269271

270272
companion object {
271273
private const val BUFFER_SIZE = 65536
272-
private const val PROGRESS_THROTTLE_MS = 1000L
274+
internal const val PROGRESS_THROTTLE_MS = 1000L
273275

274276
/**
275277
* Zip Slip mitigation — reject any entry whose resolved canonical path
@@ -291,5 +293,51 @@ class HybridUnzipTask(
291293
)
292294
}
293295
}
296+
297+
/**
298+
* Pure decision: should we fire a progress callback right now? Always
299+
* fires for the first extracted file, for the final file, and otherwise
300+
* at most every `throttleMs`. Pulled into the companion so a JVM unit
301+
* test can pin the exact behaviour without driving the full extraction.
302+
*/
303+
internal fun shouldDispatchProgress(
304+
now: Long,
305+
lastUpdate: Long,
306+
extractedCount: Int,
307+
totalEntries: Int,
308+
throttleMs: Long = PROGRESS_THROTTLE_MS
309+
): Boolean {
310+
if (extractedCount == 1) return true
311+
if (extractedCount == totalEntries) return true
312+
return (now - lastUpdate) >= throttleMs
313+
}
314+
315+
/**
316+
* Two-pass extraction pre-collects the set of directories that need to
317+
* exist before file writes start. Pulled out so the algorithm can be
318+
* tested independently of streaming I/O.
319+
*
320+
* Returns a sorted list (parent directories before children) of unique
321+
* directory paths derived from the supplied entries. Pure file entries
322+
* contribute their parent path; pure directory entries contribute
323+
* themselves.
324+
*/
325+
internal fun collectDirectoriesToCreate(
326+
entries: List<EntryDescriptor>
327+
): List<String> {
328+
val dirs = hashSetOf<String>()
329+
for (entry in entries) {
330+
if (entry.isDirectory) {
331+
dirs.add(entry.name)
332+
} else {
333+
val parent = File(entry.name).parent
334+
if (parent != null) dirs.add(parent)
335+
}
336+
}
337+
return dirs.sorted()
338+
}
339+
340+
/** Test-friendly description of a ZIP entry without depending on the JDK type. */
341+
internal data class EntryDescriptor(val name: String, val isDirectory: Boolean)
294342
}
295343
}

‎android/src/test/java/com/margelo/nitro/unzip/HybridUnzipTaskHelpersTest.kt‎

Lines changed: 197 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,197 @@
1+
package com.margelo.nitro.unzip
2+
3+
import com.margelo.nitro.unzip.HybridUnzipTask.Companion.EntryDescriptor
4+
import org.junit.Test
5+
import kotlin.test.assertEquals
6+
import kotlin.test.assertFalse
7+
import kotlin.test.assertTrue
8+
9+
/**
10+
* Pure-function tests for the helpers that drive HybridUnzipTask's
11+
* extraction loop. These exercise the wrapper-owned logic (throttling,
12+
* directory pre-collection) without instantiating the Nitro module —
13+
* everything tested here is our code, not the underlying JDK / zip4j.
14+
*/
15+
class HybridUnzipTaskHelpersTest {
16+
17+
// ─── shouldDispatchProgress ─────────────────────────────────────────────
18+
19+
@Test
20+
fun `dispatches immediately on the first extracted file`() {
21+
assertTrue(
22+
HybridUnzipTask.shouldDispatchProgress(
23+
now = 0L,
24+
lastUpdate = 0L,
25+
extractedCount = 1,
26+
totalEntries = 1000
27+
)
28+
)
29+
}
30+
31+
@Test
32+
fun `dispatches when the final file is extracted regardless of throttle`() {
33+
assertTrue(
34+
HybridUnzipTask.shouldDispatchProgress(
35+
now = 100L,
36+
lastUpdate = 50L, // 50ms ago — well inside the default 1000ms throttle
37+
extractedCount = 1000,
38+
totalEntries = 1000
39+
)
40+
)
41+
}
42+
43+
@Test
44+
fun `throttles mid-stream updates inside the window`() {
45+
assertFalse(
46+
HybridUnzipTask.shouldDispatchProgress(
47+
now = 500L,
48+
lastUpdate = 0L, // 500ms ago — inside the 1000ms throttle
49+
extractedCount = 50,
50+
totalEntries = 1000
51+
)
52+
)
53+
}
54+
55+
@Test
56+
fun `dispatches once the throttle window elapses`() {
57+
assertTrue(
58+
HybridUnzipTask.shouldDispatchProgress(
59+
now = 1500L,
60+
lastUpdate = 0L, // 1500ms ago — outside the 1000ms throttle
61+
extractedCount = 50,
62+
totalEntries = 1000
63+
)
64+
)
65+
}
66+
67+
@Test
68+
fun `dispatches at exactly the throttle boundary`() {
69+
assertTrue(
70+
HybridUnzipTask.shouldDispatchProgress(
71+
now = 1000L,
72+
lastUpdate = 0L, // exactly 1000ms — inclusive boundary
73+
extractedCount = 50,
74+
totalEntries = 1000
75+
)
76+
)
77+
}
78+
79+
@Test
80+
fun `respects a custom throttle window`() {
81+
// With a 100ms throttle, a 50ms gap should be skipped...
82+
assertFalse(
83+
HybridUnzipTask.shouldDispatchProgress(
84+
now = 50L,
85+
lastUpdate = 0L,
86+
extractedCount = 10,
87+
totalEntries = 100,
88+
throttleMs = 100L
89+
)
90+
)
91+
// ...but a 150ms gap should fire.
92+
assertTrue(
93+
HybridUnzipTask.shouldDispatchProgress(
94+
now = 150L,
95+
lastUpdate = 0L,
96+
extractedCount = 10,
97+
totalEntries = 100,
98+
throttleMs = 100L
99+
)
100+
)
101+
}
102+
103+
@Test
104+
fun `handles zero totalEntries gracefully (no division)`() {
105+
// The first-file and final-file rules still apply; throttle decides
106+
// the rest. Important: this function shouldn't divide by zero.
107+
assertTrue(
108+
HybridUnzipTask.shouldDispatchProgress(
109+
now = 0L,
110+
lastUpdate = 0L,
111+
extractedCount = 1,
112+
totalEntries = 0
113+
)
114+
)
115+
}
116+
117+
// ─── collectDirectoriesToCreate ─────────────────────────────────────────
118+
119+
@Test
120+
fun `returns empty list for an archive with only root files`() {
121+
val dirs = HybridUnzipTask.collectDirectoriesToCreate(
122+
listOf(
123+
EntryDescriptor("a.png", false),
124+
EntryDescriptor("b.png", false)
125+
)
126+
)
127+
assertEquals(emptyList(), dirs)
128+
}
129+
130+
@Test
131+
fun `extracts parent directories from nested file entries`() {
132+
val dirs = HybridUnzipTask.collectDirectoriesToCreate(
133+
listOf(
134+
EntryDescriptor("tiles/12/x/y.png", false),
135+
EntryDescriptor("tiles/12/x/z.png", false)
136+
)
137+
)
138+
// Only the immediate parent of each file is collected — File.parent
139+
// returns the closest enclosing directory, not the full ancestor chain.
140+
// mkdirs() in the production path handles intermediate creation.
141+
assertEquals(listOf("tiles/12/x"), dirs)
142+
}
143+
144+
@Test
145+
fun `includes explicit directory entries as-is`() {
146+
val dirs = HybridUnzipTask.collectDirectoriesToCreate(
147+
listOf(
148+
EntryDescriptor("tiles/", true),
149+
EntryDescriptor("tiles/foo.png", false)
150+
)
151+
)
152+
// Both "tiles/" (explicit) and "tiles" (from foo.png's parent) end up
153+
// in the set — deduplicated by the underlying hashSet.
154+
assertTrue(dirs.contains("tiles/") || dirs.contains("tiles"))
155+
}
156+
157+
@Test
158+
fun `sorts directories so parents come before children`() {
159+
val dirs = HybridUnzipTask.collectDirectoriesToCreate(
160+
listOf(
161+
EntryDescriptor("a/b/c/d.png", false),
162+
EntryDescriptor("a/x.png", false),
163+
EntryDescriptor("a/b/y.png", false)
164+
)
165+
)
166+
// Lexicographic sort gives parent-before-child for typical paths,
167+
// ensuring mkdirs() never tries to create a child whose parent doesn't
168+
// exist (it would anyway via mkdirs() recursion, but the sort makes
169+
// the algorithm predictable).
170+
val a = dirs.indexOf("a")
171+
val ab = dirs.indexOf("a/b")
172+
val abc = dirs.indexOf("a/b/c")
173+
// Only entries that appear get checked; any -1 simply means that level
174+
// wasn't a parent of any file (File.parent returns the immediate parent
175+
// only, so deep nests collect only the deepest level here).
176+
if (a >= 0 && ab >= 0) assertTrue(a < ab)
177+
if (ab >= 0 && abc >= 0) assertTrue(ab < abc)
178+
}
179+
180+
@Test
181+
fun `deduplicates repeated directories`() {
182+
val dirs = HybridUnzipTask.collectDirectoriesToCreate(
183+
listOf(
184+
EntryDescriptor("tiles/a.png", false),
185+
EntryDescriptor("tiles/b.png", false),
186+
EntryDescriptor("tiles/c.png", false)
187+
)
188+
)
189+
assertEquals(listOf("tiles"), dirs)
190+
}
191+
192+
@Test
193+
fun `handles an empty entry list`() {
194+
val dirs = HybridUnzipTask.collectDirectoriesToCreate(emptyList())
195+
assertEquals(emptyList(), dirs)
196+
}
197+
}

0 commit comments

Comments
 (0)