(dev/core#4160) Convert html special chars to their characters …

[yashodha]

…in ical file generation

Overview

When an event is set to public, the event registration receipt includes a link to download a .ics (ical) file of the event.
This ICS file is generated on the fly, and adds the description of the event to the description field in the .ics. All html tags are stripped from the text, however special chars that have been converted to their html code are not converted back to characters in the .ics.
We need to update the generation of the .ics file to include the conversion of html char code back to characters equivalent.
e.g.

&egrave ; > è
&quot ; > "
&Acirc ; > Â
&nbsp ; > [space]

(put the space before ; so that one can see the char code)
etc...

Before

After

[civibot]

(Standard links)

• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[totten]

Just noting that the function here (getCompleteInfo()) is called by a few things:

ext/eventics/eventics.php:    $info = CRM_Event_BAO_Event::getCompleteInfo($start, $type, $event_id, $end, FALSE);
ext/org.civicoop.eventprivateical/CRM/Eventprivateical/Page/Ical.php:    $info = CRM_Event_BAO_Event::getCompleteInfo($start, $type, $id, $end, false);
ext/de.systopia.eventmessages/Civi/EventMessages/AttachmentProvider/ICal.php:            $event_data = \CRM_Event_BAO_Event::getCompleteInfo('19800101', null, $event_id, null, false);
core/civicrm-core/CRM/Event/ICalendar.php:    $info = CRM_Event_BAO_Event::getCompleteInfo($start, $type, $id, $end);
core/civicrm-core/CRM/Event/Page/List.php:    $info = CRM_Event_BAO_Event::getCompleteInfo($start, $type, $id, $end);
core/civicrm-core/CRM/Core/Block.php:    $info = CRM_Event_BAO_Event::getCompleteInfo(date("Ymd"));
core/civicrm-core/tests/phpunit/CRM/Event/BAO/EventPermissionsTest.php:    $info = CRM_Event_BAO_Event::getCompleteInfo('20000101');
wp-plugin/civievent-widget/civievent-widget.php:                        $cal = CRM_Event_BAO_Event::getCompleteInfo( null, $event_type_id );

I don't know enough about the method and its usage to figure if this change is good or bad, but it's probably worth skimming a few of them to gauge r-technical impact.

[totten]

cc @mlutfy @jensschuppe - Since this might affect eventics and eventmessages, maybe you have thoughts?

[jensschuppe]

Thanks for the heads-up, @totten!

systopia/de.systopia.eventmessages is currently doing something similar for possibly affected fields:

https://github.com/systopia/de.systopia.eventmessages/blob/e703c1f8d255d7d7bac40c0aa57849b3d2e2dbc7/Civi/EventMessages/AttachmentProvider/ICal.php#L73-L78

Also, there's #23638 and dev/core#1541 - not sure how much they're related though.

[totten]

1. So in the eventmessages example, it does the same transform on several more fields. And it's similar to this PR in that both are focused on ICS/ICAL generation. @jensschuppe @yashodha For the ical scenario, how many fields should we be concerned about?
2. It definitely hurts my brain to track down the proper encoding for data in getCompleteInfo() -- i.e. there are multiple callers (CRM_Core_Block, CRM_Event_ICalendar, CRM_Event_Page_List, and the various extensions). There are multiple output media (HTML, ICal). There are several text fields (title, summary, description) with different encodings on-disk.
3. Part of me wonders if some of these callers should be hitting Event.get API instead.
4. As a general matter, throwing in extra decodes randomly is liable to create security issues (e.g. if a user-supplied < becomes a literal < in an HTML output, then sanitized user-content can become unsanitized JS commands). But if one looks at the specific callers+fields, you might find mitigating factors (e.g. one caller displays to a non-HTML medium; another displays HTML but calls purify).
5. I'm guessing that nobody else has done (or wants to do) the due-diligence of re-validating the security of all the callers with this change.
6. If the main issue is how the data appears in ICS/ICAL, then perhaps a safer approach would be:
   • Hold steady the data-format returned by getCompleteInfo() (for whoever calls it now - they're not gonna break)
   • Update the ICal.tpl view to properly render the format returned by getCompleteInfo()

[jensschuppe]

To be honest, eventmessages just uses code copied from where Core builds an iCal file and might as well call the Event.get API. Actually, a Core utility for just generating an iCal file for a given event ID with correct encoding would be much more preferred. The only place this is being done in Core is \CRM_Event_ICalendar::run(), a page controller that writes to the output buffer directly and CRM_Utils_System::civiExit()s - the very reason we had to copy code in the first place.

So what about providing such a utility method in \CRM_Utils_ICalendar that extensions and Core can use, without changing code in \CRM_Event_BAO_Event::getCompleteInfo() for not breaking anything?

[totten]

That sounds like a good idea to me, @jensschuppe

[jensschuppe]

@totten See #26980 for a first try on that.

[stesi561]

@jensschuppe given #26980 is merged does this mean that this gets closed? Or rewritten?

[jensschuppe]

Yes, I would say that #26980 replaces this PR, so it can be closed.