Event title html encoding

Created on 1 February 2023, over 1 year ago
Updated 23 February 2023, over 1 year ago

We have ampersands in our event titles. Is there a why to support that? Currently, the titles and modal titles get rendered encoded, like &.

Looks like there is a similar issue in https://www.drupal.org/project/fullcalendar_view/issues/3173041 πŸ› Modal window title contains encoded html text Closed: outdated , but in this module, it's happening for both the title and modal title.

✨ Feature request
Status

Fixed

Version

1.0

Component

Code

Created by

πŸ‡ΊπŸ‡ΈUnited States jonraedeke

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @jonraedeke
  • First commit to issue fork.
  • @codebymikey opened merge request.
  • Status changed to Needs review over 1 year ago
  • The issue is probably the source of the data. If the data is coming from Views, then the issue lies in the default Field Formatter probably escaping the value, and then FullCalendar escaping it a second time.

    This can be addressed by using a custom field formatter similar to how the aggregator module worked.

    You can test this field formatter with dangerously named titles to check for any potential XSS vectors.

  • πŸ‡¦πŸ‡ΊAustralia mingsong πŸ‡¦πŸ‡Ί

    Thanks @codebymikey for investigating and the patch.

    I wonder if it is possible to use the field rewrite functionality provide the the View module instead?

    https://www.drupal.org/docs/8/core/modules/views/rewrite-the-output-of-a... β†’

  • No worries, I ran into the same issue in a private project, and had to implement the same solution.

    I think I initially tried that myself, but I don't think it would work since the rewrites happen, and then its output is sent over to whatever Field Formatter you have, Plain text in my case, which is turn passes the (rewrite) output into an inline_template twig template and escapes it for HTML by default, whereas we want to return the raw unescaped HTML value since FullCalendar escapes it for us anyway.

  • πŸ‡¦πŸ‡ΊAustralia mingsong πŸ‡¦πŸ‡Ί

    Yes, only allowed tags would work with view field rewriting. The example you gave won’t work as the script tag is not allowed. I think there are another contribute modules which provide the field formatter or different approaches that allow html markup in view results.

  • Status changed to Closed: won't fix over 1 year ago
  • πŸ‡¦πŸ‡ΊAustralia mingsong πŸ‡¦πŸ‡Ί

    I close it as it is out of scope of this module.

  • No, the use of <script> in the title was more of an edge case scenario to test that the field formatter proposed here doesn't actually introduce a XSS vulnerability, and is properly escaped.

    The reason the custom field formatter was necessary in the first place was because relatively simple titles like Beans & Toast were being rendered as Beans &amp; Toast in FullCalendar (and can't be fixed with field rewrites), and obviously isn't a good idea if the module is meant to natively support Views out of the box, so rather than search for and introduce another third party module, it made sense for the feature to be available in here as an option.

    The example you gave won’t work as the script tag is not allowed.

    I don't understand what you mean here, isn't the idea to render whatever you have in your content's title on the calendar without needing to specially rewrite it?

  • πŸ‡¦πŸ‡ΊAustralia mingsong πŸ‡¦πŸ‡Ί

    Drupal View module provides the capability to rewrite a field content in the view result.

  • πŸ‡ΊπŸ‡ΈUnited States jonraedeke

    Thank you codebymikey and mingsong. I discovered my issue with the json feed from views. In the REST export: Row style options, you can check a box to enable "Raw". This fixes it.

  • πŸ‡ΊπŸ‡ΈUnited States jonraedeke

    I don't see where this module sanitizing the title though. Is there a reason it is rendering and sanitizing the description and not the title?

    The patch doesn't work in my case since I am using the HTML Title module and there are a few other things happening here. It does seem like I'll have to create a custom field formatter.

  • @jonraedeke opened merge request.
  • πŸ‡ΊπŸ‡ΈUnited States jonraedeke

    I added an MR for another approach that works with either plain text or raw title in the json feed. Basically, just sanitize and render the event title. This would just work out of the box and not require additional configuration.

  • πŸ‡¦πŸ‡ΊAustralia mingsong πŸ‡¦πŸ‡Ί

    Thanks @Jon,

    You raised a really good point.

    The MR 6 looks to me. @Mikey, what do you think?

    It would be idea, if we could use one single approach to sanitize all contents, which are the title and description at this stage.

    Pardon me for having limit knowledge in front-end development.

  • Status changed to Needs review over 1 year ago
  • πŸ‡¦πŸ‡ΊAustralia mingsong πŸ‡¦πŸ‡Ί
  • I think the issue has transformed a fair bit from what the original issue was - Simple HTML characters were being escaped as HTML when being used in a JSON context.

    I somehow missed the "Raw" and {{ title__value }} rewrite options while doing my initial tests, so thanks for spotting that!

    The MR 6 looks to me. @Mikey, what do you think?

    +  Drupal.fullcalendar_block.sanitizeTitle = function sanitizeTitle(title) {
    +    const doc = new DOMParser().parseFromString(title, "text/html");
    +    return doc.documentElement.textContent;
    +  }
    

    If we're gonna go this route, then we probably wanna use innerText rather than textContent here so that the contents of potential style and script tags are ignored (it also strips out excess whitespace as necessary).

    I'm however a little skeptical merging this in in its current state since it's a breaking change as to how things currently work.

    I've attached examples of existing events that should work as is β†’ , and shouldn't be automatically stripped/converted into plain text.

    The presence of actual HTML (that should be rendered/stripped) in the events title is more of an edge-case - most users will expect whatever they have in their title to show up unaltered which is why its the default behaviour, and is more of an issue with the event source IMHO.

    In the case of the HTML Title β†’ module, this can be easily solved by using Views rewrite to strip out the tags on the source level. From a quick test, the module itself doesn't allow simple titles such as Safari < Chrome (which again lends to the point that trying to strip tags by shouldn't be default behaviour). The description is a little different since its content is directly used in a modal as HTML.

    All that being said, while I still think this is a bit out of scope, it's also a relatively easy win since most of the code is already in there, however, I believe it should be added in as a non-breaking option for whoever needs to support it.

    I'll push my amendments to the existing PR for further review/discussion as necessary.

  • Status changed to Needs work over 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States jonraedeke

    Great point about introducing a breaking change. I was not successful using {{ title_value }} as a replacement. I still get the encoded ampersand. I was also not successful in stripping tags using the raw option for the title field. Javascript is visible, but does not execute.

    Fantastic idea to make this an Advanced Drupal setting! This would be a huge convenience for us.

    I tried the latest MR and I'm not seeing the purpose of the raw_title_field: 'rawTitle' setting as it doesn't seem to control anything currently. I guess we would need to separate out the rendering from the sanitizing. Even if we do, is there a case where the title would not be title?

    Sidenote: In the js, it seems as though info[rawTitleField] is always undefined so that always passes !== false.

  • I tried the latest MR and I'm not seeing the purpose of the raw_title_field: 'rawTitle' setting as it doesn't seem to control anything currently.

    Sidenote: In the js, it seems as though info[rawTitleField] is always undefined so that always passes !== false.

    Yeah, that's intentional. The info[rawTitleField] is just an optional field whose presence can be used to opt in/out of the sanitization on an individual event level if necessary. If the field isn't provided, then it relies on just the html_title advanced setting.

    The raw_title_field is a configuration to avoid breaking existing events which might originally use the default "rawTitle" field for something else (that's why it does a strict boolean check to mitigate any collisions).

    You can test it with an event source like this β†’ and seeing how it works with an advanced setting of html_title: true vs html_title: false

    This is also another edge case, so might be better off making raw_title_field NULL by default, and only doing the extra sanitization processing if the field has been configured. If you agree with this approach, then feel free to update the PR and README and put it back up for review.

  • πŸ‡ΊπŸ‡ΈUnited States jonraedeke

    Thank you for the clarification! This works really well for me and I think it will come in handy for others.

  • Status changed to RTBC over 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States jonraedeke
  • Status changed to Fixed over 1 year ago
  • πŸ‡¦πŸ‡ΊAustralia mingsong πŸ‡¦πŸ‡Ί

    Beautiful works.

    I learn a lots from you guys. Appreciate as always.

  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024