Page MenuHomePhabricator

Not loading site CSS on Special:UserLogin/Preferences breaks wikis which use it to create a skin/theme
Closed, ResolvedPublic

Description

Bug 70672 (T72672) fixes the security hole of allowing Javascript in CSS in the Mediawiki namespace. It does this by breaking the functionality of loading CSS when on the Special:UserLogin and Special:Preferences pages. Unfortunately this means that any custom styles are not loaded. To the end user it causes momentarily confusion that they may have been maliciously redirected to a different site to enter their username and password. This is an undesirable side effect for the user interface.

I have created an example extension that will prevent saving any custom CSS that contains Javascript imports.
https://github.com/Alexia/Bug70672

Example error output:
http://imgur.com/a/TnsTY#0

Original bug:
https://bugzilla.wikimedia.org/show_bug.cgi?id=70672


Version: 1.23.5
Severity: major
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=70672

Details

Reference
bz71621

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 3:43 AM
bzimport set Reference to bz71621.
bzimport added a subscriber: Unknown Object (MLST).

Many scripts/styles run on a site level; it does not make sense that they do not work on two pages. CSS is used e.g. in Arabic Wikipedia to adjust general font size and fix a bunch of directionality issues.

Yes, this is needed on fa, ckb, arz, glk, mzn, ps, pnb. Why? Because all of these wikis are overriding default browser defined sans-serif font on their Common.css and it is needed their UI would be consistent on preference and eventually login/logout also.

gerritadmin wrote:

Change 165979 had a related patch set uploaded by Legoktm:
Re-enable site-wide styles on Special:Preferences/UserLogin

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

This would result in loading only part of a module, which is in my opinion against expectations.

I don't like escalating a relatively simple bug into a social problem, but I think this is one of those cases where that is appropriate.

Maybe we should discourage people from theming their wiki to this extreme via this method? Third parties should add a stylesheet via LocalSettings instead of Common.css.

This should be done by a developer instead (especially for third parties).

For Wikimedia sites, I'd like to think that, while it's kind of undocumented, that people really should not significantly change the site interface. Users should not be able to notice a difference outside the content area when the styles are not loaded.

That thing about people thinking it's a different site, that goes both ways. When they visit that different language edition, should that be allowed to look like a completely different website?

Main reason being that the software interface is provided by MediaWiki core. If there are problems there, they should be reported to the software and addressed accordingly. Things can be iterated and tried in gadgets, but for something so central to the software, it either shouldn't be done (e.g. bad idea), or should be done (good idea) and done in the software itself so that it may benefit a wider audience (and usually a higher quality result in terms of browser support, user experience, performance and maintainability).

Something as fundamental as the site font, for example. That's either a personal preference one could question whether it's responsible for users to override, or there's a technical reason (eg. their wiki's language doesn't render well in the font we choose by default) - in which case we shouldn't put that burden on them. By all means that is a high priority problem for the foundation and MediaWiki software to address.

There have also been proposals in the past to technically restrict the ability of MediaWiki:Common.css to affect anything outside page content, but that hasn't gotten anywhere. And I'm also not convinced that that'd be a good thing. There's plenty of grey area where it's technically outside the content area, but part of a larger customisation that doesn't interfere with the software interface.

(In reply to Krinkle from comment #5)

Maybe we should discourage people from theming their wiki to this extreme
via this method? Third parties should add a stylesheet via LocalSettings
instead of Common.css.

People use Common.css and Skin.css to theme their wikis only because MediaWiki doesn't provide any proper way to do it on-wiki.
For a single site that you're running yourself, making or installing a custom skin is good. But for wiki farms where individual communities run wikis where they do not have FTP access, they need the theme to be editable on-wiki.

(In reply to Krinkle from comment #5)

This would result in loading only part of a module, which is in my opinion
against expectations.

We already serve only the CSS of the module to no-JS users, so it's not totally against expectations.

I don't like escalating a relatively simple bug into a social problem, but I
think this is one of those cases where that is appropriate.

Maybe we should discourage people from theming their wiki to this extreme
via this method? Third parties should add a stylesheet via LocalSettings
instead of Common.css.

This should be done by a developer instead (especially for third parties).

Part of the problem is that AFAIK there is no good/easy way to do that outside of a) writing your own skin (overkill in most cases), b) directly editing the skin stylesheets, which we really don't want people to do.

