Last modified: 2014-11-18 00:08:48 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T73621, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 71621 - Not loading site CSS on Special:UserLogin/Preferences breaks wikis which use it to create a skin/theme
Not loading site CSS on Special:UserLogin/Preferences breaks wikis which use ...
Status: PATCH_TO_REVIEW
Product: MediaWiki
Classification: Unclassified
User login and signup (Other open bugs)
1.23.5
All All
: Normal major (vote)
: ---
Assigned To: Nobody - You can work on this!
: code-update-regression
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2014-10-03 19:51 UTC by Alexia E. Smith
Modified: 2014-11-18 00:08 UTC (History)
17 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---
legoktm.wikipedia: Backport_to_Stable?


Attachments

Description Alexia E. Smith 2014-10-03 19:51:16 UTC
Bug 70672 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
Comment 1 Kunal Mehta (Legoktm) 2014-10-03 19:54:48 UTC
Also bawolff's email: https://lists.wikimedia.org/pipermail/wikitech-l/2014-October/078903.html
Comment 2 Zack 2014-10-04 13:44:30 UTC
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.
Comment 3 ebrahim 2014-10-04 16:17:55 UTC
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.
Comment 4 Gerrit Notification Bot 2014-10-10 07:00:50 UTC
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
Comment 5 Krinkle 2014-10-10 18:44:51 UTC
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.
Comment 6 Matt Russell 2014-10-11 07:13:32 UTC
(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.
Comment 7 Kunal Mehta (Legoktm) 2014-10-12 00:18:24 UTC
(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.
Comment 8 [[kgh]] 2014-10-22 12:50:56 UTC
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.
Comment 9 Krinkle 2014-10-22 13:33:53 UTC
(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).
Comment 10 Alexia E. Smith 2014-10-22 14:06:54 UTC
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.
Comment 11 Krinkle 2014-10-22 14:18:08 UTC
(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).
Comment 12 Krinkle 2014-10-22 14:19:29 UTC
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.
Comment 13 Subfader 2014-10-29 21:48:15 UTC
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...
Comment 14 Mark A. Hershberger 2014-10-30 15:40:22 UTC
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.
Comment 15 Gerrit Notification Bot 2014-11-09 21:51:01 UTC
Change 165979 merged by jenkins-bot:
Make allowing site-wide styles on restricted special pages a config option

https://gerrit.wikimedia.org/r/165979
Comment 16 [[kgh]] 2014-11-10 12:40:44 UTC
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
Comment 17 Gerrit Notification Bot 2014-11-17 23:52:30 UTC
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
Comment 18 Gerrit Notification Bot 2014-11-17 23:55:45 UTC
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
Comment 19 Gerrit Notification Bot 2014-11-18 00:01:49 UTC
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
Comment 20 Kunal Mehta (Legoktm) 2014-11-18 00:08:48 UTC
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.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links