[#3277] - [#33453] JHtmlBootstrap::addTab() double escapes quotes
- Closed
- 12 Mar 2014
- Medium
- Build: staging
- # 3277
- Diff
- Bakual:FixDoublescapedTabs
- Foreign ID 33453
- Success The Travis CI build passed Details
User tests: Successful: Unsuccessful:
Issue
Tab titles generated using the JLayout layouts/libraries/cms/html/bootstrap/addtabscript.php will be escaped by that JLayout. But the title is already escaped using JText::_('CONSTANT', true) (The second argument true means jsSafe). Thus we end up double escaping the title.
Test
Edit a language string which will end up in a tab title and add a single quote to it. For example:COM_NEWSFEEDS_EDIT_NEWSFEED="Edit News' Feed"
This one will end up in the newsfeed edit form like this:
Apply patch and the backslash will be gone.
Background
This is a regression introduced with Joomla 3.2.3 due to this PR: #3203
The problem (which we didn't catch...) was that before it used JText::sprintf('CONSTANT', true). But JText::sprintf() doesn't take a second argument, thus it didn't escape the string. Since that PR also changed the JText calls to JText::_('CONSTANT', true) (removing the sprintf part), the strings are now correctly escaped already.
Tracker
http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=33453
| Description | <h3>Issue</h3> <p>Tab titles generated using the JLayout <code>layouts/libraries/cms/html/bootstrap/addtabscript.php</code> will be escaped by that JLayout. But the title is already escaped using <code>JText::_('CONSTANT', true)</code> (The second argument <code>true</code> means <code>jsSafe</code>). Thus we end up double escaping the title.</p> <h3>Test</h3> <p>Edit a language string which will end up in a tab title and add a single quote to it. For example:<br><code>COM_NEWSFEEDS_EDIT_NEWSFEED="Edit News' Feed"</code><br> This one will end up in the newsfeed edit form like this:<br><a href="https://f.cloud.github.com/assets/1018684/2384059/ff99665c-a906-11e3-9be3-76e522afb1f7.png" target="_blank"><img src="https://f.cloud.github.com/assets/1018684/2384059/ff99665c-a906-11e3-9be3-76e522afb1f7.png" alt="newsfeed" style="max-width:100%;"></a><br> Apply patch and the backslash will be gone.</p> <h3>Background</h3> <p>This is a regression introduced with Joomla 3.2.3 due to this PR: <a href="https://github.com/joomla/joomla-cms/pull/3203" class="issue-link" title="[#33368] Wrong sprintf prevents escaping single quotes in tabs language strings values">#3203</a><br> The problem (which we didn't catch...) was that before it used <code>JText::sprintf('CONSTANT', true)</code>. But <code>JText::sprintf()</code> doesn't take a second argument, thus it didn't escape the string. Since that PR also changed the JText calls to <code>JText::_('CONSTANT', true)</code> (removing the sprintf part), the strings are now correctly escaped already.</p> | ⇒ | <h3>Issue</h3> <p>Tab titles generated using the JLayout <code>layouts/libraries/cms/html/bootstrap/addtabscript.php</code> will be escaped by that JLayout. But the title is already escaped using <code>JText::_('CONSTANT', true)</code> (The second argument <code>true</code> means <code>jsSafe</code>). Thus we end up double escaping the title.</p> <h3>Test</h3> <p>Edit a language string which will end up in a tab title and add a single quote to it. For example:<br><code>COM_NEWSFEEDS_EDIT_NEWSFEED="Edit News' Feed"</code><br> This one will end up in the newsfeed edit form like this:<br><a href="https://f.cloud.github.com/assets/1018684/2384059/ff99665c-a906-11e3-9be3-76e522afb1f7.png" target="_blank"><img src="https://f.cloud.github.com/assets/1018684/2384059/ff99665c-a906-11e3-9be3-76e522afb1f7.png" alt="newsfeed" style="max-width:100%;"></a><br> Apply patch and the backslash will be gone.</p> <h3>Background</h3> <p>This is a regression introduced with Joomla 3.2.3 due to this PR: <a href="https://github.com/joomla/joomla-cms/pull/3203" class="issue-link" title="[#33368] Wrong sprintf prevents escaping single quotes in tabs language strings values">#3203</a><br> The problem (which we didn't catch...) was that before it used <code>JText::sprintf('CONSTANT', true)</code>. But <code>JText::sprintf()</code> doesn't take a second argument, thus it didn't escape the string. Since that PR also changed the JText calls to <code>JText::_('CONSTANT', true)</code> (removing the sprintf part), the strings are now correctly escaped already.</p> <h4>Tracker</h4> <p><a href="http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=33453">http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=33453</a></p> |
What about in the redirect component where we had a sprintf left?
Good point. I even figured out that JText::sprintf() can in fact return jsSafe strings. One just has to do the call correct 
Updated PR.
Ich habe nur die beiden Dateien geändert
administrator/components/com_redirect/views/link/tmpl/edit.php
layouts/libraries/cms/html/bootstrap/addtabscript.php
und die Symbole waren wieder vorhanden.
Ich habe nur die beiden Dateien geändert
administrator/components/com_redirect/views/link/tmpl/edit.php
layouts/libraries/cms/html/bootstrap/addtabscript.phpund die Symbole waren wieder vorhanden.
Translation: He changed those two files and the icons were visible again. He refers to an image tag I use in my template as part of the tab title.
So one successful test
infograf768
- | Status | New | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2014-03-12 07:14:57 |
| Title |
|
||||||
This PR should be reverted and security-reviewed imho.
The code it's fixing is fine because as @Bakual says it's already JS safe. This was already the expected behaviour before 3.2.3 that you need to pass in a JS safe string. So I think if there are security issues you can obviously see take them up with the JSST - I don't see why we need to emergency revert this unless there is some 0 day emergency
I agree with @wilsonge obviously and disagree with @beat 
The original PR tried to fix an issue (btw not a security issue), but introduced a backward compatibility break with existing extensions and didn't do what it's supposed to do anyway.
I don't see how the current code would be a security issue. It wasn't before the original PR and isn't now.
I did not say that this PR introduces a vuln. Just at first sight, removing addslashes on arguments inside a programatic Javascript output didn't sound right for sure and looked at first glance like a possible security issue that needs review. It sure isn't best php security practices to javascript-escape strings way up in the chain (as here in JText translations) instead of at the place where they are used (e.g. mixed into js like here). It for sure makes a security audit very very hard. But that's not an issue of this PR in particular, but a general issue.
I now understand that the change in this PR is a reversal to obey an old unoptimal API. That said, I bet that lots of extensions are not aware that the strings given to those functions must be escaped.
For sure, any new JLayouts we add in future should escape properly at output-time.
TL;DR: So as this PR is already a reversal, unless proven to be a security issue (as we should analyze in JSST privately), we don't need to revert this PR for now. 
What about in the redirect component where we had a sprintf left?