Mercurial > p > roundup > code
changeset 8218:32aaf5dc562b
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.
| author | John Rouillard <rouilj@ieee.org> |
|---|---|
| date | Tue, 17 Dec 2024 19:42:46 -0500 |
| parents | cd76d5d59c37 |
| children | 9404d56d830f |
| files | CHANGES.txt doc/rest.txt doc/upgrading.txt roundup/rest.py share/roundup/templates/classic/detectors/userauditor.py share/roundup/templates/devel/detectors/userauditor.py share/roundup/templates/jinja2/detectors/userauditor.py share/roundup/templates/minimal/detectors/userauditor.py share/roundup/templates/responsive/detectors/userauditor.py test/rest_common.py test/test_liveserver.py |
| diffstat | 11 files changed, 227 insertions(+), 22 deletions(-) [+] |
line wrap: on
line diff
--- a/CHANGES.txt Mon Dec 16 21:29:07 2024 -0500 +++ b/CHANGES.txt Tue Dec 17 19:42:46 2024 -0500 @@ -43,6 +43,12 @@ 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 @@ 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
--- a/doc/rest.txt Mon Dec 16 21:29:07 2024 -0500 +++ b/doc/rest.txt Tue Dec 17 19:42:46 2024 -0500 @@ -1392,10 +1392,12 @@ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 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 @@ 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 @@ 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?
--- a/doc/upgrading.txt Mon Dec 16 21:29:07 2024 -0500 +++ b/doc/upgrading.txt Tue Dec 17 19:42:46 2024 -0500 @@ -133,6 +133,32 @@ 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) ------------------------------------------
--- a/roundup/rest.py Mon Dec 16 21:29:07 2024 -0500 +++ b/roundup/rest.py Tue Dec 17 19:42:46 2024 -0500 @@ -2729,17 +2729,27 @@ ''' 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 '''
--- a/share/roundup/templates/classic/detectors/userauditor.py Mon Dec 16 21:29:07 2024 -0500 +++ b/share/roundup/templates/classic/detectors/userauditor.py Tue Dec 17 19:42:46 2024 -0500 @@ -68,6 +68,10 @@ 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:
--- a/share/roundup/templates/devel/detectors/userauditor.py Mon Dec 16 21:29:07 2024 -0500 +++ b/share/roundup/templates/devel/detectors/userauditor.py Tue Dec 17 19:42:46 2024 -0500 @@ -68,6 +68,10 @@ 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:
--- a/share/roundup/templates/jinja2/detectors/userauditor.py Mon Dec 16 21:29:07 2024 -0500 +++ b/share/roundup/templates/jinja2/detectors/userauditor.py Tue Dec 17 19:42:46 2024 -0500 @@ -68,6 +68,10 @@ 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:
--- a/share/roundup/templates/minimal/detectors/userauditor.py Mon Dec 16 21:29:07 2024 -0500 +++ b/share/roundup/templates/minimal/detectors/userauditor.py Tue Dec 17 19:42:46 2024 -0500 @@ -68,6 +68,10 @@ 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:
--- a/share/roundup/templates/responsive/detectors/userauditor.py Mon Dec 16 21:29:07 2024 -0500 +++ b/share/roundup/templates/responsive/detectors/userauditor.py Tue Dec 17 19:42:46 2024 -0500 @@ -68,6 +68,10 @@ 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:
--- a/test/rest_common.py Mon Dec 16 21:29:07 2024 -0500 +++ b/test/rest_common.py Tue Dec 17 19:42:46 2024 -0500 @@ -2427,6 +2427,89 @@ 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 @@ 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 @@ "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 @@ "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,
--- a/test/test_liveserver.py Mon Dec 16 21:29:07 2024 -0500 +++ b/test/test_liveserver.py Tue Dec 17 19:42:46 2024 -0500 @@ -216,7 +216,7 @@ @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 @@ @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 @@ # 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