Skip to content

Refactor unzipToTempDir with security checks and limits #75

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
HGBits wants to merge 1 commit into from
Closed

Refactor unzipToTempDir with security checks and limits #75

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 38 additions & 13 deletions app/src/main/java/org/nqmgaming/aneko/presentation/AnekoViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,32 +460,57 @@ class AnekoViewModel @Inject constructor(
return null
}


fun unzipToTempDir(input: InputStream, tempDir: File): List<File> {
val zis = ZipInputStream(BufferedInputStream(input))
val buffer = ByteArray(DEFAULT_BUFFER_SIZE)
val extractedFiles = mutableListOf<File>()

private companion object {
const val MAX_ZIP_ENTRIES = 200
const val MAX_ZIP_BYTES = 50L * 1024 * 1024
}
Comment on lines +463 to +466

@coderabbitai coderabbitai Bot May 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Critical: duplicate companion object in the same class — won't compile.

AnekoViewModel already declares a companion object at lines 46–52 (PREF_KEY_THEME, PREF_KEY_ACCENT, …). Kotlin allows at most one companion object per class, so adding this second private companion object at line 463 produces a compile error (Only one companion object is allowed per class). Merge the new constants into the existing companion object instead.

🛠️ Proposed fix 
     companion object {
         const val PREF_KEY_THEME = "theme"
         const val PREF_KEY_ACCENT = "accent_color"
         const val PREF_KEY_DYNAMIC_COLOR = "dynamic_color"
         const val PREF_KEY_FINISHED_SETUP = "finished_setup"
         const val PREF_KEY_KNOWN_SKINS = "known_skins"
+
+        private const val MAX_ZIP_ENTRIES = 200
+        private const val MAX_ZIP_BYTES = 50L * 1024 * 1024
     }

And remove lines 463–466.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private companion object {
const val MAX_ZIP_ENTRIES = 200
const val MAX_ZIP_BYTES = 50L * 1024 * 1024
}
companion object {
const val PREF_KEY_THEME = "theme"
const val PREF_KEY_ACCENT = "accent_color"
const val PREF_KEY_DYNAMIC_COLOR = "dynamic_color"
const val PREF_KEY_FINISHED_SETUP = "finished_setup"
const val PREF_KEY_KNOWN_SKINS = "known_skins"
private const val MAX_ZIP_ENTRIES = 200
private const val MAX_ZIP_BYTES = 50L * 1024 * 1024
}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/src/main/java/org/nqmgaming/aneko/presentation/AnekoViewModel.kt` around
lines 463 - 466, AnekoViewModel currently has a second private companion object
declaring MAX_ZIP_ENTRIES and MAX_ZIP_BYTES which causes a compile error; move
these two constants into the existing companion object that contains
PREF_KEY_THEME / PREF_KEY_ACCENT (merge MAX_ZIP_ENTRIES and MAX_ZIP_BYTES into
that companion), then delete the redundant private companion object so only one
companion object remains in the AnekoViewModel class.


fun unzipToTempDir(input: InputStream, tempDir: File): List<File> {
val canonicalBase = tempDir.canonicalPath + File.separator
val buffer = ByteArray(DEFAULT_BUFFER_SIZE)
val extracted = mutableListOf<File>()
var entryCount = 0
var totalBytes = 0L

ZipInputStream(BufferedInputStream(input)).use { zis ->
var entry = zis.nextEntry
while (entry != null) {
if (!entry.isDirectory && !entry.name.startsWith("__MACOSX") && !entry.name.endsWith(".DS_Store")) {
val outFile = File(tempDir, entry.name)
if (++entryCount > MAX_ZIP_ENTRIES)
throw SecurityException()

val name = entry.name
val skip = entry.isDirectory
|| name.startsWith("__MACOSX")
|| name.endsWith(".DS_Store")

if (!skip) {
if (name.startsWith("/") || name.startsWith("\\")
|| name.contains(".."))
throw SecurityException()

val outFile = File(tempDir, name)
if (!outFile.canonicalPath.startsWith(canonicalBase))
throw SecurityException()

outFile.parentFile?.mkdirs()

BufferedOutputStream(FileOutputStream(outFile)).use { bos ->
var count: Int
while (zis.read(buffer).also { count = it } != -1) {
totalBytes += count
if (totalBytes > MAX_ZIP_BYTES)
throw SecurityException()
Comment on lines +478 to +502

@coderabbitai coderabbitai Bot May 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Throw SecurityException with a descriptive message.

All four throw SecurityException() sites are message-less, so when extraction aborts there's nothing in logs/Timber to indicate which guard fired (entry-count cap, suspicious name, zip-slip, or byte cap). The outer catch (e: Exception) in importSkinFromStream (line 403) only calls e.printStackTrace() and returns null, which silently surfaces as a generic import failure to the user. Add a reason to each throw so failures are diagnosable and so the catch site can map them to a clearer Toast.

🛠️ Proposed fix 
-            if (++entryCount > MAX_ZIP_ENTRIES)
-                throw SecurityException()
+            if (++entryCount > MAX_ZIP_ENTRIES)
+                throw SecurityException("ZIP exceeds max entries ($MAX_ZIP_ENTRIES)")
@@
-                if (name.startsWith("/") || name.startsWith("\\")
-                        || name.contains(".."))
-                    throw SecurityException()
+                if (name.startsWith("/") || name.startsWith("\\")
+                        || name.contains(".."))
+                    throw SecurityException("Suspicious zip entry name: $name")
@@
-                if (!outFile.canonicalPath.startsWith(canonicalBase))
-                    throw SecurityException()
+                if (!outFile.canonicalPath.startsWith(canonicalBase))
+                    throw SecurityException("Zip-slip detected for entry: $name")
@@
-                        if (totalBytes > MAX_ZIP_BYTES)
-                            throw SecurityException()
+                        if (totalBytes > MAX_ZIP_BYTES)
+                            throw SecurityException("ZIP exceeds max uncompressed size ($MAX_ZIP_BYTES bytes)")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (++entryCount > MAX_ZIP_ENTRIES)
throw SecurityException()
val name = entry.name
val skip = entry.isDirectory
|| name.startsWith("__MACOSX")
|| name.endsWith(".DS_Store")
if (!skip) {
if (name.startsWith("/") || name.startsWith("\\")
|| name.contains(".."))
throw SecurityException()
val outFile = File(tempDir, name)
if (!outFile.canonicalPath.startsWith(canonicalBase))
throw SecurityException()
outFile.parentFile?.mkdirs()
BufferedOutputStream(FileOutputStream(outFile)).use { bos ->
var count: Int
while (zis.read(buffer).also { count = it } != -1) {
totalBytes += count
if (totalBytes > MAX_ZIP_BYTES)
throw SecurityException()
if (++entryCount > MAX_ZIP_ENTRIES)
throw SecurityException("ZIP exceeds max entries ($MAX_ZIP_ENTRIES)")
val name = entry.name
val skip = entry.isDirectory
|| name.startsWith("__MACOSX")
|| name.endsWith(".DS_Store")
if (!skip) {
if (name.startsWith("/") || name.startsWith("\\")
|| name.contains(".."))
throw SecurityException("Suspicious zip entry name: $name")
val outFile = File(tempDir, name)
if (!outFile.canonicalPath.startsWith(canonicalBase))
throw SecurityException("Zip-slip detected for entry: $name")
outFile.parentFile?.mkdirs()
BufferedOutputStream(FileOutputStream(outFile)).use { bos ->
var count: Int
while (zis.read(buffer).also { count = it } != -1) {
totalBytes += count
if (totalBytes > MAX_ZIP_BYTES)
throw SecurityException("ZIP exceeds max uncompressed size ($MAX_ZIP_BYTES bytes)")
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/src/main/java/org/nqmgaming/aneko/presentation/AnekoViewModel.kt` around
lines 478 - 502, Update the four message-less throws of SecurityException inside
the ZIP extraction loop in AnekoViewModel (the block that checks entryCount vs
MAX_ZIP_ENTRIES, the name validation using entry.name, the zip-slip check using
outFile.canonicalPath vs canonicalBase, and the byte cap check using totalBytes
vs MAX_ZIP_BYTES) to include clear descriptive messages (e.g. "zip entry limit
exceeded", "invalid/skipped entry name", "zip-slip detected", "zip byte limit
exceeded"); ensure these messages are created where entryCount, name,
outFile.canonicalPath and totalBytes are validated so the outer
importSkinFromStream catch can inspect e.message and map specific failures to
user-facing Toasts or better log entries.

bos.write(buffer, 0, count)
}
}
extractedFiles.add(outFile)
extracted.add(outFile)
}
zis.closeEntry()
entry = zis.nextEntry
}

zis.close()
return extractedFiles
}
return extracted
}

fun moveSkinFilesToFinalDestination(sourceDir: File, destDir: File, overwrite: Boolean) {
sourceDir.walkTopDown().forEach { file ->
Expand Down Expand Up @@ -625,4 +650,4 @@ class AnekoViewModel @Inject constructor(
it.copy(newAvailableSkins = emptyList())
}
}
}
}
Loading