Edgewall Software
Modify

Opened 5 years ago

Closed 5 years ago

#12325 closed defect (fixed)

ValueError: invalid literal for int() with base 10: ')(,.",\'",[--'

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.0.11
Component: general Version:
Severity: normal Keywords:
Cc: Branch:
Release Notes:

Coerce invalid int and bool request arguments to their defaults rather than letting a ValueError or TypeError propagate.

API Changes:
Internal Changes:

Description

2016-01-17 16:55:07,918 Trac[main] ERROR: Internal Server Error: <RequestWithSession "GET '/report?asc=%29%28%2C.%22%2C%27%22%2C%5B--&format=csv'">, referrer None
Traceback (most recent call last):
  File "/srv/trac-hacks.org/pve/lib/python2.7/site-packages/trac/web/main.py", line 554, in _dispatch_request
    dispatcher.dispatch(req)
  File "/srv/trac-hacks.org/pve/lib/python2.7/site-packages/trac/web/main.py", line 247, in dispatch
    resp = chosen_handler.process_request(req)
  File "/srv/trac-hacks.org/pve/lib/python2.7/site-packages/trac/ticket/report.py", line 176, in process_request
    template, data, content_type = self._render_list(req)
  File "/srv/trac-hacks.org/pve/lib/python2.7/site-packages/trac/ticket/report.py", line 296, in _render_list
    asc = bool(int(req.args.get('asc', 1)))
ValueError: invalid literal for int() with base 10: ')(,.",\'",[--'

