Fix crash bugs and harden security found in full-app scan#30
Merged
Conversation
Bugs: - QuickConnect: stop converting the server code to Int, which dropped leading zeros and displayed a code the server never accepts; format it defensively (old code crashed on codes shorter than 3 chars) - QuickConnect: wrap the polling coroutine in try/catch so a network error or expired secret shows "Unavailable" instead of crashing the app; stop polling once authentication succeeds and guard against duplicate polling loops on fragment recreation - onSetRating: reject non-HeartRating ratings instead of crashing with ClassCastException (reachable by any connected controller) - onSetMediaItems: coerce indexOfFirst() == -1 to 0 so a stale cache can't produce an invalid start index - AlbumArtContentProvider: use ConcurrentHashMap for the URI map, which is written from session threads and read from binder threads - DashTuneMusicService: cancel serviceScope in onDestroy to stop leaked coroutines; parse cache_size/bitrate prefs with toLongOrNull/ toIntOrNull instead of crashing on malformed values - JellyfinAccountManager: null-safe server comparison in storeAccount (getUserData can return null) - CredentialsFragment: null-safe InputMethodManager lookup after the suspend login call Security: - Disable android:allowBackup so the cached library DB and preferences cannot be extracted via device backup Co-Authored-By: Claude Fable 5 <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Full scan of the app (all Kotlin sources, manifest, build config) for bugs and security issues. This PR fixes the confirmed, high-confidence findings; lower-severity observations that were deliberately left unchanged are listed at the bottom.
Bug fixes
QuickConnect sign-in (
SignInViewModel,CredentialsFragment)Integer.valueOf(response.content.code)converted the server's string code to an Int, so012345was shown as123 45— a code the server never accepts. The code now stays aStringend to end (verified against the SDK:QuickConnectResult.codeisString).code.substring(0, 3)threwStringIndexOutOfBoundsExceptionfor codes under 3 characters. Formatting is now length-safe (formatQuickConnectCode, covered by new unit tests).viewModelScopeand killed the process. It now catches, records to Crashlytics, and shows "Unavailable".startQuickConnectis idempotent while a poll is active.Media session (
DashTuneSessionCallback)onSetRatingcrashed on non-heart ratings. The blindrating as HeartRatingcast threwClassCastExceptionfor any connected controller sending a differentRatingsubtype (the service is exported, as AAOS requires). It now returnsERROR_NOT_SUPPORTED.onSetMediaItemscould produce start index -1. The non-audiobook single-item path didn't coerceindexOfFirst()like the audiobook path does; a stale cache mismatch would yield an invalid start index.Playback service (
DashTuneMusicService)serviceScopewas never cancelled inonDestroy, leaking any in-flight coroutines (position reports, sync) past service destruction.onCreate.cache_sizewas parsed withtoLong()andbitratewithtoInt(); garbage in prefs (e.g. from a bad restore) would crash the service at startup. NowtoLongOrNull()/toIntOrNull()with sane fallbacks (also fixed inMediaItemFactory.streamingUri).Others
AlbumArtContentProvider.uriMapis written from media-session threads and read from binder threads with no synchronization — now aConcurrentHashMap.JellyfinAccountManager.storeAccountcalled.equals()ongetUserData(...), which can return null → NPE. Now a null-safe comparison.CredentialsFragmentdid an uncheckedas InputMethodManagercast on a nullable lookup after a suspend call — NPE if the activity is gone by then.Security hardening
android:allowBackup="false". The auth token lives inAccountManager(not backed up), but the Room library cache, playlist history, and server details were extractable via device backup. Nothing in the backup is worth restoring, so backup is disabled outright.Reviewed, deliberately not changed
android:usesCleartextTraffic="true"— required for HTTP Jellyfin servers on LANs; removing it would break common setups.getUniversalAudioStreamUrladds noApiKeyparam; auth is header-only), so the download index retains no credentials after logout.Authenticator.getAuthTokenreturns the account "password" (always"") as the token — effectively dead code since nothing calls it; changing it to hand out the real token would widen exposure, so it was left alone.Testing
./gradlew :automotive:testDebugUnitTest— all existing tests pass, plus new tests for QuickConnect code formatting (leading zeros, short codes, odd lengths).🤖 Generated with Claude Code