Page MenuHomePhabricator

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

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.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Krinkle moved this task from Inbox to External on the MediaWiki-ResourceLoader board.
Krinkle added a subscriber: Krinkle.

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 moved this task from Limbo to Watching on the Performance-Team (Radar) board.
Mainframe98 added a subscriber: Mainframe98.

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.

@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.

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

Change 682085 merged by jenkins-bot:

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

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

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

Change 682711 abandoned by Jforrester:

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

Reason:

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