[#2765] - [#33089] Remove escaping of Fieldset descriptions.
- Closed
- 12 May 2014
- Medium
- Build: staging
- # 2765
- Diff
- piotr-cz:patch_Description
- Foreign ID 33089
- Success The Travis CI build passed Details
User tests: Successful: Unsuccessful:
Issue tracker item 33089
At this moment when using a form field description
<fieldset description="COM_EXAMPLE_DESC" />
with language string
COM_EXAMPLE_DESC="This is <strong>important!</strong>"
The translation is being escaped in views:
echo '<p class="tip">' . $this->escape(JText::_($fieldSet->description)) . '</p>';
preventing use HTML tags in the description:
This is <strong>important!</strong>
and not
This is important!
However this is not a case for
- the spacer field, which may be used as a workaround,
- and other field descriptions
So I think HTML escaping should be removed for fieldset descriptions.
This allows to be more descriptive, add hyperlinks, images, etc.
@betweenbrain Yes, it's the first line in the PR description: Issue tracker item 33089
I've decided to put it on top, as it has been overseen in my previous PR's 
Thanks. I ask as I don't see any tracker link in the email from Github. The
text starts with "At this moment when using a form field description". Will
check it when I get to a desktop.
Best,
Matt Thomas
@betweenbrain
http://matt-thomas.me/
http://betweenbrain.com/
https://github.com/betweenbrain
Sent from mobile. Please pardon any typos or brevity.
On Jan 7, 2014 7:54 AM, "Piotr" notifications@github.com wrote:
@betweenbrain https://github.com/betweenbrain Yes, it's the first line
in the PR description: Issue tracker item 33089
http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=33089&start=0I've decided to put it on top, as it has been overseen in my previous PR's [image:
—
Reply to this email directly or view it on GitHub#2765 (comment)
.
I see. This is because I created a pull request first (you received an email), then tracker item with reference to PR and at the end updated the PR description with issue tracer code.
Could it be escape it for security reasons? I had previously demonstrated
to JSST that language files can be exploited.
Best,
Matt Thomas
@betweenbrain https://twitter.com/betweenbrain
http://matt-thomas.me/
http://betweenbrain.com/
https://github.com/betweenbrain
On Tue, Jan 7, 2014 at 8:33 AM, Thomas Hunziker notifications@github.comwrote:
I had a look when this was introduced to see if there was a reason. Commit
is 9a6abe49a6abe4from 4 years ago, whith message "Merging JForm branch down to trunk".Imho we can safely remove this escaping. Looks like it has slipped in
without any particular reason.—
Reply to this email directly or view it on GitHub#2765 (comment)
.
@betweenbrain At this moment Form field labels and descriptions are not being escaped, so this PR doesn't change much in terms of security.
To address the language strings issue I think it's better to escape inline javascript inside the JLanguage package.
Anyway better safe than sorry.
I don't think this is about security. If so, then it would have to be applied to all instances. Also it was introduced as part of a bigger commit and not to solve a security issue.
Question: If this is merged to master branch, I'm planning to add a pull request for the 2.5. Are there any objections against it?
| Title |
|
||||||
| Title |
|
||||||
Okay, seems that I've broken this Pull Request while doing rebase.
I'll close it as it's sitting here for too long and there's no consensus about security.
| Status | New | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2014-05-12 14:08:07 |
Nice! Is there a tracker item for this yet?
Best,
Matt Thomas
@betweenbrain
http://matt-thomas.me/
http://betweenbrain.com/
https://github.com/betweenbrain
Sent from mobile. Please pardon any typos or brevity.
On Jan 7, 2014 7:01 AM, "Piotr" notifications@github.com wrote: