fix: encrypt saved FTP/SFTP credentials - #2566
Conversation
Greptile SummaryThe PR moves saved FTP/SFTP configurations from plaintext WebView storage into an encrypted native store.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the native source is now included in the Android plugin, and migration removes the plaintext copy only after commit succeeds and that success propagates through the Cordova promise. Important Files Changed
Sequence DiagramsequenceDiagram
participant App as WebView startup
participant Legacy as localStorage
participant Bridge as Cordova system bridge
participant Secure as EncryptedSharedPreferences
App->>Legacy: Read legacy storageList
alt Legacy list exists
App->>Bridge: secureSet(storageList)
Bridge->>Secure: commit encrypted value
Secure-->>Bridge: durable write result
alt Write succeeds
Bridge-->>App: Resolve
App->>Legacy: Remove plaintext storageList
else Write fails
Bridge-->>App: Reject
App->>Legacy: Preserve plaintext fallback
end
end
App->>Bridge: secureGet(storageList)
Bridge->>Secure: Read encrypted value
Secure-->>App: Saved storage list
App->>App: Populate in-memory cache
Reviews (3): Last reviewed commit: "fix(secure-store): remove plaintext fall..." | Re-trigger Greptile |
…rets Addresses Greptile review on Acode-Foundation#2566: - Add missing <source-file> for SecureStore.java so Cordova copies it into the Android build (System.java references it -> was a compile failure). - SecureStore now uses commit() instead of apply() and reports write failure, so the migration only deletes the plaintext copy after the encrypted value is durably on disk (prevents credential loss on a crash mid-migration).
|
Thanks for the review! Pushed 57d8c16 addressing both points:
|
This comment was marked as outdated.
This comment was marked as outdated.
No problem. Changes look safe for a possible preview trigger. |
This comment has been minimized.
This comment has been minimized.
|
Preview Release for this, has been built. |
…on error Per review: the plaintext SharedPreferences fallback could report success and cause the JS migration to delete localStorage, then become unreadable once EncryptedSharedPreferences recovers (it encrypts lookup keys), leaving the saved server list empty. Now encryption failure propagates (set/get return false/null) so the legacy copy is kept and retried next launch — and no cleartext fallback is ever written.
This comment was marked as outdated.
This comment was marked as outdated.
|
A side note: this can break any plugins that use storageList from |
|
Good point, thanks for flagging it. A couple of thoughts:
And there's a security angle to that same access: right now any installed plugin can read That said, if there are plugins that legitimately need the saved-server list, I'm happy to add a proper accessor to the |
Yeah, there is no need for a plugin to read. |
|
@MYounas126 Well, yeah. That's the usage for these types things comes from edgey cases. Security is first, While I cannot link any plugins code related it as I'm not in a state to do so currently. It might be fine to move forward; As it was just a side note. |
|
The legitimate plugin I found other than deprecated ones due to our CM 6 Migration: Also, With the proposed implementation the plugins can still access them just like Acode can as long as the Java bridge provides a decoded, decrypted to the Javascript side. |
@bajrangCoder Well, no. As a Plugin needs know which folders the App has access to, which helps for URI restructuring, |
|
Thanks, went and looked at better-filebrowser — fair point, it's a real case. It reads and writes storageList but only touches uuid/name/uri (no creds, it's just registering a local folder), so my change would break it since I'm pulling storageList out of localStorage entirely. And you're right about the bridge as well. secureGet is reachable from any JS in the origin, so a plugin can still read the encrypted list, passwords included, the same way Acode does. So realistically all this PR buys is getting the cleartext copy off disk, which was the original #2561 concern (backups, forensics, other apps reading the webview data dir). It doesn't keep the list away from plugins, and I don't think that's really doable in a single-origin webview anyway. So maybe a cleaner approach: keep the non-secret list in localStorage so those plugins keep working (uuid/name/uri/type), and only move the passwords into the encrypted store, keyed per entry and put back at connect time. That still fixes the at-rest issue, doesn't break the plugins, and actually keeps passwords out of the plugin-visible list. It's more work than what's here now — I avoided it at first because the password-in-url matching in getVirtualPath gets in the way, but that's fixable by stripping creds off both sides before comparing. I'm happy to redo it that way, or keep this one as the at-rest fix and do the split as a follow-up. Whatever you two think is best. |
Fixes #2561.
Saved FTP/SFTP servers are kept in
localStorage.storageList, and the WebView writes localStorage to disk in plaintext — so the server passwords end up sitting unencrypted inapp_webview/.../Local Storage/leveldb.This PR moves that list into a native encrypted store (AES256-GCM via AndroidX Security-Crypto), which is the same mechanism the auth plugin already uses for the account token.
I started out planning to just pull the password out of the stored URL and keep it somewhere separate, but that doesn't work here:
Url.parse(url).urlkeeps the password, andhelpers.getVirtualPathmatches active file URIs against that full URL to rewrite remote paths, so dropping the password breaks path display. So instead I left the in-memory data exactly as it was (the URL still has its credentials in memory, which is fine) and only changed where it's persisted — plaintext localStorage becomes an encrypted native store. That keeps the diff small and every existing reader behaves the same.What's in here:
system.secureSet/secureGet/secureRemovebridge (SecureStore.java, backed byEncryptedSharedPreferences).src/lib/secureStorageList.js— an in-memory cache loaded once at startup from the encrypted store, plus a one-time migration of any existing plaintextlocalStorage.storageList. The plaintext copy is only deleted after the encrypted write succeeds, so nobody loses their saved servers if something goes wrong mid-migration.Uri.js,helpers.js,externalFs.jsandfileBrowser.jsnow go through that module instead of touchinglocalStorage.storageListdirectly.onDeviceReadyawaitssecureStorageList.hydrate()before anything reads the list.Existing users' saved servers migrate automatically on the next launch — no re-entry.
I don't have the Cordova/Android build set up on my machine yet, so I haven't been able to run this on a device. Could you trigger a preview build, or tell me how you'd prefer it verified? The main things to check are: add an SFTP/FTP server, restart the app, and confirm it reconnects without asking for the password again — and that the credentials no longer appear in the localStorage leveldb.