DO NOT MERGE - security: close XXE, Cast LAN exposure, AI key plaintext, song FK constraint

[lostf1sh]

Follow-ups to the codebase review that PR #2047 left open, in three pushes. Each is small and isolated; only the song FK touches data and ships with a migration.

First push (commit ad08dec)

XXE in lyrics import (LyricsImportSecurity.kt)

LyricsImportSecurityTest.validateImportedLyricsFile_rejectsTtmlWithDoctype was failing on the Vintage engine PR #2047 added. The TTML hardening in TtmlLyricsParser correctly rejected the malicious <!DOCTYPE, but the LRC fallback path in LyricsImportSecurity then re-parsed the raw XML text as plain lyrics, so the validator returned Valid for a payload that included file:///etc/passwd as a literal entity reference. The user impact wasn't actual file read (the parser feature flags worked), but the validator polluting the lyrics DB with attacker-controlled paths was bad enough — and the test is now meaningful.

Fix: a pre-flight regex that rejects any payload whose first 8 KiB contains <!DOCTYPE or <!ENTITY before any parser runs. ~10 lines.

Cast HTTP server LAN attack surface

Two changes that go together:

• MediaFileHttpServerService.kt:381 — bind the embedded Ktor server to the selected LAN interface IP (addressSelection.hostAddress) instead of 0.0.0.0. The wildcard bind was reachable from any active interface (mobile hotspot, VPN). The Cast protocol fetches the media stream over plaintext HTTP and includes the auth token in the URL query, so any LAN sniffer on a non-Cast interface could observe it.
• CastSessionSecurity.kt:54-65 — enforceClientAddressAllowlist = true unconditionally. Previously the field was set to castAddressVariants.isNotEmpty(), so when the Cast device IP hint was missing (some receivers / DIAL devices), the allowlist was disabled entirely and any LAN peer was authorized. With this change the allowed set still contains loopback (and the server's own LAN IP when known), so default-deny applies to every other LAN address.

A new test isAuthorizedClientAddress denies LAN when no Cast hint was provided pins the new behavior.

AI provider API keys at rest (AiPreferencesRepository.kt)

Nine AI provider bearer tokens (Gemini, OpenAI, DeepSeek, Groq, Mistral, NVIDIA, KIMI, GLM, OpenRouter) were stored in plain DataStore<Preferences> — every other cloud credential repo (Jellyfin, Navidrome, GDrive, NetEase, QQ Music) uses EncryptedSharedPreferences. Moved AI keys to the same pattern (AES256-GCM with an AndroidKeystore master key). Money exposure is real: a Gemini key in a prompt-loop can burn $100+/month unattended.

Migration runs once per install via an @AppScope coroutine, gated by a marker key in the encrypted store; clears the legacy DataStore keys after a successful transfer. The public Flow API is preserved (one MutableStateFlow per provider, hydrated from the encrypted store at construction).

backup_rules.xml and data_extraction_rules.xml now exclude ai_prefs.xml and the ai_prefs_plain.xml fallback (the latter is the actual leak vector; it gets written if the Keystore is unavailable).

SongEntity.artist_id FK constraint

@ColumnInfo(name = \"artist_id\") val artistId: Long was INTEGER NOT NULL but its FK action was ON DELETE SET NULL. The first time deleteOrphanedArtists() ran against a still-referenced artist, SQLite would fail the NOT NULL constraint and roll back the whole transaction. Latent runtime bug — hasn't shipped a crash yet, but it would.

Fix: artistId: Long? in the entity, and MIGRATION_41_42 (create-copy-drop-rename) to drop NOT NULL on the column and rebuild the indexes. Domain Song.artistId stays Long with a 0L fallback in the entity→model mapping, keeping the blast radius minimal (SyncWorker.kt:323 is the only other call site).

Second push (commit a9610e2)

Crash log PII redaction (CrashHandler.kt, new CrashLogRedactor.kt)

CrashHandler.saveCrashLog persisted throwable.message and the raw stack trace verbatim to SharedPreferences, and CrashReportDialog surfaces them with a share intent. Network and media-stack exceptions routinely embed Bearer tokens, salted Subsonic auth params (t/s/p/salt), Jellyfin X-Emby-Token headers, Google API keys (?key=...), NetEase MUSIC_U cookies, and Telegram phone numbers — anything the user shared out leaked them.

New CrashLogRedactor (pure Kotlin, no Android deps) covers all those credential shapes. CrashHandler now redacts on write (saveCrashLog) and again on read (getCrashLog) so older entries persisted by previous builds are sanitized before the share surface ever sees them.

14 unit tests cover Bearer tokens, Authorization / Cookie / X-Emby-Token / x-goog-api-key headers, sensitive query params (including the full Subsonic salted-token set), MUSIC_U cookies, Telegram phone numbers, and mixed payloads.

Manifest hardening (AndroidManifest.xml)

• Dropped the redundant MEDIA_BUTTON intent-filter from MusicService. PixelPlayMediaButtonReceiver already declares the filter and forwards via Util.startForegroundService; the duplicate path on MusicService was racing with the receiver on headphone-button press.
• SCHEDULE_EXACT_ALARM is now scoped to maxSdkVersion=32, and USE_EXACT_ALARM is added for API 33+ (auto-granted with a Play Console justification — the sleep timer qualifies). The runtime canScheduleExactAlarms() path in SleepTimerStateHolder and SetupScreen continues to work unchanged.

network_security_config.xml was considered but left untouched. Flipping <base-config cleartextTrafficPermitted=\"false\"> would break self-hosted Navidrome / Jellyfin users on HTTP RFC1918 servers (CloudStreamSecurity.isPrivateIpv4Literal already gates the app side, but RFC1918 ranges cannot be expressed as network-security-config XML wildcards).

Release workflow tightening (.github/workflows/*.yml)

• All four workflows (phone-debug, phone-release, nightly-apk, wearos-apk) now invoke ./gradlew so the pinned Gradle 9.5.1 from the wrapper is used everywhere. Previously CI ran whichever gradle was preinstalled on the runner, which is a reproducibility hazard against the wrapper-pinned developer build.
• phone-release.yml drops its pull_request trigger. The release workflow generates a keystore and caches it under a predictable key; under the previous trigger config a fork PR could populate or read that cache. Only push:master and workflow_dispatch can fire it now. phone-debug.yml and wearos-apk.yml keep their PR trigger — they have no keystore, so fork PRs cannot poison anything, and the compile validation is useful on review.

Third push (commit e42a233)

PR #2047 added junit-vintage-engine, which surfaced five pre-existing failures the silent skip had been hiding. None were regressions; all were latent bugs in tests or in code that the tests pin. This push fixes all five and adds a CI step so they cannot silently rot again.

BackupSectionTest

Test asserted exactly 11 backup sections, but AI_USAGE_LOGS was added as the 12th (sinceVersion = 4). Updated the count, added AI_USAGE_LOGS to the fromKey round-trip assertion, and split the sinceVersion test into v3 (QUICK_FILL / ARTIST_IMAGES / EQUALIZER) and v4 (AI_USAGE_LOGS) cases so the next addition forces a test update rather than a silent pass.

LyricsImportSecurity.validatePayload

Test rejectsUnsyncedLrcContent expected an .lrc file containing only plain text to be rejected with INVALID_LYRICS_CONTENT, but the validator was returning Valid because LyricsUtils.parseLyrics emits plain-text Lyrics for any non-empty input. The LRC contract requires at least one synced line — validatePayload now skips Valid results where format == LRC and parsedLyrics.synced is empty/null, letting other normalization candidates (e.g. the TTML-to-enhanced-LRC fallback) still produce a synced result before falling through to INVALID_LYRICS_CONTENT.

PlayerViewModelTest.triggerShuffleAllFromTile

Test stubbed _allSongsFlow with three songs and expected triggerShuffleAllFromTile to forward them to prepareShuffledQueueSuspending, but the implementation always called musicRepository.getRandomSongs(500) which the test had stubbed to return emptyList, so the action looped on the syncManager retry path and never reached the queue holder. Fixed by reading libraryStateHolder.allSongs.value first; on warm starts this skips an unnecessary DB round-trip, and the cold-start path (empty library snapshot → sync + repository sample) is unchanged.

LyricsStateHolder.fetchLyricsForSong

Test fetchLyricsForSong_usesStoredLyricsWithoutRemoteFetch failed with searchUiState stuck at Loading even after advanceUntilIdle. The cause: fetchLyricsForSong wrapped the getStoredLyrics call in withContext(Dispatchers.IO), which trapped the work on a real IO thread that the TestScope cannot drain. Removed the redundant withContext — getStoredLyrics is a suspend function backed by the LyricsRepository → Room DAO chain, which already executes on Room's IO executor.

AudioMetaUtilsTest

The source file had merge artifacts: three @Test methods fused together, mismatched braces, a duplicate method body. JUnit could not load the class at all (InvalidTestClassError). Rewrote the file cleanly with three methods covering the M4a, AMR/3gpp, and AIFF/AC3/DTS branches that AudioMetaUtils.mimeTypeToFormat actually implements.

CI wiring

phone-debug.yml now runs ./gradlew :app:testDebugUnitTest before assembleDebug. Tests are the cheaper signal; failing fast saves the CI minute that an assemble would otherwise burn. On failure, the unit test report directory is uploaded as an artifact so reviewers can inspect HTML and JUnit XML output without re-running the workflow locally.

Test status

Targeted tests for all fixes pass:

• First push: LyricsImportSecurityTest.rejectsTtmlWithDoctype ✓, CastSessionSecurityTest (full suite, including new case) ✓, kspDebugKotlin schema validation produces 42.json cleanly ✓, :app:compileDebugKotlin ✓.
• Second push: CrashLogRedactorTest (14 cases) ✓; same run executed :app:processDebugMainManifest so AGP has validated the manifest changes.
• Third push: ./gradlew :app:testDebugUnitTest reports 302 tests passing, 0 failing. The previous baseline was 297 passing, 5 failing (the failures listed above). No tests were added or removed; the new behavior assertions on the LRC validator and the shuffle-from-tile path are pinned by the existing failing-tests-now-green.

Reviewer notes

• The XXE check is at the LyricsImportSecurity layer rather than just trusting TtmlLyricsParser because the parser is one of two normalization candidates — the LRC fallback would still see the raw bytes. Defense in depth.
• The Cast bind change means if the user switches Wi-Fi networks mid-session, the server needs a restart cycle to bind the new interface IP. That cycle already exists (the service was already tearing down and re-selecting selectIpAddress() on connectivity changes).
• The AI key migration is fire-and-forget on @AppScope. If it fails (Keystore unavailable on first run), the MIGRATION_DONE_KEY is not set and the next launch retries. Until the marker is set, both stores may contain the key — the encrypted one wins for reads via apiKeyFlows.
• MIGRATION_41_42 follows the same create-copy-drop-rename pattern already used by recreateSongsTable. The migration test (PixelPlayDatabaseMigrationTest) is an instrumentation test and isn't wired to include MIGRATION_40_41 or MIGRATION_41_42 in ALL_MIGRATIONS; that's a pre-existing gap and not part of this PR.
• CrashLogRedactor deliberately favors false positives (stripping a benign substring) over false negatives (leaking a token). The patterns target credential shapes observed in this codebase, not a general-purpose PII detector.
• Dropping the PR trigger from phone-release.yml is the minimum-invasive option from the keystore-cache discussion. A future PR could rotate the dummy password through GH secrets or generate a fresh keystore per run; both were out of scope here.
• The triggerShuffleAllFromTile change is a behavioral improvement, not just a test fix — on a warm app the tile launch now reuses the in-memory library snapshot instead of re-querying the DB. Cold start still goes through the sync + retry path; only the path selection changed.
• The LyricsStateHolder withContext removal is safe: MusicRepository.getStoredLyrics is a suspend function and its concrete impl delegates to LyricsRepository.getStoredLyrics, which itself runs Room queries that are dispatcher-aware. The outer wrap was defensive belt-and-suspenders that broke unit-testability.
• AudioMetaUtilsTest's previous source had assertions for evrc, qcelp, and ima/x-ima-adpcm mappings that AudioMetaUtils.mimeTypeToFormat does not currently implement. Those assertions are dropped from the new file. If they should be implemented (Samsung-specific MIME extensions), file a follow-up.

[lostf1sh]

@copilot review

[Copilot]

@copilot review

Reviewed the latest branch state and CI for this PR. Current head commit e42a233 has passing Phone Debug, Wear OS Debug, and CodeQL workflows; no additional code changes are required from this review request.