For Wikimedia sites, I'd like to think that, while it's kind of
undocumented, that people really should not significantly change the site
interface. Users should not be able to notice a difference outside the
content area when the styles are not loaded.

That thing about people thinking it's a different site, that goes both ways.
When they visit that different language edition, should that be allowed to
look like a completely different website?

Main reason being that the software interface is provided by MediaWiki core.
If there are problems there, they should be reported to the software and
addressed accordingly. Things can be iterated and tried in gadgets, but for
something so central to the software, it either shouldn't be done (e.g. bad
idea), or should be done (good idea) and done in the software itself so that
it may benefit a wider audience (and usually a higher quality result in
terms of browser support, user experience, performance and maintainability).

Something as fundamental as the site font, for example. That's either a
personal preference one could question whether it's responsible for users to
override, or there's a technical reason (eg. their wiki's language doesn't
render well in the font we choose by default) - in which case we shouldn't
put that burden on them. By all means that is a high priority problem for
the foundation and MediaWiki software to address.

There have also been proposals in the past to technically restrict the
ability of MediaWiki:Common.css to affect anything outside page content, but
that hasn't gotten anywhere. And I'm also not convinced that that'd be a
good thing. There's plenty of grey area where it's technically outside the
content area, but part of a larger customisation that doesn't interfere with
the software interface.

Those might be reasonable changes for the future, but I don't think it's okay to do such a major change without proper notice/release notes, and definitely not appropriate to do in a security patch.

So what's next? How do we get all the affected wikis out of their current misery? This bug should not be a back-burner thing as it looks like at the moment.

I think the MW software should allow wikis to adapt the overall appearance without having to use a custom skin. So far the easiest way was to use Common.css/js etc. which is no longer working for integral pages such as the login and preferences.

I think the proposed change tries to address this issue. Another possibility may probably be an extension that allows placing custom CSS- and/or JS-file(s) on the server which is then packed into a resource loader module loaded by the extension. This may perhaps also be a setting in MW core which allows to point to the respective file(s) and then does the same job. Perhaps there are other ways.

