Implement _fragmentParser
#1
Conversation
0xedward
changed the title
0xedward
changed the title
_fragmentParser
src/sanitizer.js
Outdated
| // TODO implement this function and remove eslint-disable comment above | ||
| /** | ||
| * fragmentParser | ||
| * a parser that doesn't run scripts or loads resources |
There was a problem hiding this comment.
ouch. I just tested this. That's not actually true 🤣 😭
This is bad, but it doesn't justify throwing all of your other useful code changes away. Can you change that comment and file a follow-up? I have some ideas, that I'll need to verify and will get into that issue once it is filed.
There was a problem hiding this comment.
ouch. I just tested this. That's not actually true 🤣 😭
Oh noo, that's right... Range.createContextualFragment will execute scripts 😢
I guess we are 11 years too late to take advantage of this bug 🙃
This is bad, but it doesn't justify throwing all of your other useful code changes away. Can you change that comment and file a follow-up?
Sure! I'll filed an issue, updated the comment to remove "doesn't run scripts" and added a reference to the issue in code.
I have some ideas, that I'll need to verify and will get into that issue once it is filed.
I found an interesting old WebKit, WhatWG and Twitter conversations on this.
Also this post might be useful here - https://grrr.tech/posts/create-dom-node-from-html-string/
There was a problem hiding this comment.
After some research, I think we just have to use the same techniques DOMPurify is using: Use a new document, disregard the context :-(
An alternative would be to create a mock-context, i.e., adding the context Element to the mock-document and then parsing in there. That won't help when the stack of open elements contains more than 1 required element (would work for tr in table but maybe not for td in tr, because we're lacking the outer table). Not sure if that's worth for a polyfill, when DOMPurify has so far been almost untouched by this flaw?
There was a problem hiding this comment.
After some research, I think we just have to use the same techniques DOMPurify is using: Use a new document, disregard the context :-(
Not sure if that's worth for a polyfill, when DOMPurify has so far been almost untouched by this flaw?
Ah, should we abandon this PR and go back to shimming DOMPurify?
There was a problem hiding this comment.
Yeah, I'm afraid we have to...Sorry for this all...
There was a problem hiding this comment.
No problem at all! I learned a lot from going down this path 😄
I'll go back to your original plan and start working on it!
|
No problem! I'm glad we explored this option, since I learned a lot about |
mozilla#3
Will first merge and squash the commits into 0xedward/sanitizer-polyfill main branch, then make a separate PR to merge into mozilla/sanitizer-polyfill for a clean commit history