ENH Avoid potential data disclosure (#233)

silverstripe · Feb 20, 2024 · 87e8c17 · 87e8c17
1 parent c74c049
commit 87e8c17
Show file tree

Hide file tree

Showing 6 changed files with 35 additions and 38 deletions.
diff --git a/client/dist/js/bundle.js b/client/dist/js/bundle.js
diff --git a/client/src/components/LinkField/LinkField.js b/client/src/components/LinkField/LinkField.js
@@ -353,20 +353,20 @@ const LinkField = ({
       if (!linkData) {
         continue;
       }
-      const type = types.hasOwnProperty(data[linkID]?.typeKey) ? types[data[linkID]?.typeKey] : {};
+      const type = types.hasOwnProperty(linkData.typeKey) ? types[linkData.typeKey] : {};
       links.push(<LinkPickerTitle
         key={linkID}
         id={linkID}
-        title={data[linkID]?.Title}
-        description={data[linkID]?.description}
-        versionState={data[linkID]?.versionState}
+        title={linkData.title}
+        description={linkData.description}
+        versionState={linkData.versionState}
         typeTitle={type.title || ''}
         typeIcon={type.icon}
         onDelete={handleDelete}
         onClick={() => { setEditingID(linkID); }}
         onButtonKeyDownEdit={() => setIsKeyboardEditing(true)}
         onUnpublishedVersionedState={handleUnpublishedVersionedState}
-        canDelete={data[linkID]?.canDelete ? true : false}
+        canDelete={linkData.canDelete ? true : false}
         isMulti={isMulti}
         isFirst={i === 0}
         isLast={i === linkIDs.length - 1}

diff --git a/client/src/components/LinkField/tests/LinkField-test.js b/client/src/components/LinkField/tests/LinkField-test.js
@@ -71,11 +71,11 @@ test('LinkField returns list of links if they exist', async () => {
 
   await doResolve({ json: () => ({
     1: {
-      Title: 'Page title',
+      title: 'Page title',
       typeKey: 'sitetree',
     },
     2: {
-      Title: 'Email title',
+      title: 'Email title',
       typeKey: 'email',
     },
   }) });
@@ -119,13 +119,11 @@ test('LinkField tab order', async () => {
 
   await doResolve({ json: () => ({
     123: {
-      Title: 'First title',
-      Sort: 1,
+      title: 'First title',
       typeKey: 'mylink',
     },
     456: {
-      Title: 'Second title',
-      Sort: 2,
+      title: 'Second title',
       typeKey: 'mylink',
     },
   }) });
@@ -189,8 +187,7 @@ test('LinkField will render link-picker if ownerID is not 0 and isMulti and has
   />);
   await doResolve({ json: () => ({
     123: {
-      Title: 'First title',
-      Sort: 1,
+      title: 'First title',
       typeKey: 'mylink',
     },
   }) });

diff --git a/src/Controllers/LinkFieldController.php b/src/Controllers/LinkFieldController.php
@@ -118,11 +118,7 @@ private function getLinkData(Link $link): array
         if (!$link->canView()) {
             $this->jsonError(403);
         }
-        $data = $link->jsonSerialize();
-        $data['canDelete'] = $link->canDelete();
-        $data['description'] = $link->getDescription();
-        $data['versionState'] = $link->getVersionedState();
-        return $data;
+        return $link->getData();
     }
 
     /**

diff --git a/src/Models/Link.php b/src/Models/Link.php
@@ -201,33 +201,30 @@ public function onBeforeWrite(): void
     }
 
     /**
-     * Serialise the link data as a JSON string so it can be fetched from JavaScript.
+     * Get data for this link type which is required for displaying this link in the react component.
      */
-    public function jsonSerialize(): mixed
+    public function getData(): array
     {
         $typeKey = LinkTypeService::create()->keyByClassName(static::class);
 
         if (!$typeKey) {
             return [];
         }
 
-        // TODO: this could lead to data disclosure - we should only return the fields that are actually needed
-        $data = $this->toMap();
-        $data['typeKey'] = $typeKey;
-
-        unset($data['ClassName']);
-        unset($data['RecordClassName']);
-
-        $data['Title'] = $this->getTitle();
-
-        return $data;
+        return [
+            'title' => $this->getTitle(),
+            'description' => $this->getDescription(),
+            'canDelete' => $this->canDelete(),
+            'versionState' => $this->getVersionedState(),
+            'typeKey' => $typeKey,
+        ];
     }
 
     /**
-     * Return a rendered version of this form.
+     * Return a rendered version of this link.
      *
-     * This is returned when you access a form as $FormObject rather
-     * than <% with FormObject %>
+     * This is returned when you access a link as $LinkRelation or $Me rather
+     * than <% with LinkRelation %>
      *
      * @return DBHTMLText
      */

diff --git a/tests/php/Controllers/LinkFieldControllerTest.php b/tests/php/Controllers/LinkFieldControllerTest.php
@@ -232,11 +232,11 @@ public function testLinkFormPost(
         $ownerLinkID = $owner->LinkID;
         $id = $this->getID($idType);
         if ($dataType === 'valid') {
-            $data = $this->getFixtureLink()->jsonSerialize();
+            $data = $this->getFixtureLink()->toMap();
             $data['Phone'] = '9876543210';
             $data['ID'] = $id;
         } elseif ($dataType === 'invalid-id') {
-            $data = $this->getFixtureLink()->jsonSerialize();
+            $data = $this->getFixtureLink()->toMap();
             $data['Phone'] = '9876543210';
             $data['ID'] = $id + 99999;
         } else {
@@ -475,6 +475,7 @@ public function provideLinkFormReadOnly(): array
     public function testLinkData(
         string $idType,
         int $expectedCode,
+        array $expectedData = []
     ): void {
         $id = $this->getID($idType);
         if ($id === -1) {
@@ -487,8 +488,7 @@ public function testLinkData(
         $this->assertSame($expectedCode, $response->getStatusCode());
         if ($expectedCode === 200) {
             $data = json_decode($response->getBody(), true);
-            $this->assertSame($id, $data['ID']);
-            $this->assertSame('0123456789', $data['Phone']);
+            $this->assertEquals($expectedData, $data);
             $link = $this->getFixtureLink();
             $this->assertSame($link->getVersionedState(), $data['versionState']);
         }
@@ -500,6 +500,13 @@ public function provideLinkData(): array
             'Valid' => [
                 'idType' => 'existing',
                 'expectedCode' => 200,
+                'expectedData' => [
+                    'title' => 'My phone link 01',
+                    'description' => '0123456789',
+                    'canDelete' => true,
+                    'versionState' => 'draft',
+                    'typeKey' => 'testphone',
+                ],
             ],
             'Reject invalid ID' => [
                 'idType' => 'invalid',