Page MenuHomePhabricator

CentralNotice provides a means for non interface-admins to bypass new CSS/JS restrictions
Closed, ResolvedPublic

Description

As was pointed out in T171987, using CentralNotice can still be used by the 90+ meta admins and meta central notice admins to inject javascript to all projects. T190015 states that "Javascript in CentralNotice banners" is not covered in the new restrictions.

How can this vector be reduced?

Event Timeline

T135963 is relevant, I think? If the plan is to completely remove inline JS everywhere, CentralNotice's JS system will need to be reworked anyway, as the current system is to have all JS inline, I think. (Including using copy-pasted code for the close button, IIRC.)

T135963 is relevant, I think? If the plan is to completely remove inline JS everywhere, CentralNotice's JS system will need to be reworked anyway, as the current system is to have all JS inline, I think. (Including using copy-pasted code for the close button, IIRC.)

That's no longer the near term plan (Eventually yes,but in the near term we're just planning to get rid of external js not inline js)

Maybe CN should ony work for CN- & Meta-Admins if 2FA is enabled.

Maybe CN should ony work for CN- & Meta-Admins if 2FA is enabled.

AFAIK there's no way currently to enforce 2FA on user(groups); although work is planned for that: T150562: Be able to force OATHAuth for certain user groups.

@MarcoAurelio I don't think @Steinsplitter was asking about forcing the group to use it, but to change the CN editing process to reject users that don't have 2FA in addition to other permissions?

One obvious thing would be to limit who can edit central notices (do all meta admins truly do that?).

Ideally editing central notices and editing central notices with raw html should be separate privileges, but that's probably not as easy to achieve.

Hi! Just a few notes here:

  • Inline JS is used quite a lot in CentralNotice banners, for both community and Fundraising campaigns. However, many campaigns don't really need it.
  • The reason Meta Admins can edit CentralNotice banners is that banners are stored as Mediawiki namespace pages. So, anyone with the ability to edit interface messages (i.e. Admins) can edit banners.

Certainly this is something that we could work on mitigating, say, by moving banners to their own namespace, and/or by making an additional type of CentralNotice banner that doesn't allow inline JS, and reducing the number of people who can edit banners that do include JS. Neither change would be quick, though...

  • The reason Meta Admins can edit CentralNotice banners is that banners are stored as Mediawiki namespace pages. So, anyone with the ability to edit interface messages (i.e. Admins) can edit banners.

With https://gerrit.wikimedia.org/r/c/mediawiki/core/+/449626, it is now possible to define specific MediaWiki namespace pages as requiring 'editsitejs' rights to edit, so that might be a relatively easy way to patch this (maybe you'd need to add a hook or something).

At a glance, banners are MediaWiki namespace pages prefixed with Centralnotice-template-, so it's easy to set custom rights requirements for editing them.
The more problematic part is that banners are raw HTML, and there is no easy way to filter out Javascript from that; to make them safe, we'd have to limit them to wikitext which might be problematic and in any case a nontrivial change.

Still, limiting the number of users who can edit them seems like an easy first step. Probably not all meta admins need access?

@matmarex @Tgr Interesting! Well, if permission changes are pretty easy and not a risk to CentralNotice infrastructure ahead of the year-end fundraising campaigns, I'm ok with it!!! Moving CN pages out of the Mediawiki namespace is probably eventually a good idea (for previous discussions, see T33595 and this RFC). And, in general, the CentralNotice backend and administration interface need quite a lot of reworking--it has to be carefully planned and executed, though.

Creating a "JS/CSS-free" type of banner and further restricting editing rights for banners with raw injectible content is a smaller change, but I'd still ask that it be done in the first half of a calendar year.

Adding @Seddon and @DStrine for comments on community and product aspects of this (especially @Tgr's suggestion to restrict Meta Admins from editing CN banners without moving them to a different namespace). Thanks!!!!

I would be extremely hesitant to further reduce the oversight of use of the CentralNotice by removing Meta admins from those who can edit the CentralNotices.

Note that there is no serious additional security risk from allowing Meta admins to disable (or enable) existing banners. Splitting off CentralNotice JS into separate wiki pages, even if it were still outputted as inline JS, could allow admins to safely edit CentralNotices without risk as well. (This would also have the bonus of making it much easier to review JS, something that is seriously needed.)

Tgr added a subtask: Restricted Task.Nov 2 2018, 11:48 PM
Ejegg closed subtask Restricted Task as Resolved.Dec 12 2018, 3:21 AM
chasemp added a project: Security-Team.