From cd946b6c80957c6b55ca63f0cabbe659e72c8a95 Mon Sep 17 00:00:00 2001 From: Florian Thoma Date: Wed, 5 Apr 2023 16:33:41 +1000 Subject: [PATCH 1/4] Group visibility for SITETREE_GRANT_ACCESS permissions Make groups visible if member has SITETREE_GRANT_ACCESS permissions, otherwise the dropdown for selecting the group is empty --- src/Security/Group.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/Security/Group.php b/src/Security/Group.php index eb279e111..aba30eaf6 100755 --- a/src/Security/Group.php +++ b/src/Security/Group.php @@ -635,6 +635,11 @@ class Group extends DataObject return true; } + // if user can grant access for specific groups, they need to be able to see the groups + if (Permission::checkMember($member, "SITETREE_GRANT_ACCESS")) { + return true; + } + return false; } From 92061a3ba66d2e677e4b3f1a5a4cc269480f6534 Mon Sep 17 00:00:00 2001 From: Dylan Wagstaff Date: Tue, 11 Apr 2023 10:52:41 +1200 Subject: [PATCH 2/4] FIX stabilise typed APIs (#10740) Since 4.12 the use of typehints and return types has caused issues with values fetched directly from config without validation. This has lead to upgrade woes in a minor version (#10721) with no immediate recourse other than manual system intervention. To use types, we should ensure types, leaving a stable API that won't error on a bad value - or should give a thoughtful and directive error message if so. Issue #10721 summary: SessionMiddleware runs before FlushMiddleware SessionMiddleware causes a PHP fatal error passing `null` to a `string` parameter. `null` comes from config, because default string value doesn't exist. We need flush for this - but system execution never makes it that far. --- src/Control/Cookie.php | 16 +++++++++++----- src/Control/CookieJar.php | 4 ++-- src/Control/Session.php | 4 ++-- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/Control/Cookie.php b/src/Control/Cookie.php index aafefb82b..2995e844f 100644 --- a/src/Control/Cookie.php +++ b/src/Control/Cookie.php @@ -14,6 +14,12 @@ class Cookie { use Configurable; + public const SAMESITE_LAX = 'Lax'; + + public const SAMESITE_STRICT = 'Strict'; + + public const SAMESITE_NONE = 'None'; + /** * @config * @@ -25,7 +31,7 @@ class Cookie * Must be "Strict", "Lax", or "None" * @config */ - private static string $default_samesite = 'Lax'; + private static string $default_samesite = self::SAMESITE_LAX; /** * Fetch the current instance of the cookie backend. @@ -110,14 +116,14 @@ class Cookie public static function validateSameSite(string $sameSite): void { $validValues = [ - 'Strict', - 'Lax', - 'None', + self::SAMESITE_STRICT, + self::SAMESITE_LAX, + self::SAMESITE_NONE, ]; if (!in_array($sameSite, $validValues)) { throw new LogicException('Cookie samesite must be "Strict", "Lax", or "None"'); } - if ($sameSite === 'None' && !Director::is_https(self::getRequest())) { + if ($sameSite === self::SAMESITE_NONE && !Director::is_https(self::getRequest())) { Injector::inst()->get(LoggerInterface::class)->warning('Cookie samesite cannot be "None" for non-https requests.'); } } diff --git a/src/Control/CookieJar.php b/src/Control/CookieJar.php index 73467526c..04b5ee74b 100644 --- a/src/Control/CookieJar.php +++ b/src/Control/CookieJar.php @@ -207,8 +207,8 @@ class CookieJar implements Cookie_Backend { Deprecation::notice('4.12.0', 'The relevant methods will include a `$sameSite` parameter instead.'); if ($name === session_name()) { - return Session::config()->get('cookie_samesite'); + return Session::config()->get('cookie_samesite') ?? Cookie::SAMESITE_LAX; } - return Cookie::config()->get('default_samesite'); + return Cookie::config()->get('default_samesite') ?? Cookie::SAMESITE_LAX; } } diff --git a/src/Control/Session.php b/src/Control/Session.php index 01623d87f..31ed9b426 100644 --- a/src/Control/Session.php +++ b/src/Control/Session.php @@ -139,7 +139,7 @@ class Session * Must be "Strict", "Lax", or "None". * @config */ - private static string $cookie_samesite = 'Lax'; + private static string $cookie_samesite = Cookie::SAMESITE_LAX; /** * Name of session cache limiter to use. @@ -374,7 +374,7 @@ class Session } } - $sameSite = static::config()->get('cookie_samesite'); + $sameSite = static::config()->get('cookie_samesite') ?? Cookie::SAMESITE_LAX; Cookie::validateSameSite($sameSite); $secure = $this->isCookieSecure($sameSite, Director::is_https($request)); From fd5d8217e83768d7bf841e94b2d4d82642d5bc58 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Tue, 14 Feb 2023 12:52:39 +1300 Subject: [PATCH 3/4] [CVE-2023-22728] Check canView before printing from GridField --- src/Forms/GridField/GridFieldPrintButton.php | 24 ++++++++++--------- .../GridField/GridFieldPrintButtonTest.php | 16 +++++++++++-- .../GridFieldPrintButtonTest/TestObject.php | 7 ++++++ 3 files changed, 34 insertions(+), 13 deletions(-) diff --git a/src/Forms/GridField/GridFieldPrintButton.php b/src/Forms/GridField/GridFieldPrintButton.php index 1ec2be669..1fc965808 100644 --- a/src/Forms/GridField/GridFieldPrintButton.php +++ b/src/Forms/GridField/GridFieldPrintButton.php @@ -228,21 +228,23 @@ class GridFieldPrintButton extends AbstractGridFieldComponent implements GridFie /** @var DataObject $item */ foreach ($items->limit(null) as $item) { - $itemRow = new ArrayList(); + if (!$item->hasMethod('canView') || $item->canView()) { + $itemRow = new ArrayList(); - foreach ($printColumns as $field => $label) { - $value = $gridFieldColumnsComponent - ? strip_tags($gridFieldColumnsComponent->getColumnContent($gridField, $item, $field)) - : $gridField->getDataFieldValue($item, $field); + foreach ($printColumns as $field => $label) { + $value = $gridFieldColumnsComponent + ? strip_tags($gridFieldColumnsComponent->getColumnContent($gridField, $item, $field)) + : $gridField->getDataFieldValue($item, $field); - $itemRow->push(new ArrayData([ - "CellString" => $value, + $itemRow->push(new ArrayData([ + "CellString" => $value, + ])); + } + + $itemRows->push(new ArrayData([ + "ItemRow" => $itemRow ])); } - - $itemRows->push(new ArrayData([ - "ItemRow" => $itemRow - ])); if ($item->hasMethod('destroy')) { $item->destroy(); } diff --git a/tests/php/Forms/GridField/GridFieldPrintButtonTest.php b/tests/php/Forms/GridField/GridFieldPrintButtonTest.php index 59e73a494..f78c305ba 100644 --- a/tests/php/Forms/GridField/GridFieldPrintButtonTest.php +++ b/tests/php/Forms/GridField/GridFieldPrintButtonTest.php @@ -32,6 +32,19 @@ class GridFieldPrintButtonTest extends SapphireTest } public function testLimit() + { + $this->assertEquals(42, $this->getTestableRows()->count()); + } + + public function testCanViewIsRespected() + { + $orig = TestObject::$canView; + TestObject::$canView = false; + $this->assertEquals(0, $this->getTestableRows()->count()); + TestObject::$canView = $orig; + } + + private function getTestableRows() { $list = TestObject::get(); @@ -48,7 +61,6 @@ class GridFieldPrintButtonTest extends SapphireTest // Printed data should ignore pagination limit $printData = $button->generatePrintData($gridField); - $rows = $printData->ItemRows; - $this->assertEquals(42, $rows->count()); + return $printData->ItemRows; } } diff --git a/tests/php/Forms/GridField/GridFieldPrintButtonTest/TestObject.php b/tests/php/Forms/GridField/GridFieldPrintButtonTest/TestObject.php index 2e8f80cc9..dd5638d09 100644 --- a/tests/php/Forms/GridField/GridFieldPrintButtonTest/TestObject.php +++ b/tests/php/Forms/GridField/GridFieldPrintButtonTest/TestObject.php @@ -12,4 +12,11 @@ class TestObject extends DataObject implements TestOnly private static $db = [ 'Name' => 'Varchar' ]; + + public static bool $canView = true; + + public function canView($member = null) + { + return static::$canView; + } } From 1a5bb4cbece1721203977910b8ecd8b79c18dc77 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Tue, 14 Feb 2023 11:41:51 +1300 Subject: [PATCH 4/4] [CVE-2023-22729] Escaped double slash is absolute URL --- src/Control/Director.php | 5 +-- tests/php/Control/DirectorTest.php | 50 +++++++++++++++++++++++++----- 2 files changed, 45 insertions(+), 10 deletions(-) diff --git a/src/Control/Director.php b/src/Control/Director.php index 91023b7a7..ee7266bd4 100644 --- a/src/Control/Director.php +++ b/src/Control/Director.php @@ -822,10 +822,11 @@ class Director implements TemplateGlobalProvider return ( // Base check for existence of a host on a compliant URL parse_url($url ?? '', PHP_URL_HOST) - // Check for more than one leading slash without a protocol. + // Check for more than one leading slash (forward or backward) without a protocol. // While not a RFC compliant absolute URL, it is completed to a valid URL by some browsers, // and hence a potential security risk. Single leading slashes are not an issue though. - || preg_match('%^\s*/{2,}%', $url ?? '') + // Note: Need 4 backslashes to have a single non-escaped backslash for regex. + || preg_match('%^\s*(\\\\|/){2,}%', $url ?? '') || ( // If a colon is found, check if it's part of a valid scheme definition // (meaning its not preceded by a slash). diff --git a/tests/php/Control/DirectorTest.php b/tests/php/Control/DirectorTest.php index 05e24046b..b8be9ec46 100644 --- a/tests/php/Control/DirectorTest.php +++ b/tests/php/Control/DirectorTest.php @@ -232,6 +232,9 @@ class DirectorTest extends SapphireTest public function testIsAbsoluteUrl() { $this->assertTrue(Director::is_absolute_url('http://test.com/testpage')); + $this->assertTrue(Director::is_absolute_url('https:/\\test.com')); + $this->assertTrue(Director::is_absolute_url('https:\\/test.com')); + $this->assertTrue(Director::is_absolute_url('https:\\\\test.com')); $this->assertTrue(Director::is_absolute_url('ftp://test.com')); $this->assertFalse(Director::is_absolute_url('test.com/testpage')); $this->assertFalse(Director::is_absolute_url('/relative')); @@ -241,6 +244,11 @@ class DirectorTest extends SapphireTest $this->assertTrue(Director::is_absolute_url("https://test.com/?url=http://foo.com")); $this->assertTrue(Director::is_absolute_url("trickparseurl:http://test.com")); $this->assertTrue(Director::is_absolute_url('//test.com')); + $this->assertTrue(Director::is_absolute_url('\\/\\/test.com')); + $this->assertTrue(Director::is_absolute_url('\/\/test.com')); + $this->assertTrue(Director::is_absolute_url('/\\test.com')); + $this->assertTrue(Director::is_absolute_url('\\\\test.com')); + $this->assertFalse(Director::is_absolute_url('\\test.com')); $this->assertTrue(Director::is_absolute_url('/////test.com')); $this->assertTrue(Director::is_absolute_url(' ///test.com')); $this->assertTrue(Director::is_absolute_url('http:test.com')); @@ -258,8 +266,17 @@ class DirectorTest extends SapphireTest { $this->assertFalse(Director::is_relative_url('http://test.com')); $this->assertFalse(Director::is_relative_url('https://test.com')); + $this->assertFalse(Director::is_relative_url('https:/\\test.com')); + $this->assertFalse(Director::is_relative_url('https:\\/test.com')); + $this->assertFalse(Director::is_relative_url('https:\\\\test.com')); $this->assertFalse(Director::is_relative_url(' https://test.com/testpage ')); $this->assertTrue(Director::is_relative_url('test.com/testpage')); + $this->assertFalse(Director::is_relative_url('//test.com')); + $this->assertFalse(Director::is_relative_url('\\/\\/test.com')); + $this->assertFalse(Director::is_relative_url('\/\/test.com')); + $this->assertFalse(Director::is_relative_url('/\\test.com')); + $this->assertFalse(Director::is_relative_url('\\\\test.com')); + $this->assertTrue(Director::is_relative_url('\\test.com')); $this->assertFalse(Director::is_relative_url('ftp://test.com')); $this->assertTrue(Director::is_relative_url('/relative')); $this->assertTrue(Director::is_relative_url('relative')); @@ -401,17 +418,34 @@ class DirectorTest extends SapphireTest ); } - /** - * Mostly tested by {@link testIsRelativeUrl()}, - * just adding the host name matching aspect here. - */ public function testIsSiteUrl() { - $this->assertFalse(Director::is_site_url("http://test.com")); + $this->assertFalse(Director::is_site_url('http://test.com')); + $this->assertTrue(Director::is_site_url('/relative-path')); + $this->assertTrue(Director::is_site_url('relative-path')); $this->assertTrue(Director::is_site_url(Director::absoluteBaseURL())); - $this->assertFalse(Director::is_site_url("http://test.com?url=" . Director::absoluteBaseURL())); - $this->assertFalse(Director::is_site_url("http://test.com?url=" . urlencode(Director::absoluteBaseURL() ?? ''))); - $this->assertFalse(Director::is_site_url("//test.com?url=" . Director::absoluteBaseURL())); + $this->assertFalse(Director::is_site_url('http://test.com?url=' . Director::absoluteBaseURL())); + $this->assertFalse(Director::is_site_url('http://test.com?url=' . urlencode(Director::absoluteBaseURL() ?? ''))); + $this->assertFalse(Director::is_site_url('http:\\\\test.com')); + $this->assertFalse(Director::is_site_url('http:\\\\test.com?url=' . Director::absoluteBaseURL())); + $this->assertFalse(Director::is_site_url('http:\\\\test.com?url=' . urlencode(Director::absoluteBaseURL() ?? ''))); + $this->assertFalse(Director::is_site_url('http:\\/test.com')); + $this->assertFalse(Director::is_site_url('http:\\/test.com?url=' . Director::absoluteBaseURL())); + $this->assertFalse(Director::is_site_url('http:\\/test.com?url=' . urlencode(Director::absoluteBaseURL() ?? ''))); + $this->assertFalse(Director::is_site_url('//test.com')); + $this->assertFalse(Director::is_site_url('//test.com?url=' . Director::absoluteBaseURL())); + $this->assertFalse(Director::is_site_url('\\/\\/test.com')); + $this->assertFalse(Director::is_site_url('\\/\\/test.com?url=' . Director::absoluteBaseURL())); + $this->assertFalse(Director::is_site_url('\/\/test.com')); + $this->assertFalse(Director::is_site_url('\/\/test.com?url=' . Director::absoluteBaseURL())); + $this->assertFalse(Director::is_site_url('\\/test.com')); + $this->assertFalse(Director::is_site_url('\\/test.com?url=' . Director::absoluteBaseURL())); + $this->assertFalse(Director::is_site_url('/\\test.com')); + $this->assertFalse(Director::is_site_url('/\\test.com?url=' . Director::absoluteBaseURL())); + $this->assertFalse(Director::is_site_url('\\\\test.com')); + $this->assertFalse(Director::is_site_url('\\\\test.com?url=' . Director::absoluteBaseURL())); + $this->assertTrue(Director::is_site_url('\\test.com')); + $this->assertTrue(Director::is_site_url('\\test.com?url=' . Director::absoluteBaseURL())); $this->assertFalse(Director::is_site_url('http://google.com\@test.com')); $this->assertFalse(Director::is_site_url('http://google.com/@test.com')); $this->assertFalse(Director::is_site_url('http://google.com:pass\@test.com'));