Unexpected Charset Possible for Currency Symbol

We do not recommend it, but users can call the Php function `setlocale` and that might affect the character used as currency symbol. A problem arises when the caller to setlocale does not specify a character set - in that case, Php will attempt to return its `localeconv()` values in a single-byte character set rather than UTF-8. This is particularly problematic for currency symbols. PhpSpreadsheet till now has accepted such a character, and that can lead to corrupt spreadsheets. It is changed to validate the currency symbol as UTF-8, and fall back to a different choice if not (e.g. EUR rather than 0x80, which is how the euro symbol is depicted in Win-1252).

An additional problem arises because Linux systems seem to return the alternate symbol with a trailing blank, but Windows systems do not. To allow callers to get a consistent result, a parameter is added to `getCurrencyCode` which will trim or not (default) the currency code.

src/PhpSpreadsheet/Shared/StringHelper.php

@@ -19,17 +19,17 @@ class StringHelper
     /**
      * Decimal separator.
      */
-    private static ?string $decimalSeparator;
+    private static ?string $decimalSeparator = null;
 
     /**
      * Thousands separator.
      */
-    private static ?string $thousandsSeparator;
+    private static ?string $thousandsSeparator = null;
 
     /**
      * Currency code.
      */
-    private static ?string $currencyCode;
+    private static ?string $currencyCode = null;
 
     /**
      * Is iconv extension avalable?
@@ -511,6 +511,23 @@ class StringHelper
         return implode('', $characters);
     }
 
+    private static function useAlt(string $altValue, string $default, bool $trimAlt): string
+    {
+        return ($trimAlt ? trim($altValue) : $altValue) ?: $default;
+    }
+
+    private static function getLocaleValue(string $key, string $altKey, string $default, bool $trimAlt = false): string
+    {
+        $localeconv = localeconv();
+        $rslt = $localeconv[$key];
+        // win-1252 implements Euro as 0x80 plus other symbols
+        if (preg_match('//u', $rslt) !== 1) {
+            $rslt = '';
+        }
+
+        return $rslt ?: self::useAlt($localeconv[$altKey], $default, $trimAlt);
+    }
+
     /**
      * Get the decimal separator. If it has not yet been set explicitly, try to obtain number
      * formatting information from locale.
@@ -518,14 +535,7 @@ class StringHelper
     public static function getDecimalSeparator(): string
     {
         if (!isset(self::$decimalSeparator)) {
-            $localeconv = localeconv();
-            self::$decimalSeparator = ($localeconv['decimal_point'] != '')
-                ? $localeconv['decimal_point'] : $localeconv['mon_decimal_point'];
-
-            if (self::$decimalSeparator == '') {
-                // Default to .
-                self::$decimalSeparator = '.';
-            }
+            self::$decimalSeparator = self::getLocaleValue('decimal_point', 'mon_decimal_point', '.');
         }
 
         return self::$decimalSeparator;
@@ -549,14 +559,7 @@ class StringHelper
     public static function getThousandsSeparator(): string
     {
         if (!isset(self::$thousandsSeparator)) {
-            $localeconv = localeconv();
-            self::$thousandsSeparator = ($localeconv['thousands_sep'] != '')
-                ? $localeconv['thousands_sep'] : $localeconv['mon_thousands_sep'];
-
-            if (self::$thousandsSeparator == '') {
-                // Default to .
-                self::$thousandsSeparator = ',';
-            }
+            self::$thousandsSeparator = self::getLocaleValue('thousands_sep', 'mon_thousands_sep', ',');
         }
 
         return self::$thousandsSeparator;
@@ -577,22 +580,10 @@ class StringHelper
      *    Get the currency code. If it has not yet been set explicitly, try to obtain the
      *        symbol information from locale.
      */
