Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
The search on the /admin/modules/uninstall page is broken. Entering at least two characters makes all modules disappear.
Proposed resolution
Find reason for no results, fix it.
Remaining tasks
Do it, provide patch, review and commit it.
User interface changes
The uninstall search works again!
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#24 | 2512106.24.patch | 2.93 KB | alexpott |
#24 | 2512106.24-test-only.patch | 1.96 KB | alexpott |
#24 | 16-24-interdiff.txt | 3.13 KB | alexpott |
#16 | search_filter_on_the-2512106-16.patch | 2.04 KB | cilefen |
#16 | search_filter_on_the-2512106-16-tests.patch | 945 bytes | cilefen |
Comments
Comment #1
cilefen CreditAttribution: cilefen commentedComment #2
droplet CreditAttribution: droplet commentedany idea how to set allowed tags in inline_template ?
Comment #3
LKS90 CreditAttribution: LKS90 commentedThe patch works for me, I don't have an answer for your question though.
Comment #4
cilefen CreditAttribution: cilefen commented#2280965: [meta] Remove every SafeMarkup::set() call contains some suggested patterns for avoiding SafeMarkup::set().
Comment #5
cilefen CreditAttribution: cilefen commentedI searched around a bit but couldn't figure it out.
Comment #6
cilefen CreditAttribution: cilefen commented@LKS90 HEAD is using an inline twig template to safely render the table of module names. Some commit has broken this search form by stripping the
<label>
tag. The question in #2 was over how one would allow the label tag to pass through.Comment #7
cilefen CreditAttribution: cilefen commentedI found it.
Comment #8
TR CreditAttribution: TR commentedXss.php hasn't changed its list of allowed tags since it was first created more than two years ago in commit 23b59123. Adding a new tag to the allowed list in Xss.php may avoid this particular problem, but it will have very wide consequences (every module uses Xss::filter() at some point) and may impact security.
The issue which broke the uninstall page is #2273925: Ensure #markup is XSS escaped in Renderer::doRender() (I used git bisect). IMO a proper fix will be to address how to use known-good markup in an inline_template element, now that this markup is being automatically sanitized. The patch in #1 is a solution, but it would be nice to keep the inline_template here if possible (see #2280965: [meta] Remove every SafeMarkup::set() call). Unfortunately, SafeMarkup::set() doesn't work in the inline_template. If the uninstall page has this problem, it's inevitable that it will show up again, especially in contrib, so it makes sense to figure out a global fix rather than just make <label> a safe tag like in #7.
Comment #9
droplet CreditAttribution: droplet commentedahh. So SafeMarkup::format is the best option at the moment.
Comment #10
droplet CreditAttribution: droplet commentedComment #11
cilefen CreditAttribution: cilefen commentedWe need commas after the last array elements.
Comment #12
droplet CreditAttribution: droplet commentedthat's args for the function. adding commas would cause syntax errorComment #13
droplet CreditAttribution: droplet commentedComment #14
cilefen CreditAttribution: cilefen commented+1 for RTBC. This path is in-scope and uses one of the allowed sanitization methods.
I hate to be a nudge but there should be a comma here also as the end of the multiline 'data' array element.
Comment #15
droplet CreditAttribution: droplet commentedCool, no problems :)
Comment #16
cilefen CreditAttribution: cilefen commentedI added a small regression test.
Comment #18
droplet CreditAttribution: droplet commentedComment #21
TR CreditAttribution: TR commentedComment #23
alexpottThe result of inline templates should be marked safe. This is indicative of a larger issue.
Comment #24
alexpottWe need to do something like this. btw this is a really nice find. Glad we're dealing with this now.
Comment #25
alexpottComment #26
alexpottRetitling to detail what the actual issue is.
Comment #28
star-szrComment #29
joelpittetWow surprised we didn't run into this already. Thanks for the test coverage @alexpott.
Comment #30
TR CreditAttribution: TR commentedI tested the patch and indeed it solves this specific problem (filtering on uninstall modules page is broken) and also the larger issue of using known-good markup in an inline_template. The added test ensures that the markup required by the JavaScript for the filter to work is present - that should preclude this from getting broken again.
+1.
Comment #31
catchCommitted/pushed to 8.0.x, thanks!