Skip to content

Commit 6af6771

Browse files
kenjiskenjis
authored
Merge pull request from GHSA-m6m8-6gq8-c9fj
fix: validation placeholder
2 parents dee68cc + 2e7eb50 commit 6af6771

File tree

8 files changed

+117
-37
lines changed

8 files changed

+117
-37
lines changed

Diff for: ‎system/Validation/Validation.php

+66-26
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,10 @@
1616
use CodeIgniter\HTTP\RequestInterface;
1717
use CodeIgniter\Validation\Exceptions\ValidationException;
1818
use CodeIgniter\View\RendererInterface;
19+
use Config\Services;
1920
use Config\Validation as ValidationConfig;
2021
use InvalidArgumentException;
22+
use LogicException;
2123
use TypeError;
2224

2325
/**
@@ -40,10 +42,18 @@ class Validation implements ValidationInterface
4042
protected $ruleSetInstances = [];
4143

4244
/**
43-
* Stores the actual rules that should
44-
* be ran against $data.
45+
* Stores the actual rules that should be run against $data.
4546
*
4647
* @var array
48+
*
49+
* [
50+
* field1 => [
51+
* 'label' => label,
52+
* 'rules' => [
53+
* rule1, rule2, ...
54+
* ],
55+
* ],
56+
* ]
4757
*/
4858
protected $rules = [];
4959

@@ -133,8 +143,7 @@ public function run(?array $data = null, ?string $group = null, ?string $dbGroup
133143
// Run through each rule. If we have any field set for
134144
// this rule, then we need to run them through!
135145
foreach ($this->rules as $field => $setup) {
136-
// Blast $rSetup apart, unless it's already an array.
137-
$rules = $setup['rules'] ?? $setup;
146+
$rules = $setup['rules'];
138147

139148
if (is_string($rules)) {
140149
$rules = $this->splitRules($rules);
@@ -483,6 +492,14 @@ public function setRules(array $rules, array $errors = []): ValidationInterface
483492
$rule = ['rules' => $rule];
484493
}
485494
}
495+
496+
if (isset($rule['rules']) && is_string($rule['rules'])) {
497+
$rule['rules'] = $this->splitRules($rule['rules']);
498+
}
499+
500+
if (is_string($rule)) {
501+
$rule = ['rules' => $this->splitRules($rule)];
502+
}
486503
}
487504

488505
$this->rules = $rules;
@@ -645,47 +662,70 @@ public function loadRuleGroup(?string $group = null)
645662
*
646663
* and the following rule:
647664
*
648-
* 'required|is_unique[users,email,id,{id}]'
665+
* 'is_unique[users,email,id,{id}]'
649666
*
650667
* The value of {id} would be replaced with the actual id in the form data:
651668
*
652-
* 'required|is_unique[users,email,id,13]'
669+
* 'is_unique[users,email,id,13]'
653670
*/
654671
protected function fillPlaceholders(array $rules, array $data): array
655672
{
656-
$replacements = [];
673+
foreach ($rules as &$rule) {
674+
$ruleSet = $rule['rules'];
657675

658-
foreach ($data as $key => $value) {
659-
$replacements["{{$key}}"] = $value;
660-
}
676+
foreach ($ruleSet as &$row) {
677+
if (is_string($row)) {
678+
$placeholderFields = $this->retrievePlaceholders($row, $data);
661679

662-
if ($replacements !== []) {
663-
foreach ($rules as &$rule) {
664-
$ruleSet = $rule['rules'] ?? $rule;
680+
foreach ($placeholderFields as $field) {
681+
$validator ??= Services::validation(null, false);
665682

666-
if (is_array($ruleSet)) {
667-
foreach ($ruleSet as &$row) {
668-
if (is_string($row)) {
669-
$row = strtr($row, $replacements);
683+
$placeholderRules = $rules[$field]['rules'] ?? null;
684+
685+
// Check if the validation rule for the placeholder exists
686+
if ($placeholderRules === null) {
687+
throw new LogicException(
688+
'No validation rules for the placeholder: ' . $field
689+
);
670690
}
671-
}
672-
}
673691

674-
if (is_string($ruleSet)) {
675-
$ruleSet = strtr($ruleSet, $replacements);
676-
}
692+
// Check if the rule does not have placeholders
693+
foreach ($placeholderRules as $placeholderRule) {
694+
if ($this->retrievePlaceholders($placeholderRule, $data)) {
695+
throw new LogicException(
696+
'The placeholder field cannot use placeholder: ' . $field
697+
);
698+
}
699+
}
677700

678-
if (isset($rule['rules'])) {
679-
$rule['rules'] = $ruleSet;
680-
} else {
681-
$rule = $ruleSet;
701+
// Validate the placeholder field
702+
if (! $validator->check($data[$field], implode('|', $placeholderRules))) {
703+
// if fails, do nothing
704+
continue;
705+
}
706+
707+
// Replace the placeholder in the rule
708+
$ruleSet = str_replace('{' . $field . '}', $data[$field], $ruleSet);
709+
}
682710
}
683711
}
712+
713+
$rule['rules'] = $ruleSet;
684714
}
685715

686716
return $rules;
687717
}
688718

719+
/**
720+
* Retrieves valid placeholder fields.
721+
*/
722+
private function retrievePlaceholders(string $rule, array $data): array
723+
{
724+
preg_match_all('/{(.+?)}/', $rule, $matches);
725+
726+
return array_intersect($matches[1], array_keys($data));
727+
}
728+
689729
/**
690730
* Checks to see if an error exists for the given field.
691731
*/

Diff for: ‎tests/system/Validation/StrictRules/DatabaseRelatedRulesTest.php

+8-2
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,10 @@ public function testIsUniqueIgnoresParamsPlaceholders(): void
121121
'email' => 'derek@world.co.uk',
122122
];
123123

124-
$this->validation->setRules(['email' => 'is_unique[user.email,id,{id}]']);
124+
$this->validation->setRules([
125+
'id' => 'is_natural_no_zero',
126+
'email' => 'is_unique[user.email,id,{id}]',
127+
]);
125128
$this->assertTrue($this->validation->run($data));
126129
}
127130

@@ -221,7 +224,10 @@ public function testIsNotUniqueIgnoresParamsPlaceholders(): void
221224
'id' => $row->id,
222225
'email' => 'derek@world.co.uk',
223226
];
224-
$this->validation->setRules(['email' => 'is_not_unique[user.email,id,{id}]']);
227+
$this->validation->setRules([
228+
'id' => 'is_natural_no_zero',
229+
'email' => 'is_not_unique[user.email,id,{id}]',
230+
]);
225231
$this->assertTrue($this->validation->run($data));
226232
}
227233

