Page MenuHomePhabricator

Administrators can no longer view deleted history of js/css pages
Open, MediumPublic

"The World Burns" token, awarded by Riley_Huntley."Evil Spooky Haunted Tree" token, awarded by matej_suchanek."Cookie" token, awarded by Xaosflux."Heartbreak" token, awarded by BethNaught.
Assigned To
Authored By
Xaosflux, Aug 28 2018


Following T190015, sysops can no longer use their deletedhistory/deletedtext access on css/js pages.

While it is expected that non-interface administrators should not be able to undelete jss/css pages, we do expect that they may still view the deleted history, and even advertise it in the page notice.

You do not have permission to view a page's deleted history, for the following reason:
The action you have requested is limited to users in one of the groups: Administrators, Researchers.

Example can be seen here: (assuming you are a testwiki admin, and not something extra like an interface admin)


Related Gerrit Patches:

Event Timeline

Xaosflux triaged this task as High priority.Aug 28 2018, 1:23 PM
Xaosflux created this task.
Restricted Application removed a project: Patch-For-Review. · View Herald TranscriptAug 28 2018, 1:23 PM
Aklapper removed Tgr as the assignee of this task.Aug 28 2018, 1:36 PM

Please remove incorrect project tags or assignees when creating tasks.

Tgr added a comment.Aug 28 2018, 1:58 PM

Is that a problem? I was wondering about it when I wrote the patch for deletion, but since administrators should not be able to undelete, viewing deleted revisions did not seem to be particularly useful.

(The error message is buggy. In general, permission error messages in MediaWiki don't support "dynamic" permisssions (which depend on something other than the user, e.g. the target page) all that well. MediaWiki will call something like $title->userCan( 'viewdeleted', $user ), which will return false, and then it will blindly assume that the problem is that the user is not in a group that has viewdeleted in general, and show an error message with those groups. T180888: All permission checks should be able to return a custom error message is more or less the related task.)

It is a bit odd that the new rights for editing are being applied to viewing deleted history too, and that should probably be fixed. I don't know that it's really "high" priority as it's currently triaged.

Xaosflux lowered the priority of this task from High to Medium.Aug 28 2018, 2:06 PM

I think its a problem, as it gives interface administrator the ability to hide revisions from most everyone (oh noes its 'superdelete') - limiting the transparency of what they did.

It is a problem: firstly because of transparency, per Xaosflux, and secondly because it is a change that was not advertised in the consultation. This change was about removing the ability to edit, not the ability to view; why should that not also apply to deleted pages?

Is that a problem? I was wondering about it when I wrote the patch for deletion

Then why not ask beforehand?

stjn added a comment.Aug 28 2018, 2:33 PM

A problem lies here: should administrators see deleted revisions of those files if no ‘pure interface administrator’ can? I don’t really have a preference, but it would be kind of strange to provide more privileges to non-intadmins than to intadmins.

Xaosflux added a comment.EditedAug 28 2018, 2:36 PM

@stjn, FYI if you are an intadmin, and NOT also an admin (or otherwise a holder of viewdelete) - you can't see these deleted pages either; I expected that behavior - just FYI

stjn added a comment.Aug 28 2018, 2:37 PM

This is what I intended to convey with the comment above.

Teles added a comment.Aug 28 2018, 2:42 PM

So, both admins and IntAdmins can’t see delete reviews? Sorry if I didn’t get it right. If that is correct, is there any other local right that can see it or only advanced global rights will have this permission?

@Teles right now on most configuration you must be sysop AND intadmin to use viewdelete on jss/css pages.

Tgr added a comment.Aug 28 2018, 2:49 PM

It is a bit odd that the new rights for editing are being applied to viewing deleted history too

Technically, they are the rights for doing anything with a CSS/JS page. I followed how user subpages are handled - without edituserjs, you cannot do anything other than read. In hindisght, that's a but weird - for example, it would block FlaggedRevs from working on such a page, and there isn't really any reason for that.
OTOH I'm not sure how much MediaWiki can be trusted to have sane permission hierarchies (if you can't edit, you shouldn't be able to undelete either etc), and for something as security-critical as this, it makes sense to err on the side of caution.

See, I thought that having separate "view deleted stuff" and "delete and undelete stuff" permissions was exactly to cover such cases.

Why would there be any security issues with admins being able to view deleted js/css pages? The security issues surely come from edits being potentially harmful. If there truly are any revisions which must be hidden from almost everyone's view then isn't that what Oversight is for?

The potential security issue I can see is a malicious admin recovering attack/exploit code for reuse elsewhere, or an "innocent" admin being asked, badgered, coerced, etc. to do so, though in either case, oversight of the revisions in question would be sufficient to prevent this, as @Thryduulf pointed out.

Xaosflux added a comment.EditedAug 28 2018, 5:56 PM

In order for oversighters (who are in almost all cases also admin) to suppress they will need to be able to see the deleted history, which they can not now.

As far as this "view" vs "edit" - without this access we will need to expand interface admin to all sorts of non-technical editors such as: oversighters, members of arbitration committees, etc.

Tgr added a comment.EditedAug 28 2018, 6:25 PM

The security issue is that some extension provides some functionality which is essentially a way to edit pages, and provides its own user right for it, and only checks for that user right and not for edit. Title::checkUserConfigPermissions() was written in a very trigger-happy way to prevent that (or at least I assume that's the reason) and I followed suit with Title::checkSiteConfigPermissions().

Making exceptions for deletedhistory/deletedtext/viewsuppressed is safe, it's just kind of inelegant.

To me that sounds like a problem with these permissions, then. I think that any extension should check for the relevant user rights when it needs to check, not for some related user right.

What are the next steps here - will someone from the dev community claim this and work on it?

Tgr claimed this task.Aug 29 2018, 12:20 PM
Tgr added a comment.Aug 29 2018, 12:24 PM

In the short term let's whitelist deletion-related read rights. In the longer term I'd like to hear the opinion of someone from the Security team (ping @Bawolff, @Reedy).

Change 456140 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/core@master] Fix CSS/JSON/JS deleted-view permissions

MBH removed a subscriber: MBH.Aug 29 2018, 1:48 PM
jrbs moved this task from Backlog to Other team on the Trust-and-Safety board.Sep 1 2018, 12:04 AM
1997kB removed a subscriber: 1997kB.Jan 25 2019, 3:13 PM
Tgr moved this task from Backlog to Next on the User-Tgr board.
Krinkle removed a subscriber: Krinkle.Feb 10 2020, 11:41 PM
Chenzw added a subscriber: Chenzw.Feb 27 2020, 3:05 PM