Merge pull request #5327 from magento-qwerty/2.3-bugfixes-02072020

dvoskoboinikov · web-flow · commit ab6e459b92d8 · 2020-02-21T15:33:51.000-06:00
Fixed Issues:
- MC-30971: CSP policies must be defined in a single header
- MC-30981: CSP rules not enabled on first login to admin
- MC-30403: “What is this ?” button does not work correctly
diff --git a/app/code/Magento/Backend/Block/Store/Switcher.php b/app/code/Magento/Backend/Block/Store/Switcher.php
@@ -473,6 +473,8 @@ public function getCurrentWebsiteName()
                 return $website->getName();
             }
         }
+
+        return '';
     }
 
     /**
@@ -489,6 +491,8 @@ public function getCurrentStoreGroupName()
                 return $group->getName();
             }
         }
+
+        return '';
     }
 
     /**
@@ -505,6 +509,8 @@ public function getCurrentStoreName()
                 return $store->getName();
             }
         }
+
+        return '';
     }
 
     /**
@@ -590,7 +596,7 @@ public function getHintHtml()
             class="admin__field-tooltip-action action-help"><span>%s</span></a></span></div>';
             $title =  $this->escapeHtmlAttr(__('What is this?'));
             $span= $this->escapeHtml(__('What is this?'));
-            sprintf($html, $this->escapeUrl($url), $title, $span);
+            $html = sprintf($html, $this->escapeUrl($url), $title, $span);
         }
         return $html;
     }
diff --git a/app/code/Magento/Csp/Model/Collector/CspWhitelistXml/Data.php b/app/code/Magento/Csp/Model/Collector/CspWhitelistXml/Data.php
@@ -0,0 +1,44 @@
+<?php
+/**
+ * Copyright © Magento, Inc. All rights reserved.
+ * See COPYING.txt for license details.
+ */
+
+declare(strict_types=1);
+
+namespace Magento\Csp\Model\Collector\CspWhitelistXml;
+
+use Magento\Framework\Serialize\SerializerInterface;
+use Magento\Framework\Config\Data\Scoped;
+use Magento\Framework\Config\ScopeInterface;
+use Magento\Framework\Config\CacheInterface;
+
+/**
+ * Provides CSP whitelist configuration
+ */
+class Data extends Scoped
+{
+    /**
+     * Scope priority loading scheme
+     *
+     * @var array
+     */
+    protected $_scopePriorityScheme = ['global'];
+
+    /**
+     * Constructor
+     *
+     * @param Reader $reader
+     * @param ScopeInterface $configScope
+     * @param CacheInterface $cache
+     * @param SerializerInterface $serializer
+     */
+    public function __construct(
+        Reader $reader,
+        ScopeInterface $configScope,
+        CacheInterface $cache,
+        SerializerInterface $serializer
+    ) {
+        parent::__construct($reader, $configScope, $cache, 'csp_whitelist_config', $serializer);
+    }
+}
diff --git a/app/code/Magento/Csp/Model/Collector/CspWhitelistXmlCollector.php b/app/code/Magento/Csp/Model/Collector/CspWhitelistXmlCollector.php
@@ -9,7 +9,7 @@
 namespace Magento\Csp\Model\Collector;
 
 use Magento\Csp\Api\PolicyCollectorInterface;
