From a32c57e3f722f6344b16d9def838ebcce77d2de8 Mon Sep 17 00:00:00 2001 From: John Rouillard Date: Tue, 17 Dec 2024 19:42:46 -0500 Subject: [PATCH] fix(REST): issue2551383; improve errors for bad json, fix PUT docs While adding fuzz testing for email addresses via REST /rest/data/user/1/address, I had an error when setting the address to the same value it currently had. Traced this to a bug in userauditor.py. Fixed the bug. Documented in upgrading.txt. While trying to track down issue, I realized invalid json was being accepted without error. So I fixed the code that parses the json and have it return an error. Also modified some tests that broke (used invalid json, or passed body (e.g. DELETE) but shouldn't have. Add tests for bad json to verify new code. Fixed test that wasn't initializing the body_file in each loop, so the test wasn't actually supplying a body. Also realised PUT documentation was not correct. Output format isn't quite like GET. Fuss tests for email address also added. --- CHANGES.txt | 11 ++- doc/rest.txt | 40 +++++++- doc/upgrading.txt | 26 +++++ roundup/rest.py | 18 +++- .../classic/detectors/userauditor.py | 4 + .../templates/devel/detectors/userauditor.py | 4 + .../templates/jinja2/detectors/userauditor.py | 4 + .../minimal/detectors/userauditor.py | 4 + .../responsive/detectors/userauditor.py | 4 + test/rest_common.py | 97 +++++++++++++++++-- test/test_liveserver.py | 37 ++++++- 11 files changed, 227 insertions(+), 22 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index f5f87cab5..746ac31cd 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -43,6 +43,12 @@ Fixed: incorrectly. (John Rouillard) - issue2551382 - invalid @verbose, @page_* values in rest uri's generate 409 not 400 error. (John Rouillard) +- fix issues with rest doc and use of PUT on a property item. Response + is similar to use of PUT on the item, not a GET on the + item. Discovered while fuzz testing. (John Rouillard) +- issue2551383 - Setting same address via REST PUT command results in + an error. Now the userauditor does not trigger an error if a user + sets the primary address to the existing value. (John Rouillard) Features: @@ -71,7 +77,10 @@ Features: endpoint. Raw file/msg data can be retrieved using the /binary_content attribute and an Accept header to select the mime type for the data (e.g. image/png for a png file). The existing html - interface method still works and is supported, but is legacy. + interface method still works and is supported, but is legacy. (John + Rouillard) +- added fuzz testing for some code. Found issue2551382 and + others. (John Rouillard) 2024-07-13 2.4.0 diff --git a/doc/rest.txt b/doc/rest.txt index 1befc8ed4..76af6e1fa 100644 --- a/doc/rest.txt +++ b/doc/rest.txt @@ -1392,10 +1392,12 @@ Other Supported Methods for Items ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ The method ``PUT`` is allowed on individual items, e.g. -``/data/issue/42`` On success it returns the same parameters as the -respective ``GET`` method. Note that for ``PUT`` an Etag has to be -supplied, either in the request header or as an @etag parameter. An -example:: +``/data/issue/42`` On success it returns a data structure similar to +the respective ``GET`` method. However it is only concerned with the +changes that have occurred. Since it is not a full ``GET`` request, it +doesn't include an etag or unchanged attributes. Note that for ``PUT`` +an Etag has to be supplied, either in the request header or as an +@etag parameter. An example:: curl -u admin:admin -X PUT \ --header 'Referer: https://example.com/demo/' \ @@ -1615,7 +1617,22 @@ Other Supported Methods for fields The method ``PUT`` is allowed on a property e.g., ``/data/issue/42/title``. On success it returns the same parameters as -the respective ``GET`` method. Note that for ``PUT`` an Etag has to be +the respective ``PUT`` method on the item. For example:: + + { + "data": { + "id": "42", + "type": "issue", + "link": "https://.../demo/rest/data/issue/42", + "attribute": { + "title": "this is a new title" + } + } + } + +If the new value for the title was the same as on the server, the +attribute property would be empty since the value was not changed. +Note that for ``PUT`` an Etag has to be supplied, either in the request header or as an @etag parameter. Example using multipart/form-data rather than json:: @@ -2438,3 +2455,16 @@ Rate limit tests:: will show you the number of remaining requests to the REST interface for the user identified by password. + + +Notes V2 API +~~~~~~~~~~~~ + +These may never be implemented but, some nits to consider. + +The shape of a GET and PUT/PATCH responses are different. "attributes" +is used for GET and "attribute" is used with PATCH/PUT. A PATCH or a +PUT can update multiple properties when used with an item endpoint. +"attribute" kind of makes sense when used with a property endpoint +but.... Maybe standardize on one shape so the client doesn't have to +special case? diff --git a/doc/upgrading.txt b/doc/upgrading.txt index 8439ac54e..729098dd0 100644 --- a/doc/upgrading.txt +++ b/doc/upgrading.txt @@ -133,6 +133,32 @@ to:: at the top of both files. The icing macro used in other tracker templates was renamed to frame in this tracker template. +Update userauditor.py detector (recommended) +-------------------------------------------- + +When using the REST interface, setting the address property of the +user to the same value it currently has resulted in an error. + +If you have not changed your userauditor, you can copy one from any of +the supplied templates in the ``detectors/userauditor.py`` file. Use +``roundup-admin templates`` to find a list of template directories. + +If you have changed your userauditor from the stock version, apply the +following diff:: + + raise ValueError('Email address syntax is invalid + "%s"'%address) + + check_main = db.user.stringFind(address=address) + + # allow user to set same address via rest + + if check_main: + + check_main = nodeid not in check_main + + + # make sure none of the alts are owned by anyone other than us (x!=nodeid) + +add the lines marked with ``+`` in the file in the location after +check_main is assigned. + More secure session cookie handling (info) ------------------------------------------ diff --git a/roundup/rest.py b/roundup/rest.py index 771eb9760..1a9b94b22 100644 --- a/roundup/rest.py +++ b/roundup/rest.py @@ -2729,17 +2729,27 @@ class SimulateFieldStorageFromJson(): ''' def __init__(self, json_string): - ''' Parse the json string into an internal dict. ''' + '''Parse the json string into an internal dict. + + Because spec for rest post once exactly (POE) shows + posting empty content. An empty string results in an empty + dict, not a json parse error. + ''' def raise_error_on_constant(x): raise ValueError("Unacceptable number: %s" % x) + if json_string == "": + self.json_dict = {} + self.value = None + return + try: self.json_dict = json.loads(json_string, parse_constant=raise_error_on_constant) self.value = [self.FsValue(index, self.json_dict[index]) for index in self.json_dict] - except ValueError: - self.json_dict = {} - self.value = None + except (json.decoder.JSONDecodeError, ValueError) as e: + raise ValueError(e.args[0] + ". JSON is: " + json_string) + class FsValue: '''Class that does nothing but response to a .value property ''' diff --git a/share/roundup/templates/classic/detectors/userauditor.py b/share/roundup/templates/classic/detectors/userauditor.py index 562dce7f6..d33ca0dc1 100644 --- a/share/roundup/templates/classic/detectors/userauditor.py +++ b/share/roundup/templates/classic/detectors/userauditor.py @@ -68,6 +68,10 @@ def audit_user_fields(db, cl, nodeid, newvalues): raise ValueError('Email address syntax is invalid "%s"'%address) check_main = db.user.stringFind(address=address) + # allow user to set same address via rest + if check_main: + check_main = nodeid not in check_main + # make sure none of the alts are owned by anyone other than us (x!=nodeid) check_alts = [x for x in db.user.filter(None, {'alternate_addresses' : address}) if x != nodeid] if check_main or check_alts: diff --git a/share/roundup/templates/devel/detectors/userauditor.py b/share/roundup/templates/devel/detectors/userauditor.py index 562dce7f6..d33ca0dc1 100644 --- a/share/roundup/templates/devel/detectors/userauditor.py +++ b/share/roundup/templates/devel/detectors/userauditor.py @@ -68,6 +68,10 @@ def audit_user_fields(db, cl, nodeid, newvalues): raise ValueError('Email address syntax is invalid "%s"'%address) check_main = db.user.stringFind(address=address) + # allow user to set same address via rest + if check_main: + check_main = nodeid not in check_main + # make sure none of the alts are owned by anyone other than us (x!=nodeid) check_alts = [x for x in db.user.filter(None, {'alternate_addresses' : address}) if x != nodeid] if check_main or check_alts: diff --git a/share/roundup/templates/jinja2/detectors/userauditor.py b/share/roundup/templates/jinja2/detectors/userauditor.py index 562dce7f6..d33ca0dc1 100644 --- a/share/roundup/templates/jinja2/detectors/userauditor.py +++ b/share/roundup/templates/jinja2/detectors/userauditor.py @@ -68,6 +68,10 @@ def audit_user_fields(db, cl, nodeid, newvalues): raise ValueError('Email address syntax is invalid "%s"'%address) check_main = db.user.stringFind(address=address) + # allow user to set same address via rest + if check_main: + check_main = nodeid not in check_main + # make sure none of the alts are owned by anyone other than us (x!=nodeid) check_alts = [x for x in db.user.filter(None, {'alternate_addresses' : address}) if x != nodeid] if check_main or check_alts: diff --git a/share/roundup/templates/minimal/detectors/userauditor.py b/share/roundup/templates/minimal/detectors/userauditor.py index 562dce7f6..d33ca0dc1 100644 --- a/share/roundup/templates/minimal/detectors/userauditor.py +++ b/share/roundup/templates/minimal/detectors/userauditor.py @@ -68,6 +68,10 @@ def audit_user_fields(db, cl, nodeid, newvalues): raise ValueError('Email address syntax is invalid "%s"'%address) check_main = db.user.stringFind(address=address) + # allow user to set same address via rest + if check_main: + check_main = nodeid not in check_main + # make sure none of the alts are owned by anyone other than us (x!=nodeid) check_alts = [x for x in db.user.filter(None, {'alternate_addresses' : address}) if x != nodeid] if check_main or check_alts: diff --git a/share/roundup/templates/responsive/detectors/userauditor.py b/share/roundup/templates/responsive/detectors/userauditor.py index 562dce7f6..d33ca0dc1 100644 --- a/share/roundup/templates/responsive/detectors/userauditor.py +++ b/share/roundup/templates/responsive/detectors/userauditor.py @@ -68,6 +68,10 @@ def audit_user_fields(db, cl, nodeid, newvalues): raise ValueError('Email address syntax is invalid "%s"'%address) check_main = db.user.stringFind(address=address) + # allow user to set same address via rest + if check_main: + check_main = nodeid not in check_main + # make sure none of the alts are owned by anyone other than us (x!=nodeid) check_alts = [x for x in db.user.filter(None, {'alternate_addresses' : address}) if x != nodeid] if check_main or check_alts: diff --git a/test/rest_common.py b/test/rest_common.py index 522fddda1..3ab3484ef 100644 --- a/test/rest_common.py +++ b/test/rest_common.py @@ -2427,6 +2427,89 @@ def testDispatchBadAccept(self): json_dict = json.loads(b2s(results)) self.assertIn('Unable to parse Accept Header. Invalid param: foo. Acceptable types: */*, application/json', json_dict['error']['msg']) + def testBadJson(self): + '''Run some JSON we don't accept through the wringer + ''' + body=b'{ "title": "Joe Doe has problems", \ + "nosy": [ "1", "3" ], \ + "assignedto": "2", \ + "abool": true, \ + "afloat": 2.3, \ + "anint": Infinity }' + + expected={ "error": + {"status": 400, + "msg": ("Unacceptable number: Infinity. JSON is: " + + b2s(body)), + } + } + + env = { "CONTENT_TYPE": "application/json", + "CONTENT_LENGTH": len(body), + "REQUEST_METHOD": "PUT" + } + 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 + results = self.server.dispatch(env["REQUEST_METHOD"], + "/rest/data/issue/1", + form) + + self.assertEqual(json.loads(results), expected) + + body=b'{ "title": "Joe Doe has problems", \ + nosy: [ "1", "3" ], \ + "assignedto": "2", \ + "abool": true, \ + "afloat": 2.3, \ + "anint": Infinity }' + self.maxDiff = None + expected={ "error": + {"status": 400, + "msg": ("Expecting property name enclosed in double " + "quotes: line 1 column 53 (char 52). JSON is: " + + b2s(body)), + } + } + + env = { "CONTENT_TYPE": "application/json", + "CONTENT_LENGTH": len(body), + "REQUEST_METHOD": "PUT" + } + 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 + results = self.server.dispatch(env["REQUEST_METHOD"], + "/rest/data/issue/1", + form) + + self.assertEqual(json.loads(results), expected) + + def testStatsGen(self): # check stats being returned by put and get ops # using dispatch which parses the @stats query param @@ -2835,27 +2918,20 @@ def testDispatch(self): etag = calculate_etag(self.db.issue.getnode("1"), self.db.config['WEB_SECRET_KEY']) etagb = etag.strip ('"') - env = {"CONTENT_TYPE": "application/json", - "CONTENT_LEN": 0, - "REQUEST_METHOD": "DELETE" } + env = {"REQUEST_METHOD": "DELETE" } self.server.client.env.update(env) # use text/plain header and request json output by appending # .json to the url. headers={"accept": "text/plain", - "content-type": env['CONTENT_TYPE'], "if-match": '"%s"'%etagb, "content-length": 0, } self.headers=headers - body_file=BytesIO(b'') # FieldStorage needs a file - form = client.BinaryFieldStorage(body_file, - headers=headers, - environ=env) self.server.client.request.headers.get=self.get_header self.db.setCurrentUser('admin') # must be admin to delete issue results = self.server.dispatch('DELETE', "/rest/data/issue/1.json", - form) + self.empty_form) self.assertEqual(self.server.client.response_code, 200) print(results) json_dict = json.loads(b2s(results)) @@ -3213,7 +3289,6 @@ def testMethodOverride(self): "CONTENT_LENGTH": len(body), "REQUEST_METHOD": "POST" } - body_file=BytesIO(body) # FieldStorage needs a file self.server.client.request.headers.get=self.get_header for method in ( "GET", "PUT", "PATCH" ): headers={"accept": "application/json; version=1", @@ -3221,6 +3296,8 @@ def testMethodOverride(self): "content-length": len(body), "x-http-method-override": "DElETE", } + body_file=BytesIO(body) # FieldStorage needs a file + self.headers=headers form = client.BinaryFieldStorage(body_file, headers=headers, diff --git a/test/test_liveserver.py b/test/test_liveserver.py index 6e6284f47..ebc85912f 100644 --- a/test/test_liveserver.py +++ b/test/test_liveserver.py @@ -216,7 +216,7 @@ class FuzzGetUrls(WsgiSetup, ClientSetup): @example("@verbose", "1#") @example("@verbose", "#1stuff") @settings(max_examples=_max_examples, - deadline=10000) # 10000ms + deadline=10000) # in ms def test_class_url_param_accepting_integer_values(self, param, value): """Tests all integer args for rest url. @page_* is the same code for all *. @@ -244,7 +244,7 @@ def test_class_url_param_accepting_integer_values(self, param, value): @example("@verbose", "10#") @example("@verbose", u'Ø\U000dd990') @settings(max_examples=_max_examples, - deadline=10000) # 10000ms + deadline=10000) # in ms def test_element_url_param_accepting_integer_values(self, param, value): """Tests args accepting int for rest url. """ @@ -267,6 +267,39 @@ def test_element_url_param_accepting_integer_values(self, param, value): # invalid value for param self.assertEqual(f.status_code, 400) + @given(emails()) + @settings(max_examples=_max_examples, + deadline=10000) # in ms + def test_email_param(self,email): + session, _response = self.create_login_session() + url = '%s/rest/data/user/1/address' % (self.url_base()) + headers = {"Accept": "application/json", + "Content-Type": "application/json", + "x-requested-with": "rest", + "Origin": self.url_base(), + "Referer": self.url_base() + } + + #--header 'If-Match: "e2e6cc43c3475a4a3d9e5343617c11c3"' \ + + f = session.get(url) + stored_email = f.json()['data']['data'] + headers['If-Match'] = f.headers['etag'] + + payload = {'data': email} + f = session.put(url, json=payload, headers=headers) + + self.assertEqual(f.status_code, 200) + + if stored_email == email: + # if the email we are setting is the same as present, we + # don't make a change so the attribute dict is empty aka false. + self.assertEqual(f.json()['data']['attribute'], {}) + else: + self.assertEqual(f.json()['data']['attribute']['address'], + email) + + @skip_requests class BaseTestCases(WsgiSetup, ClientSetup): """Class with all tests to run against wsgi server. Is reused when