From d21925e11055e8799ac220ca6b1762cc98009c00 Mon Sep 17 00:00:00 2001 From: Ivan Malison Date: Fri, 6 Feb 2026 20:23:51 -0800 Subject: [PATCH] security: store credentials in secure storage --- app.config.js | 1 + app/(tabs)/settings/servers.tsx | 30 +++++++---- context/AuthContext.tsx | 38 ++++---------- package.json | 1 + services/backgroundSync.ts | 20 +------- tests/unit/AuthContext.test.ts | 2 +- tests/unit/serverStorage.test.ts | 46 +++++++++++++++-- types/server.ts | 8 ++- utils/authStorage.ts | 77 ++++++++++++++++++++++++++++ utils/secretStore.ts | 41 +++++++++++++++ utils/serverStorage.ts | 87 ++++++++++++++++++++++++++++---- yarn.lock | 5 ++ 12 files changed, 284 insertions(+), 72 deletions(-) create mode 100644 utils/authStorage.ts create mode 100644 utils/secretStore.ts diff --git a/app.config.js b/app.config.js index c954029..bb0f28a 100644 --- a/app.config.js +++ b/app.config.js @@ -60,6 +60,7 @@ export default { }, plugins: [ "expo-router", + "expo-secure-store", [ "expo-splash-screen", { diff --git a/app/(tabs)/settings/servers.tsx b/app/(tabs)/settings/servers.tsx index 3432f15..c50e9cd 100644 --- a/app/(tabs)/settings/servers.tsx +++ b/app/(tabs)/settings/servers.tsx @@ -57,7 +57,8 @@ export default function ServersScreen() { }; const handleEdit = (server: SavedServer) => { - setEditingServer({ ...server }); + // Password is stored securely; don't prefill it into UI. + setEditingServer({ ...server, password: "" }); setDialogMode("edit"); setDialogVisible(true); }; @@ -94,12 +95,16 @@ export default function ServersScreen() { const handleSave = async () => { if (!editingServer) return; - if ( - !editingServer.apiUrl || - !editingServer.username || - !editingServer.password - ) { - Alert.alert("Error", "Server URL, username, and password are required"); + if (!editingServer.apiUrl || !editingServer.username) { + Alert.alert("Error", "Server URL and username are required"); + return; + } + + const needsPassword = + dialogMode === "add" || + (!editingServer.hasPassword && !editingServer.password); + if (needsPassword && !editingServer.password) { + Alert.alert("Error", "Password is required"); return; } @@ -110,7 +115,7 @@ export default function ServersScreen() { const success = await login( editingServer.apiUrl, editingServer.username, - editingServer.password, + editingServer.password || "", true, // save to server list ); if (success) { @@ -124,12 +129,15 @@ export default function ServersScreen() { } } else if (editingServer.id) { // Editing existing server - await updateServer(editingServer.id, { + const updates: Partial = { nickname: editingServer.nickname, apiUrl: editingServer.apiUrl, username: editingServer.username, - password: editingServer.password, - }); + ...(editingServer.password + ? { password: editingServer.password } + : {}), + }; + await updateServer(editingServer.id, updates); setDialogVisible(false); setEditingServer(null); } diff --git a/context/AuthContext.tsx b/context/AuthContext.tsx index c98dea2..736ea9d 100644 --- a/context/AuthContext.tsx +++ b/context/AuthContext.tsx @@ -1,4 +1,9 @@ import { SavedServer, SavedServerInput } from "@/types/server"; +import { + clearStoredCredentials, + getStoredCredentials, + storeCredentials, +} from "@/utils/authStorage"; import { base64Encode } from "@/utils/base64"; import { deleteServer as deleteServerFromStorage, @@ -14,7 +19,6 @@ import { clearWidgetCredentials, saveCredentialsToWidget, } from "@/widgets/storage"; -import AsyncStorage from "@react-native-async-storage/async-storage"; import React, { createContext, ReactNode, @@ -84,12 +88,6 @@ interface AuthContextType extends AuthState { const AuthContext = createContext(undefined); -const STORAGE_KEYS = { - API_URL: "mova_api_url", - USERNAME: "mova_username", - PASSWORD: "mova_password", -}; - export function AuthProvider({ children }: { children: ReactNode }) { const { triggerRefresh } = useMutation(); const [state, setState] = useState({ @@ -135,11 +133,7 @@ export function AuthProvider({ children }: { children: ReactNode }) { return; } - const [apiUrl, username, password] = await Promise.all([ - AsyncStorage.getItem(STORAGE_KEYS.API_URL), - AsyncStorage.getItem(STORAGE_KEYS.USERNAME), - AsyncStorage.getItem(STORAGE_KEYS.PASSWORD), - ]); + const { apiUrl, username, password } = await getStoredCredentials(); if (apiUrl && username && password) { setState({ @@ -174,11 +168,7 @@ export function AuthProvider({ children }: { children: ReactNode }) { }); if (response.ok) { - await Promise.all([ - AsyncStorage.setItem(STORAGE_KEYS.API_URL, normalizedUrl), - AsyncStorage.setItem(STORAGE_KEYS.USERNAME, username), - AsyncStorage.setItem(STORAGE_KEYS.PASSWORD, password), - ]); + await storeCredentials({ apiUrl: normalizedUrl, username, password }); // Also save to SharedPreferences for widget access await saveCredentialsToWidget(normalizedUrl, username, password); @@ -200,11 +190,9 @@ export function AuthProvider({ children }: { children: ReactNode }) { setActiveServerIdState(newServer.id); await refreshSavedServers(); } else { - // Update password if changed - if (existing.password !== password) { - await updateServerInStorage(existing.id, { password }); - await refreshSavedServers(); - } + // Always keep the stored password up to date (secure store). + await updateServerInStorage(existing.id, { password }); + await refreshSavedServers(); await setActiveServerId(existing.id); setActiveServerIdState(existing.id); } @@ -229,11 +217,7 @@ export function AuthProvider({ children }: { children: ReactNode }) { } async function logout() { - await Promise.all([ - AsyncStorage.removeItem(STORAGE_KEYS.API_URL), - AsyncStorage.removeItem(STORAGE_KEYS.USERNAME), - AsyncStorage.removeItem(STORAGE_KEYS.PASSWORD), - ]); + await clearStoredCredentials(); // Also clear widget credentials await clearWidgetCredentials(); diff --git a/package.json b/package.json index bd6bbc7..5c76b5c 100644 --- a/package.json +++ b/package.json @@ -39,6 +39,7 @@ "expo-linking": "~8.0.11", "expo-notifications": "~0.32.16", "expo-router": "~6.0.23", + "expo-secure-store": "~15.0.8", "expo-splash-screen": "~31.0.13", "expo-status-bar": "~3.0.9", "expo-system-ui": "~6.0.9", diff --git a/services/backgroundSync.ts b/services/backgroundSync.ts index 14f0489..c8db87e 100644 --- a/services/backgroundSync.ts +++ b/services/backgroundSync.ts @@ -1,4 +1,4 @@ -import AsyncStorage from "@react-native-async-storage/async-storage"; +import { getStoredCredentials } from "@/utils/authStorage"; import { AppState, Platform } from "react-native"; const BACKGROUND_SYNC_TASK = "MOVA_BACKGROUND_SYNC"; @@ -9,23 +9,7 @@ const isHeadlessContext = () => { return AppState.currentState === null || AppState.currentState === undefined; }; -// Storage keys for credentials (must match AuthContext STORAGE_KEYS) -const API_URL_KEY = "mova_api_url"; -const USERNAME_KEY = "mova_username"; -const PASSWORD_KEY = "mova_password"; - -async function getStoredCredentials(): Promise<{ - apiUrl: string | null; - username: string | null; - password: string | null; -}> { - const [apiUrl, username, password] = await Promise.all([ - AsyncStorage.getItem(API_URL_KEY), - AsyncStorage.getItem(USERNAME_KEY), - AsyncStorage.getItem(PASSWORD_KEY), - ]); - return { apiUrl, username, password }; -} +// Credentials are stored via utils/authStorage (secure store + legacy migration). // Lazy-load expo modules to avoid issues in headless widget context let BackgroundFetch: typeof import("expo-background-fetch") | null = null; diff --git a/tests/unit/AuthContext.test.ts b/tests/unit/AuthContext.test.ts index 7e1a1eb..4e16079 100644 --- a/tests/unit/AuthContext.test.ts +++ b/tests/unit/AuthContext.test.ts @@ -32,7 +32,7 @@ describe("AuthContext multi-server", () => { id: "1", apiUrl: "https://test.com", username: "user", - password: "pass", + hasPassword: true, }, ]); diff --git a/tests/unit/serverStorage.test.ts b/tests/unit/serverStorage.test.ts index 99e5feb..5e46bff 100644 --- a/tests/unit/serverStorage.test.ts +++ b/tests/unit/serverStorage.test.ts @@ -14,6 +14,12 @@ jest.mock("@react-native-async-storage/async-storage", () => ({ removeItem: jest.fn(), })); +jest.mock("../../utils/secretStore", () => ({ + getSecret: jest.fn(), + setSecret: jest.fn(), + deleteSecret: jest.fn(), +})); + describe("serverStorage", () => { beforeEach(() => { jest.clearAllMocks(); @@ -38,20 +44,34 @@ describe("serverStorage", () => { (AsyncStorage.getItem as jest.Mock).mockResolvedValue( JSON.stringify(mockServers), ); + const { getSecret, setSecret } = jest.requireMock( + "../../utils/secretStore", + ); + (getSecret as jest.Mock).mockResolvedValue("pass1"); const servers = await getSavedServers(); - expect(servers).toEqual(mockServers); + expect(setSecret).toHaveBeenCalled(); + expect(servers).toEqual([ + { + id: "1", + apiUrl: "https://server1.com", + username: "user1", + hasPassword: true, + }, + ]); }); }); describe("saveServer", () => { it("should add new server with generated id", async () => { (AsyncStorage.getItem as jest.Mock).mockResolvedValue(null); + const { setSecret } = jest.requireMock("../../utils/secretStore"); const server = await saveServer({ apiUrl: "https://new.com", username: "user", password: "pass", }); expect(server.id).toBeDefined(); + expect(setSecret).toHaveBeenCalled(); expect(AsyncStorage.setItem).toHaveBeenCalled(); }); }); @@ -69,10 +89,19 @@ describe("serverStorage", () => { (AsyncStorage.getItem as jest.Mock).mockResolvedValue( JSON.stringify(mockServers), ); + const { getSecret } = jest.requireMock("../../utils/secretStore"); + (getSecret as jest.Mock).mockResolvedValue("pass1"); await updateServer("1", { nickname: "My Server" }); expect(AsyncStorage.setItem).toHaveBeenCalledWith( "mova_saved_servers", - JSON.stringify([{ ...mockServers[0], nickname: "My Server" }]), + JSON.stringify([ + { + id: "1", + apiUrl: "https://server1.com", + username: "user1", + nickname: "My Server", + }, + ]), ); }); }); @@ -96,10 +125,21 @@ describe("serverStorage", () => { (AsyncStorage.getItem as jest.Mock).mockResolvedValue( JSON.stringify(mockServers), ); + const { getSecret, deleteSecret } = jest.requireMock( + "../../utils/secretStore", + ); + (getSecret as jest.Mock).mockResolvedValue("pass"); await deleteServer("1"); + expect(deleteSecret).toHaveBeenCalled(); expect(AsyncStorage.setItem).toHaveBeenCalledWith( "mova_saved_servers", - JSON.stringify([mockServers[1]]), + JSON.stringify([ + { + id: "2", + apiUrl: "https://server2.com", + username: "user2", + }, + ]), ); }); }); diff --git a/types/server.ts b/types/server.ts index 902c7f4..86030b6 100644 --- a/types/server.ts +++ b/types/server.ts @@ -3,8 +3,12 @@ export interface SavedServer { nickname?: string; apiUrl: string; username: string; - password: string; defaultCaptureTemplate?: string; + hasPassword?: boolean; +} + +export interface SavedServerWithPassword extends SavedServer { + password: string; } -export type SavedServerInput = Omit; +export type SavedServerInput = Omit; diff --git a/utils/authStorage.ts b/utils/authStorage.ts new file mode 100644 index 0000000..31f9f28 --- /dev/null +++ b/utils/authStorage.ts @@ -0,0 +1,77 @@ +import { deleteSecret, getSecret, setSecret } from "@/utils/secretStore"; +import AsyncStorage from "@react-native-async-storage/async-storage"; + +export const AUTH_STORAGE_KEYS = { + API_URL: "mova_api_url", + USERNAME: "mova_username", + PASSWORD: "mova_password", +} as const; + +export async function getStoredCredentials(): Promise<{ + apiUrl: string | null; + username: string | null; + password: string | null; +}> { + // Prefer secure store, but migrate from AsyncStorage if needed. + const [apiUrl, username, password] = await Promise.all([ + getSecret(AUTH_STORAGE_KEYS.API_URL), + getSecret(AUTH_STORAGE_KEYS.USERNAME), + getSecret(AUTH_STORAGE_KEYS.PASSWORD), + ]); + + if (apiUrl && username && password) { + return { apiUrl, username, password }; + } + + const [legacyApiUrl, legacyUsername, legacyPassword] = await Promise.all([ + AsyncStorage.getItem(AUTH_STORAGE_KEYS.API_URL), + AsyncStorage.getItem(AUTH_STORAGE_KEYS.USERNAME), + AsyncStorage.getItem(AUTH_STORAGE_KEYS.PASSWORD), + ]); + + if (legacyApiUrl && legacyUsername && legacyPassword) { + await Promise.all([ + setSecret(AUTH_STORAGE_KEYS.API_URL, legacyApiUrl), + setSecret(AUTH_STORAGE_KEYS.USERNAME, legacyUsername), + setSecret(AUTH_STORAGE_KEYS.PASSWORD, legacyPassword), + AsyncStorage.removeItem(AUTH_STORAGE_KEYS.API_URL), + AsyncStorage.removeItem(AUTH_STORAGE_KEYS.USERNAME), + AsyncStorage.removeItem(AUTH_STORAGE_KEYS.PASSWORD), + ]); + return { + apiUrl: legacyApiUrl, + username: legacyUsername, + password: legacyPassword, + }; + } + + return { apiUrl: null, username: null, password: null }; +} + +export async function storeCredentials(input: { + apiUrl: string; + username: string; + password: string; +}) { + await Promise.all([ + setSecret(AUTH_STORAGE_KEYS.API_URL, input.apiUrl), + setSecret(AUTH_STORAGE_KEYS.USERNAME, input.username), + setSecret(AUTH_STORAGE_KEYS.PASSWORD, input.password), + // Best-effort cleanup of legacy keys. + AsyncStorage.removeItem(AUTH_STORAGE_KEYS.API_URL), + AsyncStorage.removeItem(AUTH_STORAGE_KEYS.USERNAME), + AsyncStorage.removeItem(AUTH_STORAGE_KEYS.PASSWORD), + ]); +} + +export async function clearStoredCredentials() { + await Promise.all([ + deleteSecret(AUTH_STORAGE_KEYS.API_URL), + deleteSecret(AUTH_STORAGE_KEYS.USERNAME), + deleteSecret(AUTH_STORAGE_KEYS.PASSWORD), + // Best-effort cleanup of legacy keys. + AsyncStorage.removeItem(AUTH_STORAGE_KEYS.API_URL), + AsyncStorage.removeItem(AUTH_STORAGE_KEYS.USERNAME), + AsyncStorage.removeItem(AUTH_STORAGE_KEYS.PASSWORD), + ]); +} diff --git a/utils/secretStore.ts b/utils/secretStore.ts new file mode 100644 index 0000000..c4cc7fc --- /dev/null +++ b/utils/secretStore.ts @@ -0,0 +1,41 @@ +import AsyncStorage from "@react-native-async-storage/async-storage"; + +// Thin wrapper around secure storage with a web fallback. +// On web, AsyncStorage is used (best-effort; not truly secure). + +async function isWeb(): Promise { + // Avoid importing react-native in unit test (node) environment. + // If react-native isn't available/parsable, fall back to web behavior. + try { + const { Platform } = await import("react-native"); + return Platform.OS === "web"; + } catch { + return true; + } +} + +export async function getSecret(key: string): Promise { + if (await isWeb()) { + return await AsyncStorage.getItem(key); + } + const SecureStore = await import("expo-secure-store"); + return await SecureStore.getItemAsync(key); +} + +export async function setSecret(key: string, value: string): Promise { + if (await isWeb()) { + await AsyncStorage.setItem(key, value); + return; + } + const SecureStore = await import("expo-secure-store"); + await SecureStore.setItemAsync(key, value); +} + +export async function deleteSecret(key: string): Promise { + if (await isWeb()) { + await AsyncStorage.removeItem(key); + return; + } + const SecureStore = await import("expo-secure-store"); + await SecureStore.deleteItemAsync(key); +} diff --git a/utils/serverStorage.ts b/utils/serverStorage.ts index f490c1e..338b15d 100644 --- a/utils/serverStorage.ts +++ b/utils/serverStorage.ts @@ -1,4 +1,9 @@ -import { SavedServer, SavedServerInput } from "@/types/server"; +import { + SavedServer, + SavedServerInput, + SavedServerWithPassword, +} from "@/types/server"; +import { deleteSecret, getSecret, setSecret } from "@/utils/secretStore"; import AsyncStorage from "@react-native-async-storage/async-storage"; const STORAGE_KEYS = { @@ -6,6 +11,24 @@ const STORAGE_KEYS = { ACTIVE_SERVER_ID: "mova_active_server_id", }; +const SERVER_PASSWORD_PREFIX = "mova_server_password:"; + +function passwordKey(id: string) { + return `${SERVER_PASSWORD_PREFIX}${id}`; +} + +async function getServerPassword(id: string): Promise { + return await getSecret(passwordKey(id)); +} + +async function setServerPassword(id: string, password: string): Promise { + await setSecret(passwordKey(id), password); +} + +async function deleteServerPassword(id: string): Promise { + await deleteSecret(passwordKey(id)); +} + function generateId(): string { return Date.now().toString(36) + Math.random().toString(36).substr(2); } @@ -13,7 +36,39 @@ function generateId(): string { export async function getSavedServers(): Promise { try { const stored = await AsyncStorage.getItem(STORAGE_KEYS.SAVED_SERVERS); - return stored ? JSON.parse(stored) : []; + const parsed: any[] = stored ? JSON.parse(stored) : []; + + // Migrate legacy stored passwords into secure store, and remove from AsyncStorage. + let didMigrate = false; + const sanitized = await Promise.all( + parsed.map(async (s) => { + if (typeof s?.id !== "string") return s; + if (typeof s?.password === "string" && s.password.length > 0) { + await setServerPassword(s.id, s.password); + const { password, ...rest } = s; + didMigrate = true; + return rest; + } + return s; + }), + ); + + if (didMigrate) { + await AsyncStorage.setItem( + STORAGE_KEYS.SAVED_SERVERS, + JSON.stringify(sanitized), + ); + } + + const withFlags: SavedServer[] = await Promise.all( + sanitized.map(async (s) => { + if (!s?.id) return s; + const pw = await getServerPassword(s.id); + return { ...s, hasPassword: !!pw }; + }), + ); + + return withFlags; } catch { return []; } @@ -23,11 +78,14 @@ export async function saveServer( input: SavedServerInput, ): Promise { const servers = await getSavedServers(); - const newServer: SavedServer = { ...input, id: generateId() }; - servers.push(newServer); + const id = generateId(); + await setServerPassword(id, input.password); + const { password: _password, ...rest } = input; + const newServer: SavedServer = { ...rest, id, hasPassword: true }; + servers.push({ ...rest, id }); await AsyncStorage.setItem( STORAGE_KEYS.SAVED_SERVERS, - JSON.stringify(servers), + JSON.stringify(servers.map(({ hasPassword, ...s }) => s)), ); return newServer; } @@ -39,10 +97,14 @@ export async function updateServer( const servers = await getSavedServers(); const index = servers.findIndex((s) => s.id === id); if (index !== -1) { - servers[index] = { ...servers[index], ...updates }; + if (typeof updates.password === "string") { + await setServerPassword(id, updates.password); + } + const { password: _password, ...rest } = updates; + servers[index] = { ...servers[index], ...rest }; await AsyncStorage.setItem( STORAGE_KEYS.SAVED_SERVERS, - JSON.stringify(servers), + JSON.stringify(servers.map(({ hasPassword, ...s }) => s)), ); } } @@ -50,9 +112,10 @@ export async function updateServer( export async function deleteServer(id: string): Promise { const servers = await getSavedServers(); const filtered = servers.filter((s) => s.id !== id); + await deleteServerPassword(id); await AsyncStorage.setItem( STORAGE_KEYS.SAVED_SERVERS, - JSON.stringify(filtered), + JSON.stringify(filtered.map(({ hasPassword, ...s }) => s)), ); } @@ -70,7 +133,11 @@ export async function setActiveServerId(id: string | null): Promise { export async function findServerById( id: string, -): Promise { +): Promise { const servers = await getSavedServers(); - return servers.find((s) => s.id === id); + const server = servers.find((s) => s.id === id); + if (!server) return undefined; + const password = await getServerPassword(id); + if (!password) return undefined; + return { ...server, password }; } diff --git a/yarn.lock b/yarn.lock index 67180ae..89abe29 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4907,6 +4907,11 @@ expo-router@~6.0.23: use-latest-callback "^0.2.1" vaul "^1.1.2" +expo-secure-store@~15.0.8: + version "15.0.8" + resolved "https://registry.yarnpkg.com/expo-secure-store/-/expo-secure-store-15.0.8.tgz#678065599bb76061b5a85b15b9426bf7a11089ae" + integrity sha512-lHnzvRajBu4u+P99+0GEMijQMFCOYpWRO4dWsXSuMt77+THPIGjzNvVKrGSl6mMrLsfVaKL8BpwYZLGlgA+zAw== + expo-server@^1.0.5: version "1.0.5" resolved "https://registry.yarnpkg.com/expo-server/-/expo-server-1.0.5.tgz#2d52002199a2af99c2c8771d0657916004345ca9"