Privacy Engineering is triaging this.

Noting here that there are some services and tools which rely upon mediawiki.revision-visibility-change, see recent example: T342599.

Removing the actor that performed the suppression seems fine, but please retain information about the target (page_title, page_namespace, page_id, etc.). This event stream is the only means we have for clients to remove information copied from the wiki that should no longer be public.

The page/revision information affected is publicly visible through page histories, so indeed that should stay.

As far as I can tell it's only the performer for suppressions which shouldn't be publicly available (performer for revdeletes should also remain)

^^agree with the above analysis — if we can selectively remove the performer of suppressions, then this should be considered resolved.

I'm adding the Data-Platform-SRE tag, because it will likely be our team performing the deployment of the updated views, once a decision has been made and a patch agreed upon.

@BTullis this is for the event stream, not the DB replicas.

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/EventBus/+/963324

@stwalkerster: in trying to make this change, I just want to get clarity on the nomenclature here, since "suppressed" and "deleted" are overloaded and inconsistently used.

In the description and T342487#9041988, you make a distinction between these

This information *is* available for revision deletions, and as such it *should* be shown when a standard revision deletion is performed. It should not be shown when a suppression is performed.

performer for revdeletes should also remain

So this is what I understood, in technical terms:

rev_deleted bits 1, 2, and/or 4 set indicate a "standard revision deletion"
rev_deleted bits 8 (or 12) set indicate a "suppression"

Is that correct?

^ would mean that 'suppression' == only if not explicitly not viewable for regular admins? E.g. if content is hidden, but is still visible to regular admins, it would be okay to expose the performer?

Okay, please tell me if this is correct:

if ( $rev->getVisibility() & RevisionRecord::DELETED_RESTRICTED )
   // exclude performer info
else 
   // include performer info

Okay, yes, I think I understand now. If an admin clicks the special 'Suppress data from administrators as well as others' box when changing visibility, then the performer should not be shown. This is true for both revision visibility change events, as well as page deletes, which also have this option.

Latest patch in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/EventBus/+/963324 does this.

@Ottomata Thanks! A good line of thinking is whether or not something is logged in the suppression log ( [[Special:Log/suppress]] ). If it is, it should be treated as private.

I'm not sure if it's an issue or not within EventStreams (probably worth verifying), but there's also suppression capabilities within the AbuseFilter log and also when blocking registered users. Both of those actions land in the suppression log already.

there's also suppression capabilities within the AbuseFilter log and also when blocking registered users

We don't have any event streams about AbuseFilters.

We do have events about user-blocks-changes, but they are not exposed externally. I think those are probably private in general? But, even so, all of our streams are publicly exposable for the most part so far, so we should have the same behavior in our internal stream (in case we expose it one day?). I'll try to implement that too.

In T342487#9225959, @Ottomata wrote:

Okay, please tell me if this is correct:

if ( $rev->getVisibility() & RevisionRecord::DELETED_RESTRICTED )
   // exclude performer info
else 
   // include performer info

This taps directly into the internal flag representing the "Suppress data from administrators" checkbox. I would caution against that as this duplicates logic from MediaWiki core, and may be making some usually-correct-but-technically-different assumptions about how revision delete works.

When deleting a revision, there are several options which translate to a field in the rev_deleted bitfield. Each has a public constant, such as:

• DELETED_TEXT
• DELETED_COMMENT
• DELETED_USER

Additionally, there is the option to "Suppress data even from administrators" in which case DELETED_RESTRICTED is added as well. However, this by no means implies that a specific part of the revision is deleted. It could be supressing the text, the comment, the editor's username, or some combinatin of the two.

When a revision is deleted, or its visibility is subsequently changed further, we record this administrator action in the logging table, publicly listed at Special:Log/delete?subtype=revision. This is where the performer (log_user), reason (log_comment), and time of action are recorded.

If the "Supress" option is enabled, this event doesn't go the public deletion log, and instead goes to the (itself restricted) log at Special:Log/supress which is only visible to administrators. That is to say, only administrators can see who/when/why a given "Supress" action was performed. As the checkbox label says, the wiki page revision metadata is of course supressed "even for administrators" and separate from the visibility of the log event.

