Skip to content

Commit

Permalink
Wrap the getFormattedIpfsUrl call in getAssetImageURL in a try catch,… (
Browse files Browse the repository at this point in the history
#21418)

… so the app doesn't crash on certain ipfs urls

Fixes #21417

## **Description**
See the code comment added in this PR for full details:

```
    // With v11.1.0, we started seeing errors thrown that included this
    // line in the stack trace. The cause is that the `getIpfsCIDv1AndPath`
    // method within assets-controllers/src/assetsUtil.ts can throw
    // if part of the ipfsUrl, i.e. the `image` variable within this function,
    // contains characters not in the Base58 alphabet. Details on that are
    // here https://digitalbazaar.github.io/base58-spec/#alphabet. This happens
    // with some NFTs, when we attempt to parse part of their IPFS image address
    // with the `CID.parse` function (CID is part of the multiform package)
    //
    // Before v11.1.0 `getFormattedIpfsUrl` was not used in the extension codebase.
    // Its use within assets-controllers always ensures that errors are caught
    // and ignored. So while we were handling NFTs that can cause this error before,
    // we were always catching and ignoring the error. As of PR #20172, we started
    // passing all NFTs image URLs to `getAssetImageURL` from nft-items.js, which is
    // why we started seeing these errors cause crashes for users in v11.1.0
    //
    // For the sake of a quick fix, we are wrapping this call in a try-catch, which
    // the assets-controllers already do in some form in all cases where this function
    // is called. This probably does not affect user experience, as we would not have
    // correctly rendered these NFTs before v11.1.0 either (due to the same error
    // disuccessed in this code comment).
    //
    // In the future, we can look into solving the root cause, which might require
    // no longer using multiform's CID.parse() method within the assets-controller
```

## **Manual testing steps**

Not sure of an easy way to test.

One way would be:
1. Add an NFT to your wallet which has an ipfs image url with the
character I in it
2. Go to the NFT tab. That NFT should not be visible, but the app should
not crash


## **Related issues**

Related to #18564,
but does not fix it.

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've clearly explained:
  - [ ] What problem this PR is solving.
  - [ ] How this problem was solved.
  - [ ] How reviewers can test my changes.
- [ ] I’ve indicated what issue this PR is linked to: Fixes #???
- [ ] I’ve included tests if applicable.
- [ ] I’ve documented any added code.
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
- [ ] 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.
  • Loading branch information
danjm committed Oct 18, 2023
1 parent 18d33b8 commit a51fd86
Showing 1 changed file with 31 additions and 1 deletion.
32 changes: 31 additions & 1 deletion ui/helpers/utils/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { isObject } from '@metamask/utils';
// eslint-disable-next-line import/no-duplicates
import { isStrictHexString } from '@metamask/utils';
import { CHAIN_IDS, NETWORK_TYPES } from '../../../shared/constants/network';
import { logErrorWithMessage } from '../../../shared/modules/error';
import {
toChecksumHexAddress,
stripHexPrefix,
Expand Down Expand Up @@ -503,7 +504,36 @@ export function getAssetImageURL(image, ipfsGateway) {
}

if (ipfsGateway && image.startsWith('ipfs://')) {
return getFormattedIpfsUrl(ipfsGateway, image, true);
// With v11.1.0, we started seeing errors thrown that included this
// line in the stack trace. The cause is that the `getIpfsCIDv1AndPath`
// method within assets-controllers/src/assetsUtil.ts can throw
// if part of the ipfsUrl, i.e. the `image` variable within this function,
// contains characters not in the Base58 alphabet. Details on that are
// here https://digitalbazaar.github.io/base58-spec/#alphabet. This happens
// with some NFTs, when we attempt to parse part of their IPFS image address
// with the `CID.parse` function (CID is part of the multiform package)
//
// Before v11.1.0 `getFormattedIpfsUrl` was not used in the extension codebase.
// Its use within assets-controllers always ensures that errors are caught
// and ignored. So while we were handling NFTs that can cause this error before,
// we were always catching and ignoring the error. As of PR #20172, we started
// passing all NFTs image URLs to `getAssetImageURL` from nft-items.js, which is
// why we started seeing these errors cause crashes for users in v11.1.0
//
// For the sake of a quick fix, we are wrapping this call in a try-catch, which
// the assets-controllers already do in some form in all cases where this function
// is called. This probably does not affect user experience, as we would not have
// correctly rendered these NFTs before v11.1.0 either (due to the same error
// disuccessed in this code comment).
//
// In the future, we can look into solving the root cause, which might require
// no longer using multiform's CID.parse() method within the assets-controller
try {
return getFormattedIpfsUrl(ipfsGateway, image, true);
} catch (e) {
logErrorWithMessage(e);
return '';
}
}
return image;
}
Expand Down

0 comments on commit a51fd86

Please sign in to comment.