- Issue created by @ioannis.cherouvim
- Status changed to Needs review
about 1 year ago 10:57am 16 August 2023 - last update
about 1 year ago 29,961 pass - 🇫🇮Finland lauriii Finland
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.
- Status changed to Needs work
about 1 year ago 2:55pm 16 August 2023 - 🇺🇸United States smustgrave
Since it doesn't appear to be an XSS issue going to lower to a normal.
Tagging for tests per #2
- Status changed to Needs review
about 1 year ago 4:22pm 16 August 2023 - last update
about 1 year ago 29,962 pass - last update
about 1 year ago 29,961 pass, 1 fail - 🇺🇸United States danflanagan8 St. Louis, US
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 →
- 🇫🇮Finland lauriii Finland
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.
- 🇺🇸United States danflanagan8 St. Louis, US
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? - Status changed to RTBC
about 1 year ago 6:08pm 16 August 2023 - 🇫🇮Finland lauriii Finland
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.
- last update
about 1 year ago 30,047 pass, 1 fail The last submitted patch, 4: 3380624-4-FAIL.patch, failed testing. View results →
- 🇺🇸United States danflanagan8 St. Louis, US
Wrong patch was picked up by the RTBC.
- last update
about 1 year ago 30,051 pass, 1 fail The last submitted patch, 4: 3380624-4-FAIL.patch, failed testing. View results →
- last update
about 1 year ago 30,052 pass - last update
about 1 year ago 30,057 pass - last update
about 1 year ago 30,059 pass - last update
about 1 year ago 30,061 pass - last update
about 1 year ago 30,061 pass - last update
about 1 year ago 30,101 pass - last update
about 1 year ago 30,136 pass - last update
about 1 year ago 30,136 pass - last update
about 1 year ago 30,137 pass - last update
12 months ago 30,147 pass - last update
12 months ago 30,147 pass - last update
12 months ago 30,149 pass - 🇬🇧United Kingdom longwave UK
Committed and pushed 60f2a21d48 to 11.x and ca113255fa to 10.1.x. Thanks!
-
longwave →
committed ca113255 on 10.1.x
Issue #3380624 by danflanagan8, lauriii, ioannis.cherouvim: Toolbar...
-
longwave →
committed ca113255 on 10.1.x
- Status changed to Fixed
12 months ago 8:46pm 12 September 2023 -
longwave →
committed 60f2a21d on 11.x
Issue #3380624 by danflanagan8, lauriii, ioannis.cherouvim: Toolbar...
-
longwave →
committed 60f2a21d on 11.x
- 🇬🇧United Kingdom longwave UK
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.