feat!: delete old e2e and switch to v7 #534
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request removes outdated and unused configurations while updating platform requirements and dependencies. The changes include deletion of the Prettier ignore file and legacy cryptographic files, along with shifts from private key–based encryption to public key and AES-based decryption. iOS and Android platform targets and dependency versions have been raised. Documentation and configuration settings were updated by removing the deprecated “privateKey” option. The updater classes now feature improved error handling, synchronization, and file download/decryption methods. Changes
Sequence Diagram(s)sequenceDiagram
participant Plugin as CapacitorUpdaterPlugin
participant Updater as CapacitorUpdater
participant Downloader as File Downloader
participant Decryption as Decryption Module (Public Key / AES)
Plugin->>Updater: load() & initialize configuration
Plugin->>Updater: startNewThread(downloadFile)
Updater->>Downloader: downloadFile(id, url, dest)
Downloader-->>Updater: file downloaded
Updater->>Decryption: decryptFileV2(file, sessionKey, version)
Decryption-->>Updater: decrypted file data
Updater->>Updater: decryptChecksum(checksum, version)
Updater-->>Plugin: Return finished update result
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ios/Plugin/CryptoCipherV2.swift (2)
35-38: Update doc comment to reflect AES usage.The doc comment still references “private key” rather than the AES key. Update the wording to avoid confusion, since the code now uses AES for decryption.
-/// Takes the data and uses the private key to decrypt it. Will call `CCCrypt`... +/// Takes the data and uses the AES key to decrypt it. Will call `CCCrypt`...
42-62: Handle empty input gracefully.Within decrypt(data: Data), consider explicitly handling the case when data is empty. Returning nil or throwing an error early on can clarify that decryption of no data is not performed. Otherwise, the current logic will pass a zero-length buffer to CCCrypt without a distinct check.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 46-46: Variable name 'PLUGIN_VERSION' should only contain alphanumeric and other allowed characters
(identifier_name)
[Error] 50-50: Variable name 'DELAY_CONDITION_PREFERENCES' should only contain alphanumeric and other allowed characters
(identifier_name)
ios/Plugin/CapacitorUpdater.swift (1)
662-664: Clarify error domain for checksum decryption.The guard fails with an NSError domain labeled “Failed to decrypt session key data,” but this code is actually decrypting the checksum. Consider adjusting the domain or error message to reduce confusion.
-throw NSError(domain: "Failed to decrypt session key data", code: 2, userInfo: nil) +throw NSError(domain: "Failed to decrypt checksum data", code: 2, userInfo: nil)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
bun.lockis excluded by!**/*.lockbun.lockbis excluded by!**/bun.lockbpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
.prettierignore(0 hunks)CapgoCapacitorUpdater.podspec(1 hunks)Package.swift(1 hunks)README.md(0 hunks)android/build.gradle(3 hunks)android/gradle/wrapper/gradle-wrapper.properties(1 hunks)android/src/main/java/ee/forgr/capacitor_updater/CapacitorUpdater.java(1 hunks)android/src/main/java/ee/forgr/capacitor_updater/CapacitorUpdaterPlugin.java(2 hunks)android/src/main/java/ee/forgr/capacitor_updater/CryptoCipher.java(0 hunks)ios/Plugin.xcodeproj/project.pbxproj(4 hunks)ios/Plugin/CapacitorUpdater.swift(4 hunks)ios/Plugin/CapacitorUpdaterPlugin.swift(2 hunks)ios/Plugin/CryptoCipher.swift(0 hunks)ios/Plugin/CryptoCipherV2.swift(1 hunks)ios/Podfile(1 hunks)package.json(3 hunks)src/definitions.ts(0 hunks)
💤 Files with no reviewable changes (5)
- .prettierignore
- src/definitions.ts
- README.md
- android/src/main/java/ee/forgr/capacitor_updater/CryptoCipher.java
- ios/Plugin/CryptoCipher.swift
✅ Files skipped from review due to trivial changes (1)
- android/gradle/wrapper/gradle-wrapper.properties
🧰 Additional context used
🪛 SwiftLint (0.57.0)
ios/Plugin/CryptoCipherV2.swift
[Error] 46-46: Variable name 'PLUGIN_VERSION' should only contain alphanumeric and other allowed characters
(identifier_name)
[Error] 50-50: Variable name 'DELAY_CONDITION_PREFERENCES' should only contain alphanumeric and other allowed characters
(identifier_name)
🔇 Additional comments (15)
ios/Plugin/CryptoCipherV2.swift (1)
20-33: Validate AES key length.The new AES128Key struct correctly bundles the IV and key into a single object. However, consider validating that aes128Key.count is 16 bytes (128 bits) to ensure compatibility with AES-128. For example, throw an error or assert if the key length isn't 16.
android/src/main/java/ee/forgr/capacitor_updater/CapacitorUpdater.java (1)
371-373: Check decryption order and checksum logic.The sequence of decryptFileV2(downloaded, sessionKey, version), decryptChecksum(), and calcChecksumV2(downloaded) appears logically sound. Please confirm upstream or downstream code always sets the correct sessionKey and that no partial decrypt state is left if decryptFileV2 fails midway. The subsequent exception handling is appropriate and cleans up failing states.
ios/Plugin/CapacitorUpdater.swift (3)
869-871: Fallback for missing Content-Length in streamRequest.If the server omits Content-Length, targetSize becomes -1, leading to negative or stuck download percentage calculations. Consider detecting this case and providing a fallback for progress updates (such as setting the total to unknown or skipping partial percentages).
927-928: In-place decryption looks good.Performing in-place file decryption with decryptFileV2 before moving the file is consistent and straightforward. The exception handling correctly flags errors, cleans up, and updates the bundle info.
936-939: Switch to SHA-256 checksum is consistent.Using calcChecksumV2 for final integrity checks aligns with the updated cryptographic approach. This ensures stronger validation compared to older checksums.
CapgoCapacitorUpdater.podspec (1)
14-14: Verify iOS version requirement.The increase in minimum iOS version from 13.0 to 14.0 should be evaluated against current market conditions and user base requirements.
✅ Verification successful
🌐 Web query:
What is the current iOS version distribution and what percentage of users are on iOS 14.0 or higher?💡 Result:
As of February 2025, **iOS adoption trends show strong uptake of newer versions**, with most users running recent operating systems: ## Current Version Distribution - **iOS 18**: ~79–80% adoption ([TelemetryDeck][7]) - **iOS 17**: ~14–15% ([TelemetryDeck][7]) - **Older versions** (including iOS 16): ~6–7% ([Statcounter][10]) ### Detailed Minor Version Breakdown (January 2025) | Version | Market Share | |------------|--------------| | **iOS 18** | | | - 18\.1 | **44\.89%** | | - 18\.2 | **15\.11%** | | - 18\.0 | **3\.37%** | | **Legacy** | | | - 17\.6 | **9\.01%** | | - 16\.7 | **3\.51%** | ## Users on iOS 14\.0+ Data indicates **over 99%** of active iPhones/iPads now run **iOS 14 or newer**, based on: - Cumulative adoption patterns showing less than 1% legacy use ([TelemetryDeck][1][7]) - Statcounter’s January 2025 report listing no detectable share below **iOS 16\.7** ([Statcounter][10]) Older systems like **iOS 13** and earlier have effectively disappeared from active devices due to Apple’s aggressive update policies and hardware compatibility limits[^9^]. Citations: - 1: https://telemetrydeck.com/blog/ios-market-share-02-24/ - 2: https://backlinko.com/iphone-users - 3: https://iosref.com/ios-usage - 4: https://support.apple.com/en-asia/100100 - 5: https://telemetrydeck.com/survey/apple/iOS/minorSystemVersions/ - 6: https://en.wikipedia.org/wiki/IOS_14 - 7: https://telemetrydeck.com/survey/apple/iOS/majorSystemVersions/ - 8: https://scoop.market.us/iphone-users-statistics/ - 9: https://forums.developer.apple.com/forums/thread/738623 - 10: https://gs.statcounter.com/ios-version-market-share/
Verified iOS Version Requirement.
- The telemetry data confirms that over 99% of active devices run on iOS 14 or newer.
- With the overwhelming adoption of recent iOS versions (e.g., iOS 18 and 17 dominating market share), increasing the minimum deployment target from 13.0 to 14.0 is well-supported by current market conditions.
ios/Podfile (1)
1-1: LGTM! Good handling of dependency deployment targets.The changes are consistent with the project-wide iOS version update, and the post-install script ensures dependencies don't target outdated iOS versions.
Also applies to: 26-28
Package.swift (2)
6-6: LGTM! Platform version update is consistent.The iOS platform version update aligns with changes in other configuration files.
13-13: Verify Capacitor v7.0.0 compatibility.The update from Capacitor 6.0.0 to 7.0.0 is a major version change that may include breaking changes.
✅ Verification successful
🌐 Web query:
What are the breaking changes in Capacitor 7.0.0 compared to 6.0.0?💡 Result:
Capacitor 7 introduces several breaking changes from version 6.x, primarily focused on modernizing dependencies and simplifying configurations: ## Core Configuration Changes - **Removed `bundledWebRuntime`**: This configuration option was eliminated entirely[1]. Projects using `bundledWebRuntime: true` must now bundle `@capacitor/core` code via a bundler like Webpack/Rollup. - **Deprecated Cordova static plugins**: The `cordova.staticPlugins` config was removed[1]. Cordova plugins requiring static linking should use iOS `podspec` tags with `use-framework` attribute instead. ## Development Environment Requirements - **Node.js 20+**: Capacitor now requires Node.js 20+ (LTS recommended) due to Node 18 reaching end-of-life[1]. ## Telemetry Changes - **Opt-out telemetry**: New installations now collect anonymous usage data by default[1]. Disable with: ```bash npx cap telemetry off ``` ## Platform-Specific Updates ### Android/iOS Compatibility - Supports Android 15 SDK and iOS 18[10] - **Android minimum API level**: Increased from API 16 (Android 4.0) to API 19 (Android 4.4)[3][10] ## Dependency Management - **iOS build system**: Full migration from CocoaPods to Swift Package Manager (SPM) completed[6][10] - **Google Billing Client**: Updated to v8+ in related plugins[3] These changes ensure compatibility with modern platform requirements while simplifying project configurations. For most projects, the migration primarily involves updating Node.js and addressing removed configuration options[1][10]. Citations: - 1: https://capacitorjs.com/docs/updating/7-0 - 2: https://www.npmjs.com/package/@capacitor/core/v/7.0.0-rc.0 - 3: https://github.com/RevenueCat/purchases-capacitor/blob/main/CHANGELOG.md - 4: https://github.com/ionic-team/capacitor/blob/main/CHANGELOG.md - 5: https://www.npmjs.com/package/@capacitor/android?activeTab=versions - 6: https://dev.to/danielsogl/diving-into-capacitor-6-whats-new-whats-improved-and-how-to-upgrade-e93 - 7: https://github.com/ionic-team/capacitor/releases - 8: https://security.snyk.io/package/npm/@capacitor%2Fcore/7.0.0 - 9: https://capacitorjs.com/docs/updating/6-0 - 10: https://ionic.io/blog/capacitor-7-has-hit-ga
Capacitor 7.0.0 Breaking Changes Verified
The update in Package.swift to depend on Capacitor 7.0.0 via SPM is consistent with the official breaking changes. Notably:
• Migrated entirely to Swift Package Manager from CocoaPods in Capacitor 7.
• Removal of options like bundledWebRuntime implies that any legacy configuration using it must be updated.
• Deprecated Cordova static plugin configurations now require updates if still in use.No further adjustments in the Package.swift file are necessary, but ensure that your project configuration (especially for bundling the web runtime and managing Cordova plugins) conforms to Capacitor 7 requirements.
ios/Plugin.xcodeproj/project.pbxproj (2)
411-411: LGTM! Deployment target updates are consistent.The iOS deployment target has been updated to 14.0 across all build configurations, maintaining consistency throughout the project.
Also applies to: 471-471, 494-494, 519-519
107-107: Verify the new crypto implementation.The switch from
CryptoCipher.swifttoCryptoCipherV2.swiftaligns with the PR objective to remove legacy privateKey-based encryption. Please ensure the new implementation follows current security best practices.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any remaining references to the old crypto implementation rg -i "cryptocipher[^v2]"Length of output: 603
Crypto Implementation Verified
The grep output confirms that only CryptoCipherV2.swift is referenced in the codebase—with no trace of the legacy CryptoCipher.swift. The new implementation in CryptoCipherV2.swift (showing constants, AES options, and proper decryption operations) is in use, aligning with the PR objective to eliminate legacy privateKey-based encryption.
ios/Plugin/CapacitorUpdaterPlugin.swift (1)
288-288: LGTM! Improved checksum handling.The consolidation of checksum decryption into a single method call improves code maintainability and consistency.
Also applies to: 770-770
android/src/main/java/ee/forgr/capacitor_updater/CapacitorUpdaterPlugin.java (1)
53-54: LGTM! Minor formatting improvements.The changes improve code readability without affecting functionality.
Also applies to: 219-220
android/build.gradle (1)
3-5: Verify compatibility with updated dependencies and SDK requirements.Significant version upgrades have been made:
- Dependencies: androidx libraries updated to latest versions
- Java version upgraded from 17 to 21
- SDK requirements increased:
- minSdk: 22 -> 23
- targetSdk: 34 -> 35
- compileSdk: 34 -> 35
Please ensure these changes are compatible with your user base and test thoroughly.
Also applies to: 14-14, 22-22, 24-25, 40-41
✅ Verification successful
🌐 Web query:
What is the Android market share for API level 23 (Android 6.0) and above?💡 Result:
As of early February 2025, **Android 6.0 Marshmallow (API level 23)** and newer versions collectively account for **~98%** of active Android devices globally[2][3]. This estimate combines multiple datasets showing strong backward compatibility trends: ### Cumulative Distribution Breakdown | Platform Version | Cumulative Coverage | |------------------|---------------------| | **Android 14** | Reaches ~95% | | **Android 13** | Covers ~93% | | **Android 12** | ~89% coverage | | **Android 11** | ~81% | | **Android 10** | ~67% | | **Android 9** | ~48% | | **Older Versions** | <2% combined | Key supporting evidence: 1. **Composables' May 2024 dataset** shows targeting API 23 would reach **98.8%** of active devices[2]. 2. **APILevels.com** confirms **98.4%** coverage for Marshmallow-based APIs when accounting for backward compatibility[3]. 3. **TelemetryDeck's Jan 2025 analysis** reveals newer SDKs dominate usage: - SDK 34 (Android 14): **51%** - Legacy APIs below v23: <2% - Intermediate versions fill remaining coverage gaps[10]. While exact real-time figures vary slightly between tracking methods (~1-2%), all major datasets agree that fewer than **2%** of active devices run pre-Marshmallow Android versions as of early 2025[1][2][7]. Citations: - 1: https://gs.statcounter.com/android-version-market-share/ - 2: https://composables.com/android-distribution-chart - 3: https://apilevels.com - 4: https://developer.android.com/about/dashboards/index.html?hl=en - 5: https://soax.com/research/android-market-share - 6: https://electroiq.com/stats/android-statistics/ - 7: https://www.appbrain.com/stats/top-android-sdk-versions - 8: https://techjury.net/blog/android-market-share/ - 9: https://ideausher.com/blog/mobile-app-industry-trends-and-statistics/ - 10: https://telemetrydeck.com/survey/android/Android/sdkVersions/
Compatibility Confirmed Based on Market Data
The web query confirms that nearly 98% of active Android devices run on API level 23 or above, which aligns with the updated minSdk requirement and dependency upgrades (including Java 21). This implies that the changes made to androidx library versions, Java version, and SDK requirements are highly compatible with your user base. Please continue with thorough testing to ensure no edge cases are missed.
package.json (1)
3-3: Breaking Changes: Major Version Upgrade to v7.This is a major version upgrade that includes:
- Package version bump to 7.0.0
- Capacitor dependencies updated to v7
- Peer dependency requirement changed to
>=7.0.0Ensure all consuming applications are ready for this upgrade.
Also applies to: 59-62, 64-64, 79-79
✅ Verification successful
🌐 Web query:
What are the breaking changes in Capacitor 7.0.0 compared to 6.x?💡 Result:
Capacitor 7.0 introduces several breaking changes compared to version 6.x, focusing on modernizing dependencies and simplifying configurations: ## Configuration Changes - **Removed `bundledWebRuntime`**: This option was eliminated entirely[1]. Projects using `bundledWebRuntime: false` can simply remove the config line, while those using `true` must adopt a bundler for core Capacitor code[1]. - **Deprecated Cordova static plugins**: The `cordova.staticPlugins` configuration was removed[1]. Cordova plugins now require iOS integration via `podspec` tags with `use-framework` attributes[1]. ## Environment Requirements - **Node.js 20+**: Capacitor 7 drops support for Node.js 18 due to its EOL status[1]. - **Telemetry opt-out**: Telemetry collection is now enabled by default for new users (disabled in CI environments)[1]. Disable with `npx cap telemetry off`[1]. ## Plugin Development Changes - **New success/error handling**: Plugin methods must use `resolve()`/`reject()` instead of deprecated `success()`/`error()`[7]. - **Removed helper properties**: Replace `platform` with `getPlatform()` and `isNative` with `isNativePlatform()`[7]. - **Android fragment changes**: The `BridgeFragment` class was removed - plugins needing fragment support must implement custom solutions[7]. ## Platform-Specific Updates **Android** - Minimum SDK bumped from API 16 to 19[2][10] - Requires Android 15 compatibility[5] **iOS** - Full Swift Package Manager (SPM) support replaces experimental implementation from v6[4][7] - Requires iOS 18 compatibility[5] ## Dependency Modernization - All official plugins updated for Android 15 and iOS 18 compatibility[5] - BillingClient 6+ required for Android IAP implementations (RevenueCat example shows API changes like ReplacementMode)[2] For most projects, upgrading involves: 1. Updating Node.js to v20+ 2. Removing deprecated config options 3. Migrating plugin success/error handlers 4. Testing Android/iOS build configurations with new SDK requirements Developers should consult plugin-specific changelogs (like RevenueCat's v10 migration[2]) and Ionic's official migration guide[5] for detailed workflows. Citations: - 1: https://capacitorjs.com/docs/updating/7-0 - 2: https://github.com/RevenueCat/purchases-capacitor/blob/main/CHANGELOG.md - 3: https://github.com/ionic-team/capacitor/blob/main/CHANGELOG.md - 4: https://dev.to/danielsogl/diving-into-capacitor-6-whats-new-whats-improved-and-how-to-upgrade-e93 - 5: https://ionic.io/blog/capacitor-7-has-hit-ga - 6: https://capacitorjs.com/docs/updating/6-0 - 7: https://capacitorjs.com/docs/updating/plugins/7-0 - 8: https://ionicframework.com/docs/updating/7-0 - 9: https://github.com/ionic-team/capacitor/releases - 10: https://ionic.io/docs/auth-connect/upgrading
Major Version Upgrade Confirmed: Breaking Changes Accurately Reflected
The version bump to 7.0.0, along with the updates to Capacitor dependencies and peer dependency requirements (≥7.0.0), aligns well with the documented breaking changes in Capacitor 7.0. As highlighted by the web query, Capacitor 7.0 introduces several breaking changes—including configuration, environment, and plugin development modifications—that require careful attention when upgrading consuming applications.
- Version updated to 7.0.0.
- Capacitor dependencies now target v7, corresponding with removal of certain legacy configurations and increased requirements (e.g., Node.js 20+).
- Peer dependency requirements updated to “>=7.0.0” ensure compatibility with the new version.
These changes adequately prepare consuming applications for the breaking changes introduced in Capacitor 7.0.
|
New, updated, and removed dependencies detected. Learn more about Socket for GitHub ↗︎
|
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (3)
android/src/main/java/ee/forgr/capacitor_updater/CapacitorUpdaterPlugin.java (2)
309-320: Fix syntax error instartNewThreadmethod.There's a syntax error in the code block. The closing parenthesis should be a closing brace.
Apply this diff to fix the syntax:
- try { - if (waitTime.longValue() > 0) { - Thread.sleep(waitTime.longValue()); - } - function.run(); - } catch (Exception e) { - e.printStackTrace(); - } - }); - bgTask.start(); - return bgTask; - } + try { + if (waitTime.longValue() > 0) { + Thread.sleep(waitTime.longValue()); + } + function.run(); + } catch (Exception e) { + e.printStackTrace(); + } + } + bgTask.start(); + return bgTask; + }🧰 Tools
🪛 GitHub Actions: autofix.ci
[error] 317-317: Sad sad panda, parsing errors detected in line: 317, column: 10! Expecting --> '}' <-- but found --> ')' <--!
🪛 GitHub Actions: Build source code and test it
[error] 317-317: Sad sad panda, parsing errors detected in line: 317, column: 10! Expecting --> '}' <-- but found --> ')' <--!
1213-1220: Enhance URL validation for better security.The current URL validation is basic and might miss malformed URLs that could cause issues. Consider enhancing it to:
- Validate the scheme (https)
- Check for allowed domains
- Validate URL format more thoroughly
Example improvement:
private boolean isValidURL(String urlStr) { try { - new URL(urlStr); - return true; + URL url = new URL(urlStr); + String protocol = url.getProtocol().toLowerCase(); + // Ensure HTTPS for security + if (!protocol.equals("https")) { + return false; + } + // Validate against allowed domains + String host = url.getHost().toLowerCase(); + return isAllowedDomain(host); } catch (MalformedURLException e) { return false; } } + + private boolean isAllowedDomain(String host) { + // Add your allowed domains here + return host.endsWith(".capgo.app") || host.equals("plugin.capgo.app"); + }ios/Plugin/CryptoCipherV2.swift (1)
73-99:🧩 Analysis chain
Verify RSA padding configuration.
The
decryptmethod accepts a padding parameter without validation. Consider enforcing OAEP padding for better security.
🏁 Script executed:
#!/bin/bash # Search for RSA padding usage across the codebase rg -A 5 "SecPadding|RSAEncryptionOAEPSHA|padding.*RSA"Length of output: 1097
RSAPublicKey Review: Padding and Attribution Concerns
• The decryption function passes a default instance using SecPadding() without explicitly enforcing OAEP padding. This could pose a security risk if the underlying implementation does not default to a secure mode. Consider explicitly specifying and validating the padding (e.g., using OAEP) for enhanced security.
• The code includes segments copied from an external source, but while attribution is mentioned, there isn’t a clear verification of the accompanying license. Please confirm that the external code is used in compliance with its license and add appropriate documentation.
🧹 Nitpick comments (5)
android/src/main/java/ee/forgr/capacitor_updater/CapacitorUpdaterPlugin.java (1)
1222-1244: Consider more specific exception handling in background tasks.While the error handling is good, consider catching and handling specific exceptions separately instead of using a generic Exception catch block. This would allow for more targeted error recovery strategies.
Example improvement:
- } catch (final Exception e) { + } catch (final IOException e) { + // Handle I/O related errors + Log.e(CapacitorUpdater.TAG, "I/O error during background task", e); + } catch (final SecurityException e) { + // Handle security-related errors + Log.e(CapacitorUpdater.TAG, "Security error during background task", e); + } catch (final Exception e) { + // Handle other unexpected errors + Log.e(CapacitorUpdater.TAG, "Unexpected error during background task", e);ios/Plugin/CryptoCipherV2.swift (1)
219-240: Enhance error handling in decryptChecksum.Consider adding error handling for:
- Invalid base64 encoding
- Empty or malformed checksum
- Memory allocation failures
public static func decryptChecksum(checksum: String, publicKey: String, version: String) throws -> String { + guard !checksum.isEmpty else { + throw CustomError.cannotDecode + } + guard let checksumBytes: Data = Data(base64Encoded: checksum) else { + throw CustomError.cannotDecode + } - let checksumBytes: Data = Data(base64Encoded: checksum)! if publicKey.isEmpty { return checksum }ios/Plugin/CapacitorUpdaterPlugin.swift (1)
808-809: Add detailed logging for checksum verification.Consider adding more detailed logging to help diagnose checksum verification failures:
res.checksum = try self.implementation.decryptChecksum(checksum: res.checksum, version: latestVersionName) + print("\(CapacitorUpdater.TAG) Checksum verification - Expected: \(res.checksum), Actual: \(next.getChecksum())") if res.checksum != "" && next.getChecksum() != res.checksum && res.manifest == nil {ios/Plugin/CapacitorUpdater.swift (1)
686-687: Add progress reporting for large files.Consider adding progress reporting for large file checksum calculations to improve user experience.
+ private func notifyProgress(_ progress: Double) { + // Notify progress through delegate/callback + } checksum = self.calcChecksumV2(filePath: finalPath) + print("\(CapacitorUpdater.TAG) Checksum calculation complete")🧰 Tools
🪛 GitHub Actions: Build source code and test it
[error] 686-686: value of type 'CapacitorUpdater' has no member 'calcChecksumV2'
[error] 687-687: static member 'TAG' cannot be used on instance of type 'CapacitorUpdater'
android/src/main/java/ee/forgr/capacitor_updater/CapacitorUpdater.java (1)
764-823: Add input validation and improve error messages.Consider adding:
- File size validation
- More descriptive error messages
- Memory usage checks
private void decryptFileV2( final File file, final String ivSessionKey, final String version ) throws IOException { + if (file.length() > MAX_FILE_SIZE) { + throw new IOException("File too large: " + file.length()); + } if ( this.publicKey.isEmpty() || ivSessionKey == null ||
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
CapgoCapacitorUpdater.podspec(1 hunks)Package.swift(1 hunks)README.md(0 hunks)android/build.gradle(3 hunks)android/src/main/java/ee/forgr/capacitor_updater/CapacitorUpdater.java(1 hunks)android/src/main/java/ee/forgr/capacitor_updater/CapacitorUpdaterPlugin.java(1 hunks)ios/Plugin.xcodeproj/project.pbxproj(4 hunks)ios/Plugin/CapacitorUpdater.swift(5 hunks)ios/Plugin/CapacitorUpdaterPlugin.swift(2 hunks)ios/Plugin/CryptoCipherV2.swift(1 hunks)ios/Podfile(1 hunks)package.json(2 hunks)src/definitions.ts(0 hunks)
💤 Files with no reviewable changes (2)
- src/definitions.ts
- README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- CapgoCapacitorUpdater.podspec
- ios/Podfile
- ios/Plugin.xcodeproj/project.pbxproj
- Package.swift
- android/build.gradle
🧰 Additional context used
🪛 GitHub Actions: autofix.ci
android/src/main/java/ee/forgr/capacitor_updater/CapacitorUpdaterPlugin.java
[error] 317-317: Sad sad panda, parsing errors detected in line: 317, column: 10! Expecting --> '}' <-- but found --> ')' <--!
android/src/main/java/ee/forgr/capacitor_updater/CapacitorUpdater.java
[error] 500-500: Sad sad panda, parsing errors detected in line: 500, column: 3! Expecting: one of these possible Token sequences.
🪛 GitHub Actions: Build source code and test it
android/src/main/java/ee/forgr/capacitor_updater/CapacitorUpdaterPlugin.java
[error] 317-317: Sad sad panda, parsing errors detected in line: 317, column: 10! Expecting --> '}' <-- but found --> ')' <--!
ios/Plugin/CapacitorUpdater.swift
[error] 145-145: static member 'TAG' cannot be used on instance of type 'CapacitorUpdater'
[error] 181-181: static member 'TAG' cannot be used on instance of type 'CapacitorUpdater'
[error] 366-366: static member 'TAG' cannot be used on instance of type 'CapacitorUpdater'
[error] 401-401: value of type 'CapacitorUpdater' has no member 'hasOldPrivateKeyPropertyInConfig'
[error] 421-421: cannot find 'verifyChecksum' in scope
[error] 427-427: cannot find 'verifyChecksum' in scope
[error] 473-473: value of type 'CapacitorUpdater' has no member 'hasOldPrivateKeyPropertyInConfig'
[warning] 533-533: initialization of 'UnsafeMutablePointer' results in a dangling pointer
[warning] 579-579: initialization of 'UnsafeMutablePointer' results in a dangling pointer
[error] 686-686: value of type 'CapacitorUpdater' has no member 'calcChecksumV2'
[error] 687-687: static member 'TAG' cannot be used on instance of type 'CapacitorUpdater'
android/src/main/java/ee/forgr/capacitor_updater/CapacitorUpdater.java
[error] 500-500: Sad sad panda, parsing errors detected in line: 500, column: 3! Expecting: one of these possible Token sequences.
🔇 Additional comments (6)
android/src/main/java/ee/forgr/capacitor_updater/CapacitorUpdaterPlugin.java (1)
234-268: Good use of Phaser for flexible synchronization.The switch from CountDownLatch to Phaser is a good improvement as it:
- Allows dynamic registration/deregistration of parties
- Provides better control over synchronization phases
- Supports reusability across multiple synchronization points
The implementation with
semaphoreWait,semaphoreUp, andsemaphoreDownmethods is clean and well-documented.ios/Plugin/CryptoCipherV2.swift (2)
23-68: LGTM! Secure AES implementation.The AES128Key struct follows cryptographic best practices:
- Proper separation of IV and key
- Secure CBC mode with PKCS7 padding
- Safe memory handling
- Controlled debug access
126-127: Verify license compatibility.The code is copied from an external source. Please verify the license compatibility with the project's MPL 2.0 license.
✅ Verification successful
🌐 Web query:
What is the license of swiftutils library by btnguyen2k?💡 Result:
The `swiftutils` library by btnguyen2k uses a **permissive open-source license** with an explicit disclaimer of warranties. According to its GitHub repository[3], the license details are specified in `LICENSE.txt`, though third-party libraries included may have their own licenses[3]. From related code in its predecessor project `swift-rsautils` (now merged into `swiftutils`), the core terms include: - **"AS IS"** provision without warranties[1] - **No liability** for damages arising from use[1] - Full disclaimer of merchantability/fitness for purpose[1] This aligns with BSD/MIT-style licensing but lacks explicit naming. Users should review `LICENSE.txt` in the repository for exact terms and verify third-party dependencies' licenses separately[3]. Citations: - 1: https://github.com/btnguyen2k/swift-rsautils - 2: https://forums.developer.apple.com/forums/thread/683426 - 3: https://github.com/btnguyen2k/swiftutils
License Compatibility Verified for Copied Code
- The swiftutils library by btnguyen2k is released under a permissive open-source license (BSD/MIT-style) with explicit warranty disclaimers.
- Such permissive licenses are generally compatible with the MPL 2.0, provided that proper attribution and the original license notices are retained.
- The existing in-code attribution (link to the swiftutils source) is a good practice, but ensure that any required license text or notices are included as mandated by the permissive license.
package.json (3)
3-3: Major Version Bump VerificationThe version has been updated to "7.0.0", reflecting the intentional breaking changes and alignment with the new feature set (e.g., removal of legacy options). Please ensure that corresponding documentation and changelogs are updated accordingly.
61-78: Dependency Updates and Consistency CheckAll updated dependencies in the devDependencies block (e.g., "@capacitor/android", "@capacitor/cli", "@capacitor/core", "@capacitor/ios", "@types/node", "eslint", "prettier-plugin-java", "rollup", "typescript") now target versions aligned with the new Capacitor v7 requirements. Also, the addition of "husky" at "^9.1.7" is clearly meant to support new commit hook workflows.
Note: The ESLint version was modified from "^8.57.1" to "^8.57.0" (a minor patch-level adjustment). Please confirm if this downgrade was intentional or if it warrants aligning to the higher patch version.
81-81: Peer Dependency Update for Breaking ChangeThe peer dependency constraint for "@capacitor/core" has been updated to ">=7.0.0". This ensures that consumers of the package are now required to use Capacitor v7 or above, which is consistent with the overall upgrade strategy. It is important to communicate this breaking change clearly to users during the upgrade.
| checksum = try self.implementation.decryptChecksum(checksum: checksum, version: version) | ||
| if (checksum != "" || self.implementation.publicKey != "") && next.getChecksum() != checksum { |
There was a problem hiding this comment.
Fix static member access.
Replace instance member access with static member access for TAG:
- checksum = try self.implementation.decryptChecksum(checksum: checksum, version: version)
- if (checksum != "" || self.implementation.publicKey != "") && next.getChecksum() != checksum {
- print("\(CapacitorUpdater.TAG) Error checksum", next.getChecksum(), checksum)
+ checksum = try implementation.decryptChecksum(checksum: checksum, version: version)
+ if (checksum != "" || implementation.publicKey != "") && next.getChecksum() != checksum {
+ print("\(CapacitorUpdater.TAG) Error checksum", next.getChecksum(), checksum)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| checksum = try self.implementation.decryptChecksum(checksum: checksum, version: version) | |
| if (checksum != "" || self.implementation.publicKey != "") && next.getChecksum() != checksum { | |
| checksum = try implementation.decryptChecksum(checksum: checksum, version: version) | |
| if (checksum != "" || implementation.publicKey != "") && next.getChecksum() != checksum { | |
| print("\(CapacitorUpdater.TAG) Error checksum", next.getChecksum(), checksum) |
ios/Plugin/CapacitorUpdater.swift
Outdated
| private func decryptFileV2(filePath: URL, sessionKey: String, version: String) throws { | ||
| if self.publicKey.isEmpty || sessionKey.isEmpty || sessionKey.components(separatedBy: ":").count != 2 { | ||
| print("\(self.TAG) Cannot find public key or sessionKey") | ||
| return | ||
| } | ||
| do { | ||
| guard let rsaPublicKey: RSAPublicKey = .load(rsaPublicKey: self.publicKey) else { | ||
| print("cannot decode publicKey", self.publicKey) | ||
| throw CustomError.cannotDecode | ||
| } | ||
|
|
||
| let sessionKeyArray: [String] = sessionKey.components(separatedBy: ":") | ||
| guard let ivData: Data = Data(base64Encoded: sessionKeyArray[0]) else { | ||
| print("cannot decode sessionKey", sessionKey) | ||
| throw CustomError.cannotDecode | ||
| } | ||
|
|
||
| guard let sessionKeyDataEncrypted = Data(base64Encoded: sessionKeyArray[1]) else { | ||
| throw NSError(domain: "Invalid session key data", code: 1, userInfo: nil) | ||
| } | ||
|
|
||
| guard let sessionKeyDataDecrypted = rsaPublicKey.decrypt(data: sessionKeyDataEncrypted) else { | ||
| throw NSError(domain: "Failed to decrypt session key data", code: 2, userInfo: nil) | ||
| } | ||
|
|
||
| let aesPrivateKey = AES128Key(iv: ivData, aes128Key: sessionKeyDataDecrypted) | ||
|
|
||
| guard let encryptedData = try? Data(contentsOf: filePath) else { | ||
| throw NSError(domain: "Failed to read encrypted data", code: 3, userInfo: nil) | ||
| } | ||
|
|
||
| guard let decryptedData = aesPrivateKey.decrypt(data: encryptedData) else { | ||
| throw NSError(domain: "Failed to decrypt data", code: 4, userInfo: nil) | ||
| } | ||
|
|
||
| try decryptedData.write(to: filePath) | ||
|
|
||
| } catch { | ||
| print("\(self.TAG) Cannot decode: \(filePath.path)", error) | ||
| self.sendStats(action: "decrypt_fail", versionName: version) | ||
| throw CustomError.cannotDecode | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix static member access and enhance error handling.
- Fix static member access for TAG
- Add additional error handling for file operations
- print("\(self.TAG) Cannot find public key or sessionKey")
+ print("\(CapacitorUpdater.TAG) Cannot find public key or sessionKey")
return
}
do {
+ guard FileManager.default.fileExists(atPath: filePath.path) else {
+ throw CustomError.cannotDecode
+ }
guard let rsaPublicKey: RSAPublicKey = .load(rsaPublicKey: self.publicKey) else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private func decryptFileV2(filePath: URL, sessionKey: String, version: String) throws { | |
| if self.publicKey.isEmpty || sessionKey.isEmpty || sessionKey.components(separatedBy: ":").count != 2 { | |
| print("\(self.TAG) Cannot find public key or sessionKey") | |
| return | |
| } | |
| do { | |
| guard let rsaPublicKey: RSAPublicKey = .load(rsaPublicKey: self.publicKey) else { | |
| print("cannot decode publicKey", self.publicKey) | |
| throw CustomError.cannotDecode | |
| } | |
| let sessionKeyArray: [String] = sessionKey.components(separatedBy: ":") | |
| guard let ivData: Data = Data(base64Encoded: sessionKeyArray[0]) else { | |
| print("cannot decode sessionKey", sessionKey) | |
| throw CustomError.cannotDecode | |
| } | |
| guard let sessionKeyDataEncrypted = Data(base64Encoded: sessionKeyArray[1]) else { | |
| throw NSError(domain: "Invalid session key data", code: 1, userInfo: nil) | |
| } | |
| guard let sessionKeyDataDecrypted = rsaPublicKey.decrypt(data: sessionKeyDataEncrypted) else { | |
| throw NSError(domain: "Failed to decrypt session key data", code: 2, userInfo: nil) | |
| } | |
| let aesPrivateKey = AES128Key(iv: ivData, aes128Key: sessionKeyDataDecrypted) | |
| guard let encryptedData = try? Data(contentsOf: filePath) else { | |
| throw NSError(domain: "Failed to read encrypted data", code: 3, userInfo: nil) | |
| } | |
| guard let decryptedData = aesPrivateKey.decrypt(data: encryptedData) else { | |
| throw NSError(domain: "Failed to decrypt data", code: 4, userInfo: nil) | |
| } | |
| try decryptedData.write(to: filePath) | |
| } catch { | |
| print("\(self.TAG) Cannot decode: \(filePath.path)", error) | |
| self.sendStats(action: "decrypt_fail", versionName: version) | |
| throw CustomError.cannotDecode | |
| } | |
| } | |
| private func decryptFileV2(filePath: URL, sessionKey: String, version: String) throws { | |
| if self.publicKey.isEmpty || sessionKey.isEmpty || sessionKey.components(separatedBy: ":").count != 2 { | |
| print("\(CapacitorUpdater.TAG) Cannot find public key or sessionKey") | |
| return | |
| } | |
| do { | |
| guard FileManager.default.fileExists(atPath: filePath.path) else { | |
| throw CustomError.cannotDecode | |
| } | |
| guard let rsaPublicKey: RSAPublicKey = .load(rsaPublicKey: self.publicKey) else { | |
| print("cannot decode publicKey", self.publicKey) | |
| throw CustomError.cannotDecode | |
| } | |
| let sessionKeyArray: [String] = sessionKey.components(separatedBy: ":") | |
| guard let ivData: Data = Data(base64Encoded: sessionKeyArray[0]) else { | |
| print("cannot decode sessionKey", sessionKey) | |
| throw CustomError.cannotDecode | |
| } | |
| guard let sessionKeyDataEncrypted = Data(base64Encoded: sessionKeyArray[1]) else { | |
| throw NSError(domain: "Invalid session key data", code: 1, userInfo: nil) | |
| } | |
| guard let sessionKeyDataDecrypted = rsaPublicKey.decrypt(data: sessionKeyDataEncrypted) else { | |
| throw NSError(domain: "Failed to decrypt session key data", code: 2, userInfo: nil) | |
| } | |
| let aesPrivateKey = AES128Key(iv: ivData, aes128Key: sessionKeyDataDecrypted) | |
| guard let encryptedData = try? Data(contentsOf: filePath) else { | |
| throw NSError(domain: "Failed to read encrypted data", code: 3, userInfo: nil) | |
| } | |
| guard let decryptedData = aesPrivateKey.decrypt(data: encryptedData) else { | |
| throw NSError(domain: "Failed to decrypt data", code: 4, userInfo: nil) | |
| } | |
| try decryptedData.write(to: filePath) | |
| } catch { | |
| print("\(self.TAG) Cannot decode: \(filePath.path)", error) | |
| self.sendStats(action: "decrypt_fail", versionName: version) | |
| throw CustomError.cannotDecode | |
| } | |
| } |
🧰 Tools
🪛 GitHub Actions: Build source code and test it
[error] 145-145: static member 'TAG' cannot be used on instance of type 'CapacitorUpdater'
[error] 181-181: static member 'TAG' cannot be used on instance of type 'CapacitorUpdater'
| private String decryptChecksum(String checksum, String version) | ||
| throws IOException { | ||
| if (this.publicKey.isEmpty()) { | ||
| Log.e(CapacitorUpdater.TAG, "The public key is empty"); | ||
| return checksum; | ||
| } | ||
| try { | ||
| byte[] checksumBytes = Base64.decode(checksum, Base64.DEFAULT); | ||
| PublicKey pKey = CryptoCipherV2.stringToPublicKey(this.publicKey); | ||
| byte[] decryptedChecksum = CryptoCipherV2.decryptRSA(checksumBytes, pKey); | ||
| // return Base64.encodeToString(decryptedChecksum, Base64.DEFAULT); | ||
| String result = Base64.encodeToString(decryptedChecksum, Base64.DEFAULT); | ||
| return result.replaceAll("\\s", ""); // Remove all whitespace, including newlines | ||
| } catch (GeneralSecurityException e) { | ||
| Log.e(TAG, "decryptChecksum fail: " + e.getMessage()); | ||
| this.sendStats("decrypt_fail", version); | ||
| throw new IOException("Decryption failed: " + e.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix parsing errors and enhance error handling.
- Fix the parsing error by correcting the method indentation
- Add proper error handling for Base64 decoding
- private String decryptChecksum(String checksum, String version)
- throws IOException {
- if (this.publicKey.isEmpty()) {
- Log.e(CapacitorUpdater.TAG, "The public key is empty");
- return checksum;
- }
+ private String decryptChecksum(String checksum, String version) throws IOException {
+ if (this.publicKey.isEmpty()) {
+ Log.e(CapacitorUpdater.TAG, "The public key is empty");
+ return checksum;
+ }
+ if (checksum == null || checksum.isEmpty()) {
+ throw new IOException("Invalid checksum");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private String decryptChecksum(String checksum, String version) | |
| throws IOException { | |
| if (this.publicKey.isEmpty()) { | |
| Log.e(CapacitorUpdater.TAG, "The public key is empty"); | |
| return checksum; | |
| } | |
| try { | |
| byte[] checksumBytes = Base64.decode(checksum, Base64.DEFAULT); | |
| PublicKey pKey = CryptoCipherV2.stringToPublicKey(this.publicKey); | |
| byte[] decryptedChecksum = CryptoCipherV2.decryptRSA(checksumBytes, pKey); | |
| // return Base64.encodeToString(decryptedChecksum, Base64.DEFAULT); | |
| String result = Base64.encodeToString(decryptedChecksum, Base64.DEFAULT); | |
| return result.replaceAll("\\s", ""); // Remove all whitespace, including newlines | |
| } catch (GeneralSecurityException e) { | |
| Log.e(TAG, "decryptChecksum fail: " + e.getMessage()); | |
| this.sendStats("decrypt_fail", version); | |
| throw new IOException("Decryption failed: " + e.getMessage()); | |
| } | |
| } | |
| private String decryptChecksum(String checksum, String version) throws IOException { | |
| if (this.publicKey.isEmpty()) { | |
| Log.e(CapacitorUpdater.TAG, "The public key is empty"); | |
| return checksum; | |
| } | |
| if (checksum == null || checksum.isEmpty()) { | |
| throw new IOException("Invalid checksum"); | |
| } | |
| try { | |
| byte[] checksumBytes = Base64.decode(checksum, Base64.DEFAULT); | |
| PublicKey pKey = CryptoCipherV2.stringToPublicKey(this.publicKey); | |
| byte[] decryptedChecksum = CryptoCipherV2.decryptRSA(checksumBytes, pKey); | |
| // return Base64.encodeToString(decryptedChecksum, Base64.DEFAULT); | |
| String result = Base64.encodeToString(decryptedChecksum, Base64.DEFAULT); | |
| return result.replaceAll("\\s", ""); // Remove all whitespace, including newlines | |
| } catch (GeneralSecurityException e) { | |
| Log.e(TAG, "decryptChecksum fail: " + e.getMessage()); | |
| this.sendStats("decrypt_fail", version); | |
| throw new IOException("Decryption failed: " + e.getMessage()); | |
| } | |
| } |
🧰 Tools
🪛 GitHub Actions: autofix.ci
[error] 500-500: Sad sad panda, parsing errors detected in line: 500, column: 3! Expecting: one of these possible Token sequences.
🪛 GitHub Actions: Build source code and test it
[error] 500-500: Sad sad panda, parsing errors detected in line: 500, column: 3! Expecting: one of these possible Token sequences.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
ios/Plugin/CapacitorUpdater.swift (3)
356-375: Improve error handling in checksum decryption.The checksum decryption implementation could be improved with better error handling and logging.
Apply this diff to improve the implementation:
public func decryptChecksum(checksum: String, version: String) throws -> String { if self.publicKey.isEmpty { return checksum } do { + print("\(CapacitorUpdater.TAG) Starting checksum decryption for version \(version)") + let checksumBytes: Data = Data(base64Encoded: checksum)! guard let rsaPublicKey: RSAPublicKey = .load(rsaPublicKey: self.publicKey) else { print("cannot decode publicKey", self.publicKey) throw CustomError.cannotDecode } guard let decryptedChecksum = rsaPublicKey.decrypt(data: checksumBytes) else { - throw NSError(domain: "Failed to decrypt session key data", code: 2, userInfo: nil) + throw CustomError.checksumDecryptionFailed } return decryptedChecksum.base64EncodedString() } catch { print("\(CapacitorUpdater.TAG) Cannot decrypt checksum: \(checksum)", error) self.sendStats(action: "decrypt_fail", versionName: version) throw CustomError.cannotDecode } }
623-681: Improve download error handling and progress tracking.The download implementation could be enhanced with better error handling and progress tracking:
- Add retry logic for failed downloads
- Improve progress tracking granularity
- Add timeout handling
Apply this diff to improve the implementation:
let request = session.streamRequest(url, headers: requestHeaders).validate().onHTTPResponse(perform: { response in if let contentLength = response.headers.value(for: "Content-Length") { targetSize = (Int(contentLength) ?? -1) + Int(totalReceivedBytes) } + // Validate response status code + guard (200...299).contains(response.statusCode) else { + mainError = NSError(domain: "HTTPError", code: response.statusCode, userInfo: nil) + semaphore.signal() + return + } }).responseStream { [weak self] streamResponse in guard let self = self else { return } switch streamResponse.event { case .stream(let result): if case .success(let data) = result { self.tempData.append(data) self.savePartialData(startingAt: UInt64(totalReceivedBytes)) totalReceivedBytes += Int64(data.count) - let percent = max(10, Int((Double(totalReceivedBytes) / Double(targetSize)) * 70.0)) + // More granular progress tracking + let percent = max(10, min(70, Int((Double(totalReceivedBytes) / Double(targetSize)) * 70.0))) + self.notifyDownload(id: id, percent: percent) - let currentMilestone = (percent / 10) * 10 - if currentMilestone > lastSentProgress && currentMilestone <= 70 { - for milestone in stride(from: lastSentProgress + 10, through: currentMilestone, by: 10) { - self.notifyDownload(id: id, percent: milestone, ignoreMultipleOfTen: false) - } - lastSentProgress = currentMilestone - } } else { print("\(CapacitorUpdater.TAG) Download failed") + // Add retry logic + if let error = result.error, (error as NSError).code == NSURLErrorTimedOut { + mainError = error as NSError + semaphore.signal() + } }
477-484: Enhance checksum verification robustness.The checksum verification could be improved with better error handling and validation:
- Add file existence check
- Add file size validation
- Improve error messages
Apply this diff to improve the implementation:
if !self.publicKey.isEmpty && !sessionKey.isEmpty { + guard FileManager.default.fileExists(atPath: destFilePath.path) else { + throw CustomError.fileNotFound + } + + // Validate file size + let fileAttributes = try FileManager.default.attributesOfItem(atPath: destFilePath.path) + guard let fileSize = fileAttributes[.size] as? NSNumber, fileSize.int64Value > 0 else { + throw CustomError.invalidFileSize + } + // assume that calcChecksum != null let calculatedChecksum = CryptoCipherV2.calcChecksum(filePath: destFilePath) if calculatedChecksum != fileHash { - throw NSError(domain: "ChecksumError", code: 1, userInfo: [NSLocalizedDescriptionKey: "Computed checksum is not equal to required checksum (\(calculatedChecksum) != \(fileHash))"]) + throw CustomError.checksumMismatch(calculated: calculatedChecksum, expected: fileHash) } }
| private func decryptFileV2(filePath: URL, sessionKey: String, version: String) throws { | ||
| if self.publicKey.isEmpty || sessionKey.isEmpty || sessionKey.components(separatedBy: ":").count != 2 { | ||
| print("\(CapacitorUpdater.TAG) Cannot find public key or sessionKey") | ||
| return | ||
| } | ||
| do { | ||
| guard let rsaPublicKey: RSAPublicKey = .load(rsaPublicKey: self.publicKey) else { | ||
| print("cannot decode publicKey", self.publicKey) | ||
| throw CustomError.cannotDecode | ||
| } | ||
|
|
||
| let sessionKeyArray: [String] = sessionKey.components(separatedBy: ":") | ||
| guard let ivData: Data = Data(base64Encoded: sessionKeyArray[0]) else { | ||
| print("cannot decode sessionKey", sessionKey) | ||
| throw CustomError.cannotDecode | ||
| } | ||
|
|
||
| guard let sessionKeyDataEncrypted = Data(base64Encoded: sessionKeyArray[1]) else { | ||
| throw NSError(domain: "Invalid session key data", code: 1, userInfo: nil) | ||
| } | ||
|
|
||
| guard let sessionKeyDataDecrypted = rsaPublicKey.decrypt(data: sessionKeyDataEncrypted) else { | ||
| throw NSError(domain: "Failed to decrypt session key data", code: 2, userInfo: nil) | ||
| } | ||
|
|
||
| let aesPrivateKey = AES128Key(iv: ivData, aes128Key: sessionKeyDataDecrypted) | ||
|
|
||
| guard let encryptedData = try? Data(contentsOf: filePath) else { | ||
| throw NSError(domain: "Failed to read encrypted data", code: 3, userInfo: nil) | ||
| } | ||
|
|
||
| guard let decryptedData = aesPrivateKey.decrypt(data: encryptedData) else { | ||
| throw NSError(domain: "Failed to decrypt data", code: 4, userInfo: nil) | ||
| } | ||
|
|
||
| try decryptedData.write(to: filePath) | ||
|
|
||
| } catch { | ||
| print("\(CapacitorUpdater.TAG) Cannot decode: \(filePath.path)", error) | ||
| self.sendStats(action: "decrypt_fail", versionName: version) | ||
| throw CustomError.cannotDecode | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and memory management in decryption.
The decryption implementation could be improved in several areas:
- Use custom error types instead of generic
NSErrorfor better error handling - Clear sensitive data from memory after use
- Add debug logging for troubleshooting
Apply this diff to improve the implementation:
private func decryptFileV2(filePath: URL, sessionKey: String, version: String) throws {
if self.publicKey.isEmpty || sessionKey.isEmpty || sessionKey.components(separatedBy: ":").count != 2 {
print("\(CapacitorUpdater.TAG) Cannot find public key or sessionKey")
return
}
do {
+ // Debug logging
+ print("\(CapacitorUpdater.TAG) Starting decryption for version \(version)")
+
guard let rsaPublicKey: RSAPublicKey = .load(rsaPublicKey: self.publicKey) else {
print("cannot decode publicKey", self.publicKey)
throw CustomError.cannotDecode
}
let sessionKeyArray: String] = sessionKey.components(separatedBy: ":")
guard let ivData: Data = Data(base64Encoded: sessionKeyArray[0]) else {
print("cannot decode sessionKey", sessionKey)
throw CustomError.cannotDecode
}
guard let sessionKeyDataEncrypted = Data(base64Encoded: sessionKeyArray[1]) else {
- throw NSError(domain: "Invalid session key data", code: 1, userInfo: nil)
+ throw CustomError.invalidSessionKey
}
guard let sessionKeyDataDecrypted = rsaPublicKey.decrypt(data: sessionKeyDataEncrypted) else {
- throw NSError(domain: "Failed to decrypt session key data", code: 2, userInfo: nil)
+ throw CustomError.sessionKeyDecryptionFailed
}
let aesPrivateKey = AES128Key(iv: ivData, aes128Key: sessionKeyDataDecrypted)
guard let encryptedData = try? Data(contentsOf: filePath) else {
- throw NSError(domain: "Failed to read encrypted data", code: 3, userInfo: nil)
+ throw CustomError.fileReadError
}
guard let decryptedData = aesPrivateKey.decrypt(data: encryptedData) else {
- throw NSError(domain: "Failed to decrypt data", code: 4, userInfo: nil)
+ throw CustomError.dataDecryptionFailed
}
try decryptedData.write(to: filePath)
+
+ // Clear sensitive data from memory
+ sessionKeyDataDecrypted.resetBytes(in: 0..<sessionKeyDataDecrypted.count)
+ decryptedData.resetBytes(in: 0..<decryptedData.count)
} catch {
print("\(CapacitorUpdater.TAG) Cannot decode: \(filePath.path)", error)
self.sendStats(action: "decrypt_fail", versionName: version)
throw CustomError.cannotDecode
}
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
android/src/main/java/ee/forgr/capacitor_updater/CapacitorUpdater.java (2)
425-428: Consider adding synchronization for version conflict check.The version conflict check might have a race condition if multiple downloads are initiated simultaneously. Consider adding synchronization or using atomic operations.
- if (this.activity != null && DownloadWorkerManager.isVersionDownloading(version)) { - Log.i(TAG, "Version already downloading: " + version); - return; - } + synchronized(this) { + if (this.activity != null && DownloadWorkerManager.isVersionDownloading(version)) { + Log.i(TAG, "Version already downloading: " + version); + return; + } + // Start download here to ensure atomicity + this.saveBundleInfo(id, new BundleInfo(id, version, BundleStatus.DOWNLOADING, new Date(System.currentTimeMillis()), "")); + }
51-51: Consider breaking down the CapacitorUpdater class for better maintainability.The class has grown to handle multiple responsibilities including:
- Cryptographic operations
- Download management
- Bundle management
- Stats tracking
Consider extracting these into separate classes following the Single Responsibility Principle:
CryptoManager: Handle all cryptographic operationsDownloadManager: Manage download operationsBundleManager: Handle bundle state and operationsStatsTracker: Handle stats collection and reporting
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
android/src/main/java/ee/forgr/capacitor_updater/CapacitorUpdater.java(1 hunks)android/src/main/java/ee/forgr/capacitor_updater/CapacitorUpdaterPlugin.java(0 hunks)
💤 Files with no reviewable changes (1)
- android/src/main/java/ee/forgr/capacitor_updater/CapacitorUpdaterPlugin.java
🔇 Additional comments (2)
android/src/main/java/ee/forgr/capacitor_updater/CapacitorUpdater.java (2)
330-334: LGTM! Improved cryptographic implementation.The code now correctly implements the new cryptographic approach using
CryptoCipherV2for both file and checksum decryption. The sequence of operations (decrypt file → decrypt checksum → verify) is logically sound.
335-339: LGTM! Robust error handling for checksum validation.The implementation includes:
- Proper validation of checksum when either checksumDecrypted is non-empty or publicKey is present
- Appropriate error logging with detailed context
- Stats tracking for checksum failures
- Clean error propagation
|
Summary by CodeRabbit