AUD-02: key material & keychain audit + remediation (F-02/F-06/F-07/F-08) - #2185
AUD-02: key material & keychain audit + remediation (F-02/F-06/F-07/F-08)#2185seshanthS wants to merge 2 commits into
Conversation
Read-only pilot audit (SELF-3180). Adds the findings report, current-behavior characterization tests, owner dispositions, and the SPEC backlog/status-log update. Confirmed: F-01 plaintext mnemonic to iCloud/Drive (Critical), F-02 legacy-format parse failure overwrites the secret (Critical), plus F-03..F-09 and F-10..F-14 lows. Every Critical/Major independently refutation-verified (Stage 4). Dispositions: fix F-02/F-06/F-07/F-08 (remediation PR fix/aud-02-key-material); accept+document F-03/F-05/F-09; defer F-01/F-04; drop lows. AUD-02 row -> In Review. Tests written but not validated in-session (worktree has no node_modules; the stale main checkout's jest is broken). Pre-commit hook bypassed: it runs `yarn gitleaks` but origin/dev is pnpm; gitleaks itself was run directly (protect --staged --config=gitleaks-override.toml) and reported no leaks. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Remediation for the AUD-02 key-material audit (SELF-3180). Stacked on the
read-only audit commit; targets the owner-accepted fixes.
F-02 (Critical): loadOrCreateMnemonic no longer overwrites a present-but-
unparseable secret. It fails closed — fires keychainCryptoFailureCallback
('crypto_failed') and returns false — so the create/overwrite block only runs
when no secret existed. Corrupted/legacy entries route to recovery instead of
being destroyed.
F-07 (Major): hasSecretStored returns a tri-state (present | absent | unknown)
instead of collapsing read errors to false. SplashScreen treats only an
explicit 'absent' as a fresh install, so a transient keychain error no longer
misroutes an existing user into onboarding/recovery.
F-08 (Major): cloud backup keeps a single canonical Drive file. Android upload
updates in place via setIdOfFileToUpdate (new settingStore.backupFileId),
adopting/deduping any pre-existing duplicates; download/delete operate by id
with a list+dedupe self-heal fallback. iOS already overwrites a fixed path.
F-06: corrected the useStrongBox JSDoc (effective default is false; StrongBox
off by default due to Samsung-device issues).
Tests: flipped the F-02 characterization test to assert no-overwrite + failure
callback; added F-07 tri-state/routing tests; added F-08 update-in-place,
dedupe, download-by-id, and clear-on-disable tests. Validated in-worktree:
jest (33 passed), tsc --noEmit (clean), check-test-requires (clean).
Pre-commit hook bypassed (runs `yarn gitleaks`; repo is pnpm). gitleaks ran
directly (protect --staged --config=gitleaks-override.toml): no leaks.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 207023a62f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| const [canonicalId, ...duplicates] = ids; | ||
| if (duplicates.length > 0) { | ||
| await Promise.all(duplicates.map(id => gdrive.files.delete(id))); |
There was a problem hiding this comment.
On Android accounts that already have duplicate encrypted-private-key files from the old upload path, this deletes every id after whichever files.list happens to return first before the selected file is parsed during restore. Since the list request does not request an order or compare modifiedTime, the first id can be stale or corrupt, so a cloud restore can delete the remaining valid/newer copies and then fail or restore the wrong mnemonic. Choose and validate the canonical backup first, and delete duplicates only after a successful parse/update.
Useful? React with 👍 / 👎.
| if (!fileId) { | ||
| fileId = await resolveCanonicalDriveFileId(gdrive); | ||
| if (fileId) { | ||
| setBackupFileId(fileId); | ||
| } | ||
| } |
There was a problem hiding this comment.
Once backupFileId is persisted, Android restore skips the name-based lookup and goes straight to getText for that cached id. If the Drive file was deleted/recreated or the user signs into a different Google account while local settings still hold the old id, a current same-name backup can exist but restore still fails on the stale id; the previous code listed by name every time. Clear the cached id and retry resolveCanonicalDriveFileId on not-found/access failures before surfacing the error.
Useful? React with 👍 / 👎.
📝 WalkthroughWalkthroughThis PR delivers two AUD-02 security remediations: ChangesFail-closed keychain secret handling and startup routing
Drive backup canonical file id caching and self-healing
AUD-02 audit report, plan completion, and spec status update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR delivers the AUD-02 security audit report plus remediations for four accepted findings: F-02 (fail-closed on unparseable keychain secret), F-07 (tri-state
Confidence Score: 4/5The F-02 and F-07 fixes are correct and well-tested; the cloud-backup changes are mostly solid but have one correctness gap in the self-healing path. The F-02 and F-07 remediations are clean and regression-tested. The F-08 Drive deduplication logic works for the happy path, but resolveCanonicalDriveFileId propagates deletion failures to callers rather than treating cleanup as best-effort — meaning users with accumulated duplicate files could have their first backup or restore aborted by a transient network error during the cleanup phase. The download function also loses the old list-based resilience when the stored backupFileId becomes stale. app/src/services/cloud-backup/index.ts — specifically the duplicate-deletion error handling in resolveCanonicalDriveFileId and the absence of a stale-ID fallback in download. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (duplicates.length > 0) { | ||
| await Promise.all(duplicates.map(id => gdrive.files.delete(id))); | ||
| } |
There was a problem hiding this comment.
Promise.all will reject if any single delete call fails (transient network blip, Drive quota, etc.), and resolveCanonicalDriveFileId has no try/catch around it, so the rejection propagates unhandled to both callers (upload line 144 and download line 99). For a user who accumulated duplicates under the old code, the very first upload or download after this PR lands would attempt the self-healing cleanup, and any deletion hiccup aborts the primary operation entirely — even though the canonical ID was already resolved and the actual file is reachable. Wrapping the deletion in a try/catch (or using Promise.allSettled) and continuing to return canonicalId regardless would make the cleanup genuinely best-effort.
| const { backupFileId, setBackupFileId } = useSettingStore.getState(); | ||
| let fileId = backupFileId; | ||
| if (!fileId) { | ||
| fileId = await resolveCanonicalDriveFileId(gdrive); | ||
| if (fileId) { | ||
| setBackupFileId(fileId); | ||
| } | ||
| } | ||
|
|
||
| if (!isDriveFile(firstFile)) { | ||
| if (!fileId) { | ||
| throw new Error( | ||
| 'Couldnt find the encrypted backup, did you back it up previously?', | ||
| ); | ||
| } | ||
| const resolvedFileId = fileId; | ||
| const mnemonicString = await withRetries(() => | ||
| gdrive.files.getText(firstFile.id), | ||
| gdrive.files.getText(resolvedFileId), | ||
| ); |
There was a problem hiding this comment.
upload handles a stale backupFileId in its catch block by clearing the stored ID and falling through to a resolveCanonicalDriveFileId-backed create path. download does not: if backupFileId is set but the Drive file was deleted externally (e.g., user cleared app data from Google account settings), getText(resolvedFileId) fails and the function propagates the Drive API error with no opportunity to re-list and find the file under a new ID. Before this PR, download always listed first and was therefore resilient to moved/re-created files. Adding the same catch-and-re-list recovery that upload uses would close the gap.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6ed2fd10-5266-452c-b8f7-c43955ef7d34
📒 Files selected for processing (10)
app/src/integrations/keychain/index.tsapp/src/providers/authProvider.tsxapp/src/screens/app/SplashScreen.tsxapp/src/services/cloud-backup/index.tsapp/src/stores/settingStore.tsapp/tests/src/providers/authProvider.aud02.test.tsxapp/tests/src/services/cloud-backup.test.tsdocs/reviews/2026-06-17-key-material-keychain-audit.mdspecs/projects/sdk/workstreams/audits/SPEC.mdspecs/projects/sdk/workstreams/audits/plans/AUD-02-key-material-keychain-lifecycle.md
| const { files } = await gdrive.files.list({ | ||
| spaces: APP_DATA_FOLDER_ID, | ||
| q: `name = '${FILE_NAME}'`, | ||
| }); | ||
| const ids = (files as unknown[]).filter(isDriveFile).map(file => file.id); | ||
| if (ids.length === 0) { | ||
| return null; | ||
| } | ||
| const [canonicalId, ...duplicates] = ids; | ||
| if (duplicates.length > 0) { | ||
| await Promise.all(duplicates.map(id => gdrive.files.delete(id))); | ||
| } | ||
| return canonicalId; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect current Drive list usage and dependency version so the fix can target the wrapper's supported list options.
rg -n -C 4 'files\.list\(|orderBy|modifiedTime|createdTime' --glob 'app/**/*.{ts,tsx,js,jsx}'
rg -n -C 2 '`@robinbobin/react-native-google-drive-api-wrapper`' package.json pnpm-lock.yamlRepository: selfxyz/self
Length of output: 3171
🏁 Script executed:
# Find the wrapper source or check what parameters are documented
fd -i 'google-drive' app/ --type f | head -20Repository: selfxyz/self
Length of output: 38
🌐 Web query:
@robinbobin/react-native-google-drive-api-wrapper files.list orderBy parameter support
💡 Result:
The @robinbobin/react-native-google-drive-api-wrapper library does not explicitly document or expose an orderBy parameter in its IFilesListQueryParameters interface for the files.list() method [1][2]. While the underlying Google Drive API files.list method natively supports an orderBy parameter to sort results by fields like modifiedTime, name, or createdTime [3][4], this specific wrapper library provides a simplified interface [1][2]. The library primarily facilitates file listing via a q parameter (which supports a ListQueryBuilder helper class) and handles specific query parameter transformations, such as converting arrays to comma-separated strings for properties like spaces [1][2]. Because the library's IFilesListQueryParameters interface is not fully exhaustive of all native Google Drive API v3 parameters, you may find that passing an orderBy property directly into the queryParameters object of the list() method is not supported by the library's type definitions or internal processing [1][2]. If you need to use orderBy, you may need to check the library's source code for the IFilesListQueryParameters type definition or consider if the wrapper allows passing arbitrary parameters that are then passed through to the underlying fetch request [1][2].
Citations:
- 1: https://github.com/RobinBobin/react-native-google-drive-api-wrapper/blob/main/current/README.md
- 2: https://www.npmjs.com/package/@robinbobin/react-native-google-drive-api-wrapper
- 3: https://developers.google.cn/workspace/drive/api/reference/rest/v3/files/list
- 4: https://googleapis.dev/nodejs/googleapis/latest/drive/interfaces/Params%24Resource%24Files%24List-1.html
🏁 Script executed:
# Check the full resolveCanonicalDriveFileId and surrounding context to understand the data flow
cd app && sed -n '35,85p' src/services/cloud-backup/index.tsRepository: selfxyz/self
Length of output: 1556
The Google Drive wrapper v2.2.6 does not expose orderBy, making the current canonical file selection non-deterministic and risking data loss.
gdrive.files.list() without explicit orderBy has undefined ordering from the Google Drive API. The code selects the first returned file as canonical and deletes the rest, but without deterministic ordering, a stale duplicate could be returned first, leaving you restoring outdated mnemonic data.
The underlying Google Drive API supports orderBy for sorting by modifiedTime, but the wrapper's IFilesListQueryParameters interface does not expose it. You need one of:
- Upgrade or fork the wrapper to expose
orderByparameter and sort bymodifiedTimedescending - Store backup creation/modification metadata outside of the filename and use it to select the newest copy before deletion
- Explicitly document and test that duplicates are acceptable (i.e., any copy is safe to restore)
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect current Drive list usage and dependency version so the fix can target the wrapper's supported list options.
rg -n -C 4 'files\.list\(|orderBy|modifiedTime|createdTime' --glob 'app/**/*.{ts,tsx,js,jsx}'
rg -n -C 2 '`@robinbobin/react-native-google-drive-api-wrapper`' package.json pnpm-lock.yamlRepository: selfxyz/self
Length of output: 3171
🏁 Script executed:
# Read the complete file to examine the flagged issues
cat -n app/src/services/cloud-backup/index.ts | head -200Repository: selfxyz/self
Length of output: 6794
🏁 Script executed:
# Check wrapper API and documentation if available locally
rg -n 'orderBy|pageSize|spaces|maxResults' --glob 'node_modules/**/*robinbobin*' -A 3 | head -100Repository: selfxyz/self
Length of output: 205
🏁 Script executed:
# Specifically inspect lines around line 160 for error logging
sed -n '150,170p' app/src/services/cloud-backup/index.tsRepository: selfxyz/self
Length of output: 756
🏁 Script executed:
# Search for wrapper API documentation or type definitions in node_modules
find node_modules -path '*robinbobin*' -name '*.ts' -o -name '*.d.ts' -o -name 'README.md' 2>/dev/null | head -20Repository: selfxyz/self
Length of output: 38
🏁 Script executed:
# Check the wrapper package for list API signature
find node_modules/@robinbobin -type f \( -name '*.d.ts' -o -name 'index.js' \) 2>/dev/null | xargs grep -l 'files\.list\|function list' 2>/dev/null | head -5Repository: selfxyz/self
Length of output: 38
🌐 Web query:
Google Drive API v3 files.list orderBy parameter support query language modifiedTime
💡 Result:
The Google Drive API v3 files.list method explicitly supports modifiedTime as a valid key for the orderBy parameter [1][2]. Key details regarding this parameter include: Sorting Behavior: You can use modifiedTime to sort files by the last time they were modified by anyone [1]. By default, this sorts in ascending order, but you can append desc to the key to sort in descending order (e.g., ?orderBy=modifiedTime desc) [1][2]. Optimization: Official documentation explicitly recommends using modifiedTime for time-related sorting on large item collections because it is optimized to handle these queries efficiently, whereas other keys like createdTime may result in timeouts [1][3]. Separation of Concerns: It is important to distinguish between the orderBy parameter and the q (query) parameter [3][4]: - The orderBy parameter determines the sort order of the results returned [1]. - The q parameter is used to filter results (e.g., q=modifiedTime > '2026-06-01T00:00:00Z') using the Drive search query language [4]. The modifiedTime field is a valid attribute to filter by within these search queries as well [4]. In summary, modifiedTime is fully supported for both sorting (orderBy) and filtering (q) in the Google Drive API v3 [1][3][4].
Citations:
- 1: https://developers.google.com/workspace/drive/api/reference/rest/v3/files/list
- 2: https://googleapis.dev/nodejs/googleapis/latest/drive/interfaces/Params%24Resource%24Files%24List-1.html
- 3: https://developers.google.com/workspace/drive/api/guides/search-files
- 4: https://developers.google.com/workspace/drive/api/guides/ref-search-terms
Resolve canonical backup deterministically and add stale-cache fallback to prevent data loss and restore failures.
The resolveCanonicalDriveFileId function (lines 43–55) selects the first file returned by files.list() without specifying orderBy, meaning if Drive returns an older duplicate first, the newer backup is deleted and the stale data restored. Use orderBy: 'modifiedTime desc' to keep the most recent backup and delete older duplicates only after confirming the chosen file is readable.
The download() function (lines 87–120) caches the backup file ID but never falls back to re-listing and resolving duplicates if the cached ID becomes stale (e.g., file deleted server-side). If getText() fails on a stale cached ID, the restore fails even though a valid backup exists on Drive. Add a fallback: if getText() fails on the cached ID, clear it and call resolveCanonicalDriveFileId() again before throwing.
Line 160: Remove the raw error object from the log. Google Drive API errors can expose credentials, tokens, and session identifiers. Log only the error message: console.warn('Cloud backup update failed; creating a new file', (e as Error).message).
| const { backupFileId, setBackupFileId } = useSettingStore.getState(); | ||
| let fileId = backupFileId; | ||
| if (!fileId) { | ||
| fileId = await resolveCanonicalDriveFileId(gdrive); | ||
| if (fileId) { | ||
| setBackupFileId(fileId); | ||
| } | ||
| } | ||
|
|
||
| if (!isDriveFile(firstFile)) { | ||
| if (!fileId) { | ||
| throw new Error( | ||
| 'Couldnt find the encrypted backup, did you back it up previously?', | ||
| ); | ||
| } | ||
| const resolvedFileId = fileId; | ||
| const mnemonicString = await withRetries(() => | ||
| gdrive.files.getText(firstFile.id), | ||
| gdrive.files.getText(resolvedFileId), |
There was a problem hiding this comment.
Fall back when the cached Drive file id is stale.
When backupFileId is present, download() calls getText() directly; if that id was deleted or belongs to a stale Drive state, recovery fails without trying the name-based self-heal path. Catch the cached-id read failure, clear it, resolve the canonical id, and retry once before surfacing the restore error.
🐛 Proposed fallback shape
const { backupFileId, setBackupFileId } = useSettingStore.getState();
let fileId = backupFileId;
if (!fileId) {
fileId = await resolveCanonicalDriveFileId(gdrive);
if (fileId) {
setBackupFileId(fileId);
}
}
if (!fileId) {
throw new Error(
'Couldnt find the encrypted backup, did you back it up previously?',
);
}
- const resolvedFileId = fileId;
- const mnemonicString = await withRetries(() =>
- gdrive.files.getText(resolvedFileId),
- );
+ let mnemonicString: string;
+ try {
+ mnemonicString = await withRetries(() => gdrive.files.getText(fileId));
+ } catch (error) {
+ if (!backupFileId) {
+ throw error;
+ }
+ setBackupFileId(null);
+ const resolvedFileId = await resolveCanonicalDriveFileId(gdrive);
+ if (!resolvedFileId) {
+ throw error;
+ }
+ setBackupFileId(resolvedFileId);
+ mnemonicString = await withRetries(() =>
+ gdrive.files.getText(resolvedFileId),
+ );
+ }🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and read the relevant section
cat app/src/services/cloud-backup/index.ts | head -120 | tail -40Repository: selfxyz/self
Length of output: 1034
🏁 Script executed:
# Get the full context of the download function to understand error handling
rg -A 30 "const.*download.*=.*async" app/src/services/cloud-backup/index.tsRepository: selfxyz/self
Length of output: 38
🏁 Script executed:
# Search for tests related to cloud-backup to check if stale ID scenario is covered
find . -name "*.test.ts" -o -name "*.spec.ts" | xargs grep -l "cloud-backup\|backupFileId" 2>/dev/null | head -5Repository: selfxyz/self
Length of output: 102
🏁 Script executed:
# Check how getText() is defined and what errors it may throw
rg -B 5 -A 10 "getText\(" app/src/services/cloud-backup/index.ts | head -40Repository: selfxyz/self
Length of output: 507
🏁 Script executed:
# Read the test file to see if stale ID scenario is tested
cat app/tests/src/services/cloud-backup.test.tsRepository: selfxyz/self
Length of output: 22203
🏁 Script executed:
# Search for the gdrive implementation to understand getText() error behavior
rg -B 5 -A 15 "files.*getText" --type ts --type tsx | head -60Repository: selfxyz/self
Length of output: 84
🏁 Script executed:
# Check if there's any error handling in withRetries wrapper
rg -B 5 -A 15 "withRetries" app/src/services/cloud-backup/index.tsRepository: selfxyz/self
Length of output: 2534
🏁 Script executed:
# Check the upload function to see if it has similar error handling for stale IDs
rg -B 10 -A 20 "Cloud backup update failed" app/src/services/cloud-backup/index.tsRepository: selfxyz/self
Length of output: 918
🏁 Script executed:
# Check withRetries implementation to understand what errors it might catch/rethrow
fd -e ts -e tsx | xargs rg -l "withRetries" | head -3Repository: selfxyz/self
Length of output: 155
🏁 Script executed:
# Find the withRetries utility
rg -B 3 -A 12 "export.*withRetries" app/src/utils/retry.tsRepository: selfxyz/self
Length of output: 547
Add stale cache detection and recovery to download() to match error handling in upload().
When backupFileId is cached, download() calls getText() directly without error handling. If that file ID was deleted or belongs to stale Drive state, the function fails hard. The upload() function already implements the correct pattern: catch the error, clear the stale ID from cache, resolve the canonical file ID via name-based lookup, and retry. Apply the same approach to download() to recover from stale cached IDs before surfacing the restore error.
The test suite covers the happy path but does not exercise the getText failure scenario, leaving this recovery path untested.
Proposed fix shape
const { backupFileId, setBackupFileId } = useSettingStore.getState();
let fileId = backupFileId;
if (!fileId) {
fileId = await resolveCanonicalDriveFileId(gdrive);
if (fileId) {
setBackupFileId(fileId);
}
}
if (!fileId) {
throw new Error(
'Couldnt find the encrypted backup, did you back it up previously?',
);
}
- const resolvedFileId = fileId;
- const mnemonicString = await withRetries(() =>
- gdrive.files.getText(resolvedFileId),
- );
+ let mnemonicString: string;
+ try {
+ mnemonicString = await withRetries(() => gdrive.files.getText(fileId));
+ } catch (error) {
+ if (!backupFileId) {
+ throw error;
+ }
+ setBackupFileId(null);
+ const resolvedFileId = await resolveCanonicalDriveFileId(gdrive);
+ if (!resolvedFileId) {
+ throw error;
+ }
+ setBackupFileId(resolvedFileId);
+ mnemonicString = await withRetries(() =>
+ gdrive.files.getText(resolvedFileId),
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { backupFileId, setBackupFileId } = useSettingStore.getState(); | |
| let fileId = backupFileId; | |
| if (!fileId) { | |
| fileId = await resolveCanonicalDriveFileId(gdrive); | |
| if (fileId) { | |
| setBackupFileId(fileId); | |
| } | |
| } | |
| if (!isDriveFile(firstFile)) { | |
| if (!fileId) { | |
| throw new Error( | |
| 'Couldnt find the encrypted backup, did you back it up previously?', | |
| ); | |
| } | |
| const resolvedFileId = fileId; | |
| const mnemonicString = await withRetries(() => | |
| gdrive.files.getText(firstFile.id), | |
| gdrive.files.getText(resolvedFileId), | |
| const { backupFileId, setBackupFileId } = useSettingStore.getState(); | |
| let fileId = backupFileId; | |
| if (!fileId) { | |
| fileId = await resolveCanonicalDriveFileId(gdrive); | |
| if (fileId) { | |
| setBackupFileId(fileId); | |
| } | |
| } | |
| if (!fileId) { | |
| throw new Error( | |
| 'Couldnt find the encrypted backup, did you back it up previously?', | |
| ); | |
| } | |
| let mnemonicString: string; | |
| try { | |
| mnemonicString = await withRetries(() => gdrive.files.getText(fileId)); | |
| } catch (error) { | |
| if (!backupFileId) { | |
| throw error; | |
| } | |
| setBackupFileId(null); | |
| const resolvedFileId = await resolveCanonicalDriveFileId(gdrive); | |
| if (!resolvedFileId) { | |
| throw error; | |
| } | |
| setBackupFileId(resolvedFileId); | |
| mnemonicString = await withRetries(() => | |
| gdrive.files.getText(resolvedFileId), | |
| ); | |
| } |
| } catch (e) { | ||
| // Stored id may be stale (file deleted server-side); fall back to create. | ||
| console.warn('Cloud backup update failed; creating a new file', e); | ||
| setBackupFileId(null); | ||
| } |
There was a problem hiding this comment.
Don’t log raw Drive errors.
The raw error object from the Drive update path can contain request/response metadata; avoid emitting headers, tokens, or session identifiers into app logs. Log a fixed sanitized message instead. As per coding guidelines, “NEVER log sensitive data including PII, credentials, tokens, API keys, private keys, or session identifiers.”
🔒 Proposed log sanitization
} catch (e) {
// Stored id may be stale (file deleted server-side); fall back to create.
- console.warn('Cloud backup update failed; creating a new file', e);
+ console.warn('Cloud backup update failed; creating a new file');
setBackupFileId(null);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (e) { | |
| // Stored id may be stale (file deleted server-side); fall back to create. | |
| console.warn('Cloud backup update failed; creating a new file', e); | |
| setBackupFileId(null); | |
| } | |
| } catch (e) { | |
| // Stored id may be stale (file deleted server-side); fall back to create. | |
| console.warn('Cloud backup update failed; creating a new file'); | |
| setBackupFileId(null); | |
| } |
Source: Coding guidelines
Don’t log raw Drive errors.
The raw error object from the Drive update path can contain request/response metadata; avoid emitting headers, tokens, or session identifiers into app logs. Log a sanitized status/message instead. As per coding guidelines, “NEVER log sensitive data including PII, credentials, tokens, API keys, private keys, or session identifiers.”
🔒 Proposed log sanitization
} catch (e) {
// Stored id may be stale (file deleted server-side); fall back to create.
- console.warn('Cloud backup update failed; creating a new file', e);
+ console.warn(
+ 'Cloud backup update failed; creating a new file',
+ e instanceof Error ? e.message : 'Unknown error',
+ );
setBackupFileId(null);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (e) { | |
| // Stored id may be stale (file deleted server-side); fall back to create. | |
| console.warn('Cloud backup update failed; creating a new file', e); | |
| setBackupFileId(null); | |
| } | |
| } catch (e) { | |
| // Stored id may be stale (file deleted server-side); fall back to create. | |
| console.warn( | |
| 'Cloud backup update failed; creating a new file', | |
| e instanceof Error ? e.message : 'Unknown error', | |
| ); | |
| setBackupFileId(null); | |
| } |
Source: Coding guidelines
| } catch (e) { | ||
| // Stored id may be stale (file deleted server-side); fall back to create. | ||
| console.warn('Cloud backup update failed; creating a new file', e); | ||
| setBackupFileId(null); | ||
| } | ||
| await withRetries(() => | ||
| gdrive.files | ||
| .newMultipartUploader() | ||
| .setData(JSON.stringify(mnemonic)) | ||
| .setDataMimeType(MIME_TYPES.application.json) | ||
| .setRequestBody({ name: FILE_NAME, parents: [APP_DATA_FOLDER_ID] }) | ||
| .execute(), | ||
| ); | ||
| } | ||
|
|
||
| const created = await withRetries(() => | ||
| gdrive.files | ||
| .newMultipartUploader() | ||
| .setData(data) | ||
| .setDataMimeType(MIME_TYPES.application.json) | ||
| .setRequestBody({ name: FILE_NAME, parents: [APP_DATA_FOLDER_ID] }) | ||
| .execute(), | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the cloud-backup file structure and content
cd app/src/services/cloud-backup && find . -type f && wc -l index.tsRepository: selfxyz/self
Length of output: 114
🏁 Script executed:
# Get the full context of the backup file, focusing on the relevant function
cat -n app/src/services/cloud-backup/index.ts | head -200Repository: selfxyz/self
Length of output: 6794
🏁 Script executed:
# Look for setBackupFileId usage and backupFileId references
rg -n "backupFileId" app/src/services/cloud-backup/Repository: selfxyz/self
Length of output: 464
🏁 Script executed:
# Check if there's a name-based lookup function for existing backups
rg -n "FILE_NAME\|gdrive\.files\." app/src/services/cloud-backup/ -A 3 -B 3Repository: selfxyz/self
Length of output: 38
🏁 Script executed:
# Check the withRetries implementation to understand what errors it retries
rg -n "withRetries" app/src/utils/retry/ -A 10Repository: selfxyz/self
Length of output: 117
🏁 Script executed:
# Check if withRetries is exported from utils/retry
cat -n app/src/utils/retry.tsRepository: selfxyz/self
Length of output: 1509
Re-resolve by name before creating after an update failure.
If the update fails (after 10 retries) but the file still exists server-side, the catch handler clears the cache and immediately creates a new Drive file, introducing duplicates instead of adopting the existing canonical file. Call resolveCanonicalDriveFileId() again after the update fails; if a file is found, attempt the update again or proceed to creation only if resolution returns null.
1 participant
Linear: SELF-3180
Pilot of the Codebase Audits workstream. This PR contains the AUD-02 findings report and the remediation for the owner-accepted findings (two commits: audit doc + tests, then fixes).
Audit
Read-only audit of mnemonic/keychain generation, storage, migration, biometric gating, cloud backup/restore, and the vendored keychain patch. Full report:
docs/reviews/2026-06-17-key-material-keychain-audit.md. Every Critical/Major was independently refutation-verified.Fixes in this PR
loadOrCreateMnemonicno longer overwrites a present-but-unparseable secret. It fails closed (fireskeychainCryptoFailureCallback('crypto_failed'), returnsfalse), so the create/overwrite path only runs when no secret existed. Corrupted/legacy entries route to recovery instead of being destroyed.hasSecretStoredreturns a tri-state (present | absent | unknown) instead of collapsing read errors tofalse.SplashScreentreats only an explicitabsentas a fresh install, so a transient keychain error no longer misroutes an existing user into onboarding/recovery.uploadupdates in place viasetIdOfFileToUpdate(newsettingStore.backupFileId), adopting/deduping pre-existing duplicates; download/delete operate by id with a list+dedupe self-heal fallback. iOS already overwrites a fixed path.useStrongBoxJSDoc (effective default isfalse; StrongBox off by default due to Samsung-device issues).Owner dispositions (recorded in the report)
Validation
Run in-worktree after building
@selfxyz/common,mobile-sdk-alpha,webview-bridge:jest(authProvider + cloud-backup): 33 passedtsc --noEmit: cleancheck-test-requires.cjs: cleanThe F-02 characterization test is committed pinning current behavior (audit commit) then flipped to assert the fix (remediation commit).
Reviewer note
Both commits used
git commit --no-verify: the pre-commit hook runsyarn gitleaks, butdevis on pnpm so the hook errors before scanning.gitleaks protect --staged --config=gitleaks-override.tomlwas run directly on each commit — no leaks. The staleyarn-based hook is worth a separate fix.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests