From 75a85b412c4adb86d3cea6986a4c0394f7a531be Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Tue, 16 Aug 2016 16:36:27 -0400 Subject: [PATCH] API: Use U+001F (Unit Separator) for separating multi-valued parameters MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit When a multi-valued parameter's value begins with U+001F, the values will be split on that character instead of pipes. This will be useful for things such as action=options&change= or meta=allmessages&amargs=. Since MediaWiki doesn't otherwise accept C0 control characters (WebRequest::getVal() replaces them with �), there's no possibility that this will conflict with a literal use of U+001F. Special:ApiSandbox and mw.Api are updated to make use of this, with the latter having an option to disable the behavior in case something is depending on [ 'foo', 'bar|baz' ] turning into 'foo|bar|baz'. Pipe is still used as the separator when the value doesn't begin with U+001F, and will be forever since it's generally more human-friendly and is needed for backwards compatibility with basically every API client in existence. The requirement that the value begin with U+001F, rather than simply contain U+001F, is to avoid clients having to somehow special-case "param=foo|bar" where that's intended to be a single value "foo|bar" rather than two values "foo" and "bar". Bug: T141960 Change-Id: I45f69997667b48887a2b67e93906364a652ace5a --- RELEASE-NOTES-1.28 | 6 ++ includes/WebRequest.php | 26 +++++++ includes/api/ApiBase.php | 42 +++++++++- includes/api/i18n/en.json | 6 +- .../mediawiki.special.apisandbox.js | 18 ++++- resources/src/mediawiki/api.js | 16 +++- resources/src/mediawiki/api/options.js | 10 ++- tests/phpunit/includes/api/ApiBaseTest.php | 76 +++++++++++++++++++ tests/phpunit/includes/api/MockApi.php | 6 ++ .../mediawiki.api.options.test.js | 66 +++++++++++++++- 10 files changed, 257 insertions(+), 15 deletions(-) diff --git a/RELEASE-NOTES-1.28 b/RELEASE-NOTES-1.28 index d31943d952..58d9a0791e 100644 --- a/RELEASE-NOTES-1.28 +++ b/RELEASE-NOTES-1.28 @@ -47,6 +47,9 @@ production. collation, you will need to run the updateCollation.php maintenance script. * Two new codes have been added to #time parser function: "xit" for days in current month, and "xiz" for days passed in the year, both in Iranian calendar. +* mw.Api has a new option, useUS, to use U+001F (Unit Separator) when + appropriate for sending multi-valued parameters. This defaults to true when + the mw.Api instance seems to be for the local wiki. === External library changes in 1.28 === @@ -84,6 +87,9 @@ production. action=createaccount, action=linkaccount, and action=changeauthenticationdata in the query string is now deprecated and outputs a warning. They should be submitted in the POST body instead. +* (T141960) Multi-valued parameters may now be separated using U+001F (Unit Separator) + instead of the pipe character. This will be useful if some of the multiple + values need to contain pipes, e.g. for action=options. === Action API internal changes in 1.28 === * Added a new hook, 'ApiMakeParserOptions', to allow extensions to better diff --git a/includes/WebRequest.php b/includes/WebRequest.php index b5c57ee972..1e27f9876f 100644 --- a/includes/WebRequest.php +++ b/includes/WebRequest.php @@ -394,6 +394,32 @@ class WebRequest { } } + /** + * Fetch a scalar from the input without normalization, or return $default + * if it's not set. + * + * Unlike self::getVal(), this does not perform any normalization on the + * input value. + * + * @since 1.28 + * @param string $name + * @param string|null $default Optional default + * @return string + */ + public function getRawVal( $name, $default = null ) { + $name = strtr( $name, '.', '_' ); // See comment in self::getGPCVal() + if ( isset( $this->data[$name] ) && !is_array( $this->data[$name] ) ) { + $val = $this->data[$name]; + } else { + $val = $default; + } + if ( is_null( $val ) ) { + return $val; + } else { + return (string)$val; + } + } + /** * Fetch a scalar from the input or return $default if it's not set. * Returns a string. Arrays are discarded. Useful for diff --git a/includes/api/ApiBase.php b/includes/api/ApiBase.php index 66c1b530de..ca890913f1 100644 --- a/includes/api/ApiBase.php +++ b/includes/api/ApiBase.php @@ -1000,6 +1000,26 @@ abstract class ApiBase extends ContextSource { $type = $this->getModuleManager()->getNames( $paramName ); } } + + $request = $this->getMain()->getRequest(); + $rawValue = $request->getRawVal( $encParamName ); + if ( $rawValue === null ) { + $rawValue = $default; + } + + // Preserve U+001F for self::parseMultiValue(), or error out if that won't be called + if ( isset( $value ) && substr( $rawValue, 0, 1 ) === "\x1f" ) { + if ( $multi ) { + // This loses the potential $wgContLang->checkTitleEncoding() transformation + // done by WebRequest for $_GET. Let's call that a feature. + $value = join( "\x1f", $request->normalizeUnicode( explode( "\x1f", $rawValue ) ) ); + } else { + $this->dieUsage( + "U+001F multi-value separation may only be used for multi-valued parameters.", + 'badvalue_notmultivalue' + ); + } + } } if ( isset( $value ) && ( $multi || is_array( $type ) ) ) { @@ -1145,6 +1165,24 @@ abstract class ApiBase extends ContextSource { return $value; } + /** + * Split a multi-valued parameter string, like explode() + * @since 1.28 + * @param string $value + * @param int $limit + * @return string[] + */ + protected function explodeMultiValue( $value, $limit ) { + if ( substr( $value, 0, 1 ) === "\x1f" ) { + $sep = "\x1f"; + $value = substr( $value, 1 ); + } else { + $sep = '|'; + } + + return explode( $sep, $value, $limit ); + } + /** * Return an array of values that were given in a 'a|b|c' notation, * after it optionally validates them against the list allowed values. @@ -1159,13 +1197,13 @@ abstract class ApiBase extends ContextSource { * @return string|string[] (allowMultiple ? an_array_of_values : a_single_value) */ protected function parseMultiValue( $valueName, $value, $allowMultiple, $allowedValues ) { - if ( trim( $value ) === '' && $allowMultiple ) { + if ( ( trim( $value ) === '' || trim( $value ) === "\x1f" ) && $allowMultiple ) { return []; } // This is a bit awkward, but we want to avoid calling canApiHighLimits() // because it unstubs $wgUser - $valuesList = explode( '|', $value, self::LIMIT_SML2 + 1 ); + $valuesList = $this->explodeMultiValue( $value, self::LIMIT_SML2 + 1 ); $sizeLimit = count( $valuesList ) > self::LIMIT_SML1 && $this->mMainModule->canApiHighLimits() ? self::LIMIT_SML2 : self::LIMIT_SML1; diff --git a/includes/api/i18n/en.json b/includes/api/i18n/en.json index 1815836235..68147c22e3 100644 --- a/includes/api/i18n/en.json +++ b/includes/api/i18n/en.json @@ -1481,14 +1481,14 @@ "api-help-param-deprecated": "Deprecated.", "api-help-param-required": "This parameter is required.", "api-help-datatypes-header": "Data types", - "api-help-datatypes": "Some parameter types in API requests need further explanation:\n;boolean\n:Boolean parameters work like HTML checkboxes: if the parameter is specified, regardless of value, it is considered true. For a false value, omit the parameter entirely.\n;timestamp\n:Timestamps may be specified in several formats. ISO 8601 date and time is recommended. All times are in UTC, any included timezone is ignored.\n:* ISO 8601 date and time, 2001-01-15T14:56:00Z (punctuation and Z are optional)\n:* ISO 8601 date and time with (ignored) fractional seconds, 2001-01-15T14:56:00.00001Z (dashes, colons, and Z are optional)\n:* MediaWiki format, 20010115145600\n:* Generic numeric format, 2001-01-15 14:56:00 (optional timezone of GMT, +##, or -## is ignored)\n:* EXIF format, 2001:01:15 14:56:00\n:*RFC 2822 format (timezone may be omitted), Mon, 15 Jan 2001 14:56:00\n:* RFC 850 format (timezone may be omitted), Monday, 15-Jan-2001 14:56:00\n:* C ctime format, Mon Jan 15 14:56:00 2001\n:* Seconds since 1970-01-01T00:00:00Z as a 1 to 13 digit integer (excluding 0)\n:* The string now", + "api-help-datatypes": "Some parameter types in API requests need further explanation:\n;boolean\n:Boolean parameters work like HTML checkboxes: if the parameter is specified, regardless of value, it is considered true. For a false value, omit the parameter entirely.\n;timestamp\n:Timestamps may be specified in several formats. ISO 8601 date and time is recommended. All times are in UTC, any included timezone is ignored.\n:* ISO 8601 date and time, 2001-01-15T14:56:00Z (punctuation and Z are optional)\n:* ISO 8601 date and time with (ignored) fractional seconds, 2001-01-15T14:56:00.00001Z (dashes, colons, and Z are optional)\n:* MediaWiki format, 20010115145600\n:* Generic numeric format, 2001-01-15 14:56:00 (optional timezone of GMT, +##, or -## is ignored)\n:* EXIF format, 2001:01:15 14:56:00\n:*RFC 2822 format (timezone may be omitted), Mon, 15 Jan 2001 14:56:00\n:* RFC 850 format (timezone may be omitted), Monday, 15-Jan-2001 14:56:00\n:* C ctime format, Mon Jan 15 14:56:00 2001\n:* Seconds since 1970-01-01T00:00:00Z as a 1 to 13 digit integer (excluding 0)\n:* The string now\n;alternative multiple-value separator\n:Parameters that take multiple values are normally submitted with the values separated using the pipe character, e.g. param=value1|value2 or param=value1%7Cvalue2. If a value must contain the pipe character, use U+001F (Unit Separator) as the separator ''and'' prefix the value with U+001F, e.g. param=%1Fvalue1%1Fvalue2.", "api-help-param-type-limit": "Type: integer or max", "api-help-param-type-integer": "Type: {{PLURAL:$1|1=integer|2=list of integers}}", "api-help-param-type-boolean": "Type: boolean ([[Special:ApiHelp/main#main/datatypes|details]])", "api-help-param-type-password": "", "api-help-param-type-timestamp": "Type: {{PLURAL:$1|1=timestamp|2=list of timestamps}} ([[Special:ApiHelp/main#main/datatypes|allowed formats]])", "api-help-param-type-user": "Type: {{PLURAL:$1|1=user name|2=list of user names}}", - "api-help-param-list": "{{PLURAL:$1|1=One of the following values|2=Values (separate with {{!}})}}: $2", + "api-help-param-list": "{{PLURAL:$1|1=One of the following values|2=Values (separate with {{!}} or [[Special:ApiHelp/main#main/datatypes|alternative]])}}: $2", "api-help-param-list-can-be-empty": "{{PLURAL:$1|0=Must be empty|Can be empty, or $2}}", "api-help-param-limit": "No more than $1 allowed.", "api-help-param-limit2": "No more than $1 ($2 for bots) allowed.", @@ -1496,7 +1496,7 @@ "api-help-param-integer-max": "The {{PLURAL:$1|1=value|2=values}} must be no greater than $3.", "api-help-param-integer-minmax": "The {{PLURAL:$1|1=value|2=values}} must be between $2 and $3.", "api-help-param-upload": "Must be posted as a file upload using multipart/form-data.", - "api-help-param-multi-separate": "Separate values with |.", + "api-help-param-multi-separate": "Separate values with | or [[Special:ApiHelp/main#main/datatypes|alternative]].", "api-help-param-multi-max": "Maximum number of values is {{PLURAL:$1|$1}} ({{PLURAL:$2|$2}} for bots).", "api-help-param-default": "Default: $1", "api-help-param-default-empty": "Default: (empty)", diff --git a/resources/src/mediawiki.special/mediawiki.special.apisandbox.js b/resources/src/mediawiki.special/mediawiki.special.apisandbox.js index 3d90307e81..5c3715db02 100644 --- a/resources/src/mediawiki.special/mediawiki.special.apisandbox.js +++ b/resources/src/mediawiki.special/mediawiki.special.apisandbox.js @@ -116,10 +116,24 @@ capsuleWidget: { getApiValue: function () { - return this.getItemsData().join( '|' ); + var items = this.getItemsData(); + if ( items.join( '' ).indexOf( '|' ) === -1 ) { + return items.join( '|' ); + } else { + return '\x1f' + items.join( '\x1f' ); + } }, setApiValue: function ( v ) { - this.setItemsFromData( v === undefined || v === '' ? [] : String( v ).split( '|' ) ); + if ( v === undefined || v === '' || v === '\x1f' ) { + this.setItemsFromData( [] ); + } else { + v = String( v ); + if ( v.indexOf( '\x1f' ) !== 0 ) { + this.setItemsFromData( v.split( '|' ) ); + } else { + this.setItemsFromData( v.substr( 1 ).split( '\x1f' ) ); + } + } }, apiCheckValid: function () { var ok = this.getApiValue() !== undefined || suppressErrors; diff --git a/resources/src/mediawiki/api.js b/resources/src/mediawiki/api.js index a8ee4c7a82..b7579ff182 100644 --- a/resources/src/mediawiki/api.js +++ b/resources/src/mediawiki/api.js @@ -9,6 +9,9 @@ * `options` to mw.Api constructor. * @property {Object} defaultOptions.parameters Default query parameters for API requests. * @property {Object} defaultOptions.ajax Default options for jQuery#ajax. + * @property {boolean} defaultOptions.useUS Whether to use U+001F when joining multi-valued + * parameters (since 1.28). Default is true if ajax.url is not set, false otherwise for + * compatibility. * @private */ var defaultOptions = { @@ -95,6 +98,8 @@ options.ajax.url = String( options.ajax.url ); } + options = $.extend( { useUS: !options.ajax || !options.ajax.url }, options ); + options.parameters = $.extend( {}, defaultOptions.parameters, options.parameters ); options.ajax = $.extend( {}, defaultOptions.ajax, options.ajax ); @@ -147,14 +152,19 @@ * * @private * @param {Object} parameters (modified in-place) + * @param {boolean} useUS Whether to use U+001F when joining multi-valued parameters. */ - preprocessParameters: function ( parameters ) { + preprocessParameters: function ( parameters, useUS ) { var key; // Handle common MediaWiki API idioms for passing parameters for ( key in parameters ) { // Multiple values are pipe-separated if ( $.isArray( parameters[ key ] ) ) { - parameters[ key ] = parameters[ key ].join( '|' ); + if ( !useUS || parameters[ key ].join( '' ).indexOf( '|' ) === -1 ) { + parameters[ key ] = parameters[ key ].join( '|' ); + } else { + parameters[ key ] = '\x1f' + parameters[ key ].join( '\x1f' ); + } } // Boolean values are only false when not given at all if ( parameters[ key ] === false || parameters[ key ] === undefined ) { @@ -186,7 +196,7 @@ delete parameters.token; } - this.preprocessParameters( parameters ); + this.preprocessParameters( parameters, this.defaults.useUS ); // If multipart/form-data has been requested and emulation is possible, emulate it if ( diff --git a/resources/src/mediawiki/api/options.js b/resources/src/mediawiki/api/options.js index 0af2a754b7..069fbbfcda 100644 --- a/resources/src/mediawiki/api/options.js +++ b/resources/src/mediawiki/api/options.js @@ -41,9 +41,13 @@ value = options[ name ] === null ? null : String( options[ name ] ); // Can we bundle this option, or does it need a separate request? - bundleable = - ( value === null || value.indexOf( '|' ) === -1 ) && - ( name.indexOf( '|' ) === -1 && name.indexOf( '=' ) === -1 ); + if ( this.defaults.useUS ) { + bundleable = name.indexOf( '=' ) === -1; + } else { + bundleable = + ( value === null || value.indexOf( '|' ) === -1 ) && + ( name.indexOf( '|' ) === -1 && name.indexOf( '=' ) === -1 ); + } if ( bundleable ) { if ( value !== null ) { diff --git a/tests/phpunit/includes/api/ApiBaseTest.php b/tests/phpunit/includes/api/ApiBaseTest.php index 5d1ead0c2f..ec9d6990b9 100644 --- a/tests/phpunit/includes/api/ApiBaseTest.php +++ b/tests/phpunit/includes/api/ApiBaseTest.php @@ -43,4 +43,80 @@ class ApiBaseTest extends ApiTestCase { ); } + /** + * @dataProvider provideGetParameterFromSettings + * @param string|null $input + * @param array $paramSettings + * @param mixed $expected + * @param string[] $warnings + */ + public function testGetParameterFromSettings( $input, $paramSettings, $expected, $warnings ) { + $mock = new MockApi(); + $wrapper = TestingAccessWrapper::newFromObject( $mock ); + + $context = new DerivativeContext( $mock ); + $context->setRequest( new FauxRequest( $input !== null ? [ 'foo' => $input ] : [] ) ); + $wrapper->mMainModule = new ApiMain( $context ); + + if ( $expected instanceof UsageException ) { + try { + $wrapper->getParameterFromSettings( 'foo', $paramSettings, true ); + } catch ( UsageException $ex ) { + $this->assertEquals( $expected, $ex ); + } + } else { + $result = $wrapper->getParameterFromSettings( 'foo', $paramSettings, true ); + $this->assertSame( $expected, $result ); + $this->assertSame( $warnings, $mock->warnings ); + } + } + + public static function provideGetParameterFromSettings() { + $c0 = ''; + $enc = ''; + for ( $i = 0; $i < 32; $i++ ) { + $c0 .= chr( $i ); + $enc .= ( $i === 9 || $i === 10 || $i === 13 ) + ? chr( $i ) + : '�'; + } + + return [ + 'Basic param' => [ 'bar', null, 'bar', [] ], + 'String param' => [ 'bar', '', 'bar', [] ], + 'String param, defaulted' => [ null, '', '', [] ], + 'String param, empty' => [ '', 'default', '', [] ], + 'String param, required, empty' => [ + '', + [ ApiBase::PARAM_DFLT => 'default', ApiBase::PARAM_REQUIRED => true ], + new UsageException( 'The foo parameter must be set', 'nofoo' ), + [] + ], + 'Multi-valued parameter' => [ + 'a|b|c', + [ ApiBase::PARAM_ISMULTI => true ], + [ 'a', 'b', 'c' ], + [] + ], + 'Multi-valued parameter, alternative separator' => [ + "\x1fa|b\x1fc|d", + [ ApiBase::PARAM_ISMULTI => true ], + [ 'a|b', 'c|d' ], + [] + ], + 'Multi-valued parameter, other C0 controls' => [ + $c0, + [ ApiBase::PARAM_ISMULTI => true ], + [ $enc ], + [] + ], + 'Multi-valued parameter, other C0 controls (2)' => [ + "\x1f" . $c0, + [ ApiBase::PARAM_ISMULTI => true ], + [ substr( $enc, 0, -3 ), '' ], + [] + ], + ]; + } + } diff --git a/tests/phpunit/includes/api/MockApi.php b/tests/phpunit/includes/api/MockApi.php index 9a64d0864a..d7db538273 100644 --- a/tests/phpunit/includes/api/MockApi.php +++ b/tests/phpunit/includes/api/MockApi.php @@ -1,12 +1,18 @@ warnings[] = $warning; + } + public function getAllowedParams() { return [ 'filename' => null, diff --git a/tests/qunit/suites/resources/mediawiki.api/mediawiki.api.options.test.js b/tests/qunit/suites/resources/mediawiki.api/mediawiki.api.options.test.js index a48067125f..0797f32dfb 100644 --- a/tests/qunit/suites/resources/mediawiki.api/mediawiki.api.options.test.js +++ b/tests/qunit/suites/resources/mediawiki.api/mediawiki.api.options.test.js @@ -18,10 +18,10 @@ assert.deepEqual( stub.getCall( 0 ).args, [ { foo: 'bar' } ], '#saveOptions called correctly' ); } ); - QUnit.test( 'saveOptions', function ( assert ) { + QUnit.test( 'saveOptions without Unit Separator', function ( assert ) { QUnit.expect( 13 ); - var api = new mw.Api(); + var api = new mw.Api( { useUS: false } ); // We need to respond to the request for token first, otherwise the other requests won't be sent // until after the server.respond call, which confuses sinon terribly. This sucks a lot. @@ -75,4 +75,66 @@ } } ); } ); + + QUnit.test( 'saveOptions with Unit Separator', function ( assert ) { + QUnit.expect( 14 ); + + var api = new mw.Api( { useUS: true } ); + + // We need to respond to the request for token first, otherwise the other requests won't be sent + // until after the server.respond call, which confuses sinon terribly. This sucks a lot. + api.getToken( 'options' ); + this.server.respond( + /meta=tokens&type=csrf/, + [ 200, { 'Content-Type': 'application/json' }, + '{ "query": { "tokens": { "csrftoken": "+\\\\" } } }' ] + ); + + api.saveOptions( {} ).done( function () { + assert.ok( true, 'Request completed: empty case' ); + } ); + api.saveOptions( { foo: 'bar' } ).done( function () { + assert.ok( true, 'Request completed: simple' ); + } ); + api.saveOptions( { foo: 'bar', baz: 'quux' } ).done( function () { + assert.ok( true, 'Request completed: two options' ); + } ); + api.saveOptions( { foo: 'bar|quux', bar: 'a|b|c', baz: 'quux' } ).done( function () { + assert.ok( true, 'Request completed: bundleable with unit separator' ); + } ); + api.saveOptions( { foo: 'bar|quux', bar: 'a|b|c', 'baz=baz': 'quux' } ).done( function () { + assert.ok( true, 'Request completed: not bundleable with unit separator' ); + } ); + api.saveOptions( { foo: null } ).done( function () { + assert.ok( true, 'Request completed: reset an option' ); + } ); + api.saveOptions( { 'foo|bar=quux': null } ).done( function () { + assert.ok( true, 'Request completed: reset an option, not bundleable' ); + } ); + + // Requests are POST, match requestBody instead of url + this.server.respond( function ( request ) { + switch ( request.requestBody ) { + // simple + case 'action=options&format=json&formatversion=2&change=foo%3Dbar&token=%2B%5C': + // two options + case 'action=options&format=json&formatversion=2&change=foo%3Dbar%7Cbaz%3Dquux&token=%2B%5C': + // bundleable with unit separator + case 'action=options&format=json&formatversion=2&change=%1Ffoo%3Dbar%7Cquux%1Fbar%3Da%7Cb%7Cc%1Fbaz%3Dquux&token=%2B%5C': + // not bundleable with unit separator + case 'action=options&format=json&formatversion=2&optionname=baz%3Dbaz&optionvalue=quux&token=%2B%5C': + case 'action=options&format=json&formatversion=2&change=%1Ffoo%3Dbar%7Cquux%1Fbar%3Da%7Cb%7Cc&token=%2B%5C': + // reset an option + case 'action=options&format=json&formatversion=2&change=foo&token=%2B%5C': + // reset an option, not bundleable + case 'action=options&format=json&formatversion=2&optionname=foo%7Cbar%3Dquux&token=%2B%5C': + assert.ok( true, 'Repond to ' + request.requestBody ); + request.respond( 200, { 'Content-Type': 'application/json' }, + '{ "options": "success" }' ); + break; + default: + assert.ok( false, 'Unexpected request: ' + request.requestBody ); + } + } ); + } ); }( mediaWiki ) ); -- 2.20.1