MetaMask / MetaMask/metamask-extension
Flag controller-state writes that bypass PersistenceManager
- Dominant language
- TypeScript
- Stars
- 13.2k
- Forks
- 5.6k
- Avg merge
- 2d 5h
- Merged PRs (30d)
- 451
Description
**What is this about?**
`BrowserStorageAdapter` sends StorageService operations to `browser.storage.local`. `CronjobControllerStorageManager` also writes `temp-cronjob-storage` there directly. #44251 moves StorageService to IndexedDB, and the cronjob-storage task in #44256 removes the temporary cronjob store. This issue is blocked on both.
## Goal
Fail CI when application code adds a direct controller-state write to `storage.local` outside the approved persistence path.
## Before implementation
Before writing code, try to prove this issue is not worth doing. Make the requirement less dumb before optimizing anything. Look for code, state, or writes we can delete. If deletion does not solve it, prefer a smaller change using what already exists. Record the finding and close the issue if the proposal no longer makes sense.
## Open questions
- Which direct `storage.local` callers remain after #44251 and the cronjob-storage task in #44256?
- Which remaining callers are intentional low-level boundaries?
- Is a lint rule or an architectural test a better fit for the current module boundaries?
**Technical Details**
The implementation must catalogue the remaining intentional low-level callers, including extension migrations, before choosing a lint rule or architectural test.
**Acceptance Criteria**
- [ ] #44251 has moved StorageService off `browser.storage.local`.
- [ ] The cronjob-storage task in #44256 has removed `CronjobControllerStorageManager` and `temp-cronjob-storage`.
- [ ] A new controller-state persistence bypass fails CI with a useful message.
- [ ] Each remaining direct `storage.local` caller is explicitly allowed and has an owner.
**Stakeholder review needed before the work gets merged**
- [ ] Engineering (needed in most cases)
- [ ] Design
- [ ] Product
- [ ] QA (automation tests are required to pass before merging PRs but not all changes are covered by automation tests - please review if QA is needed beyond automation tests)
- [ ] Security
- [ ] Legal
- [ ] Marketing
- [ ] Management (please specify)
- [ ] Other (please specify)
**References**
- Parent: #44253
- StorageService migration: #44251
- Cronjob-storage task: #44256
Contributor guide
Assessment
This issue has not been assessed yet.