-
Notifications
You must be signed in to change notification settings - Fork 1
HPB-4006 prevent unsafe request data #29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 25 commits
bd70371
aa4c010
4fdca15
c5f6722
b6524ea
ab806f2
a576dd9
c544a03
c7df77f
acdb7a6
dc911f1
b5f0922
b7c590d
09dbde9
368fccf
082351d
da7ca65
7877685
38ceef0
51b6479
3bbde4f
4f76367
efe3ae8
b72726a
eac95d2
96aefae
0817eee
8c7f99a
7e0efbc
4ec6942
d989686
ae6d2d7
d15e50d
5464b76
992249c
61b014a
370e174
fe37273
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,10 @@ | |
| "license": "MIT", | ||
| "autoload": { | ||
| "psr-4": { | ||
| "Hihaho\\PhpstanRules\\": "src/" | ||
| "Hihaho\\PhpstanRules\\": "src/", | ||
| "Illuminate\\Foundation\\Http\\": "tests/stubs/Illuminate/Foundation/Http", | ||
| "App\\Http\\Controllers\\": "tests/stubs/App/Http/Controllers", | ||
|
||
| "App\\Http\\Requests\\": "tests/stubs/App/Http/Requests" | ||
| } | ||
| }, | ||
| "autoload-dev": { | ||
|
|
@@ -23,7 +26,7 @@ | |
| "minimum-stability": "stable", | ||
| "require": { | ||
| "php": "^8.2", | ||
| "phpstan/phpstan": "^1.10.55", | ||
| "phpstan/phpstan": "^1.11.0", | ||
|
||
| "illuminate/support": "^10|^11", | ||
| "laravel/pint": "^1.13.9" | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the PHPStan and Laravel Pint packages are used during development. So made more sense to have them in the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| }, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| <?php declare(strict_types=1); | ||
|
|
||
| namespace Hihaho\PhpstanRules\Rules\Validation; | ||
|
|
||
| use Illuminate\Foundation\Http\FormRequest; | ||
| use PhpParser\Node; | ||
| use PhpParser\Node\Expr\MethodCall; | ||
| use PHPStan\Analyser\Scope; | ||
| use PHPStan\Rules\RuleErrorBuilder; | ||
| use PHPStan\ShouldNotHappenException; | ||
| use ReflectionClass; | ||
| use ReflectionException; | ||
|
|
||
| final class ScopeFormRequestValidateMethods extends ScopeValidationMethods | ||
| { | ||
| public function getNodeType(): string | ||
| { | ||
| return MethodCall::class; | ||
| } | ||
|
|
||
| /** | ||
| * @phpstan-param MethodCall $node | ||
| * @throws ReflectionException | ||
| * @throws ShouldNotHappenException | ||
| */ | ||
| public function processNode(Node $node, Scope $scope): array | ||
| { | ||
| if (str_starts_with($scope->getNamespace(), 'App\\Http\\Request')) { | ||
| return []; | ||
| } | ||
|
|
||
| if (! $this->hasFormRequestClass($scope)) { | ||
| return []; | ||
| } | ||
|
|
||
| if ($this->usesValidMethod(varName: $this->nameFrom($node->var), methodName: $this->nameFrom($node))) { | ||
| return []; | ||
| } | ||
|
|
||
| return [ | ||
| RuleErrorBuilder::message( | ||
| 'Usage of unvalidated request data is not allowed outside of App\\Http\\Requests' | ||
| ) | ||
| ->nonIgnorable() | ||
| ->tip('Use $request->safe() to use request data') | ||
| ->identifier('hihaho.request.unsafeRequestData') | ||
| ->build(), | ||
| ]; | ||
| } | ||
|
|
||
| /** @throws ReflectionException */ | ||
| private function hasFormRequestClass(Scope $scope): bool | ||
|
||
| { | ||
| $parentClassName = static fn (ReflectionClass $fqn) => $fqn->getParentClass()->getName(); | ||
|
|
||
| return collect($this->getClassMethods($scope)) | ||
| ->map(fn (array $method) => $this->getMethodParameterClassnames($method)) | ||
| ->flatten() | ||
| ->map(fn (string $className) => new ReflectionClass($className)) | ||
| ->filter(fn (ReflectionClass $className) => $parentClassName($className) === FormRequest::class) | ||
| ->isNotEmpty(); | ||
| } | ||
|
|
||
| private function usesValidMethod(string $varName, string $methodName): bool | ||
| { | ||
| if ($varName === 'request' && ($methodName === 'safe' || $methodName === 'validated')) { | ||
| return true; | ||
| } | ||
|
|
||
| return $varName === 'safe' && $this->isValidateMethod($methodName); | ||
|
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| <?php declare(strict_types=1); | ||
|
|
||
| namespace Hihaho\PhpstanRules\Rules\Validation; | ||
|
|
||
| use Illuminate\Http\Request as IlluminateRequest; | ||
| use PhpParser\Node; | ||
| use PhpParser\Node\Expr\MethodCall; | ||
| use PHPStan\Analyser\Scope; | ||
| use PHPStan\Rules\RuleErrorBuilder; | ||
| use PHPStan\ShouldNotHappenException; | ||
| use ReflectionException; | ||
|
|
||
| final class ScopeIlluminateRequestValidateMethods extends ScopeValidationMethods | ||
| { | ||
| public function getNodeType(): string | ||
| { | ||
| return MethodCall::class; | ||
| } | ||
|
|
||
| /** | ||
| * @phpstan-param MethodCall $node | ||
| * @throws ShouldNotHappenException | ReflectionException | ||
| */ | ||
| public function processNode(Node $node, Scope $scope): array | ||
| { | ||
| if (str_starts_with($scope->getNamespace(), 'App\\Http\\Request')) { | ||
| return []; | ||
| } | ||
|
|
||
| if (! $this->hasIlluminateRequestClass($scope)) { | ||
| return []; | ||
| } | ||
|
|
||
| if (! $this->isValidateMethod($this->nameFrom($node))) { | ||
| return []; | ||
| } | ||
|
|
||
| return [ | ||
| RuleErrorBuilder::message( | ||
| 'Usage of unvalidated request data is not allowed outside of App\\Http\\Requests' | ||
| ) | ||
| ->nonIgnorable() | ||
| ->tip('Use $request->safe() to use request data') | ||
| ->identifier('hihaho.request.unsafeRequestData') | ||
| ->build(), | ||
| ]; | ||
| } | ||
|
|
||
| /** @throws ReflectionException */ | ||
| private function hasIlluminateRequestClass(Scope $scope): bool | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this check is a bit simpler than the FormRequest one. Reason is that we directly have a class we need to check. So no need for extra checks or conversions. |
||
| { | ||
| return collect($this->getClassMethods($scope)) | ||
| ->map(fn (array $method) => $this->getMethodParameterClassnames($method) | ||
| ->filter(fn (string $fqn) => $fqn === IlluminateRequest::class) | ||
| ) | ||
| ->isNotEmpty(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| <?php declare(strict_types=1); | ||
|
|
||
| namespace Hihaho\PhpstanRules\Rules\Validation; | ||
|
|
||
| use Illuminate\Support\Collection; | ||
| use Illuminate\Support\Stringable; | ||
| use PhpParser\Node; | ||
| use PhpParser\Node\Expr\MethodCall; | ||
| use PhpParser\Node\Expr\Variable; | ||
| use PhpParser\Node\Identifier; | ||
| use PHPStan\Analyser\Scope; | ||
| use PHPStan\Rules\Rule; | ||
| use PHPStan\Type\ObjectType; | ||
| use ReflectionClass; | ||
| use ReflectionException; | ||
| use ReflectionIntersectionType; | ||
| use ReflectionMethod; | ||
| use ReflectionNamedType; | ||
| use ReflectionParameter; | ||
| use ReflectionUnionType; | ||
|
|
||
| /** | ||
| * @implements \PHPStan\Rules\Rule<\PhpParser\Node\Expr\MethodCall> | ||
| */ | ||
| abstract class ScopeValidationMethods implements Rule | ||
| { | ||
| abstract public function getNodeType(): string; | ||
|
|
||
| abstract public function processNode(Node $node, Scope $scope): array; | ||
|
|
||
| /** | ||
| * @phpstan-return ReflectionMethod[] | ||
| * @throws ReflectionException | ||
| */ | ||
| protected function getClassMethods(Scope $scope): array | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the |
||
| { | ||
| $type = new ObjectType(className: $scope->getClassReflection()?->getName(), classReflection: $scope->getClassReflection()); | ||
| /** @var ReflectionMethod[] $methods */ | ||
| $methods = array_map(static function (string $className): array { | ||
| return (new ReflectionClass($className))->getMethods(); | ||
| }, $type->getObjectClassNames()); | ||
|
|
||
| return $methods; | ||
| } | ||
|
|
||
| protected function getMethodParameterClassnames(array $method): Collection | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this method receives a list of methods, for which it will get the parameters and return the type. |
||
| { | ||
| return collect($method) | ||
| ->map(fn (ReflectionMethod $method) => $method->getParameters()) | ||
| ->map(fn (array $parameter) => array_map(static function (ReflectionParameter $parameter) { | ||
| /** @var ReflectionNamedType|ReflectionUnionType|ReflectionIntersectionType|null $type */ | ||
| $type = $parameter->getType(); | ||
|
|
||
| return $type?->getName(); | ||
| }, $parameter)) | ||
| ->flatten(); | ||
| } | ||
|
|
||
| protected function nameFrom(MethodCall|Variable $var): string | ||
|
||
| { | ||
| if ($var->name instanceof Stringable) { | ||
| return $var->name->toString(); | ||
| } | ||
|
|
||
| if ($var->name instanceof Identifier) { | ||
| return $var->name->toString(); | ||
| } | ||
|
|
||
| return $var->name; | ||
| } | ||
|
|
||
| protected function isValidateMethod(string $methodName): bool | ||
| { | ||
| $methodNames = [ | ||
| 'collect', | ||
| 'all', | ||
| 'only', | ||
| 'except', | ||
| 'input', | ||
| 'get', | ||
| 'keys', | ||
| 'string', | ||
| 'str', | ||
| 'integer', | ||
| 'float', | ||
| 'boolean', | ||
| ]; | ||
|
|
||
| return in_array($methodName, $methodNames, strict: true); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| <?php declare(strict_types=1); | ||
|
|
||
| namespace Rules\Validation; | ||
|
|
||
| use Hihaho\PhpstanRules\Rules\Validation\ScopeFormRequestValidateMethods; | ||
| use PHPStan\Rules\Rule; | ||
| use PHPStan\Testing\RuleTestCase; | ||
| use PHPUnit\Framework\Attributes\Test; | ||
|
|
||
| final class ScopeFormRequestValidateMethodsTest extends RuleTestCase | ||
| { | ||
| protected function getRule(): Rule | ||
| { | ||
| return new ScopeFormRequestValidateMethods(); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function form_request_class_does_not_use_unvalidated_data_outside_its_namespace(): void | ||
| { | ||
| $this->analyse([__DIR__ . '/../../stubs/App/Http/Requests/UserRequest.php'], []); | ||
|
||
|
|
||
| $this->analyse([__DIR__ . '/../../stubs/App/Http/Controllers/PetControllerStub.php'], [ | ||
| [ | ||
| 'Usage of unvalidated request data is not allowed outside of App\\Http\\Requests', | ||
| 14, | ||
| 'Use $request->safe() to use request data', | ||
| ], | ||
| ]); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| <?php declare(strict_types=1); | ||
|
|
||
| namespace Rules\Validation; | ||
|
|
||
| use Hihaho\PhpstanRules\Rules\Validation\ScopeIlluminateRequestValidateMethods; | ||
| use PHPStan\Analyser\Error; | ||
| use PHPStan\Rules\Rule; | ||
| use PHPStan\Testing\RuleTestCase; | ||
| use PHPUnit\Framework\Attributes\Test; | ||
|
|
||
| /** | ||
| * @extends RuleTestCase<ScopeIlluminateRequestValidateMethods> | ||
| */ | ||
| final class ScopeIlluminateRequestValidateMethodTest extends RuleTestCase | ||
| { | ||
| protected function getRule(): Rule | ||
| { | ||
| return new ScopeIlluminateRequestValidateMethods(); | ||
| } | ||
|
|
||
| #[Test] | ||
| public function illuminate_http_request_does_not_use_unvalidated_methods_outside_app_http_requests_namespace(): void | ||
| { | ||
| /** @var Error[] $errors */ | ||
| $errors = $this->gatherAnalyserErrors([__DIR__ . '/../../stubs/App/Http/Controllers/PeopleControllerStub.php']); | ||
| $validErrors = array_filter( | ||
|
||
| $errors, | ||
| static fn (Error $error): bool => $error->getIdentifier() === 'hihaho.request.unsafeRequestData' | ||
| ); | ||
| self::assertCount(6, $validErrors); | ||
|
|
||
| $unsafeRequestDataError = static fn (int $line) => [ | ||
|
||
| 'Usage of unvalidated request data is not allowed outside of App\\Http\\Requests', | ||
| $line, | ||
| 'Use $request->safe() to use request data', | ||
| ]; | ||
|
|
||
| $this->analyse([__DIR__ . '/../../stubs/App/Http/Controllers/PeopleControllerStub.php'], [ | ||
| $unsafeRequestDataError(13), | ||
| $unsafeRequestDataError(14), | ||
| $unsafeRequestDataError(15), | ||
| $unsafeRequestDataError(16), | ||
| $unsafeRequestDataError(18), | ||
| $unsafeRequestDataError(21), | ||
| [ | ||
| 'No error with identifier method.notFound is reported on line 20.', | ||
|
||
| 20, | ||
| ], | ||
| ]); | ||
|
|
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| <?php declare(strict_types=1); | ||
|
|
||
| namespace App\Http\Controllers; | ||
|
|
||
| use Illuminate\Http\JsonResponse; | ||
| use Illuminate\Http\Request; | ||
|
|
||
| final class PeopleControllerStub | ||
| { | ||
| public function __invoke(Request $request) | ||
| { | ||
| return new JsonResponse([ | ||
| 'first_name' => $request->input('first_name'), | ||
| 'last_name' => $request->string('last_name'), | ||
| 'age' => $request->integer('age'), | ||
| 'has_children' => $request->boolean('has_children'), | ||
| 'children' => [ | ||
| 'name' => $request->only(['name']), | ||
| ], | ||
| 'email' => $request->safe()->email, // @phpstan-ignore method.notFound | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While the |
||
| 'city' => $request->get('city'), | ||
| ]); | ||
| } | ||
|
|
||
| public function show(Request $request): JsonResponse | ||
| { | ||
| $data = $request->validate([]); | ||
|
|
||
| return new JsonResponse([ | ||
| 'first_name' => $data['first_name'], | ||
| 'last_name' => $data['last_name'], | ||
| 'age' => $data['age'], | ||
| 'has_children' => $data['has_children'], | ||
| 'children' => [ | ||
| 'name' => $data['name'], | ||
| ], | ||
| 'email' => $data['email'], | ||
| 'city' => $data['city'], | ||
| ]); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
map stubs to the stubbed namespace