Overall security of the extension before we make it the default behavior on Chinese, Greek and Catalan wikipedia.
Description
Description
Details
Details
Related Changes in Gerrit:
Customize query in gerrit
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Declined | None | T88882 Move Hovercards to OOjs and OOjs UI | |||
| Resolved | None | T92555 Turn off 2 month test release of Hovercards on Catalan, and Greek Wikipedias on 2015-06-18 | |||
| Resolved | • Tnegrin | T88164 Make hovercards default for Catalan and Greek Wikipedias for 2 months starting 2015-04-15 | |||
| Resolved | • csteipp | T88171 Overall security review of Popups/Hovercards extension |
Event Timeline
Comment Actions
Just to confirm, this is Extension:Popups (https://gerrit.wikimedia.org/r/p/mediawiki/extensions/Popups.git), right?
Comment Actions
Looks much better this time around!
- ext.popups.renderer.article.js
In createImgThumbnail, it's unlikely (but not guaranteed) that the thumbnail url will include \, ), or " characters, which would allow someone to inject css, which could inject javascript. Either bail out if you find those, or urlenode them.
- ext.popups.settings.js
In renderOption, you're passing content[1] into .html without validating it. Do you need to generate html there? If not, use .text. If you do need html, document that the parameter is html, and make sure that you're escaping any inputs-- e.g., in settings.render you should pass mw.message( ... ).escaped() instead of .text().
Comment Actions
Change 199210 had a related patch set uploaded (by Prtksxna):
settings: Use .text() instead of .html() for option's label
Comment Actions
Change 199211 had a related patch set uploaded (by Prtksxna):
renderer.article: Ignore thumnail if the URL has suspicious characters
Comment Actions
Thanks for reviewing @csteipp!
In createImgThumbnail, it's unlikely (but not guaranteed) that the thumbnail url will include \, ), or " characters, which would allow someone to inject css, which could inject javascript. Either bail out if you find those, or urlenode them.
https://gerrit.wikimedia.org/r/199211
I now check the thumbnail.source at article.createThumbnail and return a <span> if those characters are found. Neither article.createSvgImageThumbnail nor article.createImgThumbnail is called with such a URL now. Would you rather have me returning the <span> from the respective functions instead?
In renderOption, you're passing content[1] into .html without validating it. Do you need to generate html there? If not, use .text. If you do need html, document that the parameter is html, and make sure that you're escaping any inputs-- e.g., in settings.render you should pass mw.message( ... ).escaped() instead of .text().
Comment Actions
Change 199211 merged by jenkins-bot:
renderer.article: Ignore thumnail if the URL has suspicious characters
Comment Actions
Change 199210 merged by jenkins-bot:
settings: Use .text() instead of .html() for option's label
Comment Actions
I have recently submitted a patch for T93720: In Hovercards, HTML is sometimes showing, e.g. apostrophes are shown as ', which needs your review — https://gerrit.wikimedia.org/r/#/c/199840/
Comment Actions
Change 486844 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/Popups@master] Add missing HTML escaping to all existing page preview types
Comment Actions
Change 486844 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Add missing HTML escaping to all existing page preview types