improve REST interface security

When using REST, we reflect the client's origin. If the wildcard '*'
is used in allowed_api_origins all origins are allowed. When this is
done, it also added an 'Access-Control-Allow-Credentials: true'
header.

This Credentials header should not be added if the site is matched
only by '*'. This header should be provided only for explicit origins
(e.g. https://example.org) not for the wildcard.

This is now fixed for CORS preflight OPTIONS request as well as normal
GET, PUT, DELETE, POST, PATCH and OPTIONS requests.

A missing Access-Control-Allow-Credentials will prevent the tracker
from being accessed using credentials. This prevents an unauthorized
third party web site from using a user's credentials to access
information in the tracker that is not publicly available.

Added test for this specific case.

In addition, allowed_api_origins can include explicit origins in
addition to '*'. '*' must be first in the list.

Also adapted numerous tests to work with these changes.

Doc updates.

Author: rouilj

CHANGES.txt

@@ -65,6 +65,9 @@ Fixed:
   Schlatterbeck)
 - Update some template schema files to assign Register permissions for the
   Anonymous user. Replaces the old Create permission. (John Rouillard)
+- Allow '*' and explicit origins in allowed_api_origins. Only return 
+  'Access-Control-Allow-Credentials' when not matching '*'. Fixes
+  security issue with rest when using '*'.
 
 Features:
 

doc/rest.txt

@@ -231,14 +231,26 @@ the browser and must all be present:
 * `Access-Control-Request-Method`
 * `Origin`
 
-The 204 response will include the headers:
+The headers of the 204 response depend on the
+``allowed_api_origins`` setting. If a ``*`` is included as the
+first element, any client can read the data but they can not
+provide authentication. This limits the available data to what
+the anonymous user can see in the web interface.
+
+All 204 responses will include the headers:
 
 * `Access-Control-Allow-Origin`
 * `Access-Control-Allow-Headers`
 * `Access-Control-Allow-Methods`
-* `Access-Control-Allow-Credentials: true`
 * `Access-Control-Max-Age: 86400`
 
+If the client's ORIGIN header matches an entry besides ``*`` in the
+``allowed_api_origins`` it will also include:
+
+* `Access-Control-Allow-Credentials: true`
+
+permitting the client to log in and perform authenticated operations.
+  
 If the endpoint accepts the PATCH verb the header `Accept-Patch` with
 valid mime types (usually `application/x-www-form-urlencoded,
 multipart/form-data`) will be included.

doc/upgrading.txt

@@ -236,6 +236,25 @@ are not used.)
 For details on WAL mode see `<https://www.sqlite.org/wal.html>`_
 and `<https://www.sqlite.org/pragma.html#pragma_journal_mode>`_.
 
+Change in processing allowed_api_origins setting
+------------------------------------------------
+
+In this release you can use both ``*`` (as the first origin) and
+explicit origins in the `allowed_api_origins`` setting in
+``config.ini``. (Before it was only one or the other.)
+
+You do not need to use ``*``. If you do, it allows any client
+anonymous (unauthenticated) access to the Roundup tracker. This
+is the same as browsing the tracker without logging in. If they
+try to provide credentials, access to the data will be denied by
+`CORS`_.
+
+If you include explicit origins (e.g. \https://example.com),
+users from those origins will not be blocked if they use
+credentials to log in.
+
+.. _CORS: https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS
+
 Change in processing of In-Reply_to email header
 ------------------------------------------------
 

roundup/cgi/client.py

@@ -1279,15 +1279,20 @@ 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):
+    def is_origin_header_ok(self, api=False, credentials=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.
+           Header is ok (return True) if ORIGIN is missing and it is a GET.
+           Header is ok 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.
+             Header is ok if ORIGIN matches an element of allowed_api_origins.
+             Header is ok if allowed_api_origins includes '*' as first
+               element and credentials is False.
+           Otherwise header is not ok.
+
+           In a credentials context, if we match * we will return
+           header is not ok. All credentialed requests must be
+           explicitly matched.
         """
 
         try:
@@ -1312,9 +1317,15 @@ def is_origin_header_ok(self, api=False):
         # 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:
-            if allowed_origins[0] == '*' or origin in allowed_origins:
-                return True
+        if origin in allowed_origins:
+             return True
+        # Block use of * when origin match is used for
+        # allowing credentials. See:
+        # https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS
+        # under Credentials Requests and Wildcards
+        if ( allowed_origins and allowed_origins[0] == '*'
+             and not credentials):
+            return True
 
         return False
 

roundup/configuration.py

@@ -571,9 +571,10 @@ 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.")
+        if '*' in pathlist and pathlist[0] != '*':
+            raise OptionValueError(
+                self, _val,
+                "If using '*' it must be the first element.")
 
     def _value2str(self, value):
         return ','.join(value)
@@ -1317,13 +1318,13 @@ def str2value(self, value):
 '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.
-Also the value * can be used to match any origin. Note that
-this value allows any web page on the internet to make
-authenticated requests against your Roundup tracker and
-is not a good idea.
+The value '*' can be used to match any origin. It must be
+first in the list if used. Note that this value allows
+any web page on the internet to make anonymous requests
+against your Roundup tracker.
 
 You need to set these if you have a web application on a
-different origin accessing your roundup instance.
+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.)"""),

roundup/rest.py

@@ -2201,11 +2201,24 @@ def dispatch(self, method, uri, input):
             self.client.request.headers.get("Origin")
         )
 
-        # allow credentials
-        self.client.setHeader(
-            "Access-Control-Allow-Credentials",
-            "true"
-        )
+        # Allow credentials if origin is acceptable.
+        #
+        # If Access-Control-Allow-Credentials header not returned,
+        # but the client request is made with credentials
+        # data will be sent but not made available to the
+        # calling javascript in browser.
+        # Prevents exposure of data to an invalid origin when
+        # credentials are sent by client.
+        #
+        # If admin puts * first in allowed_api_origins
+        # we do not allow credentials but do reflect the origin.
+        # This allows anonymous access.
+        if self.client.is_origin_header_ok(api=True, credentials=True):
+            self.client.setHeader(
+                "Access-Control-Allow-Credentials",
+                "true"
+            )
+
         # set allow header in case of error. 405 handlers below should
         # replace it with a custom version as will OPTIONS handler
         # doing CORS.