Edgewall Software
Modify

Opened 2 years ago

Closed 2 years ago

#13168 closed defect (fixed)

Query._get_constraints() should not modify a list in req.args

Reported by: Jun Omae Owned by: Jun Omae
Priority: normal Milestone: 1.0.18
Component: query system Version:
Severity: normal Keywords:
Cc: Branch:
Release Notes:

Avoid modification of a list in req.args in Query module.

API Changes:
Internal Changes:

Description (last modified by Jun Omae)

I sometimes encounter IndexErrors from Query._get_constraints() on our production environment with private plugin which calls Query._get_constraints().

ERROR: Internal Server Error: <RequestWithSession "POST '/query'">, referrer 'https://example.org/query'
Traceback (most recent call last):
  File "/venv/lib/python2.7/site-packages/trac/web/main.py", line 554, in _dispatch_request
    dispatcher.dispatch(req)
  File "/venv/lib/python2.7/site-packages/trac/web/main.py", line 257, in dispatch
    self._post_process_request(req, *resp)
  File "/venv/lib/python2.7/site-packages/trac/web/main.py", line 388, in _post_process_request
    resp = f.post_process_request(req, *resp)
  File "/vol/releases/20190322024230/share/plugins/XxxPlugin.egg/xxx/project/ticket/custom_field.py", line 626, in post_process_request
    if not mod._get_constraints(req) and 'order' not in req.args:
  File "/venv/lib/python2.7/site-packages/trac/ticket/query.py", line 1066, in _get_constraints
    del vals[idx]
IndexError: list assignment index out of range

Investigating the issue, it is caused by that Query._get_constraints() modifies a list in req.args and is called twice when a condition is removed.

I don't consider the method should modify a list in req.args.

[9f904f1df/jomae.git]:

  • trac/ticket/query.py

    diff --git a/trac/ticket/query.py b/trac/ticket/query.py
    index 3fbe171f1..80ca3c054 100644
    a b class QueryModule(Component):  
    10441044            # requested for clients without JavaScript
    10451045            add_num = None
    10461046            constraints = {}
    1047             for k, vals in req.args.iteritems():
     1047            for k in req.args:
    10481048                match = self.add_re.match(k)
    10491049                if match:
    10501050                    add_num = match.group(1)
    class QueryModule(Component):  
    10561056                clause_num = int(match.group('clause'))
    10571057                if field not in fields:
    10581058                    continue
    1059                 if not isinstance(vals, (list, tuple)):
    1060                     vals = [vals]
     1059                # use list() to avoid modification of a list in req.args
     1060                vals = list(req.args.getlist(k))
    10611061                if vals:
    10621062                    mode = req.args.get(k + '_mode')
    10631063                    if mode:
  • trac/ticket/tests/query.py

    diff --git a/trac/ticket/tests/query.py b/trac/ticket/tests/query.py
    index cb2d0fc6e..e8cce442c 100644
    a b ORDER BY COALESCE(c.%(ticket)s,'')='',c.%(ticket)s,t.id""" % quoted)  
    11081108        prop = req.chrome['script_data']['properties']['milestone']
    11091109        self.assertEqual({'label': 'Milestone', 'type': 'text'}, prop)
    11101110
     1111    def test_get_constraints_keep_req_args(self):
     1112        arg_list = (('0_type', 'defect'), ('0_type', 'task'),
     1113                    ('0_type', 'enhancement'), ('rm_filter_0_type_1', '-'),
     1114                    ('1_type', 'task'))
     1115        req = MockRequest(self.env, method='POST', path_info='/query',
     1116                          arg_list=arg_list)
     1117        orig_args = arg_list_to_args(arg_list)
     1118        mod = QueryModule(self.env)
     1119        self.assertTrue(mod.match_request(req))
     1120        template, data, content_type = mod.process_request(req)
     1121        self.assertEqual([{'type': ['defect', 'enhancement']},
     1122                          {'type': ['task']}],
     1123                         data['query'].constraints)
     1124        self.assertEqual(orig_args, req.args)
     1125
    11111126
    11121127class QueryLinksTestCase(unittest.TestCase):
    11131128

Attachments (0)

Change History (4)

comment:1 by Jun Omae, 2 years ago

Description: modified (diff)

comment:2 by Ryan J Ollos, 2 years ago

The change looks good to me.

comment:3 by Jun Omae, 2 years ago

Owner: set to Jun Omae
Status: newassigned

Thanks for the reviewing. I'll push the change.

comment:4 by Jun Omae, 2 years ago

Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Committed in [16922] and merged in [16923,16924].

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Jun Omae.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Jun Omae to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.