Bug: No placeholder icon when safe app's logo cannot be loaded #1516

mmv08 · 2023-01-10T15:28:47Z

What it solves

Resolves safe-global/safe-react-apps#637 by rendering an img tag inside the sandboxed iframe for fetching safe apps icon (for more context see #1193)

thanks to @katspaugh for answering questions and suggesting script tag workaround

How this PR fixes it

It renders an img tag and attaches error event listener. It also allows scripts inside the iframe to define the error listener. Shouldn't be a security risk. Also it seems like scripts must be same-origin and cross origin ones have to be explicitly enabled

How to test it

Fetch an app with an erroring image (I don't know such app so i ran https://github.com/safe-global/safe-apps-sdk/tree/master/packages/safe-apps-test-app locally)

Screenshots

github-actions · 2023-01-10T15:29:13Z

Branch preview

✅ Deploy successful!

https://bug_safe_apps_placeholder_cion--webcore.review-web-core.5afe.dev

mmv08 · 2023-01-10T15:30:11Z

src/components/safe-apps/SafeAppIcon/index.tsx

-      background: url('${src}') center center/contain no-repeat;
-    }
-  </style>`
+const APP_LOGO_FALLBACK_IMAGE = `/images/apps/app-placeholder.svg`


a new cross-theme icon by Lila

github-actions · 2023-01-10T15:30:29Z

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	0	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

src/components/safe-apps/SafeAppIcon/index.tsx

katspaugh · 2023-01-10T15:41:25Z

src/components/safe-apps/SafeAppIcon/index.tsx

+const getIframeContent = (url: string, alt: string, width: number, height: number): string => {
+  return `
+     <body style="margin: 0;">
+       <img src=${url} alt=${alt} width="${width}" height="${height}" />


I would add style="display: block" to avoid a line underneath.

What line? I didn't see anything unwanted in all browsers I tested (chrome, safari desktop, firefox)

I remember you mentioned it in the slack convo but when working on it I didn't see a need for it

This line:

Inline elements such as img have a line-height that are bigger than the height of the image itself.

Doesn't really matter in this case tho, because the iframe itself is 40x40.

katspaugh · 2023-01-11T07:51:53Z

src/components/safe-apps/SafeAppIcon/index.tsx

