validate input type in vDDDTypes.from_ical before calling .upper()#1467
validate input type in vDDDTypes.from_ical before calling .upper()#1467HrachShah wants to merge 2 commits into
Conversation
…t.subcomponents is annotated list[Component] and property_items, to_ical, and walk all call .property_items/.name/.walk on whatever is stored there, so a stray non-Component entry (a string, dict, None, or list) used to crash the next serialization with AttributeError: 'str' object has no attribute 'property_items' rather than failing at the offending call site. The isinstance(component, Component) guard turns that into a clear, immediate TypeError naming the bad value's type and repr, so add_component('foo') now raises 'add_component requires a Component instance, got str: foo' instead of crashing later in to_ical. All 14,675 existing tests still pass; verified manually that add_component with str, NoneType, int, dict, and list each surface a TypeError at the call site, while a valid Component still appends and round-trips through to_ical/from_ical as before.
vDDDTypes.from_ical started with ical.upper() without guarding against non-string input, so passing None, an int, a float, a datetime instance, a list, or a dict surfaced a confusing AttributeError like 'NoneType object has no attribute upper' (or, for bytes through the duration branch, a TypeError about a bytes-like object vs str). The new path wraps the .upper() call in a try/except for (AttributeError, TypeError) and translates both into a clear ValueError that names the offending type and repr, matching the wrapping pattern used by the sibling vDate / vDatetime / vTime / vPeriod / vDuration.from_ical helpers. Added a parametrized test in test_vDDDTypes covering None, int, float, datetime, date, list, and dict inputs and asserting the new clear ValueError. All 14,682 existing tests still pass; the new tests fail on the unpatched code with the original AttributeError.
HrachShah
requested review from
SashankBhamidi,
angatha,
niccokunzmann and
stevepiercy
as code owners
Documentation build overview
72 files changed ·
|
| if isinstance(ical, cls): | ||
| return ical.dt | ||
| u = ical.upper() | ||
| try: |
There was a problem hiding this comment.
A type check should be prefered over exceptions. I tried to derive the possible type (also as verification) and found vTime.from_ical, which is wrongly annotated as just str despite it handling bytes explicitly.
ical in vDuration.from_ical may be annotated with str since Pattern.match takes just str. Since that, it may call to_unicode or this method should, which makes the remaining bytes handling easyer. You may also decide to not support bytes as that is currently not the case.
vPeriod.from_ical has no annotated type. It may be just str or depending on the annotation of this argument ical, it may be bytes but the implementation needs to be adjusted so ical.split("/") is done for str and ical.split(b"/") for a bytes argument
| @@ -92,7 +87,18 @@ def to_ical(self) -> bytes: | |||
| def from_ical(cls, ical, timezone=None): | |||
There was a problem hiding this comment.
| def from_ical(cls, ical, timezone=None): | |
| def from_ical(cls, ical: self | str | bytes, timezone=None): |
| f"Expected datetime, date, or time as str or bytes, " | ||
| f"got {type(ical).__name__}: {ical!r}" | ||
| ) from e | ||
| if u.startswith(("P", "-P", "+P")): |
There was a problem hiding this comment.
Will fail for bytes. bytes may be supported by a type check and then using .startswith((b"P", b"-P", b"+P"))`
| ) from e | ||
| if u.startswith(("P", "-P", "+P")): | ||
| return vDuration.from_ical(ical) | ||
| if "/" in u: |
There was a problem hiding this comment.
Also a type error. Would need a b"/" in u instead
angatha
left a comment
angatha
left a comment
There was a problem hiding this comment.
Thank you for your contribution. The first type check is fine but the second one with try-except is to vague. Please repalce it with a type check: isinstance(ical, (str, bytes)) or just str depending on your decision.
|
The diff --git a/src/icalendar/tests/prop/test_vDDDTypes.py b/src/icalendar/tests/prop/test_vDDDTypes.py
@@
@pytest.mark.parametrize(
"bad_input",
- [None, 42, 3.14, datetime(2025, 1, 1), date(2025, 1, 1), ["20250101"], {"dt": "20250101"}],
+ [
+ None,
+ 42,
+ 3.14,
+ datetime(2025, 1, 1),
+ date(2025, 1, 1),
+ ["20250101"],
+ {"dt": "20250101"},
+ ],
ids=["None", "int", "float", "datetime", "date", "list", "dict"],
)
@@
- with pytest.raises(ValueError, match="Expected datetime, date, or time as str or bytes"):
+ with pytest.raises(
+ ValueError, match="Expected datetime, date, or time as str or bytes"
+ ):
vDDDTypes.from_ical(bad_input)I verified locally on the PR head with: ruff format src/icalendar/tests/prop/test_vDDDTypes.py
ruff check src/icalendar/tests/prop/test_vDDDTypes.py
python -m pytest -q src/icalendar/tests/prop/test_vDDDTypes.pyThe test file passed: |
3 participants
vDDDTypes.from_ical started with
ical.upper()without guarding against non-string input, so passingNone, anint, afloat, adatetimeinstance, alist, or adictsurfaced a confusingAttributeError: 'NoneType' object has no attribute 'upper'. The new path wraps the.upper()call in atry/except (AttributeError, TypeError)and translates both into a clearValueErrorthat names the offending type and repr, matching the wrapping pattern used by the siblingvDate,vDatetime,vTime,vPeriod, andvDurationfrom_icalhelpers (e.g.vDatetime.from_ical'sraise ValueError(f"Wrong datetime format: {ical}") from e).Added a parametrized test in
test_vDDDTypescoveringNone,int,float,datetime,date,list, anddictinputs and asserting the new clearValueError. All 14,682 existing tests still pass; the new tests fail on the unpatched code with the originalAttributeError.📚 Documentation preview 📚: https://icalendar--1467.org.readthedocs.build/en/1467/