Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

Issue summary: View changes
FileSize
14.12 KB
11.85 KB
mradcliffe’s picture

Was 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.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

@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.

chx’s picture

chx’s picture

Beyond 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.

andypost’s picture

@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()

mradcliffe’s picture

_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..?

Using inline_template and existing markup

Using list item markup

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.

andypost’s picture

Issue tags: +Field API

The 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.

  1. +++ b/core/modules/options/src/Plugin/Field/FieldType/ListFloatItem.php
    @@ -53,14 +53,18 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
    +      '#theme' => 'item_list',
    +      '#options' => array(
    
    +++ b/core/modules/options/src/Plugin/Field/FieldType/ListIntegerItem.php
    @@ -53,14 +53,18 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
    +      '#theme' => 'item_list',
    +      '#options' => array(
    
    +++ b/core/modules/options/src/Plugin/Field/FieldType/ListTextItem.php
    @@ -55,13 +55,17 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
    +      '#theme' => 'item_list',
    +      '#items' => array(
    

    Suppose #items should be used everywhere

  2. +++ b/core/modules/options/src/Plugin/Field/FieldType/ListItemBase.php
    @@ -105,10 +110,22 @@ public function settingsForm(array &$form, array &$form_state, $has_data) {
    -  abstract protected function allowedValuesDescription();
    +  protected function allowedValuesDescription() {
    

    I see no reason in this change

mradcliffe’s picture

I 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.

vijaycs85’s picture

aneek’s picture

So this fix needed to be fixed by the issue #2289999? I tried with the "ListFloatItem.php" file and by changing the following,

protected function allowedValuesDescription() {
    /*$description = '<p>' . t('The possible values this field can contain. Enter one value per line, in the format key|label.');
    $description .= '<br/>' . t('The key is the stored value, and must be numeric. The label will be used in displayed values and edit forms.');
    $description .= '<br/>' . t('The label is optional: if a line contains a single number, it will be used as key and label.');
    $description .= '<br/>' . t('Lists of labels are also accepted (one label per line), only if the field does not hold any values yet. Numeric keys will be automatically generated from the positions in the list.');
    $description .= '</p>';
    $description .= '<p>' . t('Allowed HTML tags in labels: @tags', array('@tags' => _field_filter_xss_display_allowed_tags())) . '</p>';
    return $description;*/
    $build['description'] = array(
      '#type' => 'inline_template',
      '#template' => '<p>The possible values this field can contain. Enter one value per line, in the format key|label.
        <br/>The key is the stored value, and must be numeric. The label will be used in displayed values and edit forms.
        <br/>The label is optional: if a line contains a single number, it will be used as key and label.
        <br/>Lists of labels are also accepted (one label per line), only if the field does not hold any values yet. Numeric keys will be automatically generated from the positions in the list.
        </p>
        <p>Allowed HTML tags in labels: {{ tags }}</p>',
      '#context' => array(
        'tags' => _field_filter_xss_display_allowed_tags(),
      ),
    );
    return drupal_render($build);
  }

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.

mradcliffe’s picture

Thanks, @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.

aneek’s picture

Assigned: Unassigned » aneek
Status: Active » Needs work

@mradcliffe, okay then I'll start implementing the patches. Assigning this ticket to myself.

aneek’s picture

FileSize
25.24 KB

While 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,

$form['instance']['description'] = array(
      '#type' => 'textarea',
      '#title' => $this->t('Help text'),
      '#default_value' => $this->instance->getDescription(),
      '#rows' => 5,
      '#description' => $this->t('Instructions to present to the user below this field on the editing form.<br />Allowed HTML tags: @tags', array('@tags' => _field_filter_xss_display_allowed_tags())) . '<br />' . $this->t('This field supports tokens.'),
      '#weight' => -10,
    );

Twig double espaces the string present in the #description while it's already escaped in t() 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.

pbz1912’s picture

Adding an additional problem with the image field's manage form:

aneek’s picture

Created a patch using inline_template. But had to implement SafeMarkup::set() in core/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!

aneek’s picture

Status: Needs work » Needs review
aneek’s picture

Status: Needs review » Needs work

May have found a better solution thanks to @longwave issue # 2307125

aneek’s picture

With 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!

aneek’s picture

Status: Needs work » Needs review
herom’s picture

Status: Needs review » Needs work

Thanks for working on this.

+++ b/core/modules/field_ui/src/Form/FieldInstanceEditForm.php
@@ -118,12 +118,20 @@ public function buildForm(array $form, FormStateInterface $form_state, FieldInst
+      '#template' => 'Instructions to present to the user below this field on the editing form.<br />Allowed HTML tags: {{ tags }} <br /> This field supports tokens.',
...
-      '#description' => $this->t('Instructions to present to the user below this field on the editing form.<br />Allowed HTML tags: @tags', array('@tags' => _field_filter_xss_display_allowed_tags())) . '<br />' . $this->t('This field supports tokens.'),

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 removing t() calls in the patch.

aneek’s picture

@herom, thanks for pointing this. I'll surely fix it and post another one for testing.

The last submitted patch, 16: 2309929-16-testonly-fail.patch, failed testing.

The last submitted patch, 19: 2309929-19-testonly-fail.patch, failed testing.

The last submitted patch, 19: 2309929-19-fix-with-test.patch, failed testing.

aneek’s picture

Fixed the translation fix mentioned by herom. Marking the new fix for review.
Don't know why the previous patches failed.

aneek’s picture

Status: Needs work » Needs review

The last submitted patch, 19: 2309929-19-fix-with-test.patch, failed testing.

The last submitted patch, 27: 2309929-28-testonly-fail.patch, failed testing.

joelpittet’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/image/src/Plugin/Field/FieldType/ImageItem.php
    @@ -219,12 +224,17 @@ public function instanceSettingsForm(array $form, FormStateInterface $form_state
    -      '#type' => 'item',
    +      '#type' => 'fieldset',
    ...
    -      '#field_prefix' => '<div class="container-inline">',
    -      '#field_suffix' => '</div>',
    +      '#attributes' => array(
    +        'class' => array(
    +          'container-inline',
    +          'fieldgroup',
    +          'form-composite',
    +        )
    +      ),
    

    I don't think this is correct.

  2. +++ b/core/modules/options/src/Plugin/Field/FieldType/ListFloatItem.php
    @@ -54,13 +54,20 @@ public static function schema(FieldStorageDefinitionInterface $field_definition)
    -    $description = '<p>' . t('The possible values this field can contain. Enter one value per line, in the format key|label.');
    -    $description .= '<br/>' . t('The key is the stored value, and must be numeric. The label will be used in displayed values and edit forms.');
    -    $description .= '<br/>' . t('The label is optional: if a line contains a single number, it will be used as key and label.');
    -    $description .= '<br/>' . t('Lists of labels are also accepted (one label per line), only if the field does not hold any values yet. Numeric keys will be automatically generated from the positions in the list.');
    -    $description .= '</p>';
    -    $description .= '<p>' . t('Allowed HTML tags in labels: @tags', array('@tags' => _field_filter_xss_display_allowed_tags())) . '</p>';
    -    return $description;
    

    @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.

aneek’s picture

@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 in SafeMarkup::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.

andypost’s picture

If contrib want's to add or remove anything to this it was and still is painful.

Not 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.

still think that there may be a better solution to change these $description concatinated HTML into something other and which is more managable.

Maybe just a special template?
And I think the idea with separate element was great to display this Long text

aneek’s picture

@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?

joelpittet’s picture

@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.

aneek’s picture

@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).

aneek’s picture

Assigned: aneek » Unassigned
joelpittet’s picture

I'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.

aneek’s picture

@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. :-(

joelpittet’s picture

@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

aneek’s picture

@joelpittet, with the latest code checked out and with no patches applied this seems to be fixed. Can you please confirm?

chx’s picture

aneek’s picture

@chx, yes we do have a test. 2309929-28-testonly-fail can be used I think. Please refer to comment #27.

aneek’s picture

FileSize
29.22 KB

@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.
Double escaped string

joelpittet’s picture

@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.

aneek’s picture

Thanks @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.

swentel’s picture

#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)

mareksal’s picture

Assigned: Unassigned » mareksal
Issue tags: +Amsterdam 2014

Hello,

I would like to work on this issue. I am tagging it "Amsterdam 2014" as I am now at Amsterdam 2014 sprint

Cheers

Marek

mareksal’s picture

As @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.

mareksal’s picture

Status: Needs work » Needs review
oenie’s picture

Issue tags: -Amsterdam 2014 +Amsterdam2014

fixing the amsterdam sprint tag to amsterdam2014

malcomio’s picture

Status: Needs review » Reviewed & tested by the community

The patch in #49 fixes the issue for me

swentel’s picture

Status: Reviewed & tested by the community » Postponed
aneek’s picture

Status: Postponed » Needs work
joelpittet’s picture

Seems #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!

jibran’s picture

Issue tags: +SafeMarkup