Page MenuHomePhabricator

Security Review of Article Placeholder
Closed, ResolvedPublic

Description

As discussed a while ago, the ArticlePlaceholder extension is ready for a security review now.
https://www.mediawiki.org/wiki/Extension:ArticlePlaceholder

The main part of the extension, the display of the Wikidata entity, is written in Lua and therefore shouldn't be a big problem security wise.
The searchHook is used to add results of the ArticlePlaceholder to the search.

The whole code was reviewed by the Wikidata team (mainly by @hoo) before.

Details

Related Gerrit Patches:
mediawiki/extensions/ArticlePlaceholder : masterImplement feedback from the security review

Event Timeline

Lucie created this task.Nov 10 2015, 3:48 PM
Lucie claimed this task.
Lucie raised the priority of this task from to Normal.
Lucie updated the task description. (Show Details)
Lucie added subscribers: hoo, Lucie, Lydia_Pintscher, csteipp.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 10 2015, 3:48 PM

Hi @Lucie,

I took a look at this again from commit c0c5b0c.

Two minor issues that need to be fixed before this gets deployed:

  • Line 103: $this->getOutput()->setPageTitle( $this->msg( 'articleplaceholder-abouttopic' ) ) - This should either be escaped() or parsed, so that a malicious admin can't sneak javascript onto the site through the message.
  • Line 292: $this->getOutput()->setPageTitle( $label ); - This looks like an xss as is, if the entity is something like https://test.wikidata.org/wiki/Q1923
Lucie removed Lucie as the assignee of this task.Dec 11 2015, 2:24 PM
Lucie set Security to None.

Change 258451 had a related patch set uploaded (by Lucie Kaffee):
Implement feedback from the security review

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

Lucie added a comment.Dec 11 2015, 5:53 PM

Hey Chris, thanks for reviewing!
There are new features, I'm working on that might be interesting for the security review;
Mainly, the sorting of statement groups since I will read a list of property ids from a wikipage.
So the content of a certain wiki page is parsed to an array.
The patch isn't merged yet, it's here: https://gerrit.wikimedia.org/r/#/c/255549/
I thought this might be interesting for you to have a look at, too.
Thanks!

Change 258451 merged by jenkins-bot:
Implement feedback from the security review

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

csteipp closed this task as Resolved.Jan 4 2016, 10:36 PM
csteipp claimed this task.

Fixes look good, thanks!