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