(In reply to Kunal Mehta (Legoktm) from comment #7)

(In reply to Krinkle from comment #5)

This would result in loading only part of a module, which is in my opinion
against expectations.

We already serve only the CSS of the module to no-JS users, so it's not
totally against expectations.

No we don't. Only for exceptional modules that are designed specifically to be a base module without javascript, loaded explicitly via addModuleStyles, thus bypassing ResourceLoader. If you're loading a regular module containing javascript files via addModuleStyles, you're doing it wrong. It doesn't throw an exception for that right now, but we should if that helps.

When writing css rules in a stylesheet loaded by ResourceLoader one should be able to assume javascript execution alongside of it.

(In reply to Kunal Mehta (Legoktm) from comment #7)

Those might be reasonable changes for the future, but I don't think it's
okay to do such a major change without proper notice/release notes, and
definitely not appropriate to do in a security patch.

I agree. But on the other hand one could also argue we never supported this in the first place. The community never asked to make usability and design decisions for the software interface. And contrary to the usual case, it's actually smaller wikis that do this, not the larger language editions of Wikipedia. Which I suspect might be due to lack of peer review and a wider audience to notice the (possibly negative) impact of a such a change.

I'm not arguing that though. It's been too years since Common.css was added and these wikis started doing it to take it back now. Should the community ask for a feature if there's an existing exploit they can use to emulate a requested feature? (e.g. if Common.css was limited to content area and content pages, this would never be possible and we'd have to consider it as an actual feature, which we probably wouldn't allow for good reasons on WMF. The intent to improve the interface is completely valid however and we'd actively help those wikis improve their interface from the software perspective instead, it's merely about the implementation details here, not about the actual visual changes to interface).

The main issue is that it is such a long standing feature that breaking it in security patch without notification to end users is not acceptable. Then dragging along not issuing a recall on the patch or providing a corrected patch is also not acceptable.

(In reply to Krinkle from comment #9)

(In reply to Kunal Mehta (Legoktm) from comment #7)

(In reply to Krinkle from comment #5)

This would result in loading only part of a module, which is in my opinion
against expectations.

We already serve only the CSS of the module to no-JS users, so it's not
totally against expectations.

No we don't. Only for exceptional modules that are designed specifically to
be a base module without javascript, loaded explicitly via addModuleStyles,
thus bypassing ResourceLoader. If you're loading a regular module containing
javascript files via addModuleStyles, you're doing it wrong. It doesn't
throw an exception for that right now, but we should if that helps.

When writing css rules in a stylesheet loaded by ResourceLoader one should
be able to assume javascript execution alongside of it.

For the 'site' module you are absolutely right of course. In general a module is either executed regularly (addModules/mw.loader.load) or styles only (e.g. Vector skin; addModuleStyles/load.php only=styles). In case of the 'site' wiki-page module, it is loaded with both addModuleStyles and addModuleScripts separately because the scripts have to execute in the global javascript context for legacy reasons, and Common.css historically is both the companion of Common.js as well as the standalone stylesheet for wiki content (e.g. infobox, or fallback styles for collapsible elements).

For the record, these last few comments were mostly to provide more context, data and factor of influence. I don't feel it's appropriate for me to make a decision about this.

Is there no really easier way to suppress disasbling MediaWiki NS CSS when Special:Preferences is loaded?

Only I can edit the MW NS. I see no risk...

Kunal's fix here makes sense to me. Perhaps the ideal would be that everyone who sets up a wiki has full time developers available to help style their site, but the fact is that CSS and JS are known and used by amateurs to produce the effects they want. Restricting that ability causes problems for end users.

As far as security, it makes sense to keep user js/css off these pages, but MediaWiki: namespaced js/css should be available and is, by default, more tightly controlled.

Since the problem that is solved here is visible on WikiApiary, I'm going to ask Jamie Thingelstad to test the patch there and merge it if it solves the problem.

gerritadmin wrote:

Change 165979 merged by jenkins-bot:
Make allowing site-wide styles on restricted special pages a config option

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

I believe that this is great news and having a configuration setting for this [1] is great and very much acceptable. Thanks a ton for making this possible!

Since this went to master I think this change should be backported to the 1.19, 1.23 and 1.24 branches.

[1] https://www.mediawiki.org/wiki/Manual:$wgAllowSiteCSSOnRestrictedPages

gerritadmin wrote:

Change 174012 had a related patch set uploaded by Legoktm:
Make allowing site-wide styles on restricted special pages a config option

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

gerritadmin wrote:

Change 174014 had a related patch set uploaded by Legoktm:
Make allowing site-wide styles on restricted special pages a config option

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

gerritadmin wrote:

Change 174018 had a related patch set uploaded by Legoktm:
Make allowing site-wide styles on restricted special pages a config option

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

I uploaded patches for REL1_24, REL1_23, and REL1_22. I started on REL1_19 and then it got all scary so I stopped.

I'm not sure what to do about @since 1.25 tags, so I left them as they are.

Change 175671 had a related patch set uploaded (by Mglaser):
Make allowing site-wide styles on restricted special pages a config option

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

Patch-For-Review

Change 174014 merged by Mglaser:
Make allowing site-wide styles on restricted special pages a config option

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

Change 174018 merged by Mglaser:
Make allowing site-wide styles on restricted special pages a config option

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

Change 175671 merged by jenkins-bot:
Make allowing site-wide styles on restricted special pages a config option

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

Aklapper set Security to None.
Aklapper subscribed.

I assume this can be closed as fixed now?

Legoktm claimed this task.

I assume this can be closed as fixed now?

Yes!