fix(mobile): make secure storage fallible and fail-closed - #5923
Open
mortenolsrud wants to merge 2 commits into
Open
fix(mobile): make secure storage fallible and fail-closed#5923mortenolsrud wants to merge 2 commits into
mortenolsrud wants to merge 2 commits into
Conversation
BREAKING CHANGE: SecureSet, SecureGet, and SecureDelete now return errors. SecureSet(key, value string) error SecureGet(key string) (value string, found bool, err error) SecureDelete(key string) error iOS: - Use SecItemUpdate before falling back to SecItemAdd (non-destructive) - Check and propagate all OSStatus values - Distinguish missing key (found=false) from empty value (found=true) Android: - Remove plaintext fallback (wails_secure_plain eliminated) - Require API 23+ for EncryptedSharedPreferences; return error below - Use commit() instead of apply() for durable writes - Use contains() to distinguish missing from empty - Return structured JSON envelope across JNI bridge Desktop stub returns ErrSecureStorageUnsupported for all three methods. Empty key is rejected at the Go layer before crossing any bridge. Delete of a non-existent key succeeds on both platforms. Closes wailsapp#5913 (partially — secure storage portion of the mobile hardening audit)
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughSecure storage now uses error-returning APIs across mobile platforms. Android and iOS return structured JSON results, distinguish missing values, validate keys, and propagate native failures. Unsupported platforms return ChangesSecure storage API contract
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MobileManager
participant PlatformAdapter
participant NativeSecureStorage
participant EventHandler
MobileManager->>PlatformAdapter: SecureSet, SecureGet, or SecureDelete
PlatformAdapter->>NativeSecureStorage: Validated request
NativeSecureStorage-->>PlatformAdapter: JSON result or native error
PlatformAdapter-->>MobileManager: Value, found status, or error
EventHandler-->>MobileManager: common:secureValue or common:secureError
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 1
🧹 Nitpick comments (1)
v3/internal/commands/build_assets/android/app/src/main/java/com/wails/app/WailsBridge.java (1)
748-763: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
securePrefs()recreates the encrypted store on every call in bothWailsBridge.javacopies. Each secure storage operation builds aMasterKeyand callsEncryptedSharedPreferences.create, which performs Android Keystore and Tink keyset work.
v3/internal/commands/build_assets/android/app/src/main/java/com/wails/app/WailsBridge.java#L748-L763: store the createdSharedPreferencesin a field and return the cached instance on later calls, keeping thenullreturn for the unavailable case.v3/examples/mobile/build/android/app/src/main/java/com/wails/app/WailsBridge.java#L748-L763: apply the same caching so the generated example matches the template.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v3/internal/commands/build_assets/android/app/src/main/java/com/wails/app/WailsBridge.java` around lines 748 - 763, Cache the SharedPreferences instance created by securePrefs() in a field, returning the cached value on subsequent calls while preserving the existing null result for unsupported Android versions or initialization failures. Apply the same change at v3/internal/commands/build_assets/android/app/src/main/java/com/wails/app/WailsBridge.java lines 748-763 and v3/examples/mobile/build/android/app/src/main/java/com/wails/app/WailsBridge.java lines 748-763 so both copies remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@v3/internal/commands/build_assets/android/app/src/main/java/com/wails/app/WailsBridge.java`:
- Around line 782-784: In both WailsBridge.java files at
v3/internal/commands/build_assets/android/app/src/main/java/com/wails/app/WailsBridge.java:782-784
and
v3/examples/mobile/build/android/app/src/main/java/com/wails/app/WailsBridge.java:782-784,
add a private secureError(String) helper that builds the error envelope with
JSONObject, safely handling null messages and JSON escaping. Update the catch
blocks in secureSet, secureGet, and secureDelete in both files to call this
helper instead of manually concatenating the message, keeping both copies
identical.
---
Nitpick comments:
In
`@v3/internal/commands/build_assets/android/app/src/main/java/com/wails/app/WailsBridge.java`:
- Around line 748-763: Cache the SharedPreferences instance created by
securePrefs() in a field, returning the cached value on subsequent calls while
preserving the existing null result for unsupported Android versions or
initialization failures. Apply the same change at
v3/internal/commands/build_assets/android/app/src/main/java/com/wails/app/WailsBridge.java
lines 748-763 and
v3/examples/mobile/build/android/app/src/main/java/com/wails/app/WailsBridge.java
lines 748-763 so both copies remain consistent.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d5c1d08-358c-41b3-9c6e-6ff3b11e9d1a
⛔ Files ignored due to path filters (1)
v3/examples/mobile/go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
v3/examples/mobile/build/android/app/src/main/java/com/wails/app/WailsBridge.javav3/examples/mobile/go.modv3/examples/mobile/native_features_android.gov3/examples/mobile/native_features_ios.gov3/internal/commands/build_assets/android/app/src/main/java/com/wails/app/WailsBridge.javav3/pkg/application/application_android_nocgo.gov3/pkg/application/mobile.gov3/pkg/application/mobile_features_android.gov3/pkg/application/mobile_features_ios.gov3/pkg/application/mobile_features_ios.hv3/pkg/application/mobile_features_ios.mv3/pkg/application/mobile_stub.go
Address CodeRabbit review feedback: - Cache EncryptedSharedPreferences instance after first initialization - Add secureError() helper using JSONObject to safely handle null getMessage() and avoid JSON injection from special characters - Replace manual string concatenation in all catch blocks
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.
Description
The mobile secure storage API (
SecureSet,SecureGet,SecureDelete) silently swallows all native failures, cannot distinguish a missing key from an empty stored value, and on Android falls back to plaintext preferences when encrypted storage initialization fails.This PR makes all three methods fallible, removes the plaintext fallback, and fixes the destructive delete-before-add Keychain pattern on iOS.
iOS — non-destructive Keychain updates:
SecItemUpdatefirst, falls back toSecItemAddonly onerrSecItemNotFoundOSStatusvalues are checked and propagated as typed errorsfound=false) from empty stored value (found=true)Android — fail-closed encrypted storage:
wails_secure_plainplaintext fallback entirelyEncryptedSharedPreferences; returns explicit error belowcommit()instead ofapply()for durable, verifiable writescontains(key)to distinguish missing from emptyDesktop stub returns
ErrSecureStorageUnsupported(previously returned apparent success).Empty key is rejected at the Go layer before crossing either native bridge. Delete of a non-existent key succeeds on both platforms.
Type of change
How Has This Been Tested?
ErrSecureStorageUnsupported.android/arm64, CGO_ENABLED=0 nocgo path) — compiles.The change is backwards-compatible for callers that used the methods as standalone statements (fire-and-forget). Callers that captured
SecureGet's return value are the source-breaking change.Test Configuration
Checklist:
Notes:
wails_secure_plainplaintext storage that was used as a silent fallback — a data-at-rest confidentiality issue.Summary by CodeRabbit