When the action is completed, in addition to having been recorded in the logging table (Special:Log), public actions (i.e. not "suppressed") are also naturally emitted as a public RecentChanges event which EventBus already consumes and forwards to EventStreams. I wonder if the same problem exists there? If not, why is there an entirely separate pipeline from this coming from a different hook in core rather than build on the more well-known recent changes feed?

In T342487#9221078, @Ottomata wrote:

@BTullis this is for the event stream, not the DB replicas.

Oops. Apologies for misinterpreting the description and misjudging the priority.

Thanks @Krinkle!

this by no means implies that a specific part of the revision is deleted

This is how the revision-visibility-change stream, as well as the new mediawiki.page_change.v1 stream works. They only redact those parts of the revision in the event, and indicate which parts should be deleted by consumers of the streams.

IIUC, we just need to make sure that if DELETED_RESTRICTED is set, that the info performer of the restriction is not exposed in the stream. WE still need to send the info about which parts of the revision were 'deleted', so that downstream consumers (enterprise, dumps 2.0, WikiWho (T342599), etc.) can react and delete from their datastores too.

why is there an entirely separate pipeline from this coming from a different hook in core rather than build on the more well-known recent changes feed

The recentchange stream does not have a deterministic schema, making it difficult to use for typed and SQL based systems.

@stwalkerster

there's also suppression capabilities [...] when blocking registered users

Okay, in this case, as far as I can tell, this is about hiding the target blocked user's username, right? Not the performer user's info?

I believe we do expose this info when we shouldn't. Opening a new bug.

{T348252}

In T342487#9228277, @Ottomata wrote:

Okay, in this case, as far as I can tell, this is about hiding the target blocked user's username, right? Not the performer user's info?

Both, ideally, should be hidden.

Both, ideally, should be hidden.

Hm, afaict there isn't the same concept when a user name is hidden. The admin can hide the username, but not suppress the fact that the user's name is being hidden (in the log), right?

So, are you saying that, if an admin wants to hide a user's name, then their own user info should be hidden in the log as well? Is that what happens in the log? The log goes to the secret log instead of the public log?

if an admin wants to hide a user's name, then their own user info should be hidden in the log as well? Is that what happens in the log? The log goes to the secret log instead of the public log?

Yes, blocks with the suppression flag set are sent to the private suppression log, not the public block log.

Just so I'm sure we understand each other:

blocks with the suppression flag

By 'suppression flag', you mean when the 'Hide username from edits and lists' box is checked when creating a block for a user, right? I.e. if DatabaseBlock getHideName() returns true?

By 'suppression flag', you mean when the 'Hide username from edits and lists' box is checked when creating a block for a user

Correct - on enwiki we've renamed the checkbox to "Suppress username from edits and lists" just to confuse matters.

This has now landed in master; does it need back-porting to production?

It can go out with the train.

@stwalkerster I don't think I have perms in any production wikis to verify that this fix is working. Could you do your edit suppression with your sandbox again and to verify?

Thanks!

I performed a suppression and an unsuppression. These are the entries from the private suppression log:

• 2023-10-30T18:00:43 Stwalkerster talk contribs block secretly changed visibility of a revision on page User:Stwalkerster/sandbox: content unhidden and removed restrictions for administrators (T342487) (diff | more...)
• 2023-10-30T18:00:27 Stwalkerster talk contribs block secretly changed visibility of a revision on page User:Stwalkerster/sandbox: content hidden and applied restrictions to administrators (T342487) (diff | more...)

Suppression looks fine, as it doesn't reveal it was me performing the suppression