Diff for: ‎tests/system/Validation/ValidationTest.php

+24-9
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,12 @@ public function testSetRulesStoresRules(): void
8686
'bar' => 'baz|belch',
8787
];
8888
$this->validation->setRules($rules);
89-
$this->assertSame($rules, $this->validation->getRules());
89+
90+
$expected = [
91+
'foo' => ['rules' => ['bar', 'baz']],
92+
'bar' => ['rules' => ['baz', 'belch']],
93+
];
94+
$this->assertSame($expected, $this->validation->getRules());
9095
}
9196

9297
public function testSetRuleStoresRule()
@@ -97,7 +102,7 @@ public function testSetRuleStoresRule()
97102
$this->assertSame([
98103
'foo' => [
99104
'label' => null,
100-
'rules' => 'bar|baz',
105+
'rules' => ['bar', 'baz'],
101106
],
102107
], $this->validation->getRules());
103108
}
@@ -110,7 +115,7 @@ public function testSetRuleMultipleWithIndividual()
110115
$this->assertSame([
111116
'username' => [
112117
'label' => 'Username',
113-
'rules' => 'required|min_length[3]',
118+
'rules' => ['required', 'min_length[3]'],
114119
],
115120
'password' => [
116121
'label' => 'Password',
@@ -136,11 +141,11 @@ public function testSetRuleAddsRule()
136141
$this->assertSame([
137142
'bar' => [
138143
'label' => null,
139-
'rules' => 'bar|baz',
144+
'rules' => ['bar', 'baz'],
140145
],
141146
'foo' => [
142147
'label' => null,
143-
'rules' => 'foo|foz',
148+
'rules' => ['foo', 'foz'],
144149
],
145150
], $this->validation->getRules());
146151
}
@@ -158,7 +163,7 @@ public function testSetRuleOverwritesRule()
158163
$this->assertSame([
159164
'foo' => [
160165
'label' => null,
161-
'rules' => 'foo|foz',
166+
'rules' => ['foo', 'foz'],
162167
],
163168
], $this->validation->getRules());
164169
}
@@ -176,7 +181,7 @@ public function testSetRuleOverwritesRuleReverse()
176181
$this->assertSame([
177182
'foo' => [
178183
'label' => null,
179-
'rules' => 'bar|baz',
184+
'rules' => ['bar', 'baz'],
180185
],
181186
], $this->validation->getRules());
182187
}
@@ -549,7 +554,7 @@ public function testSetRuleGroup(): void
549554
{
550555
$this->validation->setRuleGroup('groupA');
551556
$this->assertSame([
552-
'foo' => 'required|min_length[5]',
557+
'foo' => ['rules' => ['required', 'min_length[5]']],
553558
], $this->validation->getRules());
554559
}
555560

