Non-admins can see contents of deleted pages when viewing abusefilter details
OpenPublic

Description

[[Healthy Children Project's Center for Breastfeeding]] was deleted on enwiki, however I can still see parts of the edit history (and therefore, article content) by looking at specific abuse filter log entries like [[Special:AbuseLog/7900305]] even though I am a non-sysop, and shouldn't be able to see deleted pages.

I'm marking this as major since non-sysops shouldn't be able to see deleted pages.


Version: unspecified
Severity: enhancement

bzimport set Reference to bz42734.
Legoktm created this task.Via LegacyDec 5 2012, 7:38 PM
Aklapper added a comment.Via ConduitDec 7 2012, 11:34 AM

Confirming.

csteipp added a comment.Via ConduitDec 7 2012, 2:14 PM

We can definitely hide the data when the article has been deleted, as was done in this case. We store the article title in the AbuseFilter log, so we can check if it's been deleted.

However, if a public rule is triggered for a particular revision that later is deleted, AbuseFilter doesn't keep around enough information to prevent showing this.

A workaround is to restrict the abusefilter-log-detail permission. By default, it's given to *.

Legoktm added a comment.Via ConduitDec 7 2012, 2:34 PM

(In reply to comment #3)

We can definitely hide the data when the article has been deleted, as was
done in this case.

What was hidden in this case? I'm pretty sure I can see everything.

We store the article title in the AbuseFilter log, so we can
check if it's been deleted.

However, if a public rule is triggered for a particular revision that later
is
deleted, AbuseFilter doesn't keep around enough information to prevent
showing
this.

I'm not following you. Can't it just check if the title exists and choose to show the detailed information based on that?

A workaround is to restrict the abusefilter-log-detail permission. By
default,
it's given to *.

I'm not sure if restricting abusefilter-log-detail is the right solution. There are non-sysops who have the abusefilter-modify right through the EFM group who technically shouldn't be able to see deleted content, but would still be able to.

And if we're still tracking harmless things like WikiLove ([[Special:AbuseFilter/423]]) in the EF, it makes no sense to hide that from typical users.

Krenair added a comment.Via ConduitDec 7 2012, 4:24 PM

(In reply to comment #4)

> We store the article title in the AbuseFilter log, so we can
> check if it's been deleted.
>
> However, if a public rule is triggered for a particular revision that later
> is
> deleted, AbuseFilter doesn't keep around enough information to prevent
> showing
> this.

I'm not following you. Can't it just check if the title exists and choose to
show the detailed information based on that?

A page would be considered existing if it has at least one revision which is not deleted. This means that if you:

Create a page
Edit it, edit hits a filter and gets logged
Delete the page - the details of your edits should now be hidden in AF
Someone else creates the page with the same title

Suddenly, the AF entry is visible again because the title exists.

MZMcBride added a comment.Via ConduitDec 8 2012, 3:36 AM

(In reply to comment #5)

A page would be considered existing if it has at least one revision which is
not deleted. This means that if you:

Create a page
Edit it, edit hits a filter and gets logged
Delete the page - the details of your edits should now be hidden in AF
Someone else creates the page with the same title

Suddenly, the AF entry is visible again because the title exists.

I'm not sure this edge case is very important.

csteipp added a comment.Via ConduitDec 11 2012, 11:19 PM

I'm adding this to our backlog of Admin Tools, but just to flesh out the requirements:

  • The AbuseLog entry should be hidden in cases where we have a legal obligation to remove the content. So as Krenair pointed out, we can't just not show it if the page doesn't exist, otherwise anyone could create the page to read the content.
  • Is it really desired by most of the admins to have these log entries hidden whenever a page is deleted, even when, for example, it's just advertising vs copyright violation? Or would it only be for pages that are deleted for specific reasons?
  • On the overall list of priorities, is this something admins are desperate for? Or more of a nice to have right now?
Legoktm added a comment.Via ConduitDec 15 2012, 7:47 PM

(In reply to comment #7)

  • The AbuseLog entry should be hidden in cases where we have a legal obligation to remove the content. So as Krenair pointed out, we can't just not show it if the page doesn't exist, otherwise anyone could create the page to read the content.

True, ideally the log entry would be undeleted when the page is undeleted.

  • Is it really desired by most of the admins to have these log entries hidden whenever a page is deleted, even when, for example, it's just advertising vs copyright violation? Or would it only be for pages that are deleted for specific reasons?

I think its more important for say, copyvios and major BLP violations to be removed rather than things that were deleted in AFD for being [[WP:NOT]] (out of scope). Maybe just any variables which contain page text (like old_wikitext, the diff, etc) should be removed from the details & examine pages so the log entry remains, similar to how if a page is deleted, any entries in [[Special:Log/move]] aren't.

  • On the overall list of priorities, is this something admins are desperate for? Or more of a nice to have right now?

Given that this bug has existed ever since the AbuseFilter was introduced (2 years), I don't think its that urgent since no one ever noticed it until I did.

As in interim solution, it would be nice to be able to delete certain log entries (possibly a separate bug in itself).

csteipp added a comment.Via ConduitDec 17 2012, 6:04 PM

As in interim solution, it would be nice to be able to delete certain log
entries (possibly a separate bug in itself).

Users with the 'abusefilter-hide-log' privilege can do this. Although now that we automatically hide a revision's logs when the revision is deleted, I'm not sure if admins are in the habit of deleting the AF log when an entire page is deleted.

We may be able to hook into the UI to make hiding the AF logs easy when someone is deleting a page. I'm going to mark this as a lower priority enhancement, since it sounds like it would be useful, but the overall result is possible in the existing tools (unless I'm missing something).

Actually, I'm pretty sure this could be implemented as a pretty simple gadget, which might be a good way to try out the requirements.

hoo added a comment.Via ConduitDec 29 2012, 1:50 AM

I just wrote a patch which auto deletes all log entries for a given page by the time it's deleted. I'm not sure anymore this is the best way to go (as only Oversighters can see them then)... any ideas?

Alternatives would be to implement a automated deletion with user confirmation or adding a page_id field to the abuse log table.

Legoktm added a comment.Via ConduitJan 21 2013, 2:35 AM

(In reply to comment #10)

I just wrote a patch which auto deletes all log entries for a given page by
the
time it's deleted. I'm not sure anymore this is the best way to go (as only
Oversighters can see them then)... any ideas?

I don't think giving non-oversight'ers a way to oversight log entries is a good idea. AF should probably have a way to "revdel" log entries to the "viewdeleted" level, and then a further level of "suppressed", making it equivalent to normal log entries. Maybe a separate bug?

Alternatives would be to implement a automated deletion with user
confirmation
or adding a page_id field to the abuse log table.

I'm not too familiar if/how page_id's change when deleting and undeleting, but as long as it provided an way to delete them and then undelete them it would work.

csteipp added a comment.Via ConduitFeb 22 2013, 5:59 PM

Would it be common for a page that needs to be deleted and suppress to be moved around first, so that we can't match the page's title in the abusefilter logs?

Again, if we're talking about a single revision for a page (excluding the very first revision when the page is created), suppressing the revision will also suppress the AbuseFilter logs automatically. So this is only when the initial edit needs to be suppressed (since AF logs it without a revision_id, since no revision exists at the time that we're filtering), or the page title needs suppression.

How about inserting a link on the delete confirmation page (using the hook), that links to the abuse filter logs for that page, if any abusefilter logs exist for that title and the user has suppression rights? That would let someone who is suppressing data know that they need to also check the logs and see if any entries need suppression as well. But the interface would remain unchanged for most users.

hoo added a comment.Via ConduitFeb 22 2013, 6:52 PM

(In reply to comment #12)

Would it be common for a page that needs to be deleted and suppress to be
moved
around first, so that we can't match the page's title in the abusefilter
logs?

No

Again, if we're talking about a single revision for a page (excluding the
very
first revision when the page is created), suppressing the revision will also
suppress the AbuseFilter logs automatically. So this is only when the initial
edit needs to be suppressed (since AF logs it without a revision_id, since no
revision exists at the time that we're filtering), or the page title needs
suppression.

How about inserting a link on the delete confirmation page (using the hook),
that links to the abuse filter logs for that page, if any abusefilter logs
exist for that title and the user has suppression rights? That would let
someone who is suppressing data know that they need to also check the logs
and
see if any entries need suppression as well. But the interface would remain
unchanged for most users.

That would come with a different flavor of the problem I mentioned above: Only oversighters can hide and unhide log entries (at least on WMF wikis) as we only have one hidden level. If I recall it right it's not trivial to introduce more than one... I thought about this bug (and even hacked up some stuff) in December and wasn't able to come up with a perfect solution (see above)

Aklapper added a comment.Via ConduitFeb 28 2014, 3:04 PM

Marius Hoch: You assigned this issue to yourself 14 months ago.
Could you please provide a status update and inform us whether you are still working (or still plan to work) on this issue?
Only in case you do not plan to work on this issue anymore, should the assignee be set back to default and the bug status changed from ASSIGNED to NEW/UNCONFIRMED? Thanks.

hoo added a comment.Via ConduitFeb 28 2014, 3:07 PM

(In reply to Andre Klapper from comment #14)

Marius Hoch: You assigned this issue to yourself 14 months ago.
Could you please provide a status update and inform us whether you are still
working (or still plan to work) on this issue?
Only in case you do not plan to work on this issue anymore, should the
assignee be set back to default and the bug status changed from ASSIGNED to
NEW/UNCONFIRMED? Thanks.

Well, I (hopefully) still have the patch mentioned in #c10 around (and can rebase it to work with AbuseFilter master). But I want some more feedback about this... if this feedback is given, I guess I can work on this (again).

csteipp added a comment.Via ConduitMar 3 2014, 6:49 PM

Did we pull out the automatic hiding of deleted revisions? I'm sure I saw that, but I'm not seeing it right now. That would be along the same lines, where a non oversighter was essentially causing a suppression event. But if we don't have that, then hoo's patch probably is doing that, and seems wrong.

I think we could introduce delete and suppress for the log-- that seems like it's going to be needed in the long run, and would make this work.

hoo added a comment.Via ConduitMar 3 2014, 7:19 PM

(In reply to Chris Steipp from comment #16)

I think we could introduce delete and suppress for the log-- that seems like
it's going to be needed in the long run, and would make this work.

I initially thought this would require a schema change, but that's not true:

afl_deleted tinyint(1) NOT NULL DEFAULT 0,

If we have that, we can probably use it together with a slightly modified version of my patch mentioned above.

werdna removed a subscriber: werdna.Via WebDec 10 2014, 5:27 PM
MGChecker added a subscriber: MGChecker.Via WebMon, Jul 6, 12:17 PM
Restricted Application added a subscriber: Luke081515. · View Herald TranscriptVia HeraldMon, Jul 6, 12:17 PM

Add Comment