fix: issue2551387 - TypeError: not indexable.

Fix crash due to uninitialized list element on a (Mini)FieldStorage
when unexpected input is posted via wsgi. This doesn't happen when
running roundup-server. It might happen under other front ends.

Moved the code that sets '.list = [] if .list == None' to the main
flow. Added an exception hander that logs the value of self.form if
self.form.list raises an AttributeError. This exception should never
happen if I understand the code correctly (but I probably don't).

Fixed a number of test cases that were broken because I was calling
Client and passing '[]' rather than a cgi.formStorage object.

Added test cases:

   create a FileStorage (self.form) with .list = None.

   check AttributeError exception and verify logging.

Problem reported and debugged by Christof Meerwald.

Author: rouilj

CHANGES.txt

@@ -67,6 +67,10 @@ Fixed:
   result, incorrectly formatted CORS preflight requests
   (e.g. missing Origin header) can now return HTTP status 403 as
   well as status 400. (John Rouillard)
+- issue2551387 - TypeError: not indexable. Fix crash due to
+  uninitialized list element on a (Mini)FieldStorage when unexpected
+  input is posted via wsgi. (Reported and debugged by Christof
+  Meerwald; fix John Rouillard)
 
 Features:
 

roundup/cgi/client.py

@@ -504,13 +504,26 @@ def __init__(self, instance, request, env, form=None, translator=None):
             # It's a workaround for a bug in cgi.FieldStorage. See class
             # def for details.
             self.form = BinaryFieldStorage(fp=request.rfile, environ=env)
-            # In some case (e.g. content-type application/xml), cgi
-            # will not parse anything. Fake a list property in this case
-            if self.form.list is None:
-                self.form.list = []
         else:
             self.form = form
 
+        # When the CONTENT-TYPE is not 'application/x-www-form-urlencoded':
+        # or multipart/*, cgi.(Mini)FieldStorage sets the list property to
+        # None. Initialize an empty list property in this case so we can
+        # query the list in all cases.
+        try:
+            if (self.form.list is None):
+                self.form.list = []
+        except AttributeError:
+            # self.form should always be some type of
+            # FieldStorage. If we get an AttributeError,
+            # print what the form is.
+            # FIXME: plan on removing this in 2028 to improve
+            # performance if there are no reports of it being triggered.
+            logger.error(("Invalid self.form found (please report "
+                         "to the roundup-users mailing list): %s") % self.form)
+            raise
+
         # turn debugging on/off
         try:
             self.debug = int(env.get("ROUNDUP_DEBUG", 0))

test/db_test_base.py

@@ -30,6 +30,14 @@
 from email import message_from_string
 
 import pytest
+
+try:
+    from StringIO import cStringIO as IOBuff
+except ImportError:
+    # python 3
+    from io import BytesIO as IOBuff
+
+from roundup.cgi.client import BinaryFieldStorage
 from roundup.hyperdb import String, Password, Link, Multilink, Date, \
     Interval, DatabaseError, Boolean, Number, Node, Integer
 from roundup.mailer import Mailer
@@ -43,7 +51,7 @@
 from roundup.exceptions import UsageError, Reject
 from roundup.mlink_expr import ExpressionError
 
-from roundup.anypy.strings import b2s, s2b, u2s
+from roundup.anypy.strings import b2s, bs2b, s2b, u2s
 from roundup.anypy.cmp_ import NoneAndDictComparable
 from roundup.anypy.email_ import message_from_bytes
 
@@ -4340,6 +4348,14 @@ def testHTMLItemDerefFail(self):
         ae(bool(issue ['priority']['name']),False)
 
 def makeForm(args):
+    """ Takes a dict of form elements or a FieldStorage.
+
+         FieldStorage is just returned so you can pass the result from
+         makeFormFromString through it cleanly.
+    """
+    if isinstance(args, cgi.FieldStorage):
+        return args
+
     form = cgi.FieldStorage()
     for k,v in args.items():
         if type(v) is type([]):
@@ -4352,6 +4368,33 @@ def makeForm(args):
             form.list.append(cgi.MiniFieldStorage(k, v))
     return form
 
+def makeFormFromString(bstring, env=None):
+    """used for generating a form that looks like a rest or xmlrpc
+       payload. Takes a binary or regular string and stores
+       it in a FieldStorage object.
+
+       :param: bstring - binary or regular string
+       :param" env - dict of environment variables. Names/keys
+               must be uppercase.
+                  e.g. {"REQUEST_METHOD": "DELETE"}
+
+       Ideally this would be part of makeForm, but the env dict
+       is needed here to allow FieldStorage to properly define
+       the resulting object.
+    """
+    rfile=IOBuff(bs2b(bstring))
+
+    # base headers required for BinaryFieldStorage/FieldStorage
+    e = {'REQUEST_METHOD':'POST',
+         'CONTENT_TYPE': 'text/xml',
+         'CONTENT_LENGTH': str(len(bs2b(bstring)))
+         }
+    if env:
+        e.update(env)
+
+    form = BinaryFieldStorage(fp=rfile, environ=e)
+    return form
+
 class FileUpload:
     def __init__(self, content, filename):
         self.content = content

test/rest_common.py

@@ -3,6 +3,7 @@
 import shutil
 import sys
 import errno
+import logging
 
 from time import sleep
 from datetime import datetime, timedelta
@@ -76,6 +77,10 @@ def dst(self, dt):
 
 class TestCase():
 
+    @pytest.fixture(autouse=True)
+    def inject_fixtures(self, caplog):
+        self._caplog = caplog
+
     backend = None
     url_pfx = 'http://tracker.example/cgi-bin/roundup.cgi/bugs/rest/data/'
 
@@ -291,7 +296,8 @@ def setUp(self):
             'HTTP_ORIGIN': 'http://tracker.example'
         }
         self.dummy_client = client.Client(self.instance, MockNull(),
-                                          self.client_env, [], None)
+                                          self.client_env,
+                                          cgi.FieldStorage(), None)
         self.dummy_client.request.headers.get = self.get_header
         self.dummy_client.db = self.db
 
