-
Notifications
You must be signed in to change notification settings - Fork 18
[SBA-05] Introduce new password encryption abstractions #137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR establishes foundational abstractions for password-based encryption and credential storage as the first step toward supporting session-based authentication. It introduces a clean separation between encryption logic (EncryptionService/PasswordEncryptionService) and credential persistence (CredentialRepository/BaseCredentialRepository/PasswordCredentialRepository), laying the groundwork for future passkey and session-based authentication implementations.
- Adds
EncryptionServiceinterface andPasswordEncryptionServiceimplementation for password-based encryption operations - Adds
CredentialRepositoryinterface withBaseCredentialRepositorybase class andPasswordCredentialRepositoryconcrete implementation for localStorage-backed encrypted credential storage - Introduces new types (
UnlockMethod,UnlockOptions) to support different authentication methods
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/types/lnc.ts | Adds UnlockMethod type and UnlockOptions type for authentication abstractions |
| lib/encryption/encryptionService.ts | Defines the EncryptionService interface for encryption/decryption operations without storage dependencies |
| lib/encryption/passwordEncryptionService.ts | Implements EncryptionService using password-based encryption with salt and test cipher verification |
| lib/encryption/passwordEncryptionService.test.ts | Comprehensive unit tests for PasswordEncryptionService covering unlock, encrypt/decrypt, and error scenarios |
| lib/repositories/credentialRepository.ts | Defines CredentialRepository interface and BaseCredentialRepository base class with localStorage operations |
| lib/repositories/credentialRepository.test.ts | Unit tests for BaseCredentialRepository functionality including persistence and namespace isolation |
| lib/repositories/passwordCredentialRepository.ts | Combines BaseCredentialRepository with PasswordEncryptionService for password-protected credential storage |
| lib/repositories/passwordCredentialRepository.test.ts | Comprehensive tests for PasswordCredentialRepository including unlock flows and persistence across reloads |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
7b397b9 to
b3aedad
Compare
jbrill
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No testing done yet on my end. Just some comments and questions on the code -- looks great thus far! 🎊
| * Throws if the service has not been unlocked. | ||
| */ | ||
| async encrypt(data: string): Promise<string> { | ||
| if (!this.isUnlocked() || !this.password || !this.salt) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could have this state !this.isUnlocked() || !this.password || !this.salt be a getter/re-usable method, especially since it is repeated in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this.isUnlocked() checks the password and salt. We have to do it here as well, otherwise TS will complain about them possibly being undefined in the line below.
return encrypt(data, this.password, this.salt);
The other option is to use this.salt! but I try to avoid using those exclamations in this way because it's dangerous if used incorrectly.
| } | ||
| } else { | ||
| // New user - generate new salt | ||
| this.salt = generateSalt(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this part of the code, we generate a salt for a new user.
Does it make sense to do this in the unlock method? Would it be better to do this outside of the scope of unlock, which in my opinion might be more ergonomic? Perhaps a new method like below would make sense?
/**
* Initialize the service for a new user (generates salt, etc.)
*/
initialize(options: InitializeOptions): Promise<void>;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better here so consumers only call one method to unlock the encrypted data. It makes the public API easier to understand IMO. Don't set the salt field for new users and set it for returning users, versus having two different methods to call.
| /** | ||
| * Generate a test cipher (for storage by repository) | ||
| */ | ||
| createTestCipher(): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this method needed? Is it for testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is used to verify the password is valid before trying to decrypt the actual data. We do this currently in master, see the credentialStore. It is possible to decrypt data using the wrong password but it won't throw an error, it'll just return garbage unencrypted data. So what we do is use TEST_DATA, which is a string that we hard-code. When a new user creates a password, we encrypt the TEST_DATA and store that in localStorage. Take a look in your browser localStorage after pairing and you'll see cipher in there along with the rest of the data. Now when a returning user enter's their password, we first try to decrypt the cipher and compare the returned value to TEST_DATA. If this matches, then we know the password is correct.
| * Check if this service has stored credentials/data available | ||
| */ | ||
| hasStoredData(): Promise<boolean>; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see in the credentialRepository we have an interface, followed by a base class. There's a different pattern in encryptionService which is just an interface, which passwordEncryptionService extends. Why have the two patterns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this because CredentialRepository has a base class with shared functionality. I put the base class in the same file with the interface. The EncryptionService doesn't have this inheritance structure.
b3aedad to
29a1465
Compare
Summary
This PR introduces the foundational encryption and repository abstractions needed for the upcoming session-based authentication features. It establishes a clean separation between encryption logic (
EncryptionService) and credential storage (CredentialRepository), with concrete password-based implementations for each.These abstractions provide the extension points for passkey and session-based authentication in future PRs. Rather than modifying existing credential storage code directly, we're building a parallel architecture that will eventually power the
UnifiedCredentialStorewhile maintaining backward compatibility.Screenshots
This PR contains this small slice of the new layered architecture.
Technical Notes
EncryptionServiceinterface: A pure encryption abstraction with no storage dependencies. This allows different unlock methods (password, passkey, session) to implement their own encryption strategies while sharing the same contract. The interface handlesunlock,encrypt,decrypt, andlockoperations.PasswordEncryptionService: ImplementsEncryptionServiceusing the existing password-based encryption utilities (lib/util/encryption.ts). It manages password and salt material in memory and generates test ciphers for password verification on subsequent unlocks. The service is stateless with respect to storage—persistence is delegated to the repository layer.CredentialRepositoryinterface andBaseCredentialRepository: Defines how encrypted credentials are stored and retrieved. The base class handles localStorage operations (serializing all credentials under a single namespaced key) while subclasses manage the unlock/lock lifecycle specific to their encryption method.PasswordCredentialRepository: CombinesBaseCredentialRepositorywithPasswordEncryptionServiceto provide password-protected credential storage. On first unlock, it generates and persists the salt and test cipher; on subsequent unlocks, it loads stored values and verifies the password before decryption can proceed.Auth types: Added minimal types (
UnlockMethod,UnlockOptions) tolib/types/lnc.tsthat are needed by these implementations. Passkey and session variants will be added in later PRs when their implementations land.Steps to Test
The classes added here are currently not being used. The unit tests are the only way to confirm the correct behavior.
Run the unit tests to verify the new encryption and repository implementations:
Confirm CI checks pass:
Related Issues & Pull Requests