Refactor unzipToTempDir with security checks and limits - #75
Conversation
Fixed path traversal and zip bomb crashes I left loose limits because I don't know the exact range of the skins. Please review the proposal and correct what is necessary.
📝 WalkthroughWalkthroughThe ChangesZIP Extraction Security Hardening
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
🧹 Nitpick comments (2)
app/src/main/java/org/nqmgaming/aneko/presentation/AnekoViewModel.kt (2)
463-513: 💤 Low valueIndentation of the new companion object and
unzipToTempDiris inconsistent with the rest of the class.The new block is indented 4 spaces less than surrounding class members (compare with
parseSkinMetadata/moveSkinFilesToFinalDestination). It still parses as long as the braces match, but it makes the file harder to scan and breaks the existing convention. Reflow to match the class body's 4-space member indent.🤖 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 - 513, The companion object and unzipToTempDir block are mis-indented relative to the class members; reflow the code so the private companion object (with MAX_ZIP_ENTRIES / MAX_ZIP_BYTES) and the fun unzipToTempDir(...) are indented the same 4-space level as other class members (e.g., parseSkinMetadata and moveSkinFilesToFinalDestination), ensuring braces and inner lines keep their existing relative indentation.
487-493: 💤 Low value
name.contains("..")is overly strict and redundant with the canonical-path check.
contains("..")rejects legitimate filenames such ascat..frame1.pngorv1..2.png. The real defense —outFile.canonicalPath.startsWith(canonicalBase)on line 492 — already prevents zip-slip regardless of how..appears in the name. Either drop the..substring check or restrict it to per-component equality (split('/', '\\').any { it == ".." }). The leading/and\checks are fine to keep as a quick reject for absolute paths.🛠️ Proposed fix (component-wise check)
- if (name.startsWith("/") || name.startsWith("\\") - || name.contains("..")) - throw SecurityException() + val parts = name.split('/', '\\') + if (name.startsWith("/") || name.startsWith("\\") || parts.any { it == ".." }) + throw SecurityException("Suspicious zip entry name: $name")🤖 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 487 - 493, The check that rejects any name containing ".." is too strict; update the validation around the variable name in the file (the block that creates outFile and compares outFile.canonicalPath to canonicalBase in AnekoViewModel) to either remove the name.contains("..") check entirely or replace it with a per-path-component check such as splitting on '/' and '\' and rejecting only if any component == ".."; keep the existing leading '/' and '\' checks and retain the outFile.canonicalPath.startsWith(canonicalBase) security check to prevent zip-slip.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@app/src/main/java/org/nqmgaming/aneko/presentation/AnekoViewModel.kt`:
- Around line 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.
- Around line 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.
---
Nitpick comments:
In `@app/src/main/java/org/nqmgaming/aneko/presentation/AnekoViewModel.kt`:
- Around line 463-513: The companion object and unzipToTempDir block are
mis-indented relative to the class members; reflow the code so the private
companion object (with MAX_ZIP_ENTRIES / MAX_ZIP_BYTES) and the fun
unzipToTempDir(...) are indented the same 4-space level as other class members
(e.g., parseSkinMetadata and moveSkinFilesToFinalDestination), ensuring braces
and inner lines keep their existing relative indentation.
- Around line 487-493: The check that rejects any name containing ".." is too
strict; update the validation around the variable name in the file (the block
that creates outFile and compares outFile.canonicalPath to canonicalBase in
AnekoViewModel) to either remove the name.contains("..") check entirely or
replace it with a per-path-component check such as splitting on '/' and '\' and
rejecting only if any component == ".."; keep the existing leading '/' and '\'
checks and retain the outFile.canonicalPath.startsWith(canonicalBase) security
check to prevent zip-slip.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8c13c23a-28ea-4228-8706-e710a2d8f994
📒 Files selected for processing (1)
app/src/main/java/org/nqmgaming/aneko/presentation/AnekoViewModel.kt
| private companion object { | ||
| const val MAX_ZIP_ENTRIES = 200 | ||
| const val MAX_ZIP_BYTES = 50L * 1024 * 1024 | ||
| } |
There was a problem hiding this comment.
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.
| 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.
| 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() |
There was a problem hiding this comment.
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.
| 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.
1 participant
Fixed path traversal and zip bomb crashes
I left loose limits because I don't know the exact range of the skins. Please review the proposal and correct what is necessary.
Summary by CodeRabbit