That's infoleak

I can't see the watchers when I just tested, and I'm a flagged revs reviewed on enwiki. Dupe of {T234736}?

@DannyS712 when viewing https://en.wikipedia.org/wiki/Special:PendingChanges I see this

According to https://en.wikipedia.org/wiki/Special:UserRights/Asartea you are a member of "Pending changes reviewers" (reviewer user group).

This group has the unreviewedpages permission (https://phabricator.wikimedia.org/diffusion/EFLR/browse/master/extension.json$108). So, you should be able to see this.

However, https://en.wikipedia.org/wiki/Special:ListGroupRights does not list that permission for that group. This seems like a bug.

In T281065#7035610, @matmarex wrote:

According to https://en.wikipedia.org/wiki/Special:UserRights/Asartea you are a member of "Pending changes reviewers" (reviewer user group).

This group has the unreviewedpages permission (https://phabricator.wikimedia.org/diffusion/EFLR/browse/master/extension.json$108). So, you should be able to see this.

However, https://en.wikipedia.org/wiki/Special:ListGroupRights does not list that permission for that group. This seems like a bug.

unreviewedpages doesn't allow seeing the number of watchers, just the overall special page, there is a separate permission check for the number of watchers - https://gerrit.wikimedia.org/g/mediawiki/extensions/FlaggedRevs/+/b4ecaf215a497dfcd7e9d33d52a550f754ef2f77/frontend/specialpages/reports/UnreviewedPages.php#163

So, did some more digging
Relevant things:

• Special:UnreviewedPages
• Special:PendingChanges
• $wgFlaggedRevsProtection
• the unreviewedpages permission
• the unwatchedpages

Special:UnreviewedPages requires unreviewedpages to view, and checks for unwatchedpages to display the number of watchers.[1] This was what I was mistakenly looking at when I posted my comment T281065#7066461. When $wgFlaggedRevsProtection is enabled, as it is on enwiki, this special page is not enabled[2].

When $wgFlaggedRevsProtection is enabled, the unreviewedpages right is removed from reviewer and editor groups[3], accounting for the perceived issue with Special:ListGroupRights discussed above.

Special:PendingChanges, which is enabled, does not require any rights to view, and checks for unreviewedpages to decide whether to display the number of watchers. In theory, only administrators should have this right at this point, but I suspect the issue is that the conditional removal of this right from the reviewer group was sometimes failing, like the other tickets about flagged revs user rights. I propose the following fix:

1. switch Special:PendingChanges to check for unwatchedpages to decide to display the number of watchers, because that is the right that is needed elsewhere
2. later, also remove unreviewedpages from administrators when the special page is disabled, like from reviewer and editor, because after step 1 the right isn't also used for something else anymore

I believe that since a) this is fairly hard to exploit, even more so now that the flagged revs rights handling is believed to have been fixed and b) the fixes are fairly sane even without knowing the security issue, that a patch can be done through gerrit - is it okay to send a patch for step 1 publicly?

On the other hand, this might interfere with the workflows on some wikis if they are expecting Special:PendingChanges to show information about watchers to reviewers, since it might be a higher priority to investigate pending changes to pages without watchers.

(also, since there is now a lot discussing and debugging on this task, I propose T234736 be closed as a duplicate of this)

[1] https://gerrit.wikimedia.org/g/mediawiki/extensions/FlaggedRevs/+/b4ecaf215a497dfcd7e9d33d52a550f754ef2f77/frontend/specialpages/reports/UnreviewedPages.php#163
[2] https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/FlaggedRevs/+/b4ecaf215a497dfcd7e9d33d52a550f754ef2f77/frontend/FlaggedRevsUIHooks.php#1329
[3] https://gerrit.wikimedia.org/g/mediawiki/extensions/FlaggedRevs/+/b4ecaf215a497dfcd7e9d33d52a550f754ef2f77/FlaggedRevsSetup.php#156

@DannyS712 -

In T281065#7066588, @DannyS712 wrote:

So, did some more digging

Thanks! And sorry for the delayed response here.

I propose the following fix:

1. switch Special:PendingChanges to check for unwatchedpages to decide to display the number of watchers, because that is the right that is needed elsewhere
2. later, also remove unreviewedpages from administrators when the special page is disabled, like from reviewer and editor, because after step 1 the right isn't also used for something else anymore

This is all a confusing mess of rights and workflows IMO, but this does sound correct to me just thinking it through for a bit. I mean, assuming we don't want to introduce a more specific right for this action.

I believe that since a) this is fairly hard to exploit, even more so now that the flagged revs rights handling is believed to have been fixed and b) the fixes are fairly sane even without knowing the security issue, that a patch can be done through gerrit - is it okay to send a patch for step 1 publicly?

I'd rate this as low risk to be pushed through gerrit, especially if timed well with the train.

On the other hand, this might interfere with the workflows on some wikis if they are expecting Special:PendingChanges to show information about watchers to reviewers, since it might be a higher priority to investigate pending changes to pages without watchers.

