Mercurial > p > roundup > code
changeset 6681:ab2ed11c021e
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 | John Rouillard <rouilj@ieee.org> |
|---|---|
| date | Tue, 17 May 2022 17:18:51 -0400 |
| parents | b4d0b48b3096 |
| children | 0b8d34b64930 |
| files | CHANGES.txt doc/rest.txt roundup/cgi/client.py roundup/configuration.py test/test_cgi.py test/test_config.py |
| diffstat | 6 files changed, 222 insertions(+), 22 deletions(-) [+] |
line wrap: on
line diff
--- a/CHANGES.txt Tue May 17 14:09:00 2022 -0400 +++ b/CHANGES.txt Tue May 17 17:18:51 2022 -0400 @@ -91,6 +91,9 @@ 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:
--- a/doc/rest.txt Tue May 17 14:09:00 2022 -0400 +++ b/doc/rest.txt Tue May 17 17:18:51 2022 -0400 @@ -72,6 +72,11 @@ 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 @@ 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. -The headers: +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 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 @@ `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 ================
--- a/roundup/cgi/client.py Tue May 17 14:09:00 2022 -0400 +++ b/roundup/cgi/client.py Tue May 17 17:18:51 2022 -0400 @@ -581,7 +581,7 @@ # 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 @@ 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 @@ 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 - def handle_csrf(self, xmlrpc=False): + if not api: + return 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 @@ # 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 @@ 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 @@ # 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
--- a/roundup/configuration.py Tue May 17 14:09:00 2022 -0400 +++ b/roundup/configuration.py Tue May 17 17:18:51 2022 -0400 @@ -540,6 +540,38 @@ _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 @@ 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.
--- a/test/test_cgi.py Tue May 17 14:09:00 2022 -0400 +++ b/test/test_cgi.py Tue May 17 17:18:51 2022 -0400 @@ -955,7 +955,7 @@ # 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 @@ 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 @@ 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 @@ # 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 @@ # 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'
--- a/test/test_config.py Tue May 17 14:09:00 2022 -0400 +++ b/test/test_config.py Tue May 17 17:18:51 2022 -0400 @@ -279,6 +279,18 @@ 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):