- Issue created by @jonraedeke
- First commit to issue fork.
- @codebymikey opened merge request.
- Status changed to Needs review
almost 2 years ago 10:38am 2 February 2023 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 aninline_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
almost 2 years ago 12:39pm 2 February 2023 - π¦πΊ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 asBeans & 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
almost 2 years ago 12:56am 6 February 2023 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 thantextContent
here so that the contents of potentialstyle
andscript
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
almost 2 years ago 3:45pm 7 February 2023 - πΊπΈ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 betitle
?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 thehtml_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
vshtml_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
almost 2 years ago 10:33pm 23 February 2023 -
Mingsong β
committed fa6de796 on 1.0.x authored by
jonraedeke β
Issue #3338412: Event title html encoding
-
Mingsong β
committed fa6de796 on 1.0.x authored by
jonraedeke β
- Status changed to Fixed
almost 2 years ago 11:30pm 23 February 2023 - π¦πΊAustralia mingsong π¦πΊ
Beautiful works.
I learn a lots from you guys. Appreciate as always.
Automatically closed - issue fixed for 2 weeks with no activity.