@@ -2294,6 +2300,30 @@ def testdetermine_output_formatBadAccept(self):
                 self.assertNotIn("X-Content-Type-Options", 
                               self.server.client.additional_headers)
 
+    def testBadFormAttributeErrorException(self):
+        env = {
+            'PATH_INFO': 'rest/data/user',
+            'HTTP_HOST': 'localhost',
+            'TRACKER_NAME': 'rounduptest',
+            "REQUEST_METHOD": "GET"
+        }
+
+
+        with self._caplog.at_level(logging.ERROR, logger="roundup"):
+            with self.assertRaises(AttributeError) as exc:
+                self.dummy_client = client.Client(
+                    self.instance, MockNull(), env, [], None)
+
+        self.assertEqual(exc.exception.args[0],
+                         "'list' object has no attribute 'list'")
+
+        # log should look like (with string not broken into parts):
+        #    [('roundup', 40, 
+        #       'Invalid self.form found (please report to the '
+        #       'roundup-users mailing list): []')]
+        log = self._caplog.record_tuples[:]
+        self.assertIn("Invalid self.form found", log[0][2])
+
     def testDispatchBadAccept(self):
         # simulate: /rest/data/issue expect failure unknown accept settings
         body=b'{ "title": "Joe Doe has problems", \
@@ -4508,16 +4538,16 @@ def wh(s):
             'TRACKER_NAME': 'rounduptest',
             "REQUEST_METHOD": "GET"
         }
+
         self.dummy_client = client.Client(self.instance, MockNull(), env,
-                                          [], None)
+                                          cgi.FieldStorage(), None)
         self.dummy_client.db = self.db
         self.dummy_client.request.headers.get = self.get_header
         self.empty_form = cgi.FieldStorage()
         self.terse_form = cgi.FieldStorage()
         self.terse_form.list = [
             cgi.MiniFieldStorage('@verbose', '0'),
         ]
-        self.dummy_client.form = cgi.FieldStorage()
         self.dummy_client.form.list = [
             cgi.MiniFieldStorage('@fields', 'username,address'),
         ]
@@ -4575,7 +4605,7 @@ def wh(s):
         )
 
         self.dummy_client = client.Client(self.instance, MockNull(), env,
-                                          [], None)
+                                          cgi.FieldStorage(), None)
         self.dummy_client.db = self.db
         self.dummy_client.request.headers.get = self.get_header
         self.empty_form = cgi.FieldStorage()
@@ -4645,7 +4675,7 @@ def wh(s):
         }
 
         self.dummy_client = client.Client(self.instance, MockNull(), env,
-                                          [], None)
+                                          cgi.FieldStorage(), None)
         self.dummy_client.db = self.db
         self.dummy_client.request.headers.get = self.get_header
         self.empty_form = cgi.FieldStorage()