-    public static function getCurrencyCode(): string
+    public static function getCurrencyCode(bool $trimAlt = false): string
     {
-        if (!empty(self::$currencyCode)) {
-            return self::$currencyCode;
-        }
-        self::$currencyCode = '$';
-        $localeconv = localeconv();
-        if (!empty($localeconv['currency_symbol'])) {
-            self::$currencyCode = $localeconv['currency_symbol'];
-
-            return self::$currencyCode;
-        }
-        if (!empty($localeconv['int_curr_symbol'])) {
-            self::$currencyCode = $localeconv['int_curr_symbol'];
-
-            return self::$currencyCode;
+        if (!isset(self::$currencyCode)) {
+            self::$currencyCode = self::getLocaleValue('currency_symbol', 'int_curr_symbol', '$', $trimAlt);
         }
 
         return self::$currencyCode;

tests/PhpSpreadsheetTests/Reader/Csv/CsvNumberFormatLocaleTest.php

@@ -6,8 +6,12 @@ namespace PhpOffice\PhpSpreadsheetTests\Reader\Csv;
 
 use PhpOffice\PhpSpreadsheet\Cell\DataType;
 use PhpOffice\PhpSpreadsheet\Reader\Csv;
+use PHPUnit\Framework\Attributes;
+use PHPUnit\Framework\Attributes\DataProvider;
 use PHPUnit\Framework\TestCase;
 
+// separate processes due to setLocale
+#[Attributes\RunTestsInSeparateProcesses]
 class CsvNumberFormatLocaleTest extends TestCase
 {
     private bool $localeAdjusted;
@@ -44,8 +48,7 @@ class CsvNumberFormatLocaleTest extends TestCase
         }
     }
 
-    #[\PHPUnit\Framework\Attributes\DataProvider('providerNumberFormatNoConversionTest')]
-    #[\PHPUnit\Framework\Attributes\RunInSeparateProcess]
+    #[DataProvider('providerNumberFormatNoConversionTest')]
     public function testNumberFormatNoConversion(mixed $expectedValue, string $expectedFormat, string $cellAddress): void
     {
         if (!$this->localeAdjusted) {
@@ -85,8 +88,7 @@ class CsvNumberFormatLocaleTest extends TestCase
         ];
     }
 
-    #[\PHPUnit\Framework\Attributes\DataProvider('providerNumberValueConversionTest')]
-    #[\PHPUnit\Framework\Attributes\RunInSeparateProcess]
+    #[DataProvider('providerNumberValueConversionTest')]
     public function testNumberValueConversion(mixed $expectedValue, string $cellAddress): void
     {
         if (!$this->localeAdjusted) {

tests/PhpSpreadsheetTests/Shared/StringHelperLocaleTest.php

@@ -0,0 +1,56 @@
+<?php
+
+declare(strict_types=1);
+
+namespace PhpOffice\PhpSpreadsheetTests\Shared;
+
+use PhpOffice\PhpSpreadsheet\Shared\StringHelper;
+use PHPUnit\Framework\Attributes;
+use PHPUnit\Framework\TestCase;
+
+// separate processes due to setLocale
+#[Attributes\RunTestsInSeparateProcesses]
+class StringHelperLocaleTest extends TestCase
+{
+    /**
+     * @var false|string
+     */
+    private $currentLocale;
+
+    protected function setUp(): void
+    {
+        $this->currentLocale = setlocale(LC_ALL, '0');
+    }
+
+    protected function tearDown(): void
+    {
+        if (is_string($this->currentLocale)) {
+            setlocale(LC_ALL, $this->currentLocale);
+        }
+        StringHelper::setCurrencyCode(null);
+    }
+
+    public function testCurrency(): void
+    {
+        if (!setlocale(LC_ALL, 'de_DE.UTF-8', 'deu_deu.utf8')) {
+            self::markTestSkipped('Unable to set German UTF8 locale for testing.');
+        }
+        $result = StringHelper::getCurrencyCode();
+        self::assertSame('€', $result);
+        if (!setlocale(LC_ALL, 'en_us')) {
+            self::markTestSkipped('Unable to set US locale for testing.');
+        }
+        $result = StringHelper::getCurrencyCode();
+        self::assertSame('€', $result, 'result persists despite locale change');
+        StringHelper::setCurrencyCode(null);
+        $result = StringHelper::getCurrencyCode();
+        self::assertSame('$', $result, 'locale now used');
+        StringHelper::setCurrencyCode(null);
+        if (!setlocale(LC_ALL, 'deu_deu', 'de_DE')) {
+            self::markTestSkipped('Unable to set German single-byte locale for testing.');
+        }
+        // Seems like Linux returns trailing blank, Win doesn't
+        $result = StringHelper::getCurrencyCode(true); // trim if alt symbol is used
+        self::assertSame('EUR', $result, 'non-UTF8 result ignored');
+    }
+}