XSS via text format names

Here we go.

There are many occurences though and i am thinking about putting check_plain directly to filter_formats() function, during the load phase. Not sure if that will break anything. Opinion?

Ah, I see a problem in the patch format, the previous one will fail testing. Reroll.

+++ modules/filter/filter.module	24 Apr 2010 09:17:25 -0000
@@ -999,11 +999,11 @@ function _filter_tips($format_id, $long 
-    $tips[$format->name] = array();
+    $tips[check_plain($format->name)] = array();
...
-        $tips[$format->name][$name] = array('tip' => $tip, 'id' => $name);
+        $tips[check_plain($format->name)][$name] = array('tip' => $tip, 'id' => $name);

Why those changes?

Powered by Dreditor.

Latest patch in #635212: Fallback format is not configurable contains all necessary/proper security fixes for this issue, which need to be moved over here.

The remaining changes of that patch. No idea whether they are still valid.

Unless someone confirms that #7 is required, demoting to normal.

There is no need for this patch in D8, at least as of current dev version.

Simple patch adding check_plain() to the filter module. Still applies nicely. Why isn't this in Core yet?

I'm unable to reproduce the XSS on node/add/article. The form API already does check_plain().

But I can reproduce it on /filter/tips which is linked on the node creation form as "More information about text formats".

So I would opt that only the change to filter.pages.inc is needed.

This was marked as needing tests in #9. I don't see any existing tests for the filter/tips page. Personally, I would just add onto the end of testTextFormatCRUD() rather than creating a whole new test class at this stage of the 7.x lifecycle, but others might feel differently. Someone should be able to just copy the first 5 lines of the function, change the title to include a javascript alert, drupalGet() the tips page, and make sure it's encoded. I'm tagging this as a Novice issue for the addition of the test.

As for the fix in #12, it looks good to me. I'd RTBC it if it weren't for the test. Once applied, the filter names are encoded on filter/tips.

I think it's correct to remove the additional changes to _filter_tips() that were in #3 (sun had a problem with them as mentioned in #4). _filter_tips() is only called in three places, each of them inside a call to theme_filter_tips(). If the filter names were run through check_plain() in _filter_tips() and in theme_filter_tips(), they would end up double-encoded.

Added the test case. Please review now.

forgot to add interdiff.

Thanks for your hard work @joshi.rohit100. =) I have a few notes about the test.

Sorry, I know I wasn't clear, but I meant that I would literally append the new assertion into the end of testTextFormatCRUD() rather than adding a new method or class with method. That way we would avoid forcing Simpletest to create yet another new environment for this one, simple assertion.

+++ b/modules/filter/filter.test
@@ -73,6 +73,19 @@ class FilterCRUDTestCase extends DrupalWebTestCase {
+    // Add a text format with minimum data only.

Also, I know I said you could "copy the first five lines," but this comment should be changed and made relevant to the new assertion.

Finally, could you make a tests-only patch, please? It's good to know for certain that the test is going to fail without the fix.

thanks @dcam for pointing in right direction. Here is the test only patch.

Ok, this is a great demonstration of why it's good to include a tests-only patch. The test passed, even though we know from manual testing that there is a bug. Now we know that something is wrong with the test.

The test had a permission problem, it could not see the filter format as the anonymous test had no access to it.

I've now adapted it to give anonymous permission to it.

Yay, the tests only patch failed... now the full patch.

@dcam: Can you agree with the test like this?

Thank you, @joshi.rohit100 and @helmo. You both did great work on this. RTBC.

Committed to 7.x - thanks!

(And it looks like this code is in Drupal 8 too, but the XSS issue there is fixed by Twig auto-escape.)