feat(icon): enable svg icons by default#1508
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 updates the default icon rendering to use SVGs and cleans up demo app configuration. While a functional improvement, enabling SVG icons by default introduces a critical Cross-Site Scripting (XSS) vulnerability due to unsanitized SVG content. Furthermore, a critical issue with unit tests needs to be resolved.
| const ICON_CONFIG = new InjectionToken<IconConfig>('ICON_CONFIG', { | ||
| providedIn: 'root', | ||
| factory: () => ({ disableSvgIcons: true }) | ||
| factory: () => ({ disableSvgIcons: false }) |
There was a problem hiding this comment.
Enabling SVG icons by default exposes a potential Cross-Site Scripting (XSS) vulnerability. The parseDataSvgIcon function in projects/element-ng/icon/si-icons.ts uses DomSanitizer.bypassSecurityTrustHtml on SVG content without sanitization, bypassing Angular's XSS protection. This could allow malicious scripts if icon content originates from untrusted sources. Recommendation: Sanitize the SVG content (e.g., using DOMPurify) before passing it to bypassSecurityTrustHtml in si-icons.ts.
Furthermore, changing the default of disableSvgIcons to false breaks existing unit tests in projects/element-ng/icon/si-icon.component.spec.ts. These tests rely on the old default and need to be updated. Please modify the affected test suite to explicitly provide provideIconConfig({ disableSvgIcons: true }) to correctly test the non-default case.
There was a problem hiding this comment.
No, I will provide something. We should have done this a long time ago.
There was a problem hiding this comment.
Angular doesn't have its own way to sanitize svg content. So we must have to use some other 3rd party like DOMPurify.
Question is do we introduce DOMPurify as peerDep?
There was a problem hiding this comment.
This does not work, as we need to support SSR here. In the beginning we werer using the DOMParser (which is basically the DOMPurify approach
There was a problem hiding this comment.
in that case we can use https://github.com/kkomelin/isomorphic-dompurify
There was a problem hiding this comment.
font-awesome btw just bypasses as well.
There was a problem hiding this comment.
based on my understanding of the code, https://github.com/kkomelin/isomorphic-dompurify will always load us JSDom into the browser
There was a problem hiding this comment.
I don't think this is a real issue, the majority of users will simply reference an existing icon from our package. In rare cases, the will provide an SVG icon by the self. In such a case we could simply improve the addIcon description that this scenario is risky and might cause potential cross site scripting problems
chintankavathia
force-pushed
the
feat/enable/svg
branch
2 times, most recently
from
ab857f0 to
0d4293c
Compare
|
Documentation. Coverage Reports: |
chintankavathia
force-pushed
the
feat/enable/svg
branch
4 times, most recently
from
8ad19e0 to
88fb396
Compare
chintankavathia
force-pushed
the
feat/enable/svg
branch
from
88fb396 to
4ee6527
Compare
spike-rabbit
left a comment
spike-rabbit
left a comment
There was a problem hiding this comment.
Lets wait if we can have the icons as CSS masks.
projects/element-ng/chat-messages/si-chat-input.component.spec.ts
Outdated
Show resolved
Hide resolved
BREAKING CHANGE: icons are now rendered as SVG by default instead of icon fonts.
Applications that rely on icon fonts must explicitly opt out by using:
```ts
provideIconConfig({ disableSvgIcons: true })
```
chintankavathia
force-pushed
the
feat/enable/svg
branch
from
4ee6527 to
f311a2b
Compare
|
|
3 participants
BREAKING CHANGE: icons are now rendered as SVG by default instead of icon fonts.
Applications that rely on icon fonts must explicitly opt out by using: