Correct ICS text escaping per RFC 5545#941
Merged
Conversation
The calendar's ICS subscription feed had three related issues with text escaping. do_ics_escaping replaced semicolons with backslash-colon instead of backslash-semicolon, so any event text containing a semicolon produced an invalid escape sequence. The same function escaped backslashes last, meaning the backslashes it had just inserted when escaping commas and semicolons were themselves doubled, producing corrupt output for any string containing those characters. Newlines were not escaped at all, so a stray LF in a post title or taxonomy term would terminate the line and break every calendar client's parse of the remainder of the feed. The DESCRIPTION field also concatenated field labels and values directly into the feed without passing them through the escaper, and the SUMMARY field did the same for the post status label. Taxonomy term names and custom status labels are user-editable, so a comma, semicolon, backslash or newline in either would corrupt the event. Fixing the escaper to follow RFC 5545 section 3.3.11 (escape backslashes first, then newlines, then the separator characters) and routing every variable field through it closes all three paths with the minimum number of changes. New tests pin the escaper's behaviour across each escapable character and their combination.
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.
Summary
The calendar module's ICS subscription feed had three long-standing escaping bugs that could produce invalid
.icsoutput for any event text containing common punctuation or user-supplied content.do_ics_escaping()replaced semicolons with backslash-colon rather than the RFC 5545 backslash-semicolon sequence, so any event text containing a semicolon produced an invalid escape. The same function escaped backslashes as its final step, which meant the backslashes it had just inserted whilst escaping commas and semicolons were themselves doubled on the second pass, corrupting the output for any string that contained those characters. Newlines were not escaped at all, so a stray LF in a post title, taxonomy term or metadata field would terminate the line and break every calendar client's parse of the remainder of the feed.Two call sites in
handle_ics_subscription()also bypassed the escaper entirely. The SUMMARY field concatenated the post status label directly, and the DESCRIPTION field concatenated the label and value of every information field without passing them through. Since taxonomy term names, custom status labels, and filter-supplied field values can all contain any of the special characters, the only safe option is to escape them.The fix reorders
do_ics_escaping()to match RFC 5545 section 3.3.11 (escape backslashes first, then newlines, then semicolons, then commas) and routes the previously-unescaped call sites through it. New tests pin the escaper's behaviour across each character class and their combination so any future regression is caught immediately.Test plan
composer test:integration— the newCalendarIcsEscapingTestcases pass.icsfeed parses cleanly in a calendar client