Page MenuHomePhabricator

AbuseFilter should respect revdel flags
Closed, ResolvedPublic

Description

Once a revision has been hidden from public view, the associated abusefilter log entries should also be redacted so that the revision/log details can not be seen without the appropriate permissions.

The AbuseFilter has its own 'visibility' system which isn't very flexible and it may not be necessary once the extension respects the revdel flags associated with the revision and log entries.


Version: unspecified
Severity: major
URL: triage
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=18043

Details

Reference
bz28633

Event Timeline

bzimport raised the priority of this task from to High.Nov 21 2014, 11:34 PM
bzimport set Reference to bz28633.
bzimport added a subscriber: Unknown Object (MLST).

Is this related to why I can't see the contents of the page you
mention in Bug #28632 even though you have undeleted it?

(In reply to comment #1)

Is this related to why I can't see the contents of the page you
mention in Bug #28632 even though you have undeleted it?

No.

Thehelpfulonewiki wrote:

I just noticed this bug, was going to post a new one - but:

Basically, I would like to be able to have a feature for admins to be able to effectively revdelete abuse filter logs, just as Oversights can suppress them thanks to the bug being fixed.

There's some filters that are more likely to have the need for suppression, and whilst they will eventually need supression by an oversighter, and admin could easily help out temporarily to hide it.

I don't know how easy/difficult this would be to implement.

Thanks,

Thehelpfulone

For now this is probably in the "patches welcome" category.

What would be necessary would be to hook the "revision deleted" event, and, when that occurs, find the filter log entries with the same title, user and timestamp, and set their afl_deleted flags to the appropriate value.

It would not be advisable to do this dynamically, as the original reporter suggests.

This could be more easily implemented once bug 18374 is solved.
Also, I was wrong about AbuseFilter 'visibility' being unnecessary once this bug is solved, as it would still be needed for AF log entries about edits which were never saved.

(In reply to comment #5)

This could be more easily implemented once bug 18374 is solved.

Well, not quite. They would both be made easier by a particular change to the filter's functionality.

Bumping priority. Thehelpfulone tells me via email:

this bug is, in my
opinon, very crucial to fix as it means that if a page has been oversighted
but it was flagged by an abuse filter, then the logs of this abuse filter
will show the content of the oversighted page. Therefore, this currently
means that oversighters need to oversight 2 things - the page itself *and* the
abuse filter log. This can unfortunately create a lot of extra work for
them, so if this bug could be fixed, I'm sure they would appreciate it,
especially as most of these oversighted pages contain sensitive personal
information.

Bumping back to normal priority. Mark, let's talk about the correct priority for this?

(In reply to comment #8)

Bumping back to normal priority. Mark, let's talk about the correct priority
for this?

Yeah, highest was probably over-the-top. But normal is higher than "lowest", so I'm happy.

Thehelpfulonewiki wrote:

So I asked an oversighter who knows about this bug to justify why it should be a higher priority, speaking from their experience:

"As it stands, when an oversighter suppresses something, we have to go hunting to find out if a log exists, then find it, then suppress it separately. It doubles both the time required, and the likelihood that private information will be missed accidentally"

I believe this is a strong argument for fixing this bug more quickly. Hope you agree. - THO

Re-raising to HIGHEST after reading Thehelpfulone's explanation and the possibility of private information leaking out.

Fix committed in r111217. Comments welcomed.

Thehelpfulonewiki wrote:

Reopening, this still has not been fixed yet, https://gerrit.wikimedia.org/r/#/c/3435/ is where it is in gerrit.

still not fixed as the log entries are still visible when someone revdel or suppress and edit -- it seems to be deployed in gerrit (see above)?

Confirmed. One edit was revdel'd and then later suppressed at nlwiki. It had tripped and that resulted in 8 (2x4) logged filter additions that needed to be suppressed. And they had to be suppressed individually, no checkbox to select to group apply.

https://nl.wikipedia.org/w/index.php?title=Speciaal%3ALogboeken&type=suppress&user=&page=&year=&month=-1&tagfilter=&hide_patrol_log=1

(In reply to comment #16)

Confirmed. One edit was revdel'd and then later suppressed at nlwiki. It
had
tripped and that resulted in 8 (2x4) logged filter additions that needed to
be
suppressed. And they had to be suppressed individually, no checkbox to select
to group apply.

https://nl.wikipedia.org/w/index.
php?title=Speciaal%3ALogboeken&type=suppress&user=&page=&year=&month=-
1&tagfilter=&hide_patrol_log=1

So there are two bugs here.

The user attempted to save the page at 13:02:56, and received a warning. Then s/he clicked "save" again, and saved successfully at 13:03:20.

Because the first edit attempt was rejected with a warning, it was not associated with a revision, so deleting the revision which was subsequently created had no effect. I'm not sure what the best solution here is, maybe just a bulk suppression UI.

The second edit attempt succeeded, but afl_rev_id was still not set, because of interaction with the autosummary feature.

if ( ( $vars->getVar('article_prefixedtext')->toString() !==
$article->getTitle()->getPrefixedText() ) ||
( $vars->getVar('summary')->toString() !== $summary )
) {
return true;
}

If the summary is different after the edit is saved compared to when the filter was run, afl_rev_id is not updated. The edit in question did have an autosummary, and I've confirmed locally that enabling autosummaries allows the bug to be reproduced.

Just removing the summary condition should fix that part of it.

(In reply to comment #17)

Just removing the summary condition should fix that part of it.

I94321905f38eafde8add00eff73745af255c1f15

(In reply to comment #17)

The user attempted to save the page at 13:02:56, and received a warning. Then
s/he clicked "save" again, and saved successfully at 13:03:20.

Because the first edit attempt was rejected with a warning, it was not
associated with a revision, so deleting the revision which was subsequently
created had no effect. I'm not sure what the best solution here is, maybe
just a bulk suppression UI.

This is a mess of an issue. :-(

Perhaps we could automatically add a note on action=revisiondelete saying that there are AbuseFilter entries for a page of this title not associated with any particular revision, and asking the user to check to make sure they don't need to suppress these too? Somewhat succinctly (albeit not hugely accurately), "there are possible matching AbuseFilter entries for this page title"?

That would probably suffice as long as there was an easy means to collect all the hits in the abusefilter, so the OS can review and then select those that should be culled. Doing one by one (for either) is a tad trying.

(In reply to comment #19)

(In reply to comment #17)

The user attempted to save the page at 13:02:56, and received a warning. Then
s/he clicked "save" again, and saved successfully at 13:03:20.

Because the first edit attempt was rejected with a warning, it was not
associated with a revision, so deleting the revision which was subsequently
created had no effect. I'm not sure what the best solution here is, maybe
just a bulk suppression UI.

This is a mess of an issue. :-(

Perhaps we could automatically add a note on action=revisiondelete saying
that
there are AbuseFilter entries for a page of this title not associated with
any
particular revision, and asking the user to check to make sure they don't
need
to suppress these too? Somewhat succinctly (albeit not hugely accurately),
"there are possible matching AbuseFilter entries for this page title"?

I agree, that's a possibility. But, abusefilter logs can only be hidden by oversighters and as nlwiki doesn't have local OS no one can hide the entries apart from the stewards (and most of the local users do not even know you *can* hide those). That's why it would be nice if they would be automatically be hidden (even a revdel system for the logs perhaps?)

(In reply to comment #21)

(In reply to comment #19)

Perhaps we could automatically add a note on action=revisiondelete saying
that there are AbuseFilter entries for a page of this title not
associated with any particular revision, and asking the user to check to
make sure they don't need to suppress these too? Somewhat succinctly (albeit
not hugely accurately), "there are possible matching AbuseFilter entries
for this page title"?

I agree, that's a possibility. But, abusefilter logs can only be hidden by
oversighters and as nlwiki doesn't have local OS no one can hide the entries
apart from the stewards (and most of the local users do not even know you
*can* hide those). That's why it would be nice if they would be automatically
be hidden (even a revdel system for the logs perhaps?)

The problem is that the RevDel system is about deleting revisions and log entries; AF log entries that block a save don't get associated with a revision, so AF can't automatically hide these. The automatic-hiding-of-AF-hits can only work because the AF hit is recorded against a revision that is being suppressed. My suggestion would at least highlight that further work could be needed.

Presumably if NLwiki has no oversighters it also can't suppress the edits using RevDelete, so they are just leaving them available to any passing sysop? That's rather troubling. Theoretically NLwiki could extend the "hide this AF hit" to local sysops (assuming that's OK with legal and the community itself?), but if they are dealing with a lot of these issues they should probably consider getting some local OSers like other big wikis with this issue. If neither of these options appeal, at the least if the notice is present it could be locally adjust to advise asking for a steward to help.

(In reply to comment #22)

(In reply to comment #21)

(In reply to comment #19)

Perhaps we could automatically add a note on action=revisiondelete saying
that there are AbuseFilter entries for a page of this title not
associated with any particular revision, and asking the user to check to
make sure they don't need to suppress these too? Somewhat succinctly (albeit
not hugely accurately), "there are possible matching AbuseFilter entries
for this page title"?

I agree, that's a possibility. But, abusefilter logs can only be hidden by
oversighters and as nlwiki doesn't have local OS no one can hide the entries
apart from the stewards (and most of the local users do not even know you
*can* hide those). That's why it would be nice if they would be automatically
be hidden (even a revdel system for the logs perhaps?)

The problem is that the RevDel system is about deleting revisions and log
entries; AF log entries that block a save don't get associated with a
revision,
so AF can't automatically hide these. The automatic-hiding-of-AF-hits can
only
work because the AF hit is recorded against a revision that is being
suppressed. My suggestion would at least highlight that further work could be
needed.

Presumably if NLwiki has no oversighters it also can't suppress the edits
using
RevDelete, so they are just leaving them available to any passing sysop?
That's
rather troubling. Theoretically NLwiki could extend the "hide this AF hit" to
local sysops (assuming that's OK with legal and the community itself?), but
if
they are dealing with a lot of these issues they should probably consider
getting some local OSers like other big wikis with this issue. If neither of
these options appeal, at the least if the notice is present it could be
locally
adjust to advise asking for a steward to help.

Sure, stewards help where they can. And personally I don't see the need for local oversighters, nor does the community. But isn't it possible then to give admins the rights to revdel abuse filter log entries? (instead of only be able to suppress these) In that case admins could revdel them and with real privacy issues stewards could suppress them as well.

Would it be ok for us to close this one as fixed, since Tim's fix handles the most immediate issue? At a minimum, I'm unassigning this from Tim, but we understand there's potentially a feature request or two buried in here. Might be best to file those as separate issues.

billinghurst / John Mark / Trijnstel:
Could you answer comment 24, please?

I thought that was rhetorical. I am comfortable to see changes at work, and it is one that I have only tripped over once, so not one in which I am an expert.

Thanks billinghurst, closing out this bug for now. Feel free to reopen or file a new bug as appropriate.