fix(grantAccess): validate queue item before setting allowlist#2605
fix(grantAccess): validate queue item before setting allowlist#2605aristidesstaffieri merged 1 commit intorelease/5.38.0from
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a security vulnerability in the grantAccess handler where domains were being added to the allowlist before validating that a matching queue item exists. The PR also includes significant encryption improvements to the session management system, including proper IV handling and increased PBKDF2 iterations for better security.
Changes:
- Fixed grantAccess handler to validate queue items before modifying the allowlist
- Added comprehensive test coverage for grantAccess handler with edge cases
- Improved encryption security by generating unique IVs per encryption and prepending them to ciphertext
- Increased PBKDF2 iterations from 1,000 to 600,000 for stronger key derivation
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| extension/src/background/messageListener/handlers/grantAccess.ts | Moved setAllowListDomain call after queue validation check to prevent unauthorized domain additions |
| extension/src/background/messageListener/tests/grantAccess.test.ts | Added comprehensive test coverage including valid cases, missing UUIDs, non-existent UUIDs, empty queues, multi-item queues, and invalid response types |
| extension/src/background/helpers/session.ts | Refactored encryption to generate unique IVs per encryption, prepend IVs to ciphertext, and increased PBKDF2 iterations from 1,000 to 600,000 |
| extension/src/background/helpers/tests/session.test.ts | Updated tests to remove IV field from hashKey and added test to verify different ciphertexts for same plaintext |
| extension/src/background/messageListener/tests/createAccount.test.ts | Updated test assertion from checking hashKey.iv to hashKey.key to match new structure |
| extension/src/background/ducks/session.ts | Removed iv field from hashKey interface and initial state |
| extension/public/static/manifest/v3.json | Bumped version from 5.37.2 to 5.37.3 |
| extension/package.json | Bumped version from 5.37.2 to 5.37.3 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot can you rebase into onto release/5.38.0 |
|
@aristidesstaffieri I've opened a new pull request, #2606, to work on those changes. Once the pull request is ready, I'll request review from you. |
Move setAllowListDomain call after queue validation check to ensure domains are only added to the allowlist when a valid response queue item exists. Add comprehensive test coverage for grantAccess handler.
60a1624 to
986c87d
Compare
Move setAllowListDomain call after queue validation check to ensure domains are only added to the allowlist when a valid response queue item exists. Add comprehensive test coverage for grantAccess handler.
Move setAllowListDomain call after queue validation check to ensure domains are only added to the allowlist when a valid response queue item exists. Add comprehensive test coverage for grantAccess handler.
The
grantAccesshandler adds a domain to the allowlist BEFORE checking if a valid queue item exists. The domain is persisted even if no matching UUID is found.Changes:
Move setAllowListDomain call after queue validation check to ensure
domains are only added to the allowlist when a valid response queue
item exists. Add comprehensive test coverage for grantAccess handler.