event: message
id: [{"topic":"eqiad.mediawiki.revision-visibility-change","partition":0,"offset":-1},{"topic":"codfw.mediawiki.revision-visibility-change","partition":0,"timestamp":1698688827001}]
data: {"$schema":"/mediawiki/revision/visibility-change/1.0.0","meta":{"uri":"https://en.wikipedia.org/wiki/User:Stwalkerster/sandbox","request_id":"ff197a33-7b11-4ffb-82e2-f80a347b4493","id":"e1d41305-3443-47b7-bf8f-0f5db5bc2fb8","dt":"2023-10-30T18:00:27Z","domain":"en.wikipedia.org","stream":"mediawiki.revision-visibility-change","topic":"codfw.mediawiki.revision-visibility-change","partition":0,"offset":374694},"database":"enwiki","page_id":59627106,"page_title":"User:Stwalkerster/sandbox","page_namespace":2,"rev_id":1175405362,"rev_timestamp":"2023-09-14T20:22:21Z","rev_sha1":"14n12z2r5ebejrqamp9mflvw8vb1hwn","rev_minor_edit":false,"rev_len":1291,"rev_content_model":"wikitext","rev_content_format":"text/x-wiki","page_is_redirect":false,"comment":"added [[Category:X9]] using [[WP:HC|HotCat]]","parsedcomment":"added <a href=\"/wiki/Category:X9\" title=\"Category:X9\">Category:X9</a> using <a href=\"/wiki/Wikipedia:HC\" class=\"mw-redirect\" title=\"Wikipedia:HC\">HotCat</a>","rev_parent_id":1170712760,"visibility":{"text":false,"user":true,"comment":true},"prior_state":{"visibility":{"text":true,"user":true,"comment":true}}}

Unsuppression, however, looks like it's leaking the performer:

event: message
id: [{"topic":"eqiad.mediawiki.revision-visibility-change","partition":0,"offset":-1},{"topic":"codfw.mediawiki.revision-visibility-change","partition":0,"timestamp":1698688843001}]
data: {"$schema":"/mediawiki/revision/visibility-change/1.0.0","meta":{"uri":"https://en.wikipedia.org/wiki/User:Stwalkerster/sandbox","request_id":"1eb377d5-af01-402c-9e15-8fc6ca637d5f","id":"c58e8639-73da-46b7-8d7d-b89f0ca0ce59","dt":"2023-10-30T18:00:43Z","domain":"en.wikipedia.org","stream":"mediawiki.revision-visibility-change","topic":"codfw.mediawiki.revision-visibility-change","partition":0,"offset":374695},"database":"enwiki","page_id":59627106,"page_title":"User:Stwalkerster/sandbox","page_namespace":2,"rev_id":1175405362,"rev_timestamp":"2023-09-14T20:22:21Z","rev_sha1":"14n12z2r5ebejrqamp9mflvw8vb1hwn","rev_minor_edit":false,"rev_len":1291,"rev_content_model":"wikitext","rev_content_format":"text/x-wiki","performer":{"user_text":"Stwalkerster","user_groups":["abusefilter","checkuser","suppress","sysop","*","user","autoconfirmed"],"user_is_bot":false,"user_id":851859,"user_registration_dt":"2006-01-28T22:30:11Z","user_edit_count":22185},"page_is_redirect":false,"comment":"added [[Category:X9]] using [[WP:HC|HotCat]]","parsedcomment":"added <a href=\"/wiki/Category:X9\" title=\"Category:X9\">Category:X9</a> using <a href=\"/wiki/Wikipedia:HC\" class=\"mw-redirect\" title=\"Wikipedia:HC\">HotCat</a>","rev_parent_id":1170712760,"visibility":{"text":true,"user":true,"comment":true},"prior_state":{"visibility":{"text":false,"user":true,"comment":true}}}

I've also just realised that the original bug description didn't actually mention unsuppression, but since it's also logged to the private suppression log it should also be hidden.

Unsuppression, however, looks like it's leaking the performer:
unsuppression, but since it's also logged to the private suppression log it should also be hidden.

Interesting. So, IIUC, if either the old or new visibility settings have DELETED_RESTRICTED set, then the performer should not be shown? Def not obvious, but I suppose it makes sense?

Wait, does it make sense? The admin is removing the suppression, why shouldn't the admin's action be in the public log?

I suppose even if it doesn't make sense, we should respect what MW does.

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/EventBus/+/971310

In T342487#9303227, @Ottomata wrote:

Unsuppression, however, looks like it's leaking the performer:
unsuppression, but since it's also logged to the private suppression log it should also be hidden.

Interesting. So, IIUC, if either the old or new visibility settings have DELETED_RESTRICTED set, then the performer should not be shown? Def not obvious, but I suppose it makes sense?

The above is true, but the above also connects somewhat disparate variables and internals together to try to make sense of it, which indeed makes it seem difficult to understand.

