Page MenuHomePhabricator

Special:Log implements revdel restrictions incorrectly when filtering on log type or log author
Closed, ResolvedPublic

Description

See T19342 for original bug which was fixed incorrectly.

Currently LogPager::enforceActionRestrictions() and LogPager::enforcePerformerRestrictions() are reversed (The former does the latter and the latter does the former). This means that the wrong types of restrictions were being enforced. Additionally LogPager::limitType is not calling enforceActionRestrictions, which it should.

So basically, if you searched for a specific log type, it would include log entries of that type where the log type was revision deleted. If you searched for a specific log title, it would filter out log entries where the author was deleted (instead of type like it was supposed to). If you searched for a specific author, it would filter out entries where the type is deleted (instead of author).

[discovered while looking into T187638]

Event Timeline

Bawolff created this task.Feb 23 2018, 9:42 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 23 2018, 9:43 PM

Further investigation suggests this was mostly introduced in ce881e02e8d6 (Although not filtering out DELETED_ACTION entries when searching by log_type appears to predate that commit by quite a bit)

Reedy updated the task description. (Show Details)Feb 23 2018, 10:01 PM
Anomie added a subscriber: Anomie.Feb 23 2018, 10:21 PM

Currently LogPager::enforceActionRestrictions() and LogPager::enforcePerformerRestrictions() are reversed

Yes, that does seem to be the case.

Additionally LogPager::limitType is not calling enforceActionRestrictions, which it should.

I'm not sure that DELETED_ACTION, despite the name, is actually supposed to hide the log_type or log_action. It seems to fairly consistently hide just the title, revision, and parameters.

Exceptions are:

  • ApiQueryRecentChanges hides both type and action.
  • ApiQueryWatchlist too, although anything with an unviewable action won't even get to that point since it has to be filtered out entirely to hide the title.
  • Various log formatters effectively hide the action in getActionLinks() and getActionText(), but not necessarily the type (that would be up to the caller, I suppose). OTOH, they may only be doing that because the action message and action links would reveal the title, revision, and/or parameters. They don't hide either elsewhere.

If you do want it to hide the log_type and/or log_action, there are a lot more places in the code that you'll need to change.

Various log formatters effectively hide the action in getActionLinks() and getActionText(), but not necessarily the type (that would be up to the caller, I suppose). OTOH, they may only be doing that because the action message and action links would reveal the title, revision, and/or parameters. They don't hide either elsewhere.

In practise, when I was testing, log_type was hidden from view when DELETED_ACTION was set for the types of logs I had tested (Although I didn't test that extensively).

So basically the big place where log_type is disclosed is the non-api version of Special:Recentchanges. Ultimately I believe consistency is much more important than what data is deleted. If DELETED_ACTION hides the log_action when viewing on Special:Log, then it should be hidden everywhere. If its not hidden everywhere, than it should be visible everywhere. Otherwise users might have incorrect expectations over how the feature works, and end up disclosing data they thought they didn't

What other places is log_type (deleted or not) shown other than Special:Log (and calls to LogEventsList), RC and watchlist? I can't really think of other places where logs are shown at all.

If DELETED_ACTION hides the log_action when viewing on Special:Log, then it should be hidden everywhere.

Except it doesn't, until you're thinking of changing it now.

I note that even though Special:Log doesn't display the action-based i18n message, the action is still revealed in the data-mw-logaction property. So even there it's not currently being hidden.

Hmm. I still maintain it should be made consistent (either shown everywhere including i18n messages or shown nowhere), but its clearly a separate issue from the main bug, that should be talked about separately (perhaps as a non-security bug). I'll remove that part of the patch.

version 2 of patch

Hmm. I still maintain it should be made consistent (either shown everywhere including i18n messages or shown nowhere)

As I said, it's probably not displaying the i18n message because to do so would require exposing the log entry's parameters. And whoever wrote the code probably didn't think it was important enough to either create special no-parameters messages for each type+action or to worry about creating a generic message to somehow expose the type and/or action in a user-friendly manner (i.e. not "delete/delete").

version 2 of patch

Code-Review+1

Since this issue was not present in any MediaWiki stable releae, I am making this public immediately.

Bawolff changed the visibility from "Custom Policy" to "Public (No Login Required)".Mar 5 2018, 10:47 PM

Change 416595 had a related patch set uploaded (by Brian Wolff; owner: Brian Wolff):
[mediawiki/core@master] SECURITY: Fix revdel checks in LogPager

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

Bawolff closed this task as Resolved.Mar 5 2018, 10:52 PM
Bawolff claimed this task.

For reference also:

[22:44]	logmsgbot	!log bawolff@tin Synchronized php-1.31.0-wmf.23/includes/logging/LogPager.php: T188145 (duration: 00m 58s)

Change 416595 merged by jenkins-bot:
[mediawiki/core@master] SECURITY: Fix revdel checks in LogPager

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