Extra <body> node and ignored <script> with hook uponSanitizeElement #199

jfparadis · 2017-02-21T11:29:54Z

Two issues with hook uponSanitizeElement:

Calling sanitize causes one extra callback, aways with extra node.
Example: <div><script>alert(0)</script></div>
Hook called 4 times: BODY, DIV, SCRIPT, text
When sanitizing string containing only <script> tag, there is no callback for script.
Example: <script>alert(0)</script>
Hook called 1 time: BODY

Please see this: JSFiddle.

The text was updated successfully, but these errors were encountered:

cure53 · 2017-02-21T11:46:26Z

Ah, that's the old problem with the browser moving SCRIPT, STYLE and others into the HEAD element when not prepended by any other element. This is causing trouble in many situations. Technically not our fault but I feel more and more we should fix this.

How do you feel about a FORCE_BODY configuration flag that simply makes sure that nothing goes into the HEAD? We could default it to false right now for 0.8.x and set it to true starting with 0.9.x.

jfparadis · 2017-02-21T12:21:49Z

Very interesting.

Question: if <script> gets moved to <head>, why uponSanitizeElement is not called with <head>, then with the <script>?

cure53 · 2017-02-21T13:30:33Z

We use the doc.body as the platform to check and sanitize the markup :) That's why it simply disappears.

The idea is now to pre-fill the body with a nonsense element, then initialize the document, then remove the nonsense element again - all behind the suggested FORCE_BODY flag. Then your bug should disappear :)

cure53 · 2017-02-23T19:36:16Z

Would the proposed fix work for you?

jfparadis · 2017-02-23T20:50:13Z

Yes, FORCE_BODY would work. There is another approach:

If I create a simple test.html with the following content:

<script src="hello.js"></script>

And if I load it in the browser and look at the DOM, I obtain:

<html>
    <head>
        <script src="hello.js"></script>
    </head>
    <body>
    </body>
</html>

Because the callback starts at doc.body, we get the current behavior (the head is ignored).

Now, if I create a simple test2.html with the following content:

<html>
    <head>
        <script src="hello.js"></script>
    </head>
    <body>
    </body>
</html>

I get the same behavior, the <head> is also ignored ( you can see the result on my updated JSFiddle, the head is also ignored).

An alternative approach would be to call doc.head followed by doc.body. The advantage would be to always process head markup and body markup, and to mimic more closely the browser behavior.

cure53 · 2017-02-24T08:56:14Z

We thought about that initially but I think the changes would be too disruptive. I'll see that I get an implementation of FORCE_BODY to work asap.

Added two tests for FORCE_BODY Added description to README.md

cure53 · 2017-02-24T11:13:34Z

@jfparadis Check out the latest commits in the master branch. This should eliminate the problem. tests are green as well.

Changed the FORCE_BODY inject to <remove></remove> Added additional test coverage

jfparadis · 2017-02-26T13:38:50Z

A few notes:

This solution makes perfect sense: as you say in the description, the current behavior was counter-intuitive. This changes solves the normal case.
And if someone absolutely need to parse the whole document, this is covered by {WHOLE_DOCUMENT: true}.
I've updated the JSFiddle with the latest code, and it works as specified.
I the JSFiddle, when sending a complete document with the new option (third test), we don't get HTML, HEAD, or a duplicate BODY. That's what one should expect.

Well done!

cure53 · 2017-02-26T14:48:57Z

Thanks, my pleasure :)

cure53 added a commit that referenced this issue Feb 24, 2017

See #199

0dc3fb8

Added two tests for FORCE_BODY Added description to README.md

cure53 added a commit that referenced this issue Feb 24, 2017

See #199

71f44d4

Changed the FORCE_BODY inject to <remove></remove> Added additional test coverage

jfparadis closed this as completed Feb 26, 2017

jfparadis mentioned this issue May 14, 2017

Option to set config once #213

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extra <body> node and ignored <script> with hook uponSanitizeElement #199

Extra <body> node and ignored <script> with hook uponSanitizeElement #199

jfparadis commented Feb 21, 2017

cure53 commented Feb 21, 2017

jfparadis commented Feb 21, 2017 •

edited

Loading

cure53 commented Feb 21, 2017 •

edited

Loading

cure53 commented Feb 23, 2017

jfparadis commented Feb 23, 2017 •

edited

Loading

cure53 commented Feb 24, 2017

cure53 commented Feb 24, 2017

jfparadis commented Feb 26, 2017

cure53 commented Feb 26, 2017

Extra <body> node and ignored <script> with hook uponSanitizeElement #199

Extra <body> node and ignored <script> with hook uponSanitizeElement #199

Comments

jfparadis commented Feb 21, 2017

cure53 commented Feb 21, 2017

jfparadis commented Feb 21, 2017 • edited Loading

cure53 commented Feb 21, 2017 • edited Loading

cure53 commented Feb 23, 2017

jfparadis commented Feb 23, 2017 • edited Loading

cure53 commented Feb 24, 2017

cure53 commented Feb 24, 2017

jfparadis commented Feb 26, 2017

cure53 commented Feb 26, 2017

jfparadis commented Feb 21, 2017 •

edited

Loading

cure53 commented Feb 21, 2017 •

edited

Loading

jfparadis commented Feb 23, 2017 •

edited

Loading