(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

diff --git a/RELEASE-NOTES-1.21 b/RELEASE-NOTES-1.21
index fad46f5..ee49e09 100644 (file)
--- 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 e41fbc0..9878396 100644 (file)
--- 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 ed3fe76..3c5c114 100644 (file)
--- 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 01a3760..20b4185 100644 (file)
--- 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 c6b2bb6..340172c 100644 (file)
--- 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 c564403..2331459 100644 (file)
--- 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 );
+       }
 }