We definitely need to hide action (imho, although adding Aaron since he wrote https://www.mediawiki.org/wiki/Special:Code/MediaWiki/r46917).

In Special:Log, the type can (In reply to Bawolff (Brian Wolff) from comment #0)

Additionally, if
filtering by log type such revdeleted entries aren't shown

I'm not seeing. If I only "Hide action and target", and don't suppress, the log entry still shows up for anons when filtering by type. Am I missing a step?

(In reply to Chris Steipp from comment #1)

We definitely need to hide action (imho, although adding Aaron since he
wrote r46917).

In Special:Log, the type can (In reply to Bawolff (Brian Wolff) from comment
#0)

Additionally, if
filtering by log type such revdeleted entries aren't shown

I'm not seeing. If I only "Hide action and target", and don't suppress, the
log entry still shows up for anons when filtering by type. Am I missing a
step?

Whoops, my mistake. I was testing with something in the title field.

Still it seems questionable to allow filtering by log_type, but not allow showing log_type. All one would have to do to find out what the log_type was, is do a search for each log_type with an appropriate timestamp range (since the timestamp is never hidden).

Unless its really log_action we're trying to hide, not log_type. I'll be honest I'm not really clear what the specific use cases are to hide the type/action, my main concern was that the UI promises to hide it, thus the API should honor the promise.

I wonder whether the hiding of action was really intended, or whether it's just because the action text is (or at the time was) generated whole by LogPage::actionText() and it was easier to generically hide the whole thing instead of modifying that method to be aware of whether the title and other details were supposed to be hidden when expanding the i18n message.

At any rate, there's no point in hiding log_type since it's easily discoverable through the web UI by filtering by log type.

I'd lean towards that there isn't any point in hiding log_action either. But if we do hide log_action, then yes, we need to restrict queries with leaction just as we do letitle or leuser.

So, the first question: Do we even want to hide log_action, or do we let r46917 stand? If yes, I'll fix the API. If no, we could as well update LogFormatter to use messages such as "rev-deleted-event-$type-$action" (with fallback to "rev-deleted-event") if available and adjust the description for DELETED_ACTION to refer to "title and other details".

The field values don't matter. As Brad said, it's about the title and possibly some log_params that form the action text.

Adding Dan, since it seems like the question is how are admins actually using this feature.

Aaron clarified on irc that the intent of DELETED_ACTION was to hide the rendering of the log message, which usually includes the title and other parameters based on the action taken. The actual action taken (this was a "delete", or a user "create") wasn't intended to be hidden.

I think the distinction only matters if an admin is trying to hide that they took a particular action, instead of just getting rid of the content of an action that might contain vandalism or libel. If admins are doing the former, then the api leaks the general action they took; if we only care about the later, then we should probably just update the message that we show on the revdel form, and call it "Title and other details" instead of "Hide action and target".

Dan, do you have any thoughts, or know of an oversighter who I should ask?

(In reply to Chris Steipp from comment #5)

Adding Dan, since it seems like the question is how are admins actually
using this feature.

The best thing to do here is to go over this in person really quickly. I'll send you an invite for tomorrow and we can figure out the use case and what the best way forward is.

So, in my opinion as a product manager, I agree with Brian that the API should not expose the type of log entry if it was previously suppressed. The UI and API should be consistent in this regard.

In terms of severity, I'd rate this bug as fairly minor. It's definitely a security hole that needs to be fixed, but there's not really an awful lot that one could do with this malicious information if it's just the class of log entry.

Let me know if there's anything else I can do for you.

Thanks Dan. So based on that, a version of bawolffs patch that leaves out the changes for type seems appropriate. Brian / Brad, would either of you be able to do that?

(In reply to Chris Steipp from comment #8)

Thanks Dan. So based on that, a version of bawolffs patch that leaves out
the changes for type seems appropriate. Brian / Brad, would either of you be
able to do that?

I'd much rather we decide that "Hide action and target" was referring to the text in Special:Log rather than the log_action field itself, and the web UI hiding log_action was accidental just because that's not actually exposed anywhere in the web UI other than in combination with the title and other details.

But if we're really not going to go that way, the patch itself is easy to fix. I'll need to double-check that the anti-brute-force-search code will work right though, it's not immediately obvious to me from the diff itself.

Created attachment 16943
Hide log action from api when log is flagged with DELETED_ACTION

Updated bawolffs patch to only hide action, and not type.

Brad/Aaron, I know you didn't prefer this approach, but does this patch look reasonable based on what Dan said for the desired functionality?

And are there any other places that you know of where we made the assumption that DELETED_ACTION was about the message, instead of the actual log_action field?

attachment bug72222.patch ignored as obsolete

(In reply to Chris Steipp from comment #10)

Brad/Aaron, I know you didn't prefer this approach,

That's an understatement. As I stated in comment 9, the only reason log_action isn't shown in the web UI is because the web UI doesn't show log_type or log_action outside of the filtering (which only takes log_type into account) and the specific message used to display the log details. The UI string says it hides the "action and target" because it's referring to the description of the action (which includes the title and other details) in the web UI.

I might post an RFC to propose reverting this and fixing the UI string instead, once this is public.

but does this patch look
reasonable based on what Dan said for the desired functionality?

Apparently fld_action was never actually used (r63301 didn't actually add llprop=action, and it would always have been a synonym for llprop=type). You should kill it and just use fld_type everywhere.

Other than that, it looks like it will correctly implement what it is intending to implement.

And are there any other places that you know of where we made the assumption
that DELETED_ACTION was about the message, instead of the actual log_action
field?

Not that I know of. But since that was the intended interpretation, it wouldn't surprise me.

Created attachment 16966
Hide log action from api when log is flagged with DELETED_ACTION

As Brad pointed out, fld_action was never set because 'action' was never added to the list of properties you could get.

Attached:

(In reply to Brad Jorsch from comment #11)

I might post an RFC to propose reverting this and fixing the UI string
instead, once this is public.

I definitely think we should. I really don't like deciding how a feature is supposed to work inside of a security bug. It's just hard to get all the right people looking at the issue while there's a chance it's leaking data someone thought was going to be hidden.

On the other side of this, is there a good reason to *not* hide this? Do we know of any api consumers that would be negatively impacted by this?

I noticed that Aaron gave basically the same answer about this in 2009 (https://www.mediawiki.org/wiki/Special:Code/MediaWiki/46807#c1647), so we definitely have been assuming this for a long time.

Dan, would it make sense to privately ask some of the oversighters who might be affected by this what they think? If no one is concerned that the action is going to out them for some reason, and MediaWiki has assumed Aaron's definition of this feature for the last 5 years, it seems like we should just make it the official definition.

(In reply to Chris Steipp from comment #13)

On the other side of this, is there a good reason to *not* hide this? Do we
know of any api consumers that would be negatively impacted by this?

The main reason is in not hiding stuff that doesn't need to be hidden. Consider for example bug 49088 and bug 58993 where we have or are looking at unhiding more stuff that has traditionally been hidden.

I don't know of any clients that would be negatively affected, but I can't say there aren't any.

I deployed

ping @Mglaser

Here's the backport for REL1_23 for review. REL1_24 applies nicely, without a backport.

REL1_22 and REL1_19 are a bit more tricky and I still need some time to do them.

Actually, I'm lost with the backport for REL1_22. There is apparently no user rights check at all in the relevant code bit. This would need a bigger refacturing, as I see it. Can anyone help me with the backport (@csteipp?) or do you think it's ok to not backport this at all to REL1_22 and REL1_19?

The bit about checking the user rights in the "Paranoia: avoid brute force searches" check wasn't added until 1.23 (specifically Gerrit change 107389). To backport this mistake to earlier versions, you'd probably just want to go with adding "|| !is_null( $params['action'] )" to the existing "!is_null( $title )" check.

The second part should be straightforward enough to backport.

Thanks, @Anomie. Could you have a look?

My test scenario is:

• create a page with 3 revisions
• delete middle revision
• call api.php?action=query&list=logevents as anonymous user

This is still displayed:
<item logid="14" ns="0" title="Deletedtest" pageid="10" type="delete" action="revision" user="WikiSysop" timestamp="2014-11-26T16:34:08Z" comment="">

It shouldn't be, right?

Backport attempt for REL1_19. Someone please check this.

In T74222#788716, @Mglaser wrote:

Thanks, @Anomie. Could you have a look?

You screwed up merging the patch.

Since Phabricator is apparently too "fancy" to support a normal file upload instead of drag-and-drop only, I'll copy-paste what should be a fixed patch here for you:

From 1da5ceb457bca68d41c0dbe718a0ea3be776c8c7 Mon Sep 17 00:00:00 2001
From: csteipp <csteipp@wikimedia.org>
Date: Wed, 29 Oct 2014 08:41:20 -0700
Subject: [PATCH] SECURITY: Do not show log action if revdeleted

Also do not include revdeleted entries in search results when
filtering by action if user cannot view that info.

Bug: 72222
Change-Id: I9f331c421c55323018765456d6a99229e1fff592
---
 includes/api/ApiQueryLogEvents.php | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/includes/api/ApiQueryLogEvents.php b/includes/api/ApiQueryLogEvents.php
index 26774ef..0e8c5e6 100644
--- a/includes/api/ApiQueryLogEvents.php
+++ b/includes/api/ApiQueryLogEvents.php
@@ -157,7 +157,7 @@ class ApiQueryLogEvents extends ApiQueryBase {
 		$this->addOption( 'USE INDEX', $index );
 
 		// Paranoia: avoid brute force searches (bug 17342)
-		if ( !is_null( $title ) ) {
+		if ( !is_null( $title ) || !is_null( $params['action'] ) ) {
 			$this->addWhere( $db->bitAnd( 'log_deleted', LogPage::DELETED_ACTION ) . ' = 0' );
 		}
 		if ( !is_null( $user ) ) {
@@ -300,10 +300,13 @@ class ApiQueryLogEvents extends ApiQueryBase {
 			$title = Title::makeTitle( $row->log_namespace, $row->log_title );
 		}
 
-		if ( $this->fld_title || $this->fld_ids ) {
+		if ( $this->fld_title || $this->fld_ids || $this->fld_type ) {
 			if ( LogEventsList::isDeleted( $row, LogPage::DELETED_ACTION ) ) {
 				$vals['actionhidden'] = '';
 			} else {
+				if ( $this->fld_type ) {
+					$vals['action'] = $row->log_action;
+				}
 				if ( $this->fld_title ) {
 					ApiQueryBase::addTitleInfo( $vals, $title );
 				}
@@ -313,9 +316,8 @@ class ApiQueryLogEvents extends ApiQueryBase {
 			}
 		}
 
-		if ( $this->fld_type || $this->fld_action ) {
+		if ( $this->fld_type ) {
 			$vals['type'] = $row->log_type;
-			$vals['action'] = $row->log_action;
 		}
 
 		if ( $this->fld_details && $row->log_params !== '' ) {
-- 
2.1.3

Oh. Thanks a lot. The patch applies nicely. However, I still can see the deleted revision as an anonymous user :/

Tested api.php?action=query&list=logevents on REL1_24, REL1_23, REL1_22. Still works. However, see my comment above about REL1_22. If this backport is ok, I'll do the same for REL1_19.

Just found that the patch for REL1_22 also applies to REL1_19.

In T74222#788716, @Mglaser wrote:

Thanks, @Anomie. Could you have a look?

bug72222b-REL1_22.patch1 KBDownload

My test scenario is:

• create a page with 3 revisions
• delete middle revision
• call api.php?action=query&list=logevents as anonymous user

The test case I'm using is,

• Move a page
• Go to Special:Log and change the visibility of the move log entry
• Only select "Hide action and target"
• Visit api.php?action=query&list=logevents as an unprivileged user

Without the patch, you see '...type="move" action="move"...', with the patch applied, 'action="move"' isn't visible.

Patch for REL1_23, and anomie's patch applied on 1.22/1.19 fix the issue.

Change 178527 had a related patch set uploaded (by Anomie):
Revert "SECURITY: Do not show log action if revdeleted" and fix UI message

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

Patch-For-Review

Change 178527 merged by jenkins-bot:
Revert "SECURITY: Do not show log action if revdeleted" and fix UI message

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

Change 180152 had a related patch set uploaded (by Mglaser):
Revert "SECURITY: Do not show log action if revdeleted" and fix UI message

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

Patch-For-Review

Change 180181 had a related patch set uploaded (by Mglaser):
Revert "SECURITY: Do not show log action if revdeleted" and fix UI message

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

Patch-For-Review

Change 180192 had a related patch set uploaded (by Mglaser):
Revert "SECURITY: Do not show log action if revdeleted" and fix UI message

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

Patch-For-Review

Change 180196 had a related patch set uploaded (by Mglaser):
Revert "SECURITY: Do not show log action if revdeleted" and fix UI message

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

Patch-For-Review

Change 180152 merged by jenkins-bot:
Revert "SECURITY: Do not show log action if revdeleted" and fix UI message

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

Change 180181 merged by jenkins-bot:
Revert "SECURITY: Do not show log action if revdeleted" and fix UI message

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

Change 180192 merged by jenkins-bot:
Revert "SECURITY: Do not show log action if revdeleted" and fix UI message

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

Change 180196 merged by jenkins-bot:
Revert "SECURITY: Do not show log action if revdeleted" and fix UI message

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