@@ -1386,7 +1391,7 @@ public function provideStringRulesCases(): iterable
13861391
protected function placeholderReplacementResultDetermination(string $placeholder = 'id', ?array $data = null)
13871392
{
13881393
if ($data === null) {
1389-
$data = [$placeholder => 'placeholder-value'];
1394+
$data = [$placeholder => '12'];
13901395
}
13911396

13921397
$validationRules = $this->getPrivateMethodInvoker($this->validation, 'fillPlaceholders')($this->validation->getRules(), $data);
@@ -1421,13 +1426,15 @@ public function testPlaceholderReplacementTestFails()
14211426

14221427
public function testPlaceholderReplacementSetSingleRuleString()
14231428
{
1429+
$this->validation->setRule('id', null, 'required|is_natural_no_zero');
14241430
$this->validation->setRule('foo', null, 'required|filter[{id}]');
14251431

14261432
$this->placeholderReplacementResultDetermination();
14271433
}
14281434

14291435
public function testPlaceholderReplacementSetSingleRuleArray()
14301436
{
1437+
$this->validation->setRule('id', null, ['required', 'is_natural_no_zero']);
14311438
$this->validation->setRule('foo', null, ['required', 'filter[{id}]']);
14321439

14331440
$this->placeholderReplacementResultDetermination();
@@ -1436,6 +1443,7 @@ public function testPlaceholderReplacementSetSingleRuleArray()
14361443
public function testPlaceholderReplacementSetMultipleRulesSimpleString()
14371444
{
14381445
$this->validation->setRules([
1446+
'id' => 'required|is_natural_no_zero',
14391447
'foo' => 'required|filter[{id}]',
14401448
]);
14411449

@@ -1445,6 +1453,7 @@ public function testPlaceholderReplacementSetMultipleRulesSimpleString()
14451453
public function testPlaceholderReplacementSetMultipleRulesSimpleArray()
14461454
{
14471455
$this->validation->setRules([
1456+
'id' => ['required', 'is_natural_no_zero'],
14481457
'foo' => ['required', 'filter[{id}]'],
14491458
]);
14501459

@@ -1454,6 +1463,9 @@ public function testPlaceholderReplacementSetMultipleRulesSimpleArray()
14541463
public function testPlaceholderReplacementSetMultipleRulesComplexString()
14551464
{
14561465
$this->validation->setRules([
1466+
'id' => [
1467+
'rules' => 'required|is_natural_no_zero',
1468+
],
14571469
'foo' => [
14581470
'rules' => 'required|filter[{id}]',
14591471
],
@@ -1465,6 +1477,9 @@ public function testPlaceholderReplacementSetMultipleRulesComplexString()
14651477
public function testPlaceholderReplacementSetMultipleRulesComplexArray()
14661478
{
14671479
$this->validation->setRules([
1480+
'id' => [
1481+
'rules' => ['required', 'is_natural_no_zero'],
1482+
],
14681483
'foo' => [
14691484
'rules' => ['required', 'filter[{id}]'],
14701485
],

Diff for: ‎user_guide_src/source/changelogs/v4.3.5.rst

+3
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,9 @@ Release Date: Unreleased
1212
SECURITY
1313
********
1414

15+
- *Remote Code Execution Vulnerability in Validation Placeholders* was fixed.
16+
See the `Security advisory GHSA-m6m8-6gq8-c9fj <https://github.com/codeigniter4/CodeIgniter4/security/advisories/GHSA-m6m8-6gq8-c9fj>`_
17+
for more information.
1518
- Fixed that ``Session::stop()`` did not destroy the session.
1619
See :ref:`Session Library <session-stop>` for details.
1720

Diff for: ‎user_guide_src/source/installation/upgrade_435.rst

+6
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,12 @@ Mandatory File Changes
1818
Breaking Changes
1919
****************
2020

21+
Validation Placeholders
22+
=======================
23+
24+
- To use :ref:`validation-placeholders` securely, please remember to create a validation rule for the field you will use as a placeholder.
25+
26+
2127
Session::stop()
2228
===============
2329

Diff for: ‎user_guide_src/source/libraries/validation.rst

+8
Original file line numberDiff line numberDiff line change
@@ -436,6 +436,8 @@ This method sets a rule group from the validation configuration to the validatio
436436

437437
.. literalinclude:: validation/018.php
438438

439+
.. _validation-placeholders:
440+
439441
Validation Placeholders
440442
=======================
441443

@@ -446,6 +448,9 @@ replaced by the **value** of the matched incoming field. An example should clari
446448

447449
.. literalinclude:: validation/020.php
448450

451+
.. note:: Since v4.3.5, you must set the validation rules for the placeholder
452+
field (``id``).
453+
449454
In this set of rules, it states that the email address should be unique in the database, except for the row
450455
that has an id matching the placeholder's value. Assuming that the form POST data had the following:
451456

@@ -457,6 +462,9 @@ then the ``{id}`` placeholder would be replaced with the number **4**, giving th
457462

458463
So it will ignore the row in the database that has ``id=4`` when it verifies the email is unique.
459464

465+
.. note:: Since v4.3.5, if the placeholder (``id``) value does not pass the
466+
validation, the placeholder would not be replaced.
467+
460468
This can also be used to create more dynamic rules at runtime, as long as you take care that any dynamic
461469
keys passed in don't conflict with your form data.
462470

Diff for: ‎user_guide_src/source/libraries/validation/020.php

+1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
<?php
22

33
$validation->setRules([
4+
'id' => 'is_natural_no_zero',
45
'email' => 'required|valid_email|is_unique[users.email,id,{id}]',
56
]);

Diff for: ‎user_guide_src/source/libraries/validation/022.php

+1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
<?php
22

33
$validation->setRules([
4+
'id' => 'is_natural_no_zero',
45
'email' => 'required|valid_email|is_unique[users.email,id,4]',
56
]);

0 commit comments

Comments
 (0)