+const getIframeContent = (url: string, alt: string, width: number, height: number): string => {
+  return `
+     <body style="margin: 0;">
+       <img src=${url} alt=${alt} width="${width}" height="${height}" />


I would wrap url and alt in double quotes, and escape any double quotes in those strings. Otherwise, we have a classic XSS hole here.

Actually, I would hardcode smth like "app logo" in the alt tag. The name of the app is already on the card anyway.

Good spot regarding xss. I've checked other platforms with lists to see how they implement alt attributes and seems like they don't lol (checked chrome web store, slack store)

katspaugh · 2023-01-11T10:58:14Z

src/components/safe-apps/SafeAppIcon/index.tsx

+const getIframeContent = (url: string, width: number, height: number): string => {
+  return `
+     <body style="margin: 0;">
+       <img src="${url}" alt="App logo" width="${width}" height="${height}" />


Like I said, you also might want to escape dangerous symbols here. encodeURI(url) should do the trick.

The url is validated at the fetch time and also browsers seem to be smart nowadays: https://security.stackexchange.com/questions/124840/xss-vectors-in-img-src-and-background-image-url

Given we have all these safety things in place I think an additional check inside the component is redundant

It's not about fetching, you're inserting a string directly into HTML. It can close the tag and render a butt.

Doesn't seem like a big security threat to me given that we validate the URL before and the amount of sandboxing we enforce on the iframe. But to get the pr through i added it

katspaugh · 2023-01-11T11:21:44Z

Have you tried this btw?
https://stackoverflow.com/a/980910/352796

mmv08 · 2023-01-11T13:24:25Z

Have you tried this btw? https://stackoverflow.com/a/980910/352796

It seems to follow the CSP policy set in the app and doesn't allow loading from cross origin domains. If I understand correctly, an object tag can load arbitrary data and not only images, that's why it's enforced

katspaugh

Dunno if it's worth it just to show a placeholder for broken icons (it would be better to fix such icons quickly) but looks good. 👍

JagoFigueroa · 2023-01-11T14:36:28Z

Muchas gracias gentlemen, todo bueno 👍

mmv08 requested review from katspaugh, yagopv, dasanra and DaniSomoza January 10, 2023 15:28

mmv08 force-pushed the bug/safe-apps-placeholder-cion branch from 7fc3895 to 32b69c4 Compare January 10, 2023 15:29

mmv08 commented Jan 10, 2023

View reviewed changes

mmv08 force-pushed the bug/safe-apps-placeholder-cion branch from 32b69c4 to 30e512f Compare January 10, 2023 15:31

mmv08 marked this pull request as ready for review January 10, 2023 15:31

mmv08 changed the title ~~Bug:~~ Bug: safe app's placeholder icon isn't shown Jan 10, 2023

mmv08 changed the title ~~Bug: safe app's placeholder icon isn't shown~~ Bug: No placeholder icon when safe app's logo cannot be loaded Jan 10, 2023

katspaugh reviewed Jan 10, 2023

View reviewed changes

src/components/safe-apps/SafeAppIcon/index.tsx Show resolved Hide resolved

katspaugh reviewed Jan 10, 2023

View reviewed changes

src/components/safe-apps/SafeAppIcon/index.tsx Show resolved Hide resolved

katspaugh reviewed Jan 10, 2023

View reviewed changes

mmv08 force-pushed the bug/safe-apps-placeholder-cion branch from 30e512f to cea8649 Compare January 10, 2023 16:56

katspaugh reviewed Jan 11, 2023

View reviewed changes

mmv08 force-pushed the bug/safe-apps-placeholder-cion branch 2 times, most recently from c26343f to c890435 Compare January 11, 2023 10:30

mmv08 requested a review from katspaugh January 11, 2023 10:30

katspaugh reviewed Jan 11, 2023

View reviewed changes

mmv08 requested a review from katspaugh January 11, 2023 13:24

render an img tag in SafeAppIcon iframe

d14db7b

mmv08 force-pushed the bug/safe-apps-placeholder-cion branch from c890435 to d14db7b Compare January 11, 2023 13:33

katspaugh approved these changes Jan 11, 2023

View reviewed changes

mmv08 merged commit d2b0397 into dev Jan 11, 2023

mmv08 deleted the bug/safe-apps-placeholder-cion branch January 11, 2023 16:20

github-actions bot locked and limited conversation to collaborators Jan 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug: No placeholder icon when safe app's logo cannot be loaded #1516

Bug: No placeholder icon when safe app's logo cannot be loaded #1516

mmv08 commented Jan 10, 2023 •

edited

Loading

github-actions bot commented Jan 10, 2023 •

edited

Loading

mmv08 Jan 10, 2023 •

edited

Loading

github-actions bot commented Jan 10, 2023 •

edited

Loading

katspaugh Jan 10, 2023

mmv08 Jan 10, 2023

mmv08 Jan 10, 2023

katspaugh Jan 11, 2023

katspaugh Jan 11, 2023

katspaugh Jan 11, 2023 •

edited

Loading

katspaugh Jan 11, 2023 •

edited

Loading

mmv08 Jan 11, 2023

katspaugh Jan 11, 2023

mmv08 Jan 11, 2023

mmv08 Jan 11, 2023

katspaugh Jan 11, 2023

mmv08 Jan 11, 2023

katspaugh commented Jan 11, 2023

mmv08 commented Jan 11, 2023

katspaugh left a comment

JagoFigueroa commented Jan 11, 2023

Bug: No placeholder icon when safe app's logo cannot be loaded #1516

Bug: No placeholder icon when safe app's logo cannot be loaded #1516

Conversation

mmv08 commented Jan 10, 2023 • edited Loading

What it solves

How this PR fixes it

How to test it

Screenshots

github-actions bot commented Jan 10, 2023 • edited Loading

Branch preview

mmv08 Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jan 10, 2023 • edited Loading

ESLint Summary View Full Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katspaugh Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

katspaugh Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katspaugh commented Jan 11, 2023

mmv08 commented Jan 11, 2023

katspaugh left a comment

Choose a reason for hiding this comment

JagoFigueroa commented Jan 11, 2023

mmv08 commented Jan 10, 2023 •

edited

Loading

github-actions bot commented Jan 10, 2023 •

edited

Loading

mmv08 Jan 10, 2023 •

edited

Loading

github-actions bot commented Jan 10, 2023 •

edited

Loading

katspaugh Jan 11, 2023 •

edited

Loading

katspaugh Jan 11, 2023 •

edited

Loading