Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

$escapeHtmlHelper is not optional, in case you want real HTML as a label #3015

Closed
pixiebox opened this Issue Nov 19, 2012 · 18 comments

Comments

Projects
None yet

Zend\Form\View\Helper\FormMultiCheckbox.php line 319.
Can it be optional?

Contributor

bakura10 commented Nov 20, 2012

If we do it in this view helper, we should do it in every helper that uses label. To be honest this is an edge case. I think you can afford to manually invoke every helper, so that you render the option with a for loop and not escaping the label if this is what you wan tot od.

visitek commented Dec 17, 2012

Same for content while attempting to render formButton content (<button> <span>...</span> </button>) ... span is escaped and there is no way to disable or change encoder.

Generaly all attribs are encoded with no way to change behavior of escaping strategy

Contributor

cgmartin commented Jan 22, 2013

Would love to have this solved as well. I am running into this a lot, needing unescaped labels in MultiCheckbox, Radio, and Button more than it being an edge case. And too painful to render them manually... lots of helpful stuff in FormMultiCheckbox::renderOptions().

One quick workaround fix might be to expose a setter for the protected $escapeHtmlHelper property in
Zend\Form\View\Helper\AbstractHelper, to give the form view helper a custom "pass-thru" escaper, but even that isn't optimal. Things other than the labels rely on the escaper and I would hate to open a security hole.

Seems appropriate to have a property flag to specifically enable/disable the label escaping.

@ghost ghost assigned padraic Feb 2, 2013

Same issue over here, agree with cgmartin, a setter/property flag on the element would be a nice quick solution.

Contributor

intellix commented Jul 19, 2013

Also same issue, I'm trying to replicate:

<label>
    <input type="radio">Daily<br>(£100)
</label>
<label>
    <input type="radio">Weekly<br>(£200)
</label>

But it isn't currently possible. I have to copy/paste the formMultiCheckbox class and comment out the escaping... @cgmartin what security hole are we talking about exactly? Because I control the html that goes in there, I can't see why the label should be escaped just because some prat wants to do:

'options' => array(
    'label' => $_GET['text'],
),
Contributor

cgmartin commented Jul 19, 2013

@intellix yep, that example illustrates what I was referring to. My take on ZF's security philosophy was that it should lean towards offering protections by default, and only allow non-escaping through explicit options/overrides. I could be wrong.

Core members would be able to clarify this. /cc @padraic @weierophinney

Owner

weierophinney commented Jul 22, 2013

@cgmartin -- correct. Secure by default is the mantra.

Could escaping label content be somehow handled by FormLabel helper class?

Contributor

pine3ree commented Sep 1, 2013

I don't think that allowing html tags in form label would be a great security issue, as labels are set by the developer and do not come from user input. Leaving them escaped by default, it would be nice to have a FormElement chainable method to disable escaping for the current element instance.

Owner

weierophinney commented Sep 3, 2013

@pine3ree We agree with you (see the comments from myself and @cgmartin) -- the default would need to be to escape, and then a flag to allow disabling escaping. Any chance you're willing to create a pull request?

@ghost ghost assigned weierophinney Sep 3, 2013

Contributor

pine3ree commented Sep 4, 2013

In my opinion a quick solution would be to replace

$label = $escapeHtmlHelper($label);

or similar with something like:

if (false !== $element->getOption('escapeLabel')) {
    $label = $escapeHtmlHelper($label);
}

in FormRow, FormMultiCheckbox, FormCollection (Zend\Form\View\Helper\AbstractHelper descendants)
and something similar for $buttonContent in FormButton form view helper.

In this way we can use the options property of Zend\Form\Element during form building in a consistent way.

Adding a simple setter (Zend\Form\Element::setOption($key, $value) would allow to use this method in templates to disable label escaping "on the fly":

<?php echo $this->formRow($form->get('elementName')->setOption('escapeLabel', false)); ?>

btw, FormLabel helper does not escape the label, and when data is sent to a partial view, label is sent unescaped.

Owner

weierophinney commented Sep 4, 2013

@pine3ree This sounds like a reasonable approach. Want to tackle it?

Contributor

stefanotorresi commented Sep 4, 2013

wasn't this already addressed by #4677 ?

Contributor

pine3ree commented Sep 4, 2013

oh thanks, I completely missed commit #4677.

It's nice to have a common base for label Options for all form view helpers, FormLabel included.
I took a much simpler road, just to avoid adding another class for this simple task but I had thought about something similar in the first place and i still find it more elegant.

Having a setOption in FormElement and also a setLabelOption in LabelAware interface would still be useful for on the fly modification in templates.

Extending the idea to FormButton, the $buttonContent var, weather it comes from the target $element label or as a parameter, is considered as the "label" to render even if not technically a label, and the same label option could be used to make decision about escaping. In this case i don't see the point to add another named option or options array property for it.

In ZF1 it was possible to switch of html escaping by simply adding an option 'escape' = false. This worked perfectly even for buttons. With ZF2 we miss this feature very much and would appreciate the return of the option. But why renaming it to 'escapeLabel' or 'disable_html_escape'?

Contributor

stefanotorresi commented Oct 26, 2013

@chludwig
well... while I admit the option name could be shorter, it has a few reasons:

  • the ZF2 code standard for array keys is underscore notation rather than camelcase.
  • the disable word is meant to put emphasis on the fact that by default the escape is on.
  • the html word is there because in ZF2 we have many more than just one escape helper: EscapeHtml, EscapeHtmlAttr, EscapeCss, EscapeJs, EscapeUrl.

So at the end of the day, the long option is there staring at you and saying "are you really sure?", and it's self explanatory in every possible respect.

If you want to set the option on the whole form, recursively iterating through all its elements and invoking setLabelOptions() is a matter of few lines ;)

weierophinney added a commit that referenced this issue Oct 28, 2013

@weierophinney so uhhh... is this one about to be closed?

\0/ +1

@Maks3w Maks3w closed this Dec 21, 2013

or just do it:

$this->add(array(
'name' => 'foo',
'type' => 'radio',
'options' => array(
'label' => 'Foo',
'label_options' => array(
'disable_html_escape' => true, <=== Add this line
),
'value_options' => array(
'0' => 'foo
',
'1' => 'bar
',
),
),
));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment