feat: implement APK signing fingerprint verification and improve Shiz…#334
Merged
rainxchzed merged 8 commits intomainfrom Mar 17, 2026
Merged
feat: implement APK signing fingerprint verification and improve Shiz…#334rainxchzed merged 8 commits intomainfrom
rainxchzed merged 8 commits intomainfrom
Conversation
…uku integration - Add `signingFingerprint` field to `InstalledAppEntity`, `InstalledApp`, and `ApkPackageInfo` to track app authenticity - Implement fingerprint verification during the installation process to prevent updates with mismatched signing keys - Enhance `AndroidInstallerInfoExtractor` to extract SHA-256 signing certificates for both modern (API 28+) and legacy Android versions - Refactor `DetailsViewModel` to decouple download and installation logic into distinct, manageable methods - Update `InstalledAppsRepository` and `PackageEventReceiver` to persist and verify signing fingerprints during updates - Improve Shizuku service management by moving `ShizukuStatus` to a dedicated model and refining lifecycle handling - Clean up `AutoUpdateWorker` and `UpdateScheduler` logic, including better notification handling and permission checks - Apply consistent code formatting and remove redundant comments across modified files
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds SHA‑256 APK signing-fingerprint extraction and persistence; threads Changes
Sequence DiagramsequenceDiagram
participant User as UI/User
participant VM as DetailsViewModel
participant Net as Network/FS
participant APK as ApkExtractor
participant Repo as DetailsRepository / InstalledAppsRepository
User->>VM: Request install/update(assetUrl)
VM->>Net: Download or use cached file
Net-->>VM: filePath
VM->>APK: extract ApkPackageInfo (package, version, signingFingerprint)
APK-->>VM: ApkPackageInfo
VM->>Repo: fetch storedFingerprint for package
Repo-->>VM: storedFingerprint
alt fingerprints match or none stored
VM->>Repo: save InstalledApp(with signingFingerprint)
Repo-->>VM: ack
VM->>User: emit Installed/Updated state
else mismatch detected
VM->>User: emit SigningKeyWarning(expected, actual)
User->>VM: OnOverrideSigningKeyWarning / OnDismissSigningKeyWarning
alt Override
VM->>Repo: save InstalledApp(override fingerprint)
Repo-->>VM: ack
VM->>User: emit Installed/Updated state
else Dismiss
VM->>User: emit Failed/Canceled state
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip You can disable the changed files summary in the walkthrough.Disable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AutoUpdateWorker.kt (1)
153-168:⚠️ Potential issue | 🟠 MajorExtracted fingerprint is not verified or propagated.
The APK info extraction at line 153-155 includes
signingFingerprint, but it's neither:
- Verified against
currentApp.signingFingerprintto prevent updates with mismatched signing keys (per PR objectives)- Passed to the
updateApp()callThis appears to be a gap in implementing the stated PR goal of "fingerprint verification during installation to prevent updates with mismatched signing keys."
🔧 Suggested fix: Add verification and propagation
val apkInfo = installer.getApkInfoExtractor().extractPackageInfo(filePath) ?: throw IllegalStateException("Failed to extract APK info for ${app.appName}") val currentApp = installedAppsRepository.getAppByPackage(app.packageName) if (currentApp != null) { + // Verify signing fingerprint hasn't changed (prevent supply chain attacks) + if (currentApp.signingFingerprint != null && + apkInfo.signingFingerprint != null && + currentApp.signingFingerprint != apkInfo.signingFingerprint) { + throw SecurityException( + "Signing fingerprint mismatch for ${app.appName}. " + + "Expected: ${currentApp.signingFingerprint}, " + + "Got: ${apkInfo.signingFingerprint}" + ) + } + installedAppsRepository.updateApp( currentApp.copy( isPendingInstall = true, latestVersion = latestVersion, latestAssetName = assetName, latestAssetUrl = assetUrl, latestVersionName = apkInfo.versionName, latestVersionCode = apkInfo.versionCode, + signingFingerprint = apkInfo.signingFingerprint ?: currentApp.signingFingerprint, ), ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AutoUpdateWorker.kt` around lines 153 - 168, The extracted apkInfo.signingFingerprint must be checked and propagated: after extracting apkInfo via installer.getApkInfoExtractor().extractPackageInfo, verify that if currentApp exists its signing fingerprint (currentApp.signingFingerprint) matches apkInfo.signingFingerprint and throw an IllegalStateException (or abort) on mismatch to prevent installing updates signed with a different key; then include apkInfo.signingFingerprint in the installedAppsRepository.updateApp(...) call by adding the appropriate field to currentApp.copy(...) (e.g., latestSigningFingerprint or signingFingerprint field) so the new fingerprint is stored alongside latestVersion/latestVersionCode.core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/InstalledAppsRepositoryImpl.kt (1)
181-188:⚠️ Potential issue | 🟠 MajorDon't erase the last verified fingerprint on a null update.
Because
signingFingerprintis nullable, writing it through unconditionally will wipe a previously verified signer when extraction returnsnull. That permanently disables future mismatch checks for that app.♻️ Suggested fix
- signingFingerprint = signingFingerprint, + signingFingerprint = signingFingerprint ?: app.signingFingerprint,Also applies to: 210-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/InstalledAppsRepositoryImpl.kt` around lines 181 - 188, In InstalledAppsRepositoryImpl's updateAppVersion method, don't overwrite an existing verified signingFingerprint with null: only persist signingFingerprint when the incoming signingFingerprint is non-null; if it's null, leave the stored fingerprint unchanged. Apply this same null-check patch to the other update method/variant in the file (the second updateAppVersion overload referenced around lines 210-225) so both implementations preserve an existing verified signer when the incoming value is null.
🧹 Nitpick comments (2)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AndroidInstallerInfoExtractor.kt (1)
76-90: Redundant APK parsing on legacy path.The legacy branch calls
getPackageArchiveInfo()again withGET_SIGNATURES, but the initial parsing at lines 27-36 already used a flags combination. Consider extracting signatures from the existingpackageInfoto avoid parsing the APK twice.♻️ Potential optimization
} else { `@Suppress`("DEPRECATION") - val legacyInfo = - packageManager.getPackageArchiveInfo( - filePath, - PackageManager.GET_SIGNATURES, - ) - `@Suppress`("DEPRECATION") - legacyInfo?.signatures?.firstOrNull()?.toByteArray()?.let { certBytes -> + packageInfo.signatures?.firstOrNull()?.toByteArray()?.let { certBytes -> MessageDigest .getInstance("SHA-256") .digest(certBytes) .joinToString(":") { "%02X".format(it) } } }Note: This requires adding
GET_SIGNATURESto the initial flags for API < 28. Verify this doesn't cause issues on older devices.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AndroidInstallerInfoExtractor.kt` around lines 76 - 90, The legacy branch redundantly reparses the APK with packageManager.getPackageArchiveInfo; instead include PackageManager.GET_SIGNATURES in the original flags used to populate packageInfo for API < 28 and extract signatures from the existing packageInfo.signatures (avoid calling getPackageArchiveInfo a second time). Update the initial flags computation (used when filling packageInfo) to add GET_SIGNATURES for pre-28 paths, and change the legacy branch to read packageInfo?.signatures?.firstOrNull()?.toByteArray() (or equivalent) and compute the SHA-256 digest, keeping the existing MessageDigest logic intact.core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/PackageEventReceiver.kt (1)
89-98: Consider verifying fingerprint consistency for pending installs.The receiver passes
app.signingFingerprint(the stored fingerprint) toupdateAppVersionwithout extracting and comparing against the newly installed package's actual fingerprint. While this works if verification occurred during download, consider adding a consistency check here for defense-in-depth, especially since this broadcast can trigger from external package managers.💡 Optional verification approach
if (wasActuallyUpdated) { + // Optional: Verify fingerprint hasn't changed + val newFingerprint = systemInfo.signingFingerprint // if available from monitor + if (app.signingFingerprint != null && newFingerprint != null && + app.signingFingerprint != newFingerprint) { + Logger.w { "Fingerprint mismatch detected for $packageName" } + } repo.updateAppVersion(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/PackageEventReceiver.kt` around lines 89 - 98, The update path in PackageEventReceiver currently passes the stored app.signingFingerprint into repo.updateAppVersion without verifying it against the freshly installed package; modify PackageEventReceiver so that when wasActuallyUpdated is true you extract the installed package's actual signing fingerprint (via the Android PackageManager/PackageInfo/SigningInfo APIs), compare it to app.signingFingerprint, and if they differ either abort or mark the update as unverified and log a warning; then call updateAppVersion with the actual fingerprint (or a flag/marker if mismatched) instead of blindly reusing app.signingFingerprint to ensure fingerprint consistency for pending installs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/local/db/entities/InstalledAppEntity.kt`:
- Line 26: MIGRATION_3_4 currently only creates cache_entries and omits adding
the new column declared on InstalledAppEntity (signingFingerprint), causing
schema mismatch; update the migration named MIGRATION_3_4 to execute "ALTER
TABLE installed_apps ADD COLUMN signingFingerprint TEXT" (or the appropriate SQL
for nullable String) as part of the migration steps and increment/export the
Room schema for version 4 so the version 4 schema file reflects this new column.
In
`@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.kt`:
- Around line 856-866: The success message building in AppsViewModel (the
_events.send call creating AppsEvent.ShowSuccess) currently nests the
skipped/failed logic so a non-zero failed count is omitted whenever
result.skipped > 0; change the message construction to include skipped and
failed independently (e.g., build a list of suffix parts or append ", X skipped"
and ", Y failed" conditionally) so both ", N skipped" and ", M failed" appear
when applicable while still showing "Imported ${result.imported}".
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt`:
- Around line 1043-1055: The current flow lets signer verification fail open
because when installer.getApkInfoExtractor().extractPackageInfo(filePath)
returns null you only log and continue, and checkFingerprints() treats a null
new fingerprint as success; change this so that when apkInfo is null the code
fails the verification/install path (unless the asset is explicitly a known
non-APK type—handle that with an explicit guard), and update checkFingerprints()
(or its callers) to treat a null new fingerprint as a verification failure
rather than success; locate the apk extraction and ApkPackageInfo creation and
ensure you return/throw an error or set a failed state when apkInfo == null, and
modify checkFingerprints()/fingerprint comparison logic to require a non-null
new fingerprint before considering verification successful (also apply the same
fix at the other occurrence mentioned around the check at lines 1113-1115).
- Around line 1056-1073: The fingerprint-mismatch path in the onFailure of
checkFingerprints returns while leaving UI state indicating an active install;
update the onFailure block (where apkInfo is handled and checkFingerprints is
called) to clear/reset the install state before returning—e.g., set
downloadStage back from INSTALLING to the idle/finished state and clear
currentAssetName (and any related install flags) prior to calling appendLog and
returning so the UI no longer shows an active install after failure.
- Line 62: The code imports and uses java.io.File in DetailsViewModel (import at
top and usage around the code that checks file existence/size near line 1158),
which is JVM-only and must be removed from commonMain; replace that direct File
access with an abstraction: either introduce an expect/actual API (e.g., expect
fun getFileSizeOrExists(path: String): Long? or expect class FileInfo { fun
exists(): Boolean; fun length(): Long }) with platform-specific implementations
in androidMain/jvmMain, or extend/use the existing Downloader interface to
provide methods like downloader.fileExists(path) / downloader.fileSize(path),
then update DetailsViewModel to call the abstracted methods instead of
java.io.File so no JVM types remain in commonMain.
- Around line 1015-1029: The catch block in DetailsViewModel's install job is
catching Throwable and treating cancellations as errors; change the handler to
re-throw coroutine cancellations by checking for
kotlin.coroutines.CancellationException (and
java.util.concurrent.CancellationException if used) and rethrowing it
immediately before logging/setting _state, so that true cancellations do not set
installError, reset currentAssetName, or call appendLog for an error.
---
Outside diff comments:
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AutoUpdateWorker.kt`:
- Around line 153-168: The extracted apkInfo.signingFingerprint must be checked
and propagated: after extracting apkInfo via
installer.getApkInfoExtractor().extractPackageInfo, verify that if currentApp
exists its signing fingerprint (currentApp.signingFingerprint) matches
apkInfo.signingFingerprint and throw an IllegalStateException (or abort) on
mismatch to prevent installing updates signed with a different key; then include
apkInfo.signingFingerprint in the installedAppsRepository.updateApp(...) call by
adding the appropriate field to currentApp.copy(...) (e.g.,
latestSigningFingerprint or signingFingerprint field) so the new fingerprint is
stored alongside latestVersion/latestVersionCode.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/InstalledAppsRepositoryImpl.kt`:
- Around line 181-188: In InstalledAppsRepositoryImpl's updateAppVersion method,
don't overwrite an existing verified signingFingerprint with null: only persist
signingFingerprint when the incoming signingFingerprint is non-null; if it's
null, leave the stored fingerprint unchanged. Apply this same null-check patch
to the other update method/variant in the file (the second updateAppVersion
overload referenced around lines 210-225) so both implementations preserve an
existing verified signer when the incoming value is null.
---
Nitpick comments:
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AndroidInstallerInfoExtractor.kt`:
- Around line 76-90: The legacy branch redundantly reparses the APK with
packageManager.getPackageArchiveInfo; instead include
PackageManager.GET_SIGNATURES in the original flags used to populate packageInfo
for API < 28 and extract signatures from the existing packageInfo.signatures
(avoid calling getPackageArchiveInfo a second time). Update the initial flags
computation (used when filling packageInfo) to add GET_SIGNATURES for pre-28
paths, and change the legacy branch to read
packageInfo?.signatures?.firstOrNull()?.toByteArray() (or equivalent) and
compute the SHA-256 digest, keeping the existing MessageDigest logic intact.
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/PackageEventReceiver.kt`:
- Around line 89-98: The update path in PackageEventReceiver currently passes
the stored app.signingFingerprint into repo.updateAppVersion without verifying
it against the freshly installed package; modify PackageEventReceiver so that
when wasActuallyUpdated is true you extract the installed package's actual
signing fingerprint (via the Android PackageManager/PackageInfo/SigningInfo
APIs), compare it to app.signingFingerprint, and if they differ either abort or
mark the update as unverified and log a warning; then call updateAppVersion with
the actual fingerprint (or a flag/marker if mismatched) instead of blindly
reusing app.signingFingerprint to ensure fingerprint consistency for pending
installs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3b8b5a8e-9c81-40e2-b6b5-c9bca2079d4b
📒 Files selected for processing (16)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AndroidInstallerInfoExtractor.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AutoUpdateWorker.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/PackageEventReceiver.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/UpdateScheduler.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/AndroidInstallerStatusProvider.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuServiceManager.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/model/ShizukuStatus.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/local/db/entities/InstalledAppEntity.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/mappers/InstalledAppsMappers.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/InstalledAppsRepositoryImpl.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/ApkPackageInfo.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/InstalledApp.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/repository/InstalledAppsRepository.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/use_cases/SyncInstalledAppsUseCase.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt
💤 Files with no reviewable changes (1)
- core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/use_cases/SyncInstalledAppsUseCase.kt
| val latestAssetSize: Long?, | ||
| val appName: String, | ||
| val installSource: InstallSource, | ||
| val signingFingerprint: String?, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd 'AppDatabase.kt$' core/data -x sed -n '1,220p' {}
rg -n -C3 'signingFingerprint|installed_apps|AutoMigration|Migration\(' core/dataRepository: OpenHub-Store/GitHub-Store
Length of output: 19517
🏁 Script executed:
#!/bin/bash
# Read MIGRATION_3_4.kt to see exact SQL
cat -n core/data/src/androidMain/kotlin/zed/rainxch/core/data/local/db/migrations/MIGRATION_3_4.kt
# Check for version 4 schema file
ls -la core/data/schemas/zed.rainxch.core.data.local.db.AppDatabase/ | grep -E "\.json"
# Check if migration is registered in any database initialization
rg -n "MIGRATION_3_4|addMigrations" core/dataRepository: OpenHub-Store/GitHub-Store
Length of output: 1403
Add the missing migration step to add signingFingerprint column to installed_apps table.
MIGRATION_3_4 only creates the cache_entries table but does not add the signingFingerprint column to installed_apps. The entity defines this column, but users upgrading from version 3 will not have it in their database, causing read/write failures. Add ALTER TABLE installed_apps ADD COLUMN signingFingerprint TEXT to the migration, and ensure the version 4 schema file is exported.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/local/db/entities/InstalledAppEntity.kt`
at line 26, MIGRATION_3_4 currently only creates cache_entries and omits adding
the new column declared on InstalledAppEntity (signingFingerprint), causing
schema mismatch; update the migration named MIGRATION_3_4 to execute "ALTER
TABLE installed_apps ADD COLUMN signingFingerprint TEXT" (or the appropriate SQL
for nullable String) as part of the migration steps and increment/export the
Room schema for version 4 so the version 4 schema file reflects this new column.
feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.kt
Show resolved
Hide resolved
| import zed.rainxch.githubstore.core.presentation.res.rate_limit_exceeded | ||
| import zed.rainxch.githubstore.core.presentation.res.removed_from_favourites | ||
| import zed.rainxch.githubstore.core.presentation.res.translation_failed | ||
| import java.io.File |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'import java\.io\.File|File\(' feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.ktRepository: OpenHub-Store/GitHub-Store
Length of output: 155
Move JVM file access out of commonMain.
java.io.File is JVM-only and cannot be used in the commonMain source set. Push file existence/size checks behind an expect/actual abstraction in commonMain with platform-specific implementations in androidMain/jvmMain, or delegate to the Downloader interface.
Affected lines: 62 (import), 1158 (usage).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt`
at line 62, The code imports and uses java.io.File in DetailsViewModel (import
at top and usage around the code that checks file existence/size near line
1158), which is JVM-only and must be removed from commonMain; replace that
direct File access with an abstraction: either introduce an expect/actual API
(e.g., expect fun getFileSizeOrExists(path: String): Long? or expect class
FileInfo { fun exists(): Boolean; fun length(): Long }) with platform-specific
implementations in androidMain/jvmMain, or extend/use the existing Downloader
interface to provide methods like downloader.fileExists(path) /
downloader.fileSize(path), then update DetailsViewModel to call the abstracted
methods instead of java.io.File so no JVM types remain in commonMain.
...ails/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt
Show resolved
Hide resolved
...ails/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt
Outdated
Show resolved
Hide resolved
Comment on lines
+1056
to
+1073
| apkInfo?.let { | ||
| val result = | ||
| checkFingerprints( | ||
| apkPackageInfo = apkInfo, | ||
| ) | ||
|
|
||
| result | ||
| .onFailure { | ||
| appendLog( | ||
| assetName = assetName, | ||
| size = sizeBytes, | ||
| tag = releaseTag, | ||
| result = LogResult.Downloaded, | ||
| result = Error("Fingerprints does not match!"), | ||
| ) | ||
|
|
||
| val ext = assetName.substringAfterLast('.', "").lowercase() | ||
| return | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Clear install state before returning on a fingerprint mismatch.
The inline onFailure { return } exits this function while downloadStage is still INSTALLING and currentAssetName is still set, so a rejected update leaves the UI looking like an active install.
♻️ Suggested fix
- result
- .onFailure {
+ result
+ .onFailure { error ->
+ _state.value =
+ _state.value.copy(
+ downloadStage = DownloadStage.IDLE,
+ installError = error.message,
+ )
+ currentAssetName = null
appendLog(
assetName = assetName,
size = sizeBytes,
tag = releaseTag,
- result = Error("Fingerprints does not match!"),
+ result = Error(error.message),
)
return
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt`
around lines 1056 - 1073, The fingerprint-mismatch path in the onFailure of
checkFingerprints returns while leaving UI state indicating an active install;
update the onFailure block (where apkInfo is handled and checkFingerprints is
called) to clear/reset the install state before returning—e.g., set
downloadStage back from INSTALLING to the idle/finished state and clear
currentAssetName (and any related install flags) prior to calling appendLog and
returning so the UI no longer shows an active install after failure.
- Add `SigningKeyWarning` model and update `DetailsState` to handle certificate mismatch scenarios - Implement a warning dialog in `DetailsRoot` that displays expected vs. actual fingerprints when a signing key change is detected - Add `OnDismissSigningKeyWarning` and `OnOverrideSigningKeyWarning` actions to `DetailsViewModel` to allow users to bypass the warning - Update `AutoUpdateWorker` to block automatic updates if the APK signing key has changed - Enhance `DetailsViewModel` to extract and verify APK package info/fingerprints before installation - Include the app's own SHA-256 fingerprint when saving its installation state in `GithubStoreApp` - Bump database version to 5 and add migration `MIGRATION_4_5` - Add localized string resources for signing key change titles, messages, and override actions - Minor cleanup of string concatenation logic in `AppsViewModel`
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AutoUpdateWorker.kt (1)
77-99:⚠️ Potential issue | 🟠 MajorRe-check Shizuku readiness inside the per-app loop and stop remaining updates when it drops.
Line 77 onward processes all apps even if Shizuku becomes unavailable mid-run, which conflicts with the intended graceful skip behavior and causes avoidable repeated failures.
🔧 Suggested fix
- appsWithUpdates.forEachIndexed { index, app -> + for ((index, app) in appsWithUpdates.withIndex()) { + shizukuServiceManager.refreshStatus() + val shizukuStillReady = shizukuServiceManager.status.value == ShizukuStatus.READY + if (!shizukuStillReady) { + Logger.w { + "AutoUpdateWorker: Shizuku became unavailable during run; skipping remaining updates" + } + failedApps.addAll(appsWithUpdates.drop(index).map { it.appName }) + break + } + setForeground( createForegroundInfo( "Updating ${app.appName}...", index + 1, appsWithUpdates.size, ), ) @@ - } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AutoUpdateWorker.kt` around lines 77 - 99, Re-check Shizuku readiness at the top of the per-app loop and abort remaining updates when it becomes unavailable: inside appsWithUpdates.forEachIndexed (before calling setForeground/updateApp) call the existing Shizuku readiness check (e.g. ShizukuManager.isReady() or add a small helper like isShizukuReady()) and if it returns false, log a message from AutoUpdateWorker and break out of the loop so no further updateApp calls are attempted; ensure you still clear the current app's pending flag via installedAppsRepository.updatePendingStatus(packageName, false) if needed and avoid calling updateApp when Shizuku is not ready.
🧹 Nitpick comments (2)
feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt (1)
1184-1202: Consider treating null new fingerprint as verification failure.
checkFingerprintsreturns success whenapkPackageInfo.signingFingerprintis null (line 1191). This means if fingerprint extraction fails for the new APK but package info extraction succeeds, verification passes. If an existing app already has a stored fingerprint, allowing an update with an unverifiable fingerprint weakens the TOFU guarantee.Consider returning failure when
existingApp.signingFingerprint != null && apkPackageInfo.signingFingerprint == null:♻️ Suggested fix
- if (apkPackageInfo.signingFingerprint == null) return Result.success(Unit) + if (apkPackageInfo.signingFingerprint == null) { + return if (existingApp.signingFingerprint != null) { + Result.failure(IllegalStateException("Cannot verify signing key for update")) + } else { + Result.success(Unit) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt` around lines 1184 - 1202, The current checkFingerprints allows success when apkPackageInfo.signingFingerprint is null even if an existingApp.signingFingerprint is present; change the logic in checkFingerprints so that if existingApp.signingFingerprint != null and apkPackageInfo.signingFingerprint == null you return a failure (e.g., Result.failure(IllegalStateException("Unable to verify new APK signing fingerprint"))) instead of Result.success(Unit), otherwise keep the existing equality check between existingApp.signingFingerprint and apkPackageInfo.signingFingerprint.core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AutoUpdateWorker.kt (1)
245-263: Move summary notification strings to resources for localization consistency.These user-facing strings are currently hardcoded in English; resource-backed strings will keep notifications consistent with app i18n behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AutoUpdateWorker.kt` around lines 245 - 263, The notification title and text in AutoUpdateWorker.kt are hardcoded English strings; move them into Android string resources and load localized variants instead. Replace the inline when blocks that build title and text (variables title and text using successfulApps and failedApps) with calls to context.getString(R.string.<...>) passing counts and formatted lists (or use getQuantityString for plural titles) and create corresponding entries in strings.xml for each case (success, failure, mixed) to support localization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/local/db/initDatabase.kt`:
- Line 9: Create a new Kotlin file defining the Room Migration object
MIGRATION_4_5 in package zed.rainxch.core.data.local.db.migrations that performs
the schema change to add the signingFingerprint column (e.g., execute an ALTER
TABLE <your_table> ADD COLUMN signingFingerprint TEXT NULL/DEFAULT NULL to avoid
breaking existing rows) and return the Migration(4,5) instance named
MIGRATION_4_5; then ensure this MIGRATION_4_5 object is referenced by the
existing migrations list (the same one initDatabase.kt imports) so the database
builder includes the 4->5 migration.
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AutoUpdateWorker.kt`:
- Around line 159-171: Auto-update currently only blocks when both
currentApp.signingFingerprint and apkInfo.signingFingerprint are non-null and
different, allowing updates when the installed app has a known fingerprint but
apkInfo.signingFingerprint is null; in AutoUpdateWorker, change the condition to
also treat a null apkInfo.signingFingerprint as a mismatch when
currentApp.signingFingerprint is non-null (i.e., if
currentApp.signingFingerprint != null and apkInfo.signingFingerprint == null,
log via Logger.e with the same message and throw the IllegalStateException to
block the update), ensuring the same descriptive message references app.appName,
currentApp.signingFingerprint and apkInfo.signingFingerprint.
---
Outside diff comments:
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AutoUpdateWorker.kt`:
- Around line 77-99: Re-check Shizuku readiness at the top of the per-app loop
and abort remaining updates when it becomes unavailable: inside
appsWithUpdates.forEachIndexed (before calling setForeground/updateApp) call the
existing Shizuku readiness check (e.g. ShizukuManager.isReady() or add a small
helper like isShizukuReady()) and if it returns false, log a message from
AutoUpdateWorker and break out of the loop so no further updateApp calls are
attempted; ensure you still clear the current app's pending flag via
installedAppsRepository.updatePendingStatus(packageName, false) if needed and
avoid calling updateApp when Shizuku is not ready.
---
Nitpick comments:
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AutoUpdateWorker.kt`:
- Around line 245-263: The notification title and text in AutoUpdateWorker.kt
are hardcoded English strings; move them into Android string resources and load
localized variants instead. Replace the inline when blocks that build title and
text (variables title and text using successfulApps and failedApps) with calls
to context.getString(R.string.<...>) passing counts and formatted lists (or use
getQuantityString for plural titles) and create corresponding entries in
strings.xml for each case (success, failure, mixed) to support localization.
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt`:
- Around line 1184-1202: The current checkFingerprints allows success when
apkPackageInfo.signingFingerprint is null even if an
existingApp.signingFingerprint is present; change the logic in checkFingerprints
so that if existingApp.signingFingerprint != null and
apkPackageInfo.signingFingerprint == null you return a failure (e.g.,
Result.failure(IllegalStateException("Unable to verify new APK signing
fingerprint"))) instead of Result.success(Unit), otherwise keep the existing
equality check between existingApp.signingFingerprint and
apkPackageInfo.signingFingerprint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6bf88a44-a6fc-4a9b-ad9c-8b6ee844cb45
📒 Files selected for processing (11)
composeApp/src/androidMain/kotlin/zed/rainxch/githubstore/app/GithubStoreApp.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/local/db/initDatabase.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AutoUpdateWorker.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/local/db/AppDatabase.ktcore/presentation/src/commonMain/composeResources/values/strings.xmlfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsAction.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsRoot.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsState.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/model/SigningKeyWarning.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.kt
| import zed.rainxch.core.data.local.db.migrations.MIGRATION_1_2 | ||
| import zed.rainxch.core.data.local.db.migrations.MIGRATION_2_3 | ||
| import zed.rainxch.core.data.local.db.migrations.MIGRATION_3_4 | ||
| import zed.rainxch.core.data.local.db.migrations.MIGRATION_4_5 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify MIGRATION_4_5 definition and SQL statement.
# Expect: Migration should add signingFingerprint as a nullable TEXT column.
fd -t f -e kt "MIGRATION" --exec rg -l "MIGRATION_4_5" {} \; | head -5
echo "---"
# Search for the migration definition
ast-grep --pattern $'val MIGRATION_4_5 = $_'Repository: OpenHub-Store/GitHub-Store
Length of output: 73
🏁 Script executed:
#!/bin/bash
# Find the migrations directory
fd -t d "migrations" --max-depth 5
echo "---"
# Look for any migration files (MIGRATION_*.kt)
fd -t f -e kt "MIGRATION_" --path "*migrations*" | head -20
echo "---"
# Search for MIGRATION_4_5 in all kotlin files
rg "MIGRATION_4_5" -t kt --max-count 10Repository: OpenHub-Store/GitHub-Store
Length of output: 373
🏁 Script executed:
#!/bin/bash
# Find migrations directory
find . -type d -name "migrations" 2>/dev/null
echo "---"
# Find all MIGRATION files
find . -type f -name "*MIGRATION*.kt" 2>/dev/null | head -20
echo "---"
# Search for MIGRATION_4_5 text in kotlin files
rg "MIGRATION_4_5" --type kotlin 2>/dev/null || rg "MIGRATION_4_5" 2>/dev/null | head -20
echo "---"
# Check what's in the migrations directory if it exists
ls -la core/data/src/androidMain/kotlin/zed/rainxch/core/data/local/db/migrations/ 2>/dev/null || echo "Path not found"Repository: OpenHub-Store/GitHub-Store
Length of output: 1000
MIGRATION_4_5 file is missing but referenced in the code.
The import at line 9 and the reference at line 23 use MIGRATION_4_5, but the file does not exist in core/data/src/androidMain/kotlin/zed/rainxch/core/data/local/db/migrations/. Only MIGRATION_1_2.kt, MIGRATION_2_3.kt, and MIGRATION_3_4.kt are present. Create the MIGRATION_4_5.kt file with the database schema changes for adding the signingFingerprint column.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/local/db/initDatabase.kt`
at line 9, Create a new Kotlin file defining the Room Migration object
MIGRATION_4_5 in package zed.rainxch.core.data.local.db.migrations that performs
the schema change to add the signingFingerprint column (e.g., execute an ALTER
TABLE <your_table> ADD COLUMN signingFingerprint TEXT NULL/DEFAULT NULL to avoid
breaking existing rows) and return the Migration(4,5) instance named
MIGRATION_4_5; then ensure this MIGRATION_4_5 object is referenced by the
existing migrations list (the same one initDatabase.kt imports) so the database
builder includes the 4->5 migration.
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AutoUpdateWorker.kt
Outdated
Show resolved
Hide resolved
- Add `signingFingerprint` column to the `installed_apps` table via Room migration 4 to 5 - Update `SystemPackageInfo` and `DeviceApp` domain models to include the signing fingerprint - Implement SHA-256 signing certificate extraction in `AndroidPackageMonitor` for both legacy and modern Android versions - Enhance `AutoUpdateWorker` to verify the APK's signing fingerprint against the installed version before proceeding with an update - Update `AppsRepositoryImpl` to store and propagate signing fingerprints when linking or importing apps - Refactor `ShizukuInstallerWrapper` and `AppsRepositoryImpl` for better code consistency and formatting
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AutoUpdateWorker.kt (1)
159-193:⚠️ Potential issue | 🔴 CriticalCritical: Update/install logic is incorrectly scoped inside the fingerprint verification block.
The entire
installedAppsRepository.updateApp()andinstaller.install()logic (lines 173-192) is inside theif (currentApp?.signingFingerprint != null)block. If an installed app has no stored fingerprint (first install, legacy app, or extraction failure), the APK is downloaded but never installed.The fingerprint check should only guard the verification, not the installation itself.
🐛 Proposed fix to restructure the logic
val currentApp = installedAppsRepository.getAppByPackage(app.packageName) if (currentApp?.signingFingerprint != null) { val expected = currentApp.signingFingerprint!!.trim().uppercase() val actual = apkInfo.signingFingerprint?.trim()?.uppercase() if (actual == null || expected != actual) { Logger.e { "AutoUpdateWorker: Signing key mismatch for ${app.appName}! " + "Expected: ${currentApp.signingFingerprint}, got: ${apkInfo.signingFingerprint}. " + "Skipping auto-update." } throw IllegalStateException( "Signing fingerprint verification failed for ${app.appName}, blocking auto-update", ) } - - installedAppsRepository.updateApp( - currentApp.copy( - isPendingInstall = true, - latestVersion = latestVersion, - latestAssetName = assetName, - latestAssetUrl = assetUrl, - latestVersionName = apkInfo.versionName, - latestVersionCode = apkInfo.versionCode, - ), - ) - - Logger.d { "AutoUpdateWorker: Installing ${app.appName} via Shizuku" } - try { - installer.install(filePath, ext) - } catch (e: Exception) { - installedAppsRepository.updatePendingStatus(app.packageName, false) - throw e - } - - Logger.d { "AutoUpdateWorker: Install command completed for ${app.appName}, waiting for system confirmation via broadcast" } } + + if (currentApp != null) { + installedAppsRepository.updateApp( + currentApp.copy( + isPendingInstall = true, + latestVersion = latestVersion, + latestAssetName = assetName, + latestAssetUrl = assetUrl, + latestVersionName = apkInfo.versionName, + latestVersionCode = apkInfo.versionCode, + ), + ) + } + + Logger.d { "AutoUpdateWorker: Installing ${app.appName} via Shizuku" } + try { + installer.install(filePath, ext) + } catch (e: Exception) { + if (currentApp != null) { + installedAppsRepository.updatePendingStatus(app.packageName, false) + } + throw e + } + + Logger.d { "AutoUpdateWorker: Install command completed for ${app.appName}, waiting for system confirmation via broadcast" } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AutoUpdateWorker.kt` around lines 159 - 193, The update/install logic is incorrectly nested inside the signing-fingerprint check: move the installedAppsRepository.updateApp(...), the installer.install(filePath, ext) try/catch (and the subsequent Logger.d waiting message and updatePendingStatus fallback) out of the if (currentApp?.signingFingerprint != null) block so that fingerprint verification only controls the throw on mismatch but does not prevent installs when currentApp.signingFingerprint is null; locate and adjust the block around AutoUpdateWorker where currentApp.signingFingerprint, expected/actual comparison, the IllegalStateException throw, installedAppsRepository.updateApp, and installer.install are defined to ensure verification is performed first and installation/update steps always run afterward (unless the verification throws).core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AndroidPackageMonitor.kt (1)
102-129:⚠️ Potential issue | 🟠 MajorPopulate
DeviceApp.signingFingerprintin the installed-app list.
AndroidPackageMonitor.getAllInstalledApps()is the source for manual linking, and it currently hard-codessigningFingerprint = null. That means apps linked from the picker are persisted without a fingerprint, so the new signing-key verification never activates on that path. Either request signing info in this query and compute it here, or resolve it beforeAppsRepositoryImpl.linkAppToRepo()saves the record.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AndroidPackageMonitor.kt` around lines 102 - 129, AndroidPackageMonitor.getAllInstalledApps() currently sets DeviceApp.signingFingerprint = null; change it to compute and populate the signingFingerprint from the package's signing certificates before constructing DeviceApp. For SDK >= P use pkg.signingInfo (or PackageManager.GET_SIGNING_CERTIFICATES when querying) and for older SDKs use pkg.signatures; extract the signing certificate bytes, compute the fingerprint (e.g. SHA-256 hex) and assign that string to signingFingerprint so linked apps saved by AppsRepositoryImpl.linkAppToRepo() include the signing key fingerprint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AndroidPackageMonitor.kt`:
- Around line 52-75: The signing fingerprint logic currently picks the first
certificate from packageInfo.signingInfo.signingCertificateHistory (via
firstOrNull()), which is the original cert; change it to use the current signer
by selecting the last certificate (lastOrNull()) when reading
signingCertificateHistory, while still using apkContentsSigners when
hasMultipleSigners() is true, so signingFingerprint reflects the active signer.
---
Outside diff comments:
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AndroidPackageMonitor.kt`:
- Around line 102-129: AndroidPackageMonitor.getAllInstalledApps() currently
sets DeviceApp.signingFingerprint = null; change it to compute and populate the
signingFingerprint from the package's signing certificates before constructing
DeviceApp. For SDK >= P use pkg.signingInfo (or
PackageManager.GET_SIGNING_CERTIFICATES when querying) and for older SDKs use
pkg.signatures; extract the signing certificate bytes, compute the fingerprint
(e.g. SHA-256 hex) and assign that string to signingFingerprint so linked apps
saved by AppsRepositoryImpl.linkAppToRepo() include the signing key fingerprint.
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AutoUpdateWorker.kt`:
- Around line 159-193: The update/install logic is incorrectly nested inside the
signing-fingerprint check: move the installedAppsRepository.updateApp(...), the
installer.install(filePath, ext) try/catch (and the subsequent Logger.d waiting
message and updatePendingStatus fallback) out of the if
(currentApp?.signingFingerprint != null) block so that fingerprint verification
only controls the throw on mismatch but does not prevent installs when
currentApp.signingFingerprint is null; locate and adjust the block around
AutoUpdateWorker where currentApp.signingFingerprint, expected/actual
comparison, the IllegalStateException throw, installedAppsRepository.updateApp,
and installer.install are defined to ensure verification is performed first and
installation/update steps always run afterward (unless the verification throws).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2f79e396-1eba-4657-8d15-ccb6d44dad75
📒 Files selected for processing (7)
core/data/src/androidMain/kotlin/zed/rainxch/core/data/local/db/migrations/MIGRATION_4_5.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AndroidPackageMonitor.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AutoUpdateWorker.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerWrapper.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/DeviceApp.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/SystemPackageInfo.ktfeature/apps/data/src/commonMain/kotlin/zed/rainxch/apps/data/repository/AppsRepositoryImpl.kt
✅ Files skipped from review due to trivial changes (1)
- core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/shizuku/ShizukuInstallerWrapper.kt
Comment on lines
+52
to
+75
| val signingFingerprint: String? = | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.P) { | ||
| val sigInfo = packageInfo.signingInfo | ||
| val certs = | ||
| if (sigInfo?.hasMultipleSigners() == true) { | ||
| sigInfo.apkContentsSigners | ||
| } else { | ||
| sigInfo?.signingCertificateHistory | ||
| } | ||
| certs?.firstOrNull()?.toByteArray()?.let { certBytes -> | ||
| MessageDigest | ||
| .getInstance("SHA-256") | ||
| .digest(certBytes) | ||
| .joinToString(":") { "%02X".format(it) } | ||
| } | ||
| } else { | ||
| @Suppress("DEPRECATION") | ||
| packageInfo.signatures?.firstOrNull()?.toByteArray()?.let { certBytes -> | ||
| MessageDigest | ||
| .getInstance("SHA-256") | ||
| .digest(certBytes) | ||
| .joinToString(":") { "%02X".format(it) } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Official Android documentation for SigningInfo: difference between apkContentsSigners and signingCertificateHistory, and whether signingCertificateHistory includes past signing certificates after key rotation
💡 Result:
-
apkContentsSigners/getApkContentsSigners(): returns the certs that actually signed the current APK contents, and explicitly excludes any past/rotated certs the app is authorized to use. The docs note it generally shouldn’t be used unlesshasMultipleSigners()is true (because in that case history isn’t available). [1] -
signingCertificateHistory/getSigningCertificateHistory(): returns the certs the package has proven it is authorized to use, which includes the current signer cert plus past certs provided as proof of signing-certificate rotation. The array is ordered from original (index 0) to current (last index). It returns null when the package is signed by multiple signers (rotation history isn’t used in that case). [1] -
Does
signingCertificateHistoryinclude past signing certificates after key rotation?
Yes—when the APK provides a verified proof-of-rotation record (APK Signature Scheme v3), Android links past signing certs to the new one, and those past certs are included insigningCertificateHistory(and reflected byhasPastSigningCertificates()). [1], [2]
Sources: Android SigningInfo API reference [1]; Android 9 “APK signature scheme with key rotation” documentation [2].
Use the current signer from signingCertificateHistory, not the first entry.
On API 28+, packageInfo.signingInfo.signingCertificateHistory is ordered from original (index 0) to current (last index). Using firstOrNull() selects the original signing certificate, not the current one. After a legitimate key rotation, this causes the stored fingerprint to pin to an ancestor cert and raises false mismatch warnings. Use the last entry (lastOrNull()) to capture the active signer, or rely on apkContentsSigners when hasMultipleSigners() is true.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AndroidPackageMonitor.kt`
around lines 52 - 75, The signing fingerprint logic currently picks the first
certificate from packageInfo.signingInfo.signingCertificateHistory (via
firstOrNull()), which is the original cert; change it to use the current signer
by selecting the last certificate (lastOrNull()) when reading
signingCertificateHistory, while still using apkContentsSigners when
hasMultipleSigners() is true, so signingFingerprint reflects the active signer.
- Add `checkAttestations` to `DetailsRepository` to verify build integrity via GitHub's attestations API - Implement SHA-256 checksum computation for downloaded assets before installation - Introduce `AttestationStatus` to track verification states: unchecked, checking, verified, and unverified - Update `DetailsViewModel` to trigger asynchronous attestation checks post-installation - Enhance `SmartInstallButton` with a new `AttestationBadge` component to display verification status and progress - Add localized strings for "Verified build" and "Checking..." statuses - Update `DetailsState` to persist and reflect the current attestation status in the UI
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt (3)
1195-1197:⚠️ Potential issue | 🟠 MajorA missing candidate fingerprint must fail once one is already stored.
If the installed app already has a
signingFingerprint, returning success when the new APK's fingerprint isnulllets the update bypass the signer check entirely. Keep TOFU for legacy rows if needed, but don't treat a missing fingerprint on the candidate APK as trusted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt` around lines 1195 - 1197, The current logic returns success when apkPackageInfo.signingFingerprint is null even if existingApp.signingFingerprint is non-null; change it so that if existingApp.signingFingerprint != null and apkPackageInfo.signingFingerprint == null you return a failure (e.g., Result.failure with a clear error/exception) instead of Result.success, preserving the existing early-success behavior only when existingApp.signingFingerprint == null (TOFU/legacy); update the check around existingApp.signingFingerprint and apkPackageInfo.signingFingerprint in DetailsViewModel (where these symbols are used) to enforce this failure path.
64-66:⚠️ Potential issue | 🔴 CriticalMove file hashing and file-system checks out of
commonMain.
java.io.File,FileInputStream, andjava.security.MessageDigestare JVM-only, so this shared source set will not stay portable. Hide these operations behind anexpect/actualAPI or an existing platform abstraction such asDownloader/Installerinstead of callingjava.*directly here. As per coding guidelines "Shared code must be incommonMainsource set and use expect/actual for platform-specific APIs".#!/bin/bash # Verify JVM-only APIs are referenced from a commonMain ViewModel. rg -n 'java\.io\.File|java\.io\.FileInputStream|java\.security\.MessageDigest|FileInputStream\(|MessageDigest\.getInstance|File\(' \ feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.ktAlso applies to: 1234-1243, 1277-1278
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt` around lines 64 - 66, DetailsViewModel is importing and directly using JVM-only APIs (java.io.File, FileInputStream, MessageDigest) for file hashing and filesystem checks; extract those operations into a platform-specific API (either add expect/actual functions or extend an existing platform abstraction like Downloader/Installer) and replace direct uses in DetailsViewModel with calls to the new API (e.g., provide functions such as fileExists(path: String), openFileStream(path: String) or computeChecksum(path: String): String implemented in JVM actuals). Update any internal helpers in DetailsViewModel that call File, FileInputStream, or MessageDigest to delegate to the new platform API and remove java.* imports from the commonMain file so the shared source set remains platform-agnostic.
1125-1152:⚠️ Potential issue | 🟠 MajorClear the active install state before showing the signing-key warning.
This early return leaves
downloadStageatINSTALLINGandcurrentAssetNamepopulated while the dialog is open, so the screen still looks like an ongoing install until the warning is dismissed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt` around lines 1125 - 1152, Before setting signingKeyWarning, clear the active install state so the UI stops showing an in-progress install: in the result.onFailure block update the state copy to also reset the install indicators (e.g. set downloadStage to an idle/none value such as DownloadStage.IDLE or DownloadStage.NONE and set currentAssetName to null) along with the SigningKeyWarning; keep the appendLog call and early return afterwards. Reference the existing _state.update { state -> state.copy(...) } call and modify it to include these fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/details/data/src/commonMain/kotlin/zed/rainxch/details/data/repository/DetailsRepositoryImpl.kt`:
- Around line 494-510: Change the contract of checkAttestations from Boolean to
a failure-aware type (e.g., Result<Boolean> or a dedicated
AttestationCheckResult) in the DetailsRepository interface, update the
implementation in DetailsRepositoryImpl.checkAttestations to return
Result.success(true/false) on successful lookup and Result.failure(exception) on
errors (instead of swallowing exceptions and returning false), and update the
presentation layer's AttestationStatus enum to add states for transient/error
(e.g., UNKNOWN/ERROR or CHECK_FAILED) and adjust the single caller to map
Result<Boolean> to VERIFIED/UNVERIFIED on success and the new error state on
failure so the UI can distinguish lookup failures from genuine negative results.
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/SmartInstallButton.kt`:
- Around line 550-595: AttestationBadge currently only shows for CHECKING and
VERIFIED, so UNVERIFIED builds are hidden; update the visibility and rendering
to surface UNVERIFIED explicitly by including AttestationStatus.UNVERIFIED in
the AnimatedVisibility condition (or always render the Row and switch on all
cases) and add a when branch for AttestationStatus.UNVERIFIED inside
AttestationBadge that displays a clear negative badge (e.g., an appropriate
icon, tint like MaterialTheme.colorScheme.error, and a localized string such as
Res.string.unverified_build or add that resource) analogous to the VERIFIED
branch so users can see failed attestation state.
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt`:
- Around line 378-405: The OnOverrideSigningKeyWarning branch performs install
and persistence but skips the post-install attestation flow; after successful
installer.install(...) (and after saveInstalledAppToDatabase(...) for Android)
call the same launchAttestationCheck(...) invocation used in the normal
post-install path (pass the same arguments: asset name/currentAssetName or file
path/isUpdate/releaseTag as used elsewhere) so attestationStatus is updated,
then proceed to set downloadStage to IDLE, clear currentAssetName and appendLog
as before; ensure you use the same function name launchAttestationCheck and same
parameter ordering as the other successful-install branch to keep behavior
consistent.
- Around line 1274-1288: The current reuse check in DetailsViewModel
(existingPath = downloader.getDownloadedFilePath(assetName) and comparing
existingFile.length() == sizeBytes) is unsafe; change it to either (1) key
cached files by a stable identifier like the release/download URL (call
downloader.getDownloadedFilePath(downloadUrl) or include downloadUrl when
resolving cached path) or (2) validate the file contents before reuse by
computing a checksum and comparing it to the expected hash from release metadata
(verifyHash(existingFile, expectedHash)) before setting filePath and updating
_state; update any references to assetName/sizeBytes logic (existingFile,
getDownloadedFilePath, and the reuse branch) to use the URL-based cache key or
the verified-hash check so stale or corrupted binaries are never reused without
content verification.
---
Duplicate comments:
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt`:
- Around line 1195-1197: The current logic returns success when
apkPackageInfo.signingFingerprint is null even if existingApp.signingFingerprint
is non-null; change it so that if existingApp.signingFingerprint != null and
apkPackageInfo.signingFingerprint == null you return a failure (e.g.,
Result.failure with a clear error/exception) instead of Result.success,
preserving the existing early-success behavior only when
existingApp.signingFingerprint == null (TOFU/legacy); update the check around
existingApp.signingFingerprint and apkPackageInfo.signingFingerprint in
DetailsViewModel (where these symbols are used) to enforce this failure path.
- Around line 64-66: DetailsViewModel is importing and directly using JVM-only
APIs (java.io.File, FileInputStream, MessageDigest) for file hashing and
filesystem checks; extract those operations into a platform-specific API (either
add expect/actual functions or extend an existing platform abstraction like
Downloader/Installer) and replace direct uses in DetailsViewModel with calls to
the new API (e.g., provide functions such as fileExists(path: String),
openFileStream(path: String) or computeChecksum(path: String): String
implemented in JVM actuals). Update any internal helpers in DetailsViewModel
that call File, FileInputStream, or MessageDigest to delegate to the new
platform API and remove java.* imports from the commonMain file so the shared
source set remains platform-agnostic.
- Around line 1125-1152: Before setting signingKeyWarning, clear the active
install state so the UI stops showing an in-progress install: in the
result.onFailure block update the state copy to also reset the install
indicators (e.g. set downloadStage to an idle/none value such as
DownloadStage.IDLE or DownloadStage.NONE and set currentAssetName to null) along
with the SigningKeyWarning; keep the appendLog call and early return afterwards.
Reference the existing _state.update { state -> state.copy(...) } call and
modify it to include these fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 16003414-341b-4a6c-acc6-51c8831b07a6
📒 Files selected for processing (8)
core/presentation/src/commonMain/composeResources/values/strings.xmlfeature/details/data/src/commonMain/kotlin/zed/rainxch/details/data/dto/AttestationsResponse.ktfeature/details/data/src/commonMain/kotlin/zed/rainxch/details/data/repository/DetailsRepositoryImpl.ktfeature/details/domain/src/commonMain/kotlin/zed/rainxch/details/domain/repository/DetailsRepository.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsState.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/SmartInstallButton.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/model/AttestationStatus.kt
Comment on lines
+494
to
+510
| override suspend fun checkAttestations( | ||
| owner: String, | ||
| repo: String, | ||
| sha256Digest: String, | ||
| ): Boolean = | ||
| try { | ||
| val response = | ||
| httpClient | ||
| .executeRequest<AttestationsResponse> { | ||
| get("/repos/$owner/$repo/attestations/sha256:$sha256Digest") { | ||
| header(HttpHeaders.Accept, "application/vnd.github+json") | ||
| } | ||
| }.getOrNull() | ||
| response != null && response.attestations.isNotEmpty() | ||
| } catch (e: Exception) { | ||
| logger.debug("Attestation check failed for $owner/$repo: ${e.message}") | ||
| false |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that repository failures are folded into `false`
# and that the caller maps that boolean into attestation UI state.
rg -n -C3 'checkAttestations\(|response != null && response.attestations.isNotEmpty\(\)|AttestationStatus\.(VERIFIED|UNVERIFIED)' \
feature/details/data/src/commonMain/kotlin/zed/rainxch/details/data/repository/DetailsRepositoryImpl.kt \
feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.ktRepository: OpenHub-Store/GitHub-Store
Length of output: 4488
🏁 Script executed:
# Search for other usages of checkAttestations method
rg -n 'checkAttestations' --type ktRepository: OpenHub-Store/GitHub-Store
Length of output: 96
🏁 Script executed:
# Find AttestationStatus enum definition
rg -n -A 10 'enum class AttestationStatus|sealed class AttestationStatus' --type ktRepository: OpenHub-Store/GitHub-Store
Length of output: 96
🏁 Script executed:
# Find the interface/contract definition for checkAttestations
rg -n -B 2 -A 6 'fun checkAttestations' --type kt | head -50Repository: OpenHub-Store/GitHub-Store
Length of output: 96
🏁 Script executed:
# Search for other usages of checkAttestations method
rg -n 'checkAttestations' --type-add 'kotlin:*.kt' -t kotlinRepository: OpenHub-Store/GitHub-Store
Length of output: 564
🏁 Script executed:
# Find AttestationStatus enum definition
rg -n -A 10 'enum class AttestationStatus|sealed class AttestationStatus'Repository: OpenHub-Store/GitHub-Store
Length of output: 839
🏁 Script executed:
# Find the interface/contract definition for checkAttestations
rg -n -B 2 -A 6 'fun checkAttestations'Repository: OpenHub-Store/GitHub-Store
Length of output: 2280
Rethink error handling across the attestation check architecture.
The checkAttestations() method returns Boolean, collapsing both API failures and genuine negative lookups into false. This limits callers to VERIFIED/UNVERIFIED states with no way to represent transient failures or retry-able errors.
The architectural issue spans two layers:
-
Domain layer (
DetailsRepository.kt:45-49): The interface contract definesBooleanas the return type. Change this to returnResult<Boolean>or introduce a dedicated attestation result type. -
Presentation layer (
AttestationStatusenum): Currently has UNCHECKED, CHECKING, VERIFIED, and UNVERIFIED—no state for "unknown" or "error". Add states to represent failed checks so the UI can distinguish between genuine lack of attestations vs. transient lookup failures.
This is the only caller of this method, so the change is isolated to the details feature.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/details/data/src/commonMain/kotlin/zed/rainxch/details/data/repository/DetailsRepositoryImpl.kt`
around lines 494 - 510, Change the contract of checkAttestations from Boolean to
a failure-aware type (e.g., Result<Boolean> or a dedicated
AttestationCheckResult) in the DetailsRepository interface, update the
implementation in DetailsRepositoryImpl.checkAttestations to return
Result.success(true/false) on successful lookup and Result.failure(exception) on
errors (instead of swallowing exceptions and returning false), and update the
presentation layer's AttestationStatus enum to add states for transient/error
(e.g., UNKNOWN/ERROR or CHECK_FAILED) and adjust the single caller to map
Result<Boolean> to VERIFIED/UNVERIFIED on success and the new error state on
failure so the UI can distinguish lookup failures from genuine negative results.
Comment on lines
+550
to
+595
| @Composable | ||
| private fun AttestationBadge(attestationStatus: AttestationStatus) { | ||
| AnimatedVisibility( | ||
| visible = attestationStatus == AttestationStatus.VERIFIED || attestationStatus == AttestationStatus.CHECKING, | ||
| enter = fadeIn(), | ||
| exit = fadeOut(), | ||
| ) { | ||
| Row( | ||
| modifier = Modifier.padding(top = 8.dp), | ||
| verticalAlignment = Alignment.CenterVertically, | ||
| horizontalArrangement = Arrangement.Center, | ||
| ) { | ||
| when (attestationStatus) { | ||
| AttestationStatus.CHECKING -> { | ||
| CircularProgressIndicator( | ||
| modifier = Modifier.size(14.dp), | ||
| strokeWidth = 2.dp, | ||
| color = MaterialTheme.colorScheme.onSurfaceVariant, | ||
| ) | ||
| Spacer(modifier = Modifier.width(6.dp)) | ||
| Text( | ||
| text = stringResource(Res.string.checking_attestation), | ||
| style = MaterialTheme.typography.labelSmall, | ||
| color = MaterialTheme.colorScheme.onSurfaceVariant, | ||
| ) | ||
| } | ||
| AttestationStatus.VERIFIED -> { | ||
| Icon( | ||
| imageVector = Icons.Filled.VerifiedUser, | ||
| contentDescription = null, | ||
| modifier = Modifier.size(14.dp), | ||
| tint = MaterialTheme.colorScheme.tertiary, | ||
| ) | ||
| Spacer(modifier = Modifier.width(4.dp)) | ||
| Text( | ||
| text = stringResource(Res.string.verified_build), | ||
| style = MaterialTheme.typography.labelSmall, | ||
| color = MaterialTheme.colorScheme.tertiary, | ||
| fontWeight = FontWeight.SemiBold, | ||
| ) | ||
| } | ||
| else -> {} | ||
| } | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Surface UNVERIFIED explicitly instead of hiding it.
The badge only renders for CHECKING and VERIFIED, so a completed failed attestation is indistinguishable from "not checked yet". That drops the only user-facing signal for a negative integrity result.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/SmartInstallButton.kt`
around lines 550 - 595, AttestationBadge currently only shows for CHECKING and
VERIFIED, so UNVERIFIED builds are hidden; update the visibility and rendering
to surface UNVERIFIED explicitly by including AttestationStatus.UNVERIFIED in
the AnimatedVisibility condition (or always render the Row and switch on all
cases) and add a when branch for AttestationStatus.UNVERIFIED inside
AttestationBadge that displays a clear negative badge (e.g., an appropriate
icon, tint like MaterialTheme.colorScheme.error, and a localized string such as
Res.string.unverified_build or add that resource) analogous to the VERIFIED
branch so users can see failed attestation state.
Comment on lines
+378
to
+405
| DetailsAction.OnOverrideSigningKeyWarning -> { | ||
| val warning = _state.value.signingKeyWarning ?: return | ||
| _state.update { it.copy(signingKeyWarning = null) } | ||
| viewModelScope.launch { | ||
| try { | ||
| val ext = warning.pendingAssetName.substringAfterLast('.', "").lowercase() | ||
| installer.install(warning.pendingFilePath, ext) | ||
|
|
||
| if (platform == Platform.ANDROID) { | ||
| saveInstalledAppToDatabase( | ||
| assetName = warning.pendingAssetName, | ||
| assetUrl = warning.pendingDownloadUrl, | ||
| assetSize = warning.pendingSizeBytes, | ||
| releaseTag = warning.pendingReleaseTag, | ||
| isUpdate = warning.pendingIsUpdate, | ||
| filePath = warning.pendingFilePath, | ||
| ) | ||
| } | ||
|
|
||
| _state.value = _state.value.copy(downloadStage = DownloadStage.IDLE) | ||
| currentAssetName = null | ||
| appendLog( | ||
| assetName = warning.pendingAssetName, | ||
| size = warning.pendingSizeBytes, | ||
| tag = warning.pendingReleaseTag, | ||
| result = if (warning.pendingIsUpdate) LogResult.Updated else LogResult.Installed, | ||
| ) | ||
| } catch (t: Throwable) { |
Contributor
There was a problem hiding this comment.
Keep the override path on the same post-install integrity flow.
This branch installs and persists the APK directly, but it skips launchAttestationCheck(...). That leaves attestationStatus stuck at UNCHECKED on the one path where the user explicitly accepted a signing-key change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt`
around lines 378 - 405, The OnOverrideSigningKeyWarning branch performs install
and persistence but skips the post-install attestation flow; after successful
installer.install(...) (and after saveInstalledAppToDatabase(...) for Android)
call the same launchAttestationCheck(...) invocation used in the normal
post-install path (pass the same arguments: asset name/currentAssetName or file
path/isUpdate/releaseTag as used elsewhere) so attestationStatus is updated,
then proceed to set downloadStage to IDLE, clear currentAssetName and appendLog
as before; ensure you use the same function name launchAttestationCheck and same
parameter ordering as the other successful-install branch to keep behavior
consistent.
Comment on lines
+1274
to
+1288
| val existingPath = downloader.getDownloadedFilePath(assetName) | ||
| val filePath: String | ||
|
|
||
| val existingFile = existingPath?.let { File(it) } | ||
| if (existingFile != null && existingFile.exists() && existingFile.length() == sizeBytes) { | ||
| logger.debug("Reusing already downloaded file: $assetName") | ||
| filePath = existingPath | ||
| _state.value = | ||
| _state.value.copy( | ||
| downloadProgressPercent = 100, | ||
| downloadedBytes = sizeBytes, | ||
| totalBytes = sizeBytes, | ||
| downloadStage = DownloadStage.VERIFYING, | ||
| ) | ||
| } else { |
Contributor
There was a problem hiding this comment.
Don't reuse cached downloads by filename and size alone.
Many releases keep the same asset name, and equal length does not prove identical content. Reusing a file on that check can install a stale or corrupted binary if an older download happens to match the size. Tie reuse to the release/download URL or verify the file hash before skipping the download.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt`
around lines 1274 - 1288, The current reuse check in DetailsViewModel
(existingPath = downloader.getDownloadedFilePath(assetName) and comparing
existingFile.length() == sizeBytes) is unsafe; change it to either (1) key
cached files by a stable identifier like the release/download URL (call
downloader.getDownloadedFilePath(downloadUrl) or include downloadUrl when
resolving cached path) or (2) validate the file contents before reuse by
computing a checksum and comparing it to the expected hash from release metadata
(verifyHash(existingFile, expectedHash)) before setting filePath and updating
_state; update any references to assetName/sizeBytes logic (existingFile,
getDownloadedFilePath, and the reuse branch) to use the URL-based cache key or
the verified-hash check so stale or corrupted binaries are never reused without
content verification.
- Add validation logic to verify that the linked GitHub repository's APK matches the installed app's package name and signing fingerprint - Implement background verification process: check latest release, download APK, and extract signing info before linking - Update `AppsViewModel` to handle the verification workflow and provide real-time status updates - Enhance `LinkAppBottomSheet` UI to display validation status messages (e.g., "Checking latest release...", "Verifying signing key...") - Add new localized strings for mismatch errors and validation status steps - Ensure temporary APK files used for verification are deleted after the process completes
- Replace complex logic for fetching and prioritizing localized README variants with a single request for the root `README.md` file. - Remove the use of `coroutineScope`, `async`, and `awaitAll` for concurrent README path attempts. - Simplify the fetch process to target `https://raw.githubusercontent.com/{owner}/{repo}/{branch}/README.md` directly. - Maintain markdown preprocessing and language detection for the fetched `README.md`. - Remove unused imports related to coroutines and the legacy readme helper attempts.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.kt`:
- Line 753: Replace raw English validation messages assigned via _state.update {
it.copy(repoValidationError = "...") } with localized strings retrieved through
getString(Res.string.<key>). For each occurrence (current hardcoded messages at
the repoValidationError assignments), create/use appropriate string resource
keys (e.g., invalid_github_url, invalid_owner_repo, invalid_branch_name,
invalid_ref) and call repoValidationError =
getString(Res.string.invalid_github_url) (etc.), ensuring getString is available
in this ViewModel (or inject/use the existing string/resource provider) and
update imports/usages accordingly so all four places use Res.string resources
instead of hardcoded literals.
- Around line 831-846: The current flow in AppsViewModel treats null results
from fetching or selecting a release/asset as "no validation error" which lets
linking continue; instead, change the branches that return null after
getLatestRelease, when installableAssets.isEmpty(), and when
choosePrimaryAsset(...) returns null to set an explicit failure state on _state
(e.g., update linkValidationStatus to a verification-failed message via
getString(...) and/or a dedicated error enum) and then return a non-null failure
indicator (or throw a specific VerificationException) so the caller does not
treat the outcome as successful; update the code paths around latestRelease,
installableAssets, and asset to set that error state before returning and ensure
the caller that currently treats null as "no error" checks the explicit error
state instead.
- Around line 888-900: The URL parser currently strips prefixes and extracts
owner/repo from the cleaned string (variables: input, cleaned, parts) but
accepts non-GitHub hosts (e.g., gitlab.com). Before stripping prefixes and
splitting, validate the host: parse the original input (or examine its prefix)
to ensure the host is github.com or www.github.com, and only then proceed to
removePrefix and split to produce parts; if the host is not an allowed GitHub
host return null. Update the logic around cleaned/parts to gate extraction with
this host check so only GitHub URLs produce owner/repo.
- Around line 880-883: The finally block in AppsViewModel.kt uses
java.io.File(filePath).delete() from commonMain (via the filePath local), which
must be moved behind an expect/actual platform abstraction; introduce an expect
interface (e.g. TempFileCleaner with fun delete(path: String): Boolean) in
commonMain, provide actual implementations on each platform, add a
TempFileCleaner dependency to AppsViewModel (constructor or injected property),
and replace the File(filePath).delete() call with
tempFileCleaner.delete(filePath) inside the existing try/catch so the deletion
is delegated to platform code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d83ab6fd-2aa1-4599-877f-6e174572756a
📒 Files selected for processing (4)
core/presentation/src/commonMain/composeResources/values/strings.xmlfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsState.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/components/LinkAppBottomSheet.kt
feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.kt
Outdated
Show resolved
Hide resolved
feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.kt
Outdated
Show resolved
Hide resolved
Comment on lines
+880
to
+883
| } finally { | ||
| try { | ||
| if (filePath != null) File(filePath).delete() | ||
| } catch (_: Exception) { } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify JVM-only file APIs used from commonMain source sets
rg -n --glob '**/commonMain/**/*.kt' '^\s*import\s+java\.io\.'
rg -n --glob '**/commonMain/**/*.kt' '\bFile\s*\('Repository: OpenHub-Store/GitHub-Store
Length of output: 1208
Move file deletion behind an expect/actual abstraction to keep commonMain platform-safe.
Line 882 uses java.io.File in a commonMain source set, which violates the requirement that platform-specific APIs be abstracted via expect/actual. Use an abstraction like a platform-aware cleaner interface or repository method:
Proposed refactor
- } finally {
- try {
- if (filePath != null) File(filePath).delete()
- } catch (_: Exception) { }
- }
+ } finally {
+ try {
+ filePath?.let { appsRepository.deleteTemporaryFile(it) }
+ } catch (_: Exception) { }
+ }// commonMain
expect interface TempFileCleaner {
fun delete(path: String): Boolean
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.kt`
around lines 880 - 883, The finally block in AppsViewModel.kt uses
java.io.File(filePath).delete() from commonMain (via the filePath local), which
must be moved behind an expect/actual platform abstraction; introduce an expect
interface (e.g. TempFileCleaner with fun delete(path: String): Boolean) in
commonMain, provide actual implementations on each platform, add a
TempFileCleaner dependency to AppsViewModel (constructor or injected property),
and replace the File(filePath).delete() call with
tempFileCleaner.delete(filePath) inside the existing try/catch so the deletion
is delegated to platform code.
feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.kt
Outdated
Show resolved
Hide resolved
- Implement a new "Pick Asset" step in the manual app linking flow to allow users to select a specific APK for verification. - Add support for downloading and verifying the selected asset's package name and signing fingerprint against the installed app. - Update `AppsViewModel` to handle asset selection, download progress tracking, and detailed validation logic. - Enhance GitHub URL parsing to be more robust and strictly validate the "github.com" domain. - Refactor string resources to move hardcoded UI text into `strings.xml` for better localization support. - Improve the "Apps" list sorting to prioritize apps with available updates. - Update `LinkAppBottomSheet` UI with `AnimatedContent` for smoother transitions between linking steps and a new `PickAssetStep` list.
- Add localized strings for "Link app to repo", "Export/Import", and "Pre-release support" features. - Provide translations for Italian, Japanese, Korean, Polish, Arabic, Russian, Turkish, Bengali, Chinese (Simplified), Spanish, French, and Hindi. - Include strings for repository validation, signing key verification, package name mismatch warnings, and uninstall confirmation dialogs. - Add UI labels for asset selection, download status, and GitHub API rate limiting notifications.
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
1 participant
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.
…uku integration
signingFingerprintfield toInstalledAppEntity,InstalledApp, andApkPackageInfoto track app authenticityAndroidInstallerInfoExtractorto extract SHA-256 signing certificates for both modern (API 28+) and legacy Android versionsDetailsViewModelto decouple download and installation logic into distinct, manageable methodsInstalledAppsRepositoryandPackageEventReceiverto persist and verify signing fingerprints during updatesShizukuStatusto a dedicated model and refining lifecycle handlingAutoUpdateWorkerandUpdateSchedulerlogic, including better notification handling and permission checksSummary by CodeRabbit
New Features
Bug Fixes
Chores