Add filter_xss() functionality to drupal.js

Patch failed to apply. More information can be found at http://testing.drupal.org/node/14111. If you need help with creating patches please look at http://drupal.org/patch/create

Re-attaching with patch made from within the drupal directory this time, instead of within the misc directory.

This new JS filterXSS() function is intended for use by JavaScript files in Drupal which handle user input directly. To use it just do:

Cheers,
Stella

I tested it out:
document.write(Drupal.filterXSS('<script>alert("xss!");'));
and this came out:

alert("xss!");

Looks good, and very useful IMHO.

Robin

This patch is committed :

#197425: Usability: enable BLOCKQUOTE by default

Updated. Thanks.

Cheers,
Stella

Doesn't seem to be working properly. I tried this:
document.write(Drupal.filterXSS("<strong> <script>alert('xss');</scr"+"ipt> foo </strong>"));
It wrongly removed the <strong>, and failed to remove the <script>.

Need pass this tests : http://ha.ckers.org/xss.html

Yup, there was a big error in the last patch but I've fixed that and re-rolled the patch again. I've tested it with John's example above, along with most of the ones from the link that lilou provided.

lilou: thanks for that link, it was very helpful!

Cheers,
Stella

@stella : you can test all in one ...

I can imagine why this might be useful, but it's a fair bit of code and we won't be using it yet in core.

Can you expand a bit on the use cases? Why is this needed? When is checkPlain not appropriate? How widespread is the need? (Without I guess naming specific modules that should be doing this but aren't ;)).

I know of a couple of modules (which shall remain nameless) who need this functionality. These modules have javascript which take input from the user (trusted and/or untrusted users), do something with it and then display it. This user-entered input can validly contain html tags, and this is in fact a feature. Therefore using Drupal.checkPlain() is not an option for these modules as it would break the functionality.

I've tested all of the various XSS strings in the file lilou provided and as far as I can see all tests passed. I'd appreciate it if someone else could also verify this.

Cheers,
Stella

@stella - these modules (why should they remain nameless?) take untrusted input and show them to different users? I doubt it, because then you could have run filter_xss() on the server side. I don't yet see the point in filtering a user's own input when shown to self?

I don't want to name them here because I think they're vulnerable. I've been talking to Heine about this issue, but have only just realised that no mail was sent to the security team list, so I've sent one now.

To answer your question, these modules take untrusted input from node body field and displays it to any users who can view the same node, so it's not just shown to self. See my mail to the security list for more details on how this can happen.

Cheers,
Stella

The last submitted patch failed testing.

Re-rolled patch against latest HEAD.

hmm... couldn't you do this using an AJAX request where ANY input format could be invoked?

filter_xss() is pretty limited because it allows only whitelisting HTML elements, and then it blindly allows any attributes in them, except on* and style, but you can still pass class or id, which can cause harm to the page layout, or otherwise unexpeted results. So, there maybe someone that needs a different kind of filter such as HTML Purifier, htmlLawed, WYSIWYG Filter, etc.

PS: when porting filter_xss(), please take this issue into account.

The last submitted patch failed testing.

failure was due to HEAD not installing properly

I think Markus has a valid point here:
why not implement it through an AJAX request and use the same functionality as on the server side?
This would provide the same results without duplicating any code.

Not bad, but it won't be that easy :) Should be simpler and cleaner using AJAX, but if you want it that way, here are some thoughts.

Firstly, it eats my tags:

both give just: uuu</em>. (Seems it only likes first tags: "a<em>b</em>c<em>d</em>" gives "ab</em>c<em>d</em>".)

Secondly, the "for (var character in replace) ..." loop is risky. According to par. 12.2, ECMA-357, order of properties is implementation dependent. Not sure if any implementation would really reorder that, but, if so, this would at least cause removing of Netscape 4.x JS entites ineffective.

Thirdly, entity decoding doesn't seem to take numerical character entities into account, so:

should work in IE6 (that is x0D before or inside a protocol). This is unconfirmed, because it eats my tags ;)

While trying to verify the above I've hit something like this:

which surprisingly does work (IE6, a script in an article using document.write). Not sure why. :)

Is there anywhere in core that would benefit from this?

Sorry for the delay in responding. I've completely reworked the patch based on markus_petrux's suggestion to use AJAX. It's now much simpler and easier, and since it uses Drupal's existing filter_xss(), it shouldn't have any of the limitations that existed in the previous patch.

It defines a system/filter-xss menu callback item, which calls filter_xss() on $_GET['string']. There's a short Drupal.filterXSS() javascript function which uses a synchronous ajax request to retrieve system/filter-xss. Personally I would prefer an asynchronous ajax request, but I don't see how that is possible.

I would also like to extend the patch so it can handle the second param to filter_xss(), which is an array of allowed tags, but haven't decided how best to implement this. So feedback much appreciated.

Cheers,
Stella

Being a GET request, it may fail depending on the size of the string to parse -vs- server configuration and/or hardcoded limitations.

See for example: http://httpd.apache.org/docs/2.0/mod/core.html#limitrequestline

Depending on the purpose of this API, it might be interesting to use filter_xss_admin() or check_markup(). For passing arguments, maybe using Drupal.settings?

In regards to synchronous/asynchronous dilema... if it was asynchronous, it could accept a callback which would be used to process the response, which probably be used to alter the DOM in some way, I guess. :)

Here's a new patch which uses POST instead of GET, and which has support for the optional "allowed_tags" param to filter_xss().

Within your javascript, you use it like:

or

If we were to use asynchronous, then rather than returning the safe string directly, we would need to alter the DOM and replace the unsafe string, or alter a particular div/span, etc, with the filtered string. This may not suit all use cases.

Cheers,
Stella

The last submitted patch failed testing.

Re-roll...

While using synchronous XMLHttpRequests (the A in AJAX means asynchronous, so this can no longer be named ‘AJAX’) facilitates the implementation of this function, it comes at a high cost: When the server or the user’s connection is slow, such a request COMPLETELY BLOCKS the browser. The user can’t do any action on the site and has to wait until the request finishes.

I don't think such a function is really necessary in client-side JavaScript. The reason for filtering text for XSS is that an attacker might foist code to another user, stealing their session or performing other malicious actions like forging requests. However, when the user enters text which is subsequently injected into the very same page, the user could only harm themselves. When the input is stored on the server, it has to be filtered anyway *on the server* before it is sent back to the user (or another user).

Drupal.checkPlain() is mainly used for being able to display literally what the user entered and less for security purposes.

See #586098: Add a core jQuery checkMarkup() function like check_markup() which could be more useful for input filtering.