diff --git a/inc/container.class.php b/inc/container.class.php index 2c023504..2b70fa87 100644 --- a/inc/container.class.php +++ b/inc/container.class.php @@ -621,7 +621,7 @@ public function prepareInputForUpdate($input) { // sanitize label only; name is intentionally left untouched here (see migration callers) if (isset($input['label']) && !empty($input['label'])) { - $input['label'] = PluginFieldsToolbox::sanitizeLabel((string) $input['label']); + $input['label'] = trim((string) $input['label']); } if (isset($input['itemtypes'])) { @@ -799,7 +799,7 @@ public static function generateTemplate($fields) $fields['name'] = PluginFieldsToolbox::sanitizeLabel((string) $fields['name']); $fields['id'] = (int) PluginFieldsToolbox::sanitizeLabel((string) $fields['id']); - $fields['label'] = PluginFieldsToolbox::sanitizeLabel((string) $fields['label']); + $fields['label'] = (string) $fields['label']; foreach ($itemtypes as $itemtype) { $sysname = self::getSystemName($itemtype, $fields['name']); diff --git a/inc/dropdown.class.php b/inc/dropdown.class.php index ba0e9bab..7b1e3c88 100644 --- a/inc/dropdown.class.php +++ b/inc/dropdown.class.php @@ -116,7 +116,7 @@ public static function create($input) // Safe inputs $input['name'] = PluginFieldsToolbox::sanitizeLabel((string) $input['name']); $input['id'] = (int) PluginFieldsToolbox::sanitizeLabel((string) $input['id']); - $input['label'] = PluginFieldsToolbox::sanitizeLabel((string) $input['label']); + $input['label'] = (string) $input['label']; $classname = self::getClassname($input['name']); diff --git a/inc/labeltranslation.class.php b/inc/labeltranslation.class.php index f189f51f..c319ac0b 100644 --- a/inc/labeltranslation.class.php +++ b/inc/labeltranslation.class.php @@ -113,14 +113,14 @@ public static function getTypeName($nb = 0) public function prepareInputForUpdate($input) { - $input['label'] = PluginFieldsToolbox::sanitizeLabel((string) ($input['label'] ?? '')); + $input['label'] = trim((string) $input['label']); return $input; } public function prepareInputForAdd($input) { - $input['label'] = PluginFieldsToolbox::sanitizeLabel((string) ($input['label'] ?? '')); + $input['label'] = trim((string) $input['label']); return $input; } diff --git a/inc/toolbox.class.php b/inc/toolbox.class.php index f1922a6c..8f27daab 100644 --- a/inc/toolbox.class.php +++ b/inc/toolbox.class.php @@ -400,7 +400,7 @@ public static function sanitizeLabel(string $label): string */ public static function prepareLabel(array $input): array { - $input['label'] = self::sanitizeLabel((string) ($input['label'] ?? '')); + $input['label'] = trim((string) ($input['label'] ?? '')); $input['name'] = (new self())->getSystemNameFromLabel($input['label']); return $input; diff --git a/tests/Units/LabelSpecialCharsTest.php b/tests/Units/LabelSpecialCharsTest.php new file mode 100644 index 00000000..0fd5e1c5 --- /dev/null +++ b/tests/Units/LabelSpecialCharsTest.php @@ -0,0 +1,235 @@ +. + * ------------------------------------------------------------------------- + * @copyright Copyright (C) 2013-2023 by Fields plugin team. + * @license GPLv2 https://www.gnu.org/licenses/gpl-2.0.html + * @link https://github.com/pluginsGLPI/fields + * ------------------------------------------------------------------------- + */ + +declare(strict_types=1); + +namespace GlpiPlugin\Field\Tests\Units; + +use Computer; +use Glpi\Tests\DbTestCase; +use Glpi\Tests\GLPITestCase; +use GlpiPlugin\Field\Tests\FieldTestTrait; +use PHPUnit\Framework\Attributes\DataProvider; +use PluginFieldsContainer; +use PluginFieldsDropdown; +use PluginFieldsField; +use PluginFieldsLabelTranslation; + +require_once __DIR__ . '/../FieldTestCase.php'; + +/** + * Labels are free text: they must be stored and displayed as typed by the user, + * while system names (tables, columns, classes, files) stay restricted to safe chars. + */ +final class LabelSpecialCharsTest extends DbTestCase +{ + use FieldTestTrait; + + public function setUp(): void + { + GLPITestCase::setUp(); + $this->login(); + } + + public function tearDown(): void + { + $this->tearDownFieldTest(); + GLPITestCase::tearDown(); + } + + public static function provideLabels(): iterable + { + yield 'apostrophe' => [ + 'label' => "N° d'inventaire", + ]; + + yield 'slash' => [ + 'label' => 'Site / Bâtiment', + ]; + + yield 'parentheses and ampersand' => [ + 'label' => 'Contrat (R&D)', + ]; + + yield 'double quotes' => [ + 'label' => 'Le "bon" champ', + ]; + + yield 'accents and punctuation' => [ + 'label' => 'Échéance : été, hiver ?', + ]; + + yield 'html markup' => [ + 'label' => 'Important', + ]; + + yield 'php code injection attempt' => [ + 'label' => "x'); die('pwned'); //", + ]; + } + + #[DataProvider('provideLabels')] + public function testContainerLabelKeepsSpecialChars(string $label): void + { + $container = $this->createFieldContainer([ + 'label' => $label, + 'type' => 'tab', + 'itemtypes' => [Computer::class], + 'is_active' => 1, + 'entities_id' => 0, + 'is_recursive' => 1, + ]); + + $this->assertTrue($container->getFromDB($container->getID())); + $this->assertSame($label, $container->fields['label']); + + // System name is still restricted to identifier safe chars + $this->assertMatchesRegularExpression('/^[a-z]+$/', $container->fields['name']); + + // Displayed label (translation) is kept as typed + $this->assertSame( + $label, + PluginFieldsLabelTranslation::getLabelFor([ + 'itemtype' => PluginFieldsContainer::class, + 'id' => $container->getID(), + ]), + ); + } + + #[DataProvider('provideLabels')] + public function testFieldLabelKeepsSpecialChars(string $label): void + { + $container = $this->createFieldContainer([ + 'label' => 'Label special chars ' . $this->getUniqueString(), + 'type' => 'tab', + 'itemtypes' => [Computer::class], + 'is_active' => 1, + 'entities_id' => 0, + 'is_recursive' => 1, + ]); + + $field = $this->createField([ + 'label' => $label, + 'type' => 'text', + PluginFieldsContainer::getForeignKeyField() => $container->getID(), + 'ranking' => 1, + 'is_active' => 1, + ]); + + $this->assertTrue($field->getFromDB($field->getID())); + $this->assertSame($label, $field->fields['label']); + + // System name is still restricted to identifier safe chars + $this->assertMatchesRegularExpression('/^[a-z0-9_]+$/', $field->fields['name']); + + // Displayed label (translation) is kept as typed + $this->assertSame( + $label, + PluginFieldsLabelTranslation::getLabelFor([ + 'itemtype' => PluginFieldsField::class, + 'id' => $field->getID(), + ]), + ); + } + + #[DataProvider('provideLabels')] + public function testTranslationUpdateKeepsSpecialChars(string $label): void + { + $container = $this->createFieldContainer([ + 'label' => 'Label translation ' . $this->getUniqueString(), + 'type' => 'tab', + 'itemtypes' => [Computer::class], + 'is_active' => 1, + 'entities_id' => 0, + 'is_recursive' => 1, + ]); + + $field = $this->createField([ + 'label' => 'Initial label', + 'type' => 'text', + PluginFieldsContainer::getForeignKeyField() => $container->getID(), + 'ranking' => 1, + 'is_active' => 1, + ]); + + $translations = (new PluginFieldsLabelTranslation())->find([ + 'itemtype' => PluginFieldsField::class, + 'items_id' => $field->getID(), + 'language' => $_SESSION['glpilanguage'], + ]); + $this->assertCount(1, $translations); + + $translation = new PluginFieldsLabelTranslation(); + $this->assertTrue($translation->update([ + 'id' => array_key_first($translations), + 'label' => $label, + ])); + + $this->assertSame( + $label, + PluginFieldsLabelTranslation::getLabelFor([ + 'itemtype' => PluginFieldsField::class, + 'id' => $field->getID(), + ]), + ); + } + + #[DataProvider('provideLabels')] + public function testDropdownClassIsSafelyGeneratedWithSpecialCharsInLabel(string $label): void + { + $container = $this->createFieldContainer([ + 'label' => 'Label dropdown ' . $this->getUniqueString(), + 'type' => 'tab', + 'itemtypes' => [Computer::class], + 'is_active' => 1, + 'entities_id' => 0, + 'is_recursive' => 1, + ]); + + $field = $this->createField([ + 'label' => $label, + 'type' => 'dropdown', + PluginFieldsContainer::getForeignKeyField() => $container->getID(), + 'ranking' => 1, + 'is_active' => 1, + ]); + + $classname = PluginFieldsDropdown::getClassname($field->fields['name']); + + // The label is injected in the generated class file as an exported string literal only + $class_file = PLUGINFIELDS_CLASS_PATH . '/' . $field->fields['name'] . 'dropdown.class.php'; + $this->assertFileExists($class_file); + $this->assertStringContainsString(var_export($label, true), (string) file_get_contents($class_file)); + + // The generated class is valid, loadable and returns the label as typed + $this->assertTrue(class_exists($classname)); + $this->assertSame($label, $classname::getTypeName()); + } +} \ No newline at end of file