From 733d19d0d0d460b07ddda68fedce26cbf609c38f Mon Sep 17 00:00:00 2001 From: MatmaRex Date: Fri, 7 Dec 2012 22:10:12 +0100 Subject: [PATCH] (bug 40124) allow arbitrary user preferences prefixed with 'userjs-' Before change I98df55f2 it was possible to set arbitrary preferences (ie. with anything as the key) using the action=options API. That change removed this ability by enforcing full validation of the preferences, also introducing several regressions which were fixed by follow-ups. Per the discussion on bug 40124, this changeset aims to restore this ability, but in a slightly restricted way: arbitrary preferences' names must start with userjs- prefix, to avoid any possibility of conflicting with new MediaWiki versions or extensions. The contents of these preferences is not escaped, sanitized nor validated in any way; script authors are expected to sanitize them themselves to prevent XSS attacks and other security vulnerabilities. This commit also adds the User::getOptionsKinds() method (to determine whether given preference keys are used by MediaWiki itself or an extension, intended to be used via the API, or entirely unknown) and enhances the User::resetOptions() method to allow for resetting only preferences of chosen kinds. These changes allow for fixing of Special:Preferences not to clear those additional fields when saving user settings. Change-Id: I5f9ba5b0dfe7c2ea5458d836f03429cf6d93969d --- RELEASE-NOTES-1.21 | 6 + includes/Preferences.php | 5 +- includes/User.php | 107 ++++++++++++++++- includes/api/ApiOptions.php | 61 +++++----- includes/specials/SpecialPreferences.php | 2 +- tests/phpunit/includes/api/ApiOptionsTest.php | 109 +++++++++++++++--- 6 files changed, 239 insertions(+), 51 deletions(-) diff --git a/RELEASE-NOTES-1.21 b/RELEASE-NOTES-1.21 index fad46f53c8..ee49e09934 100644 --- a/RELEASE-NOTES-1.21 +++ b/RELEASE-NOTES-1.21 @@ -78,6 +78,12 @@ production. * $wgPageInfoTransclusionLimit limits the list size of transcluded articles on the info action. Default is 50. * Added action=createaccount to allow user account creation. +* (bug 40124) action=options API also allows for setting of arbitrary + preferences, provided that their names are prefixed with 'userjs-'. This + officially reenables the feature that was undocumented and defective + in MW 1.20 (saving preferences using Special:Preferences cleared any + additional fields) and which has been disabled in 1.20.1 as a part of + a security fix (bug 42202). === Bug fixes in 1.21 === * (bug 40353) SpecialDoubleRedirect should support interwiki redirects. diff --git a/includes/Preferences.php b/includes/Preferences.php index e41fbc0392..987839646d 100644 --- a/includes/Preferences.php +++ b/includes/Preferences.php @@ -1413,9 +1413,8 @@ class Preferences { $formData[$pref] = $user->getOption( $pref, null, true ); } - // Keeps old preferences from interfering due to back-compat - // code, etc. - $user->resetOptions(); + // Keep old preferences from interfering due to back-compat code, etc. + $user->resetOptions( 'unused', $form->getContext() ); foreach ( $formData as $key => $value ) { $user->setOption( $key, $value ); diff --git a/includes/User.php b/includes/User.php index ed3fe7630f..3c5c114359 100644 --- a/includes/User.php +++ b/includes/User.php @@ -2310,12 +2310,113 @@ class User { } /** - * Reset all options to the site defaults + * Return an associative array mapping preferences keys to the kind of a preference they're + * used for. Different kinds are handled differently when setting or reading preferences. + * + * Currently, the kind is one of: + * - 'registered' - preferences which are registered in core MediaWiki or + * by extensions using the UserGetDefaultOptions hook. + * - 'registered-multiselect' - as above, using the 'multiselect' type. + * - 'userjs' - preferences with names starting with 'userjs-', intended to + * be used by user scripts. + * - 'unused' - preferences about which MediaWiki doesn't know anything. + * These are usually legacy options, removed in newer versions. + * + * @param $context IContextSource + * @param $options array assoc. array with options keys to check as keys. Defaults to $this->mOptions. + * @return array the key => kind mapping data + */ + public function getOptionKinds( IContextSource $context, $options = null ) { + $this->loadOptions(); + if ( $options === null ) { + $options = $this->mOptions; + } + + $prefs = Preferences::getPreferences( $this, $context ); + $mapping = array(); + + // Multiselect options are stored in the database with one key per + // option, each having a boolean value. Extract those keys. + $multiselectOptions = array(); + foreach ( $prefs as $name => $info ) { + if ( ( isset( $info['type'] ) && $info['type'] == 'multiselect' ) || + ( isset( $info['class'] ) && $info['class'] == 'HTMLMultiSelectField' ) ) { + $opts = HTMLFormField::flattenOptions( $info['options'] ); + $prefix = isset( $info['prefix'] ) ? $info['prefix'] : $name; + + foreach ( $opts as $value ) { + $multiselectOptions["$prefix$value"] = true; + } + + unset( $prefs[$name] ); + } + } + + // $value is ignored + foreach ( $options as $key => $value ) { + if ( isset( $prefs[$key] ) ) { + $mapping[$key] = 'registered'; + } elseif( isset( $multiselectOptions[$key] ) ) { + $mapping[$key] = 'registered-multiselect'; + } elseif ( substr( $key, 0, 7 ) === 'userjs-' ) { + $mapping[$key] = 'userjs'; + } else { + $mapping[$key] = 'unused'; + } + } + + return $mapping; + } + + /** + * Reset certain (or all) options to the site defaults + * + * The optional parameter determines which kinds of preferences will be reset. + * Supported values are everything that can be reported by getOptionKinds() + * and 'all', which forces a reset of *all* preferences and overrides everything else. + * + * @param $resetKinds array|string which kinds of preferences to reset. Defaults to + * array( 'registered', 'registered-multiselect', 'unused' ) + * for backwards-compatibility. + * @param $context IContextSource|null context source used when $resetKinds + * does not contain 'all', passed to getOptionKinds(). + * Defaults to RequestContext::getMain() when null. */ - public function resetOptions() { + public function resetOptions( + $resetKinds = array( 'registered', 'registered-multiselect', 'unused' ), + IContextSource $context = null + ) { $this->load(); + $defaultOptions = self::getDefaultOptions(); - $this->mOptions = self::getDefaultOptions(); + if ( !is_array( $resetKinds ) ) { + $resetKinds = array( $resetKinds ); + } + + if ( in_array( 'all', $resetKinds ) ) { + $newOptions = $defaultOptions; + } else { + if ( $context === null ) { + $context = RequestContext::getMain(); + } + + $optionKinds = $this->getOptionKinds( $context ); + $newOptions = array(); + + // Use default values for the options that should be deleted, and + // copy old values for the ones that shouldn't. + foreach ( $this->mOptions as $key => $value ) { + if ( in_array( $optionKinds[$key], $resetKinds ) ) { + if ( array_key_exists( $key, $defaultOptions ) ) { + $newOptions[$key] = $defaultOptions[$key]; + } + } else { + $newOptions[$key] = $value; + } + } + } + + $this->mOptions = $newOptions; $this->mOptionsLoaded = true; } diff --git a/includes/api/ApiOptions.php b/includes/api/ApiOptions.php index 01a376073f..20b41858e1 100644 --- a/includes/api/ApiOptions.php +++ b/includes/api/ApiOptions.php @@ -54,7 +54,7 @@ class ApiOptions extends ApiBase { } if ( $params['reset'] ) { - $user->resetOptions(); + $user->resetOptions( 'all' ); $changed = true; } @@ -74,35 +74,34 @@ class ApiOptions extends ApiBase { } $prefs = Preferences::getPreferences( $user, $this->getContext() ); - - // Multiselect options are stored in the database with one key per - // option, each having a boolean value. Extract those keys. - $multiselectOptions = array(); - foreach ( $prefs as $name => $info ) { - if ( ( isset( $info['type'] ) && $info['type'] == 'multiselect' ) || - ( isset( $info['class'] ) && $info['class'] == 'HTMLMultiSelectField' ) ) { - $options = HTMLFormField::flattenOptions( $info['options'] ); - $prefix = isset( $info['prefix'] ) ? $info['prefix'] : $name; - - foreach ( $options as $value ) { - $multiselectOptions["$prefix$value"] = true; - } - - unset( $prefs[$name] ); - } - } + $prefsKinds = $user->getOptionKinds( $this->getContext(), $changes ); foreach ( $changes as $key => $value ) { - if ( isset( $prefs[$key] ) ) { - $field = HTMLForm::loadInputFromParameters( $key, $prefs[$key] ); - $validation = $field->validate( $value, $user->getOptions() ); - } elseif( isset( $multiselectOptions[$key] ) ) { - // A key for a multiselect option. - $validation = true; - $value = (bool)$value; - } else { - $this->setWarning( "Not a valid preference: $key" ); - continue; + switch ( $prefsKinds[$key] ) { + case 'registered': + // Regular option. + $field = HTMLForm::loadInputFromParameters( $key, $prefs[$key] ); + $validation = $field->validate( $value, $user->getOptions() ); + break; + case 'registered-multiselect': + // A key for a multiselect option. + $validation = true; + $value = (bool)$value; + break; + case 'userjs': + // Allow non-default preferences prefixed with 'userjs-', to be set by user scripts + if ( strlen( $key ) > 255 ) { + $validation = "key too long (no more than 255 bytes allowed)"; + } elseif ( preg_match( "/[^a-zA-Z0-9_-]/", $key ) !== 0 ) { + $validation = "invalid key (only a-z, A-Z, 0-9, _, - allowed)"; + } else { + $validation = true; + } + break; + case 'unused': + default: + $validation = "not a valid preference"; + break; } if ( $validation === true ) { $user->setOption( $key, $value ); @@ -170,7 +169,11 @@ class ApiOptions extends ApiBase { } public function getDescription() { - return 'Change preferences of the current user'; + return array( + 'Change preferences of the current user', + 'Only options which are registered in core or in one of installed extensions,', + 'or as options with keys prefixed with \'userjs-\' (intended to be used by user scripts), can be set.' + ); } public function getPossibleErrors() { diff --git a/includes/specials/SpecialPreferences.php b/includes/specials/SpecialPreferences.php index c6b2bb6b9d..340172c314 100644 --- a/includes/specials/SpecialPreferences.php +++ b/includes/specials/SpecialPreferences.php @@ -78,7 +78,7 @@ class SpecialPreferences extends SpecialPage { public function submitReset( $formData ) { $user = $this->getUser(); - $user->resetOptions(); + $user->resetOptions( 'all' ); $user->saveSettings(); $url = $this->getTitle()->getFullURL( 'success' ); diff --git a/tests/phpunit/includes/api/ApiOptionsTest.php b/tests/phpunit/includes/api/ApiOptionsTest.php index c564403a9f..23314598e8 100644 --- a/tests/phpunit/includes/api/ApiOptionsTest.php +++ b/tests/phpunit/includes/api/ApiOptionsTest.php @@ -22,7 +22,11 @@ class ApiOptionsTest extends MediaWikiLangTestCase { // Set up groups $this->mUserMock->expects( $this->any() ) - ->method( 'getEffectiveGroups' )->will( $this->returnValue( array( '*', 'user')) ); + ->method( 'getEffectiveGroups' )->will( $this->returnValue( array( '*', 'user' ) ) ); + + // Set up callback for User::getOptionKinds + $this->mUserMock->expects( $this->any() ) + ->method( 'getOptionKinds' )->will( $this->returnCallback( array( $this, 'getOptionKinds' ) ) ); // Create a new context $this->mContext = new DerivativeContext( new RequestContext() ); @@ -56,6 +60,8 @@ class ApiOptionsTest extends MediaWikiLangTestCase { } public function hookGetPreferences( $user, &$preferences ) { + $preferences = array(); + foreach ( array( 'name', 'willBeNull', 'willBeEmpty', 'willBeHappy' ) as $k ) { $preferences[$k] = array( 'type' => 'text', @@ -83,6 +89,36 @@ class ApiOptionsTest extends MediaWikiLangTestCase { return true; } + public function getOptionKinds( IContextSource $context, $options = null ) { + // Match with above. + $kinds = array( + 'name' => 'registered', + 'willBeNull' => 'registered', + 'willBeEmpty' => 'registered', + 'willBeHappy' => 'registered', + 'testmultiselect-opt1' => 'registered-multiselect', + 'testmultiselect-opt2' => 'registered-multiselect', + 'testmultiselect-opt3' => 'registered-multiselect', + 'testmultiselect-opt4' => 'registered-multiselect', + ); + + if ( $options === null ) { + return $kinds; + } + + $mapping = array(); + foreach ( $options as $key => $value ) { + if ( isset( $kinds[$key] ) ) { + $mapping[$key] = $kinds[$key]; + } elseif ( substr( $key, 0, 7 ) === 'userjs-' ) { + $mapping[$key] = 'userjs'; + } else { + $mapping[$key] = 'unused'; + } + } + return $mapping; + } + private function getSampleRequest( $custom = array() ) { $request = array( 'token' => '123ABC', @@ -216,24 +252,24 @@ class ApiOptionsTest extends MediaWikiLangTestCase { $this->mUserMock->expects( $this->never() ) ->method( 'resetOptions' ); - $this->mUserMock->expects( $this->at( 1 ) ) + $this->mUserMock->expects( $this->at( 2 ) ) ->method( 'getOptions' ); - $this->mUserMock->expects( $this->at( 2 ) ) + $this->mUserMock->expects( $this->at( 3 ) ) ->method( 'setOption' ) ->with( $this->equalTo( 'willBeNull' ), $this->equalTo( null ) ); - $this->mUserMock->expects( $this->at( 3 ) ) + $this->mUserMock->expects( $this->at( 4 ) ) ->method( 'getOptions' ); - $this->mUserMock->expects( $this->at( 4 ) ) + $this->mUserMock->expects( $this->at( 5 ) ) ->method( 'setOption' ) ->with( $this->equalTo( 'willBeEmpty' ), $this->equalTo( '' ) ); - $this->mUserMock->expects( $this->at( 5 ) ) + $this->mUserMock->expects( $this->at( 6 ) ) ->method( 'getOptions' ); - $this->mUserMock->expects( $this->at( 6 ) ) + $this->mUserMock->expects( $this->at( 7 ) ) ->method( 'setOption' ) ->with( $this->equalTo( 'willBeHappy' ), $this->equalTo( 'Happy' ) ); @@ -251,17 +287,17 @@ class ApiOptionsTest extends MediaWikiLangTestCase { $this->mUserMock->expects( $this->once() ) ->method( 'resetOptions' ); - $this->mUserMock->expects( $this->at( 2 ) ) + $this->mUserMock->expects( $this->at( 3 ) ) ->method( 'getOptions' ); - $this->mUserMock->expects( $this->at( 3 ) ) + $this->mUserMock->expects( $this->at( 4 ) ) ->method( 'setOption' ) ->with( $this->equalTo( 'willBeHappy' ), $this->equalTo( 'Happy' ) ); - $this->mUserMock->expects( $this->at( 4 ) ) + $this->mUserMock->expects( $this->at( 5 ) ) ->method( 'getOptions' ); - $this->mUserMock->expects( $this->at( 5 ) ) + $this->mUserMock->expects( $this->at( 6 ) ) ->method( 'setOption' ) ->with( $this->equalTo( 'name' ), $this->equalTo( 'value' ) ); @@ -284,19 +320,19 @@ class ApiOptionsTest extends MediaWikiLangTestCase { $this->mUserMock->expects( $this->never() ) ->method( 'resetOptions' ); - $this->mUserMock->expects( $this->at( 1 ) ) + $this->mUserMock->expects( $this->at( 2 ) ) ->method( 'setOption' ) ->with( $this->equalTo( 'testmultiselect-opt1' ), $this->equalTo( true ) ); - $this->mUserMock->expects( $this->at( 2 ) ) + $this->mUserMock->expects( $this->at( 3 ) ) ->method( 'setOption' ) ->with( $this->equalTo( 'testmultiselect-opt2' ), $this->equalTo( false ) ); - $this->mUserMock->expects( $this->at( 3 ) ) + $this->mUserMock->expects( $this->at( 4 ) ) ->method( 'setOption' ) ->with( $this->equalTo( 'testmultiselect-opt3' ), $this->equalTo( false ) ); - $this->mUserMock->expects( $this->at( 4 ) ) + $this->mUserMock->expects( $this->at( 5 ) ) ->method( 'setOption' ) ->with( $this->equalTo( 'testmultiselect-opt4' ), $this->equalTo( false ) ); @@ -311,4 +347,47 @@ class ApiOptionsTest extends MediaWikiLangTestCase { $this->assertEquals( self::$Success, $response ); } + + public function testUnknownOption() { + $this->mUserMock->expects( $this->never() ) + ->method( 'resetOptions' ); + + $this->mUserMock->expects( $this->never() ) + ->method( 'saveSettings' ); + + $request = $this->getSampleRequest( array( + 'change' => 'unknownOption=1' + ) ); + + $response = $this->executeQuery( $request ); + + $this->assertEquals( array( + 'options' => 'success', + 'warnings' => array( + 'options' => array( + '*' => "Validation error for 'unknownOption': not a valid preference" + ) + ) + ), $response ); + } + + public function testUserjsOption() { + $this->mUserMock->expects( $this->never() ) + ->method( 'resetOptions' ); + + $this->mUserMock->expects( $this->at( 2 ) ) + ->method( 'setOption' ) + ->with( $this->equalTo( 'userjs-option' ), $this->equalTo( '1' ) ); + + $this->mUserMock->expects( $this->once() ) + ->method( 'saveSettings' ); + + $request = $this->getSampleRequest( array( + 'change' => 'userjs-option=1' + ) ); + + $response = $this->executeQuery( $request ); + + $this->assertEquals( self::$Success, $response ); + } } -- 2.20.1