cure53 / jPurify

jPurify
Mozilla Public License 2.0
65 stars 11 forks source link

jQuery error when using $() factory #2

Closed cure53 closed 10 years ago

cure53 commented 10 years ago
$> $('<style>')

TypeError: second is null
http://cure53.de/jpurify/jquery.js
Line 425

We need to fix this for certain elements such as template, style and others.

filedescriptor commented 10 years ago

Elements that are affected, for reference: ["body", "caption", "col", "colgroup", "head", "html", "style", "tbody", "td", "template", "tfoot", "th", "thead", "tr", "title"]

(tested in latest Chrome & Firefox)

filedescriptor commented 10 years ago

Also, a disallowed element, e.g. $('<script>'), will cause an error. It should fail silently.

cure53 commented 10 years ago

I think I found a way to cleanly aproach this. Most of the elements above already work, still having trouble with table nodes though. WIP.

cure53 commented 10 years ago

While most elements can easily be handled, some cannot because of what browsers do. Look here for example:

doc=document.implementation.createHTMLDocument();
doc.body.innerHTML='1<td>2';
doc.body.outerHTML;

Now the question is, what do we do? I want to avoid crazy HTML stunts that are against what the browser does. But then we risk breaking apps that do crazy stuff like generating single table cells using $('<td>') and alike. What do you think?

cure53 commented 10 years ago

My suggestion to tackle this would be: No extra implementation for weird element factory usage. If we start wrapping tags we might make the same mistake as jQuery and risk being vulnerable.

What I can imagine though is the following: Since I know that people like to use the factory for tables, and we cannot do $('<td>') we might want to use a pre-test. If the string to sanitize only contains al-num or looks like this ^<\w+>$ we might allow it. Sounds reasonable?

filedescriptor commented 10 years ago

Not sure if it would bring some bypasses, but we might take it as a temporary measure. We need others' opinions anyways.

cure53 commented 10 years ago

I agree. I would say the bypass potential is low - but let's see. I am mostly scared of chainings ike this:

$('<script>').attr('src','x').attr('async','true')
cure53 commented 10 years ago

My tests so far worked fine, closing this one for now.