Page MenuHomePhabricator
Create Task
Maniphest T280929

SkinModule for Vector (new and legacy) is missing toc/print styles (outputs raw server path instead)
Closed, ResolvedPublic
Actions

Description

As we were investigating T280428 we noticed that ResourceLoader output at https://fa.wikipedia.org/w/load.php?lang=fa&only=styles&skin=vector&modules=skins.vector.icons%2Cstyles contains an @import statement that is followed by the internal path of a file on WMF production servers:

excerpt of RL output to the users browser,lang=css
@import '/srv/mediawiki/php-1.37.0-wmf.1/resources/src/mediawiki.skinning/toc/print.css'

Internal paths should never leak into the client-side output of RL. Not only this could be a security risk (particularly for non-WMF wikis), but also these paths are not usable by the client.

Not tagging this task as Security because what is leaked here is of no risk to WMF. But if it should be tagged as such in the interest of non-WMF wikis, please modify.

Details

SubjectRepoBranchLines +/-
mediawiki.skinning/commonPrint.less: Import CSS with (inline)mediawiki/coreREL1_36+7 -0
mediawiki.skinning/commonPrint.less: Import CSS with (inline)mediawiki/coremaster+2 -1
Customize query in gerrit

Related Objects

Event Timeline

Huji created this task.Apr 22 2021, 5:50 PM
Restricted Application added a project: Performance-Team. · View Herald TranscriptApr 22 2021, 5:50 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Krinkle edited projects, added Web-Team-Backlog, Performance-Team (Radar); removed Performance-Team, MediaWiki-ResourceLoader.Apr 22 2021, 6:58 PM
Krinkle moved this task from Inbox to External on the MediaWiki-ResourceLoader board.
Krinkle subscribed.

From what I can tell this can only be an invalid instruction caused by bad input. The system appears to be working fine. This problem is reproducible locally as well on a plain MediaWIki setup, it's not specific to production ResourceLoader or something like that.

Once the bad input is found and fixed, we can follow-up on perhaps providing a PHPCS sniff, stylelint rule, or PHPUnit structure test to flag such input ahead of time in CI.

Krinkle renamed this task from Internal server paths are leaked into RL output to SkinModule for Vector (new and legacy) is missing toc/print styles (outputs raw server path instead).Apr 22 2021, 6:59 PM
Krinkle added a project: MediaWiki-Core-Skin-Architecture.
Krinkle moved this task from Limbo to Watching on the Performance-Team (Radar) board.
Mainframe98 claimed this task.Apr 22 2021, 8:26 PM
Mainframe98 subscribed.

I think that's on me, caused by rMW5e7076f21e83: Move legacy commonPrint styles to appropriate skin features. I think I should've used something like @import (inline) to make the Less parser include the file, not output a CSS @import statement. I'll fix that tomorrow, so I'm tentatively claiming this. Feel free to unassign if urgent.

As for finding this in CI, the CSS/Less styling tool we use should be able to ban using @import in Less files with CSS files.

Huji added a comment.Apr 22 2021, 9:06 PM

@Mainframe98 with respect, I also invite you to see the comments on https://gerrit.wikimedia.org/r/c/mediawiki/core/+/681971 and make sure you review https://www.mediawiki.org/wiki/Manual:Coding_conventions/CSS#LESS on proper use of the @noflip declaration. It was not used correctly in rMW5e7076f21e83: Move legacy commonPrint styles to appropriate skin features

R4356th subscribed.Apr 23 2021, 3:24 AM
Mainframe98 added a comment.Apr 23 2021, 7:08 AM
In T280929#7028199, @Huji wrote:

@Mainframe98 with respect, I also invite you to see the comments on https://gerrit.wikimedia.org/r/c/mediawiki/core/+/681971 and make sure you review https://www.mediawiki.org/wiki/Manual:Coding_conventions/CSS#LESS on proper use of the @noflip declaration. It was not used correctly in rMW5e7076f21e83: Move legacy commonPrint styles to appropriate skin features

I wasn't involved in the toc related styles, only the print-related part (which was a separate change). I'm aware of the @noflip issue, though, I encountered that one with MediaWiki-skins-Mirage.

gerritbot added a comment.Apr 23 2021, 7:17 AM

Change 682085 had a related patch set uploaded (by Mainframe98; author: Mainframe98):

[mediawiki/core@master] Import CSS with (inline) in Less files

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

gerritbot added a project: Patch-For-Review.Apr 23 2021, 7:17 AM
ovasileva triaged this task as Medium priority.Apr 26 2021, 2:55 PM
ovasileva edited projects, added Web-Team-Backlog (Kanbanana-FY-2020-21); removed Web-Team-Backlog.
gerritbot added a comment.Apr 26 2021, 6:50 PM

Change 682085 merged by jenkins-bot:

[mediawiki/core@master] mediawiki.skinning/commonPrint.less: Import CSS with (inline)

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

gerritbot added a comment.Apr 26 2021, 6:52 PM

Change 682711 had a related patch set uploaded (by Jforrester; author: Mainframe98):

[mediawiki/core@REL1_36] mediawiki.skinning/commonPrint.less: Import CSS with (inline)

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

gerritbot added a comment.Apr 26 2021, 6:52 PM

Change 682711 abandoned by Jforrester:

[mediawiki/core@REL1_36] mediawiki.skinning/commonPrint.less: Import CSS with (inline)

Reason:

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

ReleaseTaggerBot added a project: MW-1.37-notes (1.37.0-wmf.3; 2021-04-27).Apr 26 2021, 7:00 PM
Maintenance_bot removed a project: Patch-For-Review.Apr 26 2021, 7:11 PM
Krinkle added a comment.Apr 26 2021, 7:15 PM

I filed https://github.com/wikimedia/stylelint-config-wikimedia/issues/134 as follow up to prevent this in the future.

Jdlrobson moved this task from Backlog to Phase 4 - Standardize ResourceLoaderSkinModules styling rules on the MediaWiki-Core-Skin-Architecture board.Apr 26 2021, 9:44 PM
Jdlrobson moved this task from Needs Analysis to Ready for Signoff on the Web-Team-Backlog (Kanbanana-FY-2020-21) board.Apr 26 2021, 9:53 PM
Jdlrobson closed this task as Resolved.Apr 29 2021, 7:56 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits