Security review of Hovercards before beta->default conversion
Closed, ResolvedPublic2 Estimated Story Points
Actions

Description

Reading anticipates moving Hovercards from beta to a default feature in the middle of next quarter. After the many issues we had in the first version, we should do another detailed review before we make this change.

Acceptance criteria

Security review of current state for 1 or 2 wiki productionization
Security review after more code is introduced throughout the quarter

Details

Subject	Repo	Branch	Lines +/-
Add missing HTML escaping to all existing page preview types	mediawiki/extensions/Popups	master	+76 -13
Add client-side validation of PopupsSurveyLink	mediawiki/extensions/Popups	master	+47 -15
Annotate survey link with rel=noreferrer	mediawiki/extensions/Popups	master	+7 -0
Document PopupsSurveyLink with cautionary note	mediawiki/extensions/Popups	master	+1 -0
Allow brackets in createImgThumbnail	mediawiki/extensions/Popups	master	+6 -1

Customize query in gerrit

Related Objects
Search...

Status	Assigned	Task
Resolved	Dereckson	T68374 Enable Hovercards on se.wikimedia.org (Swedish chapter wiki)
Resolved	Jdlrobson	T70860 [GOAL] Graduate Page Previews feature (Popups extension) out of Beta Feature
Resolved	ovasileva	T131366 Measure impact of HoverCards on Central Notice interaction
Resolved	None	T133406 investigate any fr-tech work associated with hovercard test
Invalid	None	T141775 Initiate Hovercards A/B Test 3 with Central Notice: October 2016
Resolved	CKoerner_WMF	T143170 Start discussion with Italian community about running a/b test for hovercards in October
Resolved	Jdlrobson	T132602 [GOAL] Roll Hovercards out on smaller wikipedia project
Resolved	phuedx	T136713 Increase ON percentage for Hovercards A/B test 1
Resolved	dr0ptp4kt	T134778 Run Hovercards A/B test 1
Resolved	• Jhernandez	T132604 Prepare HoverCards for A/B test on smaller wikipedia project
Resolved	Pcoombe	T134291 Create banners for HoverCards A/B test
Resolved	AndyRussG	T134286 CentralNotice: test registration
Resolved	ovasileva	T147553 [EPIC] Page previews broken on many projects
Open	ovasileva	T244262 [Epic] Enable page previews and reference previews as a beta feature on all projects
Open	None	T111231 Page previews for Wikidata
Resolved	Jdlrobson	T129177 Security review of Hovercards before beta->default conversion

Event Timeline

• csteipp created this task.Mar 7 2016, 10:54 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 7 2016, 10:54 PM

Krenair added a project: Page-Previews.Mar 7 2016, 11:47 PM

Krinkle subscribed.Mar 7 2016, 11:49 PM

Ricordisamoa subscribed.Mar 8 2016, 6:09 PM

• Prtksxna added a parent task: T70860: [GOAL] Graduate Page Previews feature (Popups extension) out of Beta Feature.Mar 10 2016, 6:56 AM

Jdlrobson moved this task from Backlog to Needs Analysis on the Page-Previews board.Mar 22 2016, 9:54 PM

@csteipp, I'd like a review conducted between now and 22-April-2016. Would that be feasible?

dr0ptp4kt added a parent task: T111231: Page previews for Wikidata.Apr 4 2016, 3:25 PM

dr0ptp4kt updated the task description. (Show Details)Apr 4 2016, 4:36 PM

• csteipp moved this task from Incoming to Scheduled on the deprecated-security-team-reviews board.Apr 5 2016, 11:54 PM

dr0ptp4kt moved this task from Needs Analysis to Tracking on the Reading-Web-Sprint-70-Lady-and-the-Trumps board.Apr 11 2016, 2:26 AM

• JKatzWMF added a parent task: T132602: [GOAL] Roll Hovercards out on smaller wikipedia project.Apr 18 2016, 4:31 PM

