Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/fileSystem/ftp.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import secureCredentials from "lib/secureCredentials";
import settings from "lib/settings";
import mimeType from "mime-types";
import { decode, encode } from "utils/encodings";
Expand Down Expand Up @@ -359,10 +360,12 @@ Ftp.fromUrl = (url) => {
const { username, password, hostname, pathname, port, query } =
Url.decodeUrl(url);
const { security, mode } = query;
// Secrets are kept in the encrypted store, not in the saved URL (#2561).
const stored = secureCredentials.get(url) || {};
const ftp = new FtpClient(
hostname,
username,
password,
password || stored.password,
port || 21,
security,
mode,
Expand Down
8 changes: 6 additions & 2 deletions src/fileSystem/sftp.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import secureCredentials from "lib/secureCredentials";
import settings from "lib/settings";
import mimeType from "mime-types";
import { decode, encode } from "utils/encodings";
Expand Down Expand Up @@ -592,10 +593,13 @@ Sftp.fromUrl = (url) => {
Url.decodeUrl(url);
const { keyFile, passPhrase } = query;

// Secrets are kept in the encrypted store, not in the saved URL (#2561).
const stored = secureCredentials.get(url) || {};

const sftp = new SftpClient(hostname, port || 22, username, {
password,
password: password || stored.password,
keyFile,
passPhrase,
passPhrase: passPhrase || stored.passPhrase,
});

sftp.setPath(pathname);
Expand Down
18 changes: 16 additions & 2 deletions src/lib/remoteStorage.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import Ftp from "fileSystem/ftp";
import Sftp from "fileSystem/sftp";
import loader from "dialogs/loader";
import multiPrompt from "dialogs/multiPrompt";
import secureCredentials from "lib/secureCredentials";
import URLParse from "url-parse";
import helpers from "utils/helpers";
import Url from "utils/Url";
Expand Down Expand Up @@ -54,8 +55,12 @@ export default {
},
});

// Keep the password in the encrypted store instead of the saved
// URL, which lives in plaintext localStorage (#2561).
await secureCredentials.set(url, { password });

const res = {
url,
url: secureCredentials.stripPassword(url),
alias,
name: alias,
type: "ftp",
Expand Down Expand Up @@ -232,10 +237,14 @@ export default {
});
loader.destroy();
await helpers.showInterstitialIfReady();

// Keep secrets in the encrypted store instead of the saved URL (#2561).
await secureCredentials.set(url, { password, passPhrase });

return {
alias,
name: alias,
url,
url: secureCredentials.stripPassword(url),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stripPassword() only removes the userinfo password; the URL constructed above still contains passPhrase in its query string. updateStorage later serializes this URL into plaintext localStorage, so newly saved key-based SFTP passphrases remain exposed. Build the persisted URL without passPhrase or strip that query parameter as well.

type: "sftp",
home,
};
Expand Down Expand Up @@ -369,6 +378,11 @@ export default {
edit({ name, storageType, url }) {
let { username, password, hostname, port, query } = URLParse(url, true);

// Passwords are no longer kept in the saved URL (#2561), so pull the
// stored secret back in to prefill the edit form.
const stored = secureCredentials.get(url) || {};
if (!password && stored.password) password = stored.password;

if (username) {
username = decodeURIComponent(username);
}
Expand Down
171 changes: 171 additions & 0 deletions src/lib/secureCredentials.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,171 @@
/**
* Encrypted storage for remote-server secrets (FTP/SFTP passwords and key
* passphrases).
*
* The saved-server list itself stays in `localStorage.storageList` so plugins
* that read it keep working; only the secrets are moved out into the native
* encrypted store, keyed by connection identity. Secrets are put back into the
* URL at connect time. See #2561.
*/

const SECURE_KEY = "remoteCredentials";

/** @type {Record<string, {password?: string, passPhrase?: string}>} */
let cache = {};

/**
* Connection identity used as the lookup key: protocol, user, host and port.
* Deliberately excludes the path so every folder under a server shares one entry.
* @param {string} url
* @returns {string|null}
*/
function keyFor(url) {
if (!url) return null;
const m = /^([a-z0-9+.-]+:)\/\/([^@/]*@)?([^/:?#]+)(:(\d+))?/i.exec(url);
if (!m) return null;
const protocol = m[1].toLowerCase();
const userinfo = (m[2] || "").replace(/@$/, "");
const username = decodeURIComponent(userinfo.split(":")[0] || "");
const host = m[3].toLowerCase();
const port = m[5] || "";
return `${protocol}//${username}@${host}${port ? ":" + port : ""}`;
}

/**
* Remove `user:password@` credentials from a URL, keeping the username.
* Used so URLs saved before the migration still prefix-match today's URLs.
* @param {string} url
* @returns {string}
*/
function stripPassword(url) {
if (!url) return url;
return url.replace(
/^([a-z0-9+.-]+:\/\/)([^@/]*?):([^@/]*)@/i,
(_, scheme, user) => `${scheme}${user}@`,
);
}

/** Promisified bridge helpers — resolve to null instead of throwing. */
function secureGet(key) {
return new Promise((resolve) => {
try {
window.system.secureGet(key, resolve, () => resolve(null));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

window.system.secureGet and secureSet return Promises and do not accept callbacks, so the callbacks passed here are ignored. This outer Promise never resolves during normal operation, leaving hydrate() and therefore onDeviceReady() : stuck indefinitely.

} catch (_) {
resolve(null);
}
});
}

function secureSet(key, value) {
return new Promise((resolve, reject) => {
try {
window.system.secureSet(key, value, resolve, reject);
} catch (error) {
reject(error);
}
});
}

/**
* Load secrets into memory, and migrate any credentials still embedded in
* `localStorage.storageList` from older versions.
* Must be awaited during startup, before anything connects to a remote server.
*/
async function hydrate() {
try {
const stored = await secureGet(SECURE_KEY);
cache = stored ? JSON.parse(stored) || {} : {};
} catch (error) {
cache = {};
window.log?.("error", `secureCredentials: hydrate failed - ${error}`);
}

await migrateLegacy();
}

/**
* One-time move of inline credentials out of `localStorage.storageList`.
* The plaintext copy is only rewritten once the encrypted write is confirmed on
* disk, so an interrupted migration can't lose a saved server.
*/
async function migrateLegacy() {
let list;
try {
list = JSON.parse(localStorage.storageList || "[]");
} catch (_) {
return;
}
if (!Array.isArray(list) || !list.length) return;

let changed = false;
const pending = { ...cache };

for (const entry of list) {
const url = entry?.url;
if (!url || !/^[a-z0-9+.-]+:\/\/[^@/]*:[^@/]*@/i.test(url)) continue;

const key = keyFor(url);
if (!key) continue;

const password = decodeURIComponent(
/^[a-z0-9+.-]+:\/\/[^@/]*?:([^@/]*)@/i.exec(url)?.[1] || "",
);
if (!password) continue;

pending[key] = { ...(pending[key] || {}), password };
entry.url = stripPassword(url);
Comment on lines +103 to +116

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition only admits URLs containing user:password@, and the migration only extracts that password. Legacy key-authenticated SFTP entries commonly have no inline password but do have passPhrase in the query, so they are skipped and remain plaintext.

changed = true;
}

if (!changed) return;

try {
await secureSet(SECURE_KEY, JSON.stringify(pending));
cache = pending;
localStorage.storageList = JSON.stringify(list);
} catch (error) {
// Keep the legacy copy and retry on the next launch rather than lose it.
window.log?.("error", `secureCredentials: migration failed - ${error}`);
}
}

/**
* Secrets for a connection, or null. Synchronous by design so the existing
* synchronous `fromUrl` paths keep working.
* @param {string} url
*/
function get(url) {
const key = keyFor(url);
return (key && cache[key]) || null;
}

/**
* Persist secrets for a connection. Empty values remove the entry.
* @param {string} url
* @param {{password?: string, passPhrase?: string}} secrets
*/
async function set(url, secrets) {
const key = keyFor(url);
if (!key) return;

const clean = {};
if (secrets?.password) clean.password = secrets.password;
if (secrets?.passPhrase) clean.passPhrase = secrets.passPhrase;

const next = { ...cache };
if (Object.keys(clean).length) next[key] = clean;
else delete next[key];

await secureSet(SECURE_KEY, JSON.stringify(next));
cache = next;
}

/**
* Drop stored secrets for a connection (used when a server is removed).
* @param {string} url
*/
async function remove(url) {
await set(url, {});
Comment on lines +167 to +168

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This removal API has no callers. fileBrowser.removeStorage deletes the list entry and key file without invoking it, while editing a connection identity writes the new key without deleting the old one. As a result, credentials survive deletion and identity changes indefinitely.

}

export default { hydrate, get, set, remove, stripPassword, keyFor };
5 changes: 5 additions & 0 deletions src/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ import notificationManager from "lib/notificationManager";
import openFolder, { addedFolder } from "lib/openFolder";
import { registerPrettierFormatter } from "lib/registerPrettierFormatter";
import restoreFiles from "lib/restoreFiles";
import secureCredentials from "lib/secureCredentials";
import settings from "lib/settings";
import startAd, {
BANNER_SUPPRESSION_REASON,
Expand Down Expand Up @@ -101,6 +102,10 @@ document.addEventListener("menubutton", menuButtonHandler);

async function onDeviceReady() {
await initEncodings(); // important to load encodings before anything else
// Load remote-server secrets from the encrypted native store, migrating any
// credentials still embedded in localStorage. Must run before anything
// connects to a saved FTP/SFTP server. See issue #2561.
await secureCredentials.hydrate();

const isFreePackage = /(free)$/.test(BuildInfo.packageName);
const oldResolveURL = window.resolveLocalFileSystemURL;
Expand Down
85 changes: 85 additions & 0 deletions src/plugins/system/android/com/foxdebug/system/SecureStore.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
package com.foxdebug.system;

import android.content.Context;
import android.content.SharedPreferences;
import androidx.security.crypto.EncryptedSharedPreferences;
import androidx.security.crypto.MasterKeys;
import java.io.IOException;
import java.security.GeneralSecurityException;

/**
* Encrypted key/value store for secrets that must not sit in cleartext on disk
* (saved FTP/SFTP credentials — see #2561). Backed by AndroidX Security-Crypto
* (AES256-GCM values, AES256-SIV keys), the same mechanism the auth plugin uses
* for the account token.
*
* If the encrypted store can't be opened (keystore/crypto failure), reads and
* writes fail rather than falling back to plaintext. A plaintext fallback would
* both re-introduce cleartext credentials and become unreadable once encryption
* recovers — EncryptedSharedPreferences encrypts lookup keys, so a literal key
* written in fallback mode can't be found again. Failing instead lets the caller
* keep its source copy and retry on the next launch.
*/
public class SecureStore {

private static final String PREF_NAME = "acode_secure_store";

private final Context context;
private SharedPreferences prefs;

public SecureStore(Context context) {
this.context = context.getApplicationContext();
}

/** The encrypted preferences, or null if encryption is currently unavailable. */
private SharedPreferences prefs() {
if (prefs != null) return prefs;
try {
String masterKeyAlias = MasterKeys.getOrCreate(MasterKeys.AES256_GCM_SPEC);
prefs = EncryptedSharedPreferences.create(
PREF_NAME,
masterKeyAlias,
context,
EncryptedSharedPreferences.PrefKeyEncryptionScheme.AES256_SIV,
EncryptedSharedPreferences.PrefValueEncryptionScheme.AES256_GCM
);
} catch (GeneralSecurityException | IOException e) {
Comment thread
bajrangCoder marked this conversation as resolved.
prefs = null;
}
return prefs;
}

/**
* Store a value durably. Passing null removes the key.
* Uses commit() (not apply()) so the write is on disk before returning — the
* JS migration deletes the legacy plaintext copy only after this reports
* success.
* @return true if the write reached disk; false if encryption is unavailable.
*/
public boolean set(String key, String value) {
if (value == null) {
return remove(key);
}
SharedPreferences p = prefs();
if (p == null) return false;
return p.edit().putString(key, value).commit();
}

/** Return the stored value, or null if absent or encryption is unavailable. */
public String get(String key) {
SharedPreferences p = prefs();
if (p == null) return null;
return p.getString(key, null);
}

public boolean remove(String key) {
SharedPreferences p = prefs();
if (p == null) return false;
return p.edit().remove(key).commit();
}

public boolean contains(String key) {
SharedPreferences p = prefs();
return p != null && p.contains(key);
}
}
Loading