Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great step forward in improving security by removing innerHTML and DomSanitizer when rendering icons, opting for a safer CSS mask approach. The changes are well-implemented across the affected components and services. I've identified a bug where icons containing double quotes in their SVG data might not render correctly, and I've provided suggestions to fix this. Overall, excellent work on this refactoring.
| } | ||
| return domSanitizer.bypassSecurityTrustHtml(parsed[1]); | ||
| }; | ||
| const parseDataSvgIcon = (icon: string): string => `url("${icon}")`; |
There was a problem hiding this comment.
The current implementation of parseDataSvgIcon does not handle double quotes within the icon string, which can lead to broken image URLs. If an icon's SVG data contains a double quote, it will prematurely terminate the string within the url() function, causing the icon to fail to render. To fix this, you should escape any double quotes within the icon string.
| const parseDataSvgIcon = (icon: string): string => `url("${icon}")`; | |
| const parseDataSvgIcon = (icon: string): string => `url("${icon.replace(/"/g, '\\"')}")`; |
| return ''; | ||
| } | ||
| return this.domSanitizer.bypassSecurityTrustHtml(parsed[1]); | ||
| return `url("${icon}")`; |
There was a problem hiding this comment.
The current implementation of parseDataSvgIcon does not handle double quotes within the icon string, which can lead to broken image URLs. If an icon's SVG data contains a double quote, it will prematurely terminate the string within the url() function, causing the icon to fail to render. To fix this, you should escape any double quotes within the icon string.
| return `url("${icon}")`; | |
| return `url("${icon.replace(/"/g, '\\"')}")`; |
spike-rabbit
force-pushed
the
refactor/icons/bind-as-css-mask
branch
from
23cc34f to
93d24fd
Compare
This avoids potential XSS risks, by injecting the icon only as CSS mask-image.
spike-rabbit
force-pushed
the
refactor/icons/bind-as-css-mask
branch
from
93d24fd to
03207c6
Compare
|
|
|
Documentation. Coverage Reports: |
|
@dr-itz do you have any opinion here? I don't remember, why we did not implement it that way in the beginning. |
1 participant
This avoids potential XSS risks, by injecting the icon only as CSS mask-image.