Fix security vulnerabilities and bugs#28
Closed
chamika wants to merge 1 commit into
Closed
Conversation
Security: - Stop logging stream/album-art URLs that carry the Jellyfin access token (api_key); log track ids / paths instead. Release builds don't strip logs. - Restrict privileged session commands (LOGIN, SYNC) to our own package in onConnect; the media session is exported, so any app could previously trigger them. Repeat/shuffle remain available to all controllers. - Harden AlbumArtContentProvider: ConcurrentHashMap for the static uri map (was a plain map mutated from many threads) and a canonical-path containment check as defense-in-depth against traversal. - Disable allowBackup so cached library metadata / prefs aren't included in device backups. Bugs: - QuickConnect code is now carried as a String end-to-end. Parsing it to an Int dropped leading zeros (user saw/typed the wrong code) and could crash with StringIndexOutOfBoundsException on short values. - Wrap QuickConnect init/polling in try/catch and make the auth response null-safe; a transient error or missing field previously crashed the coroutine. - Clamp the single-item start index in onSetMediaItems with coerceAtLeast(0) to avoid an IllegalArgumentException when the selected id isn't in the resolved list. - Authenticator.getAuthToken now returns the real token via peekAuthToken (TOKEN_TYPE) instead of the intentionally-empty account password. - AlbumArtContentProvider latch waiters check file.exists() after the await before opening, instead of throwing a raw FileNotFoundException on failed downloads. Tests: - Update AuthenticatorTest to the corrected token-retrieval behavior. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Owner
Author
|
Closing as #30 addresses the same issues |
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.
Addresses security findings and functional bugs found during a code scan. All 291 unit tests pass.
Security
DashTuneMusicService) and album-art remote URLs (logged inAlbumArtContentProvider) embed the Jellyfin access token as anapi_keyquery param. Release builds don't strip logs (isMinifyEnabled = false), so the token was recoverable via logcat. Now logs track ids / URL paths only.onConnectunconditionally addedLOGIN_COMMANDandSYNC_COMMANDto every controller. Since the service isexported="true", any installed app could trigger login/sync. These two commands are now restricted to our own package; repeat/shuffle stay available to all controllers (needed by system UI).AlbumArtContentProvider.uriMapwas a plain map mutated from many binder/coroutine threads whileclearCacheiterated it (CME / lost entries). Switched toConcurrentHashMapand added a canonical-path containment check as defense-in-depth against traversal.allowBackupdisabled — prevents cached library metadata / SharedPreferences from being included in device/cloud backups.Bugs
Int, which dropped leading zeros (user shown/typing the wrong code → auth fails) and could throwStringIndexOutOfBoundsExceptionin formatting. Now carried as aStringend-to-end with length-safe formatting.!!crashing the coroutine on a transient error or missing field.onSetMediaItems— the non-audiobook single-item path could pass index-1toMediaItemsWithStartPosition. AddedcoerceAtLeast(0)(matching the audiobook branch).Authenticator.getAuthTokenreturned the wrong token — it read the (intentionally empty) account password instead of the token. Now returns the real token viapeekAuthToken(TOKEN_TYPE).file.exists()after the await instead of opening a possibly-missing file.Notes
usesCleartextTraffic="true") was intentionally left enabled. Self-hosted Jellyfin servers are commonly reached over plain HTTP on a LAN; disabling cleartext would break that core use case. The risk (credentials/token interception onhttp://servers) is a deliberate product tradeoff worth a follow-up (e.g. a network-security-config scoped to trusted hosts, or an HTTPS-preferred warning at sign-in).Testing
./gradlew :automotive:testDebugUnitTest— 291 passed.AuthenticatorTestupdated to the corrected token-retrieval behavior.🤖 Generated with Claude Code