-
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 all 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,8 @@ | |
| "license": "MIT", | ||
| "autoload": { | ||
| "psr-4": { | ||
| "Hihaho\\PhpstanRules\\": "src/" | ||
| "Hihaho\\PhpstanRules\\": "src/", | ||
| "App\\": "tests/stubs/App/" | ||
| } | ||
| }, | ||
| "autoload-dev": { | ||
|
|
@@ -23,20 +24,16 @@ | |
| "minimum-stability": "stable", | ||
| "require": { | ||
| "php": "^8.2", | ||
| "phpstan/phpstan": "^1.10.55", | ||
| "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.
|
||
| "phpstan/phpstan": "^1.12" | ||
| }, | ||
| "require-dev": { | ||
| "phpunit/phpunit": "^11.0", | ||
| "nikic/php-parser": "^5.0", | ||
| "spatie/invade": "^2.0", | ||
| "illuminate/http": "^10|^11", | ||
| "illuminate/console": "^10|^11", | ||
| "illuminate/mail": "^10|^11", | ||
| "illuminate/notifications": "^10|^11", | ||
| "illuminate/routing": "^10|^11", | ||
| "roave/security-advisories": "dev-latest" | ||
| "roave/security-advisories": "dev-latest", | ||
| "laravel/framework": "^10|^11", | ||
| "laravel/pint": "^1.18" | ||
| }, | ||
| "scripts": { | ||
| "test": "phpunit", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,31 @@ | ||
| parametersSchema: | ||
|
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. PHPStan requires a schema for the parameters used by the rule |
||
| scoperequestvalidatemethods: structure([ | ||
| allowedRequestMethods: arrayOf(string()), | ||
| ]) | ||
|
|
||
| parameters: | ||
| scoperequestvalidatemethods: | ||
| allowedRequestMethods: | ||
|
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. default valid validation methods, this can be appended by the local PHPStan config. diff --git a/phpstan.neon b/phpstan.neon
index f011012500..ee07b64d03 100644
--- a/phpstan.neon
+++ b/phpstan.neon
@@ -50,6 +50,11 @@ parameters:
# ignore the following error: Called 'Model::make()' which performs unnecessary work, use 'new Model()'.
noModelMake: false
+ scoperequestvalidatemethods:
+ allowedRequestMethods:
+ - 'safe'
+ - 'validated'
+
# Optional for having a clickable link to PHPStorm
editorUrl: 'phpstorm://open?file=%%file%%&line=%%line%%'
` |
||
| - collect | ||
| - all | ||
| - only | ||
| - except | ||
| - input | ||
| - get | ||
| - keys | ||
| - string | ||
| - str | ||
| - integer | ||
| - float | ||
| - boolean | ||
|
|
||
| services: | ||
| - | ||
| class: Hihaho\PhpstanRules\Rules\Validation\ScopeRequestValidateMethods | ||
| arguments: | ||
| allowedRequestMethods: %scoperequestvalidatemethods.allowedRequestMethods% | ||
| tags: | ||
| - phpstan.rules.rule | ||
| - | ||
| class: Hihaho\PhpstanRules\Rules\NoInvadeInAppCode | ||
| tags: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| <?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 ScopeRequestValidateMethods extends ScopeValidationMethods | ||
| { | ||
| public function __construct(array $allowedRequestMethods) | ||
| { | ||
| $this->allowedRequestMethods = array_unique($allowedRequestMethods); | ||
|
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. autowired by the PHPStan config. Currently there is no overriding, only appending to this list |
||
| } | ||
|
|
||
| public function getNodeType(): string | ||
| { | ||
| return MethodCall::class; | ||
| } | ||
|
|
||
| /** | ||
| * @phpstan-param MethodCall $node | ||
| * @throws ShouldNotHappenException | ReflectionException | ||
| */ | ||
| public function processNode(Node $node, Scope $scope): array | ||
| { | ||
| if ($this->hasValidNamespace($scope->getNamespace())) { | ||
| return []; | ||
| } | ||
|
|
||
| if (! $this->hasIlluminateRequestClass($scope)) { | ||
| return []; | ||
| } | ||
|
|
||
| if ($this->usesValidMethod(varName: $this->nodeName($node->var), methodName: $this->nodeName($node))) { | ||
| return []; | ||
| } | ||
|
|
||
| return [ | ||
| RuleErrorBuilder::message( | ||
| 'Usage of unvalidated request data is not allowed outside of App\\Http\\Requests' | ||
| ) | ||
| ->nonIgnorable() | ||
| ->addTip('Use $request->safe() / $request->validated() to use request data') | ||
| ->addTip(sprintf('Current checking: variable %s, method %s', $this->nodeName($node->var), $this->nodeName($node))) | ||
| ->identifier('hihaho.request.unsafeRequestData') | ||
| ->build(), | ||
| ]; | ||
| } | ||
|
|
||
| /** @throws ReflectionException */ | ||
| private function hasIlluminateRequestClass(Scope $scope): bool | ||
| { | ||
| return collect($this->getClassMethods($scope)) | ||
| ->map(fn (array $method) => $this->getMethodParameterClassnames($method) | ||
| ->filter(static fn (?string $fqn) => $fqn !== null) | ||
| ->filter(static fn (string $fqn) => $fqn === IlluminateRequest::class) | ||
| ) | ||
| ->isNotEmpty(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,156 @@ | ||
| <?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; | ||
| use PhpParser\Node\Expr\CallLike; | ||
| use PhpParser\Node\Expr\PropertyFetch; | ||
| use PhpParser\Node\Expr\Variable; | ||
| use PhpParser\Node\Identifier; | ||
| use PhpParser\NodeAbstract; | ||
| use PHPStan\Analyser\Scope; | ||
| use PHPStan\Node\AnonymousClassNode; | ||
| 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 | ||
| { | ||
| protected array $allowedRequestMethods = []; | ||
|
|
||
| abstract public function getNodeType(): string; | ||
|
|
||
| abstract public function processNode(Node $node, Scope $scope): array; | ||
|
|
||
| /** | ||
| * @phpstan-return ReflectionMethod[] | ||
| */ | ||
| 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 |
||
| { | ||
| if (! $className = $scope->getClassReflection()?->getName()) { | ||
| return []; | ||
| } | ||
|
|
||
| $type = new ObjectType(className: $className, classReflection: $scope->getClassReflection()); | ||
| /** @var ReflectionMethod[] $methods */ | ||
| $methods = array_map(static function (string $className): array { | ||
| try { | ||
| return (new ReflectionClass($className))->getMethods(); | ||
| } catch (ReflectionException) { | ||
| // @phpstan-ignore phpstanApi.constructor | ||
|
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. ignoring because PHPStan complains that it is not backwards compatible, but otherwise we'd get a "not found" for anonymous classes. |
||
| return (new AnonymousClassNode($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 fn (ReflectionParameter $parameter) => $parameter->getType(), $parameter) | ||
| ) | ||
| ->flatten() | ||
| ->filter() | ||
| ->filter(fn ($type) => method_exists($type, 'getName')) | ||
| ->map(fn (ReflectionNamedType|ReflectionUnionType|ReflectionIntersectionType $type) => $type->getName()); | ||
| } | ||
|
|
||
| protected function nodeName(CallLike|Expr|NodeAbstract $var): string | ||
| { | ||
| if (! property_exists($var, 'name')) { | ||
| return ''; | ||
| } | ||
|
|
||
| if ($var->name instanceof Stringable) { | ||
| return $var->name->toString(); | ||
| } | ||
|
|
||
| if ($var->name instanceof Identifier) { | ||
| if (method_exists($var->name, 'toString')) { | ||
| return $var->name->toString(); | ||
| } | ||
|
|
||
| return $var->name->name; | ||
| } | ||
|
|
||
| if ($var->name instanceof Variable) { | ||
| return $var->name->name; | ||
| } | ||
|
|
||
| if ($var->name instanceof Node\Expr\BinaryOp\Concat) { | ||
| if (method_exists($var->name, 'toString')) { | ||
| return $var->name->toString(); | ||
| } | ||
|
|
||
| return ''; | ||
| } | ||
|
|
||
| if ($var->name instanceof PropertyFetch) { | ||
| if (method_exists($var->name, 'toString')) { | ||
| return $var->name->toString(); | ||
| } | ||
|
|
||
| return $var->name->name->toString(); | ||
| } | ||
|
|
||
| if ($var instanceof PropertyFetch) { | ||
| return $var->name->toString(); | ||
| } | ||
|
|
||
| if (method_exists($var->name, 'toString')) { | ||
| return $var->name->toString(); | ||
| } | ||
|
|
||
| return $var->name ?? ''; | ||
| } | ||
|
|
||
| protected function hasValidNamespace(?string $namespace): bool | ||
| { | ||
| if (! $namespace) { | ||
| return false; | ||
| } | ||
|
|
||
| return str_starts_with($namespace, 'App\\Http\\Request'); | ||
| } | ||
|
|
||
| protected function usesValidMethod(string $varName, string $methodName): bool | ||
| { | ||
| if (! in_array($varName, ['request', 'safe', 'validated'], true)) { | ||
| return true; | ||
| } | ||
|
|
||
| if ($varName === 'request' && ($methodName === 'validated' || $methodName === 'validate')) { | ||
| return true; | ||
| } | ||
|
|
||
| if ($varName === 'request' && $methodName === 'safe') { | ||
| return true; | ||
| } | ||
|
|
||
| if ($varName === 'safe') { | ||
| return $this->isValidateMethod($methodName); | ||
| } | ||
|
|
||
| return ! $this->isValidateMethod($methodName); | ||
| } | ||
|
|
||
| protected function isValidateMethod(string $methodName): bool | ||
| { | ||
| return in_array($methodName, $this->allowedRequestMethods, strict: true); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| <?php declare(strict_types=1); | ||
|
|
||
| namespace Rules\Validation; | ||
|
|
||
| use Hihaho\PhpstanRules\Rules\Validation\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([ | ||
| 'collect', | ||
| 'all', | ||
| 'only', | ||
| 'except', | ||
| 'input', | ||
| 'get', | ||
| 'keys', | ||
| 'string', | ||
| 'str', | ||
| 'integer', | ||
| 'float', | ||
| 'boolean', | ||
| ]); | ||
| } | ||
|
|
||
| #[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); | ||
|
|
||
| foreach ($validErrors as $key => $error) { | ||
| self::assertStringContainsString('Usage of unvalidated request data is not allowed outside of App\\Http\\Requests', $error->getMessage()); | ||
| self::assertFalse($error->canBeIgnored()); | ||
| self::assertStringContainsString('Use $request->safe() / $request->validated() to use request data', $error->getTip()); | ||
| self::assertStringContainsString('Current checking: variable request, method', $error->getTip()); | ||
| match($key) { | ||
| 0 => self::assertSame(13, $error->getNodeLine()), | ||
| 1 => self::assertSame(14, $error->getNodeLine()), | ||
| 2 => self::assertSame(15, $error->getNodeLine()), | ||
| 3 => self::assertSame(16, $error->getNodeLine()), | ||
| 4 => self::assertSame(18, $error->getNodeLine()), | ||
| 5 => self::assertSame(21, $error->getNodeLine()), | ||
| }; | ||
| } | ||
| } | ||
| } |
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.
replacing the illuminate packages with laravel/framework, as we need additional classes and it's easier to have one dependency