Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(icon): load base64 data urls #1172

Merged
merged 9 commits into from
Feb 7, 2023
Merged

fix(icon): load base64 data urls #1172

merged 9 commits into from
Feb 7, 2023

Conversation

sean-perkins
Copy link
Contributor

@sean-perkins sean-perkins commented Jan 23, 2023

Previously in: #1141

I had incorrectly assumed that all data-url usage formats were consistent. This was not the case.

Developers use the base64 data-url format to pass encoded image strings to the ion-icon component. This is different than how ionicons inlines SVG images; where the entire SVG element is encoded in the data-url.

This PR reintroduces support for the base64 data-url format by only applying the patch for CSP/data parser when the data url includes utf8 encoding. Otherwise it will fallback to the previous implementation pattern and load the asset (as long as the CSP allows it).

@sean-perkins sean-perkins marked this pull request as ready for review January 23, 2023 19:10
@sean-perkins sean-perkins requested a review from a team January 25, 2023 18:36
Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worthwhile to add a test with a CSP policy enabled?

@sean-perkins
Copy link
Contributor Author

@liamdebeasi there is already: https://github.com/ionic-team/ionicons/tree/FW-3116/src/components/test/csp which validates that a SVG loads with a CSP policy and the utf8 data encoded url.

@liamdebeasi liamdebeasi merged commit 72f0936 into main Feb 7, 2023
@liamdebeasi liamdebeasi deleted the FW-3116 branch February 7, 2023 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants