-
Notifications
You must be signed in to change notification settings - Fork 29k
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
Switch to dompurify for sanitizing markdown content #131950
Conversation
Assigning: @jrieken For trusted types |
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.
lgtm
return removeEmbeddedSVGs(raw); | ||
const x = sanitize(raw); | ||
console.log(raw); | ||
console.log(x); |
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.
console.log
e39ca04
to
053257e
Compare
@jrieken Any idea on why I now get the eslint error in
The code looks like:
Where We were previously assigning to |
@mjbvz fd122aa isn't the best way to tackle this. A special sequence of casts is needed here... |
Switches us from using `insane` to instead use `dompurify`, which seems to be better maintained and also has some nice features, such as built-in trusted types support I've tried to port over our existing sanitizer settings as best as possible, but there's not always a 1:1 mapping between how insane works and how dompurify does. I'd like to get this change in early in the iteration to catch potential regressions
innerHTML can return different results on different browsers. Use `isEqualNode` instead
a6cc048
to
53de038
Compare
052818f
to
f8bd73f
Compare
I beleive this is required since we allow links to commands and loading images over remote
Fixes #40607 This change introduces a new `supportsHtml` property on `MarkdownString` that can be used to enable rendering of a safe subset of tags and attributes inside of markdown strings For backwards compatability, `supportsHtml` will default to false and must be explicitly enabled by extensions This PR will need to go in after we adopt dompurify (#131950) which should provide better control over how we actually go about sanitizing rendered html
Thanks everyone! Merging in to get testing in insiders to catch potential regressions |
Fixes #40607 This change introduces a new `supportsHtml` property on `MarkdownString` that can be used to enable rendering of a safe subset of tags and attributes inside of markdown strings For backwards compatibility, `supportsHtml` will default to false and must be explicitly enabled by extensions This PR will need to go in after we adopt dompurify (#131950) which should provide better control over how we actually go about sanitizing rendered html
Fixes #40607 This change introduces a new `supportsHtml` property on `MarkdownString` that can be used to enable rendering of a safe subset of tags and attributes inside of markdown strings For backwards compatibility, `supportsHtml` will default to false and must be explicitly enabled by extensions This PR will need to go in after we adopt dompurify (#131950) which should provide better control over how we actually go about sanitizing rendered html
Switches us from using
insane
to instead usedompurify
, which seems to be better maintained and also has some nice features, such as built-in trusted types supportI've tried to port over our existing sanitizer settings as best as possible, but there's not always a 1:1 mapping between how insane works and how dompurify does. I'd like to get this change in early in the iteration to catch potential regressions