Resolve #3143617 "Protect pager against"
Closes #3143617
Merge request reports
Activity
added 2 commits
4 5 use Drupal\Core\Pager\PagerParameters; 6 use Drupal\Tests\UnitTestCase; 7 use Symfony\Component\HttpFoundation\Request; 8 use Symfony\Component\HttpFoundation\RequestStack; 9 10 /** 11 * @coversDefaultClass \Drupal\Core\Pager\PagerParameters 12 * @group Pager 13 */ 14 class PagerParametersTest extends UnitTestCase { 15 16 /** 17 * @covers ::getQueryParameters 18 */ 19 public function testGetQueryParameters() { changed this line in version 3 of the diff
22 $parameters = new PagerParameters($request_stack); 23 $query = $request_stack->getCurrentRequest()->query; 24 $this->assertEquals([], $parameters->getQueryParameters()); 25 $query->set('page', 1); 26 $this->assertEquals(['page' => 1], $query->all(), 'page query set correctly'); 27 $this->assertEquals([], $parameters->getQueryParameters(), 'Pager filtered from empty parameters'); 28 $query->set('test', 1); 29 $this->assertEquals(['page' => 1, 'test' => 1], $query->all(), 'test value set correctly'); 30 $this->assertEquals(['test' => 1], $parameters->getQueryParameters(), 'Pager filtered with another parameter'); 31 } 32 33 /** 34 * @covers ::findPage 35 * @dataProvider providePagerQueries 36 */ 37 public function testFindPage($raw_query, $parameter, $expected_query) { changed this line in version 3 of the diff
38 $request_stack = new RequestStack(); 39 $request_stack->push(new Request()); 40 $parameters = new PagerParameters($request_stack); 41 $request_stack->getCurrentRequest()->query->set('page', $raw_query); 42 // Ensure findPage finds 0 when the query is actually empty or invalid. 43 $expected_query = $expected_query ?: [0]; 44 foreach ($expected_query as $key => $value) { 45 $this->assertSame($value, $parameters->findPage($key)); 46 } 47 } 48 49 /** 50 * @covers ::getPagerQuery 51 * @dataProvider providePagerQueries 52 */ 53 public function testGetPagerQuery($raw_query, $parameter, $expected_query) { changed this line in version 3 of the diff
51 * @dataProvider providePagerQueries 52 */ 53 public function testGetPagerQuery($raw_query, $parameter, $expected_query) { 54 $request_stack = new RequestStack(); 55 $request_stack->push(new Request()); 56 $parameters = new PagerParameters($request_stack); 57 $request_stack->getCurrentRequest()->query->set('page', $raw_query); 58 $this->assertEquals($expected_query, $parameters->getPagerQuery()); 59 } 60 61 /** 62 * Ensure missing request is handled cleanly. 63 * 64 * @covers ::getPagerParameter 65 */ 66 public function testGetPagerParameterNoRequest() { changed this line in version 3 of the diff
69 $this->assertSame('', $parameters->getPagerParameter()); 70 } 71 72 /** 73 * @covers ::getPagerParameter 74 * @dataProvider providePagerQueries 75 */ 76 public function testGetPagerParameter($raw_query, $parameter) { 77 $request_stack = new RequestStack(); 78 $request_stack->push(new Request()); 79 $parameters = new PagerParameters($request_stack); 80 $request_stack->getCurrentRequest()->query->set('page', $raw_query); 81 $this->assertSame($parameter, $parameters->getPagerParameter()); 82 } 83 84 public static function providePagerQueries() { changed this line in version 3 of the diff
80 $request_stack->getCurrentRequest()->query->set('page', $raw_query); 81 $this->assertSame($parameter, $parameters->getPagerParameter()); 82 } 83 84 public static function providePagerQueries() { 85 return [ 86 'defensive null page value' => [NULL, '', []], 87 // Array values aren't supported, so they default to empty. 88 'invalid empty page array' => [[], '', []], 89 'invalid populated array' => [[1, 2, 3], '', []], 90 // Nothing to 91 'empty string' => ['', '', []], 92 // Conventional but "zero" page values. 93 'page 0 as a string' => ['0', '0', [0]], 94 'page 0 as a integer' => [0, '0', [0]], 95 changed this line in version 4 of the diff
69 $this->assertSame('', $parameters->getPagerParameter()); 70 } 71 72 /** 73 * @covers ::getPagerParameter 74 * @dataProvider providePagerQueries 75 */ 76 public function testGetPagerParameter($raw_query, $parameter) { 77 $request_stack = new RequestStack(); 78 $request_stack->push(new Request()); 79 $parameters = new PagerParameters($request_stack); 80 $request_stack->getCurrentRequest()->query->set('page', $raw_query); 81 $this->assertSame($parameter, $parameters->getPagerParameter()); 82 } 83 84 public static function providePagerQueries() { -
This should have a doc block. And since this is used multiple time it would help to explain the return values here. For whatever reason, I started reading this from the provider and kept wondering why there were three return values when testGetPagerParameter() is only using one. We can help the next person working on this by adding a little explanation.
changed this line in version 3 of the diff
added 1647 commits
-
314624fd...e5653c11 - 1644 commits from branch
project:11.x - ffe9f7a1 - Merge branch '11.x' of github.com:drupal/drupal into 3143617-protect-pager-against
- 8a3b6dbf - Merge branch '3143617-protect-pager-against' of...
- bcaf7c1d - Address feedback
Toggle commit list-
314624fd...e5653c11 - 1644 commits from branch
73 74 /** 75 * @covers ::getPagerParameter 76 * @dataProvider providePagerQueries 77 */ 78 public function testGetPagerParameter($raw_query, $parameter) { 79 $request_stack = new RequestStack(); 80 $request_stack->push(new Request()); 81 $parameters = new PagerParameters($request_stack); 82 $request_stack->getCurrentRequest()->query->set('page', $raw_query); 83 $this->assertSame($parameter, $parameters->getPagerParameter()); 84 } 85 86 /** 87 * Data provider for testGetPagerParameter(), testGetPagerQuery(), and testFindPage(). 88 */ -
The change here is definitely an improvement over not having a doc block at all, but I wonder if this really addresses @quietone 's concerns:
And since this is used multiple time it would help to explain the return values here.
I don't see any explanation of the return values in relation to the parameters of the test methods. The test sets are named and have additional explanatory comments, which is good, I like that, but those were there before the latest commit to address the review. And I see no new explanations of the return values, so I think this is still open.
It is a shame that we aren't using PHPUnit 11 yet. If we did, we could use named arguments and could then update the signature of the various test methods to only define those parameters they actually use. To fully address the above review comment right now, I think we could:
- Add a
@returnstatement to the doc block, which explains the values of the different test sets. - Already use named arguments, although they are not yet supported. In PHPUnit 10 and below, any keys in the test sets are just discarded by PHPUnit, so we should be able to already name the different arguments, but we can't remove the unused parameters from the test methods yet. So something like:
'defensive null page value' => [ 'raw_query' => NULL, 'parameter' => '', 'expected_query' => [], ],I think doing either of those two options should be enough to fully address the earlier review as it should then be pretty clear what is in the test set combined with the new summary line.
- Add a
63 /** 64 * Ensure missing request is handled cleanly. 65 * 66 * @covers ::getPagerParameter 67 */ 68 public function testGetPagerParameterNoRequest(): void { 69 $request_stack = new RequestStack(); 70 $parameters = new PagerParameters($request_stack); 71 $this->assertSame('', $parameters->getPagerParameter()); 72 } 73 74 /** 75 * @covers ::getPagerParameter 76 * @dataProvider providePagerQueries 77 */ 78 public function testGetPagerParameter($raw_query, $parameter) { changed this line in version 5 of the diff
80 $request_stack->push(new Request()); 81 $parameters = new PagerParameters($request_stack); 82 $request_stack->getCurrentRequest()->query->set('page', $raw_query); 83 $this->assertSame($parameter, $parameters->getPagerParameter()); 84 } 85 86 /** 87 * Data provider for testGetPagerParameter(), testGetPagerQuery(), and testFindPage(). 88 */ 89 public static function providePagerQueries(): array { 90 return [ 91 'defensive null page value' => [NULL, '', []], 92 // Array values aren't supported, so they default to empty. 93 'invalid empty page array' => [[], '', []], 94 'invalid populated array' => [[1, 2, 3], '', []], 95 // Nothing to changed this line in version 5 of the diff
added 109 commits
-
bcaf7c1d...694cb802 - 107 commits from branch
project:11.x - b9f852d0 - Merge branch '11.x' of github.com:drupal/drupal into 3143617-protect-pager-against
- a05a01fe - Address feedback
-
bcaf7c1d...694cb802 - 107 commits from branch
64 64 public function getPagerParameter() { 65 65 $request = $this->requestStack->getCurrentRequest(); 66 66 if ($request) { 67 return $request->query->get('page', ''); 67 $params = $request->query->all(); 68 $page = $params['page'] ?? ''; 69 // The "page" can be an array so validate the type before casting, 70 // i.e., "?page[offset]=12" 71 return is_scalar($page) ? (string) $page : ''; - Comment on lines -67 to +71
I'm pretty certain the intent of the change in Symfony was to safeguard against behaviour changes, e.g. if you blindly use ->get('some_param') and expect a scalar but someone has diddled the URL and its an array unexpected things might happen, hence the developer has to make a conscious choice between using
->get('page')and->all('page'). The exception is there to protect us against unexpected params.So let's use the new API and simplify the amount of changes dramatically.
67 $params = $request->query->all(); 68 $page = $params['page'] ?? ''; 69 // The "page" can be an array so validate the type before casting, 70 // i.e., "?page[offset]=12" 71 return is_scalar($page) ? (string) $page : ''; 67 try { 68 return $request->query->get('page', ''); 69 } 70 catch (\Symfony\Component\HttpFoundation\Exception\BadRequestException) { 71 // Someone has tampered with the 'page' parameter. 72 return ''; 73 } changed this line in version 7 of the diff
24 $parameters = new PagerParameters($request_stack); 25 $query = $request_stack->getCurrentRequest()->query; 26 $this->assertEquals([], $parameters->getQueryParameters()); 27 $query->set('page', 1); 28 $this->assertEquals(['page' => 1], $query->all(), 'page query set correctly'); 29 $this->assertEquals([], $parameters->getQueryParameters(), 'Pager filtered from empty parameters'); 30 $query->set('test', 1); 31 $this->assertEquals(['page' => 1, 'test' => 1], $query->all(), 'test value set correctly'); 32 $this->assertEquals(['test' => 1], $parameters->getQueryParameters(), 'Pager filtered with another parameter'); 33 } 34 35 /** 36 * @covers ::findPage 37 * @dataProvider providePagerQueries 38 */ 39 public function testFindPage($raw_query, $parameter, $expected_query): void { changed this line in version 8 of the diff
40 $request_stack = new RequestStack(); 41 $request_stack->push(new Request()); 42 $parameters = new PagerParameters($request_stack); 43 $request_stack->getCurrentRequest()->query->set('page', $raw_query); 44 // Ensure findPage finds 0 when the query is actually empty or invalid. 45 $expected_query = $expected_query ?: [0]; 46 foreach ($expected_query as $key => $value) { 47 $this->assertSame($value, $parameters->findPage($key)); 48 } 49 } 50 51 /** 52 * @covers ::getPagerQuery 53 * @dataProvider providePagerQueries 54 */ 55 public function testGetPagerQuery($raw_query, $parameter, $expected_query): void { changed this line in version 8 of the diff
63 /** 64 * Ensure missing request is handled cleanly. 65 * 66 * @covers ::getPagerParameter 67 */ 68 public function testGetPagerParameterNoRequest(): void { 69 $request_stack = new RequestStack(); 70 $parameters = new PagerParameters($request_stack); 71 $this->assertSame('', $parameters->getPagerParameter()); 72 } 73 74 /** 75 * @covers ::getPagerParameter 76 * @dataProvider providePagerQueries 77 */ 78 public function testGetPagerParameter($raw_query, $parameter): void { changed this line in version 8 of the diff
