BaseHtml::activeHint does not encode hint content

[dicrtarasov]

What steps will reproduce the problem?

class Test extends Model
{
    public $attr;

    public function attributeHints()
    {
        return [
            'attr' => 'An hint with > "3" unquoted elements'
        ];
    }
}

echo $form->field($test, 'attr');

Generates broken HTML because of chars in model hint.

What is the expected result?

We can add support of 'encode' option to BaseHtml.

echo $form->field($test, 'attr', [
    'hintOptions' => [
        'encode' => true
    ]
]);

or

BaseHtml::activeHint($model, 'attr', [
    'encode' => true
]);

Additional info

Q	A
Yii version	2.0.39
PHP version	7.4
Operating system	Debian

[bizley]

How is this broken? Default implementation produces something like:

<p class="help-block">An hint with > "3" unquoted elements</p>

[dicrtarasov]

Need to be quoted by htmlspecialchars.

[bizley]

Why?

[samdark]

I guess that's because it's not a good practice to have HTML in your models. @dicrtarasov right?

[bizley]

It's true but the example doesn't contain any HTML and it's provided by developer so by default it should not need encoding like data provided by end user. It also by default doesn't generate anything inside HTML tag attributes which would require encoding. I'm just trying to understand.

[My6UoT9]

I guess this is similar to other possible injections. The basic idea is to make it impossible for a developer to make such errors, therefore such input should be escaped also. In Yii2 examples, this comes always hardcoded from the model, but technically the hint could come from anywhere, therefore it would be could practice to encode it. At best encode by default, but let the developer disable it.

[bizley]

This kind of reasoning would let us end with everything being encoded by default. If developer uses something that comes from user and wants to display it, it should be encoded but it is developer's responsibility in that case because framework is not forcing to use risky input here. We are doing the same for labels - in both cases only options are encoded because these go as HTML attributes. I would like to hear @dicrtarasov response about all this.

[dicrtarasov]

1) Text in hits does not contains HTML, but can include chars like ", or ">". It's normal. (Ex. "value must be < 30") 2) Text can be used to output in console, file or other places, so it must not be quoted by default. 3) Only output in HTML must be always quoted. 4) Adding support of encode or encodeHint options, similar to existing encodeLabel, does not broke existing code compatibility.

[dicrtarasov]

This kind of reasoning would let us end with everything being encoded by default. If developer uses something that comes from user and wants to display it, it should be encoded but it is developer's responsibility in that case because framework is not forcing to use risky input here. We are doing the same for labels - in both cases only options are encoded because these go as HTML attributes. I would like to hear @dicrtarasov response about all this.

Hint and other text are similar to labels when output in HTML. Developers mut have an posibility to use options to encode (like encodeHints => true) Output in HTML must always have options to encode content. When text are html-quoted by default, then output to console, file must be used with entitydecode?

[bizley]

I guess you are referring to encodeLabel from DataColumn or encodeLabels from Menu widget - the option there was added to prevent encoding on demand because the default behavior is to encode it. You want now opposite of that for hints.

I'm against such feature.

[dicrtarasov]

$form->field used to generate HTML similar to DataColumn or Menu widget. So we can add options to control encoding. In anyway, currently generated HTML is broken.

[dicrtarasov]

HTML is broken. So this is a bug. But can be a feature also :)

[bizley]

Please explain to me why HTML is broken here.

[dicrtarasov]

<p class="help-block">An hint with > "3" unquoted elements</p>

Is this right?

[bizley]

Yes, I believe this is a valid HTML. If I'm mistaken please state some doc links saying otherwise.

[dicrtarasov]

No, because of ">" HTML is broken

[dicrtarasov]

Unquoted content must not be used in HTML

[dicrtarasov]

https://www.w3.org/TR/xml/#syntax

[bizley]

This is about XML...

[dicrtarasov]

HTML5 ("DOCTYPE html") is XHTML.

[dicrtarasov]

Try to validate it at validator.w3c.org

[dicrtarasov]

This is for HTML: https://tools.ietf.org/html/rfc1866#section-3.2

[bizley]

https://validator.w3.org/nu/#textarea Code tested (extra markup added so it does not complain):

<!DOCTYPE html>
<html lang="en">
<head><title>Test</title></head>
<body>
<p class="help-block">An hint with > "3" unquoted elements</p>
</body>
</html>

[dicrtarasov]

Ohh.... this is special case. What about:

<!DOCTYPE html>
<html lang="en">
<head><title>Test</title></head>
<body>
<p class="help-block">An hint <with > "3" unquoted elements</p>
</body>
</html>

[dicrtarasov]

we are discussing about nothing HTML special chars in content must be quoted and nothing else.

[samdark]

@dicrtarasov is correct. Special chars should be escaped.

[bizley]

Yes, I agree. But I state it should be up to the developer, not framework. And I rest my case.

[dicrtarasov]

Ok, how developer can encode this, when using model in ActiveField ?

echo $form->field($model, 'attr');

I propose to add new option encode to hintOptions:

echo $form->field($model, `attr`, [
    'hintOptions' => [
        'encode' => true
    ]
]);

Or... what other way is?

[dicrtarasov]

I found the way, but it is not so beautifull:

echo $form->field($model, `attr`, [
    'hintOptions' => [
        'hint' => Html::encode($model->attr)
    ]
]);

It's better to add encode support to BaseHtml::activeHint:

public static function activeHint($model, $attribute, $options = [])
    {
        $attribute = static::getAttributeName($attribute);
        $hint = isset($options['hint']) ? $options['hint'] : $model->getAttributeHint($attribute);
        if (empty($hint)) {
            return '';
        }

  +   $encode = ArrayHelper::remove($options, 'encode', false);
  +   if ($encode) {
  +        $hint = static::encode($hint);
  +   }              

        $tag = ArrayHelper::remove($options, 'tag', 'div');
        unset($options['hint']);
        return static::tag($tag, $hint, $options);
    }

[samdark]

I agree that adding encoding option makes sense. @dicrtarasov do you have some time for a pull request?

[bizley]

Please remember to also add this in other places that could take possible "broken" HTML:

• yii\helpers\BaseHtml::activeLabel
• yii\grid\Column::header
• yii\grid\Column::footer
• yii\grid\GridView::caption
• yii\grid\GridView::layout
• yii\grid\GridView::emptyCell
• yii\widgets\BaseListView::summary
• yii\widgets\BaseListView::emptyText
• yii\widgets\BaseListView::layout
• yii\widgets\Breadcrumbs::itemTemplate
• and many more...

(I hope you see now how ridiculous this is...)

[samdark]

Hmm. Yes, that's a bit inconsistent...

[dicrtarasov]

I will be glad to help in any way I can. I can add functionality to BaseHtml::activeHint, but I don't know all the aspects of the other classes listed above.

[samdark]

The thing is that in many places @bizley pointed to we're not encoding anything and letting it up to end user. In order to have it all consistent we have to either adjust it everywhere or do not adjust it. In case of adjusting it everywhere we can't change behavior because of backwards compatibility (for example, one may use HTML in hints).

[samdark]

Overall it can not be efficiently done with Yii 2 without too much breakage. Closing because of that. Will contiue thinking about it for Yii 3.