[PM-18046] Implement session storage #547
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces JavaScript-managed session repositories for the IPC client in WASM environments. The changes enable WebAssembly clients to manage IPC sessions using their own state providers instead of relying solely on in-memory storage within the SDK.
Key changes:
- Added
JsSessionRepositoryfor JavaScript-backed session management - Created
GenericSessionRepositoryenum to support both in-memory and JavaScript-backed session repositories - Refactored
JsIpcClientconstructors to support both repository types
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
crates/bitwarden-ipc/src/wasm/mod.rs |
Added module imports and re-exports for new session repository types |
crates/bitwarden-ipc/src/wasm/js_session_repository.rs |
Implements JavaScript-backed session repository with thread-safe wrapper |
crates/bitwarden-ipc/src/wasm/ipc_client.rs |
Refactored to use GenericSessionRepository with two factory methods for different session repository types |
crates/bitwarden-ipc/src/wasm/generic_session_repository.rs |
Enum wrapper providing unified interface for different session repository implementations |
| pub enum GenericSessionRepository { | ||
| InMemory(Arc<InMemorySessionRepository<Session>>), | ||
| JsSessionRepository(Arc<JsSessionRepository>), | ||
| } |
There was a problem hiding this comment.
The GenericSessionRepository enum is missing a Clone implementation. This is required because IpcClient clones itself (via Arc::clone) in the start method (line 172 of ipc_client.rs) and handle_rpc_request method (line 338 of ipc_client.rs), which requires all fields to be cloneable. Since both variants contain Arc, add: #[derive(Clone)] above the enum definition.
There was a problem hiding this comment.
@copilot But I don't need JsIpcClient to be cloneable, plus, start takes self: &Arc<Self> which clones the Arc not the actual IpcClient
There was a problem hiding this comment.
Lol, created a whole new PR just to acknowledge it was wrong
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #547 +/- ##
==========================================
+ Coverage 78.36% 79.63% +1.27%
==========================================
Files 291 300 +9
Lines 29343 32247 +2904
==========================================
+ Hits 22994 25680 +2686
- Misses 6349 6567 +218 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Claude finished @coroiu's task —— View job Code Review CompleteThis PR implements JavaScript-managed session repositories for IPC clients in WASM environments, allowing clients to manage sessions using their own state providers. The implementation is well-structured with proper thread safety and clear separation of concerns. FindingsFinding 1 Finding 2 ♻️: Hardcoded Finding 3 💭: Missing documentation for error handling contract in JavaScript interface at Finding 4 🎨: Constructor naming could better reflect the security distinction at Good Practices Observed
Action Items
|
|
Claude review responses:
EDIT: Apparently claude just modifies its existing comment so these numbers are no longer valid 🤷 |
|
Wow Claude, thank you for giving me a suggestion and then raising your own suggestion as a critical issue 👍 👍 👍 👍 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| async fn get(&self, endpoint: Endpoint) -> Result<Option<Session>, Self::GetError> { | ||
| self.0 | ||
| .run_in_thread(move |repo| async move { | ||
| let js_value = repo.get(endpoint).await.map_err(|e| format!("{e:?}"))?; |
There was a problem hiding this comment.
Finding 1
Converting errors to strings using format!("{e:?}") loses structured error information and makes it difficult for callers to handle specific error cases programmatically. The same pattern appears on lines 74, 84, 87, and 96.
Context
Since you mentioned in PR comments that "We don't need to catch these errors" and implementing proper error propagation across JS/WASM boundaries is costly, this is acceptable for the current use case. However, this creates technical debt if error handling requirements change later.
Consider documenting this decision in a comment explaining that errors are intentionally simplified because they're not expected to be handled programmatically by consuming applications.
| wasm::JsSessionRepository, | ||
| }; | ||
|
|
||
| // TODO: Change session type when implementing encryption |
There was a problem hiding this comment.
Finding 2 ♻️: Hardcoded Session type creates technical debt
The hardcoded type Session = () means this enum cannot support typed sessions until refactored. When encryption is implemented, this will require either:
- Breaking API changes to add a generic parameter
- Runtime type erasure with increased complexity
The TODO acknowledges this, but consider whether the wasm-bindgen limitation could be worked around by:
- Using a trait object approach
- Implementing separate constructors for different session types
- Deferring type parameter resolution to a later point in the API
|
|
||
| #[wasm_bindgen(typescript_custom_section)] | ||
| const TS_CUSTOM_TYPES: &'static str = r#" | ||
| export interface IpcSessionRepository { |
There was a problem hiding this comment.
Finding 3 💭: Missing documentation for error handling contract
The TypeScript interface doesn't document what happens when these Promise methods throw errors. JavaScript consumers need to know:
- What types of errors to expect
- Whether they should implement error handling
- What the consequences of errors are for session state
Consider adding JSDoc comments to the interface explaining the error contract, especially since errors are converted to strings and not propagated meaningfully.
| /// Create a new `IpcClient` instance with an in-memory session repository for saving | ||
| /// sessions within the SDK. | ||
| #[wasm_bindgen(js_name = newWithSdkInMemorySessions)] | ||
| pub fn new_with_sdk_in_memory_sessions( |
There was a problem hiding this comment.
Finding 4 🎨: Constructor naming could better reflect the security distinction
The PR description mentions that in-memory sessions are preferred "when state providers might not make sense, for example if they will use insecure IPC to save the data, defeating the whole point of a secure session."
Consider renaming to emphasize this security aspect:
newWithSecureInMemorySessionsinstead ofnewWithSdkInMemorySessions- Or add documentation explaining the security implications of each choice
This would help JavaScript consumers make informed security decisions rather than viewing this as simply an implementation detail preference.
🎟️ Tracking
📔 Objective
Add JS-compatible session repository implementation. I also added the ability to switch for situations where state providers might not make sense, for example if they will use insecure IPC to save the data, defeating the whole point of a secure session.
⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes