fix quadratic time of the XML declaration regex in vXml by shamsManna · Pull Request #2037 · PyCalendar/icalendar

shamsManna · 2026-10-27T17:00:30Z

Linked issue

See feat: add the XML property from RFC 6321 #1958

Description

vXml._parse strips the declared encoding with a regex whose lazy [^>]*? and greedy \s+ both match whitespace, so a long whitespace run in the XML declaration is retried from every position: 200k spaces take 41 s in to_xcal(), and an untrusted .ics stalls xCal conversion. Fix: a single whitespace before encoding, like the bytes regex. Same problem as #1449, in the XML property from #1958.

Checklist

I added a change log entry, following the instructions in all subsections under Change log requirements.
I followed icalendar's Artificial intelligence policy and disclosed my Responsible AI use in my commit messages, if applicable.
I added or updated tests, if applicable.
I ran and ensured all tests pass locally by following Run tests.
I added or edited documentation as necessary, both as docstrings to be rendered in the API documentation and narrative documentation, following the Style guide.

vXml._parse strips the declared encoding from an XML property's declaration with a regex whose lazy [^>]*? and greedy \s+ both match whitespace, so a long run of whitespace in the declaration is retried from every position and to_xcal() takes quadratic time (200k spaces: 41 s). Match a single whitespace before encoding, as the bytes regex does. The whitespace left in the declaration is valid XML. AI: Claude Fable 5.1 (claude-fable-5-1) via Claude Code. Prompt: find a security bug in icalendar, reproduce it, and write a minimal fix with a regression test and change log entry. Output: (this commit), reviewed by the author.

github-actions · 2026-10-27T17:00:41Z

Thanks for the pull request, @shamsManna! A maintainer will review it shortly.

github-actions · 2026-10-27T17:00:43Z

Mixed activity

Activity patterns show a mix of organic and automated signals.

View full analysis →

Evidence

Rapid fork→PR pattern: 11/11 fork branches followed by upstream PRs within 17s
Young account: Account is 36 days old

Last 5 PRs:

_{This is an automated analysis by AgentScan}

niccokunzmann

Thanks for finding this.. regex is always a bit a hit and miss and we must check them all for quadratic runtime.

A possessive \s++ alone is still quadratic, because the lazy [^>]*? keeps retrying it from every position in the whitespace run. Matching the declaration as alternating possessive runs of whitespace and other characters is linear and removes all whitespace before encoding, like the original pattern. Python 3.10 keeps the single \s.

niccokunzmann

This looks good.. Thanks for the fix.

angatha · 2026-10-30T09:31:30Z

-    r"""^(\ufeff?<\?xml\s[^>]*?)\s+encoding\s*=\s*("[^"]*"|'[^']*')""",
-    re.ASCII,
-)
+# With ``[^>]*?\s+encoding``, the whitespace run before the attribute is retried


@maintainers, whats you opinion on including old code in an explanation?

I think it's not useful. Better to explain what it does now, and avoid history.

stevepiercy

Thanks for bringing this to our attention. I have a couple of questions and a suggestion to remove comments. Would you please take care? Thank you!

stevepiercy · 2026-11-11T22:52:34Z

+# With ``[^>]*?\s+encoding``, the whitespace run before the attribute is retried
+# from every position that the lazy ``[^>]*?`` reaches, so a long run of whitespace
+# in the declaration takes quadratic time.


Suggested change

# With ``[^>]*?\s+encoding``, the whitespace run before the attribute is retried

# from every position that the lazy ``[^>]*?`` reaches, so a long run of whitespace

# in the declaration takes quadratic time.

done, removed it. the comment above the regexes now just says what they do.

stevepiercy · 2026-11-11T23:04:06Z

