Merge remote-tracking branch 'origin/MAGETWO-95236' into 2.2.7-develop-pr53

SeruyV · SeruyV · commit 67681a5d8aa3 · 2018-10-24T15:16:59.000+03:00
diff --git a/app/code/Magento/AdminNotification/Block/Grid/Renderer/Actions.php b/app/code/Magento/AdminNotification/Block/Grid/Renderer/Actions.php
@@ -8,6 +8,9 @@
 
 namespace Magento\AdminNotification\Block\Grid\Renderer;
 
+/**
+ * Renderer class for action in the admin notifications grid
+ */
 class Actions extends \Magento\Backend\Block\Widget\Grid\Column\Renderer\AbstractRenderer
 {
     /**
@@ -37,7 +40,8 @@ public function __construct(
      */
     public function render(\Magento\Framework\DataObject $row)
     {
-        $readDetailsHtml = $row->getUrl() ? '<a class="action-details" target="_blank" href="' . $row->getUrl() . '">' .
+        $readDetailsHtml = $row->getUrl() ? '<a class="action-details" target="_blank" href="' .
+            $this->escapeUrl($row->getUrl()) . '">' .
             __('Read Details') . '</a>' : '';
 
         $markAsReadHtml = !$row->getIsRead() ? '<a class="action-mark" href="' . $this->getUrl(
diff --git a/app/code/Magento/Sales/Helper/Admin.php b/app/code/Magento/Sales/Helper/Admin.php
@@ -3,8 +3,12 @@
  * Copyright © Magento, Inc. All rights reserved.
  * See COPYING.txt for license details.
  */
+
 namespace Magento\Sales\Helper;
 
+/**
+ * Sales admin helper.
+ */
 class Admin extends \Magento\Framework\App\Helper\AbstractHelper
 {
     /**
@@ -148,22 +152,15 @@ public function escapeHtmlWithLinks($data, $allowedTags = null)
             $links = [];
             $i = 1;
             $data = str_replace('%', '%%', $data);
-            $regexp = "/<a\s[^>]*href\s*?=\s*?([\"\']??)([^\" >]*?)\\1[^>]*>(.*)<\/a>/siU";
+            $regexp = "#(?J)<a"
+                ."(?:(?:\s+(?:(?:href\s*=\s*(['\"])(?<link>.*?)\\1\s*)|(?:\S+\s*=\s*(['\"])(.*?)\\3)\s*)*)|>)"
+                .">?(?:(?:(?<text>.*?)(?:<\/a\s*>?|(?=<\w))|(?<text>.*)))#si";
             while (preg_match($regexp, $data, $matches)) {
-                //Revert the sprintf escaping
-                $url = str_replace('%%', '%', $matches[2]);
-                $text = str_replace('%%', '%', $matches[3]);
-                //Check for an valid url
-                if ($url) {
-                    $urlScheme = strtolower(parse_url($url, PHP_URL_SCHEME));
-                    if ($urlScheme !== 'http' && $urlScheme !== 'https') {
-                        $url = null;
-                    }
-                }
-                //Use hash tag as fallback
-                if (!$url) {
-                    $url = '#';
+                $text = '';
+                if (!empty($matches['text'])) {
+                    $text = str_replace('%%', '%', $matches['text']);
                 }
+                $url = $this->filterUrl($matches['link'] ?? '');
                 //Recreate a minimalistic secure a tag
                 $links[] = sprintf(
                     '<a href="%s">%s</a>',
@@ -178,4 +175,29 @@ public function escapeHtmlWithLinks($data, $allowedTags = null)
         }
         return $this->escaper->escapeHtml($data, $allowedTags);
     }
+
+    /**
+     * Filter the URL for allowed protocols.
+     *
+     * @param string $url
+     * @return string
+     */
+    private function filterUrl(string $url): string
+    {
+        if ($url) {
+            //Revert the sprintf escaping
+            $url = str_replace('%%', '%', $url);
+            $urlScheme = parse_url($url, PHP_URL_SCHEME);
+            $urlScheme = $urlScheme ? strtolower($urlScheme) : '';
+            if ($urlScheme !== 'http' && $urlScheme !== 'https') {
+                $url = null;
+            }
+        }
+
+        if (!$url) {
+            $url = '#';
+        }
+
+        return $url;
+    }
 }
diff --git a/app/code/Magento/Widget/Block/Adminhtml/Widget.php b/app/code/Magento/Widget/Block/Adminhtml/Widget.php
@@ -12,11 +12,12 @@
  *
  * @api
  * @since 100.0.2
+ * @SuppressWarnings(PHPMD.RequestAwareBlockMethod)
  */
 class Widget extends \Magento\Backend\Block\Widget\Form\Container
 {
     /**
-     * @return void
+     * @inheritdoc
      */
     protected function _construct()
     {
@@ -36,12 +37,16 @@ protected function _construct()
         $this->buttonList->update('save', 'region', 'footer');
         $this->buttonList->update('save', 'data_attribute', []);
 
-        $this->_formScripts[] = 'require(["mage/adminhtml/wysiwyg/widget"], function(){wWidget = new WysiwygWidget.Widget(' .
-            '"widget_options_form", "select_widget_type", "widget_options", "' .
-            $this->getUrl(
-                'adminhtml/*/loadOptions'
-            ) . '", "' . $this->getRequest()->getParam(
-                'widget_target_id'
-            ) . '");});';
+        $this->_formScripts[] = <<<EOJS
+ 		require(['mage/adminhtml/wysiwyg/widget'], function() {
+ 		    wWidget = new WysiwygWidget.Widget(
+ 		        'widget_options_form',
+ 		        'select_widget_type',
+ 		        'widget_options',
+ 		        '{$this->getUrl('adminhtml/*/loadOptions')}',
+ 		        '{$this->escapeJs($this->getRequest()->getParam('widget_target_id'))}'
+ 		    );
+ 		});
+EOJS;
     }
 }
diff --git a/dev/tests/static/framework/Magento/CodeMessDetector/Rule/Design/RequestAwareBlockMethod.php b/dev/tests/static/framework/Magento/CodeMessDetector/Rule/Design/RequestAwareBlockMethod.php
@@ -0,0 +1,53 @@
+<?php
+/**
+ * Copyright © Magento, Inc. All rights reserved.
+ * See COPYING.txt for license details.
+ */
+
+declare(strict_types=1);
+
+namespace Magento\CodeMessDetector\Rule\Design;
+
+use PHPMD\AbstractNode;
+use PHPMD\AbstractRule;
+use PHPMD\Node\ClassNode;
+use PHPMD\Node\MethodNode;
+use PDepend\Source\AST\ASTMethod;
+use PHPMD\Rule\MethodAware;
+
+/**
+ * Detect direct request usages.
+ */
+class RequestAwareBlockMethod extends AbstractRule implements MethodAware
+{
+    /**
+     * @inheritDoc
+     *
+     * @param ASTMethod|MethodNode $method
+     */
+    public function apply(AbstractNode $method)
+    {
+        $definedIn = $method->getParentType();
+        try {
+            $isBlock = ($definedIn instanceof ClassNode)
+                && is_subclass_of(
+                    $definedIn->getFullQualifiedName(),
+                    \Magento\Framework\View\Element\AbstractBlock::class
+                );
+        } catch (\Throwable $exception) {
+            //Failed to load classes.
+            return;
+        }
+
+        if ($isBlock) {
+            $nodes = $method->findChildrenOfType('PropertyPostfix') + $method->findChildrenOfType('MethodPostfix');
+            foreach ($nodes as $node) {
+                $name = mb_strtolower($node->getFirstChildOfType('Identifier')->getImage());
+                if ($name === '_request' || $name === 'getrequest') {
+                    $this->addViolation($method, [$method->getFullQualifiedName()]);
+                    break;
+                }
+            }
+        }
+    }
+}
diff --git a/dev/tests/static/framework/Magento/CodeMessDetector/resources/rulesets/design.xml b/dev/tests/static/framework/Magento/CodeMessDetector/resources/rulesets/design.xml
@@ -29,6 +29,37 @@ final class Foo
 }
 class Baz {
     final public function bad() {}
+}
+            ]]>
+        </example>
+    </rule>
+    <rule name="RequestAwareBlockMethod"
+          class="Magento\CodeMessDetector\Rule\Design\RequestAwareBlockMethod"
+          message="{0} uses request object directly. Add user input validation and suppress this warning.">
+        <description>
+            <![CDATA[
+Blocks must not depend on being used with certain controllers.
+If you use request object in a block directly you must validate all user input inside the block.
+            ]]>
+        </description>
+        <priority>2</priority>
+        <properties />
+        <example>
+            <![CDATA[
+class MyOrder extends AbstractBlock
+{
+
+    .......
+
+    public function getOrder()
+    {
+        $orderId = $this->getRequest()->getParam('order_id');
+        //Validate customer having such order.
+        if (!$this->hasOrder($this->getCustomerId(), $orderId)) {
+            ...deny access...
+        }
+        .....
+    }
 }
             ]]>
         </example>
diff --git a/dev/tests/static/testsuite/Magento/Test/Php/_files/phpmd/ruleset.xml b/dev/tests/static/testsuite/Magento/Test/Php/_files/phpmd/ruleset.xml
@@ -47,5 +47,5 @@
 
     <!-- Magento Specific Rules -->
     <rule ref="Magento/CodeMessDetector/resources/rulesets/design.xml/FinalImplementation" />
-
+    <rule ref="Magento/CodeMessDetector/resources/rulesets/design.xml/RequestAwareBlockMethod" />
 </ruleset>
diff --git a/lib/internal/Magento/Framework/Escaper.php b/lib/internal/Magento/Framework/Escaper.php
@@ -3,6 +3,7 @@
  * Copyright © Magento, Inc. All rights reserved.
  * See COPYING.txt for license details.
  */
+
 namespace Magento\Framework;
 
 /**
@@ -32,38 +33,43 @@ class Escaper
      */
     private $allowedAttributes = ['id', 'class', 'href', 'target', 'title', 'style'];
 
+    /**
+     * @var string
+     */
+    private static $xssFiltrationPattern =
+        '/((javascript(\\\\x3a|:|%3A))|(data(\\\\x3a|:|%3A))|(vbscript:))|'
+        . '((\\\\x6A\\\\x61\\\\x76\\\\x61\\\\x73\\\\x63\\\\x72\\\\x69\\\\x70\\\\x74(\\\\x3a|:|%3A))|'
+        . '(\\\\x64\\\\x61\\\\x74\\\\x61(\\\\x3a|:|%3A)))/i';
+
     /**
      * @var string[]
      */
     private $escapeAsUrlAttributes = ['href'];
 
     /**
-     * Escape string for HTML context. allowedTags will not be escaped, except the following: script, img, embed,
-     * iframe, video, source, object, audio
+     * Escape string for HTML context.
+     *
+     * AllowedTags will not be escaped, except the following: script, img, embed,
+     * iframe, video, source, object, audio.
      *
      * @param string|array $data
      * @param array|null $allowedTags
      * @return string|array
      */
     public function escapeHtml($data, $allowedTags = null)
     {
+        if (!is_array($data)) {
+            $data = (string)$data;
+        }
+
         if (is_array($data)) {
             $result = [];
             foreach ($data as $item) {
                 $result[] = $this->escapeHtml($item, $allowedTags);
             }
         } elseif (strlen($data)) {
             if (is_array($allowedTags) && !empty($allowedTags)) {
-                $notAllowedTags = array_intersect(
-                    array_map('strtolower', $allowedTags),
-                    $this->notAllowedTags
-                );
-                if (!empty($notAllowedTags)) {
-                    $this->getLogger()->critical(
-                        'The following tag(s) are not allowed: ' . implode(', ', $notAllowedTags)
-                    );
-                    $allowedTags = array_diff($allowedTags, $this->notAllowedTags);
-                }
+                $allowedTags = $this->filterProhibitedTags($allowedTags);
                 $wrapperElementId = uniqid();
                 $domDocument = new \DOMDocument('1.0', 'UTF-8');
                 set_error_handler(
@@ -197,7 +203,7 @@ public function escapeHtmlAttr($string, $escapeSingleQuote = true)
         if ($escapeSingleQuote) {
             return $this->getEscaper()->escapeHtmlAttr((string) $string);
         }
-        return htmlspecialchars($string, ENT_COMPAT, 'UTF-8', false);
+        return htmlspecialchars((string)$string, ENT_COMPAT, 'UTF-8', false);
     }
 
     /**
@@ -278,30 +284,47 @@ public function escapeJsQuote($data, $quote = '\'')
                 $result[] = $this->escapeJsQuote($item, $quote);
             }
         } else {
-            $result = str_replace($quote, '\\' . $quote, $data);
+            $result = str_replace($quote, '\\' . $quote, (string)$data);
         }
         return $result;
     }
 
     /**
      * Escape xss in urls
-     * Remove `javascript:`, `vbscript:`, `data:` words from url
      *
      * @param string $data
      * @return string
      * @deprecated 100.2.0
      */
     public function escapeXssInUrl($data)
     {
-        $pattern = '/((javascript(\\\\x3a|:|%3A))|(data(\\\\x3a|:|%3A))|(vbscript:))|'
-            . '((\\\\x6A\\\\x61\\\\x76\\\\x61\\\\x73\\\\x63\\\\x72\\\\x69\\\\x70\\\\x74(\\\\x3a|:|%3A))|'
-            . '(\\\\x64\\\\x61\\\\x74\\\\x61(\\\\x3a|:|%3A)))/i';
-        $result = preg_replace($pattern, ':', $data);
-        return htmlspecialchars($result, ENT_COMPAT | ENT_HTML5 | ENT_HTML401, 'UTF-8', false);
+        return htmlspecialchars(
+            $this->escapeScriptIdentifiers((string)$data),
+            ENT_COMPAT | ENT_HTML5 | ENT_HTML401,
+            'UTF-8',
+            false
+        );
+    }
+
+    /**
+     * Remove `javascript:`, `vbscript:`, `data:` words from the string.
+     *
+     * @param string $data
+     * @return string
+     */
+    private function escapeScriptIdentifiers(string $data): string
+    {
+        $filteredData = preg_replace(self::$xssFiltrationPattern, ':', $data) ?: '';
+        if (preg_match(self::$xssFiltrationPattern, $filteredData)) {
+            $filteredData = $this->escapeScriptIdentifiers($filteredData);
+        }
+
+        return $filteredData;
     }
 
     /**
      * Escape quotes inside html attributes
+     *
      * Use $addSlashes = false for escaping js that inside html attribute (onClick, onSubmit etc)
      *
      * @param string $data
@@ -346,4 +369,27 @@ private function getLogger()
         }
         return $this->logger;
     }
+
+    /**
+     * Filter prohibited tags.
+     *
+     * @param string[] $allowedTags
+     * @return string[]
+     */
+    private function filterProhibitedTags(array $allowedTags): array
+    {
+        $notAllowedTags = array_intersect(
+            array_map('strtolower', $allowedTags),
+            $this->notAllowedTags
+        );
+
+        if (!empty($notAllowedTags)) {
+            $this->getLogger()->critical(
+                'The following tag(s) are not allowed: ' . implode(', ', $notAllowedTags)
+            );
+            $allowedTags = array_diff($allowedTags, $this->notAllowedTags);
+        }
+
+        return $allowedTags;
+    }
 }
diff --git a/lib/internal/Magento/Framework/View/Element/AbstractBlock.php b/lib/internal/Magento/Framework/View/Element/AbstractBlock.php
