Hi @Nemo_bis, could you update the description of this ticket and add the information requested at https://www.mediawiki.org/wiki/Wikimedia_Security_Team/Security_reviews#Requesting_a_review? Thanks!

In T155087#3047619, @dpatrick wrote:

Hi @Nemo_bis, could you update the description of this ticket and add the information requested at https://www.mediawiki.org/wiki/Wikimedia_Security_Team/Security_reviews#Requesting_a_review? Thanks!

Done, I believe.

If this is intended for WMF deployment...

Non security stuff:

• nsrels-desc in english doesn't make much sense. Allows who? Namespace tabs where?
• Mixed leading whitespace in NamespaceRelations_body.php
• Needs converting to extension registration before deployment; no new extensions should be deployed using PHP entry points
• Various profiling calls on functions that really don't need them
• Underscores from variable names prefixes should be removed

Review of 00f527cadfbf (Mon Oct 24, 2016). Overall looks good from a security perspective. My only major concern is potential incompatibility with any other extension which adds a namespace tab. There are some minor coding convention things that need to be fixed. The code is a tad dense, and could perhaps benefit from splitting into finer grained functions and having more comments.

In addition to what Reedy said above

Security (minor)

• "NamespaceRelations_body.php" line 83 - The user can read test should be done against the target page of the tab, not the current page. Also since quickUserCan may return the wrong answer in certain circumstances, the extension should document that it may leak if other tabs exist if the wiki is using extensions that do expensive read checks.
• "NamespaceRelations_body.php" line 63 - Should probably preg_escape $data['customTarget'] here.

Other (major)

• It overwrites all tabs, including those from other extensions. I don't think we want it to overwrite namespace tabs defined by other extensions.

Other (minor)

• "NamespaceRelations_body.php" line 175 - if ( ( $tabs['subject']['title']->isMainPage() && $this->getNamespace( $key, 'inMainPage' ) && $this->getNamespace( $key, 'hideTalk' ) ) || $this->getNamespace( $key, 'hideTalk' ) - The first clause of the OR doesn't make sense since it will never be true when the second clause is false. This if statement should be simplified or possibly corrected.
• Add extension.json (Would also require moving the hook to be a static function of NamespaceRelations instead of a lambda function)
• Kill wfProfileIn()/Out() - These aren't used anymore.
• Some of the lines are a little on the long side, should probably be broken up.
• Tests would be nice. (Even just broad ones verifying that customTarget matches things correctly)
• [Multiple places] - Should probably use Title::makeTitleSafe instead of Title::makeTitle and maybe not show the tab, or rather show an error in the event of an invalid title (For example, with a customTarget of $1/doc, if the user goes to a page that's 254 bytes long, should it really link to the invalid title?). Or maybe it makes sense to only error when the user clicks on the tab as is the current behaviour; I'm not sure.
• "NamespaceRelations_body.php" line 99 - Its kind of odd that there is no line saying $subjectOptions = []; but there is a line unset()'ing it. Normally we declare arrays to be arrays first, but don't worry about calling unset() as we let the garbage collector clean it up once it falls out of scope (ditto for $talkOptions)

Change 356863 had a related patch set uploaded (by Nemo bis; owner: Federico Leva):
[mediawiki/extensions/NamespaceRelations@master] Some style changes

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

Change 356864 had a related patch set uploaded (by Nemo bis; owner: Federico Leva):
[mediawiki/extensions/NamespaceRelations@master] Remove wfProfile* calls

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

Change 356864 merged by jenkins-bot:
[mediawiki/extensions/NamespaceRelations@master] Remove wfProfile* calls

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

Change 356868 had a related patch set uploaded (by Nemo bis; owner: Federico Leva):
[mediawiki/extensions/NamespaceRelations@master] Some code clarifications

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

Change 356863 merged by jenkins-bot:
[mediawiki/extensions/NamespaceRelations@master] Some style changes

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

Bump any more work needed on this?

Hmm. My main concern is that https://www.mediawiki.org/wiki/Extension:NamespaceRelations states that it is unmaintained.

In T155087#4876952, @Nemo_bis wrote:

Inaccurate rationale for closure.

To be clear, are you saying you (or people you know) are interested in doing any remaining work required to get this into production?

Change 356868 abandoned by Thiemo Kreuz (WMDE):
[mediawiki/extensions/NamespaceRelations@master] Some code clarifications

Reason:
This is so old, it can not be applied any more anyway. The linked ticket got declined.

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