refactor: Remove usage of regex for portfolio URLs by NEllusion · Pull Request #8674 · MetaMask/metamask-mobile

NEllusion · 2024-02-22T16:47:38Z

Description

This PR addresses a few issues related to how we were using regex expressions. In particular, how we were using URLs in regex statements. This is due to the fact that if we use a URL in a regex statement without escaping it, the periods get interpreted as regex wildcard characters.

In this case, it means that URLs such as https://portfolioxmetamask.io would be be interpreted by MetaMask as being a valid portfolio URL. This could open up the door to phishing situations or unexpected behaviour.

Rather than attempting to make the regex safe to use, I opted to re-write the code without the need for regex. This uses built in Javascript URL for more accurate comparisons.

Related issues

https://github.com/MetaMask/mobile-planning/issues/1571

Manual testing steps

Pre-merge author checklist

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.

github-actions · 2024-02-22T16:47:48Z

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.

github-actions · 2024-02-22T17:02:14Z

E2E test started on Bitrise: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/98ba3fbe-2b24-456e-9467-f66a49fde58c
You can also kick off another Bitrise E2E smoke test by removing and re-applying the (Run Smoke E2E) label

NEllusion · 2024-02-22T17:28:48Z

Quality Gate failed

Failed conditions 8.8% Duplication on New Code (required ≤ 5%)

See analysis details on SonarCloud

I also noticed this duplicate code but its outside the scope of this change to refactor the duplicated components

tommasini

LGTM!

codecov-commenter · 2024-02-27T21:39:18Z

Codecov Report

Attention: Patch coverage is 65.00000% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 46.00%. Comparing base (d1d5c00) to head (504edcc).

Files	Patch %	Lines
app/components/UI/Tokens/index.tsx	0.00%	3 Missing ⚠️
app/components/UI/AccountOverview/index.js	0.00%	2 Missing ⚠️
app/components/UI/Bridge/utils/useGoToBridge.ts	33.33%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8674      +/-   ##
==========================================
+ Coverage   45.98%   46.00%   +0.01%     
==========================================
  Files        1273     1274       +1     
  Lines       31342    31352      +10     
  Branches     3213     3215       +2     
==========================================
+ Hits        14414    14424      +10     
  Misses      16079    16079              
  Partials      849      849

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

NicolasMassart

Awesome!

NEllusion · 2024-03-20T14:02:16Z

@NicolasMassart looks like theres a conflict, i'll re-tag you once I rebase!

app/util/url/url.test.ts

Co-authored-by: Nico MASSART <NicolasMassart@users.noreply.github.com>

sonarqubecloud · 2024-04-04T15:28:02Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
75.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

sonarqubecloud · 2024-04-04T15:28:16Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
75.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

github-actions · 2024-04-08T15:52:37Z

Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 17ae813
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/7c8529bc-2b66-4bbe-bfb9-8ae0f50bdb59

Note

You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

sethkfman

LGTM

Co-authored-by: Nico MASSART <NicolasMassart@users.noreply.github.com>

sethkfman

LGTM

sonarqubecloud · 2024-04-10T21:51:59Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
75.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

NEllusion mentioned this pull request Feb 22, 2024

fix: Address issues with inaccurate regex expressions #8661

Closed

13 tasks

NEllusion changed the title ~~Remove usage of regex for portfolio URLs~~ chore: Remove usage of regex for portfolio URLs Feb 22, 2024

NEllusion added the team-mobile-platform Mobile Platform team label Feb 22, 2024

NEllusion changed the title ~~chore: Remove usage of regex for portfolio URLs~~ refactor: Remove usage of regex for portfolio URLs Feb 22, 2024

metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Feb 22, 2024

NEllusion force-pushed the ellul/remove-regex-portfolio branch from 1ec88e8 to 6cf64a3 Compare February 22, 2024 16:58

NEllusion marked this pull request as ready for review February 22, 2024 17:00

NEllusion requested a review from a team as a code owner February 22, 2024 17:00

github-actions bot added the Run Smoke E2E label Feb 22, 2024

NEllusion force-pushed the ellul/remove-regex-portfolio branch from 6cf64a3 to 5a03866 Compare February 22, 2024 17:07

tommasini previously approved these changes Feb 22, 2024

View reviewed changes

NEllusion dismissed tommasini’s stale review via 4d0eb52 February 26, 2024 17:05

NEllusion force-pushed the ellul/remove-regex-portfolio branch from 5a03866 to 4d0eb52 Compare February 26, 2024 17:05

NEllusion requested review from sethkfman and tommasini February 27, 2024 21:29

NEllusion added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Mar 6, 2024

NEllusion mentioned this pull request Mar 6, 2024

