[Rt-commit] rt branch, 3.9-trunk, updated. rt-3.9.7-992-g438a9f3

Thomas Sibley trs at bestpractical.com
Tue Dec 21 18:16:24 EST 2010


The branch, 3.9-trunk has been updated
       via  438a9f30d49ff6f56abb013b228f9e5dc77c6989 (commit)
       via  4024f896144612e44851a470ddcac3ccec0c9eca (commit)
       via  9f386ab048dd55118a7d23475170fee34afde066 (commit)
       via  1aff7c68dd88c7f6c2904c5c09e4c0e7a2bba066 (commit)
      from  5651f1b83741816c4fecf6863080ebe036a618f8 (commit)

Summary of changes:
 lib/RT/Interface/Web.pm             |   66 +++++++++++++++++++++++++++++++++++
 sbin/rt-test-dependencies.in        |    1 +
 share/html/Admin/Global/Theme.html  |    4 ++
 share/html/Admin/Tools/Queries.html |    2 +-
 share/html/Elements/ScrubHTML       |   27 +--------------
 t/web/scrub.t                       |   46 ++++++++++++++++++++++++
 6 files changed, 119 insertions(+), 27 deletions(-)
 create mode 100644 t/web/scrub.t

- Log -----------------------------------------------------------------
commit 1aff7c68dd88c7f6c2904c5c09e4c0e7a2bba066
Author: Thomas Sibley <trs at bestpractical.com>
Date:   Tue Dec 21 12:18:40 2010 -0500

    Add a missing period

diff --git a/share/html/Admin/Tools/Queries.html b/share/html/Admin/Tools/Queries.html
index 4d16fd3..a01bbaf 100644
--- a/share/html/Admin/Tools/Queries.html
+++ b/share/html/Admin/Tools/Queries.html
@@ -48,7 +48,7 @@
 <%init>
 my $title = loc('SQL Queries');
 unless ($session{'CurrentUser'}->HasRight( Object=> $RT::System, Right => 'SuperUser')) {
- Abort(loc('This feature is only available to system administrators'));
+ Abort(loc('This feature is only available to system administrators.'));
 }
 </%init>
 <& /Admin/Elements/Header, Title => $title &>

commit 9f386ab048dd55118a7d23475170fee34afde066
Author: Thomas Sibley <trs at bestpractical.com>
Date:   Tue Dec 21 12:23:26 2010 -0500

    Lock down the theme editor to SuperUsers only

diff --git a/share/html/Admin/Global/Theme.html b/share/html/Admin/Global/Theme.html
index 160be19..ce50d2c 100644
--- a/share/html/Admin/Global/Theme.html
+++ b/share/html/Admin/Global/Theme.html
@@ -183,6 +183,10 @@ jQuery(function($) {
 });
 </script>
 <%INIT>
+unless ($session{'CurrentUser'}->HasRight( Object=> RT->System, Right => 'SuperUser')) {
+    Abort(loc('This feature is only available to system administrators.'));
+}
+
 require JSON;
 
 my $text_threshold = 0.6;

commit 4024f896144612e44851a470ddcac3ccec0c9eca
Author: Thomas Sibley <trs at bestpractical.com>
Date:   Tue Dec 21 17:59:37 2010 -0500

    Move the meat of ScrubHTML into RT::Interface::Web::ScrubHTML

diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 6774779..694b40c 100755
--- a/lib/RT/Interface/Web.pm
+++ b/lib/RT/Interface/Web.pm
@@ -2541,6 +2541,56 @@ sub _parse_saved_search {
     return ( _load_container_object( $obj_type, $obj_id ), $search_id );
 }
 
+=head2 ScrubHTML content
+
+Removes unsafe and undesired HTML from the passed content
+
+=cut
+
+my $SCRUBBER;
+sub ScrubHTML {
+    my $Content = shift;
+    $SCRUBBER = _NewScrubber() unless $SCRUBBER;
+
+    $Content = '' if !defined($Content);
+    return $SCRUBBER->scrub($Content);
+}
+
+=head2 _NewScrubber
+
+Returns a new L<HTML::Scrubber> object.  Override this if you insist on
+letting more HTML through.
+
+=cut
+
+sub _NewScrubber {
+    require HTML::Scrubber;
+    my $scrubber = HTML::Scrubber->new();
+    $scrubber->default(
+        0,
+        {
+            '*'    => 0,
+            id     => 1,
+            class  => 1,
+            # Match http, ftp and relative urls
+            # XXX: we also scrub format strings with this module then allow simple config options
+            href   => qr{^(?:http:|ftp:|https:|/|__Web(?:Path|BaseURL|URL)__)}i,
+            face   => 1,
+            size   => 1,
+            target => 1,
+            style  => qr{^(?:(?:color:\s*rgb\(\d+,\s*\d+,\s*\d+\))|
+                             (?:text-align:\s*))}ix,
+        }
+    );
+    $scrubber->deny(qw[*]);
+    $scrubber->allow(
+        qw[A B U P BR I HR BR SMALL EM FONT SPAN STRONG SUB SUP STRIKE H1 H2 H3 H4 H5 H6 DIV UL OL LI DL DT DD PRE]
+    );
+    $scrubber->comment(0);
+
+    return $scrubber;
+}
+
 RT::Base->_ImportOverlays();
 
 1;
diff --git a/share/html/Elements/ScrubHTML b/share/html/Elements/ScrubHTML
index 662309e..47d4554 100644
--- a/share/html/Elements/ScrubHTML
+++ b/share/html/Elements/ScrubHTML
@@ -45,33 +45,8 @@
 %# those contributions and any derivatives thereof.
 %#
 %# END BPS TAGGED BLOCK }}}
-<%ONCE>
-my $scrubber = HTML::Scrubber->new();
-$scrubber->default(
-    0,
-    {
-        '*'    => 0,
-        id     => 1,
-        class  => 1,
-        # Match http, ftp and relative urls
-        # XXX: we also scrub format strings with this module then allow simple config options
-        href   => qr{^(?:http:|ftp:|https:|/|__Web(?:Path|BaseURL|URL)__)}i,
-        face   => 1,
-        size   => 1,
-        target => 1,
-        style  => qr{^(?:(?:color:\s*rgb\(\d+,\s*\d+,\s*\d+\))|
-                         (?:text-align:\s*))}ix,
-    }
-);
-$scrubber->deny(qw[*]);
-$scrubber->allow(
-    qw[A B U P BR I HR BR SMALL EM FONT SPAN STRONG SUB SUP STRIKE H1 H2 H3 H4 H5 H6 DIV UL OL LI DL DT DD PRE]
-);
-$scrubber->comment(0);
-</%ONCE>
 <%init>
-$Content = '' if !defined($Content);
-return $scrubber->scrub($Content);
+return ScrubHTML($Content);
 </%init>
 <%args>
 $Content => undef

commit 438a9f30d49ff6f56abb013b228f9e5dc77c6989
Author: Thomas Sibley <trs at bestpractical.com>
Date:   Tue Dec 21 18:05:46 2010 -0500

    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.

diff --git a/lib/RT/Interface/Web.pm b/lib/RT/Interface/Web.pm
index 694b40c..7292780 100755
--- 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
index 506ecd3..3b3b1c7 100755
--- 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
new file mode 100644
index 0000000..6483a75
--- /dev/null
+++ 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);
+}
+

-----------------------------------------------------------------------


More information about the Rt-commit mailing list