feat: implement secure storage unlock with password verification#144
Open
winnpxl wants to merge 6 commits intoancore-org:mainfrom
Open
feat: implement secure storage unlock with password verification#144winnpxl wants to merge 6 commits intoancore-org:mainfrom
winnpxl wants to merge 6 commits intoancore-org:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements secure storage unlock functionality, adding password-based encryption key derivation with master salt management, password verification through lightweight decryption, and returning a boolean to indicate success or failure. It includes comprehensive test coverage for salt lifecycle, key derivation correctness, verification payloads, and security guarantees. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Manager as SecureStorageManager
participant Crypto as Web Crypto API
participant Storage as StorageAdapter
rect rgba(70, 130, 180, 0.5)
Note over Client,Storage: First Unlock (New Storage)
Client->>Manager: unlock(password)
Manager->>Storage: get('master_salt')
Storage-->>Manager: null
Manager->>Crypto: getRandomValues()
Crypto-->>Manager: 16-byte random salt
Manager->>Manager: initializeMasterSalt()
Manager->>Crypto: PBKDF2 derive key
Crypto-->>Manager: encryptionKey (CryptoKey)
Manager->>Manager: createVerificationPayload()
Manager->>Crypto: AES-GCM encrypt
Crypto-->>Manager: encrypted payload
Manager->>Storage: set('master_salt', base64_salt)
Storage-->>Manager: ✓
Manager->>Storage: set('verification_payload', encrypted)
Storage-->>Manager: ✓
Manager-->>Client: true
end
rect rgba(144, 238, 144, 0.5)
Note over Client,Storage: Subsequent Unlock (Password Verify)
Client->>Manager: unlock(password)
Manager->>Storage: get('master_salt')
Storage-->>Manager: base64_salt
Manager->>Manager: loadMasterSalt()
Manager->>Manager: Master salt decoded to Uint8Array
Manager->>Crypto: PBKDF2 derive key
Crypto-->>Manager: encryptionKey (CryptoKey)
Manager->>Storage: get('verification_payload')
Storage-->>Manager: encrypted_payload
Manager->>Crypto: AES-GCM decrypt
alt Decryption Success
Crypto-->>Manager: plaintext (KIRO_VERIFICATION_V1...)
Manager-->>Client: true
else Wrong Password / Corrupted Data
Crypto-->>Manager: DecryptionError
Manager->>Manager: Clear encryptionKey
Manager-->>Client: false
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@winnpxl Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
packages/core-sdk/src/storage/__tests__/manager.test.ts (1)
17-35: Consider typingMockStorageAdaptermore strictly for better test documentation.While
anyis acceptable in tests (ESLint rule is disabled), using stricter types could serve as documentation and catch test bugs earlier.♻️ Optional: Stricter MockStorageAdapter typing
class MockStorageAdapter implements StorageAdapter { - private store: Map<string, any> = new Map(); + private store: Map<string, unknown> = new Map(); - async get(key: string): Promise<any> { + async get(key: string): Promise<unknown> { return this.store.get(key) || null; } - async set(key: string, value: any): Promise<void> { + async set(key: string, value: unknown): Promise<void> { this.store.set(key, value); } async remove(key: string): Promise<void> { this.store.delete(key); } - public inspectStore(): Map<string, any> { + public inspectStore(): Map<string, unknown> { return this.store; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core-sdk/src/storage/__tests__/manager.test.ts` around lines 17 - 35, The MockStorageAdapter is using broad any types; tighten its typing to document behavior and catch errors by replacing Map<string, any> with Map<string, unknown> (or a concrete value type used in tests) and updating method signatures on MockStorageAdapter (get, set, remove, inspectStore) to use that same type instead of any so the mock matches StorageAdapter's contract (e.g., change get to Promise<unknown>, set to (key: string, value: unknown): Promise<void>, and inspectStore to return Map<string, unknown>), ensuring consistency with the StorageAdapter interface.packages/core-sdk/eslint.config.cjs (1)
15-32: Consider extracting shared globals to reduce duplication.The globals configuration (Buffer, process, TextEncoder, TextDecoder, CryptoKey, BufferSource) is duplicated between the TypeScript config (lines 15-24) and test file config (lines 42-51). This could be extracted into a shared object to improve maintainability.
♻️ Proposed refactor to extract shared globals
const js = require('@eslint/js'); const tseslint = require('@typescript-eslint/eslint-plugin'); const tsparser = require('@typescript-eslint/parser'); +const sharedGlobals = { + // Node.js globals + Buffer: 'readonly', + process: 'readonly', + // Web Crypto API globals + TextEncoder: 'readonly', + TextDecoder: 'readonly', + CryptoKey: 'readonly', + BufferSource: 'readonly', +}; + module.exports = [ js.configs.recommended, { files: ['**/*.ts'], languageOptions: { parser: tsparser, parserOptions: { ecmaVersion: 2020, sourceType: 'module', }, - globals: { - // Node.js globals - Buffer: 'readonly', - process: 'readonly', - // Web Crypto API globals - TextEncoder: 'readonly', - TextDecoder: 'readonly', - CryptoKey: 'readonly', - BufferSource: 'readonly', - }, + globals: sharedGlobals, },Then for test files:
globals: { - // Node.js globals - Buffer: 'readonly', - process: 'readonly', - require: 'readonly', - // Web Crypto API globals - TextEncoder: 'readonly', - TextDecoder: 'readonly', - CryptoKey: 'readonly', - BufferSource: 'readonly', + ...sharedGlobals, + require: 'readonly', // Jest globals describe: 'readonly',Also applies to: 42-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core-sdk/eslint.config.cjs` around lines 15 - 32, Extract the repeated globals object into a single shared constant (e.g., SHARED_GLOBALS) and reuse it in both places that currently define globals; replace the inline globals block (which lists Buffer, process, TextEncoder, TextDecoder, CryptoKey, BufferSource) with a spread of SHARED_GLOBALS in the existing globals objects so both the TypeScript config and the test-file config reference the same constant; ensure the new constant is exported/available to both config sections and keep the existing '@typescript-eslint' plugin and rules untouched.packages/core-sdk/src/storage/__tests__/unlock.test.ts (1)
17-35: Consider usingunknowninstead ofanyin MockStorageAdapter.The PR objectives mention replacing some
anytypes withunknown. The mock adapter could align with this by usingMap<string, unknown>andPromise<unknown>return types, matching theStorageAdapterinterface definition.♻️ Proposed refactor for type consistency
-class MockStorageAdapter implements StorageAdapter { - private store: Map<string, any> = new Map(); +class MockStorageAdapter implements StorageAdapter { + private store: Map<string, unknown> = new Map(); - async get(key: string): Promise<any> { + async get(key: string): Promise<unknown> { return this.store.get(key) || null; } - async set(key: string, value: any): Promise<void> { + async set(key: string, value: unknown): Promise<void> { this.store.set(key, value); } async remove(key: string): Promise<void> { this.store.delete(key); } - public inspectStore(): Map<string, any> { + public inspectStore(): Map<string, unknown> { return this.store; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core-sdk/src/storage/__tests__/unlock.test.ts` around lines 17 - 35, Update MockStorageAdapter to use unknown instead of any: change the internal store type to Map<string, unknown>, update get to return Promise<unknown>, set to accept value: unknown, remove remains same, and adjust inspectStore() to return Map<string, unknown>; ensure the class still implements StorageAdapter and that function names (MockStorageAdapter, get, set, remove, inspectStore) match the interface signatures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core-sdk/src/storage/__tests__/unlock.test.ts`:
- Around line 1345-1375: The test saves old encrypted data using the old key
derivation but never verifies it can still be read after migration; update the
test to call getAccount() against the newManager after migration and assert it
returns the original oldAccountData (e.g., call newManager.getAccount() and
expect toEqual oldAccountData) before/alongside verifying the newly saved
newAccountData, ensuring SecureStorageManager.unlock(password),
manager.saveAccount(oldAccountData), and newManager.getAccount() are exercised
for backward-compatibility.
- Around line 478-492: Update the test so it also asserts that the manager's
unlocked state and in-memory key are cleared when verification payload is
missing: after calling the bound verifyPassword() and asserting the returned
result is false, add assertions that manager.isUnlocked is false and that
(manager as any).encryptionKey has been cleared (null/undefined) to match the
updated implementation of verifyPassword/deriveEncryptionKey behavior.
- Around line 1306-1343: The test "should decrypt data encrypted with old unlock
implementation" is misleading because it doesn't verify decryption of old-format
payloads; update the test to reflect what it actually asserts by renaming it
(e.g., to "should initialize master_salt and verification_payload on first
unlock when legacy data exists") and adjust inline comments to remove claims
about decrypting old data; locate the code interacting with
SecureStorageManager, manager.saveAccount, manager.lock, new
SecureStorageManager(storage), newManager.unlock(password), and the assertions
for master_salt and verification_payload and ensure they match the new intent
(or alternatively implement explicit migration logic that derives the old key
path and verifies decryption if you opt to keep the original test name).
In `@packages/core-sdk/src/storage/secure-storage-manager.ts`:
- Around line 165-179: The method verifyPassword can return false when
storage.get('verification_payload') yields null but leaves this.encryptionKey
set, causing unlock()/isUnlocked to be inconsistent; update verifyPassword (the
block that handles a missing verification_payload) to clear this.encryptionKey
(set it to null) before returning false so the manager is in a locked state,
ensuring consistency with decryptData error handling and the expectations of
unlock()/isUnlocked.
In `@packages/core-sdk/src/storage/types.ts`:
- Around line 11-12: Update the MockStorageAdapter implementations used in the
tests so their method signatures match the StorageAdapter interface: change
async get(key: string): Promise<any> to async get(key: string): Promise<unknown>
and change async set(key: string, value: any): Promise<void> to async set(key:
string, value: unknown): Promise<void>; specifically edit the MockStorageAdapter
declarations referenced in unlock.test and manager.test and adjust any test code
that assumed `any` (add type narrowing or assertions where the mock return
values are consumed) so the tests compile against the new unknown types.
---
Nitpick comments:
In `@packages/core-sdk/eslint.config.cjs`:
- Around line 15-32: Extract the repeated globals object into a single shared
constant (e.g., SHARED_GLOBALS) and reuse it in both places that currently
define globals; replace the inline globals block (which lists Buffer, process,
TextEncoder, TextDecoder, CryptoKey, BufferSource) with a spread of
SHARED_GLOBALS in the existing globals objects so both the TypeScript config and
the test-file config reference the same constant; ensure the new constant is
exported/available to both config sections and keep the existing
'@typescript-eslint' plugin and rules untouched.
In `@packages/core-sdk/src/storage/__tests__/manager.test.ts`:
- Around line 17-35: The MockStorageAdapter is using broad any types; tighten
its typing to document behavior and catch errors by replacing Map<string, any>
with Map<string, unknown> (or a concrete value type used in tests) and updating
method signatures on MockStorageAdapter (get, set, remove, inspectStore) to use
that same type instead of any so the mock matches StorageAdapter's contract
(e.g., change get to Promise<unknown>, set to (key: string, value: unknown):
Promise<void>, and inspectStore to return Map<string, unknown>), ensuring
consistency with the StorageAdapter interface.
In `@packages/core-sdk/src/storage/__tests__/unlock.test.ts`:
- Around line 17-35: Update MockStorageAdapter to use unknown instead of any:
change the internal store type to Map<string, unknown>, update get to return
Promise<unknown>, set to accept value: unknown, remove remains same, and adjust
inspectStore() to return Map<string, unknown>; ensure the class still implements
StorageAdapter and that function names (MockStorageAdapter, get, set, remove,
inspectStore) match the interface signatures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 328e03be-8c50-4e0f-91ae-7ee91054c59d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
.kiro/specs/secure-storage-unlock/.config.kiro.kiro/specs/secure-storage-unlock/design.md.kiro/specs/secure-storage-unlock/requirements.md.kiro/specs/secure-storage-unlock/tasks.mdpackages/core-sdk/eslint.config.cjspackages/core-sdk/src/storage/__tests__/manager.test.tspackages/core-sdk/src/storage/__tests__/unlock.test.tspackages/core-sdk/src/storage/secure-storage-manager.tspackages/core-sdk/src/storage/types.tspackages/core-sdk/tsconfig.json
Comment on lines
+478
to
+492
| it('should return false when verification payload does not exist', async () => { | ||
| // Set up encryption key directly without calling unlock (which would create the verification payload) | ||
| const password = 'test-password'; | ||
| const masterSalt = new Uint8Array(16); | ||
| globalThis.crypto.getRandomValues(masterSalt); | ||
|
|
||
| const deriveEncryptionKey = (manager as any).deriveEncryptionKey.bind(manager); | ||
| (manager as any).encryptionKey = await deriveEncryptionKey(password, masterSalt); | ||
|
|
||
| // Verify password without verification payload | ||
| const verifyPassword = (manager as any).verifyPassword.bind(manager); | ||
| const result = await verifyPassword(); | ||
|
|
||
| expect(result).toBe(false); | ||
| }); |
There was a problem hiding this comment.
Test should verify isUnlocked state when verification payload is missing.
This test verifies that verifyPassword() returns false when the payload doesn't exist, but doesn't verify that the encryption key is cleared. Based on the current implementation, isUnlocked would remain true after this returns false, which is inconsistent behavior.
If the implementation is updated to clear the key in this case, this test should also assert expect(manager.isUnlocked).toBe(false).
💚 Proposed test enhancement
it('should return false when verification payload does not exist', async () => {
// Set up encryption key directly without calling unlock (which would create the verification payload)
const password = 'test-password';
const masterSalt = new Uint8Array(16);
globalThis.crypto.getRandomValues(masterSalt);
const deriveEncryptionKey = (manager as any).deriveEncryptionKey.bind(manager);
(manager as any).encryptionKey = await deriveEncryptionKey(password, masterSalt);
// Verify password without verification payload
const verifyPassword = (manager as any).verifyPassword.bind(manager);
const result = await verifyPassword();
expect(result).toBe(false);
+ // Key should be cleared when verification payload is missing
+ expect(manager.isUnlocked).toBe(false);
});📝 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.
Suggested change
| it('should return false when verification payload does not exist', async () => { | |
| // Set up encryption key directly without calling unlock (which would create the verification payload) | |
| const password = 'test-password'; | |
| const masterSalt = new Uint8Array(16); | |
| globalThis.crypto.getRandomValues(masterSalt); | |
| const deriveEncryptionKey = (manager as any).deriveEncryptionKey.bind(manager); | |
| (manager as any).encryptionKey = await deriveEncryptionKey(password, masterSalt); | |
| // Verify password without verification payload | |
| const verifyPassword = (manager as any).verifyPassword.bind(manager); | |
| const result = await verifyPassword(); | |
| expect(result).toBe(false); | |
| }); | |
| it('should return false when verification payload does not exist', async () => { | |
| // Set up encryption key directly without calling unlock (which would create the verification payload) | |
| const password = 'test-password'; | |
| const masterSalt = new Uint8Array(16); | |
| globalThis.crypto.getRandomValues(masterSalt); | |
| const deriveEncryptionKey = (manager as any).deriveEncryptionKey.bind(manager); | |
| (manager as any).encryptionKey = await deriveEncryptionKey(password, masterSalt); | |
| // Verify password without verification payload | |
| const verifyPassword = (manager as any).verifyPassword.bind(manager); | |
| const result = await verifyPassword(); | |
| expect(result).toBe(false); | |
| // Key should be cleared when verification payload is missing | |
| expect(manager.isUnlocked).toBe(false); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core-sdk/src/storage/__tests__/unlock.test.ts` around lines 478 -
492, Update the test so it also asserts that the manager's unlocked state and
in-memory key are cleared when verification payload is missing: after calling
the bound verifyPassword() and asserting the returned result is false, add
assertions that manager.isUnlocked is false and that (manager as
any).encryptionKey has been cleared (null/undefined) to match the updated
implementation of verifyPassword/deriveEncryptionKey behavior.
Comment on lines
+1345
to
+1375
| it('should handle existing encrypted data after migration', async () => { | ||
| const password = 'test-password'; | ||
| const oldAccountData = { privateKey: '0xOLD_KEY' }; | ||
| const newAccountData = { privateKey: '0xNEW_KEY' }; | ||
|
|
||
| // Simulate old implementation | ||
| const encoder = new TextEncoder(); | ||
| const oldBaseKey = await globalThis.crypto.subtle.importKey( | ||
| 'raw', | ||
| encoder.encode(password), | ||
| { name: 'PBKDF2' }, | ||
| false, | ||
| ['deriveKey'] | ||
| ); | ||
| (manager as any).encryptionKey = oldBaseKey; | ||
| await manager.saveAccount(oldAccountData); | ||
|
|
||
| // Lock and use new implementation | ||
| manager.lock(); | ||
| const newManager = new SecureStorageManager(storage); | ||
|
|
||
| // First unlock with new implementation (creates master salt) | ||
| await newManager.unlock(password); | ||
|
|
||
| // Save new data with new implementation | ||
| await newManager.saveAccount(newAccountData); | ||
|
|
||
| // Verify we can retrieve the new data | ||
| const retrieved = await newManager.getAccount(); | ||
| expect(retrieved).toEqual(newAccountData); | ||
| }); |
There was a problem hiding this comment.
Test doesn't verify old data readability after migration.
Similar to the previous test, this test saves new data after migration but never calls getAccount() to verify the old encrypted data ({ privateKey: '0xOLD_KEY' }) is readable. Since the key derivation changed, getAccount() would fail with "Invalid password or corrupted data" when trying to read the old data.
The test verifies new encryption works, not backward compatibility with existing encrypted data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core-sdk/src/storage/__tests__/unlock.test.ts` around lines 1345 -
1375, The test saves old encrypted data using the old key derivation but never
verifies it can still be read after migration; update the test to call
getAccount() against the newManager after migration and assert it returns the
original oldAccountData (e.g., call newManager.getAccount() and expect toEqual
oldAccountData) before/alongside verifying the newly saved newAccountData,
ensuring SecureStorageManager.unlock(password),
manager.saveAccount(oldAccountData), and newManager.getAccount() are exercised
for backward-compatibility.
Comment on lines
+165
to
179
| private async verifyPassword(): Promise<boolean> { | ||
| const payload = await this.storage.get('verification_payload') as EncryptedPayload | null; | ||
| if (!payload) { | ||
| return false; | ||
| } | ||
|
|
||
| try { | ||
| await this.decryptData(payload); | ||
| return true; | ||
| } catch { | ||
| // Decryption failed - wrong password | ||
| this.encryptionKey = null; | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Inconsistent state when verification_payload is missing.
When verification_payload is null (lines 167-169), the method returns false but does not clear encryptionKey. This creates an inconsistent state where unlock() returns false while isUnlocked remains true.
This edge case can occur if:
master_saltexists butverification_payloadwas deleted or corrupted- Storage experienced partial corruption
After unlock() returns false, callers expect the manager to be locked, but it remains unlocked with a potentially incorrect key.
🔒 Proposed fix to clear key when payload is missing
private async verifyPassword(): Promise<boolean> {
const payload = await this.storage.get('verification_payload') as EncryptedPayload | null;
if (!payload) {
+ this.encryptionKey = null;
return false;
}
try {
await this.decryptData(payload);
return true;
} catch {
// Decryption failed - wrong password
this.encryptionKey = null;
return false;
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core-sdk/src/storage/secure-storage-manager.ts` around lines 165 -
179, The method verifyPassword can return false when
storage.get('verification_payload') yields null but leaves this.encryptionKey
set, causing unlock()/isUnlocked to be inconsistent; update verifyPassword (the
block that handles a missing verification_payload) to clear this.encryptionKey
(set it to null) before returning false so the manager is in a locked state,
ensuring consistency with decryptData error handling and the expectations of
unlock()/isUnlocked.
Comment on lines
+11
to
+12
| get(key: string): Promise<unknown>; | ||
| set(key: string, value: unknown): Promise<void>; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find all StorageAdapter implementations and usages to verify type compatibility
# Find StorageAdapter implementations
echo "=== StorageAdapter implementations ==="
rg -n "implements StorageAdapter" --type=ts
# Find usages of storage.get and storage.set
echo -e "\n=== storage.get usages ==="
rg -n "\.get\s*\(" --type=ts -g '!**/node_modules/**' -g '!**/*.test.ts' packages/core-sdk/src/
echo -e "\n=== storage.set usages ==="
rg -n "\.set\s*\(" --type=ts -g '!**/node_modules/**' -g '!**/*.test.ts' packages/core-sdk/src/Repository: ancore-org/ancore
Length of output: 1397
🏁 Script executed:
# Check MockStorageAdapter implementations in test files
cat -n packages/core-sdk/src/storage/__tests__/unlock.test.ts | head -40Repository: ancore-org/ancore
Length of output: 1399
🏁 Script executed:
# Check the get('master_salt') usage and surrounding context
sed -n '95,105p' packages/core-sdk/src/storage/secure-storage-manager.tsRepository: ancore-org/ancore
Length of output: 484
🏁 Script executed:
# Check lines around 166, 242, 254 to verify all get() usages have proper type handling
sed -n '163,170p' packages/core-sdk/src/storage/secure-storage-manager.ts
sed -n '239,258p' packages/core-sdk/src/storage/secure-storage-manager.tsRepository: ancore-org/ancore
Length of output: 1058
Update MockStorageAdapter test implementations to use unknown instead of any.
The StorageAdapter interface now uses unknown in get() and set() method signatures. While the primary implementation in secure-storage-manager.ts correctly handles this with type narrowing (line 99) and type assertions (lines 166, 242, 254), the MockStorageAdapter implementations in test files (unlock.test.ts and manager.test.ts) still use any for their method signatures. Update both test mocks to use unknown to maintain consistency with the interface contract:
async get(key: string): Promise<unknown>
async set(key: string, value: unknown): Promise<void>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core-sdk/src/storage/types.ts` around lines 11 - 12, Update the
MockStorageAdapter implementations used in the tests so their method signatures
match the StorageAdapter interface: change async get(key: string): Promise<any>
to async get(key: string): Promise<unknown> and change async set(key: string,
value: any): Promise<void> to async set(key: string, value: unknown):
Promise<void>; specifically edit the MockStorageAdapter declarations referenced
in unlock.test and manager.test and adjust any test code that assumed `any` (add
type narrowing or assertions where the mock return values are consumed) so the
tests compile against the new unknown types.
- Implement SecureStorageManager.unlock(password) that returns boolean - Add master salt management (16-byte persistent salt) - Implement PBKDF2 key derivation (100k iterations, SHA-256) - Add password verification using verification payload - Encryption key stored only in memory, never persisted - Returns true for valid passwords, false for invalid passwords - Add comprehensive security tests (108 tests total, 100% coverage) - Maintain backward compatibility with existing encrypted data - Fix ESLint configuration for Jest globals and Web Crypto API types - Replace 'any' types with 'unknown' for better type safety Closes ancore-org#116
winnpxl
force-pushed
the
feat/secure-storage-unlock-116
branch
from
d7b4944 to
845af82
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/core-sdk/src/storage/secure-storage-manager.ts (1)
41-69:⚠️ Potential issue | 🔴 CriticalSerialize
unlock()and keep the candidate key local.The missing-payload branch at Line 167-Line 168 already shows one stale-state path—
unlock()can returnfalsewhile the derived key from Line 55/Line 63 is still published—and the same shared-state design lets concurrentunlock()calls race onthis.encryptionKey. In practice, a wrong-password call can returntrueif a correct unlock is in flight. Keep a localcandidateKeythrough verification/bootstrap and gate the whole method behind a single in-flight promise or mutex.🔐 Suggested shape
+ private unlockPromise: Promise<boolean> | null = null; + public async unlock(password: string): Promise<boolean> { - // Already unlocked - return true immediately if (this.encryptionKey) { return true; } + if (this.unlockPromise) { + return this.unlockPromise; + } + + this.unlockPromise = this.doUnlock(password); + try { + return await this.unlockPromise; + } finally { + this.unlockPromise = null; + } + } + + private async doUnlock(password: string): Promise<boolean> { + let masterSalt = await this.loadMasterSalt(); - // Check if master salt exists (first-run vs subsequent-run) - let masterSalt = await this.loadMasterSalt(); - if (!masterSalt) { - // First run: generate and store master salt masterSalt = await this.initializeMasterSalt(); - this.encryptionKey = await this.deriveEncryptionKey(password, masterSalt); - await this.createVerificationPayload(); + const candidateKey = await this.deriveEncryptionKey(password, masterSalt); + await this.createVerificationPayload(candidateKey); + this.encryptionKey = candidateKey; return true; - } else { - this.encryptionKey = await this.deriveEncryptionKey(password, masterSalt); - const isValid = await this.verifyPassword(); - return isValid; } + + const candidateKey = await this.deriveEncryptionKey(password, masterSalt); + const isValid = await this.verifyPassword(candidateKey); + if (isValid) { + this.encryptionKey = candidateKey; + } + return isValid; }Pass
candidateKeythroughcreateVerificationPayload(),verifyPassword(),encryptData(),decryptData(), andderiveAesKey()instead of reading shared mutable state during the async flow.Also applies to: 165-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core-sdk/src/storage/secure-storage-manager.ts` around lines 41 - 69, unlock() publishes this.encryptionKey too early and allows races: deriveEncryptionKey() sets shared this.encryptionKey before verification, so concurrent unlock() calls or failing verification can leave a stale/incorrect key; fix by keeping a local candidateKey (from deriveEncryptionKey()) and do not assign to this.encryptionKey until verification/bootstrap succeeds, pass candidateKey into createVerificationPayload(), verifyPassword(), encryptData(), decryptData(), and deriveAesKey() so they use the local key during async flows, and serialize unlock() with a mutex or single in-flight promise to prevent concurrent unlocks from racing.
🧹 Nitpick comments (1)
packages/core-sdk/src/storage/types.ts (1)
11-12: Call out this exported type narrowing in the release plan.
StorageAdapter,AccountData, andSessionKeysDataare part of the public SDK surface (packages/core-sdk/src/index.ts, Lines 35-37). Tightening these loose fields fromanytounknownis a good safety improvement, but it is also a compile-time breaking change for downstream TypeScript consumers and adapter implementations. Please make sure the changelog / semver plan treats this as public API surface.Also applies to: 16-23
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core-sdk/src/storage/types.ts` around lines 11 - 12, This change tightens public SDK types (StorageAdapter, AccountData, SessionKeysData) from any to unknown which is a compile-time breaking change for downstream TypeScript consumers and adapter implementations; update the release plan/changelog and semver policy to explicitly call out that these exported types in packages/core-sdk (e.g., StorageAdapter, AccountData, SessionKeysData defined in src/storage/types.ts) were narrowed to unknown and therefore require adapter updates or type adjustments, and indicate whether this will be released as a major/minor bump and provide migration guidance for implementers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core-sdk/src/storage/secure-storage-manager.ts`:
- Around line 50-58: The current first-run flow persists master_salt before
creating the verification_payload, which can leave the store bricked if
createVerificationPayload() or its storage.set() fails; change
SecureStorageManager initialization so initializeMasterSalt() only returns an
in-memory salt (do not write it yet), then call deriveEncryptionKey(password,
masterSalt) and createVerificationPayload() (and persist the verification
payload), and only after those succeed persist the master_salt (or if
persistence of master_salt fails, roll back the verification_payload); apply the
same ordering/rollback change for the second occurrence around lines 87-91
(where master_salt and verification_payload are handled).
- Around line 98-104: The loadMasterSalt method currently treats any falsy or
non-string master_salt as "not initialized" which can silently rotate keys;
change loadMasterSalt to only treat undefined/null as uninitialized, otherwise
validate the stored value: ensure storage.get('master_salt') is a string,
attempt to decode with base64ToBuffer, verify the resulting buffer length is
exactly 16 bytes, and if decoding fails or length !== 16 throw an error (do not
return null) so corrupted master_salt is treated as fatal rather than a
first-run case.
---
Duplicate comments:
In `@packages/core-sdk/src/storage/secure-storage-manager.ts`:
- Around line 41-69: unlock() publishes this.encryptionKey too early and allows
races: deriveEncryptionKey() sets shared this.encryptionKey before verification,
so concurrent unlock() calls or failing verification can leave a stale/incorrect
key; fix by keeping a local candidateKey (from deriveEncryptionKey()) and do not
assign to this.encryptionKey until verification/bootstrap succeeds, pass
candidateKey into createVerificationPayload(), verifyPassword(), encryptData(),
decryptData(), and deriveAesKey() so they use the local key during async flows,
and serialize unlock() with a mutex or single in-flight promise to prevent
concurrent unlocks from racing.
---
Nitpick comments:
In `@packages/core-sdk/src/storage/types.ts`:
- Around line 11-12: This change tightens public SDK types (StorageAdapter,
AccountData, SessionKeysData) from any to unknown which is a compile-time
breaking change for downstream TypeScript consumers and adapter implementations;
update the release plan/changelog and semver policy to explicitly call out that
these exported types in packages/core-sdk (e.g., StorageAdapter, AccountData,
SessionKeysData defined in src/storage/types.ts) were narrowed to unknown and
therefore require adapter updates or type adjustments, and indicate whether this
will be released as a major/minor bump and provide migration guidance for
implementers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24574e41-7a51-435b-a44c-7184472f23d6
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
.gitignorepackages/core-sdk/eslint.config.cjspackages/core-sdk/src/storage/__tests__/manager.test.tspackages/core-sdk/src/storage/__tests__/unlock.test.tspackages/core-sdk/src/storage/secure-storage-manager.tspackages/core-sdk/src/storage/types.tspackages/core-sdk/tsconfig.json
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- packages/core-sdk/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core-sdk/src/storage/tests/manager.test.ts
- packages/core-sdk/eslint.config.cjs
- packages/core-sdk/src/storage/tests/unlock.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/core-sdk/src/storage/__tests__/unlock.test.ts (1)
467-481:⚠️ Potential issue | 🟡 MinorTest should verify
isUnlockedstate when verification payload is missing.This test verifies that
verifyPassword()returnsfalsewhen the payload doesn't exist, but doesn't verify that the encryption key is cleared. Once the implementation bug inverifyPassword()is fixed (to clearencryptionKeywhen payload is missing), add an assertion to verify the locked state.💚 Proposed test enhancement after implementation fix
it('should return false when verification payload does not exist', async () => { // Set up encryption key directly without calling unlock (which would create the verification payload) const password = 'test-password'; const masterSalt = new Uint8Array(16); globalThis.crypto.getRandomValues(masterSalt); const deriveEncryptionKey = (manager as any).deriveEncryptionKey.bind(manager); (manager as any).encryptionKey = await deriveEncryptionKey(password, masterSalt); // Verify password without verification payload const verifyPassword = (manager as any).verifyPassword.bind(manager); const result = await verifyPassword(); expect(result).toBe(false); + // Key should be cleared when verification payload is missing + expect(manager.isUnlocked).toBe(false); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core-sdk/src/storage/__tests__/unlock.test.ts` around lines 467 - 481, The test currently checks that verifyPassword() returns false when the verification payload is missing but doesn't assert that the manager's unlocked state is cleared; update the test to assert that (manager as any).encryptionKey is cleared (null/undefined) and that manager.isUnlocked() (or the public isUnlocked property/method on the manager) returns false after calling verifyPassword(), and ensure this aligns with the fix in verifyPassword() which must clear encryptionKey when payload is missing.packages/core-sdk/src/storage/secure-storage-manager.ts (1)
171-185:⚠️ Potential issue | 🟠 MajorInconsistent state when
verification_payloadis missing.When
payloadisnull(line 173-174), the method returnsfalsebut does not clearencryptionKey. This creates an inconsistent state whereunlock()returnsfalsewhileisUnlockedremainstrue. The catch block correctly clearsencryptionKeyon decryption failure, but this early return does not.This edge case can occur if
master_saltexists butverification_payloadwas deleted or corrupted.🔒 Proposed fix to clear key when payload is missing
private async verifyPassword(): Promise<boolean> { const payload = await this.storage.get('verification_payload') as EncryptedPayload | null; if (!payload) { + this.encryptionKey = null; return false; } try { await this.decryptData(payload); return true; } catch { // Decryption failed - wrong password this.encryptionKey = null; return false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core-sdk/src/storage/secure-storage-manager.ts` around lines 171 - 185, In verifyPassword: the early return when the 'verification_payload' is missing leaves this.encryptionKey set, causing isUnlocked to be inconsistent; update the branch that handles payload === null to clear this.encryptionKey (set this.encryptionKey = null) before returning false so the manager's unlocked state matches the failure, keeping the existing catch behavior intact.
🧹 Nitpick comments (4)
packages/core-sdk/src/storage/secure-storage-manager.ts (2)
50-61: Inconsistent indentation in the first-run code path.The code block inside the
if (!masterSalt)branch has inconsistent indentation. Lines 51-60 should be indented relative to theifstatement, andreturn trueon line 61 should align with the rest of the block body.🧹 Proposed fix to correct indentation
if (!masterSalt) { - // First run: generate salt in memory only - masterSalt = this.initializeMasterSalt(); - - // Derive encryption key from password and master salt - this.encryptionKey = await this.deriveEncryptionKey(password, masterSalt); - - // Store verification payload first, then persist salt last (atomic ordering) - await this.createVerificationPayload(); - await this.storage.set('master_salt', bufferToBase64(masterSalt)); - - return true; + // First run: generate salt in memory only + masterSalt = this.initializeMasterSalt(); + + // Derive encryption key from password and master salt + this.encryptionKey = await this.deriveEncryptionKey(password, masterSalt); + + // Store verification payload first, then persist salt last (atomic ordering) + await this.createVerificationPayload(); + await this.storage.set('master_salt', bufferToBase64(masterSalt)); + + return true; } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core-sdk/src/storage/secure-storage-manager.ts` around lines 50 - 61, Fix the inconsistent indentation inside the first-run branch checking `if (!masterSalt)`: indent the entire block that calls `this.initializeMasterSalt()`, `this.deriveEncryptionKey(password, masterSalt)`, `this.createVerificationPayload()`, and `this.storage.set('master_salt', bufferToBase64(masterSalt))` so all statements are aligned as the body of the `if`, and ensure the `return true` statement is inside that same indented block (aligned with the other statements) so the control flow is correct and visually consistent.
84-90: Outdated docstring forinitializeMasterSalt.The docstring states "Generates a random 16-byte master salt and stores it in the storage adapter" but the method now only generates and returns the salt in memory. Storage is handled by
unlock().📝 Proposed fix for docstring
/** - * Generates a random 16-byte master salt and stores it in the storage adapter. - * `@returns` The generated master salt as a Uint8Array + * Generates a random 16-byte master salt in memory. + * Note: The caller is responsible for persisting the salt to storage. + * `@returns` The generated master salt as a Uint8Array */ private initializeMasterSalt(): Uint8Array {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core-sdk/src/storage/secure-storage-manager.ts` around lines 84 - 90, Update the docstring for initializeMasterSalt to accurately describe its behavior: state that it generates and returns a random 16-byte master salt as a Uint8Array but does not persist it (storage is handled by unlock()), remove the phrase "and stores it in the storage adapter", and ensure the return description matches the Uint8Array result.packages/core-sdk/src/storage/__tests__/unlock.test.ts (2)
1295-1332: Test name is misleading - it doesn't verify decryption of old data.The test is named "should decrypt data encrypted with old unlock implementation" but:
- It encrypts data with simulated old key derivation
- Creates a new manager and calls
unlock()which initializes newmaster_salt- Only verifies
master_saltandverification_payloadare createdThe inline comments acknowledge this: "Note: This will fail because new implementation requires master_salt". This is actually testing first-run initialization, not backward compatibility.
Consider renaming for clarity, or add a comment to the
describeblock noting this is a breaking change requiring migration.📝 Suggested rename for clarity
- it('should decrypt data encrypted with old unlock implementation', async () => { + it('should initialize new master_salt when unlocking after legacy encryption (note: old data is not readable)', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core-sdk/src/storage/__tests__/unlock.test.ts` around lines 1295 - 1332, Rename or update the test that currently uses manager.saveAccount, manager.lock, new SecureStorageManager(storage) and newManager.unlock(password) so its name and assertions reflect that it is testing first-run initialization (creation of master_salt and verification_payload) rather than decryption of legacy data; specifically either rename the test string from "should decrypt data encrypted with old unlock implementation" to something like "should initialize master_salt and verification_payload on first run when legacy key present" or change the assertions to actually attempt decryption of the payload saved by the simulated old implementation (derive the correct decryption key from (manager as any).encryptionKey and verify saved account data), and update comments to clarify whether this is a breaking change requiring migration.
1334-1364: Test doesn't verify old encrypted data is readable after migration.This test saves data with simulated old key derivation, then migrates and saves NEW data. It only verifies the new data can be retrieved. The old encrypted data (
{ privateKey: '0xOLD_KEY' }) is overwritten and never tested for readability.Since the key derivation changed, old data would NOT be decryptable - this is expected but should be documented clearly in the test.
📝 Suggested clarification
it('should handle existing encrypted data after migration', async () => { const password = 'test-password'; const oldAccountData = { privateKey: '0xOLD_KEY' }; const newAccountData = { privateKey: '0xNEW_KEY' }; // Simulate old implementation const encoder = new TextEncoder(); const oldBaseKey = await globalThis.crypto.subtle.importKey( 'raw', encoder.encode(password), { name: 'PBKDF2' }, false, ['deriveKey'] ); (manager as any).encryptionKey = oldBaseKey; await manager.saveAccount(oldAccountData); // Lock and use new implementation manager.lock(); const newManager = new SecureStorageManager(storage); - // First unlock with new implementation (creates master salt) + // First unlock with new implementation (creates master salt) + // Note: Old encrypted data is NOT readable with new key derivation await newManager.unlock(password); - // Save new data with new implementation + // Save new data (overwrites old encrypted data) await newManager.saveAccount(newAccountData); - // Verify we can retrieve the new data + // Verify we can retrieve the newly saved data (not the old data) const retrieved = await newManager.getAccount(); expect(retrieved).toEqual(newAccountData); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core-sdk/src/storage/__tests__/unlock.test.ts` around lines 1334 - 1364, The test currently overwrites old encrypted data and only asserts the new data is readable; update the test to explicitly check old-data behavior after migration: after creating old encrypted data with manager.saveAccount and then creating newManager and calling newManager.unlock(password), attempt to retrieve the old data (e.g., call newManager.getAccount or a dedicated read method before saving newAccountData) and assert the expected behavior—either that the old data is not decryptable (expect null/throws) or, if intended, that it is migrated and equals oldAccountData; include a short comment clarifying which behavior is expected and why (key derivation change vs. migration), and reference manager.saveAccount, manager.lock, SecureStorageManager constructor, newManager.unlock, and newManager.getAccount so you modify the correct steps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core-sdk/src/storage/__tests__/unlock.test.ts`:
- Around line 126-133: The test in unlock.test.ts conflicts with
loadMasterSalt(): it expects null for an empty string but loadMasterSalt()
currently decodes the empty string to a 0-byte buffer and throws "Corrupted
master_salt: expected 16 bytes"; fix by choosing one of two options: (A) Update
the test to expect an error from (manager as any).loadMasterSalt() when storage
contains '' (i.e., assert that it throws the existing error), or (B) change the
implementation of loadMasterSalt() to treat an empty string as uninitialized by
adding an early check (e.g., treat base64Salt === '' as null / return null
before calling base64ToBuffer) so the test remains valid; reference the
loadMasterSalt function and base64ToBuffer usage to locate the fix.
---
Duplicate comments:
In `@packages/core-sdk/src/storage/__tests__/unlock.test.ts`:
- Around line 467-481: The test currently checks that verifyPassword() returns
false when the verification payload is missing but doesn't assert that the
manager's unlocked state is cleared; update the test to assert that (manager as
any).encryptionKey is cleared (null/undefined) and that manager.isUnlocked() (or
the public isUnlocked property/method on the manager) returns false after
calling verifyPassword(), and ensure this aligns with the fix in
verifyPassword() which must clear encryptionKey when payload is missing.
In `@packages/core-sdk/src/storage/secure-storage-manager.ts`:
- Around line 171-185: In verifyPassword: the early return when the
'verification_payload' is missing leaves this.encryptionKey set, causing
isUnlocked to be inconsistent; update the branch that handles payload === null
to clear this.encryptionKey (set this.encryptionKey = null) before returning
false so the manager's unlocked state matches the failure, keeping the existing
catch behavior intact.
---
Nitpick comments:
In `@packages/core-sdk/src/storage/__tests__/unlock.test.ts`:
- Around line 1295-1332: Rename or update the test that currently uses
manager.saveAccount, manager.lock, new SecureStorageManager(storage) and
newManager.unlock(password) so its name and assertions reflect that it is
testing first-run initialization (creation of master_salt and
verification_payload) rather than decryption of legacy data; specifically either
rename the test string from "should decrypt data encrypted with old unlock
implementation" to something like "should initialize master_salt and
verification_payload on first run when legacy key present" or change the
assertions to actually attempt decryption of the payload saved by the simulated
old implementation (derive the correct decryption key from (manager as
any).encryptionKey and verify saved account data), and update comments to
clarify whether this is a breaking change requiring migration.
- Around line 1334-1364: The test currently overwrites old encrypted data and
only asserts the new data is readable; update the test to explicitly check
old-data behavior after migration: after creating old encrypted data with
manager.saveAccount and then creating newManager and calling
newManager.unlock(password), attempt to retrieve the old data (e.g., call
newManager.getAccount or a dedicated read method before saving newAccountData)
and assert the expected behavior—either that the old data is not decryptable
(expect null/throws) or, if intended, that it is migrated and equals
oldAccountData; include a short comment clarifying which behavior is expected
and why (key derivation change vs. migration), and reference
manager.saveAccount, manager.lock, SecureStorageManager constructor,
newManager.unlock, and newManager.getAccount so you modify the correct steps.
In `@packages/core-sdk/src/storage/secure-storage-manager.ts`:
- Around line 50-61: Fix the inconsistent indentation inside the first-run
branch checking `if (!masterSalt)`: indent the entire block that calls
`this.initializeMasterSalt()`, `this.deriveEncryptionKey(password, masterSalt)`,
`this.createVerificationPayload()`, and `this.storage.set('master_salt',
bufferToBase64(masterSalt))` so all statements are aligned as the body of the
`if`, and ensure the `return true` statement is inside that same indented block
(aligned with the other statements) so the control flow is correct and visually
consistent.
- Around line 84-90: Update the docstring for initializeMasterSalt to accurately
describe its behavior: state that it generates and returns a random 16-byte
master salt as a Uint8Array but does not persist it (storage is handled by
unlock()), remove the phrase "and stores it in the storage adapter", and ensure
the return description matches the Uint8Array result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0acd4cc-508d-473e-8852-93cb41d811a6
📒 Files selected for processing (2)
packages/core-sdk/src/storage/__tests__/unlock.test.tspackages/core-sdk/src/storage/secure-storage-manager.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/core-sdk/src/storage/secure-storage-manager.ts (1)
171-183:⚠️ Potential issue | 🟠 MajorRe-lock on every failed verification path.
verifyPassword()only clearsthis.encryptionKeyinside the decrypt catch. The missing-payload branch on Line 173, and any rejection before Line 177, leave the stale derived key live even thoughunlock()failed. A retry then hits the early return on Line 43.🔒 Minimal fix
private async verifyPassword(): Promise<boolean> { - const payload = await this.storage.get('verification_payload') as EncryptedPayload | null; - if (!payload) { - return false; - } - try { + const payload = await this.storage.get('verification_payload') as EncryptedPayload | null; + if (!payload) { + this.encryptionKey = null; + return false; + } await this.decryptData(payload); return true; } catch { // Decryption failed - wrong password this.encryptionKey = null; return false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core-sdk/src/storage/secure-storage-manager.ts` around lines 171 - 183, verifyPassword currently only clears this.encryptionKey inside the decryptData catch, leaving a stale key when storage.get returns null or if storage.get throws; update verifyPassword so any failure path clears this.encryptionKey and returns false: wrap the payload retrieval and decryptData call in a try/catch, set this.encryptionKey = null before returning false when payload is missing, and ensure the catch handler clears this.encryptionKey and returns false; reference verifyPassword, decryptData and the unlock behavior that relies on encryptionKey.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core-sdk/src/storage/secure-storage-manager.ts`:
- Around line 50-61: The unlock() first-run path sets this.encryptionKey after
calling initializeMasterSalt() and deriveEncryptionKey() but before persisting
verification data and master salt, so if createVerificationPayload() or
storage.set('master_salt', ...) throws the in-memory encryptionKey remains set
and future unlock() calls will short-circuit; to fix, defer assigning
this.encryptionKey (and/or masterSalt) until after createVerificationPayload()
and storage.set(...) succeed, or wrap the sequence in a try/catch that clears
this.encryptionKey and masterSalt on any failure (referencing unlock,
initializeMasterSalt, deriveEncryptionKey, createVerificationPayload,
storage.set, this.encryptionKey, masterSalt).
- Around line 204-208: Remove the duplicate declarations of salt and iv: there
are two consecutive calls creating salt and iv via
globalThis.crypto.getRandomValues (one pair without casting and one pair with
"as any"); delete the redundant lines that re-declare const salt and const iv
(the ones using "as any") so only a single const salt and const iv remain in the
scope where they're used (e.g., in the secure storage initialization function),
ensuring no duplicate declarations/syntax error.
---
Duplicate comments:
In `@packages/core-sdk/src/storage/secure-storage-manager.ts`:
- Around line 171-183: verifyPassword currently only clears this.encryptionKey
inside the decryptData catch, leaving a stale key when storage.get returns null
or if storage.get throws; update verifyPassword so any failure path clears
this.encryptionKey and returns false: wrap the payload retrieval and decryptData
call in a try/catch, set this.encryptionKey = null before returning false when
payload is missing, and ensure the catch handler clears this.encryptionKey and
returns false; reference verifyPassword, decryptData and the unlock behavior
that relies on encryptionKey.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee9b343e-7051-4b8b-8266-f8e411f7e7e1
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
packages/core-sdk/src/storage/__tests__/manager.test.tspackages/core-sdk/src/storage/secure-storage-manager.ts
Comment on lines
+50
to
+61
| if (!masterSalt) { | ||
| // First run: generate salt in memory only | ||
| masterSalt = this.initializeMasterSalt(); | ||
|
|
||
| // Derive encryption key from password and master salt | ||
| this.encryptionKey = await this.deriveEncryptionKey(password, masterSalt); | ||
|
|
||
| // Store verification payload first, then persist salt last (atomic ordering) | ||
| await this.createVerificationPayload(); | ||
| await this.storage.set('master_salt', bufferToBase64(masterSalt)); | ||
|
|
||
| return true; |
There was a problem hiding this comment.
Clear or defer encryptionKey if first-run setup fails.
If createVerificationPayload() or storage.set('master_salt') rejects, unlock() fails after Line 55 has already published this.encryptionKey. The next call will short-circuit on Line 43 and can save data with a salt that was never persisted.
🧱 Safer failure handling
masterSalt = this.initializeMasterSalt();
// Derive encryption key from password and master salt
this.encryptionKey = await this.deriveEncryptionKey(password, masterSalt);
// Store verification payload first, then persist salt last (atomic ordering)
- await this.createVerificationPayload();
- await this.storage.set('master_salt', bufferToBase64(masterSalt));
-
- return true;
+ try {
+ await this.createVerificationPayload();
+ await this.storage.set('master_salt', bufferToBase64(masterSalt));
+ return true;
+ } catch (error) {
+ this.encryptionKey = null;
+ await this.storage.remove('verification_payload');
+ throw error;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core-sdk/src/storage/secure-storage-manager.ts` around lines 50 -
61, The unlock() first-run path sets this.encryptionKey after calling
initializeMasterSalt() and deriveEncryptionKey() but before persisting
verification data and master salt, so if createVerificationPayload() or
storage.set('master_salt', ...) throws the in-memory encryptionKey remains set
and future unlock() calls will short-circuit; to fix, defer assigning
this.encryptionKey (and/or masterSalt) until after createVerificationPayload()
and storage.set(...) succeed, or wrap the sequence in a try/catch that clears
this.encryptionKey and masterSalt on any failure (referencing unlock,
initializeMasterSalt, deriveEncryptionKey, createVerificationPayload,
storage.set, this.encryptionKey, masterSalt).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
feat: Implement Secure Storage Unlock (#116)
Implements SecureStorageManager.unlock(password) returning Promise.
Generates and persists a master salt on first run, loads it on subsequent runs
Derives AES-256-GCM encryption key via PBKDF2 (100k iterations, SHA-256)
Verifies password via a lightweight decrypt of a stored verification payload
Returns true for correct password, false for wrong — no plaintext exposed
Encryption key kept in memory only, cleared immediately on failed verification
108 tests passing across 3 test suites
Closes #116
Summary by CodeRabbit
New Features
Bug Fixes