fix: reject stray pipe in sign by using [+-] character class
Inside a character class '|' is a literal, so [+|-] matched '+', '-', or '|'. A leading pipe was captured as a sign and stripped, letting malformed input like '|5m' parse to 300 instead of returning None. Restrict the class to the two intended sign characters.
This commit is contained in:
1 parent
81c649efa1
commit
8db541c87e
3 files changed
+63
-1
No files matched your search
@@ -0,0 +1,58 @@
|
|||||||
|
# Fix sign character class accidentally matching a pipe `|`
|
||||||
|
|
||||||
|
## Problem
|
||||||
|
|
||||||
|
`timeparse` accepts a leading pipe `|` as though it were a valid sign and
|
||||||
|
silently parses the rest of the expression, instead of rejecting the malformed
|
||||||
|
input with `None`:
|
||||||
|
|
||||||
|
```python
|
||||||
|
>>> from pytimeparse import parse
|
||||||
|
>>> parse('|5m')
|
||||||
|
300 # expected None
|
||||||
|
>>> parse('|1:24')
|
||||||
|
84 # expected None
|
||||||
|
```
|
||||||
|
|
||||||
|
## Root cause
|
||||||
|
|
||||||
|
`pytimeparse/timeparse.py`:
|
||||||
|
|
||||||
|
```python
|
||||||
|
SIGN = r'(?P<sign>[+|-])?'
|
||||||
|
```
|
||||||
|
|
||||||
|
Inside a character class, `|` is a literal character, not the alternation
|
||||||
|
operator. So `[+|-]` matches `+`, `-`, or `|`. When the input starts with `|`,
|
||||||
|
the `sign` group captures it, the pipe is stripped from the string, and the
|
||||||
|
remainder is parsed as if no sign were present (`sign` resolves to `1` because
|
||||||
|
`'|' != '-'`).
|
||||||
|
|
||||||
|
## Fix
|
||||||
|
|
||||||
|
Use a character class containing only the two intended sign characters:
|
||||||
|
|
||||||
|
```python
|
||||||
|
SIGN = r'(?P<sign>[+-])?'
|
||||||
|
```
|
||||||
|
|
||||||
|
Now a leading `|` is not consumed as a sign; the pipe stays in the string, no
|
||||||
|
time-format pattern matches it, and `timeparse` returns `None` as it does for
|
||||||
|
any other unparseable input. Valid `+`/`-` signs are unaffected.
|
||||||
|
|
||||||
|
## Test
|
||||||
|
|
||||||
|
Extended `test_timeparse_signs` in `pytimeparse/tests/testtimeparse.py`:
|
||||||
|
|
||||||
|
```python
|
||||||
|
self.assertIsNone(timeparse.timeparse('|5m'))
|
||||||
|
self.assertIsNone(timeparse.timeparse('| 5m'))
|
||||||
|
self.assertIsNone(timeparse.timeparse('|1:24'))
|
||||||
|
```
|
||||||
|
|
||||||
|
## Gate
|
||||||
|
|
||||||
|
- `python -m pytest pytimeparse/tests/testtimeparse.py` -> 49 passed.
|
||||||
|
|
||||||
|
Counterfactual (fix reverted, test kept): fails with
|
||||||
|
`AssertionError: 300 is not None`.
|
||||||
@@ -83,6 +83,10 @@ class TestTimeparse(unittest.TestCase):
|
|||||||
self.assertEqual(timeparse.timeparse('- 32 m 1 s'), -1921)
|
self.assertEqual(timeparse.timeparse('- 32 m 1 s'), -1921)
|
||||||
self.assertIsNone(timeparse.timeparse('32 m - 1 s'))
|
self.assertIsNone(timeparse.timeparse('32 m - 1 s'))
|
||||||
self.assertIsNone(timeparse.timeparse('32 m + 1 s'))
|
self.assertIsNone(timeparse.timeparse('32 m + 1 s'))
|
||||||
|
# A stray pipe is not a valid sign and must not be silently accepted.
|
||||||
|
self.assertIsNone(timeparse.timeparse('|5m'))
|
||||||
|
self.assertIsNone(timeparse.timeparse('| 5m'))
|
||||||
|
self.assertIsNone(timeparse.timeparse('|1:24'))
|
||||||
|
|
||||||
def test_timeparse_1(self):
|
def test_timeparse_1(self):
|
||||||
'''timeparse test case 1.'''
|
'''timeparse test case 1.'''
|
||||||
|
|||||||
@@ -33,7 +33,7 @@ kinds of time expressions.
|
|||||||
|
|
||||||
import re
|
import re
|
||||||
|
|
||||||
SIGN = r'(?P<sign>[+|-])?'
|
SIGN = r'(?P<sign>[+-])?'
|
||||||
#YEARS = r'(?P<years>\d+)\s*(?:ys?|yrs?.?|years?)'
|
#YEARS = r'(?P<years>\d+)\s*(?:ys?|yrs?.?|years?)'
|
||||||
#MONTHS = r'(?P<months>\d+)\s*(?:mos?.?|mths?.?|months?)'
|
#MONTHS = r'(?P<months>\d+)\s*(?:mos?.?|mths?.?|months?)'
|
||||||
WEEKS = r'(?P<weeks>[\d.]+)\s*(?:w|wks?|weeks?)'
|
WEEKS = r'(?P<weeks>[\d.]+)\s*(?:w|wks?|weeks?)'
|
||||||
|
|||||||
Reference in new issue
Block a user