Add validation constraint to `type: label`: disallow HTML markup

Conclusion: rather than solving this only for the ConfigurableLanguage config entity type, we should solve this for the label config schema type.

I am wondering if this was already handled as part of 📌 Add validation constraint to `type: label`: disallow multiple lines Fixed , which adds relevant lines to core label config data type.

$ git grep -h -A10 ^label core/config/schema/core.data_types.schema.yml
label:
  type: string
  label: 'Optional label'
  translatable: true
  constraints:
    Regex:
      # Forbid any kind of control character.
      # @see https://stackoverflow.com/a/66587087
      pattern: '/([^\PC])/u'
      match: false
      message: 'Labels are not allowed to span multiple lines or contain control characters.'

We may want to add an extra constraint to validate no HTML mark-up is present, to actually address the original report at the description.
For that drupal Html component may be enough, along with a custom constraint.

I opened a new merge request around it on top of the 2514688-label-config-validation-nomarkup branch, on the issue fork.
Let us see if tests run points somewhere.

Looking great, thank you! 🤩

We added validation constraints to type: label months ago, but only for the allowed character range. This is adding a secondary layer of validation, and one that makes a ton of sense 👍

(I actually forgot about this issue despite having commented on it at the start of the year in #16 😅)

One nit, and one important missing thing: test coverage 🤓

@Wim Leers, thanks for the feedback!

This issue is growing lines changed 😅, it seems like the extra validation is uncovering lots of places where configuration needs to be changed to be compatible with the new NoMarkup constraint.

One nit, and one important missing thing: test coverage

Incorporated your suggestion on the violation message.

On test coverage, that is still pending for the new validation constraint.
Said that, I see it being used across lots of tests ;-)

This round, I have been focused on trying to solve a subset of the test failures that the newly added configuration validation is triggering on existing tests, namely unit and kernel tests.
Basically, in many places, the randomString() method is being used to generate configuration values of type label across many tests.
That method can produce some characters that are stripped by strip_tags(), used behind the new NoMarkup constraint.
Instead randomName() should be used to generate strings that are intended to be of label configuration type.

New commits added to the merge request were the following.

- 5238db45a2 Apply 1 suggestion(s) to 1 file(s)
- 67279343f3 Use known filter format names on ValidatorsTest
- 50daa4efd3 Tweak documentation on random machine name method on test trait
- 7a6ff4b395 Expose name generation from Random component into random test related trait
- 3be9e310df Expose name generation on RandomGeneratorTrait
- 17f9235577 Use simple name generation for role name on UserCreationTrait
- 9b541f76e3 Use random name generation for BookPendingRevisionTest content type label
- 65ae3c4475 Handle extra validation error triggered on SimpleConfigValidationTest
- 4b014a1b3c Use random name generation for EntityAccessControlHandlerTest
- 0b9be9fac0 Use random name generation on EntityLanguageTestBase
- 4b580de001 Document SimpleConfigValidationTest::testSpecialCharacters() new parameter
- 6c90468755 Use name generation on EntityReferenceItemTest
- 5641798c63 Use random name generation on EntityReferenceSettingsTest
- 06a13129b4 Use random name generation on TranslationTest
- b84fa8d7b2 Use random name generation on MediaTypeCreationTrait
- 289104ff11 Use random name generation on FieldDiscoveryTest

Next steps

Likely, to continue to fix the failing tests.
Tests are understandably failing on configuration validation.

First, let us continue with the simple errors, as the last commits.

Second, let us dive into the migration test errors, throwing unserializable exceptions.
This likely similar to the one found at #3395616-8: Add validation constraints to image.settings → , fixed with commit 3efe72af45f211827a4bec599a84b50085ca5b7e.

Third, let us dive into functional test failing.
E.g. on BlockUiTest, I see the following exception thrown

Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for block.block.stark_scriptalertxsssubjectscript with the following errors: 0 [settings.label] The value should not contain HTML markup in Drupal\Core\Config\Development\ConfigSchemaChecker->onConfigSave() (line 94 of core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php). 

That is exactly expected, since its previous step is trying to save <script>alert('XSS subject');</script> as the title of a block, so it is likely we want to change the handling of the exception there to handle the new possible validation error from configuration validation, and not from the form handling side only.

Fourth, let us add specific test coverage for the new constraint.

it seems like the extra validation is uncovering lots of places where configuration needs to be changed to be compatible with the new NoMarkup constraint.

That's wonderful news! 🤩

Before making any further changes, could you please work on improving the validation error message, so that it doesn't say:

\Tests\media_library\Kernel\MediaLibraryAccessTest::testEditorOpenerAccess with data set "media_embed filter enabled" (true, true)
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for filter.format.yyd25w6y with the following errors: 0 [name] The value should not contain HTML markup

Because The value should not contain HTML markup is not very helpful: it doesn't say exactly what is the problem.

It'd be great if it could point at exactly which position the problem occurs.

This may require adopting a different strategy than strip_tags(). Which may be a good thing, because it stripping the character zero (one of the test changes you had to made) makes little sense IMHO 😅

Elevating priority because this is a security improvement and the number of test failures suggest this may be a risk already today.

Just saw that 🐛 Entity autocomplete form element ignores entities with label "0" Needs review landed. That's in a completely different part of Drupal, but shows how 0 truly is a valid (entity) label that we need to avoid false negatives for 😊

Previously tightened validation of type: label:

1. 📌 Add validation constraint to `type: label`: disallow multiple lines Fixed
2. 📌 Add validation constraint to `type: label` + `type: text`: disallow control characters Fixed

This will be an excellent third. At that point, I think that it'll be in its final form 👍

Wim Leers → credited alexpott → .

Made issue titles consistent. Updated issue summary.