Skip to content

reject CR and LF in vUri, vCalAddress and vInline values#1468

Open
alhudz wants to merge 1 commit into
collective:mainfrom
alhudz:reject-crlf-uri-values
Open

reject CR and LF in vUri, vCalAddress and vInline values#1468
alhudz wants to merge 1 commit into
collective:mainfrom
alhudz:reject-crlf-uri-values

Conversation

@alhudz

@alhudz alhudz commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Description

vUri, vCalAddress and vInline are str subclasses whose to_ical() writes the value verbatim, with none of the escaping vText applies. A value carrying a lone \r therefore lands raw in the content line, where the only guard is Contentline.__new__'s assert "\n" not in value, which never looks at \r. Consumers that treat a bare CR as a line break read the tail as a separate property.

repro: Calendar.from_jcal([..., ["url", {}, "uri", "http://e.com/a\rX-EVIL:1"], ...]).to_ical()
expected: the malformed value is rejected
actual: the output contains URL:http://e.com/a\rX-EVIL:1, an injected X-EVIL content line on a bare-CR consumer

These value types cannot encode a newline the way TEXT does, so the guard rejects raw CR/LF in __new__ rather than escaping. vXmlReference inherits the check from vUri.

Checklist

  • I've added a change log entry to /news, following the instructions in Change log entry format.
  • I've added or updated tests if applicable.
  • I've run and ensured all tests pass locally by following Run tests.
  • I've added or edited documentation, both as docstrings to be rendered in the API documentation and narrative documentation, as necessary.

📚 Documentation preview 📚: https://icalendar--1468.org.readthedocs.build/en/1468/

@read-the-docs-community

Copy link
Copy Markdown

Documentation build overview

📚 icalendar | 🛠️ Build #33178298 | 📁 Comparing 018f5f3 against latest (5b5af3a)

  🔍 Preview build  

4 files changed
± 404.html
± _modules/icalendar/prop/cal_address.html
± _modules/icalendar/prop/inline.html
± _modules/icalendar/prop/uri.html

@stevepiercy

Copy link
Copy Markdown
Member

@alhudz please see my comment regarding the treatment of a lone \r in #1462 (comment). Would you please defer any further PRs on how to treat a lone \r until we definitively settle the matter?

@niccokunzmann

niccokunzmann commented Jun 17, 2026

Copy link
Copy Markdown
Member

Yes, I think, it is good to put our heads together in an issue that lists the way to go forward and also all the places that need touching. That is to keep a record of the decisions. This is a really good find. We should give it proper attention and the documentation that it needs, so we can be sure people will understand the code and decisions in the years to come. Thank @alhudz would you like to open the issue? As a maintainer, I would chip in and do some edits, with a checklist of what I would like to see done to really assure this is thoroughly handled everywhere. Right now, splitting decision making across PRs calls in inconsistency over the long term. Thanks for the work so far!

Note: For documentation:

  • link to the issue
  • quote RFC5545 and 6868 on line breaks
  • outline how the RFC handles line breaks - actually platform dependent!

TODO:

  • So, this is also why I wanted the ical_value property - that can take care of converting the text into a platform dependent version. e.g. a solution vText can always just contain \n, vText.ical_value does the conversion to the platform.
  • list all the places that are affected by \r and platform-dependent line breaks
  • decide on a strategy
  • implement the change consistently
  • do not touch vUnkown (check with jCal RFC)

@alhudz You see, the issue is quite big and also, to solve this you do not need to do all the work. Could you open an issue to start us off?

@alhudz

alhudz commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

Opened #1471 to gather the line-break treatment in one place, with the RFC quotes, the affected paths and a checklist to start from. Left the structure loose so you can drop in the items you want tracked.

Happy to hold off on further lone \r PRs until the strategy's settled there, and I'll pick up the implementation once it is.

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.

3 participants