Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

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

Closed
pixiebox opened this issue Nov 19, 2012 · 18 comments
Closed
Assignees
Labels
Milestone

Comments

@pixiebox
Copy link

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

@bakura10
Copy link
Contributor

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
Copy link

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

@cgmartin
Copy link
Contributor

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
@philburnett
Copy link

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

@intellix
Copy link
Contributor

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'],
),

@cgmartin
Copy link
Contributor

@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

@weierophinney
Copy link
Member

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

@countless-integers
Copy link

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

@pine3ree
Copy link
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.

@weierophinney
Copy link
Member

@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
@pine3ree
Copy link
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.

@weierophinney
Copy link
Member

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

@stefanotorresi
Copy link
Contributor

wasn't this already addressed by #4677 ?

@pine3ree
Copy link
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.

@chludwig
Copy link

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

@stefanotorresi
Copy link
Contributor

@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
@pixiebox
Copy link
Author

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

\0/ +1

@Maks3w Maks3w closed this as completed Dec 21, 2013
@bruno-oliveira1013
Copy link

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 subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests