Page MenuHomePhabricator

SecurityCheck-XSS Suppressed in CentralNotice for Geotargeting
Closed, ResolvedPublic

Description

The SecurityCheck-XSS had to be suppressed in addNoticeForm() and outputNoticeDetail() to get change https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/CentralNotice/+/518449 to pass build. The reason seems to be in geoMultiSelectorTree (CentralNotice:1552), but I couldn't figure out why. It would be good for there to be more investigation by someone who understands this bettter. We are going to +2 the patch for now.

Event Timeline

Assuming this task is about the MediaWiki-extensions-CentralNotice codenase, hence adding that project tag so other people who don't know about team tags can also find this task when searching via projects.

Change 597338 had a related patch set uploaded (by Mepps; owner: Mepps):
[mediawiki/extensions/CentralNotice@master] Remove XSS suppression added in Geotargeting

https://gerrit.wikimedia.org/r/597338

Funnily enough, I removed the suppression and it's passing CI. @AndyRussG and I remain curious about why it was failing before. I will say that I know it was happening in $htmlOut .= Xml::tags( 'td', [], $this->geoMultiSelectorTree() ); in addNoticeForm() CentralNotice.php:410.

Re: SecurityCheckPlugin in CI - there's been a lot of recent work on it, which may have had some effect (or not) and it's also known to be a bit non-deterministic at times. If we (Security-Team or @Daimona) could check some of the relevant jenkins output, we could perhaps narrow it down a bit more.

The original issue was found too long ago (June 2019) for us to be able to understand what happened. The codebase has surely changed a lot since then, and so did taint-check. Also, taint-check <3.0.0 didn't warn about unused suppressions, which means we can't tell when the issue was fixed exactly.

The only way to do that would be to checkout PS4 of r518449 and run taint-check 1.5 on it, but I think it would be a waste of time. Taint-check has improved a lot since then, and the issue might have been a false positive. In fact, I've just tried running taint-check 3.0.2 on CentralNotice master (without the @suppress annotation) and didn't get any XSS reported, so it was likely a false positive. I've also checked the line 410 as reported above, and there seems to be no taintedness involved.

P.S.:

it's also known to be a bit non-deterministic at times.

You can well say that it's often black magic :-D

Change 597338 merged by jenkins-bot:
[mediawiki/extensions/CentralNotice@master] Remove XSS suppression added in Geotargeting

https://gerrit.wikimedia.org/r/597338