diff options
| -rw-r--r-- | autoload.php | 1 | ||||
| -rw-r--r-- | includes/Rest/Handler.php | 7 | ||||
| -rw-r--r-- | includes/Rest/PathTemplateMatcher/PathMatcher.php | 25 | ||||
| -rw-r--r-- | includes/Rest/PathTemplateMatcher/PathSegmentException.php | 26 | ||||
| -rw-r--r-- | tests/phpunit/unit/includes/Rest/PathTemplateMatcher/PathMatcherTest.php | 32 |
5 files changed, 79 insertions, 12 deletions
diff --git a/autoload.php b/autoload.php index 1856657512a8..6c679ae070c1 100644 --- a/autoload.php +++ b/autoload.php @@ -1981,6 +1981,7 @@ $wgAutoloadLocalClasses = [ 'MediaWiki\\Rest\\LocalizedHttpException' => __DIR__ . '/includes/Rest/LocalizedHttpException.php', 'MediaWiki\\Rest\\PathTemplateMatcher\\PathConflict' => __DIR__ . '/includes/Rest/PathTemplateMatcher/PathConflict.php', 'MediaWiki\\Rest\\PathTemplateMatcher\\PathMatcher' => __DIR__ . '/includes/Rest/PathTemplateMatcher/PathMatcher.php', + 'MediaWiki\\Rest\\PathTemplateMatcher\\PathSegmentException' => __DIR__ . '/includes/Rest/PathTemplateMatcher/PathSegmentException.php', 'MediaWiki\\Rest\\RedirectException' => __DIR__ . '/includes/Rest/RedirectException.php', 'MediaWiki\\Rest\\Reporter\\ErrorReporter' => __DIR__ . '/includes/Rest/Reporter/ErrorReporter.php', 'MediaWiki\\Rest\\Reporter\\MWErrorReporter' => __DIR__ . '/includes/Rest/Reporter/MWErrorReporter.php', diff --git a/includes/Rest/Handler.php b/includes/Rest/Handler.php index 4e64b1c01027..1fc00d74f456 100644 --- a/includes/Rest/Handler.php +++ b/includes/Rest/Handler.php @@ -384,6 +384,13 @@ abstract class Handler { * multipart/form-data POST body (i.e. parameters which would be present in PHP's $_POST * array). For validating other kinds of request bodies, override getBodyValidator(). * + * For "query" and "body" parameters, a PARAM_REQUIRED setting of "false" means the caller + * does not have to supply the parameter. For "path" parameters, the path matcher will always + * require the caller to supply all path parameters for a route, regardless of the + * PARAM_REQUIRED setting. However, "path" parameters may be specified in getParamSettings() + * as non-required to indicate that the handler services multiple routes, some of which may + * not supply the parameter. + * * @stable to override * * @return array[] Associative array mapping parameter names to diff --git a/includes/Rest/PathTemplateMatcher/PathMatcher.php b/includes/Rest/PathTemplateMatcher/PathMatcher.php index bbd750895347..e0844f8ac490 100644 --- a/includes/Rest/PathTemplateMatcher/PathMatcher.php +++ b/includes/Rest/PathTemplateMatcher/PathMatcher.php @@ -110,18 +110,24 @@ class PathMatcher { } $part = $parts[$index]; $result = false; + if ( $this->isParam( $part ) ) { foreach ( $node as $childNode ) { - $result = $this->findConflict( $childNode, $parts, $index + 1 ); - if ( $result !== false ) { - break; + // Params and tree entries for trailing empty slashes ('=') do not conflict + if ( !isset( $node['='] ) ) { + $result = $this->findConflict( $childNode, $parts, $index + 1 ); + if ( $result !== false ) { + break; + } } } } else { if ( isset( $node["=$part"] ) ) { $result = $this->findConflict( $node["=$part"], $parts, $index + 1 ); } - if ( $result === false && isset( $node['*'] ) ) { + + // Trailing empty slashes (aka an empty $part) do not conflict with params + if ( $result === false && isset( $node['*'] ) && $part !== '' ) { $result = $this->findConflict( $node['*'], $parts, $index + 1 ); } } @@ -144,15 +150,24 @@ class PathMatcher { * @param string $template The path template * @param mixed $userData User data used to identify the matched route to * the caller of match() - * @throws PathConflict + * @throws PathConflict|PathSegmentException */ public function add( $template, $userData ) { + // This will produce an empty part before any leading slash and after any trailing slash $parts = explode( '/', $template ); $length = count( $parts ); if ( !isset( $this->treesByLength[$length] ) ) { $this->treesByLength[$length] = []; } $tree =& $this->treesByLength[$length]; + + // Disallow empty path parts within the path (but not leading or trailing). + for ( $i = 1; $i < $length - 1; $i++ ) { + if ( $parts[$i] == '' ) { + throw new PathSegmentException( $template, $userData ); + } + } + $conflict = $this->findConflict( $tree, $parts ); if ( $conflict !== false ) { throw new PathConflict( $template, $userData, $conflict ); diff --git a/includes/Rest/PathTemplateMatcher/PathSegmentException.php b/includes/Rest/PathTemplateMatcher/PathSegmentException.php new file mode 100644 index 000000000000..7c4533a8c60f --- /dev/null +++ b/includes/Rest/PathTemplateMatcher/PathSegmentException.php @@ -0,0 +1,26 @@ +<?php + +namespace MediaWiki\Rest\PathTemplateMatcher; + +use Exception; + +/** + * @newable + */ +class PathSegmentException extends Exception { + public $template; + public $userData; + + /** + * @stable to call + * + * @param string $template + * @param mixed $userData + */ + public function __construct( $template, $userData ) { + $this->template = $template; + $this->userData = $userData; + parent::__construct( "Unable to add path template \"$template\" since it contains " . + "an empty path segment." ); + } +} diff --git a/tests/phpunit/unit/includes/Rest/PathTemplateMatcher/PathMatcherTest.php b/tests/phpunit/unit/includes/Rest/PathTemplateMatcher/PathMatcherTest.php index 8df7eac6fac6..c6a507ebd881 100644 --- a/tests/phpunit/unit/includes/Rest/PathTemplateMatcher/PathMatcherTest.php +++ b/tests/phpunit/unit/includes/Rest/PathTemplateMatcher/PathMatcherTest.php @@ -4,11 +4,13 @@ namespace MediaWiki\Tests\Rest\PathTemplateMatcher; use MediaWiki\Rest\PathTemplateMatcher\PathConflict; use MediaWiki\Rest\PathTemplateMatcher\PathMatcher; +use MediaWiki\Rest\PathTemplateMatcher\PathSegmentException; use MediaWikiUnitTestCase; /** * @covers \MediaWiki\Rest\PathTemplateMatcher\PathMatcher * @covers \MediaWiki\Rest\PathTemplateMatcher\PathConflict + * @covers \MediaWiki\Rest\PathTemplateMatcher\PathSegmentException */ class PathMatcherTest extends MediaWikiUnitTestCase { private static $normalRoutes = [ @@ -18,10 +20,17 @@ class PathMatcherTest extends MediaWikiUnitTestCase { '/c/{x}/e', '/c/{x}/{y}/d', '/d/', + '/d/{x}', '/', - '/d//e' + '/{x}' ]; + public static function provideErrorRoutes() { + return [ + [ '/d//e' ] + ]; + } + public static function provideConflictingRoutes() { return [ [ '/a/b', 0, '/a/b' ], @@ -29,18 +38,17 @@ class PathMatcherTest extends MediaWikiUnitTestCase { [ '/{x}/c', 1, '/b/{x}' ], [ '/b/a', 1, '/b/{x}' ], [ '/b/{x}', 1, '/b/{x}' ], - [ '/{x}/{y}/d', 2, '/c/{x}/d' ], - [ '/d/{x}', 5, '/d/' ], - [ '/{x}', 6, '/' ] + [ '/{x}/{y}/d', 2, '/c/{x}/d' ] ]; } public static function provideMatch() { return [ [ '', false ], + [ '/a/', false ], [ '/a/b', [ 'params' => [], 'userData' => 0 ] ], - [ '/b', false ], [ '/b/1', [ 'params' => [ 'x' => '1' ], 'userData' => 1 ] ], + [ '/c/1', false ], [ '/c/1/d', [ 'params' => [ 'x' => '1' ], 'userData' => 2 ] ], [ '/c/1/e', [ 'params' => [ 'x' => '1' ], 'userData' => 3 ] ], [ '/c/000/e', [ 'params' => [ 'x' => '000' ], 'userData' => 3 ] ], @@ -48,8 +56,10 @@ class PathMatcherTest extends MediaWikiUnitTestCase { [ '/c//e', [ 'params' => [ 'x' => '' ], 'userData' => 3 ] ], [ '/c///e', false ], [ '/d/', [ 'params' => [], 'userData' => 5 ] ], - [ '/', [ 'params' => [], 'userData' => 6 ] ], - [ '/d//e', [ 'params' => [], 'userData' => 7 ] ] + [ '/d/1', [ 'params' => [ 'x' => '1' ], 'userData' => 6 ] ], + [ '/', [ 'params' => [], 'userData' => 7 ] ], + [ '/1', [ 'params' => [ 'x' => '1' ], 'userData' => 8 ] ], + [ '/1/', false ] ]; } @@ -61,6 +71,14 @@ class PathMatcherTest extends MediaWikiUnitTestCase { return $pm; } + /** @dataProvider provideErrorRoutes */ + public function testAddError( $attempt ) { + $pm = $this->createNormalRouter(); + + $this->expectException( PathSegmentException::class ); + $pm->add( $attempt, 'error' ); + } + /** @dataProvider provideConflictingRoutes */ public function testAddConflict( $attempt, $expectedUserData, $expectedTemplate ) { $pm = $this->createNormalRouter(); |