-use Magento\Csp\Model\Collector\CspWhitelistXml\Reader as ConfigReader;
+use Magento\Framework\Config\DataInterface as ConfigReader;
 use Magento\Csp\Model\Policy\FetchPolicy;
 
 /**
@@ -36,7 +36,7 @@ public function __construct(ConfigReader $configReader)
     public function collect(array $defaultPolicies = []): array
     {
         $policies = $defaultPolicies;
-        $config = $this->configReader->read();
+        $config = $this->configReader->get(null);
         foreach ($config as $policyId => $values) {
             $policies[] = new FetchPolicy(
                 $policyId,
diff --git a/app/code/Magento/Csp/Model/Policy/Renderer/SimplePolicyHeaderRenderer.php b/app/code/Magento/Csp/Model/Policy/Renderer/SimplePolicyHeaderRenderer.php
@@ -45,7 +45,7 @@ public function render(PolicyInterface $policy, HttpResponse $response): void
             $header = 'Content-Security-Policy';
         }
         $value = $policy->getId() .' ' .$policy->getValue() .';';
-        if ($config->getReportUri()) {
+        if ($config->getReportUri() && !$response->getHeader('Report-To')) {
             $reportToData = [
                 'group' => 'report-endpoint',
                 'max_age' => 10886400,
@@ -57,7 +57,10 @@ public function render(PolicyInterface $policy, HttpResponse $response): void
             $value .= ' report-to '. $reportToData['group'] .';';
             $response->setHeader('Report-To', json_encode($reportToData), true);
         }
-        $response->setHeader($header, $value, false);
+        if ($existing = $response->getHeader($header)) {
+            $value = $value .' ' .$existing->getFieldValue();
+        }
+        $response->setHeader($header, $value, true);
     }
 
     /**
diff --git a/app/code/Magento/Csp/etc/di.xml b/app/code/Magento/Csp/etc/di.xml
@@ -49,6 +49,16 @@
             <argument name="fileName" xsi:type="string">csp_whitelist.xml</argument>
         </arguments>
     </type>
+    <type name="Magento\Csp\Model\Collector\CspWhitelistXml\Data">
+        <arguments>
+            <argument name="reader" xsi:type="object">Magento\Csp\Model\Collector\CspWhitelistXml\Reader\Proxy</argument>
+        </arguments>
+    </type>
+    <type name="Magento\Csp\Model\Collector\CspWhitelistXmlCollector">
+        <arguments>
+            <argument name="configReader" xsi:type="object">Magento\Csp\Model\Collector\CspWhitelistXml\Data</argument>
+        </arguments>
+    </type>
     <type name="Magento\Framework\View\TemplateEngine\Php">
         <plugin name="csp_helper_plugin" type="Magento\Csp\Plugin\TemplateRenderingPlugin" />
     </type>
diff --git a/dev/tests/integration/testsuite/Magento/Csp/CspAwareActionTest.php b/dev/tests/integration/testsuite/Magento/Csp/CspAwareActionTest.php
@@ -31,15 +31,13 @@ public function testAwareAction(): void
     {
         $this->getRequest()->setMethod('GET');
         $this->dispatch('csputil/csp/aware');
-        $headers = '';
-        foreach ($this->getResponse()->getHeaders() as $header) {
-            $headers .= $header->getFieldName() .': ' .$header->getFieldValue() .PHP_EOL;
-        }
+        $header = $this->getResponse()->getHeader('Content-Security-Policy');
+        $this->assertNotEmpty($header);
 
         $this->assertContains(
-            'Content-Security-Policy: script-src https://controller.magento.com'
+            'script-src https://controller.magento.com'
                 .' \'self\' \'sha256-H4RRnauTM2X2Xg/z9zkno1crqhsaY3uKKu97uwmnXXE=\'',
-            $headers
+            $header->getFieldValue()
         );
     }
 }
diff --git a/dev/tests/integration/testsuite/Magento/Csp/CspUtilTest.php b/dev/tests/integration/testsuite/Magento/Csp/CspUtilTest.php
@@ -20,6 +20,7 @@ class CspUtilTest extends AbstractController
      * Test that CSP helper for templates works.
      *
      * @return void
+     * @magentoConfigFixture default_store csp/mode/storefront/report_only 0
      */
     public function testPhtmlHelper(): void
     {
@@ -29,11 +30,9 @@ public function testPhtmlHelper(): void
 
         $this->assertContains('<script src="http://my.magento.com/static/script.js" />', $content);
         $this->assertContains("<script>\n    let myVar = 1;\n</script>", $content);
-        $headers = '';
-        foreach ($this->getResponse()->getHeaders() as $header) {
-            $headers .= $header->getFieldName() .': ' .$header->getFieldValue() .PHP_EOL;
-        }
-        $this->assertContains('http://my.magento.com', $headers);
-        $this->assertContains('\'sha256-H4RRnauTM2X2Xg/z9zkno1crqhsaY3uKKu97uwmnXXE=\'', $headers);
+        $header = $this->getResponse()->getHeader('Content-Security-Policy');
+        $this->assertNotEmpty($header);
+        $this->assertContains('http://my.magento.com', $header->getFieldValue());
+        $this->assertContains('\'sha256-H4RRnauTM2X2Xg/z9zkno1crqhsaY3uKKu97uwmnXXE=\'', $header->getFieldValue());
     }
 }
