Mercurial > p > roundup > code
changeset 8177:2967f37e73e4
refactor: issue2551289. invalid REST Accept header stops request
Sending a POST, PUT (maybe PATCH) with an accept header that is not
application/json or xml (if enabled) used to complete the request
before throwing a 406 error. This was wrong.
Now it reports an error without dispatching/processing the requested
transaction. This is the first of a series of refactors of the
dispatch method to make it faster and more readable by using return
early pattern and extracting methods from the code.
changes:
The following now return 406 errors not 400 errors
invalid version specified with @apiver in URL.
invalid version specified with @apiver in payload body
invalid version specified in accept headers as
application/vnd.roundup.test-vz+json or version property
Parsing the accept header returns a 400 when presented with a
parameter without an = sign or other parse error. They used to
return a 406 which is wrong since the header is malformed rather
than having a value I can't respond to.
Some error messages were made clearer.
Results in the case of an error are proper json error object rather
than text/plain strings.
New test added for testdetermine_output_formatBadAccept that test the
new method using the same test cases as for
testDispatchBadAccept. I intend to extend the test coverage for
determine_output_format to cover more cases. This should be a faster
unit test than for dispatch.
Removed .lower() calls for accept_mime_type as the input values are
taken from the values in the __accepted_content_type dict which
only has lower case values.
| author | John Rouillard <rouilj@ieee.org> |
|---|---|
| date | Sun, 08 Dec 2024 01:09:34 -0500 |
| parents | 736f769b48c8 |
| children | a73c88154a68 |
| files | CHANGES.txt doc/upgrading.txt roundup/rest.py test/rest_common.py |
| diffstat | 4 files changed, 324 insertions(+), 99 deletions(-) [+] |
line wrap: on
line diff
--- a/CHANGES.txt Sat Dec 07 17:34:06 2024 -0500 +++ b/CHANGES.txt Sun Dec 08 01:09:34 2024 -0500 @@ -33,6 +33,9 @@ - issue2551372 - Better document necessary headers for REST and fix logging to log missing Origin header (Ralf Schlatterbeck with suggestions on documentation by John Rouillard) +- issue2551289 - Invalid REST Accept header with post/put performs + change before returning 406. Error before making any changes to the + db if we can't respond with requested format. (John Rouillard) Features:
--- a/doc/upgrading.txt Sat Dec 07 17:34:06 2024 -0500 +++ b/doc/upgrading.txt Sun Dec 08 01:09:34 2024 -0500 @@ -149,6 +149,18 @@ https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#cookie_prefixes for details on this security measure. +Invalid accept header now prevents operation (info) +--------------------------------------------------- + +In earlier versions, the rest interface checked for an incorrect +"Accept" header, "@apiver", or the ".json" mime type only after +processing the request. This would lead to a 406 error, but the +requested change would still be completed. + +In this release, the validation of the output format and version +occurs before any database changes are made. Now, all errors related +to the data format (mime type, API version) will return 406 errors, +where some previously resulted in 400 errors. .. index:: Upgrading; 2.3.0 to 2.4.0
--- a/roundup/rest.py Sat Dec 07 17:34:06 2024 -0500 +++ b/roundup/rest.py Sun Dec 08 01:09:34 2024 -0500 @@ -268,6 +268,8 @@ Default `q` for values that are not specified is 1.0 + If q value > 1.0, it is parsed as a very small value. + # Based on https://gist.github.com/samuraisam/2714195 # Also, based on a snipped found in this project: # https://github.com/martinblech/mimerender @@ -2206,6 +2208,136 @@ # human may read it ), None) + def determine_output_format(self, uri): + """Returns tuple of: + + (format for returned output, + possibly modified uri, + output error object (if error else None)) + + Verify that client is requesting an output we can produce + with a version we support. + + Only application/json and application/xml (optional) + are currently supported. .vcard might be useful + in the future for user objects. + + Find format for returned output: + + 1) Get format from url extension (.json, .xml) and return + if invalid return (None, uri, 406 error) + if not found continue + 2) Parse Accept header obeying q values + if header unparsible return 400 error object. + 3) if empty or missing Accept header + return self.__default_accept_type + 4) match and return best Accept header/version + if version error found in matching type return 406 error + 5) if no requested format is supported return 406 + error + + """ + # get the request format for response + # priority : extension from uri (/rest/data/issue.json) + # only json or xml valid at this time. + # header (Accept: application/json, application/xml) + # default (application/json) + ext_type = os.path.splitext(urlparse(uri).path)[1][1:] + + # Check to see if the extension matches a value in + # self.__accepted_content_type. In the future other output + # such as .vcard for downloading user info can be + # supported. This method also allow detection of mistyped + # types like jon for json. Limit extension length to less than + # 10 characters to allow passing JWT via URL path. Could be + # useful for magic link login method or account recovery workflow, + # using a JWT with a short expiration time and limited rights + # (e.g. only password change permission)) + if ext_type and (len(ext_type) < 10): + if ext_type not in list(self.__accepted_content_type.values()): + self.client.response_code = 406 + return (None, uri, + self.error_obj( + 406, + _("Content type '%s' requested in URL is " + "not available.\nAcceptable types: %s\n") % + (ext_type, + ", ".join(sorted( + set(self.__accepted_content_type.values())))))) + + # strip extension so uri makes sense. + # E.G. .../issue.json -> .../issue + uri = uri[:-(len(ext_type) + 1)] + return (ext_type, uri, None) + + # parse Accept header and get the content type + # Acceptable types ordered with preferred one (by q value) + # first in list. + try: + accept_header = parse_accept_header( + self.client.request.headers.get('Accept') + ) + except UsageError as e: + self.client.response_code = 406 + return (None, uri, self.error_obj( + 400, _("Unable to parse Accept Header. %(error)s. " + "Acceptable types: %(acceptable_types)s") % { + 'error': e.args[0], + 'acceptable_types': " ".join(sorted( + self.__accepted_content_type.keys()))})) + + if not accept_header: + # we are using the default + return (self.__default_accept_type, uri, None) + + accept_type = "" + for part in accept_header: + if accept_type: + # we accepted the best match, stop searching for + # lower quality matches. + break + if part[0] in self.__accepted_content_type: + accept_type = self.__accepted_content_type[part[0]] + # Version order: + # 1) accept header version=X specifier + # application/vnd.x.y; version=1 + # 2) from type in accept-header type/subtype-vX + # application/vnd.x.y-v1 + # 3) from @apiver in query string to make browser + # use easy + # This code handles 1 and 2. Set api_version to none + # to trigger @apiver parsing below + # Places that need the api_version info should + # use default if version = None + try: + self.api_version = int(part[1]['version']) + except KeyError: + self.api_version = None + except (ValueError, TypeError): + self.client.response_code = 406 + # TypeError if int(None) + msg = _("Unrecognized api version: %s. " + "See /rest without specifying api version " + "for supported versions.") % ( + part[1]['version']) + return (None, uri, + self.error_obj(406, msg)) + + # accept_type will be empty only if there is an Accept header + # with invalid values. + if accept_type: + return (accept_type, uri, None) + + self.client.response_code = 400 + return (None, uri, + self.error_obj( + 406, + _("Requested content type(s) '%s' not available.\n" + "Acceptable mime types are: %s") % + (self.client.request.headers.get('Accept'), + ", ".join(sorted( + self.__accepted_content_type.keys()))))) + def dispatch(self, method, uri, input): """format and process the request""" output = None @@ -2248,75 +2380,15 @@ 'Ignoring X-HTTP-Method-Override using %s request on %s', method.upper(), uri) - # parse Accept header and get the content type - # Acceptable types ordered with preferred one first - # in list. - try: - accept_header = parse_accept_header(headers.get('Accept')) - except UsageError as e: - output = self.error_obj(406, _("Unable to parse Accept Header. %(error)s. " - "Acceptable types: %(acceptable_types)s") % { - 'error': e.args[0], - 'acceptable_types': " ".join(sorted(self.__accepted_content_type.keys()))}) - accept_header = [] + # FIXME: when this method is refactored, change + # determine_output_format to raise an exception. Catch it here + # and return early. Also set self.client.response_code from + # error object['error']['status'] and remove from + # determine_output_format. + (output_format, uri, error) = self.determine_output_format(uri) + if error: + output = error - if not accept_header: - accept_type = self.__default_accept_type - else: - accept_type = None - for part in accept_header: - if accept_type: - # we accepted the best match, stop searching for - # lower quality matches. - break - if part[0] in self.__accepted_content_type: - accept_type = self.__accepted_content_type[part[0]] - # Version order: - # 1) accept header version=X specifier - # application/vnd.x.y; version=1 - # 2) from type in accept-header type/subtype-vX - # application/vnd.x.y-v1 - # 3) from @apiver in query string to make browser - # use easy - # This code handles 1 and 2. Set api_version to none - # to trigger @apiver parsing below - # Places that need the api_version info should - # use default if version = None - try: - self.api_version = int(part[1]['version']) - except KeyError: - self.api_version = None - except (ValueError, TypeError): - # TypeError if int(None) - msg = ("Unrecognized api version: %s. " - "See /rest without specifying api version " - "for supported versions." % ( - part[1]['version'])) - output = self.error_obj(400, msg) - - # get the request format for response - # priority : extension from uri (/rest/data/issue.json), - # header (Accept: application/json, application/xml) - # default (application/json) - ext_type = os.path.splitext(urlparse(uri).path)[1][1:] - - # Check to see if the length of the extension is less than 6. - # this allows use of .vcard for a future use in downloading - # user info. It also allows passing through larger items like - # JWT that has a final component > 6 items. This method also - # allow detection of mistyped types like jon for json. - if ext_type and (len(ext_type) < 6): - # strip extension so uri make sense - # .../issue.json -> .../issue - uri = uri[:-(len(ext_type) + 1)] - else: - ext_type = None - - # headers.get('Accept') is never empty if called here. - # accept_type will be set to json if there is no Accept header - # accept_type wil be empty only if there is an Accept header - # with invalid values. - data_type = ext_type or accept_type or headers.get('Accept') or "invalid" if method.upper() == 'OPTIONS': # add access-control-allow-* access-control-max-age to support # CORS preflight @@ -2441,6 +2513,9 @@ "See /rest without specifying api version " "for supported versions.") try: + # FIXME: the version priority here is different + # from accept header. accept mime type in url + # takes priority over Accept header. Opposite here. if not self.api_version: self.api_version = int(input['@apiver'].value) # Can also return a TypeError ("not indexable") @@ -2448,7 +2523,7 @@ except (KeyError, TypeError): self.api_version = None except ValueError: - output = self.error_obj(400, msg % input['@apiver'].value) + output = self.error_obj(406, msg % input['@apiver'].value) # by this time the API version is set. Error if we don't # support it? @@ -2459,7 +2534,7 @@ # Use default if not specified for now. self.api_version = self.__default_api_version elif self.api_version not in self.__supported_api_versions: - output = self.error_obj(400, msg % self.api_version) + output = self.error_obj(406, msg % self.api_version) # sadly del doesn't work on FieldStorage which can be the type of # input. So we have to ignore keys starting with @ at other @@ -2479,19 +2554,23 @@ output = self.error_obj(405, msg.args[0]) self.client.setHeader("Allow", msg.args[1]) - return self.format_dispatch_output(data_type, output, pretty_output) + return self.format_dispatch_output(output_format, + output, + pretty_output) def format_dispatch_output(self, accept_mime_type, output, pretty_print=True): # Format the content type - if accept_mime_type.lower() == "json": + # if accept_mime_type is None, the client specified invalid + # mime types so we default to json output. + if accept_mime_type == "json" or accept_mime_type is None: self.client.setHeader("Content-Type", "application/json") if pretty_print: indent = 4 else: indent = None output = RoundupJSONEncoder(indent=indent).encode(output) - elif accept_mime_type.lower() == "xml" and dicttoxml: + elif accept_mime_type == "xml" and dicttoxml: self.client.setHeader("Content-Type", "application/xml") if 'error' in output: # capture values in error with types unsupported
--- a/test/rest_common.py Sat Dec 07 17:34:06 2024 -0500 +++ b/test/rest_common.py Sun Dec 08 01:09:34 2024 -0500 @@ -1837,6 +1837,140 @@ self.server.client.additional_headers) self.server.client.additional_headers = {} + def testdetermine_output_formatBadAccept(self): + dof = self.server.determine_output_format + + # simulate: /rest/data/issue expect failure unknown accept settings + body=b'{ "title": "Joe Doe has problems", \ + "nosy": [ "1", "3" ], \ + "assignedto": "2", \ + "abool": true, \ + "afloat": 2.3, \ + "anint": 567890 \ + }' + env = { "CONTENT_TYPE": "application/json", + "CONTENT_LENGTH": len(body), + "REQUEST_METHOD": "POST" + } + self.server.client.env.update(env) + headers={"accept": "application/zot; version=1; q=0.5", + "content-type": env['CONTENT_TYPE'], + "content-length": env['CONTENT_LENGTH'], + } + + self.headers=headers + # we need to generate a FieldStorage the looks like + # FieldStorage(None, None, 'string') rather than + # FieldStorage(None, None, []) + body_file=BytesIO(body) # FieldStorage needs a file + form = client.BinaryFieldStorage(body_file, + headers=headers, + environ=env) + self.server.client.request.headers.get=self.get_header + + (output_type, uri, error) = dof("/rest/data/issue") + + self.assertEqual(self.server.client.response_code, 406) + self.assertIn(b"Requested content type(s) 'application/zot; version=1; q=0.5' not available.\nAcceptable mime types are: */*, application/json", + s2b(error['error']['msg'])) + + # simulate: /rest/data/issue works, multiple acceptable output, one + # is valid + self.server.client.response_code = "" + env = { "CONTENT_TYPE": "application/json", + "CONTENT_LENGTH": len(body), + "REQUEST_METHOD": "POST" + } + self.server.client.env.update(env) + headers={"accept": "application/zot; version=1; q=0.75, " + "application/json; version=1; q=0.5", + "content-type": env['CONTENT_TYPE'], + "content-length": env['CONTENT_LENGTH'], + } + + self.headers=headers + # we need to generate a FieldStorage the looks like + # FieldStorage(None, None, 'string') rather than + # FieldStorage(None, None, []) + body_file=BytesIO(body) # FieldStorage needs a file + form = client.BinaryFieldStorage(body_file, + headers=headers, + environ=env) + self.server.client.request.headers.get=self.get_header + (output_type, uri, error) = dof("/rest/data/issue") + + self.assertEqual(self.server.client.response_code, "") + self.assertEqual(output_type, "json") + self.assertEqual(uri, "/rest/data/issue") + self.assertEqual(error, None) + + # test 3 accept is empty. This triggers */* so passes + self.server.client.response_code = "" + headers={"accept": "", + "content-type": env['CONTENT_TYPE'], + "content-length": env['CONTENT_LENGTH'], + } + + self.headers=headers + # we need to generate a FieldStorage the looks like + # FieldStorage(None, None, 'string') rather than + # FieldStorage(None, None, []) + body_file=BytesIO(body) # FieldStorage needs a file + form = client.BinaryFieldStorage(body_file, + headers=headers, + environ=env) + self.server.client.request.headers.get=self.get_header + (output_type, uri, error) = dof("/rest/data/issue") + + self.assertEqual(self.server.client.response_code, "") + self.assertEqual(output_type, "json") + self.assertEqual(uri, "/rest/data/issue") + self.assertEqual(error, None) + + # test 4 accept is random junk. + headers={"accept": "Xyzzy I am not a mime, type;", + "content-type": env['CONTENT_TYPE'], + "content-length": env['CONTENT_LENGTH'], + } + + self.headers=headers + # we need to generate a FieldStorage the looks like + # FieldStorage(None, None, 'string') rather than + # FieldStorage(None, None, []) + body_file=BytesIO(body) # FieldStorage needs a file + form = client.BinaryFieldStorage(body_file, + headers=headers, + environ=env) + self.server.client.request.headers.get=self.get_header + (output_type, uri, error) = dof("/rest/data/issue") + + self.assertEqual(self.server.client.response_code, 400) + self.assertEqual(output_type, None) + self.assertEqual(uri, "/rest/data/issue") + self.assertIn('Unable to parse Accept Header. Invalid media type: Xyzzy I am not a mime. Acceptable types: */* application/json', error['error']['msg']) + + # test 5 accept mimetype is ok, param is not + headers={"accept": "*/*; foo", + "content-type": env['CONTENT_TYPE'], + "content-length": env['CONTENT_LENGTH'], + } + + self.headers=headers + # we need to generate a FieldStorage the looks like + # FieldStorage(None, None, 'string') rather than + # FieldStorage(None, None, []) + body_file=BytesIO(body) # FieldStorage needs a file + form = client.BinaryFieldStorage(body_file, + headers=headers, + environ=env) + self.server.client.request.headers.get=self.get_header + (output_type, uri, error) = dof("/rest/data/issue") + + self.assertEqual(self.server.client.response_code, 400) + self.assertEqual(output_type, None) + self.assertEqual(uri, "/rest/data/issue") + self.assertIn('Unable to parse Accept Header. Invalid param: foo. Acceptable types: */* application/json', error['error']['msg']) + def testDispatchBadAccept(self): # simulate: /rest/data/issue expect failure unknown accept settings body=b'{ "title": "Joe Doe has problems", \ @@ -1868,10 +2002,11 @@ results = self.server.dispatch(env["REQUEST_METHOD"], "/rest/data/issue", form) - print(results) + json_dict = json.loads(b2s(results)) self.assertEqual(self.server.client.response_code, 406) - self.assertIn(b"Requested content type 'application/zot; version=1; q=0.5' is not available.\nAcceptable types: */*, application/json", results) + self.assertIn("Requested content type(s) 'application/zot; version=1; q=0.5' not available.\nAcceptable mime types are: */*, application/json", + json_dict['error']['msg']) # simulate: /rest/data/issue works, multiple acceptable output, one # is valid @@ -1902,12 +2037,7 @@ print(results) self.assertEqual(self.server.client.response_code, 201) json_dict = json.loads(b2s(results)) - # ERROR this should be 1. What's happening is that the code - # for handling 406 error code runs through everything and creates - # the item. Then it throws a 406 after the work is done when it - # realizes it can't respond as requested. So the 406 post above - # creates issue 1 and this one creates issue 2. - self.assertEqual(json_dict['data']['id'], "2") + self.assertEqual(json_dict['data']['id'], "1") # test 3 accept is empty. This triggers */* so passes @@ -1932,9 +2062,7 @@ print(results) self.assertEqual(self.server.client.response_code, 201) json_dict = json.loads(b2s(results)) - # This is one more than above. Will need to be fixed - # When error above is fixed. - self.assertEqual(json_dict['data']['id'], "3") + self.assertEqual(json_dict['data']['id'], "2") # test 4 accept is random junk. headers={"accept": "Xyzzy I am not a mime, type;", @@ -1956,7 +2084,7 @@ form) print(results) - self.assertEqual(self.server.client.response_code, 406) + self.assertEqual(self.server.client.response_code, 400) json_dict = json.loads(b2s(results)) self.assertIn('Unable to parse Accept Header. Invalid media type: Xyzzy I am not a mime. Acceptable types: */* application/json', json_dict['error']['msg']) @@ -1980,7 +2108,7 @@ form) print(results) - self.assertEqual(self.server.client.response_code, 406) + self.assertEqual(self.server.client.response_code, 400) json_dict = json.loads(b2s(results)) self.assertIn('Unable to parse Accept Header. Invalid param: foo. Acceptable types: */* application/json', json_dict['error']['msg']) @@ -2147,7 +2275,7 @@ results = self.server.dispatch('PUT', "/rest/data/user/%s/realname"%self.joeid, form) - self.assertEqual(self.server.client.response_code, 400) + self.assertEqual(self.server.client.response_code, 406) del(self.headers) # TEST #2 @@ -2427,13 +2555,14 @@ print(results) try: # only verify local copy not system installed copy from roundup.dicttoxml import dicttoxml - includexml = ', application/xml' + includexml = ', xml' except ImportError: includexml = '' - - response="Requested content type 'jon' is not available.\n" \ - "Acceptable types: */*, application/json%s\n"%includexml - self.assertEqual(b2s(results), response) + + json_dict = json.loads(b2s(results)) + response= ("Content type 'jon' requested in URL is not available.\n" + "Acceptable types: json%s\n") % includexml + self.assertEqual(json_dict['error']['msg'], response) # TEST #9 # GET: test that version can be set with accept: @@ -2457,7 +2586,8 @@ "/rest/data/issue/1", form) print("9a: " + b2s(results)) json_dict = json.loads(b2s(results)) - self.assertEqual(json_dict['error']['status'], 400) + # note bad @apiver returns 400 not 406. + self.assertEqual(json_dict['error']['status'], 406) self.assertEqual(json_dict['error']['msg'], "Unrecognized api version: L. See /rest without " "specifying api version for supported versions.") @@ -2469,7 +2599,8 @@ "/rest/data/issue/1", form) print("9b: " + b2s(results)) json_dict = json.loads(b2s(results)) - self.assertEqual(json_dict['error']['status'], 400) + self.assertEqual(self.server.client.response_code, 406) + self.assertEqual(json_dict['error']['status'], 406) self.assertEqual(json_dict['error']['msg'], "Unrecognized api version: z. See /rest without " "specifying api version for supported versions.") @@ -2480,9 +2611,9 @@ results = self.server.dispatch('GET', "/rest/data/issue/1", self.empty_form) print("9c:" + b2s(results)) - self.assertEqual(self.server.client.response_code, 400) + self.assertEqual(self.server.client.response_code, 406) json_dict = json.loads(b2s(results)) - self.assertEqual(json_dict['error']['status'], 400) + self.assertEqual(json_dict['error']['status'], 406) self.assertEqual(json_dict['error']['msg'], "Unrecognized api version: z. See /rest without " "specifying api version for supported versions.") @@ -2495,9 +2626,9 @@ results = self.server.dispatch('GET', "/rest/data/issue/1", self.empty_form) print("9d: " + b2s(results)) - self.assertEqual(self.server.client.response_code, 400) + self.assertEqual(self.server.client.response_code, 406) json_dict = json.loads(b2s(results)) - self.assertEqual(json_dict['error']['status'], 400) + self.assertEqual(json_dict['error']['status'], 406) self.assertEqual(json_dict['error']['msg'], "Unrecognized api version: a. See /rest without " "specifying api version for supported versions.") @@ -2728,7 +2859,7 @@ self.empty_form) print(results) json_dict = json.loads(b2s(results)) - self.assertEqual(self.server.client.response_code, 400) + self.assertEqual(self.server.client.response_code, 406) self.assertEqual(self.server.client.additional_headers['Content-Type'], "application/json") self.assertEqual(json_dict['error']['msg'],