Problem/Motivation
STR:
1. Install standard profile
2. Enable the content_translation module
3. Programatically create a language using drush for example drush ev "\Drupal\language\Entity\ConfigurableLanguage::create(['id' => 'alex', 'label' => 'Body<img/src=\"x\"/onerror=\"alert(document.domain)\"/>'])->save();"
4. Visit admin/config/regional/content-language and enable translation for article node types
5. Visit admin/structure/types/manage/article and set the Body language to be the default language.
Note that step #2 requires drush or similar access - form validation prevents you from entering script in the UI
reported by alexpott as part of the Drupal 8 security bug bounty
https://tracker.bugcrowd.com/submissions/b176a634642eaf5abbe45591c99b036...
Proposed resolution
✅ Validate language label at the API level when saved
❌ OR filter on output.
Note that we do input validation for translations, so this would potentially be in line with that, versus our normal filter on output of user content.
Remaining tasks
- ✅ decide an approach
- ✅ write patch
- write tests
- review
User interface changes
Any config form that contains a type: label
and uses validation constraints rather than form-based validation will now automatically disallow HTML markup in labels.
API changes
API addition: NoMarkup
validation constraint that can be used on any type: string
Data model changes
type: label
has stricter validation
Issue fork drupal-2514688
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #12
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis came up during a daily bugsmash ticket.
Testing on 9.5 this is still an issue it appears.
I can still programmatically create a language with custom markup but not through the UI.
Comment #13
darvanenThis was today's daily triage target for the Bug Smash Initiative.
I have confirmed behaviour using provided steps to reproduce.
Comment #14
darvanenHowever, all the provided steps to reproduce prove is that it is possible to write custom code that saves an entity without validating it.
Based on the title and IS I believe the real issue here is that if this config entity is exposed for creating via an API such as JSON:API or REST, it does not get validated.
The solution therefore should be to add a constraint to this config entity type per #2869792: [meta] Add constraints to all config entity types.
Comment #16
Wim Leers→ relevant portion:
→ it's the
label
config schema type that is missing validation constraints.#3324984-3: Create test that reports % of config entity types (and config schema types) that is validatable indeed detected this:
and in detail:
Conclusion: rather than solving this only for the
ConfigurableLanguage
config entity type, we should solve this for thelabel
config schema type.P.S.: changing parent, because this validation constraint would be helpful not only for config entities, but also config in general.
Comment #21
marvil07 CreditAttribution: marvil07 at Adapt commentedI am wondering if this was already handled as part of #3377030: Add validation constraint to `type: label`: disallow multiple lines, which adds relevant lines to core label config data type.
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.
Comment #22
Wim LeersLooking 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 🤓
Comment #23
marvil07 CreditAttribution: marvil07 at Adapt commented@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.
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 newNoMarkup
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
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.
Comment #24
Wim LeersThat's wonderful news! 🤩
Before making any further changes, could you please work on improving the validation error message, so that it doesn't say:
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.
Comment #25
Wim LeersJust saw that #3383131: Entity autocomplete form element ignores entities with label "0" 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 😊Comment #26
Wim LeersPreviously tightened validation of
type: label
:This will be an excellent third. At that point, I think that it'll be in its final form 👍
Comment #28
Wim LeersMade issue titles consistent. Updated issue summary.