Skip to content

Conversation

smilingkylan
Copy link
Contributor

@smilingkylan smilingkylan commented Aug 5, 2025

Description

The purpose if this task is to fix a vulnerability whereby long URLs do not have the most important part of their URL (domain + TLD) visible in the WebView Browser URL Bar on iOS. We fix this by getting the URL bar to prioritize that portion of the URL while ellipsizing (...) the first part of the URL (protocol and subdomain, etc)

Changelog

CHANGELOG entry: Move ellipsizing of WebView URL to beginning of URLs for security reasons

Related issues

Fixes: https://github.com/MetaMask/mobile-planning/issues/2310
Mobile-planning: https://github.com/MetaMask/mobile-planning/issues/2277

Manual testing steps

Feature: WebView URL truncation / ellipsizing (...)
  Scenario: user goes to WebView / in-app browser
    Given user navigates to http://web2cryptwalletnftmlogar.metamask.com.cybersecurityresearch.live/
    When user looks at the URL in the URL bar
    Then they should see an ellipsis (...) at the BEGINNING of the URL and NOT the end of the URL

Screenshots/Recordings

Before

Screenshot 2025-08-05 at 10 33 43 AM

After

IMG_0024

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.

Copy link
Contributor

github-actions bot commented Aug 5, 2025

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.

@smilingkylan smilingkylan added team-mobile-platform Mobile Platform team Run E2E Smoke Test WIP: Please use "Run Smoke E2E" instead labels Aug 5, 2025
@smilingkylan smilingkylan marked this pull request as ready for review August 5, 2025 00:33
@smilingkylan smilingkylan added the No QA Needed Apply this label when your PR does not need any QA effort. label Aug 5, 2025
cursor[bot]

This comment was marked as outdated.

@MetaMask MetaMask deleted a comment from sonarqubecloud bot Aug 5, 2025
@smilingkylan smilingkylan changed the title fix: TS error in BrowserUrlBar fix: Truncate URL appropriately on iOS Aug 5, 2025
@smilingkylan smilingkylan added Run Smoke E2E Requires smoke E2E testing and removed Run E2E Smoke Test WIP: Please use "Run Smoke E2E" instead labels Aug 5, 2025
@MetaMask MetaMask deleted a comment from github-actions bot Aug 5, 2025
@smilingkylan smilingkylan added Run Smoke E2E Requires smoke E2E testing and removed Run Smoke E2E Requires smoke E2E testing labels Aug 5, 2025
@MetaMask MetaMask deleted a comment from github-actions bot Aug 5, 2025
@smilingkylan smilingkylan added Run Smoke E2E Requires smoke E2E testing and removed Run Smoke E2E Requires smoke E2E testing labels Aug 5, 2025
Copy link
Contributor

github-actions bot commented Aug 5, 2025

https://bitrise.io/ Bitrise

✅✅✅ pr_smoke_e2e_pipeline passed on Bitrise! ✅✅✅

Commit hash: 8829b2e
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/995c8107-bde4-4b44-a684-372571d64239

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

cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

numberOfLines={1}
ellipsizeMode="head"
>
{inputValueRef.current || activeUrl}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of providing this with a child prop, is it possible to just set text property on the native prop of the text ref?

Copy link
Contributor Author

@smilingkylan smilingkylan Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Cal-L I'm kinda unsure of what you're asking here. I added the activeUrl in as a fallback when the component first mounts but before the BrowserTab has done a setNativeProps call. I'm on the fence of whether we really need it or not since it begs the question of whether the user is really on a web page if it hasn't even been loaded yet. I could add it to this area but the if (props.text) will mean the changing of the inputValueRef.current won't even occur since it is guarded by the conditional:

        if (props.text) {
          inputValueRef.current = props.text;
        }
        ```

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Cal-L Can we resolve the conversation?

cursor[bot]

This comment was marked as outdated.

@MetaMask MetaMask deleted a comment from cursor bot Aug 11, 2025
cursor[bot]

This comment was marked as outdated.

Copy link

Copy link

@joaoloureirop joaoloureirop added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Sep 18, 2025
@github-project-automation github-project-automation bot moved this to Needs dev review in PR review queue Sep 18, 2025
Copy link
Contributor

@sethkfman sethkfman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-project-automation github-project-automation bot moved this from Needs dev review to Review finalised - Ready to be merged in PR review queue Sep 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) No QA Needed Apply this label when your PR does not need any QA effort. Run Smoke E2E Requires smoke E2E testing size-M team-mobile-platform Mobile Platform team
Projects
Status: Review finalised - Ready to be merged
Development

Successfully merging this pull request may close these issues.

5 participants