-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
content-script lacks tests required to ensure stability and accuracy #9009
Comments
13 tasks
NicholasEllul
added a commit
that referenced
this issue
Mar 20, 2024
## **Description** This PR fixes two bugs that occured as the result of using regex to identify URLs in our content script blocklist. The first issue is that we were only escaping the first `.` found in a URL when using the inpage blocklist. This meant that entries such as `ani.gamer.com.tw` would have their first period escaped for regex parsing, but subsequent periods were treated as regex wildcards. This could lead to and unintentionally matching on URLs such as `ani.gamerxcom.tw` etc. The second issue is that we were missing a leading anchor `^` in the regex expression. This means that we would block the domain if the matched string occurred anywhere in the URL. For an example, `https://google.com?search=uscourts.gov` would be a blocked domain since it ended in `uscourts.gov`. Adding the leading anchor addresses this so we only match the correct domain. To avoid future regex complexities, this code has been refactored to use built in javascript URL parsing instead. ## **Related issues** MetaMask/mobile-planning#1571 ## **Manual testing steps** ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've clearly explained what problem this PR is solving and how it is solved. - [x] I've linked related issues - [ ] I've included manual testing steps - [x] I've included screenshots/recordings if applicable - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [X] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. - [X] I’ve properly set the pull request status: - [X] In case it's not yet "ready for review", I've set it to "draft". - [X] In case it's "ready for review", I've changed it from "draft" to "non-draft". ## **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. Note: Issue with testing has been created here: #9009
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
What is this about?
Context
The inpage bridge content-script is a file that the application injects into the various webpages visited within the in-app browser. The moment that the file is included in its javascript environment, it immediately executes while analyzing that the page meets the required criteria for the MetaMask provider to be injected.
Problem
Despite this file playing a key role in determining if MetaMask is able to work on a page, we lack the necessary tests to ensure it's stability and functional accuracy. As described in #8675 it is not straightforward to test this file due to the fact that it will execute it's code the moment it gets imported (see code). This means that in a test file, it will attempt to run it's logic prior to the developer being able to set up the necessary mocks required to mock out the window object.
Potential Solutions
Simple Solution
An initial solution would be to refactor the content-script to not execute as soon as it is imported into a javascript environment. Instead, it should be imported, and require an explicit functional call in order for it to be run. This is the approach taken by extension who exports a shouldInjectProvider that is later called explicitly. This allows for the provider to be explicitly tested as seen in extension here.
Likewise, SDK takes an even more modular approach of abstracting out each function used to determine if the provider should be injected or not. This allows for even more precise testing of the logic.
@NicolasMassart suggested that if we take a similar approach, we can instead invoke the function explicitly in one of the WebView callbacks once the page has loaded (e.g onMessage).
Ideal solution
As this code is duplicated in Extension, Mobile, and SDK, an ideal solution would be to pull this into a single package that is then imported in used across all our products. This would simplify the process and eliminate each team needing to maintain this code individually.
Scenario
No response
Design
No response
Technical Details
No response
Threat Modeling Framework
No response
Acceptance Criteria
No response
Stakeholder review needed before the work gets merged
References
No response
The text was updated successfully, but these errors were encountered: