Page MenuHomePhabricator
Create Task
Maniphest T395834

File::getLongDesc()/getShortDesc() is documented to return HTML, but some handlers return unescaped plain text
Closed, ResolvedPublicSecurity
Actions

Description

File::getLongDesc() is documented to return HTML, but the return value is treated as wikitext, and some handlers return unescaped plain text.

There is no security issue at the moment, since the wikitext parsing cleans up everything, but it seems like it's a bug waiting to happen.

Noticed in code review in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1152129/comment/7c010e3c_a286e36c/.

Details

Risk Rating
Medium
Author Affiliation
WMF Product
Related Changes in Gerrit:
SubjectRepoBranchLines +/-
Improve HTML escaping in getLongDesc(), getShortDesc() methodsmediawiki/coremaster+55 -19
Improve HTML escaping in getLongDesc(), getShortDesc() methodsmediawiki/extensions/TimedMediaHandlermaster+96 -148
Treat File::getShortDesc() as possibly unsafe HTMLmediawiki/coreREL1_44+7 -1
Treat File::getShortDesc() as possibly unsafe HTMLmediawiki/coreREL1_43+7 -1
Treat File::getShortDesc() as possibly unsafe HTMLmediawiki/coreREL1_42+7 -1
Treat File::getShortDesc() as possibly unsafe HTMLmediawiki/coreREL1_39+7 -1
Treat File::getShortDesc() as possibly unsafe HTMLmediawiki/corewmf/1.45.0-wmf.4+7 -1
Treat File::getShortDesc() as possibly unsafe HTMLmediawiki/corewmf/1.45.0-wmf.3+7 -1
Treat File::getLongDesc() as possibly unsafe HTML, not wikitextmediawiki/coremaster+26 -18
Treat File::getShortDesc() as possibly unsafe HTMLmediawiki/coremaster+7 -1
Customize query in gerrit

Event Timeline

matmarex created this task.Jun 2 2025, 6:09 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 2 2025, 6:09 PM
matmarex set Security to Software security bug.Jun 2 2025, 6:17 PM
matmarex added a project: Security-Team.
matmarex changed the visibility from "Public (No Login Required)" to "Custom Policy".
matmarex changed the subtype of this task from "Task" to "Security Issue".

Ugh, there is also getShortDesc(), which has the same problem, and that one *is* used unsafely… :/

matmarex claimed this task.Jun 2 2025, 6:41 PM
matmarex added a subscriber: gerritbot.
gerritbot added a comment.Jun 2 2025, 6:44 PM

Change #1152790 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@master] Improve HTML escaping in getLongDesc(), getShortDesc() methods

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

gerritbot added a project: Patch-For-Review.Jun 2 2025, 6:44 PM

Change #1152791 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/TimedMediaHandler@master] Improve HTML escaping in getLongDesc(), getShortDesc() methods

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

gerritbot added a comment.Jun 2 2025, 7:16 PM

Change #1152802 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@master] Treat File::getLongDesc() as possibly unsafe HTML, not wikitext

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

gerritbot added a comment.Jun 2 2025, 7:16 PM

Change #1152803 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@master] Treat File::getShortDesc() as possibly unsafe HTML

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

matmarex added a comment.Jun 2 2025, 7:18 PM

Patch https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1152803 should be backported to supported releases. Other patches don't require backports.

matmarex renamed this task from File::getLongDesc() is documented to return HTML, but the return value is treated as wikitext, and some handlers return unescaped plain text to File::getLongDesc()/getShortDesc() is documented to return HTML, but some handlers return unescaped plain text.Jun 2 2025, 7:21 PM
tstarling subscribed.Jun 4 2025, 1:44 AM

Can you update the doc comments of the File and MediaHandler methods to indicate that it is unsafe HTML and Sanitizer::removeSomeTags() should be called on the result?

gerritbot added a comment.Jun 4 2025, 1:57 AM

Change #1152803 merged by jenkins-bot:

[mediawiki/core@master] Treat File::getShortDesc() as possibly unsafe HTML

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

gerritbot added a comment.Jun 4 2025, 3:57 AM

Change #1152802 merged by jenkins-bot:

