Fixed #19414 - mojibake fix

app/Http/Controllers/Api/ImportController.php

@@ -88,15 +88,35 @@ class ImportController extends Controller
                 if (! ini_get('auto_detect_line_endings')) {
                     ini_set('auto_detect_line_endings', '1');
                 }
-                if (function_exists('iconv')) {
+                if (function_exists('iconv') || function_exists('mb_convert_encoding')) {
                     $file_contents = $file->getContent(); // TODO - this *does* load the whole file in RAM, but we need that to be able to 'iconv' it?
                     $encoding = $detector->getEncoding($file_contents);
                     \Log::debug("Discovered encoding: $encoding in uploaded CSV");
+
+                    // Only fall back to mb_detect_encoding if the Onnov detector
+                    // gave us nothing useful. Overriding a correct Onnov result
+                    // (Windows-1251 for Cyrillic bytes, for example) with a
+                    // permissive mb_detect guess re-labels the file as one of
+                    // the CJK encodings early in the fallback list and produces
+                    // mojibake on iconv.
+                    if (! mb_check_encoding($file_contents, 'UTF-8')
+                        && (! $encoding || strcasecmp($encoding, 'UTF-8') === 0)) {
+                        $detected = mb_detect_encoding($file_contents, ['UTF-8', 'GBK', 'GB2312', 'GB18030', 'BIG5', 'SJIS', 'EUC-JP', 'EUC-KR', 'Windows-1252', 'Windows-1251', 'ISO-8859-1'], true);
+                        if ($detected && strcasecmp($detected, 'UTF-8') !== 0) {
+                            $encoding = $detected;
+                            \Log::debug("Fallback detected encoding: $encoding in uploaded CSV");
+                        }
+                    }
+
                     $reader = null;
-                    if (strcasecmp($encoding, 'UTF-8') != 0) {
+                    if ($encoding && strcasecmp($encoding, 'UTF-8') != 0) {
                         $transliterated = false;
                         try {
-                            $transliterated = iconv(strtoupper($encoding), 'UTF-8', $file_contents);
+                            if (function_exists('iconv')) {
+                                $transliterated = @iconv(strtoupper($encoding), 'UTF-8//IGNORE', $file_contents);
+                            } elseif (function_exists('mb_convert_encoding')) {
+                                $transliterated = mb_convert_encoding($file_contents, 'UTF-8', $encoding);
+                            }
                         } catch (\Exception $e) {
                             $transliterated = false; // blank out the partially-decoded string
 

app/Importer/Importer.php

@@ -140,7 +140,44 @@ abstract class Importer
         }
         // By default the importer passes a url to the file.
         // However, for testing we also support passing a string directly
+        $contents = null;
         if (is_file($file)) {
+            $contents = file_get_contents($file);
+        } else {
+            $contents = $file;
+        }
+
+        if ($contents !== false && ! mb_check_encoding($contents, 'UTF-8')) {
+            $encoding = null;
+            if (class_exists('\Onnov\DetectEncoding\EncodingDetector')) {
+                $detector = new \Onnov\DetectEncoding\EncodingDetector;
+                $encoding = $detector->getEncoding($contents);
+            }
+            // Only fall back to mb_detect_encoding if the Onnov detector gave
+            // us nothing useful. Overriding a confident Onnov result with a
+            // permissive mb_detect guess re-labels the file as one of the CJK
+            // encodings early in the fallback list and produces mojibake.
+            if (! $encoding || strcasecmp($encoding, 'UTF-8') === 0) {
+                $detected = mb_detect_encoding($contents, ['UTF-8', 'GBK', 'GB2312', 'GB18030', 'BIG5', 'SJIS', 'EUC-JP', 'EUC-KR', 'Windows-1252', 'Windows-1251', 'ISO-8859-1'], true);
+                if ($detected) {
+                    $encoding = $detected;
+                }
+            }
+            if ($encoding && strcasecmp($encoding, 'UTF-8') !== 0) {
+                if (function_exists('iconv')) {
+                    $converted = @iconv(strtoupper($encoding), 'UTF-8//IGNORE', $contents);
+                    if ($converted !== false) {
+                        $contents = $converted;
+                    }
+                } elseif (function_exists('mb_convert_encoding')) {
+                    $contents = mb_convert_encoding($contents, 'UTF-8', $encoding);
+                }
+            }
+        }
+
+        if ($contents !== null) {
+            $this->csv = Reader::createFromString($contents);
+        } elseif (is_file($file)) {
             $this->csv = Reader::createFromPath($file);
         } else {
             $this->csv = Reader::createFromString($file);
@@ -265,7 +302,12 @@ abstract class Importer
 
         // $this->log("Custom Key: ${key}");
         if (array_key_exists($key, $array)) {
-            $val = Encoding::toUTF8(trim($array[$key]));
+            $trimmed = trim($array[$key]);
+            if (mb_check_encoding($trimmed, 'UTF-8')) {
+                $val = $trimmed;
+            } else {
+                $val = Encoding::toUTF8($trimmed);
+            }
         }
 
         // $this->log("${key}: ${val}");

tests/Feature/Importing/Ui/ImportTest.php

@@ -60,10 +60,17 @@ class ImportTest extends TestCase
         // 0xC0 makes it 'not unicode', and 0xFF makes it 'likely WINDOWS-1251', and 0x98 at the end makes it 'not-valid-Windows-1251'
         $evil_content = $evil_maker([0xC0, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0x00, 0x01, 0x02, 0x03, 0x98]);
 
+        // As of PR #19418 (CSV encoding hardening), the iconv call uses
+        // //IGNORE so truly-invalid byte runs are silently dropped instead
+        // of throwing back to the controller as transliterate_failure. That
+        // is deliberate: real-world CJK / Windows-1252 CSVs typically have
+        // mostly-valid content with a stray byte or two, and //IGNORE lets
+        // those imports succeed. The tradeoff is that fully-corrupt input
+        // like this test's evil_content is now accepted through the store
+        // path (the resulting rows will just be nearly empty). Verify the
+        // request no longer bounces with 422.
         $this->actingAsForApi(User::factory()->superuser()->create());
-        $results = $this->post(route('api.imports.store'), ['files' => [UploadedFile::fake()->createWithContent('myname.csv', $evil_content)]])
-            ->assertStatus(422)
-            ->assertStatusMessageIs('error')
-            ->assertMessagesAre(trans('admin/hardware/message.import.transliterate_failure', ['encoding' => 'windows-1251']));
+        $this->post(route('api.imports.store'), ['files' => [UploadedFile::fake()->createWithContent('myname.csv', $evil_content)]])
+            ->assertOk();
     }
 }

tests/Unit/Importer/ImporterEncodingTest.php

@@ -0,0 +1,131 @@
+<?php
+
+namespace Tests\Unit\Importer;
+
+use App\Importer\AssetImporter;
+use ReflectionClass;
+use Tests\TestCase;
+
+/**
+ * Regression tests for the CSV encoding-detection layer in Importer::__construct
+ * and Importer::findCsvMatch. Motivated by PR #19418 (mojibake on non-UTF-8
+ * imports, especially CJK CSVs from Excel exports on Windows).
+ *
+ * The importer accepts either a path or a raw string. Both paths must land at
+ * a CSV reader whose rows contain UTF-8 bytes regardless of the source
+ * encoding, and must not double-encode content that was already UTF-8.
+ */
+class ImporterEncodingTest extends TestCase
+{
+    private function csvRowsFromString(string $csv): array
+    {
+        $importer = new AssetImporter($csv);
+
+        $reader = (new ReflectionClass($importer))
+            ->getProperty('csv')
+            ->getValue($importer);
+
+        return iterator_to_array($reader->getRecords());
+    }
+
+    public function test_utf8_content_is_preserved_without_double_encoding(): void
+    {
+        // Chinese chars in UTF-8: 你好 = E4 BD A0 E5 A5 BD
+        $utf8Csv = "name,note\n你好,greeting\n";
+
+        $rows = $this->csvRowsFromString($utf8Csv);
+
+        $this->assertCount(2, $rows);
+        $this->assertSame(['name', 'note'], $rows[0]);
+        $this->assertSame('你好', $rows[1][0]);
+    }
+
+    public function test_non_utf8_input_becomes_valid_utf8_output(): void
+    {
+        // The purpose of the conversion layer is "whatever the source
+        // encoding, downstream sees UTF-8". We deliberately don't assert
+        // the exact converted characters here: short-string auto-detection
+        // (Onnov + mb_detect_encoding fallback) is not deterministic across
+        // encodings, and coupling the test to specific detector output
+        // makes it brittle. What matters is the invariant: non-UTF-8 bytes
+        // in, valid UTF-8 out.
+        $windows1252Csv = "name,note\ncaf\xE9,drink\n";
+
+        // Pre-condition: raw bytes are not valid UTF-8.
+        $this->assertFalse(mb_check_encoding($windows1252Csv, 'UTF-8'));
+
+        $rows = $this->csvRowsFromString($windows1252Csv);
+
+        $this->assertCount(2, $rows);
+        foreach ($rows as $row) {
+            foreach ($row as $cell) {
+                $this->assertTrue(mb_check_encoding($cell, 'UTF-8'), "Cell not UTF-8: {$cell}");
+            }
+        }
+    }
+
+    public function test_gbk_input_becomes_valid_utf8_output(): void
+    {
+        // Same invariant test with GBK-encoded bytes. Pad the content so
+        // detectors have enough signal to lean toward CJK rather than
+        // shorter-string ambiguity. Repeat the "你好" pattern several times
+        // to give the detector a strong hint.
+        $gbkGreeting = str_repeat("\xC4\xE3\xBA\xC3", 8);
+        $gbkCsv = "name,note\n{$gbkGreeting},{$gbkGreeting}\n";
+
+        $this->assertFalse(mb_check_encoding($gbkCsv, 'UTF-8'));
+
+        $rows = $this->csvRowsFromString($gbkCsv);
+
+        $this->assertCount(2, $rows);
+        foreach ($rows as $row) {
+            foreach ($row as $cell) {
+                $this->assertTrue(mb_check_encoding($cell, 'UTF-8'), "Cell not UTF-8: {$cell}");
+            }
+        }
+    }
+
+    public function test_find_csv_match_leaves_valid_utf8_unchanged(): void
+    {
+        $importer = new AssetImporter("name\ntest\n");
+
+        // "café" as valid UTF-8: 63 61 66 C3 A9
+        $utf8Value = "caf\xC3\xA9";
+        $row = ['name' => $utf8Value];
+
+        $result = $importer->findCsvMatch($row, 'name');
+
+        $this->assertSame('café', $result);
+        $this->assertSame($utf8Value, $result);
+    }
+
+    public function test_find_csv_match_converts_non_utf8_value(): void
+    {
+        $importer = new AssetImporter("name\ntest\n");
+
+        // "café" as Windows-1252: 63 61 66 E9 (not valid UTF-8 as a bare byte)
+        $windows1252Value = "caf\xE9";
+        $row = ['name' => $windows1252Value];
+
+        $result = $importer->findCsvMatch($row, 'name');
+
+        $this->assertTrue(mb_check_encoding($result, 'UTF-8'));
+        $this->assertSame('café', $result);
+    }
+
+    public function test_ascii_only_pathstring_still_works(): void
+    {
+        // Regression: the existing test suite passes literal 'assets.csv' as
+        // the constructor arg (not a real path). is_file() is false, so the
+        // string is treated as CSV content. mb_check_encoding on plain ASCII
+        // returns true, so the conversion block is skipped and the reader
+        // parses the string as before.
+        $importer = new AssetImporter('assets.csv');
+
+        $reader = (new ReflectionClass($importer))
+            ->getProperty('csv')
+            ->getValue($importer);
+
+        $this->assertNotNull($reader);
+    }
+}