Skip to content

validate property, parameter and recur names in from_jcal#1466

Open
alhudz wants to merge 2 commits into
collective:mainfrom
alhudz:jcal-validate-name-tokens
Open

validate property, parameter and recur names in from_jcal#1466
alhudz wants to merge 2 commits into
collective:mainfrom
alhudz:jcal-validate-name-tokens

Conversation

@alhudz

@alhudz alhudz commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Linked issue

No separate issue; reported and fixed here.

Description

Repro: convert untrusted jCal where a property name, parameter name or RRULE part name carries a lone \r (or a :/;) into iCalendar, for example via Calendar.from_jcal(jcal).to_ical().
Cause: Component.from_jcal, Parameters.from_jcal and vRecur.from_jcal keep the jCal name verbatim and emit it raw into the content line, whereas Parameters.from_ical already runs names through validate_token. A : or ; rewrites the property or parameter structure on the same line; a lone \r slips past the \n-only assert in Contentline and turns up as a new content line for consumers that treat a bare carriage return as a break.
Fix: run each jCal name through validate_token at parse time, raising JCalParsingError for names that are not valid iCalendar tokens.

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--1466.org.readthedocs.build/en/1466/

jcal names were emitted raw into the content line, so a `:`/`;` or a lone carriage return in a name could inject parameters or a new line. run them through validate_token at parse time like from_ical does.
@read-the-docs-community

read-the-docs-community Bot commented Jun 16, 2026

Copy link
Copy Markdown

JCalParsingError.validate_property(prop, cls, path=[1, i])
prop_name = prop[0]
try:
validate_token(prop_name)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validation can be even stricter since RFC 7265 3.3 requires lower case. Havent checked usage of JCalParsingError.validate_property but maybe the validation can be moved there?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a lower-case check after the token check here. I looked at folding it into validate_property, but that only sees the property list (not parameter names or RRULE part names), and vRecur.from_jcal calls it on the rrule property itself rather than the part keys, so I kept the check at each of the three parse sites to keep the messages specific. One small thing: property names are 3.4, 3.3 is components, so I cited 3.4 in the news entry.

"All parameter names must be strings.", cls, value=name
)
try:
validate_token(name)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name must also be lower case as per section 3.5

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, name now has to be lower case here too, otherwise it raises JCalParsingError. 3.5 was right.

recur = {}
for key, value in jcal_property[3].items():
try:
validate_token(key)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must be lower case as per 3.6.10

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, same lower-case check on the part name. 3.6.10 was right.

@angatha

angatha commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator

Nice work. I checked the lower case part and linked the sections. Hope I did not make a mistake. Can you please add the lower case validation as well?

rfc 7265 lowercases names (sections 3.4, 3.5, 3.6.10), so reject any that are not lower case after the token check.
@alhudz alhudz force-pushed the jcal-validate-name-tokens branch from dae5a8a to 3d047ac Compare June 17, 2026 13:36
@alhudz

alhudz commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

Pushed the lower-case validation for all three names, with tests and an updated news entry. A few existing recur tests passed upper-case keys like FREQ, so I switched those to lower case as well. Your section links were right, only nudged the property one from 3.3 to 3.4 (3.3 is components).

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.

2 participants