The actual business logic is expressed in way that is both simple to understand and intuitive to end-users of RC and Special:Logs:

• Events to public logs are public. Events to restricted logs are restricted.

That is to say, most wiki actions (e.g. page create, edit patrol, page delete, account create, account block, etc.) produce public log entries. Some actions (e.g. checkuser, suppress, and a few others) are always private. This is an aspect of an entire log channel of wiki actions.

There is no concept in MediaWiki of an individual log message being private.
No individual fields of a log message being private.
No complex logic where a message is private based on inferring what other fields have as their values, etc.

Yet, this is exactly what EventBus does and thus makes it inherently likely to leak private data, and confusing to its maintainers.

A more low level perspective, still quite straight forward I think:

• $wgLogRestrictions controls which log channels are private (called by LogPage::isRestricted).
• $wgLogRestrictions['suppress'] is private.
• When an admin excercises their suppression user rights to modify the state of a revision, this emits a "suppress" entry. These are always private.
• When an admin excercises their page deletion user rights to modify the state of a revision, this emits a "delete" log entry. These are always public.

The problem, as mentioned in previous comments, is that EventBus tries to 1) explicitly share information that may be from a private event, and 2) does so from a hook that doesn't tell it whether the current event is private, and thus 3) tries to reverse-engineer and predict what is likely happening or about to happen, all while 4) following a schema that designed in a way that is incompatible with how MediaWiki actually works.

My suggestion would generally be to promote and recommend use of the recentchange topic, and strip down the revision-visibility-change topic to just the bare minimum. Do customers of this topic need to know anything other than revid + visibility bits?

Good points. The larger fix is to refactor MW state hooks to an event model so that we don't have to reason about the right thing to do. But that's a big project.

strip down the revision-visibility-change topic to just the bare minimum. Do customers of this topic need to know anything other than revid + visibility bits?

This is an interesting idea. @xcollazo what do you think? We'd probably want to ask enterprise folks too.

In T342487#9316981, @Ottomata wrote:

Good points. The larger fix is to refactor MW state hooks to an event model so that we don't have to reason about the right thing to do. But that's a big project.

strip down the revision-visibility-change topic to just the bare minimum. Do customers of this topic need to know anything other than revid + visibility bits?

This is an interesting idea. @xcollazo what do you think? We'd probably want to ask enterprise folks too.

I like this idea as well.

For Dumps 2.0, all we need is: database, rev_id, rev_timestamp, and the visibility bits. Plus what we have discussed elsewhere ( T349845 ) for when those bits get unsuppressed.

CC @Milimetric, @JEbe-WMF

@xcollazo, right, for T349845 you want things like the revision editor's user_text in the event if the user_text is being 'unhidden'. For context @Krinkle, Dumps 2.0 would need this in order to backfill the user_text on a revision without having to do an API lookup. Same goes for comment. Actually, this begs a question about how you will get content after the fact if content is unhidden...hmm. Pinged you on slack, or we can continue discussion on T349845.

@Ottomata The answer is to perform an API lookup and use the stream only as a nudge to fetch new data. Indeed, that is required either way for the content as well. These events have a very low rate.

Thanks @Krinkle. Filed {T349845} for that discussion.

I'd like to resolve this task with the submitted patch. That will at least mitigate this current issue while we think about a more comprehensive solution.

In T342487#9331817, @Ottomata wrote:

I'd like to resolve this task with the submitted patch. That will at least mitigate this current issue while we think about a more comprehensive solution.

Does that require that the follow-up https://gerrit.wikimedia.org/r/971310 should be merged? If not, then I think this can likely be resolved and made public since the very specific, original issue should now be fixed via https://gerrit.wikimedia.org/r/963324.

Does that require that the follow-up https://gerrit.wikimedia.org/r/971310 should be merged?

Yes, it should be merged. Waiting for review on it now.

My apologies for the late review, +1 to Scott's point of resolving this and making it public.

+1 to Timo's overall point that MW should be the thing that decides what to publish to the outside world. I think we've always wanted to do it that way, and maybe the new reorg will let us do it? Who's in?

Change 971310 merged by jenkins-bot:

[mediawiki/extensions/EventBus@master] Don't set performer on a few more cases of suppression

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

