Skip to content

Commit

Permalink
Proposal: HTTP Response - Fix CSP non existing object when CSP is dis…
Browse files Browse the repository at this point in the history
…abled

If CSP is disabled property $CSP in HTTP/Response is not initialized.

If we try to access the CSP methods on the request object anywhere in code with CSP disabled it will crash the framework with "Call to a member function …. on null "

In order to avoid this CSP object can be initiated regardless of CSP config.

I’m aware that this is not the most efficient way to bypass the issue but some mechanism for disabling CSP should exist without having to do modifications everywhere in code.

Maybe better idea will be to create mock class to be loaded instead which will respond with catchall  magic methods like __call __set __get ….. But I don’t know if it is worth doing it as it will require adding additional class in framework.

Ref #2456

Added unit test testCSPDisabled()
  • Loading branch information
najdanovicivan committed Jan 22, 2020
1 parent fedae3c commit 947d2aa
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 8 deletions.
9 changes: 3 additions & 6 deletions system/HTTP/Response.php
Original file line number Diff line number Diff line change
Expand Up @@ -239,13 +239,10 @@ public function __construct($config)
// Also ensures that a Cache-control header exists.
$this->noCache();

// Are we enforcing a Content Security Policy?
if ($config->CSPEnabled === true)
{
$this->CSP = new ContentSecurityPolicy(new \Config\ContentSecurityPolicy());
$this->CSPEnabled = true;
}
// We need CSP object even if not enabled to avoid calls to non existing methods
$this->CSP = new ContentSecurityPolicy(new \Config\ContentSecurityPolicy());

$this->CSPEnabled = $config->CSPEnabled;
$this->cookiePrefix = $config->cookiePrefix;
$this->cookieDomain = $config->cookieDomain;
$this->cookiePath = $config->cookiePath;
Expand Down
17 changes: 15 additions & 2 deletions tests/system/HTTP/ContentSecurityPolicyTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@ class ContentSecurityPolicyTest extends \CIUnitTestCase
{

// Having this method as setUp() doesn't work - can't find Config\App !?
protected function prepare()
protected function prepare(bool $CSPEnabled = true)
{
$config = new App();
$config->CSPEnabled = true;
$config->CSPEnabled = $CSPEnabled;
$this->response = new Response($config);
$this->response->pretend(false);
$this->csp = $this->response->CSP;
Expand Down Expand Up @@ -490,4 +490,17 @@ public function testHeaderIgnoreCase()
$this->assertContains("base-uri 'self';", $result);
}

/**
* @runInSeparateProcess
* @preserveGlobalState disabled
*/
public function testCSPDisabled()
{
$this->prepare(false);
$result = $this->work();
$this->response->CSP->addStyleSrc('https://example.com');

$this->assertHeaderNotEmitted('content-security-policy', true);
}

}

0 comments on commit 947d2aa

Please sign in to comment.