issue2551205: Add support for specifying valid origins for api: xmlrpc/rest

We now have an allow list to filter the hosts allowed to do api
requests. An element of this allow list must match the http ORIGIN
header exactly or the rest/xmlrpc CORS request will result in an
error.

The tracker host is always allowed to do a request.

Author: rouilj

CHANGES.txt

@@ -91,6 +91,9 @@ Fixed:
   set to None to any filter() call should not cause a
   traceback. It will pretend as though no filter, sort or
   group was specified. (John Rouillard)
+- issue2551205 - Add support for specifying valid origins
+  for api: xmlrpc/rest. Allows CORS to work with roundup
+  backend. (John Rouillard)
 
 Features:
 

doc/rest.txt

@@ -72,6 +72,11 @@ Clients should set the header X-REQUESTED-WITH to any value and the
 tracker's config.ini should have ``csrf_enforce_header_x-requested-with
 = yes`` or ``required``.
 
+If you want to allow Roundup's api to be accessed by an application
+that is not hosted at the same origin as Roundup, you must permit
+the origin using the ``allowed_api_origins`` setting in
+``config.ini``.
+
 Rate Limiting the API
 =====================
 
@@ -211,15 +216,22 @@ require that REST be enabled. These requests do not make any changes
 or get any information from the database. As a result they are
 available to the anonymous user and any authenticated user. The user
 does not need to have `Rest Access` permissions. Also these requests
-bypass CSRF checks.
+bypass CSRF checks except for the Origin header check which is always
+run for preflight requests.
+
+You can permit only allowed ORIGINS by setting ``allowed_api_origins``
+in ``config.ini`` to the list of origins permitted to access your
+api. By default only your tracker's origin is allowed. If a preflight
+request fails, the api request will be stopped by the browser.
 
-The headers:
+The following CORS preflight headers are usually added automatically by
+the browser and must all be present:
 
 * `Access-Control-Request-Headers`
 * `Access-Control-Request-Method`
 * `Origin`
 
-must all be present. The 204 response will include:
+The 204 response will include the headers:
 
 * `Access-Control-Allow-Origin`
 * `Access-Control-Allow-Headers`
@@ -238,10 +250,6 @@ cause a problem. If it is an issue, you can see
 `Creating Custom Rate Limits`_
 and craft a rate limiter that ignores anonymous OPTIONS requests.
 
-Support for filtering by ORIGIN is not included. If you want to add
-this, see `Programming the REST API`_ on directions for overriding
-existing endpoints with a wrapper that can verify the ORIGIN.
-
 Response Formats
 ================
 

roundup/cgi/client.py

@@ -581,7 +581,7 @@ def handle_xmlrpc(self):
             # Call csrf with xmlrpc checks enabled.
             # It will return True if everything is ok,
             # raises exception on check failure.
-            csrf_ok =  self.handle_csrf(xmlrpc=True)
+            csrf_ok =  self.handle_csrf(api=True)
         except (Unauthorised, UsageError) as msg:
             # report exception back to server
             exc_type, exc_value, exc_tb = sys.exc_info()
@@ -631,10 +631,10 @@ def handle_rest(self):
         self.check_anonymous_access()
 
         try:
-            # Call csrf with xmlrpc checks enabled.
+            # Call csrf with api (xmlrpc, rest) checks enabled.
             # It will return True if everything is ok,
             # raises exception on check failure.
-            csrf_ok = self.handle_csrf(xmlrpc=True)
+            csrf_ok = self.handle_csrf(api=True)
         except (Unauthorised, UsageError) as msg:
             # report exception back to server
             exc_type, exc_value, exc_tb = sys.exc_info()
@@ -1207,8 +1207,28 @@ def check_anonymous_access(self):
                 raise Unauthorised(self._("Anonymous users are not "
                     "allowed to use the web interface"))
 
+    def is_origin_header_ok(self, api=False):
+        origin = self.env['HTTP_ORIGIN']
+        # note base https://host/... ends host with with a /,
+        # so add it to origin.
+        foundat = self.base.find(origin +'/')
+        if foundat == 0:
+            return True
+
+        if not api:
+            return False
 