tentatively scheduled for next week (https://www.mediawiki.org/wiki/Wikimedia_Security_Team/Schedule)

dr0ptp4kt added a parent task: T132604: Prepare HoverCards for A/B test on smaller wikipedia project.Apr 18 2016, 5:01 PM

dr0ptp4kt added a project: Reading-Web-Sprint-71-Matisse-Monet-Kandinsky-and-the-Departing-Painters.Apr 19 2016, 8:55 PM

Restricted Application added a subscriber: TerraCodes. · View Herald TranscriptApr 19 2016, 8:55 PM

dr0ptp4kt moved this task from Needs Analysis to Tracking on the Reading-Web-Sprint-71-Matisse-Monet-Kandinsky-and-the-Departing-Painters board.Apr 19 2016, 8:56 PM

MBinder_WMF added a project: Web-Team-Backlog.Apr 27 2016, 5:10 PM

MBinder_WMF moved this task from Incoming to 2015-16 Q4 on the Web-Team-Backlog board.Apr 27 2016, 5:14 PM

Looks mostly good, a couple minor cleanups.

The css in article.createImgThumbnail is constructed as 'url(' + url + ')', but article.createThumbnail prevents \, ', and " in the url. So either createThumbnail should filter )'s, or createImgThumbnail should put the url into a quoted string.

$wgPopupsSurveyLink is very trusted by the code, both the format (admin can add a javascript: url), and the site itself (it can do tab-napping). That should either be documented or protected against. To protect, you would whitelist https?:/// for the url, and add rel-noopener and/or rel=noreferrer to the link.

• csteipp moved this task from Scheduled to In Progress on the deprecated-security-team-reviews board.Apr 28 2016, 1:45 AM

dr0ptp4kt added a project: Reading-Web-Sprint-72-Ninety-nine-problems-but-Nirzar-aint-one.May 9 2016, 3:39 PM

Jdlrobson set the point value for this task to 2.May 9 2016, 4:14 PM

Jdlrobson moved this task from Needs Analysis to To Do on the Reading-Web-Sprint-72-Ninety-nine-problems-but-Nirzar-aint-one board.

Jdlrobson claimed this task.May 10 2016, 8:48 PM

Jdlrobson moved this task from To Do to Doing on the Reading-Web-Sprint-72-Ninety-nine-problems-but-Nirzar-aint-one board.

Change 288094 had a related patch set uploaded (by Jdlrobson):
Allow brackets in createImgThumbnail

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

gerritbot added a project: Patch-For-Review.May 10 2016, 9:11 PM

$wgPopupsSurveyLink is very trusted by the code, both the format (admin can add a javascript: url), and the site itself (it can do tab-napping). That should either be documented or protected against. To protect, you would whitelist https?:/// for the url, and add rel-noopener and/or rel=noreferrer to the link.

Yes an admin could do this, but at least for Wikimedia they'd be quite a few hurdles for them to do so - they'd have to get +2 access and go through a deploy. Thus I've gone for the documentation.

Change 288098 had a related patch set uploaded (by Jdlrobson):
Document PopupsSurveyLink with cautionary note

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

Jdlrobson moved this task from Doing to Code Review on the Reading-Web-Sprint-72-Ninety-nine-problems-but-Nirzar-aint-one board.May 10 2016, 9:16 PM

Change 288094 merged by jenkins-bot:
Allow brackets in createImgThumbnail

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

ReleaseTaggerBot added a project: MW-1.28-release (WMF-deploy-2016-05-17_(1.28.0-wmf.2)).May 11 2016, 11:00 AM

Change 288198 had a related patch set uploaded (by Phuedx):
Annotate survey link with rel=noreferrer

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

Change 288199 had a related patch set uploaded (by Phuedx):
Add client-side validation of PopupsSurveyLink

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

I submitted the above because, honestly, the cautionary note was alarming to me.

In T129177#2245856, @csteipp wrote:

Looks mostly good, a couple minor cleanups.

The css in article.createImgThumbnail is constructed as 'url(' + url + ')', but article.createThumbnail prevents \, ', and " in the url. So either createThumbnail should filter )'s, or createImgThumbnail should put the url into a quoted string.

https://gerrit.wikimedia.org/r/288094 looks like it addresses this in my testing with it. Thanks!

$wgPopupsSurveyLink is very trusted by the code, both the format (admin can add a javascript: url), and the site itself (it can do tab-napping). That should either be documented or protected against. To protect, you would whitelist https?:/// for the url, and add rel-noopener and/or rel=noreferrer to the link.

https://gerrit.wikimedia.org/r/288199 is definitely my preference, thanks! If for some reason you need to go back to allowing more urls through, just make sure to keep the documentation.

Change 288098 merged by jenkins-bot:
Document PopupsSurveyLink with cautionary note

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

Change 288198 merged by jenkins-bot:
Annotate survey link with rel=noreferrer

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

Still waiting on https://gerrit.wikimedia.org/r/288199

Jdlrobson moved this task from Code Review to -1 (Needs More Work) on the Reading-Web-Sprint-72-Ninety-nine-problems-but-Nirzar-aint-one board.May 11 2016, 9:20 PM

@Jdlrobson: I've responded to your comment. LMK what you think.

Jdlrobson moved this task from Code Review to Ready for Signoff on the Reading-Web-Sprint-72-Ninety-nine-problems-but-Nirzar-aint-one board.May 12 2016, 3:38 PM

Change 288199 merged by jenkins-bot:
Add client-side validation of PopupsSurveyLink

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

Jdlrobson removed a project: Patch-For-Review.May 12 2016, 3:44 PM

• csteipp moved this task from Incoming to Our Part Is Done on the Security-Team board.May 16 2016, 5:06 PM

per @csteipp moving it to done.

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

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

gerritbot added a project: Patch-For-Review.Jan 28 2019, 9:14 AM

Change 486844 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Add missing HTML escaping to all existing page preview types

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

sbassett moved this task from In Progress to Done on the deprecated-security-team-reviews board.Jul 9 2019, 8:15 PM

• chasemp removed a project: deprecated-security-team-reviews.Jan 8 2020, 4:13 PM

TerraCodes unsubscribed.Jan 10 2020, 2:54 AM