Skip to content

Commit

Permalink
chore: fix: prevent bad svg urls in react-native-svg (#8102) (#8134)
Browse files Browse the repository at this point in the history
## **Description**
Some dapps had bad icon urls in their metadata which prevented rendering
favicon properly. This PR adds a patch to react-native-svg to prevent
trying to render invalid svg urls.

## **Related issues**

Fixes:

[#1431](https://app.zenhub.com/workspaces/metamask-mobile-5f984938ddc0e4001d4b79cb/issues/gh/metamask/mobile-planning/1431)

## **Manual testing steps**

1. In-app browser --> Go to [https://btc20.com/](https://btc20.com/)
2. Connect to dapp with WC
3. Should not error

## **Screenshots/Recordings**

### WC connect to site with bad meta url



https://github.com/MetaMask/metamask-mobile/assets/141057783/a1645016-6814-4be6-b027-d085856bad79

### Search Bar icons



https://github.com/MetaMask/metamask-mobile/assets/141057783/0328e3f3-7b60-440b-8667-72156c5667cc

### Tab Bar icons




https://github.com/MetaMask/metamask-mobile/assets/141057783/e1bc694b-68fe-417f-b493-97c6f066690f



## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've clearly explained what problem this PR is solving and how it
is solved.
- [ ] I've linked related issues
- [ ] I've included manual testing steps
- [ ] I've included screenshots/recordings if applicable
- [ ] I’ve included tests if applicable
- [ ] 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.
- [ ] I’ve properly set the pull request status:
  - [ ] In case it's not yet "ready for review", I've set it to "draft".
- [ ] 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.

---------

## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

## **Related issues**

Fixes:

## **Manual testing steps**

1. Go to this page...
2.
3.

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->

### **After**

<!-- [screenshots/recordings] -->

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've clearly explained what problem this PR is solving and how it
is solved.
- [ ] I've linked related issues
- [ ] I've included manual testing steps
- [ ] I've included screenshots/recordings if applicable
- [ ] I’ve included tests if applicable
- [ ] 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.
- [ ] I’ve properly set the pull request status:
  - [ ] In case it's not yet "ready for review", I've set it to "draft".
- [ ] 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.

Co-authored-by: Frank von Hoven <[email protected]>
  • Loading branch information
chrisleewilcox and frankvonhoven authored Dec 15, 2023
1 parent b4e5883 commit 007e524
Showing 1 changed file with 9 additions and 2 deletions.
11 changes: 9 additions & 2 deletions patches/react-native-svg+12.1.1.patch
Original file line number Diff line number Diff line change
@@ -1,14 +1,21 @@
diff --git a/node_modules/react-native-svg/src/xml.tsx b/node_modules/react-native-svg/src/xml.tsx
index 828f104..9eaf82a 100644
index 828f104..db726a9 100644
--- a/node_modules/react-native-svg/src/xml.tsx
+++ b/node_modules/react-native-svg/src/xml.tsx
@@ -123,21 +123,32 @@ export function SvgXml(props: XmlProps) {
@@ -123,21 +123,39 @@ export function SvgXml(props: XmlProps) {
}

export async function fetchText(uri: string) {
- const response = await fetch(uri);
- return await response.text();
+ const response = await fetch(uri);
+ // This is a temporary fix for dapps with bad metadata icon urls
+ // Remove this once we replace WebsiteIcon with AvatarFavicon component
+ const excludeList = ['text/html', ''];
+ const contentType = response.headers.get('content-type') || '';
+ if (excludeList.includes(contentType)) {
+ throw new Error(`Fetching ${uri} resulted in invalid content-type ${contentType}`);
+ }
+ if (!response.ok) {
+ throw new Error('Image not found');
+ }
Expand Down

0 comments on commit 007e524

Please sign in to comment.