feat: implement web-compatible credential storage with AES-GCM encryption#141
Conversation
- Added `isNativePlatform` check to `KeyStorage`. - Implemented `localStorage` fallback for `KeyStorage` in non-native environments. - Updated `CredentialSettings` page to use `CredentialService` for saving keys instead of throwing error. - Updated `KeyStorage` tests to cover web scenarios (with window mocking). Co-authored-by: jbdevprimary <2650679+jbdevprimary@users.noreply.github.com>
- Fixed lint error in `CredentialSettings.tsx` by removing `status` from `addCredential`. - Implemented `WebEncryption` using AES-GCM in `KeyStorage.ts` to encrypt sensitive data before storing in `localStorage` on web, addressing CodeQL security alerts. - Updated `KeyStorage.test.ts` to verify web encryption logic using mocked `crypto.subtle`. - Fixed `auth-flow.integration.test.ts` failures by explicitly mocking `window.Capacitor.isNativePlatform` to true for integration tests, ensuring they use the intended mock secure storage logic. Co-authored-by: jbdevprimary <2650679+jbdevprimary@users.noreply.github.com>
- Fixed lint error in `CredentialSettings.tsx` by removing `status` from `addCredential`. - Implemented `WebEncryption` using AES-GCM in `KeyStorage.ts` to encrypt sensitive data before storing in `localStorage` on web, addressing CodeQL security alerts. - Updated `KeyStorage.test.ts` to verify web encryption logic using mocked `crypto.subtle`. - Fixed `auth-flow.integration.test.ts` failures by explicitly mocking `window.Capacitor.isNativePlatform` to true for integration tests, ensuring they use the intended mock secure storage logic. - Fixed `CredentialServicePerf.test.ts` by mocking `window.Capacitor.isNativePlatform` to true, ensuring performance tests use the mocked secure storage instead of failing web fallback. Co-authored-by: jbdevprimary <2650679+jbdevprimary@users.noreply.github.com>
…ncryption
- Switched web storage from `localStorage` to `sessionStorage` to limit persistence to the browser session.
- Implemented `WebEncryption` using AES-GCM (Web Crypto API) to encrypt credentials before storing in `sessionStorage`.
- Updated `authenticateWithBiometrics` to return failure on web, preventing biometric bypass.
- Added error handling for storage operations (e.g. quota exceeded).
- Updated `CredentialSettings` UI to display platform-aware security text ("Encrypted Session Storage" vs "Secure Storage").
- Updated `KeyStorage` tests to verify `sessionStorage` usage, encryption, and biometric failure on web.
Co-authored-by: jbdevprimary <2650679+jbdevprimary@users.noreply.github.com>
- Remove `as any` cast: use `CredentialMetadata['metadata']` for proper
type safety instead of unsafe any cast (Amazon Q + Gemini)
- Fix credential status: call setCredentialStatus(credId, 'valid') after
addCredential since addCredential defaults to 'unknown' status (Gemini)
- Import CredentialMetadata type from @thumbcode/state
Note: Jules already addressed the critical security feedback:
- Web Crypto API (AES-GCM) encryption instead of cleartext localStorage
- sessionStorage instead of localStorage (cleared on tab close)
- Biometric auth returns { success: false } on web (not silent bypass)
- Platform-aware UI text (native: "hardware-backed", web: "encrypted session")
- Error handling for quota exceeded and private browsing mode
- Null-safe Capacitor platform check
https://claude.ai/code/session_01PQd4hGQQpmGTgpHc7kGjAE
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis PR introduces web-compatible credential storage as a fallback mechanism. The implementation detects the platform environment and switches between Capacitor's Secure Storage plugin for native platforms and WebEncryption with sessionStorage for web environments. Biometric operations are restricted to native platforms only. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as CredentialSettings
participant KS as KeyStorage
participant CP as Capacitor<br/>(Native)
participant WE as WebEncryption<br/>(Web)
participant SS as sessionStorage<br/>(Web)
User->>UI: Save credential
UI->>KS: store(credential)
alt Native Platform
KS->>CP: Capacitor.Secure StoragePlugin
CP-->>KS: Success
else Web Platform
KS->>WE: encrypt(data, AES-GCM)
WE-->>KS: encryptedData
KS->>SS: sessionStorage.setItem()
SS-->>KS: Stored
end
KS-->>UI: Storage complete
UI->>UI: Clear input, show success
UI-->>User: Confirmation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ Finishing touches
🧪 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 |
Summary of ChangesHello @jbdevprimary, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's credential management by introducing a dual-platform secure storage solution. It ensures that sensitive API keys are encrypted and stored appropriately, whether on native devices using hardware-backed security or in web browsers via encrypted session storage. The changes also refine biometric authentication behavior, improve type safety, and provide a more accurate user experience regarding security information, making the system more robust and secure across different environments. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR implements web-compatible credential storage with AES-GCM encryption and addresses security feedback from PR #138. The implementation successfully adds encryption for web environments and maintains native platform security.
Critical Issues Found: 2
- Security vulnerability: Encryption key stored alongside encrypted data in sessionStorage provides minimal security benefit
- Crash risk: Missing error handling for JSON parsing in
getKey()method
Recommendations
The encryption key storage pattern (storing key and encrypted data in the same location) creates a false sense of security. Consider either:
- Removing web encryption entirely with clear user warnings about session-only storage
- Implementing proper key derivation from user input (password/PIN)
- Adding explicit documentation that web storage is for convenience, not security
The test coverage is comprehensive and properly validates both native and web paths. Performance tests confirm parallel validation is working correctly.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| class WebEncryption { | ||
| private static readonly KEY_STORAGE_KEY = 'thumbcode_web_ek'; | ||
| private static readonly ALGORITHM = 'AES-GCM'; | ||
| private static readonly KEY_LENGTH = 256; | ||
|
|
||
| private static async getKey(): Promise<CryptoKey> { | ||
| const storedKey = sessionStorage.getItem(this.KEY_STORAGE_KEY); | ||
|
|
||
| if (storedKey) { | ||
| // Import existing key | ||
| const keyData = JSON.parse(storedKey); | ||
| return crypto.subtle.importKey( | ||
| 'jwk', | ||
| keyData, | ||
| { name: this.ALGORITHM, length: this.KEY_LENGTH }, | ||
| true, | ||
| ['encrypt', 'decrypt'] | ||
| ); | ||
| } | ||
|
|
||
| // Generate new key | ||
| const key = await crypto.subtle.generateKey( | ||
| { name: this.ALGORITHM, length: this.KEY_LENGTH }, | ||
| true, | ||
| ['encrypt', 'decrypt'] | ||
| ); | ||
|
|
||
| // Export and store key | ||
| const exportedKey = await crypto.subtle.exportKey('jwk', key); | ||
| sessionStorage.setItem(this.KEY_STORAGE_KEY, JSON.stringify(exportedKey)); | ||
|
|
||
| return key; | ||
| } |
There was a problem hiding this comment.
🛑 Security Vulnerability: The encryption key is stored in sessionStorage alongside encrypted credentials, providing no meaningful security.
An attacker with access to sessionStorage can retrieve both the encryption key (stored at thumbcode_web_ek) and the encrypted credentials, then decrypt them. This defeats the purpose of encryption. The comment on line 55-56 acknowledges this but the implementation still creates a false sense of security.
For web environments, consider either removing encryption entirely (with clear warnings to users) or implementing a key derivation approach where the key is derived from user input (like a password) that isn't stored.
| // Biometric check if required | ||
| // On web, if requireBiometric is true, this will fail because authenticateWithBiometrics returns false | ||
| if (requireBiometric) { | ||
| const biometricResult = await this.authenticateWithBiometrics(); | ||
| if (!biometricResult.success) { | ||
| return { isValid: false, message: 'Biometric authentication failed' }; | ||
| return { | ||
| isValid: false, | ||
| message: biometricResult.error || 'Biometric authentication failed' | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛑 Logic Error: Missing error message propagation causes loss of specific biometric failure reasons.
When biometric authentication fails, the error message from line 207 (biometricResult.error) is lost. Users will only see a generic "Biometric authentication failed" message instead of specific failures like "user_cancel", "biometry_not_enrolled", or "permission_denied". This makes debugging authentication issues difficult.
// Biometric check if required
// On web, if requireBiometric is true, this will fail because authenticateWithBiometrics returns false
if (requireBiometric) {
const biometricResult = await this.authenticateWithBiometrics();
if (!biometricResult.success) {
return {
isValid: false,
message: biometricResult.error || 'Biometric authentication failed'
};
}
}| private static async getKey(): Promise<CryptoKey> { | ||
| const storedKey = sessionStorage.getItem(this.KEY_STORAGE_KEY); | ||
|
|
||
| if (storedKey) { | ||
| // Import existing key | ||
| const keyData = JSON.parse(storedKey); | ||
| return crypto.subtle.importKey( | ||
| 'jwk', | ||
| keyData, | ||
| { name: this.ALGORITHM, length: this.KEY_LENGTH }, | ||
| true, | ||
| ['encrypt', 'decrypt'] | ||
| ); | ||
| } | ||
|
|
||
| // Generate new key | ||
| const key = await crypto.subtle.generateKey( | ||
| { name: this.ALGORITHM, length: this.KEY_LENGTH }, | ||
| true, | ||
| ['encrypt', 'decrypt'] | ||
| ); | ||
|
|
||
| // Export and store key | ||
| const exportedKey = await crypto.subtle.exportKey('jwk', key); | ||
| sessionStorage.setItem(this.KEY_STORAGE_KEY, JSON.stringify(exportedKey)); | ||
|
|
||
| return key; | ||
| } |
There was a problem hiding this comment.
🛑 Security Vulnerability: Missing error handling for JSON parsing allows potential crashes from corrupted sessionStorage data.
If sessionStorage data is corrupted or tampered with (lines 68, 115), JSON.parse() will throw an exception that isn't caught, causing the encryption/decryption to fail silently or crash. This could lead to credential retrieval failures that are difficult to debug. The decrypt method has try-catch (line 114), but getKey method (line 68) doesn't, creating an inconsistent error handling pattern.
| private static async getKey(): Promise<CryptoKey> { | |
| const storedKey = sessionStorage.getItem(this.KEY_STORAGE_KEY); | |
| if (storedKey) { | |
| // Import existing key | |
| const keyData = JSON.parse(storedKey); | |
| return crypto.subtle.importKey( | |
| 'jwk', | |
| keyData, | |
| { name: this.ALGORITHM, length: this.KEY_LENGTH }, | |
| true, | |
| ['encrypt', 'decrypt'] | |
| ); | |
| } | |
| // Generate new key | |
| const key = await crypto.subtle.generateKey( | |
| { name: this.ALGORITHM, length: this.KEY_LENGTH }, | |
| true, | |
| ['encrypt', 'decrypt'] | |
| ); | |
| // Export and store key | |
| const exportedKey = await crypto.subtle.exportKey('jwk', key); | |
| sessionStorage.setItem(this.KEY_STORAGE_KEY, JSON.stringify(exportedKey)); | |
| return key; | |
| } | |
| private static async getKey(): Promise<CryptoKey> { | |
| const storedKey = sessionStorage.getItem(this.KEY_STORAGE_KEY); | |
| if (storedKey) { | |
| try { | |
| // Import existing key | |
| const keyData = JSON.parse(storedKey); | |
| return crypto.subtle.importKey( | |
| 'jwk', | |
| keyData, | |
| { name: this.ALGORITHM, length: this.KEY_LENGTH }, | |
| true, | |
| ['encrypt', 'decrypt'] | |
| ); | |
| } catch (error) { | |
| console.error('Failed to parse stored encryption key, generating new key:', error); | |
| // Remove corrupted key and fall through to generate new one | |
| sessionStorage.removeItem(this.KEY_STORAGE_KEY); | |
| } | |
| } | |
| // Generate new key | |
| const key = await crypto.subtle.generateKey( | |
| { name: this.ALGORITHM, length: this.KEY_LENGTH }, | |
| true, | |
| ['encrypt', 'decrypt'] | |
| ); | |
| // Export and store key | |
| const exportedKey = await crypto.subtle.exportKey('jwk', key); | |
| sessionStorage.setItem(this.KEY_STORAGE_KEY, JSON.stringify(exportedKey)); | |
| return key; | |
| } |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request does a great job of implementing a web-compatible credential storage mechanism using the Web Crypto API with AES-GCM, providing a secure fallback for when the native Capacitor Secure Storage is not available. The platform detection logic is robust, and the changes correctly handle UI and behavior differences between native and web environments, such as for biometric authentication. The test suites have been thoughtfully updated to cover both native and web execution paths, ensuring the new logic is well-tested.
I have a couple of suggestions in packages/core/src/credentials/KeyStorage.ts to improve the storage efficiency of the web encryption implementation by using Base64 encoding for binary data, which is a more standard and compact approach.
Overall, this is a solid implementation that significantly improves the application's capabilities on the web platform while maintaining security best practices.
| const encryptedArray = Array.from(new Uint8Array(encryptedContent)); | ||
| const ivArray = Array.from(iv); | ||
|
|
||
| return JSON.stringify({ | ||
| iv: ivArray, | ||
| data: encryptedArray, | ||
| }); |
There was a problem hiding this comment.
For more compact storage, consider encoding the iv and encryptedContent using Base64 instead of storing them as a JSON array of numbers. This can significantly reduce the size of the stored string in sessionStorage, especially for larger secrets. This is a more standard way to serialize binary data for JSON transport.
I've added a corresponding suggestion for the decrypt method.
Example of Base64 encoding:
const ivBase64 = btoa(String.fromCharCode(...iv));
const encryptedDataBase64 = btoa(String.fromCharCode(...new Uint8Array(encryptedContent)));
return JSON.stringify({
iv: ivBase64,
data: encryptedDataBase64,
});| const encryptedArray = Array.from(new Uint8Array(encryptedContent)); | |
| const ivArray = Array.from(iv); | |
| return JSON.stringify({ | |
| iv: ivArray, | |
| data: encryptedArray, | |
| }); | |
| const ivBase64 = btoa(String.fromCharCode(...iv)); | |
| const encryptedDataBase64 = btoa(String.fromCharCode(...new Uint8Array(encryptedContent))); | |
| return JSON.stringify({ | |
| iv: ivBase64, | |
| data: encryptedDataBase64, | |
| }); |
| const { iv, data } = JSON.parse(encryptedString); | ||
| const key = await this.getKey(); | ||
|
|
||
| const decryptedContent = await crypto.subtle.decrypt( | ||
| { name: this.ALGORITHM, iv: new Uint8Array(iv) }, | ||
| key, | ||
| new Uint8Array(data) | ||
| ); |
There was a problem hiding this comment.
To correspond with the Base64 encoding suggested for the encrypt method, you should decode the Base64 strings back into Uint8Arrays here before decryption.
Example of Base64 decoding:
const iv = new Uint8Array(atob(ivBase64).split('').map(c => c.charCodeAt(0)));
const data = new Uint8Array(atob(dataBase64).split('').map(c => c.charCodeAt(0)));| const { iv, data } = JSON.parse(encryptedString); | |
| const key = await this.getKey(); | |
| const decryptedContent = await crypto.subtle.decrypt( | |
| { name: this.ALGORITHM, iv: new Uint8Array(iv) }, | |
| key, | |
| new Uint8Array(data) | |
| ); | |
| const { iv: ivBase64, data: dataBase64 } = JSON.parse(encryptedString); | |
| const key = await this.getKey(); | |
| const iv = new Uint8Array(atob(ivBase64).split('').map(c => c.charCodeAt(0))); | |
| const data = new Uint8Array(atob(dataBase64).split('').map(c => c.charCodeAt(0))); | |
| const decryptedContent = await crypto.subtle.decrypt( | |
| { name: this.ALGORITHM, iv }, | |
| key, | |
| data | |
| ); |
Acknowledged that PR #141 addresses the feedback and supersedes this work. Stopping further development on this branch. Co-authored-by: jbdevprimary <2650679+jbdevprimary@users.noreply.github.com>
2 participants
Summary
Replaces #138 with all review feedback addressed.
Implements web-compatible credential storage using AES-GCM encryption via Web Crypto API, with Capacitor SecureStorage for native platforms. Includes biometric authentication support.
Review Feedback Addressed (from #138)
Security (Critical - All Fixed):
localStoragewith AES-GCM encryption via Web Crypto API (crypto.subtle). Keys are encrypted before storage.authenticateWithBiometrics()now returns{ success: false, error: '...' }on web instead of silently succeeding.sessionStoragewhich clears on tab close, reducing exposure window.Type Safety (Fixed):
as anycast removed (Amazon Q + Gemini): Changedmetadata: result.metadata as anytometadata: result.metadata as CredentialMetadata['metadata']setCredentialStatus(credId, 'valid')call sinceaddCredentialdefaults status to'unknown'UX (Fixed):
Error Handling (Fixed):
window.Capacitor?.isNativePlatform() ?? falseChanges
packages/core/src/credentials/KeyStorage.ts- AES-GCM WebEncryption class, sessionStorage, proper biometric returnssrc/pages/settings/CredentialSettings.tsx- Type-safe metadata, credential status fix, platform-aware UIpackages/core/src/credentials/__tests__/KeyStorage.test.ts- Full test coverage for web and native pathssrc/services/__tests__/auth-flow.integration.test.ts- Integration test for credential flowpackages/core/src/__tests__/CredentialServicePerf.test.ts- Performance benchmarksTest plan
Closes #138
https://claude.ai/code/session_01PQd4hGQQpmGTgpHc7kGjAE
Summary by CodeRabbit