Page MenuHomePhabricator
Authored By
Mattflaschen-WMF
Jun 17 2016, 12:13 AM
Size
22 KB
Referenced Files
None
Subscribers
None

T137593.patch

From f92d935408aac23d175c70f73bcdea1955f1ded1 Mon Sep 17 00:00:00 2001
From: Matthew Flaschen <mflaschen@wikimedia.org>
Date: Fri, 10 Jun 2016 16:51:42 -0400
Subject: [PATCH] SECURITY: Fix topic title visibility
There was an edge case (deletion, then suppression of the same topic)
where the topic title of a suppressed topic was not correctly
restricted.
This was because it wasn't going through the permission system,
which I realized was a problem in theory, but I didn't realize
it was also a problem in practice.
To ensure the topic titles of deleted topics is still visible
(see T119234), I added view-topic-title.
I investigated the idea of the 'log' action (not actually used even
before this patch). I don't think that approach is consistent with core.
In core, there are sensitive log actions that go to
Special:Log/suppress, and log info can be redacted with deletelogentry
(fixed in T137288).
But if you RevDel a core revision (without choosing "Suppress data from
administrators as well as others") then:
* RevDel the same revision and *do* choose to hide from administrators,
then:
* Delete the whole page
After each, the original log entry is still visible to anons. So
we don't need to auto-hide entire log entries (except
Special:Log/suppress), but we do need to make sure data that was hidden
(topic titles) is properly hidden.
So I removed the 'log' action.
While I'm in here:
* Allow seeing topic summary if topic is hidden.
Hidden topics are fully displayed (including summary) when you access a
topic directly. This inconsistency only affected direct
view-topic-summary URLs.
* Make qqq documentation of unused parameters for these log entries
clearer.
Bug: T137593
Bug: T119234
Change-Id: I15de3f50d95a6bedda8dfc984a643d0237405c89
---
FlowActions.php | 32 +++++++++++++++--
i18n/en.json | 10 ++++++
i18n/qqq.json | 18 +++++++---
includes/Formatter/RevisionFormatter.php | 7 ++--
includes/Log/ActionFormatter.php | 61 ++++++++++++++++++++++++--------
includes/Templating.php | 6 +++-
6 files changed, 110 insertions(+), 24 deletions(-)
diff --git a/FlowActions.php b/FlowActions.php
index 3524d6f..1e01bac 100644
--- a/FlowActions.php
+++ b/FlowActions.php
@@ -21,6 +21,8 @@ use Flow\Data\Listener\RecentChangesListener;
* no one can perform the action described by that key.
* * root-permissions: similar to 'permissions', but applies to the last revision
* of the root post (= the topic) for the revision the action is executed against.
+ * If root-permissions is omitted entirely, it doesn't affect what is allowed.
+ * However, if any keys are set, omitted keys are treated as prohibited.
* * core-delete-permissions: array of rights, where any of those rights will
* give you permission to do the action on a deleted board (isAllowedAny).
* Empty string and omitted behave like 'permissions'.
@@ -823,6 +825,7 @@ $wgFlowActions = array(
),
'root-permissions' => array(
PostRevision::MODERATED_NONE => '',
+ PostRevision::MODERATED_HIDDEN => '',
PostRevision::MODERATED_LOCKED => '',
),
'core-delete-permissions' => array( 'deletedtext' ),
@@ -833,6 +836,32 @@ $wgFlowActions = array(
'modules' => array(),
),
+ // This is only used when we specifically want to see the topic title. If we're
+ // cascading from a post (to view a post we need to be able to view the topic),
+ // we'll use 'view' for both the post and topic root. Unprivileged users shouldn't
+ // be able to view a post in a deleted topic, but should be able to view the topic
+ // title.
+ 'view-topic-title' => array(
+ 'performs-writes' => false,
+ 'log_type' => false, // don't log views
+ 'rc_insert' => false, // won't even be called, actually; only for writes
+ 'permissions' => array(
+ // Everyone can see topic titles on existent boards, unless the
+ // version you're viewing is suppressed, or the most recent version
+ // is
+ PostRevision::MODERATED_NONE => '',
+ PostRevision::MODERATED_HIDDEN => '',
+ PostRevision::MODERATED_LOCKED => '',
+ PostRevision::MODERATED_DELETED => '',
+ PostRevision::MODERATED_SUPPRESSED => 'flow-suppress',
+ ),
+ 'core-delete-permissions' => array( 'deletedtext' ),
+ 'links' => array(), // @todo
+ 'actions' => array(), // view is not a recorded change type, no actions will be requested
+ 'history' => array(), // views don't generate history
+ 'modules' => array(),
+ ),
+
// Actions not tied to a particular revision change_type
// or just move these to a different file
// @todo: we should probably at least add 'permissions' in these below
@@ -870,8 +899,7 @@ $wgFlowActions = array(
'modules' => array(),
),
- // log & all other formatters have same config as history
- 'log' => 'history',
+ // Other formatters have the same config as history
'recentchanges' => 'history',
'contributions' => 'history',
'checkuser' => 'history',
diff --git a/i18n/en.json b/i18n/en.json
index 18ba35a..3737d2b 100644
--- a/i18n/en.json
+++ b/i18n/en.json
@@ -18,15 +18,25 @@
"flow-talk-taken-over-comment": "/* This page has been converted into a Flow discussion board */",
"log-name-flow": "Flow activity log",
"logentry-delete-flow-delete-post": "$1 {{GENDER:$2|deleted}} a [$4 post] on \"[[$3|$5]]\" on [[$6]]",
+ "logentry-delete-flow-delete-post-topic-title-not-visible": "$1 {{GENDER:$2|deleted}} a post on a topic on [[$3]]",
"logentry-delete-flow-restore-post": "$1 {{GENDER:$2|restored}} a [$4 post] on \"[[$3|$5]]\" on [[$6]]",
+ "logentry-delete-flow-restore-post-topic-title-not-visible": "$1 {{GENDER:$2|restored}} a post on a topic on [[$3]]",
"logentry-suppress-flow-suppress-post": "$1 {{GENDER:$2|suppressed}} a [$4 post] on \"[[$3|$5]]\" on [[$6]]",
+ "logentry-suppress-flow-suppress-post-topic-title-not-visible": "$1 {{GENDER:$2|suppressed}} a post on a topic on [[$3]]",
"logentry-suppress-flow-restore-post": "$1 {{GENDER:$2|deleted}} a [$4 post] on \"[[$3|$5]]\" on [[$6]]",
+ "logentry-suppress-flow-restore-post-topic-title-not-visible": "$1 {{GENDER:$2|deleted}} a post on a topic on [[$3]]",
"logentry-delete-flow-delete-topic": "$1 {{GENDER:$2|deleted}} topic \"[[$3|$5]]\" on [[$6]]",
+ "logentry-delete-flow-delete-topic-topic-title-not-visible": "$1 {{GENDER:$2|deleted}} a topic on [[$3]]",
"logentry-delete-flow-restore-topic": "$1 {{GENDER:$2|restored}} topic \"[[$3|$5]]\" on [[$6]]",
+ "logentry-delete-flow-restore-topic-topic-title-not-visible": "$1 {{GENDER:$2|restored}} a topic on [[$3]]",
"logentry-suppress-flow-suppress-topic": "$1 {{GENDER:$2|suppressed}} topic \"[[$3|$5]]\" on [[$6]]",
+ "logentry-suppress-flow-suppress-topic-topic-title-not-visible": "$1 {{GENDER:$2|suppressed}} a topic on [[$3]]",
"logentry-suppress-flow-restore-topic": "$1 {{GENDER:$2|deleted}} topic \"[[$3|$5]]\" on [[$6]]",
+ "logentry-suppress-flow-restore-topic-topic-title-not-visible": "$1 {{GENDER:$2|deleted}} a topic on [[$3]]",
"logentry-lock-flow-lock-topic": "$1 {{GENDER:$2|marked}} topic \"[[$3|$5]]\" as resolved on [[$6]]",
+ "logentry-lock-flow-lock-topic-topic-title-not-visible": "$1 {{GENDER:$2|marked}} a topic as resolved on [[$3]]",
"logentry-lock-flow-restore-topic": "$1 {{GENDER:$2|reopened}} topic \"[[$3|$5]]\" on [[$6]]",
+ "logentry-lock-flow-restore-topic-topic-title-not-visible": "$1 {{GENDER:$2|reopened}} a topic on [[$3]]",
"logentry-import-lqt-to-flow-topic": "[[$1|$2]] on [[$3]] was imported from LiquidThreads to Flow",
"flow-user-moderated": "Moderated user",
"flow-board-header-browse-topics-link": "Browse topics",
diff --git a/i18n/qqq.json b/i18n/qqq.json
index c24e5b3..a492992 100644
--- a/i18n/qqq.json
+++ b/i18n/qqq.json
@@ -29,15 +29,25 @@
"flow-talk-taken-over-comment": "Comment of the new revision that is created when a page turns into a Flow discussion board.",
"log-name-flow": "{{doc-logpage}}\nName of the Flow log filter on the [[Special:Log]] page.",
"logentry-delete-flow-delete-post": "Text for a deletion log entry when a post was deleted. Parameters:\n* $1 - the user: link to the user page\n* $2 - the username. Can be used for GENDER.\n* $3 - the page where the post was moderated\n* $4 - permalink URL to the moderated post\n* $5 - The topic title text\n* $6 - The board page\n{{Related|Logentry-flow}}",
+ "logentry-delete-flow-delete-post-topic-title-not-visible": "Text for a deletion log entry when a post was deleted, and the topic title is not visible to the current user. Parameters:\n* $1 - the user: link to the user page\n* $2 - the username. Can be used for GENDER.\n* $3 - The board page\n{{Related|Logentry-flow}}",
"logentry-delete-flow-restore-post": "Text for a deletion log entry when a deleted post was restored. Parameters:\n* $1 - the user: link to the user page\n* $2 - the username. Can be used for GENDER.\n* $3 - the page where the post was moderated\n* $4 - permalink URL to the moderated post\n* $5 - The topic title text\n* $6 - The board page\n{{Related|Logentry-flow}}",
+ "logentry-delete-flow-restore-post-topic-title-not-visible": "Text for a deletion log entry when a deleted post was restored, and the topic title is not visible to the current user. Parameters:\n* $1 - the user: link to the user page\n* $2 - the username. Can be used for GENDER.\n* $3 - The board page\n{{Related|Logentry-flow}}",
"logentry-suppress-flow-suppress-post": "Text for a deletion log entry when a post was suppressed. Parameters:\n* $1 - the user: link to the user page\n* $2 - the username. Can be used for GENDER.\n* $3 - the page where the post was moderated\n* $4 - permalink URL to the moderated post\n* $5 - The topic title text\n* $6 - The board page\n{{Related|Logentry-flow}}",
+ "logentry-suppress-flow-suppress-post-topic-title-not-visible": "Text for a deletion log entry when a post was suppressed, and the topic title is not visible to the current user. Parameters:\n* $1 - the user: link to the user page\n* $2 - the username. Can be used for GENDER.\n* $3 - The board page\n{{Related|Logentry-flow}}",
"logentry-suppress-flow-restore-post": "Text for a deletion log entry when a suppressed post was restored. Parameters:\n* $1 - the user: link to the user page\n* $2 - the username. Can be used for GENDER.\n* $3 - the page where the post was moderated\n* $4 - permalink URL to the moderated post\n* $5 - The topic title text\n* $6 - The board page\n{{Related|Logentry-flow}}",
- "logentry-delete-flow-delete-topic": "Text for a deletion log entry when a topic was deleted. Parameters:\n* $1 - the user: link to the user page\n* $2 - the username. Can be used for GENDER.\n* $3 - the page where the topic was moderated\n* $5 - The topic title text\n* $6 - The board page\n{{Related|Logentry-flow}}",
- "logentry-delete-flow-restore-topic": "Text for a deletion log entry when a deleted topic was restored. Parameters:\n* $1 - the user: link to the user page\n* $2 - the username. Can be used for GENDER.\n* $3 - the page where the topic was moderated\n* $5 - The topic title text\n* $6 - The board page\n{{Related|Logentry-flow}}",
- "logentry-suppress-flow-suppress-topic": "Text for a deletion log entry when a topic was suppressed. Parameters:\n* $1 - the user: link to the user page\n* $2 - the username. Can be used for GENDER.\n* $3 - the page where the topic was moderated\n* $5 - The topic title text\n* $6 - The board page\n{{Related|Logentry-flow}}",
+ "logentry-suppress-flow-restore-post-topic-title-not-visible": "Text for a deletion log entry when a suppressed post was restored, and the topic title is not visible to the current user. Parameters:\n* $1 - the user: link to the user page\n* $2 - the username. Can be used for GENDER.\n* $3 - The board page\n{{Related|Logentry-flow}}",
+ "logentry-delete-flow-delete-topic": "Text for a deletion log entry when a topic was deleted. Parameters:\n* $1 - the user: link to the user page\n* $2 - the username. Can be used for GENDER.\n* $3 - the page where the topic was moderated\n* $4 - permalink URL to the topic (unused)\n* $5 - The topic title text\n* $6 - The board page\n{{Related|Logentry-flow}}",
+ "logentry-delete-flow-delete-topic-topic-title-not-visible": "Text for a deletion log entry when a topic was deleted, and the topic title is not visible to the current user. Parameters:\n* $1 - the user: link to the user page\n* $2 - the username. Can be used for GENDER.\n* $3 - The board page\n{{Related|Logentry-flow}}",
+ "logentry-delete-flow-restore-topic": "Text for a deletion log entry when a deleted topic was restored. Parameters:\n* $1 - the user: link to the user page\n* $2 - the username. Can be used for GENDER.\n* $3 - the page where the topic was moderated\n* $4 - permalink URL to the topic (unused)\n* $5 - The topic title text\n* $6 - The board page\n{{Related|Logentry-flow}}",
+ "logentry-delete-flow-restore-topic-topic-title-not-visible": "Text for a deletion log entry when a deleted topic was restored, and the topic title is not visible to the current user. Parameters:\n* $1 - the user: link to the user page\n* $2 - the username. Can be used for GENDER.\n* $3 - The board page\n{{Related|Logentry-flow}}",
+ "logentry-suppress-flow-suppress-topic": "Text for a deletion log entry when a topic was suppressed. Parameters:\n* $1 - the user: link to the user page\n* $2 - the username. Can be used for GENDER.\n* $3 - the page where the topic was moderated\n* $4 - permalink URL to the topic (unused)\n* $5 - The topic title text\n* $6 - The board page\n{{Related|Logentry-flow}}",
+ "logentry-suppress-flow-suppress-topic-topic-title-not-visible": "Text for a deletion log entry when a topic was suppressed, and the topic title is not visible to the current user. Parameters:\n* $1 - the user: link to the user page\n* $2 - the username. Can be used for GENDER.\n* $3 - The board page\n{{Related|Logentry-flow}}",
"logentry-suppress-flow-restore-topic": "Text for a deletion log entry when a suppressed topic was restored. Parameters:\n* $1 - the user: link to the user page\n* $2 - the username. Can be used for GENDER.\n* $3 - the page where the topic was moderated\n* $4 - permalink URL to the moderated topic\n* $5 - The topic title text\n* $6 - The board page\n{{Related|Logentry-flow}}",
- "logentry-lock-flow-lock-topic": "Text for a log entry when a topic was marked as resolved. Parameters:\n* $1 - the user: link to the user page\n* $2 - the username. Can be used for GENDER.\n* $3 - the page where the topic was moderated\n* $5 - The topic title text\n* $6 - The board page\n{{Related|Logentry-flow}}",
+ "logentry-suppress-flow-restore-topic-topic-title-not-visible": "Text for a deletion log entry when a suppressed topic was restored, and the topic title is not visible to the current user. Parameters:\n* $1 - the user: link to the user page\n* $2 - the username. Can be used for GENDER.\n* $3 - The board page\n{{Related|Logentry-flow}}",
+ "logentry-lock-flow-lock-topic": "Text for a log entry when a topic was marked as resolved. Parameters:\n* $1 - the user: link to the user page\n* $2 - the username. Can be used for GENDER.\n* $3 - the page where the topic was moderated\n* $4 - permalink URL to the topic (unused)\n* $5 - The topic title text\n* $6 - The board page\n{{Related|Logentry-flow}}",
+ "logentry-lock-flow-lock-topic-topic-title-not-visible": "Text for a log entry when a topic was marked as resolved, and the topic title is not visible to the current user. Parameters:\n* $1 - the user: link to the user page\n* $2 - the username. Can be used for GENDER.\n* $3 - The board page\n{{Related|Logentry-flow}}",
"logentry-lock-flow-restore-topic": "Text for a log entry when a resolved topic was reopened. Parameters:\n* $1 - the user: link to the user page\n* $2 - the username. Can be used for GENDER.\n* $3 - the page where the topic was moderated\n* $4 - permalink URL to the moderated topic\n* $5 - The topic title text\n* $6 - The board page\n{{Related|Logentry-flow}}",
+ "logentry-lock-flow-restore-topic-topic-title-not-visible": "Text for a log entry when a resolved topic was reopened, and the topic title is not visible to the current user. Parameters:\n* $1 - the user: link to the user page\n* $2 - the username. Can be used for GENDER.\n* $3 - The board page\n{{Related|Logentry-flow}}",
"logentry-import-lqt-to-flow-topic": "Text for an import log entry when a topic has been imported from LiquidThreads to Flow. Parameters:\n* $1 - The page within the topic namespace to which the topic was imported.\n* $2 - The title of the LiquidThreads thread being imported.\n* $3 - The board that was converted from LiquidThreads to Flow.",
"flow-user-moderated": "Name to display when the current user is not allowed to see the users name due to moderation",
"flow-board-header-browse-topics-link": "Text to show in the board description which links to the topics list.",
diff --git a/includes/Formatter/RevisionFormatter.php b/includes/Formatter/RevisionFormatter.php
index 4a68ef5..06fcb8d 100644
--- a/includes/Formatter/RevisionFormatter.php
+++ b/includes/Formatter/RevisionFormatter.php
@@ -1005,7 +1005,7 @@ class RevisionFormatter {
}
$root = $revision->getRootPost();
- if ( !$this->permissions->isAllowed( $root, 'view' ) ) {
+ if ( !$this->permissions->isAllowed( $root, 'view-topic-title' ) ) {
return '';
}
@@ -1026,7 +1026,7 @@ class RevisionFormatter {
}
$root = $revision->getRootPost();
- if ( !$this->permissions->isAllowed( $root, 'view' ) ) {
+ if ( !$this->permissions->isAllowed( $root, 'view-topic-title' ) ) {
return '';
}
@@ -1041,7 +1041,8 @@ class RevisionFormatter {
/** @var PostRevision $post */
$post = $revision->getCollection()->getPost()->getLastRevision();
- if ( !$this->permissions->isAllowed( $post, 'view' ) ) {
+ $permissionAction = $post->isTopicTitle() ? 'view-topic-title' : 'view';
+ if ( !$this->permissions->isAllowed( $post, $permissionAction ) ) {
return '';
}
diff --git a/includes/Log/ActionFormatter.php b/includes/Log/ActionFormatter.php
index c39485b..7e9218f 100644
--- a/includes/Log/ActionFormatter.php
+++ b/includes/Log/ActionFormatter.php
@@ -8,6 +8,7 @@ use Flow\Data\ManagerGroup;
use Flow\Model\UUID;
use Flow\Conversion\Utils;
use Flow\Repository\TreeRepository;
+use Flow\Templating;
use Flow\UrlGenerator;
use Message;
@@ -18,11 +19,24 @@ class ActionFormatter extends \LogFormatter {
static $uuids = array();
/**
+ * @var RevisionActionPermissions
+ */
+ protected $permissions;
+
+ /**
+ * @var Templating
+ */
+ protected $templating;
+
+ /**
* @param \LogEntry $entry
*/
public function __construct( \LogEntry $entry ) {
parent::__construct( $entry );
+ $this->permissions = Container::get( 'permissions' );
+ $this->templating = Container::get( 'templating' );
+
$params = $this->entry->getParameters();
// serialized topicId or postId can be stored
foreach ( $params as $key => $value ) {
@@ -69,10 +83,6 @@ class ActionFormatter extends \LogFormatter {
$title = $this->entry->getTarget();
$params = $this->entry->getParameters();
- // @todo: we should probably check if user isAllowed( <this-revision>, 'log' )
- // unlike RC, Contributions, ... this one does not batch-load all Flow
- // revisions & does not use the same Formatter, i18n message text, etc
-
if ( isset( $params['postId'] ) ) {
/** @var UrlGenerator $urlGenerator */
$urlGenerator = Container::get( 'url_generator' );
@@ -86,22 +96,45 @@ class ActionFormatter extends \LogFormatter {
$title = $anchor->resolveTitle();
}
+ $rootLastRevision = $root->getLastRevision();
+
// Give grep a chance to find the usages:
- // logentry-delete-flow-delete-post, logentry-delete-flow-restore-post,
- // logentry-suppress-flow-restore-post, logentry-suppress-flow-suppress-post,
- // logentry-delete-flow-delete-topic, logentry-delete-flow-restore-topic,
- // logentry-suppress-flow-restore-topic, logentry-suppress-flow-suppress-topic,
- $message = $this->msg( "logentry-$type-$action" )
+ //
+ // A few of the -topic-title-not-visible are not reachable with the current
+ // config (since people looking at the suppression log can see suppressed
+ // content), but are included to make it less brittle.
+ //
+ // logentry-delete-flow-delete-post, logentry-delete-flow-delete-post-topic-title-not-visible,
+ // logentry-delete-flow-restore-post, logentry-delete-flow-restore-post-topic-title-not-visible,
+ // logentry-suppress-flow-restore-post, logentry-suppress-flow-restore-post-topic-title-not-visible,
+ // logentry-suppress-flow-suppress-post, logentry-suppress-flow-suppress-post-topic-title-not-visible,
+ // logentry-delete-flow-delete-topic, logentry-delete-flow-delete-topic-topic-title-not-visible,
+ // logentry-delete-flow-restore-topic, logentry-delete-flow-restore-topic-topic-title-not-visible,
+ // logentry-suppress-flow-restore-topic, logentry-suppress-flow-restore-topic-topic-title-not-visible,
+ // logentry-suppress-flow-suppress-topic, logentry-suppress-flow-suppress-topic-topic-title-not-visible,
+ // logentry-lock-flow-lock-topic, logentry-lock-flow-lock-topic-topic-title-not-visible
+ // logentry-lock-flow-restore-topic, logentry-lock-flow-restore-topic-topic-title-not-visible,
+ $messageKey = "logentry-$type-$action";
+ $isTopicTitleVisible = $this->permissions->isAllowed( $rootLastRevision, 'view-topic-title' );
+
+ if ( !$isTopicTitleVisible ) {
+ $messageKey .= '-topic-title-not-visible';
+ }
+
+ $message = $this->msg( $messageKey )
->params( array(
Message::rawParam( $this->getPerformerElement() ),
$this->entry->getPerformer()->getName(),
- $title, // link to topic
- $title->getFullUrl(), // link to topic, higlighted post
) );
- // I don't think this is an actual security issue, but this should use Templating->getContent: T119234
- // topic title
- $message->plaintextParams( $root->getLastRevision()->getContent( 'topic-title-plaintext' ) );
+ if ( $isTopicTitleVisible ) {
+ $message->params( array(
+ $title, // Title of topic
+ $title->getFullUrl(), // Full URL of topic, with highlighted post if applicable
+ ) );
+
+ $message->plaintextParams( $this->templating->getContent( $rootLastRevision, 'topic-title-plaintext' ) );
+ }
$message->params( $root->getWorkflow()->getOwnerTitle() ); // board title object
diff --git a/includes/Templating.php b/includes/Templating.php
index 4e76fe1..4270b28 100644
--- a/includes/Templating.php
+++ b/includes/Templating.php
@@ -127,7 +127,11 @@ class Templating {
throw new InvalidInputException( 'Invalid format: ' . $format );
}
- $allowed = $this->permissions->isAllowed( $revision, 'view' );
+ $mainPermissionAction = ( $revision instanceof PostRevision && $revision->isTopicTitle() ) ?
+ 'view-topic-title' :
+ 'view';
+
+ $allowed = $this->permissions->isAllowed( $revision, $mainPermissionAction );
// Posts require view access to the topic title as well
if ( $allowed && $revision instanceof PostRevision && !$revision->isTopicTitle() ) {
$allowed = $this->permissions->isAllowed(
--
2.1.4

File Metadata

Mime Type
text/x-diff
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
3801918
Default Alt Text
T137593.patch (22 KB)

Event Timeline