Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Follow-up to #2297711: Fix HTML escaping due to Twig autoescape
Comment | File | Size | Author |
---|---|---|---|
#55 | Test___s78b3b3e806225ef_s3_simplytest_me.png | 44.79 KB | joelpittet |
#55 | Tags_settings_for_Article___s78b3b3e806225ef_s3_simplytest_me.png | 36.66 KB | joelpittet |
#49 | field_ui-fixForDoubleEscaping-2309929-49.patch | 978 bytes | mareksal |
#44 | form_fields_broke.png | 29.22 KB | aneek |
#27 | 2309929-28-fix.patch | 13.17 KB | aneek |
Comments
Comment #1
andypostComment #2
mradcliffeWas just about to report this issue.
The Allowed Values description text was useful, but a pretty awkward way of providing the user with help text in code. It's always looked ugly. I think we have to fix that now.
Or maybe it would it be possible to preprocess the form element and allow HTML in #description like in previous versions? One of the security risks that is often encountered is NOT properly escaping user input in these elements so I think auto-escaping is a benefit for the lazy, but it does limit the text.
Comment #3
joelpittet@mradcliffe Let's try to not introduce security holes. Though this is harder for somethings it forces you to think about security less in general and more when you need things to be not escaped. Instead of just forgetting to escape your input in the first place, which is what happens until the exploit and subsequent checkPlain and SA gets issued. Though I wasn't for this change at first, I'm growing fond of it more and more and hope others do to.
Thanks @andypost. Checked simplytest.me and made sure the input was known and safe, cheers.
Comment #4
chx CreditAttribution: chx commented#2289999: Add an easy way to create HTML on the fly without having to create a theme function / template
Comment #5
chx CreditAttribution: chx commentedBeyond postponed I looked at this and you are making the return value of allowedValuesDescription() safe as-is. That's not the way. Make the individual allowedValuesDescription() methods safe by using the inline template introduced in that issue.
Comment #6
andypost@chx thanx!
Usage of inline template for
allowedValuesDescription()
makes sense!OTOH this functions could be converted to "item_list" and template is only needed to pass allowed tags so would allow to remove
_field_filter_xss_display_allowed_tags()
Comment #7
mradcliffe_field_filter_xss_display_allowed_tags()
doesn't do any filtering. All it does is take an array of strings and adds angle brackets around them. It's the passing it in as @tags to the t function that does the filtering, which is what does the double-encoding.Here are a couple of screenshots and patches for an implementation with item_list or inline_template. Using item list may introduce semantic meaning..?
This is going to require an API change so I opted for marking the current abstract method as deprecated and non-abstract, and then creating a new abstract method that returns a render element.
The list element patch removes the dependency on #2289999: Add an easy way to create HTML on the fly without having to create a theme function / template.
Comment #8
andypostThe clean-up of
_field_filter*
functions is out of scope, suppose we need follow-up to discus that first.@mradcliffe nice patch! This is not api change because the method is protected. OTOH the moving
#description
property to separate element looks wrong for me.Suppose #items should be used everywhere
I see no reason in this change
Comment #9
mradcliffeI think it has to be a separate element if we need to include markup in help text. The #description should be for text only.
I wanted to remove abstract if I was deprecating allowedValuesDescription so that an implementing class would not need to implement allowedValuesDescription.
Comment #10
vijaycs85#2289999: Add an easy way to create HTML on the fly without having to create a theme function / template is in
Comment #11
aneek CreditAttribution: aneek commentedSo this fix needed to be fixed by the issue #2289999? I tried with the "ListFloatItem.php" file and by changing the following,
I get the desired result. So if you guys think this is a good approach then I can make a fix only to the
allowedValuesDescription()
function to pass HTML via twig inline_templete implementation.Comment #12
mradcliffeThanks, @aneek. That would cause no changes in visual or from Drupal 7/prior behavior, and probably what @andypost mentioned in #8. Can you make a patch and upload it?
On the other hand, I was hoping to improve upon what is current behavior as I prefer the multiple element thing in code I write nowm in D7, but that may be too controversial.
Comment #13
aneek CreditAttribution: aneek commented@mradcliffe, okay then I'll start implementing the patches. Assigning this ticket to myself.
Comment #14
aneek CreditAttribution: aneek commentedWhile making the patch I've found that when we edit a field we also get double escaped HTML tags. This code is present in "core/modules/field_ui/src/Form/FieldInstanceEditForm.php" form. The code is like,
Twig double espaces the string present in the
#description
while it's already escaped int()
function.In fixing this should we adapt the same approach as new inline_template?
We can surely see one related issue reagarding this here. So if we adapt the same approach as inline_template each time then the code will get more complex if developers just have to add HTML tags to
t()
function.Comment #15
pbz1912 CreditAttribution: pbz1912 commentedAdding an additional problem with the image field's manage form:
Comment #16
aneek CreditAttribution: aneek commentedCreated a patch using inline_template. But had to implement
SafeMarkup::set()
incore/modules/image/src/Plugin/Field/FieldType/ImageItem.php
to process proper sanitization in#field_prefix
&#field_suffix
.Let me know if there is a better way to implement this section.
Thanks!
Comment #17
aneek CreditAttribution: aneek commentedComment #18
aneek CreditAttribution: aneek commentedMay have found a better solution thanks to @longwave issue # 2307125
Comment #19
aneek CreditAttribution: aneek commentedWith the fix for
SafeMarkup::set()
new patch is created. Just changed some alignments of arrays as per phpcs error "The first index in a multi-value array must be on a new line".Thanks!
Comment #20
aneek CreditAttribution: aneek commentedComment #21
herom CreditAttribution: herom commentedThanks for working on this.
Unfortunately, this loses the ability to translate these texts. You should use a
{% trans %}
tag or the|t
filter to mark the translatable strings in the template. There are many similar cases of removingt()
calls in the patch.Comment #22
aneek CreditAttribution: aneek commented@herom, thanks for pointing this. I'll surely fix it and post another one for testing.
Comment #27
aneek CreditAttribution: aneek commentedFixed the translation fix mentioned by herom. Marking the new fix for review.
Don't know why the previous patches failed.
Comment #28
aneek CreditAttribution: aneek commentedComment #31
joelpittetI don't think this is correct.
@thedavidmeister may have a proposal to better handle this. It's a bit unmanageable at this time from a structure point of view. If contrib want's to add or remove anything to this it was and still is painful.
Comment #32
aneek CreditAttribution: aneek commented@joelpittet, so what's your idea to address this issue? As per chx's idea I implemented inline_template to fix this issue but still think that there may be a better solution to change these
$description
concatinated HTML into something other and which is more managable.Also I would like to draw your attention to this
#field_prefix
&#field_suffix
usages. In core I've seen if any HTML is added to these FAPI attributes then its wrapped inSafeMarkup::set()
. Since we are not introducing more (maybe removing the existing also) we do need a proper fix for this. In this regards, have a look at this issue raised by @japo32 as Editing the field machine name results in extra span tags visible to user.This one also deals with
#field_prefix
&#field_suffix
. So if any better approach is taken to fix this one then we can also port the#field_prefix
&#field_suffix
related changes here.Comment #33
andypostNot sure this is a case. The more confusing thing is a DX for contrib modules that will implement field types inherited from *list*
As I see most of texts are the same (number based are exactly identical) so code duplication is wrong and could be fixed here.
Maybe just a special template?
And I think the idea with separate element was great to display this Long text
Comment #34
aneek CreditAttribution: aneek commented@andypost,
I do chime in with your thoughts. Maybe a special template to manage the "old school" HTML concatinations to a better way. Any suggestions?
Comment #35
joelpittet@aneek sorry, I shouldn't have chimed in, I don't have a solution and it was really late last night. I do want a better solution here but I've no good ideas atm.
Comment #36
aneek CreditAttribution: aneek commented@joelpittet, its okay, tiring mind, I can understand :-)
At this moment I can only think of having a template but that too will be not so much dynamic as we have to send the values to be displayed in that template.
If I properly understood andypost's thought then he is talking about a "special template" which will have enough intelligence to manage those repeating HTMLs present in
$description
variable.More to my concern is about the
#field_prefix
&#field_suffix
. The HTML present in those tags are always double escaped.Btw, just found one more double escaped string in config_translation module
/core/modules/config_translation/src/FormElement/DateFormat.php
. This too has HTML in#field_suffix
field. This can be seen at admin/config/regional/date-time/formats/manage/[long/medium/short] link(s).Comment #37
aneek CreditAttribution: aneek commentedComment #38
joelpittetI'm not the security expect, @chx kept me balanced on that end of things while turning this on. We made a couple of compromises for pragmatism on #suffix/#prefix/#markup. For expedience sake I'd like to propose the same for #field_prefix/#field_suffix/#description but I have a strong feeling @chx would be more sad if I do... so um you didn't hear that from *ducks and hides*. If we did, I'd hope they would be well documented, and temporary and really only for DX sake because those fields almost always keep HTML in them as their main goal in life. Knowing where the data is coming from is key though in these compromises because if we are silly enough to use user data to fill those, we ask for trouble from the security gods.
@aneek and @andypost that idea is along the same lines as @thedavidmeister was going on about. If you can catch him on IRC it would be a good discussion to have between the 3 of you.
Comment #39
aneek CreditAttribution: aneek commented@joelpittet, I got your point. Its true that these fields only needs the data from the from creator's end but not by the end user. That do makes sence. So as of you we keep these as is? Or may be use SafeMarkup again. Please correct me if I'm wrong.
I'll try to get @thedavidmeister on #drupal. But the timezone makes the issue. :-(
Comment #40
joelpittet@aneek this may be a way forward, @larowlan and @chx came up with a decent way to solve a bunch of these:
#2324371: Fix common HTML escaped render #key values due to Twig autoescape
Comment #41
aneek CreditAttribution: aneek commented@joelpittet, with the latest code checked out and with no patches applied this seems to be fixed. Can you please confirm?
Comment #42
chx CreditAttribution: chx commentedIt's possible that #2273277: Figure out a solution for the problematic interaction between the render system and the theme system when using #pre_render solved this -- do we have tests? I guess just a new assert or two.
Comment #43
aneek CreditAttribution: aneek commented@chx, yes we do have a test. 2309929-28-testonly-fail can be used I think. Please refer to comment #27.
Comment #44
aneek CreditAttribution: aneek commented@chx, thought it seemed that it was fixed but a fresh install via cloning the 8.0.x branch broke everything again. Please have a look at the attached image.
Comment #45
joelpittet@aneek sorry I haven't got a round to testing this, but just a suggestion, turn off render caching(There is an example in sites/example.settings.local.php with sites/development.services.yml).
That way while you are testing you can ensure you aren't looking at a cached version of those fields.
Likely won't change what you are seeing, just a tip to help when working with D8 theming.
Comment #46
aneek CreditAttribution: aneek commentedThanks @joelpittet for the tip. I'll make sure its disabled. Btw, this is what I get when I deleted the Drupal 8 code and re-installed it. All the directories and databases were deleted.
So I think there is no scope of caching at the very first moment of installing Drupal.
Comment #47
swentel CreditAttribution: swentel commented#2339947: field_ui module help text doesn't display allowed tags text well formatted has a patch which apparently works (but I haven't verified)
Comment #48
mareksal CreditAttribution: mareksal commentedHello,
I would like to work on this issue. I am tagging it "Amsterdam 2014" as I am now at Amsterdam 2014 sprint
Cheers
Marek
Comment #49
mareksal CreditAttribution: mareksal commentedAs @swentel suggested, I looked into https://www.drupal.org/node/2339947 . @msankhala suggests a patch.
The patch needed to be updated:
- filename has changed
- there is a new method $this->displayAllowedTags()
But idea of the patch is the same.
I made necessary updates, prepared a new patch and uploading it now.
Comment #50
mareksal CreditAttribution: mareksal commentedComment #51
oenie CreditAttribution: oenie commentedfixing the amsterdam sprint tag to amsterdam2014
Comment #52
malcomio CreditAttribution: malcomio commentedThe patch in #49 fixes the issue for me
Comment #53
swentel CreditAttribution: swentel commentedLet's wait for #2324371: Fix common HTML escaped render #key values due to Twig autoescape first
Comment #54
aneek CreditAttribution: aneek commented#2324371: Fix common HTML escaped render #key values due to Twig autoescape is now in core. GO!!
Comment #55
joelpittetSeems #2324371: Fix common HTML escaped render #key values due to Twig autoescape has fixed this. See screenshots, closing as duplicate. Thanks for the report, screenshot and effort on this everybody!
Comment #56
jibran