Ad/fix selected network controller domain setting#22860
Ad/fix selected network controller domain setting#22860
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. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #22860 +/- ##
===========================================
+ Coverage 68.53% 68.54% +0.01%
===========================================
Files 1089 1090 +1
Lines 42980 43007 +27
Branches 11442 11458 +16
===========================================
+ Hits 29455 29478 +23
- Misses 13525 13529 +4 ☔ View full report in Codecov by Sentry. |
Builds ready [a1c01bb]
Page Load Metrics (847 ± 60 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [833006b]
Page Load Metrics (809 ± 24 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [b3df151]
Page Load Metrics (799 ± 54 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
ea07296 to
a468699
Compare
|
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again. Next stepsWhat is new author?A new npm collaborator published a version of the package for the first time. New collaborators are usually benign additions to a project, but do indicate a change to the security surface area of a package. Scrutinize new collaborator additions to packages because they now have the ability to publish code into your dependency tree. Packages should avoid frequent or unnecessary additions or changes to publishing rights. What is network access?This module accesses the network. Packages should remove all network access that is functionally unnecessary. Consumers should audit network access to ensure legitimate use. What is an install script?Install scripts are run when the package is installed. The majority of malware in npm is hidden in install scripts. Packages should not be running non-essential scripts during install and there are often solutions to problems people solve with install scripts that can be run at publish time instead. What is shell access?This module accesses the system shell. Accessing the system shell increases the risk of executing arbitrary code. Packages should avoid accessing the shell which can reduce portability, and make it easier for malicious shell access to be introduced. What is a CVE?Contains a high severity Common Vulnerability and Exposure (CVE). Remove or replace dependencies that include known high severity CVEs. Consumers can use dependency overrides or npm audit fix --force to remove vulnerable dependencies. What is a mild CVE?Contains a low severity Common Vulnerability and Exposure (CVE). Remove or replace dependencies that include known low severity CVEs. Consumers can use dependency overrides or npm audit fix --force to remove vulnerable dependencies. Why is contributor and author data important?Package does not specify a list of contributors or an author in package.json. Add a author field or contributors array to package.json. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
|
@metamaskbot update-policies |
|
Policy update failed. You can review the logs or retry the policy update here |
97a7b77 to
9427829
Compare
Builds ready [57cc976]
Page Load Metrics (1077 ± 47 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| setProviderType: (type) => { | ||
| // when using this format, type happens to be the same as the networkClientId... | ||
| this.selectedNetworkController.setNetworkClientIdForMetamask(type); | ||
| return this.networkController.setProviderType(type); | ||
| }, | ||
| setActiveNetwork: (networkConfigurationId) => { | ||
| this.selectedNetworkController.setNetworkClientIdForMetamask( | ||
| networkConfigurationId, | ||
| ); | ||
| return this.networkController.setActiveNetwork(networkConfigurationId); |
There was a problem hiding this comment.
this is now handled internally in the SelectedNetworkController: https://github.com/MetaMask/core/pull/3908/files#diff-5e705302d595033f24cac5f20d14ecf307709fd61c46a6d01bf9519bf5b91d86R154-R161
| // only use proxyClient from selectedNetworkController (and only set default selectedNetworkClientId for current origin) when the following are all true: | ||
| // 1. selectedNetworkClientIdForDomain has not been set | ||
| // 2. feature flag for perDomainNetwork is on | ||
| // 3. there exists a permission (any permission) for the given origin/domain | ||
| // Why 3? because we end up calling setupProviderEngine here regardless of whethe the in-page provider has been accessed to actually use metamask. What this really ends up meaning is that | ||
| // without doing #3, we will save a record for every single domain that the inpage provider is injected for (iframes, frames, every tab, way too much). What we really want is | ||
| // to only maintain records for domains that have actually tried using metamask. As such, we use 'have they set a permission before' as a proxy for this. | ||
| let proxyClient; |
There was a problem hiding this comment.
TODO see if this can be separated out into a separate PR
| // const checkTokenDetection = useCallback(async () => { | ||
| // try { | ||
| // const fetchedTokenData = await fetchWithCache({ | ||
| // url: `${TOKEN_API_METASWAP_CODEFI_URL}${providerConfig.chainId}`, | ||
| // functionName: 'getIsTokenDetectionSupported', | ||
| // }); | ||
| // const isTokenDetectionSupported = !fetchedTokenData?.error; | ||
| // setTokenDetectionSupported(isTokenDetectionSupported); | ||
| // setIsLoading(false); | ||
| // } catch { | ||
| // // If there's any error coming from getIsTokenDetectionSupported | ||
| // // we would like to catch this error and simply return false for the state | ||
| // // and this will be handled in UI naturally | ||
| // setTokenDetectionSupported(false); | ||
| // setIsLoading(false); | ||
| // } | ||
| // }, [providerConfig.chainId]); |
… to state regardless of whether they have permissions (#3908) ## @metamask/selected-network-controller ### Changed - **BREAKING:** Remove logic in `selectedNetworkMiddleware` to set a default `networkClientId` for the requesting origin when not already set. Now if no `networkClientId` is already set for the requesting origin, the middleware will not add the origin to `domains` state but will add the `networkClientId` currently set for the `selectedNetworkClient` from the `NetworkController` to the request object. - **BREAKING:** `setNetworkClientIdForDomain` now throws an error if passed `metamask` as its first (`domain`) argument - **BREAKING:** `setNetworkClientIdForDomain` now includes a check that the requesting `domain` has already been granted permissions in the `PermissionsController` before adding it to `domains` state and throws an error if the domain does not have permissions. - **BREAKING:** the `domains` state now no longer contains a `metamask` key. - **BREAKING:** `getProviderAndBlockTracker` now throws an error if called with any domain while the `perDomainNetwork` flag is false. (These changes help fix an issue where the `SelectedNetworkController` adds any and all domains the user visits to its domains state whether or not the user has connected to these sites.) Currently can be e2e tested on this WIP integration branch: MetaMask/metamask-extension#22860 --------- Co-authored-by: Jiexi Luan <jiexiluan@gmail.com>
… all visted domains are added to SelectedNetworkController state, use migration to clean up state
33bd8ea to
8d02240
Compare
Currently the
SelectedNetworkControlleradds any and all domains the user visits to itsdomainsstate whether or not the user has connected to these sites. This PR cleans up the undesired state caused by this bug and adds a check for existing permissions for an origin before adding it todomainsstate going forward