Page MenuHomePhabricator
Create Task
Maniphest T323376

Fix uses of OutputPageBeforeHTML hook in popular extensions that assume it is only executed once per page view
Closed, ResolvedPublic
Actions

Description

As documented on Manual:Hooks/OutputPageBeforeHTML, the OutputPageBeforeHTML hook can be executed more than once per page view, if the page content is composed from multiple sources.

For example, if the MediaWiki:Talkpageheader localisation message is defined, it is inserted at the top of all talk pages, and the hook is called twice: for the content from this message and for the content from the page's wikitext. This can be reproduced on e.g. https://fr.m.wikipedia.org/wiki/Discussion:Rhinocéros_de_Louis_XV.

However, we have some code that is not aware of that. This mistake caused major rendering issues in DiscussionTools (T316175#8404730), and after fixing that, I noticed that other extensions are also affected, but in less significant ways, so no one ever noticed. (e.g. MobileFrontend inserts duplicate <meta name="theme-color" content="#eaecf0"/> tags).

Code to review: https://codesearch-beta.wmcloud.org/deployed/?q=OutputPageBeforeHTML&files=%5C.php%24

Details

SubjectRepoBranchLines +/-
Use 'BeforePageDisplay' instead of 'OutputPageBeforeHTML' for once-per-page thingsmediawiki/extensions/MobileFrontendmaster+18 -15
Use 'BeforePageDisplay' instead of 'OutputPageBeforeHTML' for once-per-page thingsmediawiki/extensions/DiscussionToolsmaster+36 -36
Remove mention of unused OutputPageBeforeHTML hookmediawiki/extensions/WikidataPageBannermaster+1 -1
Use 'BeforePageDisplay' instead of 'OutputPageBeforeHTML' for once-per-page thingsmediawiki/extensions/ShortUrlmaster+5 -5
Customize query in gerrit

Related Objects

Event Timeline

matmarex created this task.Nov 18 2022, 2:08 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 18 2022, 2:08 PM
matmarex mentioned this in T316175: Make the mobile Add Topic button easier for people to access .Nov 18 2022, 2:08 PM
matmarex added projects: DiscussionTools, MobileFrontend, MediaWiki-extensions-ShortUrl, Wikidata-Page-Banner.Nov 18 2022, 2:20 PM

The hook is used by:

  • mediawiki/extensions/DiscussionTools
    • Looks wrong
  • mediawiki/extensions/DoubleWiki
    • Looks correct
  • mediawiki/extensions/MobileFrontend
    • Looks wrong
  • mediawiki/extensions/ShortUrl
    • Looks wrong (but harmless)
  • mediawiki/extensions/TemplateData
    • Looks correct
  • mediawiki/extensions/Wikibase
    • Looks correct
  • mediawiki/extensions/WikidataPageBanner
    • Only mentioned in a code comment (which looks wrong)
Maintenance_bot added a project: Wikidata.Nov 18 2022, 2:29 PM
matmarex added a comment.Nov 18 2022, 3:02 PM

For starters, I updated https://www.mediawiki.org/wiki/Manual:Hooks/OutputPageBeforeHTML with better advice: https://www.mediawiki.org/w/index.php?title=Manual:Hooks/OutputPageBeforeHTML&diff=5598387&oldid=4561707

gerritbot added a comment.Nov 18 2022, 3:46 PM

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

[mediawiki/extensions/DiscussionTools@master] Use 'BeforePageDisplay' instead of 'OutputPageBeforeHTML' for once-per-page things

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

gerritbot added a project: Patch-For-Review.Nov 18 2022, 3:46 PM

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

[mediawiki/extensions/MobileFrontend@master] Use 'BeforePageDisplay' instead of 'OutputPageBeforeHTML' for once-per-page things

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

gerritbot added a comment.Nov 18 2022, 3:46 PM

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

[mediawiki/extensions/ShortUrl@master] Use 'BeforePageDisplay' instead of 'OutputPageBeforeHTML' for once-per-page things

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

gerritbot added a comment.Nov 18 2022, 3:46 PM

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

[mediawiki/extensions/WikidataPageBanner@master] Remove mention of unused OutputPageBeforeHTML hook

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

matmarex claimed this task.Nov 18 2022, 3:48 PM
matmarex added a project: Editing-team (Kanban Board).
matmarex moved this task from Incoming to Code Review on the Editing-team (Kanban Board) board.
• ppelberg subscribed.Nov 18 2022, 4:50 PM
Tacsipacsi added a project: MediaWiki-extensions-DoubleWiki.Nov 18 2022, 11:11 PM
Tacsipacsi subscribed.
In T323376#8405720, @matmarex wrote:
  • mediawiki/extensions/DoubleWiki
    • Looks correct

Well, not entirely. I don’t know if there are things that work similarly to talkpageheader but in even-numbered namespaces, but I could (IMO) break it:

  1. Since DoubleWiki works with interlanguage links, I had to get MediaWiki display interlanguage links in talk namespaces. I could’ve probably written an extension that adds them (or install Wikibase), but the easiest solution was simply modifying Parser.php to remove the $nottalk check from the code recognizing interlanguage links. Isn’t likely to happen in production, but it fits for testing. 🙂
  2. Created MediaWiki:talkpageheader, calling a template in which I “forgot” to wrap the interlanguage link in <noinclude>.
  3. Also added a direct interlanguage link to a talk page.
  4. Opened the talk page with &match=hu added to the URL.
  5. Got QuadrupleWiki instead of DoubleWiki: both the talk page header and the main content got their Hungarian equivalents – except that sometimes the main content also got the Hungarian equivalent of the talk page header, probably due to cache pollution.

Until someone comes up with a such way to reproduce this that at least doesn’t involve modifying the parser, it’s pretty low-priority, but it’s still a bug.

Esanders awarded a token.Nov 20 2022, 8:26 AM
matmarex mentioned this in T323651: CVE-2023-37304: XSS in DoubleWiki extension (Wikisource).Nov 22 2022, 9:57 PM
matmarex mentioned this in T323833: "Add topic" button gets duplicated when previewing and saving in mobile DiscussionTools.Nov 25 2022, 4:13 PM
gerritbot added a comment.Nov 28 2022, 3:23 PM

Change 858618 merged by jenkins-bot:

[mediawiki/extensions/DiscussionTools@master] Use 'BeforePageDisplay' instead of 'OutputPageBeforeHTML' for once-per-page things

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

matmarex mentioned this in T323965: DoubleWiki assumes the OutputPageBeforeHTML hook is only executed once per page view.Nov 29 2022, 12:16 AM
matmarex removed a project: MediaWiki-extensions-DoubleWiki.

@Tacsipacsi You're right. I guess I didn't look too closely when I wrote that it looks correct.

I don't particularly want to spend time on DoubleWiki here. As you noted, it's not an issue in practice, and unlike MobileFrontend or DiscussionTools, no one has heard of it so I think folks aren't likely to use it as a bad example to copy-paste. I filed a separate bug for it: T323965.

matmarex renamed this task from Fix uses of OutputPageBeforeHTML hook that assume it is only executed once per page view to Fix uses of OutputPageBeforeHTML hook in popular extensions that assume it is only executed once per page view.Nov 29 2022, 12:17 AM
gerritbot added a comment.Nov 29 2022, 12:40 AM

Change 858620 merged by jenkins-bot:

[mediawiki/extensions/ShortUrl@master] Use 'BeforePageDisplay' instead of 'OutputPageBeforeHTML' for once-per-page things

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

ReleaseTaggerBot added a project: MW-1.40-notes (1.40.0-wmf.12; 2022-11-28).Nov 29 2022, 1:01 AM
gerritbot added a comment.Nov 29 2022, 9:07 AM

Change 858621 merged by jenkins-bot:

[mediawiki/extensions/WikidataPageBanner@master] Remove mention of unused OutputPageBeforeHTML hook

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

ReleaseTaggerBot edited projects, added MW-1.40-notes (1.40.0-wmf.13; 2022-12-05); removed MW-1.40-notes (1.40.0-wmf.12; 2022-11-28).Nov 29 2022, 10:01 AM
gerritbot added a comment.Nov 29 2022, 6:50 PM

Change 858619 merged by jenkins-bot:

[mediawiki/extensions/MobileFrontend@master] Use 'BeforePageDisplay' instead of 'OutputPageBeforeHTML' for once-per-page things

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

matmarex moved this task from Code Review to Ready for Sign Off on the Editing-team (Kanban Board) board.Nov 29 2022, 7:21 PM
matmarex edited projects, added Skipped QA; removed Patch-For-Review.
VPuffetMichel moved this task from Backlog to Triaged on the DiscussionTools board.Dec 1 2022, 4:12 PM
• ppelberg closed this task as Resolved.Dec 2 2022, 12:34 AM
Aklapper removed a project: WMF-General-or-Unknown.Dec 10 2022, 10:38 AM
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