r78117 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r78116‎ | r78117 | r78118 >
Date:04:54, 9 December 2010
Author:werdna
Status:ok (Comments)
Tags:
Comment:
Do not reveal page existence in colour of links in tabs when a user cannot read the page. This prevents leaking the existence (or not) of pages on wikis that are not readable anonymously
Modified paths:
  • /trunk/phase3/RELEASE-NOTES (modified) (history)
  • /trunk/phase3/includes/SkinTemplate.php (modified) (history)
  • /trunk/phase3/skins/Vector.php (modified) (history)

Diff [purge]

Index: trunk/phase3/skins/Vector.php
——@@ -73,6 +73,8 @@
7474 $action = $wgRequest->getVal( 'action', 'view' );
7575 $section = $wgRequest->getVal( 'section' );
7676
 77+ $userCanRead = $this->mTitle->quickUserCan( 'read' );
 78+
7779 // Checks if page is some kind of content
7880 if( $this->iscontent ) {
7981 // Gets page objects for the related namespaces
——@@ -93,16 +95,16 @@
9496
9597 // Adds namespace links
9698 $links['namespaces'][$subjectId] = $this->tabAction(
97 - $subjectPage, 'nstab-' . $subjectId, !$isTalk, '', true
 99+ $subjectPage, 'nstab-' . $subjectId, !$isTalk, '', $userCanRead
98100 );
99101 $links['namespaces'][$subjectId]['context'] = 'subject';
100102 $links['namespaces'][$talkId] = $this->tabAction(
101 - $talkPage, 'talk', $isTalk, '', true
 103+ $talkPage, 'talk', $isTalk, '', $userCanRead
102104 );
103105 $links['namespaces'][$talkId]['context'] = 'talk';
104106
105107 // Adds view view link
106 - if ( $this->mTitle->exists() ) {
 108+ if ( $this->mTitle->exists() && $userCanRead ) {
107109 $links['views']['view'] = $this->tabAction(
108110 $isTalk ? $talkPage : $subjectPage,
109111 'vector-view-view', ( $action == 'view' ), '', true
——@@ -155,7 +157,7 @@
156158 }
157159 }
158160 // Checks if the page has some kind of viewable content
159 - } elseif ( $this->mTitle->hasSourceText() ) {
 161+ } elseif ( $this->mTitle->hasSourceText() && $userCanRead ) {
160162 // Adds view source view link
161163 $links['views']['viewsource'] = array(
162164 'class' => ( $action == 'edit' ) ? 'selected' : false,
——@@ -169,7 +171,7 @@
170172 wfProfileIn( __METHOD__ . '-live' );
171173
172174 // Checks if the page exists
173 - if ( $this->mTitle->exists() ) {
 175+ if ( $this->mTitle->exists() && $userCanRead ) {
174176 // Adds history view link
175177 $links['views']['history'] = array(
176178 'class' => 'collapsible ' . ( ( $action == 'history' ) ? 'selected' : false ),
Index: trunk/phase3/includes/SkinTemplate.php
——@@ -728,6 +728,7 @@
729729 $action = $wgRequest->getVal( 'action', 'view' );
730730 $section = $wgRequest->getVal( 'section' );
731731 $content_actions = array();
 732+ $userCanRead = $this->mTitle->quickUserCan( 'read' );
732733
733734 $prevent_active_tabs = false;
734735 wfRunHooks( 'SkinTemplatePreventOtherActiveTabs', array( &$this, &$prevent_active_tabs ) );
——@@ -741,7 +742,7 @@
742743 $subjpage,
743744 $nskey,
744745 !$this->mTitle->isTalkPage() && !$prevent_active_tabs,
745 - '', true
 746+ '', $userCanRead
746747 );
747748
748749 $content_actions['talk'] = $this->tabAction(
——@@ -749,7 +750,7 @@
750751 'talk',
751752 $this->mTitle->isTalkPage() && !$prevent_active_tabs,
752753 '',
753 - true
 754+ $userCanRead
754755 );
755756
756757 wfProfileIn( __METHOD__ . '-edit' );
——@@ -774,7 +775,7 @@
775776 );
776777 }
777778 }
778 - } elseif ( $this->mTitle->hasSourceText() ) {
 779+ } elseif ( $this->mTitle->hasSourceText() && $userCanRead ) {
779780 $content_actions['viewsource'] = array(
780781 'class' => ($action == 'edit') ? 'selected' : false,
781782 'text' => wfMsg( 'viewsource' ),
——@@ -784,7 +785,7 @@
785786 wfProfileOut( __METHOD__ . '-edit' );
786787
787788 wfProfileIn( __METHOD__ . '-live' );
788 - if ( $this->mTitle->exists() ) {
 789+ if ( $this->mTitle->exists() && $userCanRead ) {
789790
790791 $content_actions['history'] = array(
791792 'class' => ($action == 'history') ? 'selected' : false,
Index: trunk/phase3/RELEASE-NOTES
——@@ -474,6 +474,8 @@
475475 * (bug 25512) Subcategory list should not include category prefix for members.
476476 * (bug 22753) Output from update.php is more clear when things changed, entries
477477 indicating nothing changed are now all prefixed by "..."
 478+* Page existence is now not revealed (in the colour of the tabs) to users who cannot
 479+ read the page in question.
478480
479481 === API changes in 1.17 ===
480482 * (bug 22738) Allow filtering by action type on query=logevent.

Follow-up revisions

RevisionCommit summaryAuthorDate
r78170Fix regression in r78117 per CR, was causing read, edit and history tabs to d...catrope10:55, 10 December 2010
r78172Followup r78117: information was still leaked through the caption of the Edit...catrope11:33, 10 December 2010
r79129MFT r78011 r78014 r78015 r78016 r78099 r78117 r78161 r78170 r78172 r78199 r78......platonides19:58, 28 December 2010
r79341Follow-up r79340: merge r78117, r78170, r78172 and r78506 from trunk into ski...catrope15:48, 31 December 2010

Comments

#Comment by Ilmari Karonen (talk | contribs)   05:35, 9 December 2010

I haven't tested this, and you already managed to refute two potential loopholes I suggested on IRC, but what about skins that don't use SkinTemplate? Shouldn't you also patch Skin.php, so that simply appending "?useskin=standard" to the URL won't let you see if the page exists?

Also, at a glance, it seems to me that this still reveals the existence of talk pages to users who can read the corresponding subject page and vice versa. I'm not sure if that's worth fixing, though.

#Comment by Catrope (talk | contribs)   17:08, 9 December 2010

Code looks OK. Will test this in all skins in an hour or so.

#Comment by Catrope (talk | contribs)   11:31, 10 December 2010

Took a bit longer than an hour, but I've confirmed that this works properly in all skins.

#Comment by IAlex (talk | contribs)   17:36, 9 December 2010

Title::getUserPermissionsErrorsInternal() (which is used by Title::quickUserCan()) is not designed to handle checks for "read" permissions. The only possibility is to use Title::userCanRead(). Quick example reproducing bug 26020 on the command line: php maintenance/eval.php > $title = Title::newMainPage()

> $wgEmailConfirmToEdit = false

> return $title->quickUserCan( 'read' ) bool(true) > $wgEmailConfirmToEdit = true

> return $title->quickUserCan( 'read' ) bool(false) See the patch I've attached there if you still want to use that function.

#Comment by Catrope (talk | contribs)   10:51, 10 December 2010

It gets worse: 

> $wgNamespaceProtection[NS_USER] = array( 'protect');

> $title = Title::newFromText("User:Catrope");

> echo $title->quickUserCan('read')

> $title2 = Title::newFromText("Catrope");

> echo $title2->quickUserCan('read')
1
#Comment by Catrope (talk | contribs)   10:56, 10 December 2010

Fixed in r78170. The patch on bug 26020 should probably be applied to trunk, but I don't think it's needed in 1.17.

#Comment by Nikerabbit (talk | contribs)   10:51, 10 December 2010

There is a regression related to wgNamespaceProtection which Roan is investigating right now.

Status & tagging log