diff roundup/cgi/client.py @ 7150:72a54826ff4f

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 John Rouillard <rouilj@ieee.org>
date Tue, 21 Feb 2023 16:42:20 -0500
parents 5c6dd791d638
children 1181157d7cec
line wrap: on
line diff
--- a/roundup/cgi/client.py	Thu Feb 16 21:56:08 2023 -0500
+++ b/roundup/cgi/client.py	Tue Feb 21 16:42:20 2023 -0500
@@ -621,6 +621,25 @@
         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 @@
             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 @@
             # 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 @@
 
         # 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 @@
                                           "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.

Roundup Issue Tracker: http://roundup-tracker.org/