feat: Sanitize iframe URL

[yagopv]

What it solves

Sanitize iframe URL for avoid any JS execution

[github-actions]

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	1	N/A

• Result: ✅ success
• Annotations: 0 total

---

^{Report generated by eslint-plus-action}

[github-actions]

Deployment links

🟠 Rinkeby	⚪ Mainnet	🟣 Polygon	🟡 BSC	⚫ Arbitrum	🟢 Gnosis Chain

[iamacook]

The isSameURL() check will fail here as your comparing the sanitized URL with the original appUrl. It's probably best to santiize inside useSafeAppUrl(). What do you think?

[katspaugh]

I don't think you need HTML entity unescaping. React isn't inserting the iframe URL in HTML, it sets it as a DOM property.
So if someone passes a URL in HTML encoding, it will be inserted as is.

[yagopv]

I don't think you need HTML entity unescaping. React isn't inserting the iframe URL in HTML, it sets it as a DOM property. So if someone passes a URL in HTML encoding, it will be inserted as is.

Yeah I can remove it. I took the script from a library and is working with more cases than our concrete one

[katspaugh]

It would be nice to show an error straight away.

After N seconds:

Also it showed a disclaimer modal. I think when we see a JavaScript URL, we should just immediately tell the user they are likely being phished.

[yagopv]

The isSameURL() check will fail here as your comparing the sanitized URL with the original appUrl. It's probably best to santiize inside useSafeAppUrl(). What do you think?

I did it. Moved to the hook. I changed the location of the function itself and added the tests

[iamacook]

Looks really good dude! Thanks for adding the tests as well! What do you think about redirecting to the Apps page if the sanitizer returns an empty string? It still remains in the URL:

[katspaugh]

Redirecting to Apps IMHO would be no bueno, as it will not alert the user and hide the problem.
If they stay on that page and see an error, they will be aware of potential phishing attempts.

[yagopv]

Redirecting to Apps IMHO would be no bueno, as it will not alert the user and hide the problem.
If they stay on that page and see an error, they will be aware of potential phishing attempts.

I like the error approach. Take a look now. I'm throwing an error if the sanitize call detect a javascript: injection warning the user

[katspaugh]

It crashes the app immediately now, good 👍

The error is generic tho, something we could improve. But good enough for now.

[iamacook]

Nice work 🚀

[usame-algan]

Looks good! Thanks for adding tests 💯

[yagopv]

@JagoFigueroa did you test this?

[JagoFigueroa]

Excelente implementación camarada, looks good to me as well.

Un saludo ;)

[mmv08]

Why does it allow null as a param?