Opened 3 years ago

Closed 3 years ago

#32904 closed Bug (fixed)

Tighten up the regular expression used by parse_time to accept less 'invalid' options.

Reported by: Keryn Knight Owned by: Abhyudai
Component: Utilities Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

As per discussion in the ticket #32892 and on the Github comments for same, currently the time_re allows for some variations which it arguably shouldn't.

For the historical record's sake, the current regex is: (?P<hour>\d{1,2}):(?P<minute>\d{1,2})(?::(?P<second>\d{1,2})(?:[\.,](?P<microsecond>\d{1,6})\d{0,6})?)? where you can see a whole lot of it ends up optional, and there are some ways in which that can be made to accept what we'd probably call 'invalid' (though strictly speaking the result is correct for the input portions):

>>> from django.utils.dateparse import parse_time
>>> parse_time('0:5: ')
datetime.time(0, 5)

If possible, we should derive examples of which strings might current pass and decide which, if any of them, shouldn't be accepted. It's probably also fine to leave the whole thing as-is (be liberal in what you accept etc) and just add them as necessary to the examples of valid inputs, so in future it doesn't come up again beyond "thats just an accepted quirk"

Change History (8)

comment:1 by Keryn Knight, 3 years ago

Here, for example, is one which parses into a datetime.time but I wouldn't really expect it to, and whilst the input is nonsense, doesn't cause an error like ValueError: second must be in 0..59 which would match my expectations:

>>> from django.utils.dateparse import parse_time
>>> parse_time('4:18:101')
datetime.time(4, 18, 10)
# captured data was {'hour': '4', 'minute': '18', 'second': '10', 'microsecond': None}

comment:2 by Mariusz Felisiak, 3 years ago

Triage Stage: UnreviewedAccepted

IMO the main issue is that $ is missing.

comment:3 by Mariusz Felisiak, 3 years ago

Type: Cleanup/optimizationBug

in reply to:  2 comment:4 by Nick Pope, 3 years ago

Replying to Mariusz Felisiak:

IMO the main issue is that $ is missing.

I came to the same conclusion: https://github.com/django/django/pull/14582#discussion_r664075337

comment:5 by Abhyudai, 3 years ago

Owner: changed from nobody to Abhyudai
Status: newassigned

comment:6 by Abhyudai, 3 years ago

Has patch: set

comment:7 by Mariusz Felisiak, 3 years ago

Triage Stage: AcceptedReady for checkin

comment:8 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In cf6774a5:

Fixed #32904 -- Made parse_time() more strict.

Thanks Keryn Knight for the report.

Note: See TracTickets for help on using tickets.
Back to Top