From f92d935408aac23d175c70f73bcdea1955f1ded1 Mon Sep 17 00:00:00 2001 From: Matthew Flaschen 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( , '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