Sanitize RFC 5545 control characters in TEXT on output (#1712) by Dmao233 · Pull Request #1741 · collective/icalendar

Dmao233 · 2026-08-30T16:09:41Z

Linked issue

Description

#1723 rejects control characters on parse. That still lets round-trip emit NUL, and it fails callers on junk files. nicco asked to keep parse open and emit valid TEXT. This strips leftover CONTROLs on output. Parameter NULs still go into errors.

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.

Additional information

I used AI to help draft the patch. I read the issue thread and checked the tests, including the reporter repro.

github-actions · 2026-08-30T16:09:53Z

This pull request did not pass quality checks and AI use is suspected. Please review Contribute and make any necessary amendments.

github-actions · 2026-08-30T16:09:54Z

Profile summary:

GitHub user: Dmao233
🟡 Some concerns found with user's profile.
🟡 Some concerns found with recent PR activity.
🟢 No concerns found with recent issue activity.

For a more detailed report, run `gh-profiler Dmao233`.

Full profile

GitHub user: Dmao233
🟡 Some concerns found with user's profile.
   🟡 Account age: 2 years
   🟢 Profile information:
        name: CenFangyu
        blog: https://x.com/cenfangyu17
        twitter: https://x.com/cenfangyu17
      Empty fields: company, location, email, bio

🟡 Some concerns found with recent PR activity.
   66 PRs opened in the last 21 days.
      4 opened against repos the user owns.
      0 opened against repos in publicly associated orgs.
      62 opened against external repos.

   🟡 27 of 62 external PRs closed without merging in the last 21 days.

🟢 No concerns found with recent issue activity.
   3 new issues opened in the last 21 days.
      0 opened in repos the user owns.
      0 opened in repos in publicly associated orgs.
      3 opened in external repos.

   🟢 0 external issues closed as NOT_PLANNED.
   🟢 0 external issues opened with the same title.

read-the-docs-community · 2026-08-30T16:20:35Z

Documentation build overview

📚 icalendar | 🛠️ Build #34322977 | 📁 Comparing cada72d against latest (d157305)

🔍 Preview build

3 files changed

± genindex.html
± reference/api/icalendar.prop.text.html
± _modules/icalendar/prop/text.html

CIFuzz failed after parse stayed open: dateutil.tzical raised ValueError: unsupported property: RRULE, which was not ignored. AI use: Cursor Grok 4.6 Prompt: Add newly-exposed dateutil parse errors to the fuzzer ignore list after CIFuzz failed on collective#1741.

Parse stays open; leftover CONTROLs are stripped on serialize so NUL cannot reach ICS output. AI use: Cursor Grok 4.6 Prompt: Fix NUL in TEXT values by sanitizing on output per collective#1712.

CIFuzz reached dateutil after parse stayed open; ignore those expected ValueErrors. AI use: Cursor Grok 4.6 Prompt: Add dateutil parse errors to the fuzzer ignore list after CI failed.

Dmao233 · 2026-08-30T16:31:11Z

Quality bot failed on description length, missing checklist boxes, commit author not matching, and a first commit over 500 chars.

Description is back on the template. Commits rewritten as me: d242400f, e3cf05ae.

It also flags the username Dmao233 for consecutive digits. I cannot change that.

stevepiercy · 2026-08-30T21:52:03Z

@Dmao233 there's no need to repeat anything from the comments made by automation or in pushed commits. Maintainers receive an email notification for each comment and pushed commit, as well as view the results of CI.

We need to do a better job of clarifying that in our contributing documentation, as many new contributors don't understand that AI is incredibly noisy and states the obvious.

And there's no need to reply to this comment, as that would be yet more noise, but you can use reactions. Thank you!

stevepiercy · 2026-08-31T07:10:39Z

@Dmao233 the CI fuzzing check fails. We don't review pull requests until CI checks pass. Would you please take care? Thank you!

A malformed RRULE key reached to_ical() with a raw LF after parse stayed open. from_parts now strips leftover CR/LF so serialize cannot hit the Contentline assert. AI use: Cursor Grok 4.6 Prompt: Stop CIFuzz AssertionError on unescaped newlines in content lines for collective#1741.

from_parts must keep failing loudly (issue collective#1445). CIFuzz hit that assert on a malformed RRULE; treat only that message as an expected parse/serialize error. AI use: Cursor Grok 4.6 Prompt: Revert silent newline stripping and ignore only the Contentline raw-newline assert in the fuzzer.

Dmao233 · 2026-08-31T07:31:07Z

Fuzzing is green now.

angatha

Thank you for your contribution. I have some suggestions and questions.

angatha · 2026-08-31T16:12:28Z

+    "mandatory TZOFFSETFROM",  # dateutil tzical
+    "no timezones defined",  # dateutil tzical
+    "more than one timezone available",  # dateutil tzical
+    "Unsupported DTSTART param",  # dateutil tzical


Why did thouse became a problem now?

angatha · 2026-08-31T16:12:38Z

-# control character except the horizontal tab may appear in a TEXT value.
-# The line feed, ``\x0a``, is additionally accepted here because it is the
-# result of the escaped sequences ``\N`` and ``\n``.
-UNSAFE_TEXT_CHARS = re.compile(r"[\x00-\x08\x0b-\x1f\x7f]")


This is a breaking change. Please keep it.

angatha · 2026-08-31T16:18:21Z

+_UNSAFE_TEXT_CHARS = re.compile(r"[\x00-\x08\x0b-\x1f\x7f]")
+

 def _escape_char(text: str | bytes) -> str:


Method name and description does not suggest character filtering. Additionally there is _unescape_char in string.py, which should do the inverse. I would prefere another method for that, or you move this to vText.__init__. Then it would also catch to_jcal.

angatha · 2026-08-31T16:18:51Z

+# CONTROL except HTAB (see :rfc:`5545#section-3.1`). After the line-break
+# escapes below, any remaining CONTROL other than HTAB is stripped so
+# serialized output never contains a raw control character.
+_UNSAFE_TEXT_CHARS = re.compile(r"[\x00-\x08\x0b-\x1f\x7f]")


Please rever this and use the constant defined in text.py

angatha · 2026-08-31T16:20:02Z

+        # Contentline.__new__ fails loudly on a raw LF (issue #1445). That is
+        # expected for non-TEXT values such as a malformed RRULE; do not treat
+        # it as a fuzzer crash. Other asserts still propagate.
+        if _CONTENTLINE_NEWLINE_ASSERT in str(e):
+            return -1
+        raise


Why is this now a problem?

angatha · 2026-08-31T16:20:16Z

 FORBIDDEN_VALUES = [
    *FORBIDDEN_CONTROL_CHARS,
    "A\x00B",  # NUL from the issue report
-    "a\rb",  # lone carriage return


Why was this removed?

Keep the public UNSAFE_TEXT_CHARS name. Filter in vText.__new__ so to_ical and to_jcal stay clean; leave _escape_char as an escaper. Drop unproven fuzzer ignores. AI use: Cursor Grok 4.6 Prompt: Address angatha review on collective#1741: restore UNSAFE_TEXT_CHARS and move filtering out of _escape_char.

Use a raw docstring for _strip_unsafe_text_chars. Tighten the two remaining fuzzer ignore comments to the proven CIFuzz exceptions. AI use: Cursor Grok 4.6 Prompt: Fix ruff D301 and confirm remaining collective#1741 review items.

) to_unicode leaves UUID/int/None unchanged; Event.new and vUid.new pass those into vText. Coerce to str so construction matches the old str.__new__ behavior. AI use: Cursor Grok 4.6 Prompt: Fix ruff D301 and the 3.14 UUID/int/None AttributeError from vText sanitization. Co-authored-by: CenFangyu <Dmao233@users.noreply.github.com>

Dmao233 · 2026-09-01T01:27:06Z

Addressed in 278d267 and cada72d.

Kept public UNSAFE_TEXT_CHARS; filter moved to vText.__new__ (covers to_ical and to_jcal). _escape_char is escape-only again.
Restored the control-char cases that still apply under sanitize-on-construct.
Fuzzer: only kept proven ignores (unsupported property, Contentline raw-newline assert for [Bug] unknown value type is escaped/unescaped instead of kept verbatim, breaking round-trips #1445). Dropped the broad dateutil ones.
Also coerce non-str TEXT inputs before strip so vUid.new / Event.new (UUID) keep working on 3.14.

Dmao233 requested review from SashankBhamidi, angatha, niccokunzmann and stevepiercy as code owners August 30, 2026 16:09

github-actions Bot added the ai-suspicion This contribution is possibly created with lots of AI help without enough human understanding. label Aug 30, 2026

Dmao233 added 2 commits August 30, 2026 16:30

Sanitize CONTROL characters in TEXT on output (collective#1712)

d242400

Parse stays open; leftover CONTROLs are stripped on serialize so NUL cannot reach ICS output. AI use: Cursor Grok 4.6 Prompt: Fix NUL in TEXT values by sanitizing on output per collective#1712.

Ignore dateutil VTIMEZONE parse errors in the fuzzer (collective#1712)

e3cf05a

CIFuzz reached dateutil after parse stayed open; ignore those expected ValueErrors. AI use: Cursor Grok 4.6 Prompt: Add dateutil parse errors to the fuzzer ignore list after CI failed.

cursor Bot force-pushed the cursor/fix-text-nul-sanitize-output-7468 branch from e81edf6 to e3cf05a Compare August 30, 2026 16:30

Dmao233 added 2 commits August 31, 2026 07:23

angatha requested changes Aug 31, 2026

View reviewed changes

Dmao233 and others added 3 commits September 1, 2026 01:21

		_UNSAFE_TEXT_CHARS = re.compile(r"[\x00-\x08\x0b-\x1f\x7f]")


		def _escape_char(text: str \| bytes) -> str:

Uh oh!

Uh oh!

Conversation

Dmao233 commented Aug 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Linked issue

Description

Checklist

Additional information

Uh oh!

github-actions Bot commented Aug 30, 2026

Uh oh!

github-actions Bot commented Aug 30, 2026

Uh oh!

read-the-docs-community Bot commented Aug 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Documentation build overview

Uh oh!

Dmao233 commented Aug 30, 2026

Uh oh!

stevepiercy commented Aug 30, 2026

Uh oh!

stevepiercy commented Aug 31, 2026

Uh oh!

Dmao233 commented Aug 31, 2026

Uh oh!

angatha left a comment

Choose a reason for hiding this comment

Uh oh!

angatha Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

angatha Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

angatha Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

angatha Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

angatha Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

angatha Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

Dmao233 commented Sep 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Dmao233 commented Aug 30, 2026 •

edited

Loading

read-the-docs-community Bot commented Aug 30, 2026 •

edited

Loading