Proposed fix:

  • trac/ticket/report.py

    diff --git a/trac/ticket/report.py b/trac/ticket/report.py
    index ec94423..958baa1 100644
    a b from trac.db import get_column_names  
    3030from trac.perm import IPermissionRequestor
    3131from trac.resource import Resource, ResourceNotFound
    3232from trac.ticket.api import TicketSystem
    33 from trac.util import as_int, content_disposition
     33from trac.util import as_bool, as_int, content_disposition
    3434from trac.util.datefmt import format_datetime, format_time, from_utimestamp
    3535from trac.util.presentation import Paginator
    3636from trac.util.text import exception_to_unicode, to_unicode, quote_query_string
    class ReportModule(Component):  
    293293    def _render_list(self, req):
    294294        """Render the list of available reports."""
    295295        sort = req.args.get('sort', 'report')
    296         asc = bool(int(req.args.get('asc', 1)))
     296        asc = as_bool(req.args.get('asc', 1))
    297297        format = req.args.get('format')
    298298
    299299        rows = self.env.db_query("""

Attachments (0)

Change History (15)

comment:1 by Ryan J Ollos, 5 years ago

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

Fixed in [14485], merged in [14486].

Looks like some test failures, possibly due to Pygments 2.1. I'll investigate and open a new ticket if necessary.

comment:2 by Jun Omae, 5 years ago

report.py seems to still have the same issue.

$ git grep -n '\<bool(int('
trac/ticket/query.py:341:                            val = bool(int(val))
trac/ticket/report.py:398:        asc = bool(int(asc)) # string '0' or '1' to int/boolean
trac/wiki/web_api.py:45:            options['escape_newlines'] = bool(int(req.args['escape_newlines']
trac/wiki/web_api.py:48:            options['shorten'] = bool(int(req.args['shorten'] or 0))

The following lines might have the same issue.

$ git grep -n '\<\(bool\|int\)(req\.args\>'
trac/admin/web_ui.py:469:                anchor = '#no%d' % (int(req.args.get('plugin')) + 1)
trac/search/web_ui.py:160:        noquickjump = int(req.args.get('noquickjump', '0'))
trac/ticket/admin.py:689:                                   str(int(req.args.get(key)))) for key
trac/ticket/report.py:153:        id = int(req.args.get('id', -1))
trac/ticket/report.py:389:        page = int(req.args.get('page', '1'))
trac/ticket/web_ui.py:518:        id = int(req.args.get('id'))
trac/ticket/web_ui.py:551:                cnum = int(req.args.get('cnum'))
trac/ticket/web_ui.py:557:                cnum = int(req.args.get('cnum'))
trac/ticket/web_ui.py:572:                cnum = int(req.args['cnum_edit'])
trac/ticket/web_ui.py:837:        new_version = int(req.args.get('version', 1))
trac/ticket/web_ui.py:838:        old_version = int(req.args.get('old_version', new_version))
trac/ticket/web_ui.py:1034:        new_version = int(req.args.get('version', 1))
trac/ticket/web_ui.py:1035:        old_version = int(req.args.get('old_version', new_version))
trac/timeline/web_ui.py:94:        maxrows = int(req.args.get('max', 50 if format == 'rss' else 0))
trac/versioncontrol/diff.py:335:        context = int(req.args.get('contextlines', pref))
trac/versioncontrol/diff.py:342:    arg = int(req.args.get('contextall', 0))
trac/versioncontrol/web_ui/log.py:87:        limit = int(req.args.get('limit') or self.default_log_limit)
trac/wiki/web_api.py:45:            options['escape_newlines'] = bool(int(req.args['escape_newlines']
trac/wiki/web_api.py:48:            options['shorten'] = bool(int(req.args['shorten'] or 0))
trac/wiki/web_ui.py:266:        version = int(req.args.get('version', 0)) or None
trac/wiki/web_ui.py:267:        old_version = int(req.args.get('old_version', 0)) or version
trac/wiki/web_ui.py:374:            version = int(req.args.get('version', 0))
trac/wiki/web_ui.py:375:        old_version = int(req.args.get('old_version') or 0) or version
trac/wiki/web_ui.py:516:                                version=int(req.args['version']))
tracopt/ticket/deleter.py:126:        id = int(req.args.get('id'))

comment:3 by Ryan J Ollos, 5 years ago

Resolution: fixed
Status: closedreopened

I didn't review the entire module, but it seems like a good idea to fix all possible instances.

comment:4 by Ryan J Ollos, 5 years ago

Status: reopenedassigned

Modified test case in [14505], merged to trunk in [14506].

I used as_int or as_bool for all the cases in which I could produce a traceback. Proposed changes in log:rjollos.git:t12325_value_error and log:rjollos.git:t12325_value_error_trunk.

comment:5 by Christian Boos, 5 years ago

Looks good. However, I wonder if we shouldn't rather go a step further and have:

  • req.args.as_int('blah')raise TracError(_("Invalid request arguments.")) if not OK
  • req.args.as_int('blah', default) → pick default if not OK

Same for .as_bool.

comment:6 by Ryan J Ollos, 5 years ago

That sounds like a good idea. I'll look into implementing the changes on the trunk.

comment:7 by Ryan J Ollos, 5 years ago

Milestone: 1.0.101.0.11

comment:8 by Ryan J Ollos, 5 years ago

Need to check if this issue is fixed, and if not, add to proposed changes:

[pid 8711 140573411886848] 2016-02-09 10:36:57,402 Trac[main] ERROR: Internal Server Error: <RequestWithSession "GET '/wiki/WikiStart?action=diff&version=103&old_version=101.'">, referrer None
Traceback (most recent call last):
  File "/usr/local/virtualenv/1.1dev/lib/python2.7/site-packages/trac/web/main.py", line 607, in _dispatch_request
    dispatcher.dispatch(req)
  File "/usr/local/virtualenv/1.1dev/lib/python2.7/site-packages/trac/web/main.py", line 256, in dispatch
    resp = chosen_handler.process_request(req)
  File "/usr/local/virtualenv/1.1dev/lib/python2.7/site-packages/trac/wiki/web_ui.py", line 180, in process_request
    return self._render_diff(req, versioned_page)
  File "/usr/local/virtualenv/1.1dev/lib/python2.7/site-packages/trac/wiki/web_ui.py", line 431, in _render_diff
    old_version = int(old_version)
ValueError: invalid literal for int() with base 10: '101.'

comment:9 by Ryan J Ollos, 5 years ago

This issue is probably fixed by proposed changes:

[pid 19077 140573411886848] 2016-02-10 21:04:11,539 Trac[main] ERROR: Internal Server Error: <RequestWithSession "GET '/ticket/2182?action=diff&version=93.\xea\xb0\x84\xed\x8a\xb8\xec\xb0\xa8\xed\x8a\xb8\xec\x9a\xa9\xec\x96\xb4\xec\xa0\x95\xeb\xa6\xac\xeb\xb0\x8f\xec\xba\x98\xeb\xa6\xb0\xeb\x8d\x94mainnav\xec\x9e\x98\xeb\xaa\xbb\xed\x91\x9c\xec\x8b\x9c\xeb\x90\x98\xeb\x8a\x94\xeb\xac\xb8\xec\xa0\x9c'">, referrer None
Traceback (most recent call last):
  File "/usr/local/virtualenv/1.1dev/lib/python2.7/site-packages/trac/web/main.py", line 607, in _dispatch_request
    dispatcher.dispatch(req)
  File "/usr/local/virtualenv/1.1dev/lib/python2.7/site-packages/trac/web/main.py", line 256, in dispatch
    resp = chosen_handler.process_request(req)
  File "/usr/local/virtualenv/1.1dev/lib/python2.7/site-packages/trac/ticket/web_ui.py", line 177, in process_request
    return self._process_ticket_request(req)
  File "/usr/local/virtualenv/1.1dev/lib/python2.7/site-packages/trac/ticket/web_ui.py", line 592, in _process_ticket_request
    return self._render_diff(req, ticket, data, text_fields)
  File "/usr/local/virtualenv/1.1dev/lib/python2.7/site-packages/trac/ticket/web_ui.py", line 896, in _render_diff
    new_version = int(req.args.get('version', 1))
UnicodeEncodeError: 'decimal' codec can't encode characters in position 3-14: invalid decimal Unicode string
Traceback (most recent call last):
  File "/usr/local/virtualenv/1.0-stable/lib/python2.7/site-packages/trac/web/main.py", line 554, in _dispatch_request
    dispatcher.dispatch(req)
  File "/usr/local/virtualenv/1.0-stable/lib/python2.7/site-packages/trac/web/main.py", line 247, in dispatch
    resp = chosen_handler.process_request(req)
  File "/usr/local/virtualenv/1.0-stable/lib/python2.7/site-packages/trac/timeline/web_ui.py", line 94, in process_request
    maxrows = int(req.args.get('max', 50 if format == 'rss' else 0))
UnicodeEncodeError: 'decimal' codec can't encode character u'\uff1c' in position 8: invalid decimal Unicode string
2016-03-14 23:18:12,177 Trac[main] ERROR: Internal Server Error: <RequestWithSession "GET '/timeline?authors=1&changeset=on&daysback=90&format=rss&max=50%3C%00ScRiPt%20%0d%0a%3Eprompt(943803)%3C/ScRiPt%3E&repo-trac.git=on&repo-trac.hg=on&wiki=on'">, referrer 'http://trac.edgewall.org/'
Traceback (most recent call last):
  File "build/bdist.linux-x86_64/egg/trac/web/main.py", line 607, in _dispatch_request
    dispatcher.dispatch(req)
  File "build/bdist.linux-x86_64/egg/trac/web/main.py", line 256, in dispatch
    resp = chosen_handler.process_request(req)
  File "build/bdist.linux-x86_64/egg/trac/timeline/web_ui.py", line 95, in process_request
    maxrows = int(req.args.get('max', 50 if format == 'rss' else 0))
UnicodeEncodeError: 'decimal' codec can't encode character u'\x00' in position 3: invalid decimal Unicode string
Last edited 5 years ago by Ryan J Ollos (previous) (diff)

comment:10 by Ryan J Ollos, 5 years ago

This issue will be fixed with the proposed changes:

2016-03-04 17:15:37,497 Trac[main] ERROR: Internal Server Error: <RequestWithSession "GET "/search?q=leaks&noquickjump=1'A=0&ticket=on"">, referrer "https://dev.jquery.com/search?q=leaks&noquickjump=1'A=0&ticket=on"
Traceback (most recent call last):
  File "/var/www/bugs.jquery.com/private/pve/local/lib/python2.7/site-packages/trac/web/main.py", line 554, in _dispatch_request
    dispatcher.dispatch(req)
  File "/var/www/bugs.jquery.com/private/pve/local/lib/python2.7/site-packages/trac/web/main.py", line 247, in dispatch
    resp = chosen_handler.process_request(req)
  File "/var/www/bugs.jquery.com/private/pve/local/lib/python2.7/site-packages/trac/search/web_ui.py", line 100, in process_request
    data['quickjump'] = self._check_quickjump(req, query)
  File "/var/www/bugs.jquery.com/private/pve/local/lib/python2.7/site-packages/trac/search/web_ui.py", line 160, in _check_quickjump
    noquickjump = int(req.args.get('noquickjump', '0'))
ValueError: invalid literal for int() with base 10: "1'A=0"

comment:11 by Ryan J Ollos, 5 years ago

Previously we used the pattern as_int(req.get('key', default1), default2)), where default1 is returned if 'key' is not in the dict, and default2 is returned if an exception is raised converting the value of 'key'.

req.as_int('key', default) doesn't quite have the same flexibility. default will be returned when the key is not in the dict and if an exception is raised converting the value of 'key'. If you don't specify a default you'll get an exception for both scenarios.

The only way I can see the reconcile the difference would be to have an on_error argument, and have HTTPBadRequest raised if on_error isn't specified.

Return '1' if exception is raised converting value of arg1:

>>> req.args.as_int('arg1', default='1', on_error='1')

Raise HTTPBadRequest if exception is raised converting value of arg1:

>>> req.args.as_int('arg1', default='1')

in reply to:  11 ; comment:12 by Ryan J Ollos, 5 years ago

Replying to Ryan J Ollos:

If you don't specify a default you'll get an exception for both scenarios.

On second thought, this doesn't necessarily need to be true. We could return the default when the key is not in dict.

I'm thinking that as_int and as_bool should just raise an HTTPBadRequest whenever an exception is raised converting the input parameter.

Initial changes committed to 1.0-stable in [14663], merged to trunk in [14664].

comment:13 by Ryan J Ollos, 5 years ago

Release Notes: modified (diff)

in reply to:  12 comment:14 by Ryan J Ollos, 5 years ago

Replying to Ryan J Ollos:

Replying to Ryan J Ollos:

If you don't specify a default you'll get an exception for both scenarios.

On second thought, this doesn't necessarily need to be true. We could return the default when the key is not in dict.

I think this is the key point that should be further considered. There are scenarios when we may want to return default when the value can't be converted, and there are scenarios when we may want to raise HTTPBadRequest when the value can't be converted. In both scenarios we probably want the typical behavior of returning default when the key is not in the dict. I created #12426 to discuss.

comment:15 by Ryan J Ollos, 5 years ago

Resolution: fixed
Status: assignedclosed

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos 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.