-    def handle_csrf(self, xmlrpc=False):
+        allowed_origins = self.db.config['WEB_ALLOWED_API_ORIGINS']
+        # find a match for other possible origins
+        # Original spec says origin is case sensitive match.
+        # Living spec doesn't address Origin value's case or
+        # how to compare it. So implement case sensitive.... 
+        if allowed_origins[0] == '*' or origin in allowed_origins:
+            return True
+
+        return False
+
+    def handle_csrf(self, api=False):
         '''Handle csrf token lookup and validate current user and session
 
             This implements (or tries to implement) the
@@ -1332,9 +1352,8 @@ def handle_csrf(self, xmlrpc=False):
         # self.base.find("") returns 0 for example not -1
         enforce=config['WEB_CSRF_ENFORCE_HEADER_ORIGIN']
         if 'HTTP_ORIGIN' in self.env and enforce != "no":
-            origin = self.env['HTTP_ORIGIN']
-            foundat = self.base.find(origin +'/')
-            if foundat != 0:
+            if not self.is_origin_header_ok(api=api):
+                origin = self.env['HTTP_ORIGIN']
                 if enforce in ('required', 'yes'):
                     logger.error(self._("csrf Origin header check failed for user%s. Value=%s"), current_user, origin)
                     raise Unauthorised(self._("Invalid Origin %s"%origin))
@@ -1384,13 +1403,13 @@ def handle_csrf(self, xmlrpc=False):
             raise UsageError(self._("Unable to verify sufficient headers"))
 
         enforce=config['WEB_CSRF_ENFORCE_HEADER_X-REQUESTED-WITH']
-        if xmlrpc:
+        if api:
             if enforce in ['required', 'yes']:
                 # if we get here we have usually passed at least one
                 # header check. We check for presence of this custom
-                # header for xmlrpc calls only.
+                # header for xmlrpc/rest calls only.
                 # E.G. X-Requested-With: XMLHttpRequest
-                # Note we do not use CSRF nonces for xmlrpc requests.
+                # Note we do not use CSRF nonces for xmlrpc/rest requests.
                 #
                 # see: https://www.owasp.org/index.php/Cross-Site_Request_Forgery_(CSRF)_Prevention_Cheat_Sheet#Protecting_REST_Services:_Use_of_Custom_Request_Headers
                 if 'HTTP_X_REQUESTED_WITH' not in self.env:
@@ -1405,7 +1424,7 @@ def handle_csrf(self, xmlrpc=False):
         # our own.
         otks.clean()
 
-        if xmlrpc:
+        if api:
             # Save removal of expired keys from database.
             otks.commit()
             # Return from here since we have done housekeeping

roundup/configuration.py

@@ -540,6 +540,38 @@ def get(self):
             _val = os.path.join(self.config["HOME"], _val)
         return _val
 
+class SpaceSeparatedListOption(Option):
+
+    """List of space seperated elements.
+    """
+
+    class_description = "A list of space separated elements."
+
+    def get(self):
+        pathlist = []
+        _val = Option.get(self)
+        for elem in _val.split():
+                pathlist.append(elem)
+        if pathlist:
+            return pathlist
+        else:
+            return None
+
+class OriginHeadersListOption(Option):
+
+    """List of space seperated origin header values.
+    """
+
+    class_description = "A list of space separated case sensitive origin headers 'scheme://host'."
+
+
+    def set(self, _val):
+        pathlist = self._value = []
+        for elem in _val.split():
+                pathlist.append(elem)
+        if '*' in pathlist and len(pathlist) != 1:
+            raise OptionValueError(self, _val,
+                                   "If using '*' it must be the only element.")
 
 class MultiFilePathOption(Option):
 
@@ -1170,6 +1202,21 @@ def str2value(self, value):
     log if the header is invalid or missing, but accept
     the post.
 Set this to 'no' to ignore the header and accept the post."""),
+        (OriginHeadersListOption, 'allowed_api_origins', "",
+            """A comma separated list of additonal valid Origin header
+values used when enforcing the header origin. They are used
+only for the api URL's (/rest and /xmlrpc). They are not
+used for the usual html URL's. These strings must match the
+value of the Origin header exactly. So 'https://bar.edu' and
+'https://Bar.edu' are two different Origin values. Note that
+the origin value is scheme://host. There is no path
+component. So 'https://bar.edu/' would never be valid.
+
+You need to set these if you have a web application on a
+different origin accessing your roundup instance.
+
+(The origin from the tracker.web setting in config.ini is
+always valid and does not need to be specified.)"""),
         (CsrfSettingOption, 'csrf_enforce_header_x-forwarded-host', "yes",
             """Verify that the X-Forwarded-Host http header matches
 the host part of the tracker.web setting in config.ini.

test/test_cgi.py

@@ -955,7 +955,7 @@ def testCsrfProtection(self):
         # need to set SENDMAILDEBUG to prevent
         # downstream issue when email is sent on successful
         # issue creation. Also delete the file afterwards
-        # just tomake sure that someother test looking for
+        # just to make sure that some other test looking for
         # SENDMAILDEBUG won't trip over ours.
         if 'SENDMAILDEBUG' not in os.environ:
             os.environ['SENDMAILDEBUG'] = 'mail-test1.log'
@@ -1157,6 +1157,18 @@ def hasPermission(s, p, classname=None, d=None, e=None, **kw):
         del(out[0])
 
         del(cl.env['HTTP_REFERER'])
+
+        # test by setting allowed api origins to *
+        # this should not redirect as it is not an API call.
+        cl.db.config.WEB_ALLOWED_API_ORIGINS = "  *  "
+        cl.env['HTTP_ORIGIN'] = 'https://baz.edu'
+        cl.inner_main()
+        match_at=out[0].find('Invalid Origin https://baz.edu')
+        print("result of subtest invalid origin:", out[0])
+        self.assertEqual(match_at, 36)
+        del(cl.env['HTTP_ORIGIN'])
+        cl.db.config.WEB_ALLOWED_API_ORIGINS = ""
+        del(out[0])
 
         # clean up from email log
         if os.path.exists(SENDMAILDEBUG):
@@ -1239,6 +1251,106 @@ def wh(s):
         self.assertEqual(response,expected)
         del(out[0])
 
+
+        # rest has no form content
+        cl.db.config.WEB_ALLOWED_API_ORIGINS = "https://bar.edu http://bar.edu"
+        form = cgi.FieldStorage()
+        form.list = [
+            cgi.MiniFieldStorage('title', 'A new issue'),
+            cgi.MiniFieldStorage('status', '1'),
+            cgi.MiniFieldStorage('@pretty', 'false'),
+            cgi.MiniFieldStorage('@apiver', '1'),
+        ]
+        cl = client.Client(self.instance, None,
+                           {'REQUEST_METHOD':'POST',
+                            'PATH_INFO':'rest/data/issue',
+                            'CONTENT_TYPE': 'application/x-www-form-urlencoded',
+                            'HTTP_ORIGIN': 'https://bar.edu',
+                            'HTTP_X_REQUESTED_WITH': 'rest',
+                            'HTTP_AUTHORIZATION': 'Basic YWRtaW46YWRtaW4=',
+                            'HTTP_REFERER': 'http://whoami.com/path/',
+                            'HTTP_ACCEPT': "application/json;version=1"
+                        }, form)
+        cl.db = self.db
+        cl.base = 'http://whoami.com/path/'
+        cl._socket_op = lambda *x : True
+        cl._error_message = []
+        cl.request = MockNull()
+        h = { 'content-type': 'application/json',
+              'accept': 'application/json' }
+        cl.request.headers = MockNull(**h)
+                                      
+        cl.write = wh # capture output
+
+        # Should return explanation because content type is text/plain
+        # and not text/xml
+        cl.handle_rest()
+        answer='{"data": {"link": "http://tracker.example/cgi-bin/roundup.cgi/bugs/rest/data/issue/2", "id": "2"}}\n'
+        # check length to see if pretty is turned off.
+        self.assertEqual(len(out[0]), 99)
+
+        # compare as dicts not strings due to different key ordering
+        # between python versions.
+        response=json.loads(b2s(out[0]))
+        expected=json.loads(answer)
+        self.assertEqual(response,expected)
+        del(out[0])
+
+        #####
+        cl = client.Client(self.instance, None,
+                           {'REQUEST_METHOD':'POST',
+                            'PATH_INFO':'rest/data/issue',
+                            'CONTENT_TYPE': 'application/x-www-form-urlencoded',
+                            'HTTP_ORIGIN': 'httxs://bar.edu',
+                            'HTTP_AUTHORIZATION': 'Basic YWRtaW46YWRtaW4=',
+                            'HTTP_REFERER': 'http://whoami.com/path/',
+                            'HTTP_ACCEPT': "application/json;version=1"
+                        }, form)
+        cl.db = self.db
+        cl.base = 'http://whoami.com/path/'
+        cl._socket_op = lambda *x : True
+        cl._error_message = []
+        cl.request = MockNull()
+        h = { 'content-type': 'application/json',
+              'accept': 'application/json' }
+        cl.request.headers = MockNull(**h)
+                                      
+        cl.write = wh # capture output
+
+        # Should return explanation because content type is text/plain
+        # and not text/xml
+        cl.handle_rest()
+        self.assertEqual(b2s(out[0]), "<class 'roundup.exceptions.Unauthorised'>: Invalid Origin httxs://bar.edu\n")
+        del(out[0])
+
+
+        cl.db.config.WEB_ALLOWED_API_ORIGINS = "  * "
+        cl = client.Client(self.instance, None,
+                           {'REQUEST_METHOD':'POST',
+                            'PATH_INFO':'rest/data/issue',
+                            'CONTENT_TYPE': 'application/x-www-form-urlencoded',
+                            'HTTP_ORIGIN': 'httxs://bar.edu',
+                            'HTTP_X_REQUESTED_WITH': 'rest',
+                            'HTTP_AUTHORIZATION': 'Basic YWRtaW46YWRtaW4=',
+                            'HTTP_REFERER': 'http://whoami.com/path/',
+                            'HTTP_ACCEPT': "application/json;version=1"
+                        }, form)
+        cl.db = self.db
+        cl.base = 'http://whoami.com/path/'
+        cl._socket_op = lambda *x : True
+        cl._error_message = []
+        cl.request = MockNull()
+        h = { 'content-type': 'application/json',
+              'accept': 'application/json' }
+        cl.request.headers = MockNull(**h)
+                                      
+        cl.write = wh # capture output
+
+        # create third issue
+        cl.handle_rest()
+        self.assertIn('"id": "3"', b2s(out[0]))
+        del(out[0])
+
     def testXmlrpcCsrfProtection(self):
         # set the password for admin so we can log in.
         passwd=password.Password('admin')
@@ -1663,12 +1775,11 @@ def testRegisterActionDelay(self):
         # need to set SENDMAILDEBUG to prevent
         # downstream issue when email is sent on successful
         # issue creation. Also delete the file afterwards
-        # just tomake sure that someother test looking for
+        # just to make sure that some other test looking for
         # SENDMAILDEBUG won't trip over ours.
         if 'SENDMAILDEBUG' not in os.environ:
             os.environ['SENDMAILDEBUG'] = 'mail-test1.log'
         SENDMAILDEBUG = os.environ['SENDMAILDEBUG']
-
 
         # missing opaqueregister
         cl = self._make_client({'username':'new_user1', 'password':'secret',
@@ -1727,7 +1838,7 @@ def testRegisterActionUnusedUserCheck(self):
         # need to set SENDMAILDEBUG to prevent
         # downstream issue when email is sent on successful
         # issue creation. Also delete the file afterwards
-        # just tomake sure that someother test looking for
+        # just to make sure that some other test looking for
         # SENDMAILDEBUG won't trip over ours.
         if 'SENDMAILDEBUG' not in os.environ:
             os.environ['SENDMAILDEBUG'] = 'mail-test1.log'

test/test_config.py

@@ -279,6 +279,18 @@ def testFloatAndInt_with_update_option(self):
        self.assertEqual("3", 
             config._get_option('WEB_LOGIN_ATTEMPTS_MIN')._value2str(3.00))
 
+    def testOriginHeader(self):
+        config = configuration.CoreConfig()
+
+        with self.assertRaises(configuration.OptionValueError) as cm:
+            config._get_option('WEB_ALLOWED_API_ORIGINS').set("* https://foo.edu")
+
+        config._get_option('WEB_ALLOWED_API_ORIGINS').set("https://foo.edu HTTP://baR.edu")
+
+        self.assertEqual(config['WEB_ALLOWED_API_ORIGINS'][0], 'https://foo.edu')
+        self.assertEqual(config['WEB_ALLOWED_API_ORIGINS'][1], 'HTTP://baR.edu')
+        
+
 
     def testOptionAsString(self):