Skip to content

🛡️ Sentinel: [High] Fix plaintext password storage in AsyncStorage #54

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
TargetMisser wants to merge 1 commit into
base: main
Choose a base branch
from
+48 −29
Open

🛡️ Sentinel: [High] Fix plaintext password storage in AsyncStorage #54

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
51 changes: 24 additions & 27 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

26 changes: 24 additions & 2 deletions src/screens/PasswordScreen.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,19 @@ export default function PasswordScreen() {
// Load on mount
useEffect(() => {
(async () => {
const raw = await AsyncStorage.getItem(PASSWORDS_KEY);
let raw = null;
try {
raw = await SecureStore.getItemAsync(PASSWORDS_KEY);
} catch (e) {
if (__DEV__) console.warn('Failed to read from SecureStore', e);
}

// Fallback for legacy installs
if (!raw) {
raw = await AsyncStorage.getItem(PASSWORDS_KEY);
// If we found legacy data, we'll try to migrate it on next save
Comment on lines +158 to +159
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

The legacy AsyncStorage fallback loads plaintext passwords into memory but does not migrate them into SecureStore immediately, so the plaintext copy remains on disk until the user performs a save. To fully remediate the vulnerability for legacy users, migrate as soon as AsyncStorage data is detected: attempt SecureStore.setItemAsync with the legacy value, and on success AsyncStorage.removeItem(PASSWORDS_KEY) (handling SecureStore failures gracefully).

Suggested change
raw = await AsyncStorage.getItem(PASSWORDS_KEY);
// If we found legacy data, we'll try to migrate it on next save
const legacyRaw = await AsyncStorage.getItem(PASSWORDS_KEY);
if (legacyRaw) {
raw = legacyRaw;
try {
await SecureStore.setItemAsync(PASSWORDS_KEY, legacyRaw);
await AsyncStorage.removeItem(PASSWORDS_KEY);
} catch (e) {
if (__DEV__) console.warn('Failed to migrate passwords to SecureStore', e);
}
}

Copilot uses AI. Check for mistakes.
}

if (raw) setEntries(JSON.parse(raw));
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

JSON.parse(raw) can throw if SecureStore/AsyncStorage contains corrupted or partially-written data, which would crash the screen on mount. Wrap the parse in a try/catch and consider clearing the bad value (from SecureStore and/or legacy AsyncStorage) and/or showing an error so the app can recover.

Suggested change
if (raw) setEntries(JSON.parse(raw));
if (raw) {
try {
setEntries(JSON.parse(raw));
} catch (e) {
if (__DEV__) console.warn('Invalid stored passwords data; clearing saved value', e);
await SecureStore.deleteItemAsync(PASSWORDS_KEY).catch(() => {});
await AsyncStorage.removeItem(PASSWORDS_KEY).catch(() => {});
setEntries([]);
Alert.alert('Errore', 'I dati salvati delle password non sono validi e sono stati reimpostati.');
}
}

Copilot uses AI. Check for mistakes.
const enabled = await AsyncStorage.getItem(PIN_ENABLED_KEY);
const isEnabled = enabled === 'true';
Expand All @@ -157,7 +169,17 @@ export default function PasswordScreen() {

const persist = useCallback(async (next: PasswordEntry[]) => {
setEntries(next);
await AsyncStorage.setItem(PASSWORDS_KEY, JSON.stringify(next));
const serialized = JSON.stringify(next);
try {
await SecureStore.setItemAsync(PASSWORDS_KEY, serialized);
// Clean up legacy storage if successful
await AsyncStorage.removeItem(PASSWORDS_KEY).catch(() => {});
} catch (e) {
if (__DEV__) console.error('[passwords] save error', e);
Alert.alert('Errore', 'Impossibile salvare in modo sicuro. Verifica di avere spazio o prova a ridurre le note.');
// Optionally fallback to AsyncStorage if SecureStore fails (e.g. size limits)
// await AsyncStorage.setItem(PASSWORDS_KEY, serialized);
}
Comment on lines 170 to +182
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

persist updates React state (setEntries(next)) before confirming the SecureStore write succeeded. If SecureStore.setItemAsync throws (e.g., size limits), the UI will show the new/edited entries and callers (like saveModal) will still close the modal, but the data won’t survive an app restart. Consider only updating state after a successful secure save, or have persist return a success boolean / throw so callers can keep the modal open and avoid presenting unsaved data as saved.

Copilot uses AI. Check for mistakes.
}, []);

// PIN toggle
Expand Down
Loading