+    event = Event()
+    event.add(
+        "XML",
+        '<?xml version="1.0"' + whitespace * 200_000 + '?><a xmlns="urn:x">text</a>',


How does test the presence of whitespace relative to encoding? Should there be another test for that case?

it didn't, that test has no encoding in the declaration at all. added test_xml_property_to_xcal_with_long_whitespace_around_the_encoding: 200k runs before version, before encoding and after it, and it checks the encoding still gets stripped (encoding="ISO-8859-1" with café € as the text, so a leftover encoding would garble it).

the run before version is the important one. it's whitespace that isn't followed by encoding, which is what made the old \s+ quadratic even when an encoding is present (40k tabs there took 4.9 s with the old pattern, under 1 ms now). i also added two short declarations with tabs and newlines around encoding to the parametrized encoding test.

stevepiercy · 2026-11-11T23:09:12Z

+    # whitespace and other characters that are never given back, so the match is
+    # linear and all whitespace before ``encoding`` is removed.
+    _DECLARED_ENCODING = re.compile(
+        r"""^(\ufeff?<\?xml\s[^\s>]*+(?:\s++[^\s>]++)*?)\s++encoding\s*+=\s*+"""


I think double-quotes need to be escaped. Please check against this test, and let me know what you think.

https://regex101.com/r/mXQ2XG/1

they don't need escaping in the source. both patterns are in r"""...""" triple-quoted raw strings, so a single " inside is just a literal quote and doesn't end the string. regex101 asks for \" because your link uses " as the delimiter, which corresponds to r"...".

i ran your test string through the compiled pattern on 3.11+ and it strips the encoding together with all the whitespace before it, giving <?xml version="1.0"\n\t\t\t\n\n?><a xmlns="urn:x">text</a>. a declaration like it is now in the tests.

stevepiercy · 2026-11-11T23:09:15Z

+    # A single ``\s`` before ``encoding`` keeps the match linear.
+    # The whitespace that stays in the declaration is valid XML.
+    _DECLARED_ENCODING = re.compile(
+        r"""^(\ufeff?<\?xml\s[^>]*?)\sencoding\s*=\s*("[^"]*"|'[^']*')""",


We should test this one as well.

agreed. it's now _DECLARED_ENCODING_PY310 at module level, and _DECLARED_ENCODING points to it on 3.10. a declared_encoding fixture runs the declaration tests against both, so the 3.10 regex is tested on every python version and the possessive one on 3.11+ (skipped on 3.10).

Drop the comment about the old pattern and keep the Python 3.10 regex as _DECLARED_ENCODING_PY310, so the declared encoding tests run against it on every Python version as well as against the possessive one on 3.11+. Add a test with long whitespace runs before version, before encoding and after encoding, which checks the encoding is still removed, plus declarations with whitespace and newlines around encoding.

shamsManna requested review from angatha, niccokunzmann and stevepiercy as code owners October 27, 2026 17:00

github-actions Bot added the PR quality: good label Oct 27, 2026

github-actions Bot added the agentscan:needs-review AgentScan flagged this item for review because it has mixed signals label Oct 27, 2026

niccokunzmann requested changes Oct 27, 2026

View reviewed changes

Comment thread src/icalendar/prop/xml.py Resolved

shamsManna and others added 2 commits October 28, 2026 09:56

Merge branch 'main' into xml-declaration-whitespace

224e498

niccokunzmann mentioned this pull request Oct 28, 2026

Drop support for Python 3.10 #1636

Open

9 tasks

niccokunzmann previously approved these changes Oct 28, 2026

View reviewed changes

niccokunzmann assigned stevepiercy Oct 28, 2026

angatha reviewed Oct 30, 2026

View reviewed changes

stevepiercy requested changes Nov 11, 2026

View reviewed changes

shamsManna dismissed niccokunzmann’s stale review via 5b5e119 November 13, 2026 09:17

	# With ``[^>]*?\s+encoding``, the whitespace run before the attribute is retried
	# from every position that the lazy ``[^>]*?`` reaches, so a long run of whitespace
	# in the declaration takes quadratic time.

Conversation

shamsManna commented Oct 27, 2026

Linked issue

Description

Checklist

Uh oh!

github-actions Bot commented Oct 27, 2026

Uh oh!

github-actions Bot commented Oct 27, 2026

Mixed activity

Uh oh!

niccokunzmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

niccokunzmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevepiercy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants