Fix RecursionError in Component.__repr__ on deeply nested calendars (Closes #1370)#1371
Open
gistrec wants to merge 3 commits into
Open
Fix RecursionError in Component.__repr__ on deeply nested calendars (Closes #1370)#1371gistrec wants to merge 3 commits into
gistrec wants to merge 3 commits into
Conversation
Documentation build overview
14 files changed ·
|
stevepiercy
requested changes
May 14, 2026
Member
stevepiercy
left a comment
There was a problem hiding this comment.
@gistrec thank you for your thorough analysis, code implementation, tests, and excellent comments. Would you please take a look at my minor suggestions, and update your branch against main?
Then I'd like a maintainer to review for final approval. @SashankBhamidi @angatha @niccokunzmann
Component.__repr__ recursively calls str() on every subcomponent.
Parsing accepts BEGIN/END nesting of arbitrary depth (the parser
itself is iterative), so a ~13 KB .ics with ~500 nested VEVENTs
caused any caller doing repr(cal) / str(cal) / f"{cal}" to raise
an uncaught RecursionError at the default recursion limit.
This affects logging, error reporting, and debug-page rendering of
attacker-controlled calendars (e.g. CalDAV, .ics import endpoints,
calendar invitation processing).
Replaced the recursive implementation with an iterative stack-based
walk. Output format is unchanged for normally-shaped calendars.
Added regression tests covering depths up to 1000 and the unchanged
shallow-calendar format.
Existing OSS-Fuzz harness in src/icalendar/fuzzing/ical_fuzzer.py
exercises from_ical() / walk() / to_ical() but not __repr__, which
is why this code path slipped through fuzzing.
angatha
reviewed
May 14, 2026
angatha
reviewed
May 14, 2026
angatha
reviewed
May 14, 2026
angatha
requested changes
May 14, 2026
Per @angatha review on PR collective#1371: - Parametrize ``test_str_does_not_raise_recursion_error`` with the same depths as the ``repr()`` test ([10, 100, 500, 1000]) and combine the two into a single test whose only varying axis (besides depth) is the string-conversion function. - Add exact-equality tests for ``repr()`` output: * ``test_repr_exact_output_for_nested_components`` builds nested empty ``Event``s under a ``Calendar`` and asserts the entire ``repr()`` string equals the format produced by the previous recursive implementation: ``VCALENDAR({}, VEVENT({}, ...))``. * ``test_repr_exact_output_for_sibling_components`` does the same for two sibling VEVENTs: ``VCALENDAR({}, VEVENT({}), VEVENT({}))``. Using empty constructed components keeps the expected string independent of the ``repr`` of any parsed property value.
Upstream switched to towncrier-managed CHANGES.rst in collective#1389 and cut release 7.1.1. Resolved the CHANGES.rst conflict by accepting upstream's regenerated file and migrating this PR's change log entry to a towncrier fragment at news/1370.bugfix per the new contributor workflow (docs/contribute/index.rst).
Author
|
Fixed, ready for re-review, @angatha |
angatha
approved these changes
May 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Component.__repr__(insrc/icalendar/cal/component.py) is recursive overself.subcomponents. Combined with the fact that the parser places no limit onBEGIN/ENDnesting depth, a ~13 KB crafted.icsis enough to makerepr()/str()/f"{cal}"raiseRecursionErrorat the default Python recursion limit (1000). Threshold is depth ≈ 498.from_ical(),walk(), andto_ical()are unaffected — those code paths are already iterative.Realistic exposure: anywhere a logger / error reporter / debug page formats an attacker-controlled
Calendarobject throughrepr(). CalDAV servers,.icsimport endpoints, and calendar-invitation processors are the typical consumers.The OSS-Fuzz harness exercises
from_ical()/walk()/to_ical()but does not exercise__repr__, which is why this path was not surfaced by fuzzing.Change
Replace the recursive
__repr__with an iterative stack-based walk. The output format is intentionally identical to the previous implementation for normally-shaped calendars (VCALENDAR({...}, VEVENT({...}), VEVENT({...}))); only the implementation strategy changes.The PR is intentionally minimal: I am not adding a parser-side depth limit in
handle_begin_componenthere. That would be a sensible defense-in-depth, but it changes acceptance semantics (currently-valid inputs would start being rejected), which deserves a separate discussion.Tests
Added
src/icalendar/tests/test_repr_recursion.pycovering:repr()succeeds at depths[10, 100, 500, 1000](parametrized) — previously crashed for any depth ≥ ~498.str()succeeds at depth 800 (it falls through to__repr__).VEVENTs.Local result:
9451 passed, 736 skipped(was9444 passed; the +7 are the new tests, parametrized count). No existing test changes.Reproducer (matches issue)