-
Notifications
You must be signed in to change notification settings - Fork 979
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[angular-gen2]: embed and custom code block and text block angular sanitizing html warning #3809
fix[angular-gen2]: embed and custom code block and text block angular sanitizing html warning #3809
Conversation
🦋 Changeset detectedLatest commit: 076db35 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
View your CI Pipeline Execution ↗ for commit 076db35.
☁️ Nx Cloud last updated this comment at |
Have these angular tests been failing sporadically before this PR? |
@samijaber no these are related to changes I've made here. Turns out the text component is triggering |
if (variableName) { | ||
code = code.replace( | ||
`[innerHTML]="${variableName}"`, | ||
`[innerHTML]="sanitizer.bypassSecurityTrustHtml(${variableName})"` |
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.
@samijaber this was needed for Embed and Custom Code block as well so I extended this PR to fix them too
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.
long-term I think this should be baked into mitosis: the angular generator should always use sanitizer.bypassSecurityTrustHtml
for innerHTML
(maybe with a flag to force-disable the sanitizer if needed)
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.
great idea, i've created a ticket for it
packages/sdks/mitosis.config.cjs
Outdated
code: { | ||
post: (code) => { | ||
if ( | ||
code.includes('selector: "builder-text"') || | ||
code.includes('selector: "builder-embed"') || | ||
code.includes('selector: "custom-code"') | ||
) { |
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.
We now pass json
as the 2nd arg for cost
plugins: https://github.com/BuilderIO/mitosis/blob/f3fabea5d912662e5e6454fd0f9088cb98c72f65/packages/core/src/modules/plugins.ts#L44-L84
This lets you do cleaner checks, like ['Text', 'Embed', 'CustomCode'].includes(json.name)
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.
done!
if (variableName) { | ||
code = code.replace( | ||
`[innerHTML]="${variableName}"`, | ||
`[innerHTML]="sanitizer.bypassSecurityTrustHtml(${variableName})"` |
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.
long-term I think this should be baked into mitosis: the angular generator should always use sanitizer.bypassSecurityTrustHtml
for innerHTML
(maybe with a flag to force-disable the sanitizer if needed)
Description
Uses https://angular.dev/api/platform-browser/DomSanitizer to mark text as safe html to use inside innerHTML. As text is already sanitized from the HTML editor we should be able to bypass it safely. This only shows up in the dev server and we don't receive these warnings in a prod build.
Fixes Embed and Custom Code block wrapping them inside
sanitizer.bypassSecurityTrustHtml
so that they allow embedding of iframeFixes #3793
Jira
https://builder-io.atlassian.net/browse/ENG-7950
https://builder-io.atlassian.net/browse/ENG-7901
Screenshot
Before
After