Merge branch '4.8' into 4

_config/dev.yml

@@ -17,3 +17,6 @@ SilverStripe\Dev\DevelopmentAdmin:
       controller: Silverstripe\Dev\DevConfigController
       links:
         config: 'View the current config, useful for debugging'
+
+SilverStripe\Dev\CSSContentParser:
+  disable_xml_external_entities: true

docs/en/02_Developer_Guides/19_GraphQL/04_security_and_best_practices/01_authentication.md

@@ -43,11 +43,13 @@ the [CSRF Middleware](csrf_protection) enabled. (It is by default).**
 ### HTTP basic authentication
 
 Silverstripe CMS has built-in support for [HTTP basic authentication](https://en.wikipedia.org/wiki/Basic_access_authentication).
-There is a `BasicAuthAuthenticator` which is configured for GraphQL by default, but
+
+There is a `BasicAuthAuthenticator` which can be configured for GraphQL that
 will only activate when required. It is kept separate from the SilverStripe CMS
 authenticator because GraphQL needs to use the successfully authenticated member
 for CMS permission filtering, whereas the global `BasicAuth` does not log the
-member in or use it for model security.
+member in or use it for model security.  Note that basic auth will bypass MFA authentication
+so if MFA is enabled it is not recommended that you also use basic auth for graphql.
 
 When using HTTP basic authentication, you can feel free to remove the [CSRF Middleware](csrf_protection),
 as it just adds unnecessary overhead to the request.
@@ -95,7 +97,7 @@ is applicable in the current request context (provided as an argument).
 Here's an example for implementing HTTP basic authentication:
 
 [notice]
-Note that basic auth is enabled by default.
+Note that basic authentication for graphql will bypass Multi-Factor Authentication (MFA) if that's enabled. Using basic authentication for graphql is considered insecure if you are using MFA .
 [/notice]
 
 ```yaml

src/Dev/CSSContentParser.php

@@ -2,6 +2,7 @@
 
 namespace SilverStripe\Dev;
 
+use SilverStripe\Core\Config\Configurable;
 use SilverStripe\Core\Injector\Injectable;
 use SimpleXMLElement;
 use tidy;
@@ -25,6 +26,7 @@
 class CSSContentParser
 {
     use Injectable;
+    use Configurable;
 
     protected $simpleXML = null;
 
@@ -56,6 +58,13 @@ public function __construct($content)
             $tidy = $content;
         }
 
+        // Prevent loading of external entities to prevent XXE attacks
+        // Note: as of libxml 2.9.0 entity substitution is disabled by default so this won't be required
+        if ($this->config()->get('disable_xml_external_entities')) {
+            libxml_set_external_entity_loader(function () {
+                return null;
+            });
+        }
         $this->simpleXML = @simplexml_load_string($tidy, 'SimpleXMLElement', LIBXML_NOWARNING);
         if (!$this->simpleXML) {
             throw new Exception('CSSContentParser::__construct(): Could not parse content.'

src/Forms/FileField.php

@@ -177,13 +177,46 @@ public function Value()
 
     public function validate($validator)
     {
-        if (!isset($_FILES[$this->name])) {
+        // FileField with the name multi_file_syntax[] or multi_file_syntax[key] will have the brackets trimmed in
+        // $_FILES super-global so it will be stored as $_FILES['mutli_file_syntax']
+        // multi-file uploads, which are not officially supported by Silverstripe, though may be
+        // implemented in custom code, so we should still ensure they are at least validated
+        $isMultiFileUpload = strpos($this->name, '[') !== false;
+        $fieldName = preg_replace('#\[(.*?)\]$#', '', $this->name);
+
+        if (!isset($_FILES[$fieldName])) {
             return true;
         }
 
-        $tmpFile = $_FILES[$this->name];
+        if ($isMultiFileUpload) {
+            $isValid = true;
+            foreach (array_keys($_FILES[$fieldName]['name']) as $key) {
+                $fileData = [
+                    'name' => $_FILES[$fieldName]['name'][$key],
+                    'type' => $_FILES[$fieldName]['type'][$key],
+                    'tmp_name' => $_FILES[$fieldName]['tmp_name'][$key],
+                    'error' => $_FILES[$fieldName]['error'][$key],
+                    'size' => $_FILES[$fieldName]['size'][$key],
+                ];
+                if (!$this->validateFileData($validator, $fileData)) {
+                    $isValid = false;
+                }
+            }
+            return $isValid;
+        }
+
+        // regular single-file upload
+        return $this->validateFileData($validator, $_FILES[$this->name]);
+    }
 
-        $valid = $this->upload->validate($tmpFile);
+    /**
+     * @param Validator $validator
+     * @param array $fileData
+     * @return bool
+     */
+    private function validateFileData($validator, array $fileData): bool
+    {
+        $valid = $this->upload->validate($fileData);
         if (!$valid) {
             $errors = $this->upload->getErrors();
             if ($errors) {
@@ -193,7 +226,6 @@ public function validate($validator)
             }
             return false;
         }
-
         return true;
     }
 

src/ORM/FieldType/DBText.php

@@ -80,7 +80,7 @@ public function LimitSentences($maxSentences = 2)
         }
 
         // Do a word-search
-        $words = preg_split('/\s+/u', $value);
+        $words = preg_split('/\s+/u', $value) ?: [];
         $sentences = 0;
         foreach ($words as $i => $word) {
             if (preg_match('/(!|\?|\.)$/', $word) && !preg_match('/(Dr|Mr|Mrs|Ms|Miss|Sr|Jr|No)\.$/i', $word)) {
@@ -133,8 +133,8 @@ public function Summary($maxWords = 50, $add = false)
         // Split on sentences (don't remove period)
         $sentences = array_filter(array_map(function ($str) {
             return trim($str);
-        }, preg_split('@(?<=\.)@', $value)));
-        $wordCount = count(preg_split('#\s+#u', $sentences[0]));
+        }, preg_split('@(?<=\.)@', $value) ?: []));
+        $wordCount = count(preg_split('#\s+#u', $sentences[0]) ?: []);
 
         // if the first sentence is too long, show only the first $maxWords words
         if ($wordCount > $maxWords) {
@@ -149,7 +149,7 @@ public function Summary($maxWords = 50, $add = false)
 
             // If more sentences to process, count number of words
             if ($sentences) {
-                $wordCount += count(preg_split('#\s+#u', $sentences[0]));
+                $wordCount += count(preg_split('#\s+#u', $sentences[0]) ?: []);
             }
         } while ($wordCount < $maxWords && $sentences && trim($sentences[0]));
 
@@ -169,7 +169,7 @@ public function FirstParagraph()
         }
 
         // Split paragraphs and return first
-        $paragraphs = preg_split('#\n{2,}#', $value);
+        $paragraphs = preg_split('#\n{2,}#', $value) ?: [];
         return reset($paragraphs);
     }
 

tests/php/Dev/CSSContentParserTest.php

@@ -50,4 +50,14 @@ public function testGetBySelector()
         $result = $parser->getBySelector('#A .other');
         $this->assertEquals("result", $result[0] . '');
     }
+
+    public function testXmlEntitiesDisabled()
+    {
+        // CSSContentParser uses simplexml to parse html
+        // Ensure XML entities are not substituted in to prevent XXE attacks
+        $xml = '<!DOCTYPE html [<!ENTITY myentity "World">]><html><div>Hello &myentity;</div></html>';
+        $parser = new CSSContentParser($xml);
+        $div = $parser->getBySelector('div')[0]->asXML();
+        $this->assertEquals('<div>Hello &amp;myentity;</div>', $div);
+    }
 }

tests/php/Forms/FileFieldTest.php

@@ -3,6 +3,7 @@
 namespace SilverStripe\Forms\Tests;
 
 use ReflectionMethod;
+use SilverStripe\Assets\Upload_Validator;
 use SilverStripe\Dev\FunctionalTest;
 use SilverStripe\Control\Controller;
 use SilverStripe\Forms\FileField;
@@ -39,6 +40,67 @@ public function testUploadRequiredFile()
         $this->assertTrue($form->validationResult()->isValid());
     }
 
+    /**
+     * Test that FileField::validate() is run on FileFields with both single and multi-file syntax
+     * By default FileField::validate() will return true early if the $_FILES super-global does not contain the
+     * corresponding FileField::name. This early return means the files was not fully run through FileField::validate()
+     * So for this test we create an invalid file upload on purpose and test that false was returned which means that
+     * the file was run through FileField::validate() function
+     */
+    public function testMultiFileSyntaxUploadIsValidated()
+    {
+        $names = [
+            'single_file_syntax',
+            'multi_file_syntax_a[]',
+            'multi_file_syntax_b[0]',
+            'multi_file_syntax_c[key]'
+        ];
+        foreach ($names as $name) {
+            $form = new Form(
+                Controller::curr(),
+                'Form',
+                new FieldList($fileField = new FileField($name, 'My desc')),
+                new FieldList()
+            );
+            $fileData = $this->createInvalidUploadedFileData($name, "FileFieldTest.txt");
+            // FileFields with multi_file_syntax[] files will appear in the $_FILES super-global
+            // with the [] brackets trimmed e.g. $_FILES[multi_file_syntax]
+            $_FILES = [preg_replace('#\[(.*?)\]#', '', $name) => $fileData];
+            $fileField->setValue($fileData);
+            $validator = $form->getValidator();
+            $isValid = $fileField->validate($validator);
+            $this->assertFalse($isValid, "$name was run through the validate() function");
+        }
+    }
+
+    protected function createInvalidUploadedFileData($name, $tmpFileName): array
+    {
+        $tmpFilePath = TEMP_PATH . DIRECTORY_SEPARATOR . $tmpFileName;
+
+        // multi_file_syntax
+        if (strpos($name, '[') !== false) {
+            $key = 0;
+            if (preg_match('#\[(.+?)\]#', $name, $m)) {
+                $key = $m[1];
+            }
+            return [
+                'name' => [$key => $tmpFileName],
+                'type' => [$key => 'text/plaintext'],
+                'size' => [$key => 0],
+                'tmp_name' => [$key => $tmpFilePath],
+                'error' => [$key => UPLOAD_ERR_NO_FILE],
+            ];
+        }
+        // single_file_syntax
+        return [
+            'name' => $tmpFileName,
+            'type' => 'text/plaintext',
+            'size' => 0,
+            'tmp_name' => $tmpFilePath,
+            'error' => UPLOAD_ERR_NO_FILE,
+        ];
+    }
+
     /**
      * @skipUpgrade
      */

tests/php/ORM/DBTextTest.php

@@ -114,7 +114,7 @@ public function providerLimitWordCount()
             ['The little brown fox jumped over the lazy cow.', 3, 'The little brown…'],
             [' This text has white space around the ends ', 3, 'This text has…'],
 
-            // Words less than the limt word count don't get truncated, ellipsis not added
+            // Words less than the limit word count don't get truncated, ellipsis not added
             ['Two words', 3, 'Two words'],  // Two words shouldn't have an ellipsis
             ['These three words', 3, 'These three words'], // Three words shouldn't have an ellipsis
             ['One', 3, 'One'],  // Neither should one word
@@ -216,7 +216,7 @@ public function testFirstSentence($originalValue, $expectedValue)
     }
 
     /**
-     * each test is in the format input, charactere limit, highlight, expected output
+     * each test is in the format input, character limit, highlight, expected output
      *
      * @return array
      */
@@ -268,18 +268,53 @@ public function providerContextSummary()
                 'both schön and können have umlauts',
                 21,
                 '',
-                // check non existant search term
+                // check non-existent search term
                 'both schön and können…',
             ]
+        ];
+    }
 
-
+    /**
+     * each test is in the format input, word limit, add ellipsis (false or string), expected output
+     *
+     * @return array
+     */
+    public function providerSummary()
+    {
+        return [
+            [
+                'This is some text. It is a test',
+                3,
+                false,
+                'This is some…',
+            ],
+            [
+                // check custom ellipsis
+                'This is a test text in a longer sentence and a custom ellipsis.',
+                8,
+                '...', // regular dots instead of the ellipsis character
+                'This is a test text in a longer...',
+            ],
+            [
+                'both schön and können have umlauts',
+                5,
+                false,
+                'both schön and können have…',
+            ],
+            [
+                // check invalid UTF8 handling — input is an invalid UTF sequence, output should be empty string
+                "\xf0\x28\x8c\xbc",
+                50,
+                false,
+                '',
+            ],
         ];
     }
 
     /**
      * @dataProvider providerContextSummary
      * @param string $originalValue Input
-     * @param int    $limit         Numer of characters
+     * @param int    $limit         Number of characters
      * @param string $keywords      Keywords to highlight
      * @param string $expectedValue Expected output (XML encoded safely)
      */
@@ -352,4 +387,18 @@ public function testDefaultEllipsis()
         $textObj = new DBText('Test');
         $this->assertEquals('…', $textObj->defaultEllipsis());
     }
+
+    /**
+     * @dataProvider providerSummary
+     * @param string $originalValue Input
+     * @param int    $words         Number of words
+     * @param mixed  $add           Ellipsis (false for default or string for custom text)
+     * @param string $expectedValue Expected output (XML encoded safely)
+     */
+    public function testSummary($originalValue, $words, $add, $expectedValue)
+    {
+        $text = DBField::create_field(DBText::class, $originalValue);
+        $result = $text->obj('Summary', [$words, $add])->forTemplate();
+        $this->assertEquals($expectedValue, $result);
+    }
 }