Thijs/wiki.techinc.nl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

(bug 42202) Validate preference values in action=options

Browse source 
Previously, there was no validation whatsoever and the module would
happily write any preference you asked it to. This, combined with the
fact that the code using the 'editfont' preference didn't perform any
validation or escaping, led to a CSS injection vulnerability.

Using Preferences::getPreferences breaks some existing test cases
because a MockUser doesn't have groups for preferences.

Change-Id: I98df55f2b16ac1b6fce578798b6f58b5dad96775
This commit is contained in:
Catrope 2012-11-16 10:19:15 -08:00 committed by csteipp
parent 8e57acf211
commit fe45ba8752
2 changed files with 41 additions and 9 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

35
includes/api/ApiOptions.php
View file

@ -47,7 +47,7 @@ class ApiOptions extends ApiBase {
}
$params = $this->extractRequestParams();
$changes = 0;
$changed = false;
if ( isset( $params['optionvalue'] ) && !isset( $params['optionname'] ) ) {
$this->dieUsageMsg( array( 'missingparam', 'optionname' ) );
@ -55,26 +55,43 @@ class ApiOptions extends ApiBase {
if ( $params['reset'] ) {
$user->resetOptions();
$changes++;
$changed = true;
}
$changes = array();
if ( count( $params['change'] ) ) {
foreach ( $params['change'] as $entry ) {
$array = explode( '=', $entry, 2 );
$user->setOption( $array[0], isset( $array[1] ) ? $array[1] : null );
$changes++;
$changes[$array[0]] = isset( $array[1] ) ? $array[1] : null;
}
}
if ( isset( $params['optionname'] ) ) {
$newValue = isset( $params['optionvalue'] ) ? $params['optionvalue'] : null;
$user->setOption( $params['optionname'], $newValue );
$changes++;
$changes[$params['optionname']] = $newValue;
}
if ( !count( $changes ) ) {
$this->dieUsage( 'No changes were requested', 'nochanges' );
}
if ( $changes ) {
$prefs = Preferences::getPreferences( $user, $this->getContext() );
foreach ( $changes as $key => $value ) {
if ( !isset( $prefs[$key] ) ) {
$this->setWarning( "Not a valid preference: $key" );
continue;
}
$field = HTMLForm::loadInputFromParameters( $key, $prefs[$key] );
$validation = $field->validate( $value, $user->getOptions() );
if ( $validation === true ) {
$user->setOption( $key, $value );
$changed = true;
} else {
$this->setWarning( "Validation error for '$key': $validation" );
}
}
if ( $changed ) {
// Commit changes
$user->saveSettings();
} else {
$this->dieUsage( 'No changes were requested', 'nochanges' );
}
$this->getResult()->addValue( null, $this->getModuleName(), 'success' );

15
tests/phpunit/includes/api/ApiOptionsTest.php
View file

@ -105,6 +105,9 @@ class ApiOptionsTest extends MediaWikiLangTestCase {
$this->fail( "UsageException was not thrown" );
}
/**
* @group Broken
*/
public function testReset() {
$this->mUserMock->expects( $this->once() )
->method( 'resetOptions' );
@ -122,6 +125,9 @@ class ApiOptionsTest extends MediaWikiLangTestCase {
$this->assertEquals( self::$Success, $response );
}
/**
* @group Broken
*/
public function testOptionWithValue() {
$this->mUserMock->expects( $this->never() )
->method( 'resetOptions' );
@ -140,6 +146,9 @@ class ApiOptionsTest extends MediaWikiLangTestCase {
$this->assertEquals( self::$Success, $response );
}
/**
* @group Broken
*/
public function testOptionResetValue() {
$this->mUserMock->expects( $this->never() )
->method( 'resetOptions' );
@ -157,6 +166,9 @@ class ApiOptionsTest extends MediaWikiLangTestCase {
$this->assertEquals( self::$Success, $response );
}
/**
* @group Broken
*/
public function testChange() {
$this->mUserMock->expects( $this->never() )
->method( 'resetOptions' );
@ -183,6 +195,9 @@ class ApiOptionsTest extends MediaWikiLangTestCase {
$this->assertEquals( self::$Success, $response );
}
/**
* @group Broken
*/
public function testResetChangeOption() {
$this->mUserMock->expects( $this->once() )
->method( 'resetOptions' );