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

Files: 
CommentFileSizeAuthor
#24 2512106.24.patch2.93 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,569 pass(es). View
#24 2512106.24-test-only.patch1.96 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,267 pass(es), 2 fail(s), and 0 exception(s). View
#24 16-24-interdiff.txt3.13 KBalexpott
#16 search_filter_on_the-2512106-16.patch2.04 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,209 pass(es). View
#16 search_filter_on_the-2512106-16-tests.patch945 bytescilefen
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,282 pass(es), 1 fail(s), and 0 exception(s). View
#16 diff-2512106-15-16.txt817 bytescilefen
#15 uninstall-filter-15.patch1.12 KBdroplet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,158 pass(es). View
#7 search_filter_on_the-2512106-7.patch1.67 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,316 pass(es). View
#5 search_filter_on_the-2512106-5.patch839 bytescilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,309 pass(es). View
#2 uninstall-filter.patch1.17 KBdroplet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,306 pass(es). View

Comments

cilefen’s picture

Title: Search filter for uninstall module has no results » Search filter on the uninstall modules page is broken
Priority: Normal » Major
Issue tags: +JavaScript
droplet’s picture

Status: Active » Needs review
FileSize
1.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,306 pass(es). View

any idea how to set allowed tags in inline_template ?

LKS90’s picture

Status: Needs review » Reviewed & tested by the community

The patch works for me, I don't have an answer for your question though.

cilefen’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/system/system.admin.inc
@@ -320,14 +320,12 @@ function theme_system_modules_uninstall($variables) {
+        'data' => SafeMarkup::set($col2)

#2280965: [meta] Remove every SafeMarkup::set() call contains some suggested patterns for avoiding SafeMarkup::set().

cilefen’s picture

Status: Needs work » Needs review
FileSize
839 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,309 pass(es). View

any idea how to set allowed tags in inline_template ?

I searched around a bit but couldn't figure it out.

cilefen’s picture

@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.

cilefen’s picture

FileSize
1.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,316 pass(es). View

I found it.

TR’s picture

Status: Needs review » Needs work

Xss.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.

droplet’s picture

ahh. So SafeMarkup::format is the best option at the moment.

droplet’s picture

Status: Needs work » Needs review
Related issues: +#2501737: Remove SafeMarkup::set in theme_system_modules_details()
FileSize
1.08 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,163 pass(es). View
cilefen’s picture

+++ b/core/modules/system/system.admin.inc
@@ -323,10 +323,12 @@ function theme_system_modules_uninstall($variables) {
+            '@module_name' => drupal_render($form['modules'][$module]['name'])
+          )
         )

We need commas after the last array elements.

droplet’s picture

that's args for the function. adding commas would cause syntax error

droplet’s picture

FileSize
1.08 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,170 pass(es). View
cilefen’s picture

+1 for RTBC. This path is in-scope and uses one of the allowed sanitization methods.

+++ b/core/modules/system/system.admin.inc
@@ -323,10 +323,12 @@ function theme_system_modules_uninstall($variables) {
+          )

I hate to be a nudge but there should be a comma here also as the end of the multiline 'data' array element.

droplet’s picture

FileSize
1.12 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,158 pass(es). View

Cool, no problems :)

cilefen’s picture

FileSize
817 bytes
945 bytes
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,282 pass(es), 1 fail(s), and 0 exception(s). View
2.04 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,209 pass(es). View

I added a small regression test.

The last submitted patch, 16: search_filter_on_the-2512106-16-tests.patch, failed testing.

droplet’s picture

Status: Needs review » Reviewed & tested by the community

TR’s picture

Title: Search filter on the uninstall modules page is broken » Regression: Search filter on the uninstall modules page broken by [#2273925]

The last submitted patch, 16: search_filter_on_the-2512106-16-tests.patch, failed testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

The result of inline templates should be marked safe. This is indicative of a larger issue.

alexpott’s picture

Status: Needs work » Needs review
Related issues: +#2506581: Remove SafeMarkup::set() from Renderer::doRender, +#2273925: Ensure #markup is XSS escaped in Renderer::doRender()
FileSize
3.13 KB
1.96 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,267 pass(es), 2 fail(s), and 0 exception(s). View
2.93 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,569 pass(es). View

We need to do something like this. btw this is a really nice find. Glad we're dealing with this now.

alexpott’s picture

Issue tags: +SafeMarkup
Related issues:
alexpott’s picture

Title: Regression: Search filter on the uninstall modules page broken by [#2273925] » Inline templates are XSS filtered incorrectly

Retitling to detail what the actual issue is.

The last submitted patch, 24: 2512106.24-test-only.patch, failed testing.

Cottser’s picture

Issue tags: +Twig
joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Wow surprised we didn't run into this already. Thanks for the test coverage @alexpott.

TR’s picture

I 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.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 0f9ebb2 on 8.0.x
    Issue #2512106 by cilefen, droplet, alexpott: Inline templates are XSS...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.