Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
|
@metamaskbot update-policies |
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring alerts on:
|
seaona
added
the
no-changelog
✨ Files requiring CODEOWNER review ✨🫰 @MetaMask/core-platform (4 files, +56 -2)
🧩 @MetaMask/extension-devs (5 files, +170 -325)
📜 @MetaMask/policy-reviewers (5 files, +170 -325)
Tip Follow the policy review process outlined in the LavaMoat Policy Review Process doc before expecting an approval from Policy Reviewers. 🧪 @MetaMask/qa (2 files, +10 -9)
🔗 @MetaMask/supply-chain (5 files, +170 -325)
|
|
Policies updated. 🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff |
|
❌ test-e2e-chrome-api-specs failed. View the html report here. |
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Results generated automatically by MetaMask CI |
Builds ready [b792007]
UI Startup Metrics (1219 ± 65 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
| }); | ||
| await driver.waitForSelector({ text: 'OK' }); | ||
| await driver.clickElement({ | ||
| await driver.clickElementAndWaitForWindowToClose({ |
There was a problem hiding this comment.
ℹ️ unrelated change, but to make test more robust
| }); | ||
| await driver.findElement({ text: 'Add to MetaMask', tag: 'h3' }); | ||
| await driver.clickElementSafe('[data-testid="snap-install-scroll"]', 200); | ||
| await driver.waitForSelector({ text: 'Confirm' }); |
There was a problem hiding this comment.
ℹ️ unrelated change, but not needed
| 'https://metamask.github.io/snap-account-abstraction-keyring/0.5.0/framework-7f36badc7ddb1e3597e8.js', | ||
| 'https://metamask.github.io/snap-account-abstraction-keyring/0.5.0/page-data/app-data.json', | ||
| 'https://metamask.github.io/snap-account-abstraction-keyring/0.5.0/page-data/index/page-data.json', | ||
| 'https://metamask.github.io/snap-account-abstraction-keyring/0.5.0/webpack-runtime-eb27ff9e27bd689ff465.js', |
There was a problem hiding this comment.
🎉 removing more entries from the live request allow list, yay!
| @@ -1,7 +1,7 @@ | |||
| { | |||
There was a problem hiding this comment.
ℹ️ the changes in the snap binary files are just the result of running this command yarn update-snap-binary --snap-account-abstraction-keyring@1.0.0
| import { Mockttp } from 'mockttp'; | ||
| import { mockAccountAbstractionKeyringSnap } from '../snap-binary-mocks'; | ||
|
|
||
| export async function serveSnapAccountAbstractionKeyRingFromLocalhost( |
There was a problem hiding this comment.
ℹ️ same approach as we did for the snap simple keyring #36557
I created a specific folder for the snap local sites, but won't move the other file in this PR as this would result in lot sof file changes. I'll do a follow up Pr to place the snap simple keyring local server mock in this same folder
|
@SocketSecurity ignore npm/@metamask/snap-account-abstraction-keyring-site@1.0.0. this is used only in our e2e, and it's expected |
| - '@metamask/test-dapp-multichain' | ||
| - '@metamask/test-dapp-solana' | ||
| - '@metamask/snap-simple-keyring-site' | ||
| - '@metamask/snap-account-abstraction-keyring-site' |
There was a problem hiding this comment.
ℹ️ added to exceptions to fix the unused dependencies error, because that's not used anywhere (only in the e2e)
|
@SocketSecurity ignore npm/@metamask/snap-account-abstraction-keyring-site@1.0.0 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| response.headers.get('content-type') || 'text/html; charset=utf-8', | ||
| }, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Bug: Mock Intercepts Requests, Fails Assets
The serveSnapAccountAbstractionKeyRingFromLocalhost mock broadly intercepts all metamask.github.io requests, causing unrelated sites to fail when proxied to the local server. Additionally, it incorrectly maps all snap-account-abstraction-keyring sub-paths to the root, preventing the snap's assets from loading correctly.
There was a problem hiding this comment.
we intercept all metamask.github.io requests, because there are multiple:
'https://metamask.github.io/snap-account-abstraction-keyring/0.5.0/',
'https://metamask.github.io/snap-account-abstraction-keyring/0.5.0/72585f70-0b8c9d47690d7fe2ac87.js',
'https://metamask.github.io/snap-account-abstraction-keyring/0.5.0/app-27784e6248e8644fa873.js',
'https://metamask.github.io/snap-account-abstraction-keyring/0.5.0/component---src-pages-index-tsx-65d9e73b6aea71652319.js',
'https://metamask.github.io/snap-account-abstraction-keyring/0.5.0/f36c6662-e3e593644ccdf91e0df1.js',
'https://metamask.github.io/snap-account-abstraction-keyring/0.5.0/framework-7f36badc7ddb1e3597e8.js',
'https://metamask.github.io/snap-account-abstraction-keyring/0.5.0/page-data/app-data.json',
'https://metamask.github.io/snap-account-abstraction-keyring/0.5.0/page-data/index/page-data.json',
'https://metamask.github.io/snap-account-abstraction-keyring/0.5.0/webpack-runtime-eb27ff9e27bd689ff465.js',
instead of going one by one. There's no risk, as there are no other requests going to any metamask.github site (for the test dapp, we go to localhost). We could adjust accordingly if this edge case ever happens, but it doesn't seem something likely to happen (it would be only if we went to another live site/snap site in a same spec, which is unlikely, as we separate tests by concerns)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Results generated automatically by MetaMask CI |
Builds ready [bc72af3]
UI Startup Metrics (1239 ± 72 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
| "@ethersproject/abi>@ethersproject/keccak256": true, | ||
| "@ethersproject/abi>@ethersproject/logger": true, | ||
| "ethers>@ethersproject/keccak256": true, | ||
| "ethers>@ethersproject/logger": true, |
There was a problem hiding this comment.
No new builtins/globals/endowments were introduced; it seems the dependency graph changed due to the dev dependency added in this PR, but doesn't add any new capabilities, so it seems no risk is added
@MetaMask/policy-reviewers
…nd remove live requests to the site (#36801) <!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** This PR implements a localserver for the `snap-account-abstraction-keyring-site` instead of going to the live site, in our e2e, to avoid flakiness. This implies the following: - Removing all related live requests from the allowlist - Adding the `snap-account-abstraction-keyring-site` as an npm package - Updating the snap simple keyring binaries, as the version was updated to 1.0.0 . The changes are just the result of running this yarn update-snap-binary --snap-account-abstraction-keyring@1.0.0 - In our tests, we are not going to localhost directly, as localhost is not in the allowlist for that snap. So we take the same approach as we did before, suggested by @FrederikBolding , where we proxy the localhost, so we still see the origin as metamask.github so then we don't get errors, and we avoid adding localhost in the allowlist (this was a small security concern). Context: https://consensys.slack.com/archives/GN3SR3GNM/p1759489395046839 Usage in our e2e: ```javascript await withFixtures( { dapp: true, dappPaths: ['snap-account-abstraction-keyring-site'], ... ``` - Related work: [test: add snap-simple-keyring-site as a local server and remove live requests to the site](#36557) - Snap Account Abstraction Keyring Release PR: MetaMask/snap-account-abstraction-keyring#216 [](https://codespaces.new/MetaMask/metamask-extension/pull/36801?quickstart=1) ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: ## **Related issues** Fixes: https://consensyssoftware.atlassian.net/browse/MMQA-428 ## **Manual testing steps** 1. Check ci ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > E2E tests now use a locally served `@metamask/snap-account-abstraction-keyring-site` with new mocks, removing live site requests and updating related configs and dependencies (including ethers 5.8). > > - **E2E/Testing**: > - Serve `@metamask/snap-account-abstraction-keyring-site` locally via `dappPaths` and new mock proxy (`account-abstraction-keyring-site-mocks.ts`). > - Replace snap mocks import; install flow updated to wait for dialog close; integrate site and swaps mocks together. > - Remove live site URLs from `test/e2e/mock-e2e-allowlist.js`. > - **Config/Tooling**: > - Add `@metamask/snap-account-abstraction-keyring-site` to `package.json` and `.depcheckrc.yml` ignore list. > - Update snap binary headers for `snap-account-abstraction-keyring@1.0.0`. > - Extend test helper to resolve local site path. > - **Security Policy**: > - Adjust LavaMoat Browserify/Webpack policies to reference `ethers` subpackages consistently (switch to `ethers>@ethersproject/*`). > - **Dependencies**: > - Bump `ethers` and all `@ethersproject/*` to `5.8.0`; update `ws`, `elliptic` accordingly. > - Refresh MetaMask packages (`json-rpc-engine`, `json-rpc-middleware-stream`, add legacy `providers` for site) and add site-related packages. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit bc72af3. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: MetaMask Bot <metamaskbot@users.noreply.github.com>
](https://codespaces.new/MetaMask/metamask-extension/pull/36801?quickstart=1){kind=link}
metamaskbot
added
the
release-13.6.0
5 participants
Description
This PR implements a localserver for the
snap-account-abstraction-keyring-siteinstead of going to the live site, in our e2e, to avoid flakiness. This implies the following:snap-account-abstraction-keyring-siteas an npm packageUsage in our e2e:
Related work: test: add snap-simple-keyring-site as a local server and remove live requests to the site
Snap Account Abstraction Keyring Release PR: 1.0.0 snap-account-abstraction-keyring#216
Changelog
CHANGELOG entry:
Related issues
Fixes: https://consensyssoftware.atlassian.net/browse/MMQA-428
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist
Note
E2E tests now use a locally served
@metamask/snap-account-abstraction-keyring-sitewith new mocks, removing live site requests and updating related configs and dependencies (including ethers 5.8).@metamask/snap-account-abstraction-keyring-sitelocally viadappPathsand new mock proxy (account-abstraction-keyring-site-mocks.ts).test/e2e/mock-e2e-allowlist.js.@metamask/snap-account-abstraction-keyring-sitetopackage.jsonand.depcheckrc.ymlignore list.snap-account-abstraction-keyring@1.0.0.etherssubpackages consistently (switch toethers>@ethersproject/*).ethersand all@ethersproject/*to5.8.0; updatews,ellipticaccordingly.json-rpc-engine,json-rpc-middleware-stream, add legacyprovidersfor site) and add site-related packages.Written by Cursor Bugbot for commit bc72af3. This will update automatically on new commits. Configure here.