Page MenuHomePhabricator

Unescaped messages in WikibaseMediaInfo
Closed, ResolvedPublicSecurity

Description

The following messages are used unescaped:

wikibasemediainfo-filepage-captions-title
wikibasemediainfo-filepage-structured-data-heading
wikibasemediainfo-filepage-caption-too-short
wikibasemediainfo-filepage-caption-too-long
wikibasemediainfo-filepage-caption-empty
wikibasemediainfo-time-timestamp-empty
wikibasemediainfo-time-timestamp-invalid
wikibasemediainfo-time-timestamp-formatted

the first two are used on every file page so the impact is quite high, but you need to be able to edit the messages so the risk is low.
Will upload a patch shortly.

Event Timeline

Patch below:

sbassett changed the task status from Open to In Progress.Jan 24 2022, 9:58 PM
sbassett claimed this task.
sbassett moved this task from Security Patch To Deploy to In Progress on the Security-Team board.
sbassett added subscribers: gerritbot, sbassett.

Triaged during the Security-Team clinic this morning. Like other tasks related to this issue (which used to be more consistently tracked at T2212), this should just be able to go through gerrit, ideally merged on a Monday prior to the weekly train cut.

sbassett triaged this task as Medium priority.Jan 24 2022, 9:59 PM
sbassett changed Author Affiliation from N/A to Wikimedia Communities.
sbassett changed Risk Rating from N/A to Medium.

Change 756709 had a related patch set uploaded (by SBassett; author: Dylsss):

[mediawiki/extensions/WikibaseMediaInfo@master] Escape various messages in WikibaseMediaInfo

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

Change 756709 merged by jenkins-bot:

[mediawiki/extensions/WikibaseMediaInfo@master] Escape various messages in WikibaseMediaInfo

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

So I merged a slightly uglier version of @Dylsss' patch that should function just the same, just to get this merged for wmf.19. Unfortunately, the original patch wasn't passing various qunit tests due to the escaped() method not being found on a few alleged mw.message objects - see previous failed jenkins jobs on the change set. It likely makes sense for someone more familiar with the WikibaseMediaInfo code to revisit these changes and optimize them a bit more, perhaps with ternary conditional statements like message.exists() ? message.escaped() : undefined, similar to other conditionals found elsewhere within the code.

Change 756994 had a related patch set uploaded (by SBassett; author: Dylsss):

[mediawiki/extensions/WikibaseMediaInfo@wmf/1.38.0-wmf.19] Escape various messages in WikibaseMediaInfo

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

Change 756994 merged by jenkins-bot:

[mediawiki/extensions/WikibaseMediaInfo@wmf/1.38.0-wmf.19] Escape various messages in WikibaseMediaInfo

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

@sbassett Hi, your modified patch doesn't seem to be working properly. The file information tab on Commons files is no longer visible:

image.png (371×748 px, 34 KB)

with the following error in the browser console:

TypeError: s.replace is not a function

This breakage was noted on the unofficial Discord server, copying AntiComposite's analysis here (with permission) if it makes fixing easier:

AntiComposite — Today at 19:27
https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/WikibaseMediaInfo/+/083c8409209ee03bc58c38574858de4b44b316ad%5E%21/resources/filepage/CaptionsPanel.js 
mw.message returns a mw.Message object, not a string. but mw.html.escape expects a string. it tries to do do s.replace, but mw.Message doesn't have a replace method. Probably best to use mw.message.escaped() instead of DIYing it. I'll put in a patch

AntiComposite — Today at 19:43
https://gerrit.wikimedia.org/r/c/mediawiki/extensions/WikibaseMediaInfo/+/756709/2..3
changeset 2 used mw.message.escaped(), changeset 3 changed that to the broken mw.html.escape(mw.message)
because mw.message.escaped was throwing TypeErrors in the tests...

Thoughts on rolling back train or patching?

Change 757484 had a related patch set uploaded (by SBassett; author: SBassett):

