Merge pull request #101 from creative-commoners/pulls/2/stop-using-depr

API Stop using deprecated API

src/DataFormatter/JSONDataFormatter.php

@@ -97,7 +97,7 @@ class JSONDataFormatter extends DataFormatter
 
         if ($this->relationDepth > 0) {
             foreach ($obj->hasOne() as $relName => $relClass) {
-                if (!singleton($relClass)->stat('api_access')) {
+                if (!$relClass::config()->get('api_access')) {
                     continue;
                 }
 
@@ -132,7 +132,7 @@ class JSONDataFormatter extends DataFormatter
                 $parts = explode('.', $relClass ?? '');
                 $relClass = array_shift($parts);
 
-                if (!singleton($relClass)->stat('api_access')) {
+                if (!$relClass::config()->get('api_access')) {
                     continue;
                 }
 

src/DataFormatter/XMLDataFormatter.php

@@ -2,6 +2,7 @@
 
 namespace SilverStripe\RestfulServer\DataFormatter;
 
+use SimpleXMLElement;
 use SilverStripe\Control\Controller;
 use SilverStripe\Core\Convert;
 use SilverStripe\Dev\Debug;
@@ -11,6 +12,7 @@ use SilverStripe\ORM\DataObjectInterface;
 use SilverStripe\Control\Director;
 use SilverStripe\ORM\SS_List;
 use SilverStripe\RestfulServer\RestfulServer;
+use InvalidArgumentException;
 
 /**
  * Formats a DataObject's member fields into an XML string
@@ -145,7 +147,7 @@ class XMLDataFormatter extends DataFormatter
 
         if ($this->relationDepth > 0) {
             foreach ($obj->hasOne() as $relName => $relClass) {
-                if (!singleton($relClass)->stat('api_access')) {
+                if (!singleton($relClass)::config()->get('api_access')) {
                     continue;
                 }
 
@@ -171,7 +173,7 @@ class XMLDataFormatter extends DataFormatter
                 //remove dot notation from relation names
                 $parts = explode('.', $relClass ?? '');
                 $relClass = array_shift($parts);
-                if (!singleton($relClass)->stat('api_access')) {
+                if (!singleton($relClass)::config()->get('api_access')) {
                     continue;
                 }
                 // backslashes in FQCNs kills both URIs and XML
@@ -202,7 +204,7 @@ class XMLDataFormatter extends DataFormatter
                 //remove dot notation from relation names
                 $parts = explode('.', $relClass ?? '');
                 $relClass = array_shift($parts);
-                if (!singleton($relClass)->stat('api_access')) {
+                if (!singleton($relClass)::config()->get('api_access')) {
                     continue;
                 }
                 // backslashes in FQCNs kills both URIs and XML
@@ -261,6 +263,79 @@ class XMLDataFormatter extends DataFormatter
      */
     public function convertStringToArray($strData)
     {
-        return Convert::xml2array($strData);
+        return self::xml2array($strData);
+    }
+
+    /**
+     * This was copied from Convert::xml2array() which is deprecated/removed
+     *
+     * Converts an XML string to a PHP array
+     * See http://phpsecurity.readthedocs.org/en/latest/Injection-Attacks.html#xml-external-entity-injection
+     *
+     * @uses recursiveXMLToArray()
+     * @param string $val
+     * @param boolean $disableDoctypes Disables the use of DOCTYPE, and will trigger an error if encountered.
+     * false by default.
+     * @param boolean $disableExternals Does nothing because xml entities are removed
+     * @return array
+     * @throws Exception
+     */
+    private static function xml2array($val, $disableDoctypes = false, $disableExternals = false)
+    {
+        // Check doctype
+        if ($disableDoctypes && strpos($val ?? '', '<!DOCTYPE') !== false) {
+            throw new InvalidArgumentException('XML Doctype parsing disabled');
+        }
+
+        // CVE-2021-41559 Ensure entities are removed due to their inherent security risk via
+        // XXE attacks and quadratic blowup attacks, and also lack of consistent support
+        $val = preg_replace('/(?s)<!ENTITY.*?>/', '', $val ?? '');
+
+        // If there's still an <!ENTITY> present, then it would be the result of a maliciously
+        // crafted XML document e.g. <!ENTITY><!<!ENTITY>ENTITY ext SYSTEM "http://evil.com">
+        if (strpos($val ?? '', '<!ENTITY') !== false) {
+            throw new InvalidArgumentException('Malicious XML entity detected');
+        }
+
+        // This will throw an exception if the XML contains references to any internal entities
+        // that were defined in an <!ENTITY /> before it was removed
+        $xml = new SimpleXMLElement($val ?? '');
+        return self::recursiveXMLToArray($xml);
+    }
+
+    /**
+     * @param SimpleXMLElement $xml
+     *
+     * @return mixed
+     */
+    private static function recursiveXMLToArray($xml)
+    {
+        $x = null;
+        if ($xml instanceof SimpleXMLElement) {
+            $attributes = $xml->attributes();
+            foreach ($attributes as $k => $v) {
+                if ($v) {
+                    $a[$k] = (string) $v;
+                }
+            }
+            $x = $xml;
+            $xml = get_object_vars($xml);
+        }
+        if (is_array($xml)) {
+            if (count($xml ?? []) === 0) {
+                return (string)$x;
+            } // for CDATA
+            $r = [];
+            foreach ($xml as $key => $value) {
+                $r[$key] = self::recursiveXMLToArray($value);
+            }
+            // Attributes
+            if (isset($a)) {
+                $r['@'] = $a;
+            }
+            return $r;
+        }
+
+        return (string) $xml;
     }
 }

src/RestfulServer.php

@@ -289,6 +289,10 @@ class RestfulServer extends Controller
             'limit' => (int) $this->request->getVar('limit'),
         ];
 
+        if ($limit['limit'] === 0) {
+            $limit = null;
+        }
+
         $params = $this->request->getVars();
 
         $responseFormatter = $this->getResponseDataFormatter($className);

tests/unit/RestfulServerTest.php

@@ -20,6 +20,7 @@ use SilverStripe\Dev\SapphireTest;
 use SilverStripe\RestfulServer\DataFormatter\JSONDataFormatter;
 use Page;
 use SilverStripe\Core\Config\Config;
+use SilverStripe\RestfulServer\DataFormatter\XMLDataFormatter;
 
 /**
  *
@@ -126,7 +127,8 @@ class RestfulServerTest extends SapphireTest
         $urlSafeClassname = $this->urlSafeClassname(RestfulServerTestAuthorRating::class);
         $url = "{$this->baseURI}/api/v1/$urlSafeClassname/" . $rating1->ID;
         $response = Director::test($url, null, null, 'GET');
-        $responseArr = Convert::xml2array($response->getBody());
+        $formatter = new XMLDataFormatter();
+        $responseArr = $formatter->convertStringToArray($response->getBody());
         $this->assertEquals(3, $responseArr['rate']);
     }
 
@@ -165,7 +167,8 @@ class RestfulServerTest extends SapphireTest
         $response = Director::test($url, null, null, 'GET');
         $this->assertEquals(200, $response->getStatusCode());
 
-        $responseArr = Convert::xml2array($response->getBody());
+        $formatter = new XMLDataFormatter();
+        $responseArr = $formatter->convertStringToArray($response->getBody());
         $xmlTagSafeClassName = $this->urlSafeClassname(RestfulServerTestAuthorRating::class);
         $ratingsArr = $responseArr['Ratings'][$xmlTagSafeClassName];
         $this->assertEquals(2, count($ratingsArr ?? []));
@@ -192,7 +195,8 @@ class RestfulServerTest extends SapphireTest
         $response = Director::test($url, null, null, 'GET');
         $this->assertEquals(200, $response->getStatusCode());
 
-        $responseArr = Convert::xml2array($response->getBody());
+        $formatter = new XMLDataFormatter();
+        $responseArr = $formatter->convertStringToArray($response->getBody());
         $xmlTagSafeClassName = $this->urlSafeClassname(RestfulServerTestAuthorRating::class);
 
         $this->assertTrue(array_key_exists('Ratings', $responseArr ?? []));
@@ -210,7 +214,9 @@ class RestfulServerTest extends SapphireTest
         $url = "{$this->baseURI}/api/v1/$urlSafeClassname/" . $author4->ID . '/RelatedAuthors';
         $response = Director::test($url, null, null, 'GET');
         $this->assertEquals(200, $response->getStatusCode());
-        $arr = Convert::xml2array($response->getBody());
+        
+        $formatter = new XMLDataFormatter();
+        $arr = $formatter->convertStringToArray($response->getBody());
         $xmlSafeClassName = $this->urlSafeClassname(RestfulServerTestAuthor::class);
         $authorsArr = $arr[$xmlSafeClassName];
 
@@ -239,7 +245,8 @@ class RestfulServerTest extends SapphireTest
         $response = Director::test($url, null, null, 'PUT', $body, $headers);
         $this->assertEquals(202, $response->getStatusCode()); // Accepted
         // Assumption: XML is default output
-        $responseArr = Convert::xml2array($response->getBody());
+        $formatter = new XMLDataFormatter();
+        $responseArr = $formatter->convertStringToArray($response->getBody());
         $this->assertEquals($comment1->ID, $responseArr['ID']);
         $this->assertEquals('updated', $responseArr['Comment']);
         $this->assertEquals('Updated Comment', $responseArr['Name']);
@@ -264,7 +271,8 @@ class RestfulServerTest extends SapphireTest
         $response = Director::test($url, null, null, 'POST', $body, $headers);
         $this->assertEquals(201, $response->getStatusCode()); // Created
         // Assumption: XML is default output
-        $responseArr = Convert::xml2array($response->getBody());
+        $formatter = new XMLDataFormatter();
+        $responseArr = $formatter->convertStringToArray($response->getBody());
         $this->assertTrue($responseArr['ID'] > 0);
         $this->assertNotEquals($responseArr['ID'], $comment1->ID);
         $this->assertEquals('created', $responseArr['Comment']);
@@ -339,7 +347,8 @@ class RestfulServerTest extends SapphireTest
         $body = '<RestfulServerTestComment><Comment>updated</Comment></RestfulServerTestComment>';
         $response = Director::test($url, null, null, 'PUT', $body, array('Content-Type'=>'text/xml'));
         $this->assertEquals(202, $response->getStatusCode()); // Accepted
-        $obj = Convert::xml2array($response->getBody());
+        $formatter = new XMLDataFormatter();
+        $obj = $formatter->convertStringToArray($response->getBody());
         $this->assertEquals($comment1->ID, $obj['ID']);
         $this->assertEquals('updated', $obj['Comment']);
 
@@ -350,7 +359,8 @@ class RestfulServerTest extends SapphireTest
         $response = Director::test($url, null, null, 'PUT', $body);
         $this->assertEquals(202, $response->getStatusCode()); // Accepted
         $this->assertEquals($url, $response->getHeader('Location'));
-        $obj = Convert::xml2array($response->getBody());
+        $formatter = new XMLDataFormatter();
+        $obj = $formatter->convertStringToArray($response->getBody());
         $this->assertEquals($comment1->ID, $obj['ID']);
         $this->assertEquals('updated', $obj['Comment']);
 
@@ -553,7 +563,8 @@ class RestfulServerTest extends SapphireTest
         );
         $response = Director::test($url, $data, null, 'PUT');
         // Assumption: XML is default output
-        $responseArr = Convert::xml2array($response->getBody());
+        $formatter = new XMLDataFormatter();
+        $responseArr = $formatter->convertStringToArray($response->getBody());
         $this->assertEquals(42, $responseArr['Rating']);
         $this->assertNotEquals('haxx0red', $responseArr['WriteProtectedField']);
     }
@@ -570,7 +581,8 @@ class RestfulServerTest extends SapphireTest
         );
         $response = Director::test($url, $data, null, 'PUT');
         // Assumption: XML is default output
-        $responseArr = Convert::xml2array($response->getBody());
+        $formatter = new XMLDataFormatter();
+        $responseArr = $formatter->convertStringToArray($response->getBody());
         // should output with aliased name
         $this->assertEquals(42, $responseArr['rate']);
     }
@@ -631,7 +643,8 @@ class RestfulServerTest extends SapphireTest
         $url = "{$this->baseURI}/api/v1/{$urlSafeClassname}?sort=FirstName&dir=DESC&fields=FirstName";
 
         $response = Director::test($url);
-        $results = Convert::xml2array($response->getBody());
+        $formatter = new XMLDataFormatter();
+        $results = $formatter->convertStringToArray($response->getBody());
 
         $this->assertSame('Author 4', $results[$urlSafeClassname][0]['FirstName']);
         $this->assertSame('Author 3', $results[$urlSafeClassname][1]['FirstName']);
@@ -645,7 +658,8 @@ class RestfulServerTest extends SapphireTest
         $url = "{$this->baseURI}/api/v1/{$urlSafeClassname}?sort=FirstName&dir=ASC&fields=FirstName";
 
         $response = Director::test($url);
-        $results = Convert::xml2array($response->getBody());
+        $formatter = new XMLDataFormatter();
+        $results = $formatter->convertStringToArray($response->getBody());
 
         $this->assertSame('Author 1', $results[$urlSafeClassname][0]['FirstName']);
         $this->assertSame('Author 2', $results[$urlSafeClassname][1]['FirstName']);
@@ -660,7 +674,8 @@ class RestfulServerTest extends SapphireTest
 
         $response = Director::test($url);
 
-        $results = Convert::xml2array($response->getBody());
+        $formatter = new XMLDataFormatter();
+        $results = $formatter->convertStringToArray($response->getBody());
 
         $this->assertSame('Author 1', $results[$urlSafeClassname][0]['FirstName']);
         $this->assertSame('Author 2', $results[$urlSafeClassname][1]['FirstName']);
@@ -678,7 +693,8 @@ class RestfulServerTest extends SapphireTest
         ];
         $response = Director::test($url, $data, null, 'POST');
         // Assumption: XML is default output
-        $responseArr = Convert::xml2array($response->getBody());
+        $formatter = new XMLDataFormatter();
+        $responseArr = $formatter->convertStringToArray($response->getBody());
         $this->assertEquals(42, $responseArr['Rating']);
         $this->assertNotEquals('haxx0red', $responseArr['WriteProtectedField']);
     }
@@ -692,7 +708,8 @@ class RestfulServerTest extends SapphireTest
             'rate' => '42',
         ];
         $response = Director::test($url, $data, null, 'POST');
-        $responseArr = Convert::xml2array($response->getBody());
+        $formatter = new XMLDataFormatter();
+        $responseArr = $formatter->convertStringToArray($response->getBody());
         $this->assertEquals(42, $responseArr['rate']);
     }
 
@@ -722,7 +739,8 @@ class RestfulServerTest extends SapphireTest
         $this->assertEquals(200, $response->getStatusCode());
         $this->assertStringContainsString('Unspeakable', $response->getBody());
         // Assumption: default formatter is XML
-        $responseArray = Convert::xml2array($response->getBody());
+        $formatter = new XMLDataFormatter();
+        $responseArray = $formatter->convertStringToArray($response->getBody());
         $this->assertEquals(1, $responseArray['@attributes']['totalSize']);
         unset($_SERVER['PHP_AUTH_USER']);
         unset($_SERVER['PHP_AUTH_PW']);
@@ -737,7 +755,8 @@ class RestfulServerTest extends SapphireTest
         ];
         $response = Director::test($url, $data, null, 'POST');
         // Assumption: XML is default output
-        $responseArr = Convert::xml2array($response->getBody());
+        $formatter = new XMLDataFormatter();
+        $responseArr = $formatter->convertStringToArray($response->getBody());
         $this->assertEquals('SilverStripe\\ORM\\ValidationException', $responseArr['type']);
     }
 
@@ -750,7 +769,8 @@ class RestfulServerTest extends SapphireTest
         ];
         $response = Director::test($url, $data, null, 'POST');
         // Assumption: XML is default output
-        $responseArr = Convert::xml2array($response->getBody());
+        $formatter = new XMLDataFormatter();
+        $responseArr = $formatter->convertStringToArray($response->getBody());
         $this->assertEquals(\Exception::class, $responseArr['type']);
     }
 

tests/unit/XMLDataFormatterTest.php

@@ -0,0 +1,77 @@
+<?php
+
+namespace SilverStripe\RestfulServer\Tests;
+
+use SilverStripe\Dev\SapphireTest;
+use SilverStripe\RestfulServer\DataFormatter\XMLDataFormatter;
+use Exception;
+
+class XMLDataFormatterTest extends SapphireTest
+{
+    /**
+     * Tests {@link Convert::xml2array()}
+     */
+    public function testConvertStringToArray()
+    {
+        $inputXML = <<<XML
+<?xml version="1.0"?>
+<!DOCTYPE results [
+  <!ENTITY long "SOME_SUPER_LONG_STRING">
+]>
+<results>
+    <result>My para</result>
+    <result>Ampersand &amp; is retained and not double encoded</result>
+</results>
+XML
+        ;
+        $expected = [
+            'result' => [
+                'My para',
+                'Ampersand & is retained and not double encoded'
+            ]
+        ];
+        $formatter = new XMLDataFormatter();
+        $actual = $formatter->convertStringToArray($inputXML);
+        $this->assertEquals($expected, $actual);
+    }
+
+    /**
+     * Tests {@link Convert::xml2array()} if an exception the contains a reference to a removed <!ENTITY />
+     */
+    public function testConvertStringToArrayEntityException()
+    {
+        $inputXML = <<<XML
+        <?xml version="1.0"?>
+        <!DOCTYPE results [
+            <!ENTITY long "SOME_SUPER_LONG_STRING">
+        ]>
+        <results>
+            <result>Now include &long; lots of times to expand the in-memory size of this XML structure</result>
+            <result>&long;&long;&long;</result>
+        </results>
+        XML;
+        $this->expectException(Exception::class);
+        $this->expectExceptionMessage('String could not be parsed as XML');
+        $formatter = new XMLDataFormatter();
+        $formatter->convertStringToArray($inputXML);
+    }
+
+    /**
+     * Tests {@link Convert::xml2array()} if an exception the contains a reference to a multiple removed <!ENTITY />
+     */
+    public function testConvertStringToArrayMultipleEntitiesException()
+    {
+        $inputXML = <<<XML
+        <?xml version="1.0"?>
+        <!DOCTYPE results [<!ENTITY long "SOME_SUPER_LONG_STRING"><!ENTITY short "SHORT_STRING">]>
+        <results>
+            <result>Now include &long; and &short; lots of times</result>
+            <result>&long;&long;&long;&short;&short;&short;</result>
+        </results>
+        XML;
+        $this->expectException(Exception::class);
+        $this->expectExceptionMessage('String could not be parsed as XML');
+        $formatter = new XMLDataFormatter();
+        $formatter->convertStringToArray($inputXML);
+    }
+}