better rest Origin check; refactor CORS preflight code.

A previous version allowed requests without an origin that should
require it (e.g. an OPTIONS or PATCH request). Moved the origin
checking logic into the main flow. It looks like this was limited to
OPTIONS/PATCH requests as handle_csrf() (called later in the main
flow) handles POST, PUT, DELETE verbs.

Refactored CORS preflight request code into functions and call them
from main flow. Also return immediately. Prior code processed the
options request a second time due to falling through.

Modified is_origin_header_ok to return True if origin was missing and
it was a get request.

Fixed tests that make OPTIONS requests to supply origin.

Comment fixups.

Author: rouilj

roundup/cgi/client.py

@@ -621,6 +621,25 @@ def handle_xmlrpc(self):
         self.setHeader("Content-Length", str(len(output)))
         self.write(output)
 
+    def is_cors_preflight(self):
+        return (
+            self.env['REQUEST_METHOD'] == "OPTIONS"
+            and self.request.headers.get("Access-Control-Request-Headers")
+            and self.request.headers.get("Access-Control-Request-Method")
+            and self.request.headers.get("Origin"))
+
+    def handle_preflight(self):
+        # Call rest library to handle the pre-flight request
+        handler = rest.RestfulInstance(self, self.db)
+        output = handler.dispatch(self.env['REQUEST_METHOD'],
+                                      self.path, self.form)
+
+        if self.response_code == 204:
+            self.write("")
+        else:
+            self.setHeader("Content-Length", str(len(output)))
+            self.write(output)
+
     def handle_rest(self):
         # Set the charset and language
         self.determine_charset()
@@ -640,32 +659,32 @@ def handle_rest(self):
             self.write(output)
             return
 
-        # allow preflight request even if unauthenticated
-        if (
-            self.env['REQUEST_METHOD'] == "OPTIONS"
-            and self.request.headers.get("Access-Control-Request-Headers")
-            and self.request.headers.get("Access-Control-Request-Method")
-            and self.request.headers.get("Origin")
-        ):
-            # verify Origin is allowed
-            if not self.is_origin_header_ok(api=True):
-                # Use code 400 as 401 and 403 imply that authentication
-                # is needed or authenticates person is not authorized.
-                # Preflight doesn't do authentication.
-                self.response_code = 400
+        # verify Origin is allowed on all requests including GET.
+        # If a GET, missing origin is allowed  (i.e. same site GET request)
+        if not self.is_origin_header_ok(api=True):
+            # Use code 400. Codes 401 and 403 imply that authentication
+            # is needed or authenticated person is not authorized.
+            # Preflight doesn't do authentication.
+            self.response_code = 400
+
+            if 'HTTP_ORIGIN' not in self.env:
+                msg = self._("Required Header Missing")
+            else:
                 msg = self._("Client is not allowed to use Rest Interface.")
-                output = s2b(
-                    '{ "error": { "status": 400, "msg": "%s" } }' % msg)
-                self.setHeader("Content-Length", str(len(output)))
-                self.setHeader("Content-Type", "application/json")
-                self.write(output)
-                return
 
-            # Call rest library to handle the pre-flight request
-            handler = rest.RestfulInstance(self, self.db)
-            output = handler.dispatch(self.env['REQUEST_METHOD'],
-                                      self.path, self.form)
+            output = s2b(
+                '{ "error": { "status": 400, "msg": "%s" } }' % msg)
+            self.setHeader("Content-Length", str(len(output)))
+            self.setHeader("Content-Type", "application/json")
+            self.write(output)
+            return
 
+        # Handle CORS preflight request. We know rest is enabled
+        # because handle_rest is called. Preflight requests 
+        # are unauthenticated, so no need to check permissions.
+        if ( self.is_cors_preflight() ):
+            self.handle_preflight()
+            return
         elif not self.db.security.hasPermission('Rest Access', self.userid):
             self.response_code = 403
             output = s2b('{ "error": { "status": 403, "msg": "Forbidden." } }')
@@ -680,6 +699,8 @@ def handle_rest(self):
             # Call csrf with api (xmlrpc, rest) checks enabled.
             # It will return True if everything is ok,
             # raises exception on check failure.
+            # Note this returns true for a GET request.
+            # Must check supplied Origin header for bad value first.
             csrf_ok = self.handle_csrf(api=True)
         except (Unauthorised, UsageError) as msg:
             # FIXME should return what the client requests
@@ -695,7 +716,7 @@ def handle_rest(self):
 
         # With the return above the if will never be false,
         # Keeping the if so we can remove return to pass
-        # output though  and format output according to accept
+        # output though and format output according to accept
         # header.
         if csrf_ok is True:
             # Call rest library to handle the request
@@ -1259,10 +1280,23 @@ def check_anonymous_access(self):
                                           "allowed to use the web interface"))
 
     def is_origin_header_ok(self, api=False):
+        """Determine if origin is valid for the context
+
+           Allow (return True) if ORIGIN is missing and it is a GET.
+           Allow if ORIGIN matches the base url.
+           If this is a API call:
+             Allow if ORIGIN matches an element of allowed_api_origins.
+             Allow if allowed_api_origins includes '*' as first element..
+           Otherwise disallow.
+        """
+
         try:
             origin = self.env['HTTP_ORIGIN']
         except KeyError:
-            return False
+            if self.env['REQUEST_METHOD'] == 'GET':
+                return True
+            else:
+                return False
 
         # note base https://host/... ends host with with a /,
         # so add it to origin.

test/test_liveserver.py

@@ -424,6 +424,20 @@ def test_rest_preflight_collection(self):
                    'allowed to use Rest Interface." } }'
         self.assertEqual(b2s(f.content), expected)
 
+        # Test when Origin is not sent.
+        f = requests.options(self.url_base() + '/rest/data/user',
+                             headers = {'content-type': "application/json",
+                             'x-requested-with': "rest",
+                             'Access-Control-Request-Headers':
+                                 "x-requested-with",
+                             'Access-Control-Request-Method': "PUT",})
+
+        self.assertEqual(f.status_code, 400)
+
+        expected = ('{ "error": { "status": 400, "msg": "Required'
+                    ' Header Missing" } }')
+        self.assertEqual(b2s(f.content), expected)
+
 
     def test_rest_invalid_method_collection(self):
         # use basic auth for rest endpoint
@@ -595,7 +609,8 @@ def test_rest_endpoint_attribute_options(self):
         ## test a property that doesn't exist
         f = requests.options(self.url_base() + '/rest/data/user/1/zot',
                              auth=('admin', 'sekrit'),
-                             headers = {'content-type': ""})
+                             headers = {'content-type': "",
+                                        'Origin': "http://localhost:9001",})
         print(f.status_code)
         print(f.headers)
 
@@ -936,15 +951,15 @@ def test_compression_gzip(self, method='gzip'):
                              headers = {'content-type': "",
                                         'Accept-Encoding': '%s, foo'%method,
                                         'Accept': '*/*',
-                                        'Origin': 'ZZZZ'})
+                                        'Origin': 'https://client.com'})
         print(f.status_code)
         print(f.headers)
 
         # NOTE: not compressed payload too small
         self.assertEqual(f.status_code, 400)
         expected = { 'Content-Type': 'application/json',
                      'Access-Control-Allow-Credentials': 'true',
-                     'Access-Control-Allow-Origin': 'ZZZZ',
+                     'Access-Control-Allow-Origin': 'https://client.com',
                      'Allow': 'OPTIONS, GET, POST, PUT, DELETE, PATCH',
                      'Vary': 'Origin'
         }