@@ -4710,7 +4740,7 @@ def wh(s):
             "REQUEST_METHOD": "GET"
         }
         self.dummy_client = client.Client(self.instance, MockNull(), env,
-                                          [], None)
+                                          cgi.FieldStorage(), None)
         self.dummy_client.db = self.db
         self.dummy_client.request.headers.get = self.get_header
         self.empty_form = cgi.FieldStorage()
@@ -4781,7 +4811,7 @@ def wh(s):
             "REQUEST_METHOD": "GET"
         }
         self.dummy_client = client.Client(self.instance, MockNull(), env,
-                                          [], None)
+                                          cgi.FieldStorage(), None)
         self.dummy_client.db = self.db
         self.dummy_client.request.headers.get = self.get_header
         self.empty_form = cgi.FieldStorage()
@@ -4849,7 +4879,7 @@ def wh(s):
             "REQUEST_METHOD": "GET"
         }
         self.dummy_client = client.Client(self.instance, MockNull(), env,
-                                          [], None)
+                                          cgi.FieldStorage(), None)
         self.dummy_client.db = self.db
         self.dummy_client.request.headers.get = self.get_header
         self.empty_form = cgi.FieldStorage()
@@ -4891,7 +4921,7 @@ def wh(s):
             "REQUEST_METHOD": "GET"
         }
         self.dummy_client = client.Client(self.instance, MockNull(), env,
-                                          [], None)
+                                          cgi.FieldStorage(), None)
         self.dummy_client.db = self.db
         self.dummy_client.request.headers.get = self.get_header
         self.empty_form = cgi.FieldStorage()
@@ -4930,7 +4960,7 @@ def wh(s):
             "REQUEST_METHOD": "GET"
         }
         self.dummy_client = client.Client(self.instance, MockNull(), env,
-                                          [], None)
+                                          cgi.FieldStorage(), None)
         self.dummy_client.db = self.db
         self.dummy_client.request.headers.get = self.get_header
         self.empty_form = cgi.FieldStorage()
@@ -4966,7 +4996,7 @@ def wh(s):
             "REQUEST_METHOD": "GET"
         }
         self.dummy_client = client.Client(self.instance, MockNull(), env,
-                                          [], None)
+                                          cgi.FieldStorage(), None)
         self.dummy_client.db = self.db
         self.dummy_client.request.headers.get = self.get_header
         self.empty_form = cgi.FieldStorage()
@@ -5003,7 +5033,7 @@ def wh(s):
             "REQUEST_METHOD": "GET"
         }
         self.dummy_client = client.Client(self.instance, MockNull(), env,
-                                          [], None)
+                                          cgi.FieldStorage(), None)
         self.dummy_client.db = self.db
         self.dummy_client.request.headers.get = self.get_header
         self.empty_form = cgi.FieldStorage()
@@ -5039,7 +5069,7 @@ def wh(s):
             "REQUEST_METHOD": "GET"
         }
         self.dummy_client = client.Client(self.instance, MockNull(), env,
-                                          [], None)
+                                           cgi.FieldStorage(), None)
         self.dummy_client.db = self.db
         self.dummy_client.request.headers.get = self.get_header
         self.empty_form = cgi.FieldStorage()

test/test_cgi.py

@@ -1870,8 +1870,11 @@ def testXmlrpcCsrfProtection(self):
         def wh(s):
             out.append(s)
 
-        # xmlrpc has no form content
-        form = {}
+        # create form for xmlrpc from string
+        form = db_test_base.makeFormFromString('xyzzy',
+                                               {"REQUEST_METHOD": "POST",
+                                               "CONTENT_TYPE": "text/json"})
+
         cl = client.Client(self.instance, None,
                            {'REQUEST_METHOD':'POST',
                             'PATH_INFO':'xmlrpc',

test/test_liveserver.py

@@ -333,6 +333,18 @@ def test_cookie_attributes(self):
         self.assertEqual(cookie._rest['HttpOnly'], None)  # flag is present
         self.assertEqual(cookie._rest['SameSite'], 'Lax')
 
+    def test_bad_post_data(self):
+        """issue2551387 - bad post data causes TypeError: not indexable
+        """
+        session, _response = self.create_login_session()
+
+        h = {"Content-Type": "text/plain"}
+        response = session.post(self.url_base()+'/', headers=h, data="test")
+        print(response.status_code)
+        print(response.headers)
+        print(response.text)
+        self.assertEqual(response.status_code, 200)
+
     def test_query(self):
         current_user_query = (
             "@columns=title,id,activity,status,assignedto&"