Overhaul what CSS we allow in style attributes to be safer *and* more…

… useful The regex takes care to accomodate Outlook's idiosyncrasies and ensure that the features of the rich text editor work. This is still a large hammer. If there's any forbidden CSS property specified the entire style attribute will be stripped (including otherwise allowed properties). The fix for this is parsing the CSS itself with something like CSS::Adaptor::Whitelist, but that's for another day.
bestpractical · Dec 21, 2010 · 438a9f3 · 438a9f3
1 parent 4024f89
commit 438a9f3
Show file tree

Hide file tree

Showing 3 changed files with 65 additions and 2 deletions.
diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
@@ -2578,8 +2578,24 @@ sub _NewScrubber {
             face   => 1,
             size   => 1,
             target => 1,
-            style  => qr{^(?:(?:color:\s*rgb\(\d+,\s*\d+,\s*\d+\))|
-                             (?:text-align:\s*))}ix,
+            style  => qr{
+                ^(?:\s*
+                    (?:(?:background-)?color: \s*
+                            (?:rgb\(\s* \d+, \s* \d+, \s* \d+ \s*\) |   # rgb(d,d,d)
+                               \#[a-f0-9]{3,6}                      |   # #fff or #ffffff
+                               [\w\-]+                                  # green, light-blue, etc.
+                               )                            |
+                       text-align: \s* \w+                  |
+                       font-size: \s* [\w.\-]+              |
+                       font-family: \s* [\w\s"',.\-]+       |
+                       font-weight: \s* [\w\-]+             |
+
+                       # MS Office styles, which are probably fine.  If we don't, then any
+                       # associated styles in the same attribute get stripped.
+                       mso-[\w\-]+?: \s* [\w\s"',.\-]+
+                    )\s* ;? \s*)
+                 +$ # one or more of these allowed properties from here 'till sunset
+            }ix,
         }
     );
     $scrubber->deny(qw[*]);

diff --git a/sbin/rt-test-dependencies.in b/sbin/rt-test-dependencies.in
@@ -279,6 +279,7 @@ Test::MockTime
 Log::Dispatch::Perl
 Test::WWW::Mechanize::PSGI
 Plack::Middleware::Test::StashWarnings
+Test::LongString
 .
 
 $deps{'FASTCGI'} = [ text_to_hash( << '.') ];

diff --git a/t/web/scrub.t b/t/web/scrub.t
@@ -0,0 +1,46 @@
+#!/usr/bin/perl
+use strict;
+use warnings;
+
+use RT::Test nodb => 1, tests => 6;
+use RT::Interface::Web; # This gets us HTML::Mason::Commands
+use Test::LongString;
+
+{
+    my $html = 'This is a test of <span style="color: rgb(255, 0, 0); ">color</span> and <span style="font-size: 18px; "><span style="font-family: Georgia, serif; ">font</span></span> and <em><u><strike><strong>boldness</strong></strike></u></em>.';
+    is_string(scrub_html($html), $html, "CKEditor produced HTML sails through");
+}
+
+{
+    my $html = '<p style="text-align: right; ">
+        And <span style="color: rgb(255, 0, 0); "><span style="font-size: 16px; "><span style="font-family: Georgia, serif; ">alignment with color</span></span></span>?</p>';
+    is_string(scrub_html($html), $html, "CKEditor produced HTML sails through");
+}
+
+{
+    my $html = 'This is a test of <span style="color: rgb(255, 0, 0); content: url(/Nasty/URL);">color</span> and <span style="font-size: 18px; "><span style="font-family: Georgia, serif; ">font</span></span> and <em><u><strike><strong>boldness</strong></strike></u></em>.';
+    my $expected = 'This is a test of <span>color</span> and <span style="font-size: 18px; "><span style="font-family: Georgia, serif; ">font</span></span> and <em><u><strike><strong>boldness</strong></strike></u></em>.';
+    is_string(scrub_html($html), $expected, "nasty CSS not allowed through");
+}
+
+{
+    my $html = 'Let\'s add some <span style="color: blue; font-family: Georgia">color</span> up in <span style="color: #DEADBE">here</span>.';
+    is_string(scrub_html($html), $html, "multiple props and color specs allowed");
+}
+
+{
+    my $html = q[<span lang=EN-US style='font-family:"Century Gothic","sans-serif";'>oh hai I'm some text</span>];
+    my $expected = q[<span style="font-family:&quot;Century Gothic&quot;,&quot;sans-serif&quot;;">oh hai I'm some text</span>];
+    is_string(scrub_html($html), $expected, "font lists");
+}
+
+{
+    my $html = q[<span lang=EN-US style='font-size:7.5pt;font-family:"Century Gothic","sans-serif";color:#666666;mso-fareast-language:IT'>oh hai I'm some text</span>];
+    my $expected = q[<span style="font-size:7.5pt;font-family:&quot;Century Gothic&quot;,&quot;sans-serif&quot;;color:#666666;mso-fareast-language:IT">oh hai I'm some text</span>];
+    is_string(scrub_html($html), $expected, "outlook html");
+}
+
+sub scrub_html {
+    return HTML::Mason::Commands::ScrubHTML(shift);
+}
+