Do you have any sense of how many workflows like this might exist?

(also, since there is now a lot discussing and debugging on this task, I propose T234736 be closed as a duplicate of this)

I'll do this now. Did you have a chance to verify @Tgr's speculation at T234736#7063712?

In T281065#7083471, @sbassett wrote:

@DannyS712 -

I propose the following fix:

1. switch Special:PendingChanges to check for unwatchedpages to decide to display the number of watchers, because that is the right that is needed elsewhere
2. later, also remove unreviewedpages from administrators when the special page is disabled, like from reviewer and editor, because after step 1 the right isn't also used for something else anymore

This is all a confusing mess of rights and workflows IMO, but this does sound correct to me just thinking it through for a bit. I mean, assuming we don't want to introduce a more specific right for this action.

I believe that since a) this is fairly hard to exploit, even more so now that the flagged revs rights handling is believed to have been fixed and b) the fixes are fairly sane even without knowing the security issue, that a patch can be done through gerrit - is it okay to send a patch for step 1 publicly?

I'd rate this as low risk to be pushed through gerrit, especially if timed well with the train.

I'll send a patch for part 1 on gerrit later this week, so that its public for less time before the train. Is there anyone that wants to be added as a reviewer so they see it / to make sure it gets merged before the branch cut?

On the other hand, this might interfere with the workflows on some wikis if they are expecting Special:PendingChanges to show information about watchers to reviewers, since it might be a higher priority to investigate pending changes to pages without watchers.

Do you have any sense of how many workflows like this might exist?

No idea. I just realized when I was scrolling through enwikibooks pending changes and realized that after this change, while I would still be able to see which pages are unwatched as a GS, standard reviewers wouldn't. Specifically, I would guess it would be the biggest issue for wikis where the pending changes queue is usually really full because of how the pending changes are set up - eg enwiki the default state I guess is that there are no pending changes, and when there are new edits they are reviewed quickly, so it doesn't really matter if there are unwatched pages that are edited, while for enwikibooks where there are lots of changes since basically every page has pending changes enabled, it would be more significant to be able to see which pages are unwatched. Perhaps (after this is done) include a note in tech news? I wrote a draft below

(also, since there is now a lot discussing and debugging on this task, I propose T234736 be closed as a duplicate of this)

I'll do this now. Did you have a chance to verify @Tgr's speculation at T234736#7063712?

Since the issue was happening randomly, I would have assumed that not being able to reproduce was just a part of the bug itself, but since I can no longer reproduce at all I guess @Tgr was correct about the reasoning. Either way though, there is still the issue that wikis that assign unreviewedpages rights are allowing users to see unwatched pages, likely unintentionally.

Draft for tech news, for *after* this is resolved and pending approval from the security team for disclosing it so publically (since its a config change thats going to impact WMF wikis, but also a security misconfiguration) (not tagging user notice to make sure it doesn't get added accidentally)

Previously, on wikis that had the flagged revisions extension enabled, users that had the unreviewedpages right were able to see whether a page had no watchers in the Special:PendingChanges interface. This was believed to be a mistake, since elsewhere information about unwatched pages is only available to users with the unwatchedpages right - Special:PendingChanges now checks for that right instead (assigned to administrators by default). If your wiki was relying on pending changes reviewers being able to see unwatched pages in the interface, you can request that they be granted the unwatchedpages right if there is community support, using the same process as other configuration changes outlined at https://meta.wikimedia.org/wiki/Special:MyLanguage/Requesting_wiki_configuration_changes.

I assume such a config change wouldn't need legal or T&S approval because the information from unwatchedpages is so minimal?

Change 693262 had a related patch set uploaded (by DannyS712; author: DannyS712):

[mediawiki/extensions/FlaggedRevs@master] PendingChanges: require `unwatchedpages` to show watcher info

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

In T281065#7083754, @DannyS712 wrote:

In T281065#7083471, @sbassett wrote:

I believe that since a) this is fairly hard to exploit, even more so now that the flagged revs rights handling is believed to have been fixed and b) the fixes are fairly sane even without knowing the security issue, that a patch can be done through gerrit - is it okay to send a patch for step 1 publicly?

I'd rate this as low risk to be pushed through gerrit, especially if timed well with the train.

I'll send a patch for part 1 on gerrit later this week, so that its public for less time before the train. Is there anyone that wants to be added as a reviewer so they see it / to make sure it gets merged before the branch cut?

Sorry, completely forgot about this last week. Sent https://gerrit.wikimedia.org/r/c/mediawiki/extensions/FlaggedRevs/+/693262 to change the right checked on Special:PendingChanges, hopefully can be merged before train cut on monday

Change 693262 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] PendingChanges: require `unwatchedpages` to show watcher info

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

I think this has been resolved back in May last year? Can we close it and make it public?

In T281065#7762318, @matmarex wrote:

I think this has been resolved back in May last year? Can we close it and make it public?

Yes.