From 70f00b9bacb664e4153a0148aa86315094c543e1 Mon Sep 17 00:00:00 2001 From: techmahedy Date: Fri, 30 Oct 2026 22:17:45 +0600 Subject: [PATCH] Fix validator: pipes inside regex rules, array/object input crashes, duplicate errors --- .../Http/Support/ValidationRules.php | 88 ++++ src/Phaseolies/Http/Validation/Rule.php | 4 +- .../Support/Validation/Sanitizer.php | 9 +- tests/Validation/ValidationHardeningTest.php | 421 ++++++++++++++++++ 4 files changed, 518 insertions(+), 4 deletions(-) create mode 100644 tests/Validation/ValidationHardeningTest.php diff --git a/src/Phaseolies/Http/Support/ValidationRules.php b/src/Phaseolies/Http/Support/ValidationRules.php index ccc67d3d..41fc0c83 100644 --- a/src/Phaseolies/Http/Support/ValidationRules.php +++ b/src/Phaseolies/Http/Support/ValidationRules.php @@ -150,6 +150,10 @@ protected function sanitizeUserRequest( if ($this->isNullable($input, $fieldName) && $rule !== 'null') { return null; } else { + if ($this->rejectsNonScalarValue($rule, $input[$fieldName] ?? null)) { + return $this->getErrorMessage('string', $fieldName); + } + switch ($rule) { case 'required': if ($this->isEmptyFieldRequired($input, $fieldName)) { @@ -1308,6 +1312,10 @@ protected function isDateFormat(array $input, string $fieldName, string $format) { $value = $input[$fieldName] ?? ''; + if (is_string($value) && str_contains($value, "\0")) { + return false; + } + $date = \DateTime::createFromFormat($format, $value); return $date !== false && $date->format($format) === $value; @@ -1404,6 +1412,86 @@ protected function _removeRuleSuffix(string $string): string return explode(":", $string)[0]; } + /** + * Split a "required|min:3|regex:/a|b/" rule string into its rules. + * + * @param string $ruleString + * @return array + */ + protected function splitRules(string $ruleString): array + { + $pieces = explode('|', $ruleString); + $count = count($pieces); + $rules = []; + + for ($i = 0; $i < $count; $i++) { + $rule = $pieces[$i]; + + if (str_starts_with($rule, 'regex:')) { + $joined = $rule; + $next = $i; + + while (! $this->isRegexClosed(substr($joined, 6)) && $next + 1 < $count) { + $joined .= '|' . $pieces[++$next]; + } + + // An unterminated pattern keeps its own piece instead of swallowing + // the rules that follow it. + if ($this->isRegexClosed(substr($joined, 6))) { + $rule = $joined; + $i = $next; + } + } + + $rules[] = $rule; + } + + return $rules; + } + + /** + * Determine if a regex pattern has its closing delimiter (and optional flags). + * + * @param string $pattern + * @return bool + */ + protected function isRegexClosed(string $pattern): bool + { + if (strlen($pattern) < 2) { + return false; + } + + $closing = ['(' => ')', '[' => ']', '{' => '}', '<' => '>'][$pattern[0]] ?? $pattern[0]; + + // The closing delimiter must not be escaped, i.e. be preceded by an even + // number of backslashes. + return (bool) preg_match( + '/(?splitRules($value); foreach ($fieldRules as $rule) { $ruleValue = $this->_getRuleSuffix($rule); $rule = $this->_removeRuleSuffix($rule); $errorMessage = $this->sanitizeUserRequest($input, $fieldName, $rule, $ruleValue); - if ($errorMessage) { + if ($errorMessage && ! in_array($errorMessage, $errors[$fieldName] ?? [], true)) { $errors[$fieldName][] = $errorMessage; } } diff --git a/src/Phaseolies/Support/Validation/Sanitizer.php b/src/Phaseolies/Support/Validation/Sanitizer.php index 8b1e3f61..3e7a525c 100644 --- a/src/Phaseolies/Support/Validation/Sanitizer.php +++ b/src/Phaseolies/Support/Validation/Sanitizer.php @@ -68,6 +68,9 @@ public function request(array $data, array $rules): self */ public function validate(): bool { + // Validating again must not stack the same errors on top of the old ones. + $this->errors = []; + foreach ($this->rules as $field => $ruleString) { // Custom rule class bound via Bind::to(new Rule())->context([...]) if ($ruleString instanceof Bind) { @@ -78,7 +81,7 @@ public function validate(): bool continue; } - $rulesArray = explode('|', $ruleString); + $rulesArray = $this->splitRules($ruleString); foreach ($rulesArray as $rule) { $this->applyRule($field, $rule); @@ -170,6 +173,8 @@ protected function addError(string $field, string $message) $this->errors[$field] = []; } - $this->errors[$field][] = $message; + if (!in_array($message, $this->errors[$field], true)) { + $this->errors[$field][] = $message; + } } } diff --git a/tests/Validation/ValidationHardeningTest.php b/tests/Validation/ValidationHardeningTest.php new file mode 100644 index 00000000..4e13a18e --- /dev/null +++ b/tests/Validation/ValidationHardeningTest.php @@ -0,0 +1,421 @@ +bind('translator', function () { + return new Translator($this->createMock(FileLoader::class), 'en'); + }); + + // The Rule trait flashes input to the session and reads the request. + $container->bind('request', fn() => new class { + public function except(...$keys): array + { + return []; + } + }); + $container->bind('session', fn() => new class { + public function __call($name, $args) + { + return null; + } + }); + } + + protected function tearDown(): void + { + Container::forgetInstance(); + parent::tearDown(); + } + + private function sanitizer(array $data, array $rules): Sanitizer + { + return new Sanitizer($data, $rules); + } + + private function fails(array $data, array $rules): bool + { + return $this->sanitizer($data, $rules)->fails(); + } + + /** + * Run the form-request path (Rule::sanitize) and return its errors. + * + * @return array> + */ + private function requestErrors(array $input, array $rules): array + { + $request = new class ($input) { + use Rule; + + public array $captured = []; + + public function __construct(private array $input) {} + + public function all(): array + { + return $this->input; + } + + protected function setPassedData(array $data): void {} + + protected function failValidation(array $errors): never + { + $this->captured = $errors; + + throw new HttpResponseException($errors, 422); + } + + public function run(array $rules): array + { + try { + $this->sanitize($rules); + } catch (HttpResponseException) { + } + + return $this->captured; + } + }; + + return $request->run($rules); + } + + private function splitter(): object + { + return new class { + use ValidationRules; + + public function split(string $rules): array + { + return $this->splitRules($rules); + } + }; + } + + // ---- splitting rules on "|" ------------------------------------------ + + public function testRulesAreSplitOnPipes(): void + { + $this->assertSame(['required', 'min:3', 'email'], $this->splitter()->split('required|min:3|email')); + } + + public function testASingleRuleIsReturnedAsIs(): void + { + $this->assertSame(['required'], $this->splitter()->split('required')); + $this->assertSame([''], $this->splitter()->split('')); + } + + public function testARegexWithAlternationStaysInOnePiece(): void + { + $this->assertSame( + ['required', 'regex:/^(cat|dog)$/', 'max:10'], + $this->splitter()->split('required|regex:/^(cat|dog)$/|max:10') + ); + } + + public function testARegexWithSeveralAlternationsStaysInOnePiece(): void + { + $this->assertSame(['regex:/^(a|b|c|d)$/'], $this->splitter()->split('regex:/^(a|b|c|d)$/')); + $this->assertSame(['regex:/a|b|c/'], $this->splitter()->split('regex:/a|b|c/')); + } + + public function testARegexKeepsItsFlags(): void + { + $this->assertSame( + ['regex:/^(cat|dog)$/i', 'required'], + $this->splitter()->split('regex:/^(cat|dog)$/i|required') + ); + } + + public function testARegexWithOtherDelimiters(): void + { + $this->assertSame(['regex:#^(cat|dog)$#', 'min:2'], $this->splitter()->split('regex:#^(cat|dog)$#|min:2')); + $this->assertSame(['regex:~a|b~', 'min:2'], $this->splitter()->split('regex:~a|b~|min:2')); + $this->assertSame(['regex:{a|b}', 'min:2'], $this->splitter()->split('regex:{a|b}|min:2')); + $this->assertSame(['regex:(a|b)', 'min:2'], $this->splitter()->split('regex:(a|b)|min:2')); + } + + public function testARegexWithAnEscapedDelimiterInsideIt(): void + { + $this->assertSame(['regex:/^a\/b|c$/', 'min:2'], $this->splitter()->split('regex:/^a\/b|c$/|min:2')); + } + + public function testARegexEndingInAnEscapedBackslashIsClosed(): void + { + // "\\\\/" is an escaped backslash followed by the real closing delimiter. + $this->assertSame(['regex:/^a\\\\/', 'min:2'], $this->splitter()->split('regex:/^a\\\\/|min:2')); + } + + public function testAClosedRegexDoesNotSwallowTheNextRule(): void + { + $this->assertSame(['regex:/^[a-z]+$/', 'required'], $this->splitter()->split('regex:/^[a-z]+$/|required')); + } + + public function testAnUnterminatedRegexDoesNotSwallowTheRulesAfterIt(): void + { + $this->assertSame(['regex:/abc', 'min:3'], $this->splitter()->split('regex:/abc|min:3')); + } + + public function testRegexIsOnlyTreatedSpeciallyAtTheStartOfARule(): void + { + $this->assertSame(['starts_with:regex:/a', 'b/'], $this->splitter()->split('starts_with:regex:/a|b/')); + } + + // ---- regex alternation through the validators ------------------------ + + public function testSanitizerAcceptsEveryAlternativeOfARegex(): void + { + foreach (['cat', 'dog'] as $pet) { + $this->assertFalse($this->fails(['pet' => $pet], ['pet' => 'required|regex:/^(cat|dog)$/']), $pet); + } + } + + public function testSanitizerRejectsAValueOutsideTheAlternation(): void + { + $validator = $this->sanitizer(['pet' => 'cow'], ['pet' => 'required|regex:/^(cat|dog)$/']); + + $this->assertTrue($validator->fails()); + $this->assertSame(['pet'], array_keys($validator->errors()['errors'])); + } + + public function testSanitizerRunsTheRulesAfterAnAlternationRegex(): void + { + $rules = ['pet' => 'regex:/^(cat|dog)$/|max:2']; + + $this->assertTrue($this->fails(['pet' => 'cat'], $rules), 'max:2 must still run'); + } + + public function testSanitizerRulesBeforeAnAlternationRegexStillRun(): void + { + $this->assertTrue($this->fails(['pet' => ''], ['pet' => 'required|regex:/^(cat|dog)$/'])); + } + + public function testSanitizerMatchesAnUnanchoredAlternation(): void + { + $this->assertFalse($this->fails(['x' => 'b'], ['x' => 'regex:/^a|b$/'])); + $this->assertFalse($this->fails(['x' => 'a'], ['x' => 'regex:/^a|b$/'])); + } + + public function testRequestSanitizeAcceptsEveryAlternativeOfARegex(): void + { + foreach (['cat', 'dog'] as $pet) { + $this->assertSame([], $this->requestErrors(['pet' => $pet], ['pet' => 'required|regex:/^(cat|dog)$/']), $pet); + } + } + + public function testRequestSanitizeRejectsAValueOutsideTheAlternation(): void + { + $errors = $this->requestErrors(['pet' => 'cow'], ['pet' => 'required|regex:/^(cat|dog)$/']); + + $this->assertSame(['pet'], array_keys($errors)); + } + + public function testRegexRulesWithoutPipesKeepWorking(): void + { + $this->assertFalse($this->fails(['code' => '1234'], ['code' => 'regex:/^\d{4,6}$/'])); + $this->assertTrue($this->fails(['code' => 'abc'], ['code' => 'regex:/^\d{4,6}$/'])); + } + + // ---- array and object values ----------------------------------------- + + /** + * @return array + */ + public static function scalarOnlyRules(): array + { + $rules = [ + 'min:3', 'max:3', 'date', 'gte:today', 'lte:today', 'gt:2020-01-01', 'lt:2030-01-01', + 'alpha', 'alpha_num', 'alpha_dash', 'regex:/^[a-z]+$/', 'phone', 'uuid', + 'starts_with:ab', 'ends_with:yz', 'date_format:Y-m-d', 'uppercase', 'lowercase', 'slug', + 'digits:3', 'min_digits:2', 'max_digits:4', 'same_as:other', + ]; + + return array_combine($rules, array_map(fn($r) => [$r], $rules)); + } + + #[\PHPUnit\Framework\Attributes\DataProvider('scalarOnlyRules')] + public function testAnArrayValueFailsInsteadOfThrowing(string $rule): void + { + $validator = $this->sanitizer(['f' => ['x', 'y'], 'other' => 'x'], ['f' => $rule]); + + $this->assertTrue($validator->fails()); + $this->assertArrayHasKey('f', $validator->errors()['errors']); + } + + #[\PHPUnit\Framework\Attributes\DataProvider('scalarOnlyRules')] + public function testANestedArrayValueFailsInsteadOfThrowing(string $rule): void + { + $this->assertTrue($this->fails(['f' => ['a' => ['b']], 'other' => 'x'], ['f' => $rule])); + } + + #[\PHPUnit\Framework\Attributes\DataProvider('scalarOnlyRules')] + public function testAnObjectValueFailsInsteadOfThrowing(string $rule): void + { + $this->assertTrue($this->fails(['f' => new \stdClass(), 'other' => 'x'], ['f' => $rule])); + } + + public function testAnArrayValueIsReportedAsNotAString(): void + { + $validator = $this->sanitizer(['name' => ['x']], ['name' => 'required|min:3']); + $validator->validate(); + + $this->assertSame(['The Name must be a string.'], $validator->errors()['errors']['name']); + } + + public function testAnArrayValueGivesOneErrorEvenWhenSeveralRulesRejectIt(): void + { + $validator = $this->sanitizer(['name' => ['x']], ['name' => 'required|string|min:3|max:50|alpha']); + $validator->validate(); + + $this->assertCount(1, $validator->errors()['errors']['name']); + } + + public function testStringBeforeMinStillProtectsAgainstAnArray(): void + { + $this->assertTrue($this->fails(['name' => ['x']], ['name' => 'required|string|min:3'])); + } + + public function testRequestSanitizeFailsAnArrayInsteadOfThrowing(): void + { + $errors = $this->requestErrors(['name' => ['x']], ['name' => 'required|min:3|max:50']); + + $this->assertSame(['name'], array_keys($errors)); + $this->assertCount(1, $errors['name']); + } + + public function testAnArrayStillSatisfiesRequiredAndArrayRules(): void + { + $this->assertFalse($this->fails(['tags' => ['a', 'b']], ['tags' => 'required|array'])); + $this->assertTrue($this->fails(['tags' => []], ['tags' => 'required'])); + } + + public function testRulesThatHandleArraysKeepTheirOwnBehaviour(): void + { + $this->assertTrue($this->fails(['tags' => ['a']], ['tags' => 'string'])); + $this->assertTrue($this->fails(['tags' => ['a']], ['tags' => 'email'])); + $this->assertTrue($this->fails(['tags' => ['a']], ['tags' => 'numeric'])); + } + + public function testScalarValuesAreUnaffectedByTheGuard(): void + { + $this->assertFalse($this->fails(['n' => 'abcd'], ['n' => 'min:3|max:10|alpha'])); + $this->assertFalse($this->fails(['n' => '2024-05-01'], ['n' => 'date|date_format:Y-m-d'])); + $this->assertFalse($this->fails(['n' => 12345], ['n' => 'digits:5'])); + $this->assertFalse($this->fails(['n' => 0], ['n' => 'digits:1'])); + $this->assertTrue($this->fails(['n' => 'ab'], ['n' => 'min:3'])); + } + + public function testANullAndAnEmptyStringAreStillTreatedAsEmpty(): void + { + $this->assertFalse($this->fails(['n' => ''], ['n' => 'min:3'])); + $this->assertFalse($this->fails([], ['n' => 'min:3'])); + $this->assertTrue($this->fails(['n' => null], ['n' => 'required'])); + } + + public function testAStringWithANulByteFailsDateFormatInsteadOfThrowing(): void + { + $this->assertTrue($this->fails(['d' => "2024-01-01\0"], ['d' => 'date_format:Y-m-d'])); + $this->assertTrue($this->fails(['d' => "a\0b"], ['d' => 'date_format:Y-m-d'])); + $this->assertFalse($this->fails(['d' => '2024-01-01'], ['d' => 'date_format:Y-m-d'])); + } + + // ---- validating more than once --------------------------------------- + + public function testValidatingTwiceDoesNotDuplicateErrors(): void + { + $validator = $this->sanitizer(['a' => ''], ['a' => 'required']); + + $validator->validate(); + $validator->validate(); + + $this->assertCount(1, $validator->errors()['errors']['a']); + } + + public function testCallingFailsTwiceDoesNotDuplicateErrors(): void + { + $validator = $this->sanitizer(['a' => ''], ['a' => 'required']); + + $validator->fails(); + $validator->fails(); + + $this->assertSame(['The A field is required.'], $validator->errors()['errors']['a']); + } + + public function testFailsThenValidateGivesTheSameResult(): void + { + $validator = $this->sanitizer(['a' => ''], ['a' => 'required']); + + $this->assertTrue($validator->fails()); + $this->assertFalse($validator->validate()); + $this->assertCount(1, $validator->errors()['errors']['a']); + } + + public function testErrorsAreClearedWhenTheDataBecomesValid(): void + { + $validator = $this->sanitizer(['a' => ''], ['a' => 'required']); + $validator->validate(); + + $data = new \ReflectionProperty($validator, 'data'); + $data->setValue($validator, ['a' => 'now filled']); + + $this->assertTrue($validator->validate()); + $this->assertSame([], $validator->errors()['errors']); + $this->assertNull($validator->errors()['message']); + } + + public function testDifferentFieldsKeepTheirOwnErrors(): void + { + $validator = $this->sanitizer(['a' => '', 'b' => ''], ['a' => 'required', 'b' => 'required']); + $validator->validate(); + $validator->validate(); + + $this->assertSame(['a', 'b'], array_keys($validator->errors()['errors'])); + $this->assertCount(1, $validator->errors()['errors']['a']); + $this->assertCount(1, $validator->errors()['errors']['b']); + } + + public function testTwoDifferentMessagesForOneFieldAreBothKept(): void + { + $validator = $this->sanitizer(['a' => 'ab1'], ['a' => 'alpha|uppercase']); + $validator->validate(); + + $this->assertSame( + ['The A may only contain letters.', 'The A must be uppercase.'], + $validator->errors()['errors']['a'] + ); + } + + public function testSeparateSanitizersDoNotShareErrors(): void + { + $first = $this->sanitizer(['a' => ''], ['a' => 'required']); + $second = $this->sanitizer(['a' => 'ok'], ['a' => 'required']); + + $first->validate(); + + $this->assertTrue($second->validate()); + $this->assertSame([], $second->errors()['errors']); + } +}