I work on the MediaWiki Security Team.
Yeah, i think it makes sense to move this job back to nonvoting until this ticket is figured out
Wed, Sep 19
Mon, Sep 17
While we are on the subject, I think firstname.lastname@example.org needs to be removed
Review of revision 988d242afdd2
Sun, Sep 16
Would Community-Tech be able to work on remediating this issue, as the team that worked on this feature initially?
Approved, looks good.
Fri, Sep 14
The current PHP sanitizer mechanism seems to encourage extension authors to emit HTML (rather than wikitext) if they need access to elements which would otherwise be sanitized, and the HTML-output mode bypasses the sanitizer completely. That increases the burden of security review, since now every part of that extension could be an unwitting vector for evil user-generated HTML. If instead the extension output is *always* sanitized, and there are more fine-grained mechanisms to tunnel specific "allowed" features through the sanitizer, we can undertake more focused security reviews of a smaller trusted code base.
Thu, Sep 13
FWIW, Personally, I dislike the idea of using brackets as a delimiter. That seems confusing.
Wed, Sep 12
To clarify some things:
If these user_properties actually were being used for malicious stalking, and there had been real cases where this was the root cause, then fine, let's say that and at least say how many incidents there have been. My guess, it's zero.
Tue, Sep 11
Hi. At this time we decided not to do this. We think having the whitelist reduces risk well at the same time is really not very annoying as there is a relatively easy process to get config changes through.
We're going to decline this for now. Please reopen once you have something more concrete for us to review
Hi, we need agreement that this extension should be deployed before we will review it formally.
Anyway, personally I think the various user rights exceptions with hooks are really confusing and getting out of hand. I think we should do things around "rights" and assign things to groups, for clarity's sake. So something (based on the viewdeletedfile right) along the lines of:
The extension isnt documented as restricting anything and doesnt hook into the mediawiki permission system...so that seems to be working as intended? (It does have code to not give the redirect to some people but thats not really documented as a security measure)
Mon, Sep 10
The bug is public now.
Sun, Sep 9
Its a really common pattern to do <msg>/langcode. Its even supported by us if the msg is not content language msg (which would be insane for a js msg).
Sat, Sep 8
yes. I imagine that LibraryUpgrader will get to it in short order.
This was fixed by suppressing the errors. See T203690
It appears that phan-taint-check is voting on MinervaNeue, so removing that project from this bug.
Huh. Currently failing due to:
Package mediawiki/phan-taint-check-plugin at version 1.5.0 has a PHP requirement incompatible with your PHP version (5.6.33)
It seems like it is mostly failing on double escaping in debug related code:
./gateway_common/GatewayPage.php:271 SecurityCheck-DoubleEscaped Calling method \Html::element() in \GatewayPage::displayResultsForDebug that outputs using tainted argument $[arg #3]. (Caused by: Builtin-\Html::element) (Caused by: ./gateway_common/GatewayPage.php +270) ./gateway_common/GatewayPage.php:283 SecurityCheck-DoubleEscaped Calling method \Html::element() in \GatewayPage::displayResultsForDebug that outputs using tainted argument $[arg #3]. (Caused by: Builtin-\Html::element) (Caused by: ./gateway_common/GatewayPage.php +282; ./gateway_common/GatewayPage.php +282) ./gateway_common/GatewayPage.php:287 SecurityCheck-DoubleEscaped Calling method \Html::element() in \GatewayPage::displayResultsForDebug that outputs using tainted argument $[arg #3]. (Caused by: Builtin-\Html::element) (Caused by: ./gateway_common/GatewayPage.php +279; ./gateway_common/GatewayPage.php +279) ./gateway_common/GatewayPage.php:308 SecurityCheck-DoubleEscaped Calling method \Html::element() in \GatewayPage::displayResultsForDebug that outputs using tainted argument $val. (Caused by: Builtin-\Html::element) (Caused by: ./gateway_common/GatewayPage.php +307) ./globalcollect_gateway/globalcollect_gateway.body.php:82 SecurityCheck-DoubleEscaped Calling method \htmlspecialchars() in \GlobalCollectGateway::displayBankTransferInformation that outputs using tainted argument $[arg #1]. (Caused by: ./globalcollect_gateway/globalcollect_gateway.body.php +57) ./globalcollect_gateway/globalcollect_gateway.body.php:136 SecurityCheck-DoubleEscaped Calling method \htmlspecialchars() in \GlobalCollectGateway::displayOnlineBankTransferInformation that outputs using tainted argument $[arg #1]. (Caused by: ./globalcollect_gateway/globalcollect_gateway.body.php +117)
Fri, Sep 7
interesting. TIL, that not all the logos are the same size. The docs (in includes/DefaultSettings.php) do say The logo size should be 135x135 pixels but guess that's not true in practise.
The name comes from The Good Place, which if you have seen the show, is a fitting name for the work we are doing (if you haven't, I wont spoil it for you). This also seemed like something more fun than "aht" or it's derivatives. :)
Actually we probably don't want background-size:contain, because the a.mw-wiki-logo has a size of 160x160, but logos are supposed to be 135x135 (I think).
[I didn't actually test, but when I copied into the other extension i was working on, the code didn't work]
Thu, Sep 6
Leaving this bug open as there were additional failures I surpressed. They seemed reasonable, except they don't show up when I run tests locally, so I have to figure out what's going on there...
As an aside its too bad we cant kill this behaviour. Its a privacy risk.
Thanks for the comment! If I understand correctly, this sounds sensible, but I'm really not familiar with this. Who is developing it? (You?)
There are still quite a few false positives to sort out, but its starting to get more manageable
It seems like there is still a problem when using things like $dbr->makeList( [ 'foo' => [ $evil1, $evil2 ] ], LIST_AND ); which should be safe (See Block.php in core.
Wed, Sep 5
However, if we use ULS to set the language, the assumption might be that people will touch the language selector rarely, so its not a common case to get to the other url.
Varnish seems to be caching it fine (Assuming you don't have cookies, which I imagine pretty much nobody would as not SUL). However, if you use language selector you end up at https://fixcopyright.wikimedia.org/wiki/Fix_copyright?title=Fix_copyright&uselang=fr which seems to have an X-Cache-Status of pass, so I guess that is not varnish cached.
Ok. So I made 2 patches, provided that those are merged. This is approved subject to the following: