Using https://en.wikipedia.org/wiki/Special:AbuseFilter/examine/1096397467:

• Logged in (non-oversight) users can view the old_wikitext/new_wikitext with the oversighted material.

• Logged out users cannot view the old_wikitext/new_wikitext but can view the summary, which in this case was oversighted.

I wrote the patch for this.

Thanks @Vituzzu for adding me. I received this task via mail so I already knew what this was about. However, as I understood it, it's invalid. There are two types of /examine interface, one for AbuseLog entries and one for RC. Oversighting the AbuseLog makes the former invisible to non-oversighters, and I just tested it on my wiki and BC to be sure. As for the second: it can only be accessed via search form on Special:AbuseFilter/examine, but it's perfectly normal that it remains visible: suppressing AbuseLog doesn't imply suppressing the related edit, and examinating RC uses as visibility the one for RC. So, if you want to hide /examine in this case, you have to oversight the actual edit. Again, to me this works as expected but I'd like to hear confirm before closing the task as invalid. Also, BTW, the patch above doesn't work (if I correctly understood the problem here).

@Daimona, if you view the linked edits, you see that the actual edits and summaries are suppressed, but the contents of those edit summaries are visible via "examine" even if logged out.

In T207085#4672168, @Daimona wrote:

Thanks @Vituzzu for adding me. I received this task via mail so I already knew what this was about. However, as I understood it, it's invalid. There are two types of /examine interface, one for AbuseLog entries and one for RC. Oversighting the AbuseLog makes the former invisible to non-oversighters, and I just tested it on my wiki and BC to be sure. As for the second: it can only be accessed via search form on Special:AbuseFilter/examine, but it's perfectly normal that it remains visible: suppressing AbuseLog doesn't imply suppressing the related edit, and examinating RC uses as visibility the one for RC. So, if you want to hide /examine in this case, you have to oversight the actual edit. Again, to me this works as expected but I'd like to hear confirm before closing the task as invalid. Also, BTW, the patch above doesn't work (if I correctly understood the problem here).

For clarity, you're saying that the fact https://en.wikipedia.org/wiki/Special:AbuseFilter/examine/1096397467 's summary is visible to logged-out users means everything is "working as expected"?

@lfaraone Thanks, now I got it, but the title and description of the task were totally wrong. As for the patch above, now it makes sense and it does work. However, we should also add some notice to /examine, next to the "examine" link (like we currently do for suppressed AbuseLog entries). I'll try to see if I can improve the patch myself now.

In T207085#4672174, @Samtar wrote:

In T207085#4672168, @Daimona wrote:

Thanks @Vituzzu for adding me. I received this task via mail so I already knew what this was about. However, as I understood it, it's invalid. There are two types of /examine interface, one for AbuseLog entries and one for RC. Oversighting the AbuseLog makes the former invisible to non-oversighters, and I just tested it on my wiki and BC to be sure. As for the second: it can only be accessed via search form on Special:AbuseFilter/examine, but it's perfectly normal that it remains visible: suppressing AbuseLog doesn't imply suppressing the related edit, and examinating RC uses as visibility the one for RC. So, if you want to hide /examine in this case, you have to oversight the actual edit. Again, to me this works as expected but I'd like to hear confirm before closing the task as invalid. Also, BTW, the patch above doesn't work (if I correctly understood the problem here).

For clarity, you're saying that the fact https://en.wikipedia.org/wiki/Special:AbuseFilter/examine/1096397467 's summary is visible to logged-out users means everything is "working as expected"?

No, I'm saying that the description of this task reports a problem, but the actual task is about a totally different one.

No, I'm saying that the description of this task reports a problem, but the actual task is about a totally different one.

Certainly didn't read as such - if it'd help, I can reword the task description.
Hopefully it goes without saying (but I'll say it, lest the task get closed as invalid) that this bug is exposing non-public personal information to all users - a breach of the privacy policy - and should be taken seriously.

In T207085#4672183, @Samtar wrote:

No, I'm saying that the description of this task reports a problem, but the actual task is about a totally different one.

Certainly didn't read as such - if it'd help, I can reword the task description.
Hopefully it goes without saying (but I'll say it, lest the task get closed as invalid) that this bug is exposing non-public personal information to all users - a breach of the privacy policy - and should be taken seriously.

It definitely is. I just didn't understand it at first. Thanks for rewording the description, I'm about to submitted an amended patch.

This is my patch, where the examine part is copied from Rxy's patch (and slightly beautified). Also, please note that this vulnerability also affects Special:AbuseFilter/test (fixed by the change in AbuseFilterChangesList)

(file was updated, it wouldn't have passed CI, and that's the last thing we need).

Also, we should probably make a better distinction between deleted and suppressed stuff, but it's fine to merge this for the moment.

I like Daimona's patch better. I am going to test it momentarily; assuming that it checks out, what is our approach here? Do we have Daimona post it publicly, have me merge it within minutes, have Daimona create a cherry pick for WMF current build, have someone SWAT it ASAP, or do we try to minimize the amount of time things are public in some other way (e.g. I submit and merge the patch myself, or even, we have someone who can SWAT it submit and merge the patch in both places, based on my verbal approval here?)

@Daimona: your patch checks out. But I also noticed a related issue:

• User A suppresses an edit that triggered a filter
• User B (which is a sysop and not an oversight) goes to Special:AbuseLog/123 to view that log
• User B sees the text of edit and its summary, even though they were suppressed!

Do we want to address this at the same time?

Please pardon me if I'm being naïve, but isn't the underlying issue that the AbuseLog doesn't follow revdel actions? In a perfect world, wouldn't AbuseLog entries/examinations of successful actions respect the revdel/oversight status of that action? This need not wait for that, but am I wrong in thinking that's the ideal long-term solution?

In T207085#4672476, @Huji wrote:

I am going to test it momentarily; assuming that it checks out, what is our approach here? Do we have Daimona post it publicly, have me merge it within minutes, have Daimona create a cherry pick for WMF current build, have someone SWAT it ASAP, or do we try to minimize the amount of time things are public in some other way (e.g. I submit and merge the patch myself, or even, we have someone who can SWAT it submit and merge the patch in both places, based on my verbal approval here?)

A member from the security team will also take a look and secretly deploy it once ready, verify that it works, and then make it public shortly afterwards.

In T207085#4672220, @Daimona wrote:

This is my patch, where the examine part is copied from Rxy's patch (and slightly beautified). Also, please note that this vulnerability also affects Special:AbuseFilter/test (fixed by the change in AbuseFilterChangesList)

T207085.patch1 KBDownload

(file was updated, it wouldn't have passed CI, and that's the last thing we need).

Also, we should probably make a better distinction between deleted and suppressed stuff, but it's fine to merge this for the moment.

Thanks for improve my patch :)

In T207085#4672760, @Huji wrote:

@Daimona: your patch checks out. But I also noticed a related issue:

• User A suppresses an edit that triggered a filter
• User B (which is a sysop and not an oversight) goes to Special:AbuseLog/123 to view that log
• User B sees the text of edit and its summary, even though they were suppressed!

Do we want to address this at the same time?

That is related with "SpecialAbuseLog::isHidden".

rEABF /includes/special/SpecialAbuseLog.php ( L1106-1120 , L545)

These should be use "Revision::userCan" or "Revision::userCanBitfield".

@Huji I guess the last option is the best one (someone who can deploy will auto-merge and immediately deploy). As for the mentioned problem yes, we should fix it now; the only thing we can (shortly) delay is separately handle deleted and suppressed entries (in SpecialAbuseLog::isHidden and in the added lines in AbuseFilterChangesList, where we check for visibility != 0). @Rxy (or anyone else) of course feel free to amend my patch to solve it: I probably won't have time to do it today. @Amorymeltzer yes, indeed, but this is not related to the suppression of AbuseLog entries, as the original description was about.

PS I just realized that the problem reported by Huji is related to my comment about separatig deleted and suppressed revisions. Correctly handling them now would solve it as a bonus, so if someone has the time for it, I'd suggest to do it now (it shouldn't be hard).

This is an updated version which should also solve the discrepancy between deleted and suppressed revs. An important note: I haven't tested it! It should be tested in several cases: visible, deleted or suppressed revision, and basic/sysop/oversighter user.

In T207085#4673205, @Daimona wrote:

This is an updated version which should also solve the discrepancy between deleted and suppressed revs. An important note: I haven't tested it! It should be tested in several cases: visible, deleted or suppressed revision, and basic/sysop/oversighter user.

T207085.patch4 KBDownload

CodeReview: -1

Thanks for update the patch. but I get error message in my wiki :

PHP Parse error:  syntax error, unexpected ';' in /w/extensions/AbuseFilter/includes/Views/AbuseFilterViewExamine.php on line 169


$ docker run -it -v "$PWD:/opt" --rm php:7.0 php -l
 /opt/includes/Views/AbuseFilterViewExamine.php

Parse error: syntax error, unexpected ';' in /opt/includes/Views/AbuseFilterViewExamine.php on line 169
Errors parsing /opt/includes/Views/AbuseFilterViewExamine.php


$ docker run -it -v "$PWD:/opt" --rm php:7.1 php -l
 /opt/includes/Views/AbuseFilterViewExamine.php

Parse error: syntax error, unexpected ';' in /opt/includes/Views/AbuseFilterViewExamine.php on line 169
Errors parsing /opt/includes/Views/AbuseFilterViewExamine.php


$ docker run -it -v "$PWD:/opt" --rm php:7.2 php -l /opt/includes/Views/AbuseFilterViewExamine.php

Parse error: syntax error, unexpected ';' in /opt/includes/Views/AbuseFilterViewExamine.php on line 169
Errors parsing /opt/includes/Views/AbuseFilterViewExamine.php

I tried L168
if ( is_int( SpecialAbuseLog::isHidden( $row ) ) {

to

if ( is_int( SpecialAbuseLog::isHidden( $row ) ) ) {
;

I tested that but not work correctly due to $rev->userCan first param is should be provide Revision::SUPPRESSED_ALL instead of "SpecialAbuseLog::isHidden".

because Revision::userCan is already called $this->getVisibility() in rMW /includes/Revision.php $1199

@Rxy thanks. I'm trying to include every possible case in my patch, but I don't really know how to use all of these deletion constants, so I'm going very slowly.

Also, I'm trying to fulfill the following conditions:

1. Make it possible to use /examine according to revision visibility (the main point of the task)
2. Do the same not only for edits, but also for log entries!
3. Show the examine link if and only if the user can examine the given row
4. If a revision is hidden but the user can see it, don't hide its elements (only leave them striked)
5. Ensure that the fix is applied to APIs as well

For 3, we need to conditionally execute the return added in insertExtra, checking if the user can see the row. For 4, we need to add in AbuseFilterChangesList the methods insertUserRelatedLinks and insertComment, and add a case for when the revision is hidden but the user can see it. For 2, we surely need to add a log-specific elseif next to the added lines in ViewExamine. Also, SpecialAbuseLog::isHidden can keep returning 'implicit' for deleted revisions.

I still need an answer as to how we merge a patch like this.

I'd hope https://wikitech.wikimedia.org/wiki/How_to_deploy_code#Security_patches provides some hints?

@Aklapper great direction, thank you!

I'm leaving here a note, which I'll investigate later today, and to also hear some thoughts. Maybe in ChangesList deleted/suppressed edits shouldn't be shown at all if the user can't see them (instead of showing a censored and greyed line): I think "normal" user may understand the performer and/or the target of the action even if it's hidden, by properly filtering on Special:AbuseFilter/examine and Special:AbuseFilter/test.

UPDATE: AFAICS, we don't do this for other RC lists, for instance the watchlist. So maybe we can keep them, huh?

In T207085#4674738, @Daimona wrote:

T207085-full.patch8 KBDownload

New version. Tested in all cases (anonymous/sysop/oversight user vs visible/deleted/oversighted element), and it seems to work.

Thanks for working this. Almost ok for me. but why you pass $entry->getDeleted() in F26619762$123 ?

And I noticed added permission checks are redundant with ChangesList::userCan ( rMW / includes/changes/ChangesList.php$633-639 )

I submit Patch Set 5 in here:

@Rxy thanks for your fixes. I consequently amended my patch, which is below. Again, I tested every possible case, even more than I originally did (visible/deleted(a single field or all fields)/suppressed(a single field or all fields) edit/log with an anonymous/admin/oversighter account) and it should be fine. It also passes PHPCS.

What’s the difference between the two patches?

Which one do we want to proceed with?

@Reedy There's no difference, mine includes Rxy's one: Rxy submitted the first one, I checked it out and added other things. Then Rxy amended a point in my patch in his own version; then I did the same fix in my own file, plus a couple other fixes and re-published it as the last version above. So, unless I missed something, my last patch includes all we need.

Phab shows 1KB difference ;)

@Reedy yeah, because the last Rxy's patch is just an amending of mine, and a proper subset of my last patch :-)

CR-1, there's a missing return in here. I guess this path wasn't tested?

+	/**
+	 * Insert a formatted comment. Like the parent, but don't hide details if user can see them.
+	 * @param RecentChange $rc
+	 * @return string
+	 */
+	public function insertComment( $rc ) {
+		if ( $this->isDeleted( $rc, Revision::DELETED_COMMENT ) ) {
+			if ( $this->userCan( $rc, Revision::DELETED_COMMENT ) ) {
+				' <span class="history-deleted">' .
+					Linker::commentBlock( $rc->mAttribs['rc_comment'], $rc->getTitle() ) . '</span>';
+			} else {
+				return ' <span class="history-deleted">' .
+					$this->msg( 'rev-deleted-comment' )->escaped() . '</span>';
+			}
+		} else {
+			return Linker::commentBlock( $rc->mAttribs['rc_comment'], $rc->getTitle() );
+		}
+	}

And this just adds complexity. There's a return in the if block above, so the else isn't needed. Just use an if (ie don't change this code)

 			return true;
-		}
-		if ( $row->afl_rev_id ) {
+		} elseif ( $row->afl_rev_id ) {
 			$revision = Revision::newFromId( $row->afl_rev_id );

In T207085#4676096, @Huji wrote:

I still need an answer as to how we merge a patch like this.

T207085#4672999 ...

In T207085#4677957, @Reedy wrote:

CR-1, there's a missing return in here. I guess this path wasn't tested?

It actually was tested, but I probably didn't notice missing comments. Or, actually, I noticed them but unconsciously thought they were fine like that. Either way, done.

And this just adds complexity. There's a return in the if block above, so the else isn't needed. Just use an if (ie don't change this code)

Yes, it was just for clarity: since there's a return in the first if, changing the second to an elseif helps understand at a glance that the two ifs cannot be executed together. Removed anyway.

@Reedy Thanks for deploy.

@Daimona Thanks for submit the patch

@Reedy Some more testing wouldn't be bad, if we want to do it. Also, do we want to backport to 1.32?

In T207085#4679481, @Daimona wrote:

@Reedy Some more testing wouldn't be bad, if we want to do it. Also, do we want to backport to 1.32?

It should be backported to all applicable and supported release branches

AbuseFilter isn't a bundled extension, so when it is in gerrit/hits master, this can be done in the open too

@Reedy Alright, thanks, I can push to gerrit once we're fine with it. The backports should be on wmf/1.32.0-wmf.26, REL1_27, REL1_30, REL1_31 and REL1_32; if we want to be fast, I can auto-merge there, but I guess it won't be necessary. I'll do some testing later today, if someone wants to coordinate testing to cover all possible cases you'll find me on IRC as "Daimona".

Is everyone fine with this pushed on gerrit? Ping @Reedy, @Rxy if no oppositions are raised I'll push the patch tomorrow or so.

Pushed to gerrit, for all mentioned branches. I only merged the one on master, since I'd like someone else to double-check backports to be sure that everything is fine, CC @Reedy @Legoktm. Please note that I had to manually +2verify the one on REL1_31 due to T189560. Also: can this task be made public once everything will be merged?

Not once merged, but once deployed.

@Huji the fix is already deployed to WMF wikis per T207085#4679131. There's still this patch to be merged on wmf.26 (could you please merge it?), but it doesn't really matters, it's just for completeness' sake.

I can only +1 it, as my +2 rights don't expand into the WMF branches.
Great work by the way!

Ah, I didn't know. If someone with merge access on wmf. branches will come across the patch, that'd be nice! Otherwise, we can just abandon it as the new version is already on group0 and the fix is already deployed on other wikis.

@Daimona Thanks again for dealing with this. Am I correct that this doesn't apply on edits that have since been deleted? This is probably closer to T44734, but as far as I can tell, if a page is deleted, and if one of its now-deleted edits is then suppressed, if that edit had initially trigged an abusefilter, that entry it still needs manual hiding.

@Amorymeltzer Hmm I don't think it's intended, probably it's just some edge-case, possibly due to the fact that the page doesn't exist (maybe we handle non-existing pages differently, or Revision class does so). I'll investigate tomorrow anyway. Could you please confirm that the reproduction steps below are correct?
*Create a page with some revisions (and trigger a filter with one of these)
*Delete the page
*Suppress the revision (already deleted) which triggered the AF
*Go to Special:AbuseLog for that same revision, which will only check for "deletedtext" right (i.e. it's visible to sysops (who aren't oversighters) but not to "normal" users)
Also, I'm unsure if that task is related... The reason could be similar, but in that case solving the problem isn't trivial at all.

@Daimona Correct, although to your last point, after deleting the page oversighting the deleted revision(s), the AF info was visible to a non-sysop.

@Amorymeltzer I checked, and I confirm that it's a duplicate of T44734. Also, please note that this only happens in Special:AbuseLog, and not in Special:AbuseFilter/test or /examine (as it did for the problem originally reported in this task). And when it comes to specific AbuseLog entries, suppressing them is the right way, at least while waiting for a fix to T44734 (which won't be easy).