5. Add &uselang=x-xss to the end of the URL

I was going to say... I hope this wasn't just randomly occurring right now without the use of the new x-xss language feature.

I was kind of curious as to why this wasn't caught by phan-taint-check and started investigating (Its probably because phan-taint-check is mostly disabled on logging class, as they mix plaintext and html in a very confusing way that phan-taint-check does not understand).

It seems like the issue is not Special:UserRights, but Special:Log/rights which gets included on other pages. Anyways, here's a patch.

[Not clear if we consider i18n-xss a secret security issue or a just put it on gerrit issue]

I also wonder if this is a good argument for refactoring the LogFormatter classes. The runtime mixing of html/plaintext based on $this->plaintext is quite confusing to understand, both for humans and computers imo.

In T347726#9211582, @Bawolff wrote:

I was kind of curious as to why this wasn't caught by phan-taint-check and started investigating (Its probably because phan-taint-check is mostly disabled on logging class, as they mix plaintext and html in a very confusing way).

It seems like the issue is not Special:UserRights, but Special:Log/rights which gets included on other pages. Anyways, here's a patch.

T347726.patch1 KBDownload

[Not clear if we consider i18n-xss a secret security issue or a just put it on gerrit issue]

Does it make sense to modify Language::getGroupMemberName? No particular opposition to the method you have put forward, but that is where the messages are defined and returned from using ::text.

I think its generally better to escape closer to where things are outputted. It makes it easier to keep track of what is and isn't escaped.

In T347726#9211582, @Bawolff wrote:

I was kind of curious as to why this wasn't caught by phan-taint-check and started investigating (Its probably because phan-taint-check is mostly disabled on logging class, as they mix plaintext and html in a very confusing way that phan-taint-check does not understand).

It seems like the issue is not Special:UserRights, but Special:Log/rights which gets included on other pages. Anyways, here's a patch.

T347726.patch1 KBDownload

[Not clear if we consider i18n-xss a secret security issue or a just put it on gerrit issue]

+2 to the patch.

In T347726#9211582, @Bawolff wrote:

I was kind of curious as to why this wasn't caught by phan-taint-check and started investigating (Its probably because phan-taint-check is mostly disabled on logging class, as they mix plaintext and html in a very confusing way that phan-taint-check does not understand).

It seems like the issue is not Special:UserRights, but Special:Log/rights which gets included on other pages. Anyways, here's a patch.

T347726.patch1 KBDownload

[Not clear if we consider i18n-xss a secret security issue or a just put it on gerrit issue]

This patch didn't apply for some reason. When I created my own version it worked, but I ended up not deploying my own version. I just wanted to some time to figure out what was happening with the original patch file.

In T347726#9218081, @Mstyles wrote:

This patch didn't apply for some reason. When I created my own version it worked, but I ended up not deploying my own version. I just wanted to some time to figure out what was happening with the original patch file.

Please do not ever leave non-applying patches committed to /srv/patches or you will end up blocking any future deployments. I have dropped your patch to unblock deployments. There was also a commit on top of that adding a patch for T347704, which I have dropped too, since I'm not confident it was ever deployed and tested properly.

Thanks for looking into this, @taavi.

Not sure what happened with the patch for this bug. It appears to be a simple, one-line htmlspecialchars escape, but I can confirm that the original patch did not want to apply correctly to recent master. Not sure if the patch was manually edited or something. The underlying code certainly hasn't changed in a long time. Anyhow, here's a new patch that features the same one-line change that now applies to master, wmf/1.41.0-wmf.28 and wmf/1.41.0-wmf.29. Some additional CR would be nice:

In T347726#9220774, @sbassett wrote:

As for T347704, can you describe what, specifically, leads you to believe it wasn't properly tested? I know there's no explicit CR+2 on the bug, but did this fail some local CI you ran or produce a number of production errors while deployed? Providing any specific issues that you may have noticed that drove your assumption would be appreciated.

I'm not sure if it was applied/tested or not. I just know that the patch for this task not applying blocked scap deployments using scap's built-in patch applying mechanism, which meant that there was a possibility any patches added after this one were not deployed and I didn't want to take the risk of potentially deploying a patch I wasn't familiar with.

I strongly don't understand what happened here, but looking at it in a hexdump, my patch used spaces for indenting, while scott's correctly used tabs.

Also I did find issues with this, but decided it must just be my local machine and made the same change as was in the patch to test it. The updated patch looks good too.

Ok, thanks, all. @Mstyles - I think we could try this one again (with my new patch above) during next Monday's (2023-10-09) security deployment window.