Toolbar username lazy builder only XSS filters but doesn't escape user display name - stored remote request

I don't think there's actual XSS because #markup is still run through \Drupal\Component\Utility\Xss::filterAdmin but definitely should fix this especially because it has been explicitly documented that the username is escaped.

Would be nice to write a small test for this but didn't find a good existing test case to extend.

Since it doesn't appear to be an XSS issue going to lower to a normal.

Tagging for tests per #2

Here's a fail test and another with the fix applied on top.

I never know if the test writer should remove the Needs tests tag, but I'm just going to go for it. :)

The last submitted patch, 4: 3380624-4-FAIL.patch, failed testing. View results →

I'm wondering if an integration test would be a better fit here? The unit test (in this case) is essentially just doing a very straight forward comparison between arrays which doesn't actually prove that the behavior is correct.

Hi @lauriii

For the sake of performance, I was thinking about the issue at a low level. At a low level, there's a specific method returning a render array that uses #markup where it should be using #plain_text. That's all the test covers, and I think that's all we really need to prove here.

I don't think we need to prove that using #plain_text prevents the possibility of injection in the toolbar specifically because there are presumably tests that show that #plain_text prevents injection in general.

If we did an integration test are you thinking we'd add a test module that implements user_format_name_alter and more or less reproduces what's going on in the IS?

I agree that technically this is sufficient – I'm not sure that we need an integration test. Usually this would be a nice thing to have, but the reason I'm hesitant in this case is that by default, Drupal doesn't allow entering special characters in usernames so the test would be pretty strange.

The last submitted patch, 4: 3380624-4-FAIL.patch, failed testing. View results →

Wrong patch was picked up by the RTBC.

The last submitted patch, 4: 3380624-4-FAIL.patch, failed testing. View results →

Re-uploading patch from #4.

Committed and pushed 60f2a21d48 to 11.x and ca113255fa to 10.1.x. Thanks!

longwave → credited MariaIoann → .

Apologies @mariaioann I failed to credit you in the commit message, but adding issue credit here.

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