refactor how sanitize is handled

lekoala · Feb 6, 2024 · f33e4be · f33e4be
1 parent 8fd4a21
commit f33e4be
Show file tree

Hide file tree

Showing 3 changed files with 53 additions and 20 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -4,13 +4,8 @@ on:
   push:
   pull_request:
   workflow_dispatch:
-  # Every Friday at 10:20am UTC
-  schedule:
-    - cron: '20 10 * * 5'
 
 jobs:
   ci:
     name: CI
-    # Only run cron on the silverstripe account
-    if: (github.event_name == 'schedule' && github.repository_owner == 'lekoala') || (github.event_name != 'schedule')
     uses: silverstripe/gha-ci/.github/workflows/ci.yml@v1
diff --git a/src/ExcelGridFieldExportButton.php b/src/ExcelGridFieldExportButton.php
@@ -289,13 +289,10 @@ public function generateExportFileData($gridField): Generator
             return;
         }
 
-        $sanitize_xls_chars = ExcelImportExport::config()->sanitize_xls_chars ?? "=";
-        $sanitize_xls_chars_len = strlen($sanitize_xls_chars);
-
         // Auto format using DBField methods based on column name
         $export_format = ExcelImportExport::config()->export_format;
 
-        $sanitize = $this->sanitizeXls && $sanitize_xls_chars && $this->exportType == "csv";
+        $sanitize = $this->isSanitizeEnabled();
 
         foreach ($list as $item) {
             // This can be really slow for large exports depending on how canView is implemented
@@ -356,17 +353,8 @@ public function generateExportFileData($gridField): Generator
 
                 // @link https://owasp.org/www-community/attacks/CSV_Injection
                 // [SS-2017-007] Sanitise XLS executable column values with a leading tab
-                if ($sanitize) {
-                    // If we have only one char we can make it simpler
-                    if ($sanitize_xls_chars_len === 1) {
-                        if ($value && $value[0] === $sanitize_xls_chars) {
-                            $value = "\t" . $value;
-                        }
-                    } else {
-                        if (preg_match('/^[' . $sanitize_xls_chars . '].*/', $value ?? '')) {
-                            $value = "\t" . $value;
-                        }
-                    }
+                if ($sanitize && $value && is_string($value)) {
+                    $value = self::sanitizeValue($value);
                 }
 
                 $dataRow[] = $value;
@@ -380,6 +368,46 @@ public function generateExportFileData($gridField): Generator
         }
     }
 
+    /**
+     * Sanitization is necessary for csv
+     * It can be turned off if needed
+     * If we have no chars to sanitize, it's not enabled
+     *
+     * @return boolean
+     */
+    public function isSanitizeEnabled(): bool
+    {
+        $sanitize_xls_chars = ExcelImportExport::config()->sanitize_xls_chars ?? "=";
+        $sanitize = $this->sanitizeXls && $sanitize_xls_chars && $this->exportType == "csv";
+        return $sanitize;
+    }
+
+    /**
+     * @link https://owasp.org/www-community/attacks/CSV_Injection
+     * [SS-2017-007] Sanitise XLS executable column values with a leading tab
+     */
+    public static function sanitizeValue(string $value = null): ?string
+    {
+        if (!$value) {
+            return $value;
+        }
+
+        $sanitize_xls_chars = ExcelImportExport::config()->sanitize_xls_chars ?? "=";
+        $sanitize_xls_chars_len = strlen($sanitize_xls_chars);
+
+        // If we have only one char we can make it simpler
+        if ($sanitize_xls_chars_len === 1) {
+            if ($value[0] === $sanitize_xls_chars) {
+                $value = "\t" . $value;
+            }
+        } else {
+            if (preg_match('/^[' . $sanitize_xls_chars . '].*/', $value)) {
+                $value = "\t" . $value;
+            }
+        }
+        return $value;
+    }
+
     /**
      * @return array<int|string,mixed>
      */

diff --git a/tests/ExcelImportExportTest.php b/tests/ExcelImportExportTest.php
@@ -2,6 +2,7 @@
 
 namespace LeKoala\ExcelImportExport\Test;
 
+use LeKoala\ExcelImportExport\ExcelGridFieldExportButton;
 use SilverStripe\Security\Group;
 use SilverStripe\Security\Member;
 use SilverStripe\Dev\SapphireTest;
@@ -82,4 +83,13 @@ public function testCanImportMembers(): void
         $this->assertEquals($count + 2, $newCount);
         $this->assertEquals($membersCount + 2, $newMembersCount, "Groups are not updated");
     }
+
+    public function testSanitize(): void
+    {
+        $dangerousInput = '=1+2";=1+2';
+
+        $actual = ExcelGridFieldExportButton::sanitizeValue($dangerousInput);
+        $expected = "\t" . $dangerousInput;
+        $this->assertEquals($expected, $actual);
+    }
 }