[mediawiki/extensions/WikibaseMediaInfo@master] Revert \"Escape various messages in WikibaseMediaInfo\"

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

Thoughts on rolling back train or patching?

I've created a revert ^ for now, which should test fine and then can be picked to wmf.19.

Change 757485 had a related patch set uploaded (by Brennen Bearnes; author: SBassett):

[mediawiki/extensions/WikibaseMediaInfo@wmf/1.38.0-wmf.19] Revert \"Escape various messages in WikibaseMediaInfo\"

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

Thanks. I can deploy that once tests finish.

Change 757484 merged by jenkins-bot:

[mediawiki/extensions/WikibaseMediaInfo@master] Revert \"Escape various messages in WikibaseMediaInfo\"

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

Maybe I'm missing something very obvious, but I am still not really sure why the tests were throwing errors on the original patch. When I tested the patch locally everything worked properly and fixed the XSS, and I didn't see any visible issues or errors in the console.

Maybe I'm missing something very obvious, but I am still not really sure why the tests were throwing errors on the original patch. When I tested the patch locally everything worked properly and fixed the XSS, and I didn't see any visible issues or errors in the console.

Probably WikibaseMediaInfo's custom mocked stub for the mw object needs updating: https://gerrit.wikimedia.org/g/mediawiki/extensions/WikibaseMediaInfo/+/8d65ac3bb75fdcb0247e9756645a58cad46a0466/tests/node-qunit/support/helpers.js#57 (/me wonders if that's really the best way to implement that)

Change 757485 merged by jenkins-bot:

[mediawiki/extensions/WikibaseMediaInfo@wmf/1.38.0-wmf.19] Revert \"Escape various messages in WikibaseMediaInfo\"

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

Revert tested and appears working; deployed.

Thanks, @brennen. Sorry about that.

Regarding the bug, I'm pretty sure I meant to make those mw.msg calls instead, but got distracted and since it tested fine in gerrit, just merged it. I'd guess this approach would work as a quick fix, though obviously that isn't ideal.

Maybe I'm missing something very obvious, but I am still not really sure why the tests were throwing errors on the original patch. When I tested the patch locally everything worked properly and fixed the XSS, and I didn't see any visible issues or errors in the console.

Your patch absolutely should have worked fine as-is. There are many, many instances of mw.message( ... ).escaped() within other Wikimedia codebases. I'm guessing @Majavah's findings are the likely culprit. There probably just needs to be an escaped mock added here.

Your patch absolutely should have worked fine as-is. There are many, many instances of mw.message( ... ).escaped() within other Wikimedia codebases. I'm guessing @Majavah's findings are the likely culprit. There probably just needs to be an escaped mock added here.

This appears to be the problem: testing locally with @Dylsss' first patch and adding an escaped property to the mw mock, the tests run fine. Gerrit patch coming soon for this issue, then we can likely send the first patch through gerrit as well.

Change 757726 had a related patch set uploaded (by SBassett; author: SBassett):

[mediawiki/extensions/WikibaseMediaInfo@master] Add escaped mock to mw helper object for qunit tests

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

Change 757726 merged by jenkins-bot:

[mediawiki/extensions/WikibaseMediaInfo@master] Add escaped mock to mw helper object for qunit tests

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

Thanks for the +2, @Majavah. I'm going to send up @Dylsss's original patch here, which should test fine now.

Change 757954 had a related patch set uploaded (by SBassett; author: Dylsss):

[mediawiki/extensions/WikibaseMediaInfo@master] Escape various messages in WikibaseMediaInfo

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

Change 757954 merged by jenkins-bot:

[mediawiki/extensions/WikibaseMediaInfo@master] Escape various messages in WikibaseMediaInfo

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

sbassett moved this task from In Progress to Our Part Is Done on the Security-Team board.
sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".
sbassett changed the edit policy from "Custom Policy" to "All Users".