From 77cdf1919a418fa1c306b066259b2a4e4300eb6d Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Mon, 28 Sep 2015 14:32:45 -0700 Subject: [PATCH] WatchAction: Require POST for index.php action=watch The GET variant was already rarely used because our frontend enchances these links with a click handler that uses AJAX to make a POST request to the API. The index.php url, nor its token, were used for the majority of users. Simplify this by stripping the 'token' query from these urls and requiring a POST request for index.php?action=watch and unwatch. * FormAction: Actually set a proper '
' instead of letting HTMLForm default to a confusing title path (e.g. /wiki/Pagename). Article path should not be used for POST requests. * WatchAction: Group all FormAction-related methods together. * WatchAction: Make token consistent with other actions now that it is POST-only (no "stronger" salt containing the page title). * Remove ununsed mediawiki.page.startup dependency from mediawiki.page.watch.ajax. * WatchAction: If accessed over GET directly (e.g. for users without javascript) display a confirmation form that submits the token. Similar to PurgeAction. Change-Id: I504f457e68a133bcfc418cff13b838080fec1008 --- includes/actions/Action.php | 2 +- includes/actions/FormAction.php | 3 +- includes/actions/WatchAction.php | 49 +++++++++------------ includes/skins/SkinTemplate.php | 3 +- includes/specials/SpecialUnwatchedpages.php | 3 +- resources/Resources.php | 1 - 6 files changed, 27 insertions(+), 34 deletions(-) diff --git a/includes/actions/Action.php b/includes/actions/Action.php index 43f03c3a4f..6ddc596b9d 100644 --- a/includes/actions/Action.php +++ b/includes/actions/Action.php @@ -370,7 +370,7 @@ abstract class Action { * Returns the description that goes below the \ tag * @since 1.17 * - * @return string + * @return string HTML */ protected function getDescription() { return $this->msg( strtolower( $this->getName() ) )->escaped(); diff --git a/includes/actions/FormAction.php b/includes/actions/FormAction.php index 26f43cb0c2..9decf9068d 100644 --- a/includes/actions/FormAction.php +++ b/includes/actions/FormAction.php @@ -68,8 +68,9 @@ abstract class FormAction extends Action { $form = new HTMLForm( $this->fields, $this->getContext(), $this->getName() ); $form->setSubmitCallback( array( $this, 'onSubmit' ) ); + $title = $this->getTitle(); + $form->setAction( $title->getLocalURL( array( 'action' => $this->getName() ) ) ); // Retain query parameters (uselang etc) - $form->addHiddenField( 'action', $this->getName() ); // Might not be the same as the query string $params = array_diff_key( $this->getRequest()->getQueryValues(), array( 'action' => null, 'title' => null ) diff --git a/includes/actions/WatchAction.php b/includes/actions/WatchAction.php index 8b6e329d07..51108c07a8 100644 --- a/includes/actions/WatchAction.php +++ b/includes/actions/WatchAction.php @@ -35,6 +35,9 @@ class WatchAction extends FormAction { return false; } + /** + * @return string HTML + */ protected function getDescription() { return $this->msg( 'addwatch' )->escaped(); } @@ -63,17 +66,9 @@ class WatchAction extends FormAction { // This will throw exceptions if there's a problem $this->checkCanExecute( $user ); - // Must have valid token for this action/title - $salt = array( $this->getName(), $this->getTitle()->getPrefixedDBkey() ); - - if ( $user->matchEditToken( $this->getRequest()->getVal( 'token' ), $salt ) ) { - $this->onSubmit( array() ); + $form = $this->getForm(); + if ( $form->show() ) { $this->onSuccess(); - } else { - $form = $this->getForm(); - if ( $form->show() ) { - $this->onSuccess(); - } } } @@ -86,6 +81,21 @@ class WatchAction extends FormAction { parent::checkCanExecute( $user ); } + protected function alterForm( HTMLForm $form ) { + $form->setSubmitTextMsg( 'confirm-watch-button' ); + $form->setTokenSalt( 'watch' ); + } + + protected function preText() { + return $this->msg( 'confirm-watch-top' )->parse(); + } + + public function onSuccess() { + $this->getOutput()->addWikiMsg( 'addedwatchtext', $this->getTitle()->getPrefixedText() ); + } + + /* Static utility methods */ + /** * Watch or unwatch a page * @since 1.22 @@ -176,11 +186,8 @@ class WatchAction extends FormAction { if ( $action != 'unwatch' ) { $action = 'watch'; } - $salt = array( $action, $title->getPrefixedDBkey() ); - - // This token stronger salted and not compatible with ApiWatch - // It's title/action specific because index.php is GET and API is POST - return $user->getEditToken( $salt ); + // Match ApiWatch and ResourceLoaderUserTokensModule + return $user->getEditToken( $action ); } /** @@ -195,16 +202,4 @@ class WatchAction extends FormAction { public static function getUnwatchToken( Title $title, User $user, $action = 'unwatch' ) { return self::getWatchToken( $title, $user, $action ); } - - protected function alterForm( HTMLForm $form ) { - $form->setSubmitTextMsg( 'confirm-watch-button' ); - } - - protected function preText() { - return $this->msg( 'confirm-watch-top' )->parse(); - } - - public function onSuccess() { - $this->getOutput()->addWikiMsg( 'addedwatchtext', $this->getTitle()->getPrefixedText() ); - } } diff --git a/includes/skins/SkinTemplate.php b/includes/skins/SkinTemplate.php index ae6993457b..4d7c03a1ee 100644 --- a/includes/skins/SkinTemplate.php +++ b/includes/skins/SkinTemplate.php @@ -1019,12 +1019,11 @@ class SkinTemplate extends Skin { * the global versions. */ $mode = $user->isWatched( $title ) ? 'unwatch' : 'watch'; - $token = WatchAction::getWatchToken( $title, $user, $mode ); $content_navigation['actions'][$mode] = array( 'class' => $onPage && ( $action == 'watch' || $action == 'unwatch' ) ? 'selected' : false, // uses 'watch' or 'unwatch' message 'text' => $this->msg( $mode )->text(), - 'href' => $title->getLocalURL( array( 'action' => $mode, 'token' => $token ) ) + 'href' => $title->getLocalURL( array( 'action' => $mode ) ) ); } } diff --git a/includes/specials/SpecialUnwatchedpages.php b/includes/specials/SpecialUnwatchedpages.php index b3ca006c71..4ad6ac077a 100644 --- a/includes/specials/SpecialUnwatchedpages.php +++ b/includes/specials/SpecialUnwatchedpages.php @@ -96,12 +96,11 @@ class UnwatchedpagesPage extends QueryPage { $text = $wgContLang->convert( $nt->getPrefixedText() ); $plink = Linker::linkKnown( $nt, htmlspecialchars( $text ) ); - $token = WatchAction::getWatchToken( $nt, $this->getUser() ); $wlink = Linker::linkKnown( $nt, $this->msg( 'watch' )->escaped(), array( 'class' => 'mw-watch-link' ), - array( 'action' => 'watch', 'token' => $token ) + array( 'action' => 'watch' ) ); return $this->getLanguage()->specialList( $plink, $wlink ); diff --git a/resources/Resources.php b/resources/Resources.php index 335775f100..107ccecf4c 100644 --- a/resources/Resources.php +++ b/resources/Resources.php @@ -1532,7 +1532,6 @@ return array( 'dependencies' => array( 'mediawiki.api.watch', 'mediawiki.notify', - 'mediawiki.page.startup', 'mediawiki.util', 'jquery.accessKeyLabel', 'mediawiki.RegExp', -- 2.20.1