fix hackish message escaping [SF#757128]

Author: Richard Jones

CHANGES.txt

@@ -6,6 +6,7 @@ are given with the most recent entry first.
 - handle deprecation of FCNTL in python2.2+ (sf bug 756756)
 - handle missing Subject: line (sf bug 755331)
 - handle New User creation (sf bug 754510)
+- fix hackish message escaping (sf bug 757128)
 
 
 2003-06-10 0.6.0b3

roundup/cgi/client.py

@@ -1,4 +1,4 @@
-# $Id: client.py,v 1.119 2003-06-10 22:55:30 richard Exp $
+# $Id: client.py,v 1.120 2003-06-24 03:30:30 richard Exp $
 
 __doc__ = """
 WWW request handler (also used in the stand-alone server).
@@ -68,10 +68,16 @@ def initialiseSecurity(security):
         description="User may manipulate user Roles through the web")
     security.addPermissionToRole('Admin', p)
 
-def clean_message(match, ok={'a':1,'i':1,'b':1,'br':1}):
+# used to clean messages passed through CGI variables - HTML-escape any tag
+# that isn't <a href="">, <i>, <b> and <br> (including XHTML variants) so
+# that people can't pass through nasties like <script>, <iframe>, ...
+CLEAN_MESSAGE_RE = r'(<(/?(.*?)(\s*href="[^"]")?\s*/?)>)'
+def clean_message(message, mc=re.compile(CLEAN_MESSAGE_RE, re.I)):
+    return mc.sub(clean_message_callback, message)
+def clean_message_callback(match, ok={'a':1,'i':1,'b':1,'br':1}):
     ''' Strip all non <a>,<i>,<b> and <br> tags from a string
     '''
-    if ok.has_key(match.group(2)):
+    if ok.has_key(match.group(3).lower()):
         return match.group(1)
     return '&lt;%s&gt;'%match.group(2)
 
@@ -348,8 +354,7 @@ def determine_user(self):
         # reopen the database as the correct user
         self.opendb(self.user)
 
-    def determine_context(self, dre=re.compile(r'([^\d]+)(\d+)'),
-            mc=re.compile(r'(</?(.*?)>)')):
+    def determine_context(self, dre=re.compile(r'([^\d]+)(\d+)')):
         ''' Determine the context of this page from the URL:
 
             The URL path after the instance identifier is examined. The path
@@ -397,10 +402,10 @@ def determine_context(self, dre=re.compile(r'([^\d]+)(\d+)'),
                 template_override = self.form[key].value
             elif self.FV_OK_MESSAGE.match(key):
                 ok_message = self.form[key].value
-                ok_message = mc.sub(clean_message, ok_message)
+                ok_message = clean_message(ok_message)
             elif self.FV_ERROR_MESSAGE.match(key):
                 error_message = self.form[key].value
-                error_message = mc.sub(clean_message, error_message)
+                error_message = clean_message(error_message)
 
         # determine the classname and possibly nodeid
         path = self.path.split('/')

test/test_cgi.py

@@ -8,7 +8,7 @@
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
 #
-# $Id: test_cgi.py,v 1.16 2003-05-09 01:47:50 richard Exp $
+# $Id: test_cgi.py,v 1.17 2003-06-24 03:30:40 richard Exp $
 
 import unittest, os, shutil, errno, sys, difflib, cgi, re
 
@@ -37,6 +37,26 @@ class config:
     TRACKER_NAME = 'testing testing'
     TRACKER_WEB = 'http://testing.testing/'
 
+cm = client.clean_message
+class MessageTestCase(unittest.TestCase):
+    def testCleanMessageOK(self):
+        self.assertEqual(cm('<br>x<br />'), '<br>x<br />')
+        self.assertEqual(cm('<i>x</i>'), '<i>x</i>')
+        self.assertEqual(cm('<b>x</b>'), '<b>x</b>')
+        self.assertEqual(cm('<a href="y">x</a>'),
+            '<a href="y">x</a>')
+        self.assertEqual(cm('<BR>x<BR />'), '<BR>x<BR />')
+        self.assertEqual(cm('<I>x</I>'), '<I>x</I>')
+        self.assertEqual(cm('<B>x</B>'), '<B>x</B>')
+        self.assertEqual(cm('<A HREF="y">x</A>'),
+            '<A HREF="y">x</A>')
+
+    def testCleanMessageBAD(self):
+        self.assertEqual(cm('<script>x</script>'),
+            '&lt;script&gt;x&lt;/script&gt;')
+        self.assertEqual(cm('<iframe>x</iframe>'),
+            '&lt;iframe&gt;x&lt;/iframe&gt;')
+
 class FormTestCase(unittest.TestCase):
     def setUp(self):
         self.dirname = '_test_cgi_form'
@@ -502,7 +522,9 @@ def testBackwardsCompat(self):
             [('issue', None, 'files', [('file', '-1')])]))
 
 def suite():
-    l = [unittest.makeSuite(FormTestCase),
+    l = [
+        unittest.makeSuite(FormTestCase),
+        unittest.makeSuite(MessageTestCase),
     ]
     return unittest.TestSuite(l)