Skip to content
New issue

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

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix isTemplate to not return true for string containing ${{ #11315

Merged
merged 1 commit into from
Oct 19, 2017
Merged

Fix isTemplate to not return true for string containing ${{ #11315

merged 1 commit into from
Oct 19, 2017

Conversation

slackerzz
Copy link
Member

@slackerzz slackerzz commented Oct 9, 2017 •
edited
Loading

Changed isTemplate in template.js
Now it returns false if value contains ${{

Description

Fixed Issues (if relevant)

  1. Dollar sign before config path or custom variables in cms page content makes listing crash #10501 Dollar sign before config path or custom variables in cms page content makes listing crash

Manual testing scenarios

  1. create a cms page o static block with ${{config path="web/unsecure/base_url"}} as content
  2. ...

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@okorshenko okorshenko self-assigned this Oct 9, 2017
@okorshenko okorshenko added this to the October 2017 milestone Oct 9, 2017
@okorshenko
Copy link
Contributor

Hi @slackerzz
Thank you for your contribution. We have several failed tests:
1.

Magento\Cms\Controller\Adminhtml\Page\SaveTest::testSaveActionWrongId
PHPUnit\Framework\Exception: Notice: Undefined index: content in /app/code/Magento/Cms/Controller/Adminhtml/Page/Save.php:87.

Magento\Cms\Test\Unit\Controller\Adminhtml\Block\SaveTest

Magento\Test\Php\LiveCodeTest::testCodeStyle
PHP Code Sniffer detected 2 violation(s): 

FILE: lib/internal/Magento/Framework/Escaper.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 324 | ERROR | [x] Whitespace found at end of line
(9 more lines...)

Also, please review the Backward Compatibility Guide for adding new constructor argument.

@okorshenko
Copy link
Contributor

You can run tests on local machine or wait for travis results

@orlangur orlangur self-assigned this Oct 9, 2017
@orlangur
Copy link
Contributor

orlangur commented Oct 9, 2017

We should come to conclusion in initial issue before processing this PR.

@slackerzz
Copy link
Member Author

I think that escaping ${ is the simplest way of fix that behaviour. Following the advice in the issue i will change escapeDollarSign to replace ${ with $⁠{ https://www.compart.com/en/unicode/U+2060

orlangur
orlangur requested changes Oct 10, 2017
View reviewed changes
Copy link
Contributor

@orlangur orlangur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please expand \Magento\Cms\Test\Unit\Model\PageTest with additional case and implement similar \Magento\Cms\Test\Unit\Model\BlockTest for fixed case.

After all changes are made and all builds are green, please squash all changes into a single commit.

lib/internal/Magento/Framework/Escaper.php Outdated
*
* @return string mixed
*/
public function escapeDollarSign($data)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now this is just some Magento_Cms-specific hack to overcome collision with JS templates syntax thus it should not be a part of Magento_Framework.

to replace ${ with $⁠{

No need in HTML entity, just use unicode character. As we can simply do str_replace('${', '$<proper code>{') I don't think there is a real need to do it as separate method.

If we would need something more than one-liner, the class could be something like \Magento\Cms\Model\PseudoEs6TemplateEscaper.

Copy link
Member Author

@slackerzz slackerzz Oct 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This collission happens with every text field in the Magento backend, not only in Magento_Cms #10501 (comment).
According to me it doesn't make sense to put it in Magento_Cms

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

${ in product/category name would be pretty strange. Only those places needs to be fixed where we normally use some {{ ... }} now as a variable template.

My suggestion is: simply do str_replace in place, as soon as this logic this be identified frequently reusable (hope no) it can be refactored into separate class in whatever module or framework itself is suitable (now it should not go outside Magento_Cms, depending on use cases it may become a part of Magento_Backend or Magento_Ui).

Copy link
Member Author

@slackerzz slackerzz Oct 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone sign up with a ${ inside the name, all the customer section in the backend is broken. Obviously it is not common, but we cannot trust user inputs.
${ should NEVER be saved as is, so it makes sense to add the `Escaper' class to escape every user/admin inputs

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Escaper class is not needed currently at all as I already said.

If someone sign up with a ${ inside the name, all the customer section in the backend is broken.

This is a bit different story. In such case string replace does not make a lot of sense and also such situation never occurred before in reality. Please report it separately and we can discuss a fix for it then. I tend to forbidding such combination of symbols via validation.

app/code/Magento/Cms/Controller/Adminhtml/Block/Save.php Outdated
@@ -45,6 +53,7 @@ public function execute()
$resultRedirect = $this->resultRedirectFactory->create();
$data = $this->getRequest()->getPostValue();
if ($data) {
$data['content'] = $this->escaper->escapeDollarSign($data['content']);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest moving such logic to \Magento\Cms\Model\Block::beforeSave.

app/code/Magento/Cms/Controller/Adminhtml/Page/Save.php Outdated
@@ -77,6 +84,7 @@ public function execute()
/** @var \Magento\Backend\Model\View\Result\Redirect $resultRedirect */
$resultRedirect = $this->resultRedirectFactory->create();
if ($data) {
$data['content'] = $this->escaper->escapeDollarSign($data['content']);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest moving such logic to \Magento\Cms\Model\Page::beforeSave.

@slackerzz slackerzz changed the title Escape $ in cms page/static block content Fix isTemplate to not return true for string containing ${{ Oct 11, 2017
@orlangur orlangur added 2.2.x Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release labels Oct 19, 2017
@magento-team magento-team merged commit bac9576 into magento:develop Oct 19, 2017
magento-team pushed a commit that referenced this pull request Oct 19, 2017
@vrann
MAGETWO-81329: Escape $ in cms page/static block content #11315
60ee509
@slackerzz slackerzz deleted the fix_issue_10501 branch October 20, 2017 07:20
@slackerzz slackerzz restored the fix_issue_10501 branch October 23, 2017 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orlangur orlangur orlangur approved these changes

Assignees

@orlangur orlangur

@okorshenko okorshenko

Labels
Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release
Projects
None yet
Milestone
October 2017
Development

Successfully merging this pull request may close these issues.
5 participants
@slackerzz @okorshenko @orlangur @magento-team @vkublytskyi