MetaMask / MetaMask/metamask-extension

Flag controller-state writes that bypass PersistenceManager

Open
#44,802 1 comment 0 reactions 1 assignee Claimed by @MajorLift View on GitHub
exalate-duplicate INVALID-ISSUE-TEMPLATE team-extension-platform
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.