Page MenuHomePhabricator

Security Review of Article Placeholder
Closed, ResolvedPublic


As discussed a while ago, the ArticlePlaceholder extension is ready for a security review now.

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.

Event Timeline

Lucie claimed this task.
Lucie raised the priority of this task from to Medium.
Lucie updated the task description. (Show Details)
Lucie added subscribers: hoo, Lucie, Lydia_Pintscher, csteipp.

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

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:
I thought this might be interesting for you to have a look at, too.

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

csteipp claimed this task.

Fixes look good, thanks!