@stwalkerster, https://gerrit.wikimedia.org/r/971310 should be in production, can you do your test again and verify that this has been solved?

Bump @stwalkerster ?

Apologies, I've not been well the past week so I forgot about this.

I did a suppress and an unsuppress, and these are the emitted events:

event: message
id: [{"topic":"eqiad.mediawiki.revision-visibility-change","partition":0,"timestamp":1702592240152},{"topic":"codfw.mediawiki.revision-visibility-change","partition":0,"timestamp":1702592290001}]
data: {"$schema":"/mediawiki/revision/visibility-change/1.0.0","meta":{"uri":"https://en.wikipedia.org/wiki/User:Stwalkerster/sandbox","request_id":"42bbdf3b-797b-4750-a24a-6e9c91e0a771","id":"23ea3536-dcf3-474e-9b26-72394bbca2a0","dt":"2023-12-14T22:18:10Z","domain":"en.wikipedia.org","stream":"mediawiki.revision-visibility-change","topic":"codfw.mediawiki.revision-visibility-change","partition":0,"offset":437023},"database":"enwiki","page_id":59627106,"page_title":"User:Stwalkerster/sandbox","page_namespace":2,"rev_id":1175405362,"rev_timestamp":"2023-09-14T20:22:21Z","rev_sha1":"14n12z2r5ebejrqamp9mflvw8vb1hwn","rev_minor_edit":false,"rev_len":1291,"rev_content_model":"wikitext","rev_content_format":"text/x-wiki","page_is_redirect":false,"rev_parent_id":1170712760,"visibility":{"text":false,"user":false,"comment":false},"prior_state":{"visibility":{"text":true,"user":true,"comment":true}}}

event: message
id: [{"topic":"eqiad.mediawiki.revision-visibility-change","partition":0,"timestamp":1702592240152},{"topic":"codfw.mediawiki.revision-visibility-change","partition":0,"timestamp":1702592297001}]
data: {"$schema":"/mediawiki/revision/visibility-change/1.0.0","meta":{"uri":"https://en.wikipedia.org/wiki/User:Stwalkerster/sandbox","request_id":"af646553-8f24-4843-8dfa-96cc2c46c455","id":"fca71b67-5910-4b09-9edf-1382c8b236f4","dt":"2023-12-14T22:18:17Z","domain":"en.wikipedia.org","stream":"mediawiki.revision-visibility-change","topic":"codfw.mediawiki.revision-visibility-change","partition":0,"offset":437024},"database":"enwiki","page_id":59627106,"page_title":"User:Stwalkerster/sandbox","page_namespace":2,"rev_id":1175405362,"rev_timestamp":"2023-09-14T20:22:21Z","rev_sha1":"14n12z2r5ebejrqamp9mflvw8vb1hwn","rev_minor_edit":false,"rev_len":1291,"rev_content_model":"wikitext","rev_content_format":"text/x-wiki","page_is_redirect":false,"comment":"added [[Category:X9]] using [[WP:HC|HotCat]]","parsedcomment":"added <a href=\"/wiki/Category:X9\" title=\"Category:X9\">Category:X9</a> using <a href=\"/wiki/Wikipedia:HC\" class=\"mw-redirect\" title=\"Wikipedia:HC\">HotCat</a>","rev_parent_id":1170712760,"visibility":{"text":true,"user":true,"comment":true},"prior_state":{"visibility":{"text":false,"user":false,"comment":false}}}

I don't see anything left in that which shouldn't be publicly visible.

I also tried a suppress/unsuppress of a log entry and an AbuseFilter log entry, but those didn't emit any events via this stream.

I think this is now solved - thanks to all above!

Awesome, thank you so much for reporting this and for your help figuring out the correct behavior!

Do we need/want to make this task PermanentlyPrivate or can we make it public? I'm not sure how sensitive folks might consider some of the sample data provided here.

IMHO this can be made public.

All of the examples I provided above were changes to revisions in my sandbox that have no reason to be suppressed, and I only did the suppressions to generate test cases anyway. As none of the suppressions here are keeping anything private, there's no need to hide that I did the suppressions in question (and realistically that's the only bit of info we're hiding if we do keep this private)