Security Review of Article Placeholder
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	Lucie
	Nov 10 2015, 3:48 PM

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 Changes in Gerrit:

	Subject	Repo	Branch	Lines +/-
	Implement feedback from the security review	mediawiki/extensions/ArticlePlaceholder	master	+2 -2

Customize query in gerrit

Related Objects
Search...

Status	Assigned	Task
Resolved	thiemowmde	T142940 Create client functionality for getting human readable wikitext from Wikibase statements
Declined	None	T99897 {{#property}} parser function should output EntityIdValues as local wiki links, once we have article placeholders
Invalid	Lydia_Pintscher	T99896 Use Lua for custom rendering of entity data on Special:ShowEntity
Resolved	Lucie	T99895 [Epic] Article placeholder based on data from Wikidata
Resolved	Lydia_Pintscher	T135624 Second round of the ArticlePlaceholder rollout (tracking)
Resolved	hoo	T130997 [Task] Configure ArticlePlaceholder for nnwiki
Resolved	Addshore	T123087 [Task] Track use of Create Article and Translate Article buttons
Resolved	Lucie	T117965 Review and deploy of the ArticlePlaceholder extension
Resolved	• csteipp	T118268 Security Review of Article Placeholder

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 Medium.

Lucie updated the task description. (Show Details)

Lucie added projects: Wikidata, MediaWiki-extensions-Wikibase-Client, ArticlePlaceholder, deprecated-security-team-reviews.

Lucie added subscribers: hoo, Lucie, Lydia_Pintscher, • csteipp.

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 10 2015, 3:48 PM

Lucie moved this task from Incoming to Broader tickets on the ArticlePlaceholder board.Nov 10 2015, 3:48 PM

Ricordisamoa subscribed.Nov 11 2015, 4:01 AM

• csteipp moved this task from Incoming to Scheduled on the deprecated-security-team-reviews board.Nov 12 2015, 3:35 PM

Lydia_Pintscher moved this task from incoming to monitoring on the Wikidata board.Nov 16 2015, 11:44 AM

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

• csteipp moved this task from Scheduled to In Progress on the deprecated-security-team-reviews board.Dec 10 2015, 12:58 AM

• csteipp moved this task from In Progress to Waiting/Blocked on the deprecated-security-team-reviews board.Dec 10 2015, 1:24 AM

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

gerritbot added a project: Patch-For-Review.Dec 11 2015, 2:35 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!