Skip to content

reject empty or whitespace timedelta values in _parse_timedelta#3652

Open
HrachShah wants to merge 1 commit into
tornadoweb:masterfrom
HrachShah:fix/options-parse-timedelta-empty-whitelist
Open

reject empty or whitespace timedelta values in _parse_timedelta#3652
HrachShah wants to merge 1 commit into
tornadoweb:masterfrom
HrachShah:fix/options-parse-timedelta-empty-whitelist

Conversation

@HrachShah

Copy link
Copy Markdown

What

_parse_timedelta in tornado/options.py silently accepted empty ("") and whitespace-only (" ", "\t", "\n") values as datetime.timedelta(0). Every other type-specific parser in the same module (_parse_datetime, _parse_bool, _parse_string) rejects empty input with options.Error, so this is inconsistent.

Passing --t= or --t=" " to an application using tornado.options would set a 0-second timeout instead of raising the user-visible error that the rest of the module produces.

Repro

from tornado.options import OptionParser
import datetime
opts = OptionParser()
opts.define("t", type=datetime.timedelta)
opt = opts._options["t"]
assert opt._parse_timedelta("") == datetime.timedelta(0)   # silently accepted

Fix

Early-exit guard at the top of _parse_timedelta:

if not value or not value.strip():
    raise Error("Invalid time delta: %r" % value)

The existing inner try: ... except Exception: raise is left as-is — it still legitimately catches float() conversion and timedelta(**{units: num}) construction errors. This commit focuses narrowly on the silent-accept bug; a separate PR (the existing fix/options-parse-timedelta-options-error branch) was already in flight to clean up the raise Exception() / raise no-op pair.

Test

test_parse_timedelta_empty_or_whitespace_raises constructs an _Option directly and calls _parse_timedelta(bad) for bad in ("", " ", "\t", "\n", " \t "), asserting options.Error is raised with the offending value in the message. The test fails on the pre-fix code (the call returns timedelta(0) instead of raising).

Verified

  • python3 -m pytest tornado/test/options_test.py -v — 25 passed, 0 failures
  • Pre-fix git stash + test rerun shows the regression test fails as expected
  • All existing options tests still pass (test_types, test_types_with_conf_file, test_parse_callbacks, etc.)

An empty string or whitespace-only value passed to _parse_timedelta used to
silently parse as datetime.timedelta(0): the while loop body only runs when
start < len(value), so with value="" or value=" " the loop is skipped and
the initial sum = datetime.timedelta() is returned. parse_command_line
then happily accepts '--t=' or '--t=" "', which contradicts every other
type-specific parser in this module (_parse_datetime / _parse_bool /
_parse_string all reject empty strings).

Add an early-exit guard that raises options.Error('Invalid time delta: %r'
% value) for falsy or strip()-empty inputs. The inner try/except is left
in place since it still legitimately catches the float() conversion and
datetime.timedelta(**{units: num}) construction errors that the existing
PR draft (b0ea1c1) was also going to clean up; this commit focuses
narrowly on the silent-accept bug.

A regression test exercises every empty/whitespace value the prior code
silently accepted and asserts Error is raised with the offending value
in the message.

Verified:
- python3 -m pytest tornado/test/options_test.py -v passes all 25 tests
- the new test fails on the pre-fix code (bare return timedelta(0))
- the existing test_types, test_types_with_conf_file, test_run_parse_callbacks
  tests still pass
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant