-
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 13 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,158 @@ | ||
| <?php declare(strict_types=1); | ||
|
|
||
| namespace Hihaho\PhpstanRules\Rules; | ||
|
|
||
| use Illuminate\Foundation\Http\FormRequest; | ||
| use Illuminate\Http\Request as IlluminateRequest; | ||
| use Illuminate\Support\Collection; | ||
| use Illuminate\Support\Stringable; | ||
| use PhpParser\Node; | ||
| use PhpParser\Node\Expr\MethodCall; | ||
| use PHPStan\Analyser\Scope; | ||
| use PHPStan\Rules\Rule; | ||
| use PHPStan\Rules\RuleErrorBuilder; | ||
| use PHPStan\ShouldNotHappenException; | ||
| 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> | ||
| */ | ||
| final class ScopeRequestValidateMethods implements Rule | ||
Treggats marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| { | ||
| public function getNodeType(): string | ||
| { | ||
| return MethodCall::class; | ||
| } | ||
|
|
||
| /** @throws ShouldNotHappenException | ReflectionException */ | ||
| public function processNode(Node $node, Scope $scope): array | ||
| { | ||
| if (str_starts_with($scope->getNamespace(), 'App\\Http\\Request')) { | ||
| return []; | ||
| } | ||
|
|
||
| if (! $this->hasRequestClass($scope)) { | ||
| return []; | ||
| } | ||
|
|
||
| if ($this->usesValidatedMethod($node) && $this->isBlacklistedMethod($node->name->toString())) { | ||
| return []; | ||
| } | ||
|
|
||
| if (! $this->isBlacklistedMethod($node->name->toString())) { | ||
| 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 hasRequestClass(Scope $scope): bool | ||
| { | ||
| return $this->hasIlluminateRequestClass($scope) || $this->hasFormRequestClass($scope); | ||
| } | ||
|
|
||
| /** @throws ReflectionException */ | ||
| private function hasIlluminateRequestClass(Scope $scope): bool | ||
| { | ||
| return collect($this->getClassMethods($scope)) | ||
| ->map(fn (array $method) => $this->getMethodParameterClassnames($method) | ||
| ->filter(fn (string $fqn) => $fqn === IlluminateRequest::class) | ||
| ) | ||
| ->isNotEmpty(); | ||
| } | ||
|
|
||
| /** @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(); | ||
| } | ||
|
|
||
| /** | ||
| * @phpstan-param MethodCall $node | ||
| */ | ||
| private function usesValidatedMethod(Node $node): bool | ||
| { | ||
| /** @var Node\Expr\Variable $var */ | ||
| $var = $node->var; | ||
| if ($var->name instanceof Stringable) { | ||
| return $var->name->toString() === 'safe'; | ||
| } | ||
|
|
||
| if ($var->name instanceof Node\Identifier) { | ||
| return $var->name->toString() === 'safe'; | ||
| } | ||
|
|
||
| return $var->name === 'safe'; | ||
| } | ||
|
|
||
| /** | ||
| * @phpstan-return ReflectionMethod[] | ||
| * @throws ReflectionException | ||
| */ | ||
| private function getClassMethods(Scope $scope): array | ||
| { | ||
| $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; | ||
| } | ||
|
|
||
| private function getMethodParameterClassnames(array $method): Collection | ||
| { | ||
| 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(); | ||
| } | ||
|
|
||
| private function isBlacklistedMethod(string $methodName): bool | ||
| { | ||
| $blacklistedMethodNames = [ | ||
| 'collect', | ||
| 'all', | ||
| 'only', | ||
| 'except', | ||
| 'input', | ||
| 'get', | ||
| 'keys', | ||
| 'string', | ||
| 'str', | ||
| 'integer', | ||
| 'float', | ||
| 'boolean', | ||
| ]; | ||
|
|
||
| return in_array($methodName, $blacklistedMethodNames, strict: true); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| <?php declare(strict_types=1); | ||
|
|
||
| namespace Rules; | ||
|
|
||
| use Hihaho\PhpstanRules\Rules\ScopeRequestValidateMethods; | ||
| use PHPStan\Analyser\Error; | ||
| use PHPStan\Rules\Rule; | ||
| use PHPStan\Testing\RuleTestCase; | ||
| use PHPUnit\Framework\Attributes\Test; | ||
|
|
||
| /** | ||
| * @extends RuleTestCase<ScopeRequestValidateMethods> | ||
| */ | ||
| final class ScopeRequestValidateMethodTest extends RuleTestCase | ||
| { | ||
| protected function getRule(): Rule | ||
| { | ||
| return new ScopeRequestValidateMethods(); | ||
| } | ||
|
|
||
| #[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, | ||
| ], | ||
| ]); | ||
|
|
||
| } | ||
|
|
||
| #[Test] | ||
| public function form_request_class_does_not_use_unvalidated_data_outside_its_namespace(): void | ||
| { | ||
| /** @var Error[] $errors */ | ||
| $errors = $this->gatherAnalyserErrors([__DIR__ . '/../stubs/App/Http/Requests/UserRequest.php']); | ||
| self::assertCount(0, $errors); | ||
| $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,39 @@ | ||
| <?php declare(strict_types=1); | ||
|
|
||
| namespace App\Http\Controllers; | ||
|
|
||
| use App\Http\Requests\UserRequest; | ||
| use Illuminate\Http\Request; | ||
|
|
||
| final class RequestValidateMethodsInNamespaceStub | ||
| { | ||
| public function __invoke(Request $request) | ||
| { | ||
| return response()->json([ | ||
| '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, | ||
| 'city' => $request->get('city'), | ||
| ]); | ||
| } | ||
|
|
||
| public function show(UserRequest $request) | ||
| { | ||
| return response()->json([ | ||
| 'first_name' => $request->safe()->string('first_name'), | ||
| 'last_name' => $request->safe()->string('last_name'), | ||
| 'age' => $request->safe()->integer('age'), | ||
| 'has_children' => $request->safe()->boolean('has_children'), | ||
| 'children' => [ | ||
| 'name' => $request->safe()->only(['name']), | ||
| ], | ||
| 'email' => $request->safe()->string('email'), | ||
| 'city' => $request->safe()->string('city'), | ||
| ]); | ||
| } | ||
| } |
| 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'], | ||
| ]); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| <?php declare(strict_types=1); | ||
|
|
||
| namespace App\Http\Controllers; | ||
|
|
||
| use App\Http\Requests\UserRequest; | ||
| use Illuminate\Http\JsonResponse; | ||
|
|
||
| final class PetControllerStub | ||
| { | ||
| public function __invoke(UserRequest $request) | ||
| { | ||
| return new JsonResponse([ | ||
| 'data' => [ | ||
| 'name' => $request->get('name'), | ||
| 'breed' => $request->safe()->str('breed'), | ||
| ], | ||
| ]); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| <?php declare(strict_types=1); | ||
|
|
||
| namespace App\Http\Requests; | ||
|
|
||
| use Illuminate\Foundation\Http\FormRequest; | ||
|
|
||
| final class UserRequest extends FormRequest | ||
| { | ||
| public function name(): string | ||
| { | ||
| return sprintf( | ||
| '%s %s', | ||
| $this->string('first_name'), | ||
| $this->string('last_name') | ||
| ); | ||
| } | ||
|
|
||
| public function email(): string | ||
| { | ||
| return $this->input('email'); | ||
| } | ||
|
|
||
| public function hasChildren(): bool | ||
| { | ||
| return $this->boolean('children'); | ||
| } | ||
|
|
||
| public function childrenNames(): array | ||
| { | ||
| return $this->get('children') | ||
| ->only('name'); | ||
| } | ||
| } |
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