diff --git a/dev/tests/integration/testsuite/Magento/Csp/Model/Policy/Renderer/SimplePolicyHeaderRendererTest.php b/dev/tests/integration/testsuite/Magento/Csp/Model/Policy/Renderer/SimplePolicyHeaderRendererTest.php
@@ -53,13 +53,7 @@ public function testRenderRestrictMode(): void
 
         $this->assertNotEmpty($header = $this->response->getHeader('Content-Security-Policy'));
         $this->assertEmpty($this->response->getHeader('Content-Security-Policy-Report-Only'));
-        $contentSecurityPolicyContent = [];
-        if ($header instanceof \ArrayIterator) {
-            foreach ($header as $item) {
-                $contentSecurityPolicyContent[] = $item->getFieldValue();
-            }
-        }
-        $this->assertEquals(['default-src https://magento.com \'self\';'], $contentSecurityPolicyContent);
+        $this->assertEquals('default-src https://magento.com \'self\';', $header->getFieldValue());
     }
 
     /**
@@ -79,15 +73,9 @@ public function testRenderRestrictWithReportingMode(): void
 
         $this->assertNotEmpty($header = $this->response->getHeader('Content-Security-Policy'));
         $this->assertEmpty($this->response->getHeader('Content-Security-Policy-Report-Only'));
-        $contentSecurityPolicyContent = [];
-        if ($header instanceof \ArrayIterator) {
-            foreach ($header as $item) {
-                $contentSecurityPolicyContent[] = $item->getFieldValue();
-            }
-        }
         $this->assertEquals(
-            ['default-src https://magento.com \'self\'; report-uri /csp-reports/; report-to report-endpoint;'],
-            $contentSecurityPolicyContent
+            'default-src https://magento.com \'self\'; report-uri /csp-reports/; report-to report-endpoint;',
+            $header->getFieldValue()
         );
         $this->assertNotEmpty($reportToHeader = $this->response->getHeader('Report-To'));
         $this->assertNotEmpty($reportData = json_decode("[{$reportToHeader->getFieldValue()}]", true));
diff --git a/lib/internal/Magento/Framework/HTTP/PhpEnvironment/Response.php b/lib/internal/Magento/Framework/HTTP/PhpEnvironment/Response.php
@@ -28,6 +28,9 @@ public function getHeader($name)
         $headers = $this->getHeaders();
         if ($headers->has($name)) {
             $header = $headers->get($name);
+            if (is_iterable($header)) {
+                $header = $header[0];
+            }
         }
         return $header;
     }
@@ -94,8 +97,13 @@ public function clearHeader($name)
     {
         $headers = $this->getHeaders();
         if ($headers->has($name)) {
-            $header = $headers->get($name);
-            $headers->removeHeader($header);
+            $headerValues = $headers->get($name);
+            if (!is_iterable($headerValues)) {
+                $headerValues = [$headerValues];
+            }
+            foreach ($headerValues as $headerValue) {
+                $headers->removeHeader($headerValue);
+            }
         }
 
         return $this;
@@ -203,7 +211,6 @@ public function sendHeaders()
             header($header->toString(), false);
         }
 
-        $this->headersSent = true;
         return $this;
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -473,6 +473,8 @@ public function getCurrentWebsiteName()`
`473`	`473`	`return $website->getName();`
`474`	`474`	`}`
`475`	`475`	`}`
	`476`	`+`
	`477`	`+ return '';`
`476`	`478`	`}`
`477`	`479`
`478`	`480`	`/**`
`@@ -489,6 +491,8 @@ public function getCurrentStoreGroupName()`
`489`	`491`	`return $group->getName();`
`490`	`492`	`}`
`491`	`493`	`}`
	`494`	`+`
	`495`	`+ return '';`
`492`	`496`	`}`
`493`	`497`
`494`	`498`	`/**`
`@@ -505,6 +509,8 @@ public function getCurrentStoreName()`
`505`	`509`	`return $store->getName();`
`506`	`510`	`}`
`507`	`511`	`}`
	`512`	`+`
	`513`	`+ return '';`
`508`	`514`	`}`
`509`	`515`
`510`	`516`	`/**`
`@@ -590,7 +596,7 @@ public function getHintHtml()`
`590`	`596`	`class="admin__field-tooltip-action action-help"><span>%s</span></a></span></div>';`
`591`	`597`	`$title = $this->escapeHtmlAttr(__('What is this?'));`
`592`	`598`	`$span= $this->escapeHtml(__('What is this?'));`
`593`		`- sprintf($html, $this->escapeUrl($url), $title, $span);`
	`599`	`+ $html = sprintf($html, $this->escapeUrl($url), $title, $span);`
`594`	`600`	`}`
`595`	`601`	`return $html;`
`596`	`602`	`}`
Original file line number	Diff line number	Diff line change
`@@ -45,7 +45,7 @@ public function render(PolicyInterface $policy, HttpResponse $response): void`
`45`	`45`	`$header = 'Content-Security-Policy';`
`46`	`46`	`}`
`47`	`47`	`$value = $policy->getId() .' ' .$policy->getValue() .';';`
`48`		`- if ($config->getReportUri()) {`
	`48`	`+ if ($config->getReportUri() && !$response->getHeader('Report-To')) {`
`49`	`49`	`$reportToData = [`
`50`	`50`	`'group' => 'report-endpoint',`
`51`	`51`	`'max_age' => 10886400,`
`@@ -57,7 +57,10 @@ public function render(PolicyInterface $policy, HttpResponse $response): void`
`57`	`57`	`$value .= ' report-to '. $reportToData['group'] .';';`
`58`	`58`	`$response->setHeader('Report-To', json_encode($reportToData), true);`
`59`	`59`	`}`
`60`		`- $response->setHeader($header, $value, false);`
	`60`	`+ if ($existing = $response->getHeader($header)) {`
	`61`	`+ $value = $value .' ' .$existing->getFieldValue();`
	`62`	`+ }`
	`63`	`+ $response->setHeader($header, $value, true);`
`61`	`64`	`}`
`62`	`65`
`63`	`66`	`/**`
Original file line number	Diff line number	Diff line change
`@@ -31,15 +31,13 @@ public function testAwareAction(): void`
`31`	`31`	`{`
`32`	`32`	`$this->getRequest()->setMethod('GET');`
`33`	`33`	`$this->dispatch('csputil/csp/aware');`
`34`		`- $headers = '';`
`35`		`- foreach ($this->getResponse()->getHeaders() as $header) {`
`36`		`- $headers .= $header->getFieldName() .': ' .$header->getFieldValue() .PHP_EOL;`
`37`		`- }`
	`34`	`+ $header = $this->getResponse()->getHeader('Content-Security-Policy');`
	`35`	`+ $this->assertNotEmpty($header);`
`38`	`36`
`39`	`37`	`$this->assertContains(`
`40`		`- 'Content-Security-Policy: script-src https://controller.magento.com'`
	`38`	`+ 'script-src https://controller.magento.com'`
`41`	`39`	`.' \'self\' \'sha256-H4RRnauTM2X2Xg/z9zkno1crqhsaY3uKKu97uwmnXXE=\'',`
`42`		`- $headers`
	`40`	`+ $header->getFieldValue()`
`43`	`41`	`);`
`44`	`42`	`}`
`45`	`43`	`}`
Original file line number	Diff line number	Diff line change
`@@ -20,6 +20,7 @@ class CspUtilTest extends AbstractController`
`20`	`20`	`* Test that CSP helper for templates works.`
`21`	`21`	`*`
`22`	`22`	`* @return void`
	`23`	`+ * @magentoConfigFixture default_store csp/mode/storefront/report_only 0`
`23`	`24`	`*/`
`24`	`25`	`public function testPhtmlHelper(): void`
`25`	`26`	`{`
`@@ -29,11 +30,9 @@ public function testPhtmlHelper(): void`
`29`	`30`
`30`	`31`	`$this->assertContains('<script src="http://my.magento.com/static/script.js" />', $content);`
`31`	`32`	`$this->assertContains("<script>\n let myVar = 1;\n</script>", $content);`
`32`		`- $headers = '';`
`33`		`- foreach ($this->getResponse()->getHeaders() as $header) {`
`34`		`- $headers .= $header->getFieldName() .': ' .$header->getFieldValue() .PHP_EOL;`
`35`		`- }`
`36`		`- $this->assertContains('http://my.magento.com', $headers);`
`37`		`- $this->assertContains('\'sha256-H4RRnauTM2X2Xg/z9zkno1crqhsaY3uKKu97uwmnXXE=\'', $headers);`
	`33`	`+ $header = $this->getResponse()->getHeader('Content-Security-Policy');`
	`34`	`+ $this->assertNotEmpty($header);`
	`35`	`+ $this->assertContains('http://my.magento.com', $header->getFieldValue());`
	`36`	`+ $this->assertContains('\'sha256-H4RRnauTM2X2Xg/z9zkno1crqhsaY3uKKu97uwmnXXE=\'', $header->getFieldValue());`
`38`	`37`	`}`
`39`	`38`	`}`
Original file line number	Diff line number	Diff line change
`@@ -28,6 +28,9 @@ public function getHeader($name)`
`28`	`28`	`$headers = $this->getHeaders();`
`29`	`29`	`if ($headers->has($name)) {`
`30`	`30`	`$header = $headers->get($name);`
	`31`	`+ if (is_iterable($header)) {`
	`32`	`+ $header = $header[0];`
	`33`	`+ }`
`31`	`34`	`}`
`32`	`35`	`return $header;`
`33`	`36`	`}`
`@@ -94,8 +97,13 @@ public function clearHeader($name)`
`94`	`97`	`{`
`95`	`98`	`$headers = $this->getHeaders();`
`96`	`99`	`if ($headers->has($name)) {`
`97`		`- $header = $headers->get($name);`
`98`		`- $headers->removeHeader($header);`
	`100`	`+ $headerValues = $headers->get($name);`
	`101`	`+ if (!is_iterable($headerValues)) {`
	`102`	`+ $headerValues = [$headerValues];`
	`103`	`+ }`
	`104`	`+ foreach ($headerValues as $headerValue) {`
	`105`	`+ $headers->removeHeader($headerValue);`
	`106`	`+ }`
`99`	`107`	`}`
`100`	`108`
`101`	`109`	`return $this;`
`@@ -203,7 +211,6 @@ public function sendHeaders()`
`203`	`211`	`header($header->toString(), false);`
`204`	`212`	`}`
`205`	`213`
`206`		`- $this->headersSent = true;`
`207`	`214`	`return $this;`
`208`	`215`	`}`
`209`	`216`	`}`