refactor(sdk): update blockedDomainCheck to remove regex MetaMask/metamask-sdk#740

Merged

3 tasks

NicolasMassart previously approved these changes Mar 20, 2024

View reviewed changes

NicolasMassart reviewed Mar 20, 2024

View reviewed changes

app/util/url/url.test.ts Show resolved Hide resolved

Remove usage of regex for portfolio URLs

6dcc34a

NEllusion dismissed NicolasMassart’s stale review via 6dcc34a March 20, 2024 14:08

NEllusion force-pushed the ellul/remove-regex-portfolio branch from 5ee86ff to 6dcc34a Compare March 20, 2024 14:08

NEllusion requested a review from NicolasMassart March 20, 2024 14:08

Merge branch 'main' into ellul/remove-regex-portfolio

b72ef28

NEllusion removed the Run Smoke E2E label Mar 27, 2024

sethkfman and others added 11 commits April 4, 2024 09:16

Update app/util/url/url.test.ts

27bb23d

Co-authored-by: Nico MASSART <NicolasMassart@users.noreply.github.com>

Update app/util/url/url.test.ts

911125b

Co-authored-by: Nico MASSART <NicolasMassart@users.noreply.github.com>

Update app/util/url/url.test.ts

3cd0a41

Co-authored-by: Nico MASSART <NicolasMassart@users.noreply.github.com>

Update app/util/url/url.test.ts

a2284a2

Co-authored-by: Nico MASSART <NicolasMassart@users.noreply.github.com>

Update app/util/url/url.test.ts

b5918cc

Co-authored-by: Nico MASSART <NicolasMassart@users.noreply.github.com>

Update app/util/url/url.test.ts

a3ded4b

Co-authored-by: Nico MASSART <NicolasMassart@users.noreply.github.com>

Update app/util/url/url.test.ts

d3e61bd

Co-authored-by: Nico MASSART <NicolasMassart@users.noreply.github.com>

Update app/util/url/url.test.ts

023019a

Co-authored-by: Nico MASSART <NicolasMassart@users.noreply.github.com>

Update app/util/url/url.test.ts

9c364e7

Co-authored-by: Nico MASSART <NicolasMassart@users.noreply.github.com>

Update app/util/url/url.test.ts

e136eb3

Co-authored-by: Nico MASSART <NicolasMassart@users.noreply.github.com>

Merge branch 'main' into ellul/remove-regex-portfolio

cbd1db2

NEllusion added 2 commits April 4, 2024 11:28

Merge branch 'main' into ellul/remove-regex-portfolio

504edcc

yarn deduplicate

17ae813

NEllusion added Run Smoke E2E and removed Run Smoke E2E labels Apr 8, 2024

NEllusion requested a review from NicolasMassart April 8, 2024 15:51

sethkfman previously approved these changes Apr 10, 2024

View reviewed changes

Update app/util/url/url.test.ts

7cabe00

Co-authored-by: Nico MASSART <NicolasMassart@users.noreply.github.com>

sethkfman dismissed their stale review via 7cabe00 April 10, 2024 21:41

Merge branch 'main' into ellul/remove-regex-portfolio

e80ba67

sethkfman approved these changes Apr 10, 2024

View reviewed changes

sethkfman merged commit ef475b7 into main Apr 10, 2024

sethkfman deleted the ellul/remove-regex-portfolio branch April 10, 2024 22:07

github-actions bot locked and limited conversation to collaborators Apr 10, 2024

github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Apr 10, 2024

metamaskbot added the release-7.21.0 Issue or pull request that will be included in release 7.21.0 label Apr 10, 2024

Uh oh!

Conversation

NEllusion commented Feb 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Manual testing steps

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

github-actions bot commented Feb 22, 2024

Uh oh!

github-actions bot commented Feb 22, 2024

Uh oh!

NEllusion commented Feb 22, 2024

Quality Gate failed

Uh oh!

tommasini left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Feb 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

NicolasMassart left a comment

Choose a reason for hiding this comment

Uh oh!

NEllusion commented Mar 20, 2024

Uh oh!

Uh oh!

sonarqubecloud bot commented Apr 4, 2024

Quality Gate passed

Uh oh!

sonarqubecloud bot commented Apr 4, 2024

Quality Gate passed

Uh oh!

github-actions bot commented Apr 8, 2024 • edited by metamaskbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bitrise

Uh oh!

sethkfman left a comment

Choose a reason for hiding this comment

Uh oh!

sethkfman left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Apr 10, 2024

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

NEllusion commented Feb 22, 2024 •

edited

Loading

codecov-commenter commented Feb 27, 2024 •

edited

Loading

github-actions bot commented Apr 8, 2024 •

edited by metamaskbot

Loading