-
Notifications
You must be signed in to change notification settings - Fork 27
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
refactor(config): expose sanitizeSvg
function, fix diffing logic
#2862
Conversation
🦋 Changeset detectedLatest commit: a4238ac The changes in this PR will be included in the next version bump. This PR includes changesets to release 35 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@@ -1 +1,2 @@ | |||
export * from './formatters'; | |||
export { default as sanitizeSvg } from './sanitize-svg'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that we can use the same logic in the backend.
Deploy preview for merchant-center-application-kit ready! ✅ Preview Built with commit a4238ac. |
RETURN_DOM: true, | ||
FORBID_ATTR: [ | ||
// To avoid injection by using `style="filter:url(\"data:image/svg+xml,<svg` | ||
'style', | ||
], | ||
}); | ||
}).innerHTML; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you perhaps explain what this changes are meant for?
Is it to sanitize the same way it's done in the API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes sorry. We already changed this in the backend, due to some security findings.
https://research.securitum.com/mutation-xss-via-mathml-mutation-dompurify-2-0-17-bypass/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wowzers, thanks for explaining 🙏 I guess we should remember to change that then in the <InlineSvg>
component from the ui-kit too (probably as part of the upcoming svg changes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good point!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
I noticed when using the CLI
config:sync
command that the SVG icon was always marked as changed.The problem is in the fact that the API sanitizes the SVG before saving the data but we were also pre-sanitizing it when the config was parsed. This lead to updates to have run the sanitize 2 times.
Now we don't sanitize when parsing the icon but only during the diffing, so ensure the diffed data is the same.