Safe-react: Can only add Safe apps with an INVALID manifest

[rmeissner]

Title/Description

Currently we throw an error when a Safe app with a fully valid manifest is about to be added.

Reason is that we seem to be missing a !

https://github.com/gnosis/safe-react/blob/47d20aa645ed4a79bbffa996a415802ad781b0aa/src/routes/safe/components/Apps/utils.ts#L186-L188

https://github.com/gnosis/safe-react/blob/47d20aa645ed4a79bbffa996a415802ad781b0aa/src/routes/safe/components/Apps/utils.ts#L143-L153

Environment

• Browser: Chrome
• Wallet: MM
• App Version: 2.19.2
• Environment:
  
  
  
  • production (rinkeby)
  
  

Steps to reproduce

• try to add Safe app that has name, description, iconPath and url set in the manifest

Expected result

• Can add Safe app

Obtained result

• Cannot add Safe app

Notes

• If url is removed the Safe app can be added
• It is important to check if fixing this breaks existing apps, as they might not been added with valid manifest files
  
  
  
  • https://github.com/gnosis/safe-react-apps/blob/development/apps/tx-builder/public/manifest.json (no url)
  
  
  • https://github.com/gnosis/safe-react-apps/blob/development/apps/wallet-connect/public/manifest.json (no url)
  
  
  • https://safe-cmm.gnosis.io/manifest.json (no url)
  
  
  • https://ipfs.io/ipfs/QmQovvfYYMUXjZfNbysQDUEXR8nr55iJRwcYgJQGJR7KEA/manifest.json (no url)
  
  
  • ... seems most apps are missing the url, it might be nessesary to check all default Safe apps.
  
  

[tschubotz]

To discuss: First "soft breaking" this by allowing both, and later on "hard breaking" change.

[nicosampler]

@rmeissner seems like url attribute should not be mandatory at all. In fact, I think we should remove it completely.
In the docs we are not mention this attribute: https://docs.gnosis.io/safe/docs/sdks_safe_apps/#1-your-safe-app-must-include-a-manifestjson-file[…]fsqmtgnb1j9fdr9gimptzvaeina25s92iqy37gyqyfwzw8aj.

I can not recall the intention of this attribute, but if it was to refer to the team behind the safe app, the attribute for it should be providedBy.

Am I missing something?

[rmeissner]

Yes that would be the intented attribute. I am also not sure how much sense it makes to include this mendatory (as any developer can set whatever they want there). As you said, maybe we should just remove the url check and adjust the docs to say, that you can provide an optional providedBy that will be displayed in the interface in the future. (cc @lukasschor )

[lukasschor]

Not sure if I understood correctly. But providedBy should be required (as outlined in the docs). So shouldn't it be instead of

!!appInfo.url && --> !!appInfo.providedBy &&?

[rmeissner]

this would break all existing apps (as they don't have this ... probably). Also I am not sure if I would make this required for custom apps (so not the default apps), as we cannot validate what devs are setting there.

[nicosampler]

agree with Richard!

[lukasschor]

Got it. We should still keep in mandatory as part of the listing process though. Wasn't aware this has not been properly checked so far. While developers can of course just write whatever they want there, it should be the responsibility of the curator of the default list (now Gnosis and in the future maybe a TCR) to validate this information.

[rmeissner]

One thing I just discovered: Actually if we allow the url param and safe apps use it in the manifest, this would currently break the safe app, as this url would be used in the iframe:

https://github.com/gnosis/safe-react/blob/a2bbbf4177f92f31c9a5c3ddafb836b97a87d41f/src/routes/safe/components/Apps/utils.ts#L213-L219

[nicosampler]

I think that res.url in line 216 refers to this: https://github.com/gnosis/safe-react/blob/a2bbbf4177f92f31c9a5c3ddafb836b97a87d41f/src/routes/safe/components/Apps/utils.ts#L197, and no the URL from the manifest.

I got confused with that as well.

[rmeissner]

I tested it and it causes issues.

The appInfo.data contains a url and res contains a url but their are different (one is the url of the app the other of the webpage of the developer)

as you first unwrap res and then appInfo it will overwrite the app url with the web page url

[nicosampler]

what do you suggest? ~leave it but not as required?~
~we should deprecate it anyways, and encourage users to use providedBy instead~

Edit: I understood it wrong. should we omit the url then?

[rmeissner]

in any case you should prevent this conflict ... the user might set for any other reason a url in the manifest ... that should not mess with this logic. You could not just unwrap the appInfo object ... or you could delete the url the object. Not sure what a better pattern is 🤔 .

I would probably explicitly read the fields from the manifest that we are interested in as they tend to have quite some other fields too (as they are used by react to store information too)

[nicosampler]

@rmeissner can you try it now?

[rmeissner]

I can try it later today