Page MenuHomePhabricator

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

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

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

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

dr0ptp4kt updated the task description. (Show Details)Apr 4 2016, 4:36 PM
Restricted Application added a subscriber: TerraCodes. · View Herald TranscriptApr 19 2016, 8:55 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.
Jdlrobson set the point value for this task to 2.May 9 2016, 4:14 PM

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

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

  • $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

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

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

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

phuedx added a subscriber: phuedx.May 11 2016, 2:01 PM

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

csteipp closed this task as Resolved.May 11 2016, 4:21 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.

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

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

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

csteipp moved this task from Backlog to Done on the Security-Team board.May 16 2016, 5:06 PM
Jdlrobson closed this task as Resolved.May 16 2016, 9:54 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

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