-
-
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
refactor: Refactor inpage blocklist to avoid usage of regex #8675
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. |
E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/63f2cd35-fd45-4f0e-b96d-5bbb17b85e18 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@tommasini also I struggled to mock out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested by copying the new function from here, visiting websites in the browser, and pasting the function and a call to blockedDomainCheck()
into the console.
It seems to work very well!
0ee0612
to
659f495
Compare
I've updated this to match the latest changes found on MetaMask/metamask-extension#23134 |
Bitrise🔄🔄🔄 Commit hash: 659f495 Note
|
Bitrise✅✅✅ Commit hash: 19b4d5c Note
|
Quality Gate passedIssues Measures |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8675 +/- ##
=======================================
Coverage 43.23% 43.23%
=======================================
Files 1271 1271
Lines 30905 30905
Branches 3088 3088
=======================================
Hits 13361 13361
Misses 16769 16769
Partials 775 775 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you update the tests?
As you provided an example of dangerous url that could pass with the regex, to confirm the issue is solved, you could add a test case in the unit tests matching against this ani.gamerxcom.tw
type of URL.
Same for the second case with https://google.com?search=uscourts.gov
@NicolasMassart I tried to get some kind of unit test set up and running but struggled trying to figure out how to mock out some of the browser window functions with the current setup. Unlike in extension (https://github.com/MetaMask/metamask-extension/pull/23134/files), the code in this file gets executed the moment it gets imported resulting in difficulties getting a suite of multiple tests running correctly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NicolasMassart I tried to get some kind of unit test set up and running but struggled trying to figure out how to mock out some of the browser window functions with the current setup. Unlike in extension (https://github.com/MetaMask/metamask-extension/pull/23134/files), the code in this file gets executed the moment it gets imported resulting in difficulties getting a suite of multiple tests running correctly.
As discussed, we don't have any ways to test this right now but we are aware it should be.
So creating an issue to investigate testing this injected script is a good start. (please post the link in this PR).
Related issue has been created here: #9009 |
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 asani.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 asani.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 inuscourts.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
https://github.com/MetaMask/mobile-planning/issues/1571
Manual testing steps
Pre-merge author checklist
Pre-merge reviewer checklist
Note: Issue with testing has been created here: #9009