Commit 4a0a472
authored
fix: WalletConnect sign requests show garbled origin in 'Request from' (#29102)
## **Description**
When a dapp connects via WalletConnect v2, the "Request from" field on
sign request confirmation screens occasionally renders a random long
string (or empty value) instead of the dapp's domain. This causes
confirmations to look untrustworthy and triggers "malicious/deceptive"
warnings — reported in the wild against `merchant.cray.pro`.
Root cause: `getUnverifiedRequestOrigin` in
`app/core/WalletConnect/wc-utils.ts` used a nullish-coalescing fallback
on `request.verifyContext?.verified?.origin`. The WalletConnect Verify
API can legitimately return a non-`null`/`undefined` value that is not a
URL (an empty string, or a non-URL identifier when the dapp is
unverified or initiated from a non-browser context). `??` does not fall
back on empty or non-URL strings, so the bogus value was rendered
verbatim in the UI.
Fix: only trust `verified.origin` when it parses as a URL
(`isValidUrl`). Otherwise fall back to `session.peer.metadata.url` — the
same fallback already used when `verifyContext` is entirely absent. No
new behavior, just a tighter validity check on the existing code path.
The change is isolated to one pure utility function and is covered by
new unit tests that characterize both the bug and the fix (they fail
against the prior implementation).
## **Changelog**
CHANGELOG entry: Fixed WalletConnect sign requests occasionally
displaying a garbled string in the "Request from" field instead of the
dapp's domain.
## **Related issues**
Fixes:
[WAPI-1449](https://consensyssoftware.atlassian.net/browse/WAPI-1449)
## **Manual testing steps**
The originally reported dapp (`merchant.cray.pro`) is unreliable, so the
authoritative verification path is the unit tests added in this PR. A
happy-path smoke test is also recommended to confirm no regression on
well-behaved dapps.
```gherkin
Feature: WalletConnect sign request origin display
Scenario: happy-path dapp still shows its real domain
Given a user with MetaMask Mobile installed
And a well-behaved WalletConnect v2 dapp (e.g. react-app.walletconnect.com)
When the user connects via WalletConnect and triggers a personal_sign request
Then the confirmation sheet's "Request from" row displays the dapp's domain
And no regression is observed versus main
Scenario: dapp with an invalid verifyContext origin falls back gracefully
Given a WalletConnect v2 session where verifyContext.verified.origin is empty or a non-URL string
When the user triggers a sign request
Then the confirmation sheet's "Request from" row displays session.peer.metadata.url
And no random identifier is rendered
```
Unit tests:
```
yarn jest app/core/WalletConnect/wc-utils.test.ts
```
All 42 tests pass with the fix. The 3 bug-characterization tests (empty
string, non-URL string, bare hostname) fail when the fix is reverted,
confirming they protect against regression.
## **Screenshots/Recordings**
N/A — cannot reliably reproduce the reported UI without the affected
dapp. Unit tests deterministically cover the behavior change.
### **Before**
Confirmation's "Request from" field showed a random long string for
affected dapps.
### **After**
Falls back to `session.peer.metadata.url`, so "Request from" shows the
dapp's self-reported URL instead of a garbled value.
## **Pre-merge author checklist**
- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I've included tests if applicable
- [x] I've documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] 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.
## **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.
[WAPI-1449]:
https://consensyssoftware.atlassian.net/browse/WAPI-1449?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
<!-- CURSOR_SUMMARY -->
---
> [!NOTE]
> **Medium Risk**
> Changes the logic used to display a WalletConnect request’s origin;
while small and well-tested, it affects a security-sensitive
confirmation UI field and could impact what users see when approving
signatures.
>
> **Overview**
> Prevents WalletConnect signing confirmations from showing
garbled/empty “Request from” values by only using
`request.verifyContext.verified.origin` when it’s a valid, parseable
URL; otherwise it falls back to the existing `defaultOrigin` (peer
metadata URL).
>
> Adds unit tests for `getUnverifiedRequestOrigin` covering valid URL,
empty string, non-URL identifier, missing `verifyContext`, and
bare-hostname cases to lock in the new fallback behavior.
>
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
dadb5ec. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->1 parent dcad69f commit 4a0a472
2 files changed
Lines changed: 75 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
10 | 10 | | |
11 | 11 | | |
12 | 12 | | |
| 13 | + | |
13 | 14 | | |
| 15 | + | |
14 | 16 | | |
15 | 17 | | |
16 | 18 | | |
| |||
392 | 394 | | |
393 | 395 | | |
394 | 396 | | |
| 397 | + | |
| 398 | + | |
| 399 | + | |
| 400 | + | |
| 401 | + | |
| 402 | + | |
| 403 | + | |
| 404 | + | |
| 405 | + | |
| 406 | + | |
| 407 | + | |
| 408 | + | |
| 409 | + | |
| 410 | + | |
| 411 | + | |
| 412 | + | |
| 413 | + | |
| 414 | + | |
| 415 | + | |
| 416 | + | |
| 417 | + | |
| 418 | + | |
| 419 | + | |
| 420 | + | |
| 421 | + | |
| 422 | + | |
| 423 | + | |
| 424 | + | |
| 425 | + | |
| 426 | + | |
| 427 | + | |
| 428 | + | |
| 429 | + | |
| 430 | + | |
| 431 | + | |
| 432 | + | |
| 433 | + | |
| 434 | + | |
| 435 | + | |
| 436 | + | |
| 437 | + | |
| 438 | + | |
| 439 | + | |
| 440 | + | |
| 441 | + | |
| 442 | + | |
| 443 | + | |
| 444 | + | |
| 445 | + | |
| 446 | + | |
| 447 | + | |
| 448 | + | |
| 449 | + | |
| 450 | + | |
| 451 | + | |
| 452 | + | |
| 453 | + | |
| 454 | + | |
| 455 | + | |
| 456 | + | |
| 457 | + | |
| 458 | + | |
395 | 459 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
416 | 416 | | |
417 | 417 | | |
418 | 418 | | |
419 | | - | |
| 419 | + | |
| 420 | + | |
| 421 | + | |
| 422 | + | |
| 423 | + | |
| 424 | + | |
| 425 | + | |
| 426 | + | |
| 427 | + | |
| 428 | + | |
| 429 | + | |
0 commit comments