[mediawiki/core@master] Treat File::getLongDesc() as possibly unsafe HTML, not wikitext

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

gerritbot added a comment.Jun 4 2025, 7:48 PM

Change #1153686 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@wmf/1.45.0-wmf.3] Treat File::getShortDesc() as possibly unsafe HTML

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

gerritbot added a comment.Jun 4 2025, 7:48 PM

Change #1153687 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@wmf/1.45.0-wmf.4] Treat File::getShortDesc() as possibly unsafe HTML

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

gerritbot added a comment.Jun 4 2025, 8:22 PM

Change #1153686 merged by jenkins-bot:

[mediawiki/core@wmf/1.45.0-wmf.3] Treat File::getShortDesc() as possibly unsafe HTML

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

gerritbot added a comment.Jun 4 2025, 8:22 PM

Change #1153687 merged by jenkins-bot:

[mediawiki/core@wmf/1.45.0-wmf.4] Treat File::getShortDesc() as possibly unsafe HTML

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

gerritbot added a comment.Jun 4 2025, 9:18 PM

Change #1153710 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@REL1_44] Treat File::getShortDesc() as possibly unsafe HTML

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

gerritbot added a comment.Jun 4 2025, 9:18 PM

Change #1153711 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@REL1_43] Treat File::getShortDesc() as possibly unsafe HTML

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

gerritbot added a comment.Jun 4 2025, 9:18 PM

Change #1153712 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@REL1_42] Treat File::getShortDesc() as possibly unsafe HTML

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

gerritbot added a comment.Jun 4 2025, 9:20 PM

Change #1153713 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@REL1_39] Treat File::getShortDesc() as possibly unsafe HTML

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

gerritbot added a comment.Jun 4 2025, 10:42 PM

Change #1153713 merged by jenkins-bot:

[mediawiki/core@REL1_39] Treat File::getShortDesc() as possibly unsafe HTML

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

gerritbot added a comment.Jun 4 2025, 10:44 PM

Change #1153712 merged by jenkins-bot:

[mediawiki/core@REL1_42] Treat File::getShortDesc() as possibly unsafe HTML

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

gerritbot added a comment.Jun 4 2025, 10:46 PM

Change #1153711 merged by jenkins-bot:

[mediawiki/core@REL1_43] Treat File::getShortDesc() as possibly unsafe HTML

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

gerritbot added a comment.Jun 4 2025, 10:47 PM

Change #1153710 merged by jenkins-bot:

[mediawiki/core@REL1_44] Treat File::getShortDesc() as possibly unsafe HTML

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

matmarex added a comment.Jun 5 2025, 12:01 AM
In T395834#10882383, @tstarling wrote:

Can you update the doc comments of the File and MediaHandler methods to indicate that it is unsafe HTML and Sanitizer::removeSomeTags() should be called on the result?

Done, added to https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1152790

matmarex added a project: MediaWiki-Platform-Team.Jun 5 2025, 2:24 AM
matmarex moved this task from Inbox, needs triage to In progress (DO NOT USE) on the MediaWiki-Platform-Team board.
gerritbot added a comment.Jun 5 2025, 5:15 AM

Change #1152790 merged by jenkins-bot:

[mediawiki/core@master] Improve HTML escaping in getLongDesc(), getShortDesc() methods

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

gerritbot added a comment.Jun 5 2025, 5:24 AM

Change #1152791 merged by jenkins-bot:

[mediawiki/extensions/TimedMediaHandler@master] Improve HTML escaping in getLongDesc(), getShortDesc() methods

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

matmarex closed this task as Resolved.Jun 5 2025, 4:19 PM
matmarex added a subscriber: sbassett.

@sbassett Could you make this public? Thanks.

sbassett triaged this task as Medium priority.Jun 5 2025, 4:25 PM
sbassett awarded a token.
sbassett changed Author Affiliation from N/A to WMF Product.
sbassett removed a project: Patch-For-Review.
sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".
sbassett changed Risk Rating from N/A to Medium.
sbassett moved this task from Incoming to Our Part Is Done on the Security-Team board.
sbassett added projects: Vuln-XSS, SecTeam-Processed.
Maintenance_bot added a project: Commons.Jun 5 2025, 4:30 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