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
Remove deprecated usage of inline SVG #7183
Conversation
This seems fine to me given that scripts embedded in SVG data URIs aren't executed, but I wonder if this is something we might want to run by ProdSec just to be extra-safe |
* Remove deprecated usage of inline SVG * Change pipefile back * Fix test * Cleanup * Add back xssSanitizeSvg * Fix svg xmlns addition * Add test * Remove xss sanitation * Remove dompurify * Update notices * Update unit tests * Remove unused import * Remove unneeded test * Fix e2e test * Remove print * Update e2e tests * Update snapshots * Update image test * Fix gif with caption * Add missing snapshot * Fix docstring * Update package.json * Format
* Remove deprecated usage of inline SVG * Change pipefile back * Fix test * Cleanup * Add back xssSanitizeSvg * Fix svg xmlns addition * Add test * Remove xss sanitation * Remove dompurify * Update notices * Update unit tests * Remove unused import * Remove unneeded test * Fix e2e test * Remove print * Update e2e tests * Update snapshots * Update image test * Fix gif with caption * Add missing snapshot * Fix docstring * Update package.json * Format
* Remove deprecated usage of inline SVG * Change pipefile back * Fix test * Cleanup * Add back xssSanitizeSvg * Fix svg xmlns addition * Add test * Remove xss sanitation * Remove dompurify * Update notices * Update unit tests * Remove unused import * Remove unneeded test * Fix e2e test * Remove print * Update e2e tests * Update snapshots * Update image test * Fix gif with caption * Add missing snapshot * Fix docstring * Update package.json * Format
Describe your changes
Since this PR last year, almost all SVGs used in
st.image
get rendered by using theimg
tag instead of injecting the SVG into the HTML DOM. As far as I remember, the only reason we keptmarkup
was to still support xlink functionality for external links. However, xlink has been long deprecated (see here), and usage is not recommended.Here are a couple more arguments for why we should remove the SVG markup injection:
img
tag is considered XSS save (as far as I know). On the other hand, injecting SVG into the HTML DOM is quite problematic from a security perspective, even though we try to sanitize the SVG.st.image
behave the same by using the img tag, the usage of markup did not correctly adapt to the otherst.image
parameters (width
,use_column_width
, and fullscreen mode).But this change might also break a couple of apps that include external resources within the SVG. For example, this would not work anymore since it requires an external image:
This PR also removes the
xssSanitizeSvg
for SVG data URIs. The reason is that this isn't very effective since an attacker could easily circumvent this, e.g., by using base64 or URL-encoding the SVG data URI or by embedding the image via markdown:Script in SVG is not executed when used with
img
tags, so this should be anyways safe without requiring sanitation.This PR also converts all SVGs to base64 on the backend to prevent encoding issues (e.g. #3882).
GitHub Issue Link (if applicable)
Closes: #3882
Testing Plan
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.