In T202596#4600586, @jcrespo wrote:

That looks really bad performance. Not only that scans the revision table from top to bottom (>200GB of data) making it slow, it is nondeterministically slow- it will be faster or slower depending on the parameters and existing data.

You can emulate it by running (don't run it, it doesn't finish on production and you may not be able to kill it):

root@db1089[enwiki]> select revision.rev_id, page.page_title from revision left join page     on page.page_title = concat('Diff/', revision.rev_id) where    page.page_namespace = 4 order by revision.rev_id desc limit 100;
^CCtrl-C -- query killed. Continuing normally.
ERROR 1317 (70100): Query execution was interrupted

Does this seem like a good alternative to maintaining a link table between rev_id and judgment_page.page_id?

Please create a suitable schema with simple queries. Queries that do more complex stuff than point selects using primary keys regarding the revision table will just not work on production, with very close to 1 billion rows. Please test your queries on the wikirreplicas to check they are suitable for production.

Please CC in the future @Marostegui and @mark.

Yeah, i think it makes sense to move this job back to nonvoting until this ticket is figured out

In T204016#4598875, @Mooeypoo wrote:

@Bawolff A concern was raised in the team that this is potentially quite heavier than a relatively quick security fix; it involves some potential back-and-forth on how to handle the behavior.

Now that we have the abuse filter that @MusikAnimal created, which seems to block the loophole itself on English Wiki, and considering that this only is enabled on English WIki at all, can we work on a followup fix as non-security? As in, can we have it on gerrit, going formal review?

In T204590#4590919, @Dzahn wrote:

Can someone add @sbassett to security@ please?

done, based on "new security engineer"

I think apalmer@wikimedia.org needs to be removed

can OIT please trigger the regular offboarding workflow to notify us? It already caused a mess (spam/ bounces) that the email address was removed without warning (on a weekend even) while still being on aliases and we still don't know if this is a full offboarding.

While we are on the subject, I think apalmer@wikimedia.org needs to be removed

Added

Review of revision 988d242afdd2

Would Community-Tech be able to work on remediating this issue, as the team that worked on this feature initially?

Approved, looks good.

In T21291#4468577, @Simetrical wrote:

The PHP taint module seems like a reasonable tool, although I don't know how waterproof it is. We wouldn't have to require it, but we could support it if installed, and install it on Wikimedia and monitor the logs.

A more comprehensive solution would be to make message-retrieving functions not return raw strings, but rather some class like UntrustedString. This wouldn't auto-convert to a string, you'd have to convert it with escaping methods, like maybe ->forHTML() or ->forSQL() or such. We might not even have to have a method to get the raw string, unless there are valid use-cases. Then the only problem would be if someone writes code that escapes for the wrong type of output, like escaping for SQL then outputting as HTML. I'm not so worried about this getting past code review, but if we wanted extra safety, we could have UntrustedSQLString and so on that get passed around instead of strings, and have HTML output methods only accept raw strings or UntrustedHTMLStrings (and perhaps auto-escape plain UntrustedStrings, if we're in favor of being less explicit).

This is basically the same idea as Nikerabbit's patch, except using classes instead of mangling the strings, and I think it should be usable for production. Once we have such a mechanism in place, we can also use it for Request.

In T204279#4582535, @Bawolff wrote:

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.

I'm not sure I agree with this assessment. I think the problem lies in the parser TagHook interface which promotes outputting html, not the Sanitizer. Additionally perhaps a culture of skipping the sensitization process entirely when needed instead of skipping it on a fine grained basis.

I'm not sure I see the practical difference between this proposal, and having strip markers like we currently do. Arguably this proposal is more elegant in a way as it doesn't rely on substitution but nice DOM methods, however at the end of the day, the result seems pretty similar.

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.

FWIW, Personally, I dislike the idea of using brackets as a delimiter. That seems confusing.

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.

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.

In T202213#4512564, @Framawiki wrote:

I suspect https://www.mediawiki.org/wiki/Extension:CLDR to be the extension that adds these links.

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)

In T177765#4571856, @phuedx wrote:

@Bawolff: How's this going? Sorry if I've missed an update elsewhere.

Xml::encodeJsCall() escapes its $args as JSON, not HTML directly, and outputs a snippet of JavaScript code. I don't know if "escapes HTML" is all that accurate;

The bug is public now.

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).

In T203583#4564200, @cscott wrote:

@Tgr agreed, but I was assuming @Bawolff's issue was just a risk, not an actual vulnerability; I was just putting forward a strawman guess at what he was thinking. Maybe @Bawolff can create a new issue for the privacy risk (security-tagged if it's a vulnerability), and we can discuss whether or how we might mitigate them or deprecate the REVISION* magic words independent of this particular MCR regression.

In T203435#4568889, @MarcoAurelio wrote:

Thanks @Dalba.

@Aklapper Can you make this task visible, now that it is resolved?

Thanks.

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)

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]

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...

In T203583#4563922, @stjn wrote:

In T203583#4563858, @Bawolff wrote:

As an aside its too bad we cant kill this behaviour. Its a privacy risk.

In preview or on save or both? I doubt that removal of this behaviour in previews would’ve been noticed, but most people would expect and find useful to have this feature working when saving.

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?)

In T167762#4559280, @gerritbot wrote:

Change 458165 had a related patch set uploaded (by Amire80; owner: Amire80):
[mediawiki/core@master] WIP Move exif messages to a separate i18n file

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

There are still quite a few false positives to sort out, but its starting to get more manageable

In T203490#4558237, @Jdlrobson wrote:

Immediate issue resolved. The bug remains.
Thanks for the pointer @Bawolff !

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.

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.

Mostly: @kaldari is correct in T202181#4511685 . Note that the RL svg handling is a separate subsystem (which is a bit confusing) that is not used for user svgs.

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.

In T203179#4558234, @Legoktm wrote:

Great :) And the special page TTL was fixed in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/EUCopyrightCampaign/+/457097 to be 24h.

In T135963#4560812, @daniel wrote:

There seems to be quite a bit of activity on this, but the RFC is still tagged as "under discussion". As far as I know, some parts of the RFC were approved, while other where still being discussed. What's the status here? What remains to be done? What are the plans? Is this ready for a final discussion?

Ok. So I made 2 patches, provided that those are merged. This is approved subject to the following:

In T203095#4558248, @Slaporte wrote:

@Bawolff Thanks for working on this. For the purpose of coordinating our announcement, what is the estimated time this will be done tomorrow morning?