HPB-4006 prevent unsafe request data #29

Treggats · 2024-10-04T12:48:29Z

issue HPB-4006

With this PR we make it impossible to use Request methods which don't validate the input data.

src/Rules/ScopeRequestValidateMethods.php

tests/Rules/ScopeRequestValidateMethodTest.php

src/Rules/ScopeRequestValidateMethods.php

Treggats · 2024-10-11T08:26:17Z

Currently dealing with the following error

Line src/Rules/ScopeRequestValidateMethods.php

63 Calling PHPStan\Reflection\Php\PhpParameterFromParserNodeReflection::getType() is not covered by backward compatibility promise. The method
might change in a minor PHPStan version.
💡 If you think it should be covered by backward compatibility promise, open a discussion:
💡 https://github.com/phpstan/phpstan/discussions
💡
💡 See also:
💡 https://phpstan.org/developing-extensions/backward-compatibility-promise

Co-Authored-By: Sander Muller <[email protected]>

Co-authored-by: Sander Muller <[email protected]> Signed-off-by: Tonko Mulder <[email protected]>

Signed-off-by: Tonko Mulder <[email protected]>

Treggats · 2024-10-14T12:26:08Z

composer.json

    "require": {
        "php": "^8.2",
-        "phpstan/phpstan": "^1.10.55",
+        "phpstan/phpstan": "^1.11.0",


first version where identifiers are used

Treggats · 2024-10-14T12:26:31Z

composer.json

        "psr-4": {
-            "Hihaho\\PhpstanRules\\": "src/"
+            "Hihaho\\PhpstanRules\\": "src/",
+            "Illuminate\\Foundation\\Http\\": "tests/stubs/Illuminate/Foundation/Http",


map stubs to the stubbed namespace

tests/stubs/Illuminate/Foundation/Http/FormRequest.php

src/Rules/Validation/ScopeFormRequestValidateMethods.php

Treggats · 2024-10-15T15:12:21Z

src/Rules/Validation/ScopeFormRequestValidateMethods.php

+            return true;
+        }
+
+        return $varName === 'safe' && $this->isValidateMethod($methodName);


a validation method chained of the safe() method is allowed

Treggats · 2024-10-15T15:25:38Z

tests/Rules/Validation/ScopeIlluminateRequestValidateMethodTest.php

+    {
+        /** @var Error[] $errors */
+        $errors = $this->gatherAnalyserErrors([__DIR__ . '/../../stubs/App/Http/Controllers/PeopleControllerStub.php']);
+        $validErrors = array_filter(


there are more errors present, but we only want to assert the errors from the rules we are introducing into this PR

Treggats · 2024-10-15T15:26:33Z

tests/Rules/Validation/ScopeIlluminateRequestValidateMethodTest.php

+        );
+        self::assertCount(6, $validErrors);
+
+        $unsafeRequestDataError = static fn (int $line) => [


because of the amount of the repeated error messages, I've extracted it to a simple closure

Treggats · 2024-10-15T15:27:14Z

tests/Rules/Validation/ScopeIlluminateRequestValidateMethodTest.php

+            $unsafeRequestDataError(18),
+            $unsafeRequestDataError(21),
+            [
+                'No error with identifier method.notFound is reported on line 20.',


this extra error is present because a @phpstan-ignore is used in the stub

Treggats · 2024-10-15T15:30:37Z

tests/stubs/App/Http/Controllers/PeopleControllerStub.php

+            'children' => [
+                'name' => $request->only(['name']),
+            ],
+            'email' => $request->safe()->email, // @phpstan-ignore method.notFound


While the safe() method does not exist on the Illuminate Request class, it is allowed. So ignoring the method.notFound error, in favour of being able to assert against it.

Treggats · 2024-10-15T15:37:31Z

tests/Rules/Validation/ScopeFormRequestValidateMethodsTest.php

+    #[Test]
+    public function form_request_class_does_not_use_unvalidated_data_outside_its_namespace(): void
+    {
+        $this->analyse([__DIR__ . '/../../stubs/App/Http/Requests/UserRequest.php'], []);


usage of validation methods are allowed within the App\Http\Requests namespace

Signed-off-by: Tonko Mulder <[email protected]>

Treggats · 2024-10-16T11:21:14Z

.github/workflows/tests.yml

        - name: Install dependencies
          run: |
-            composer require "illuminate/support:${{ matrix.laravel }}" --no-interaction --no-update
+            composer require "laravel/framework:${{ matrix.laravel }}" --no-interaction --no-update


replacing the illuminate packages with laravel/framework, as we need additional classes and it's easier to have one dependency

Treggats · 2024-10-16T11:21:45Z

composer.json

        "psr-4": {
-            "Hihaho\\PhpstanRules\\": "src/"
+            "Hihaho\\PhpstanRules\\": "src/",
+            "App\\Http\\Controllers\\": "tests/stubs/App/Http/Controllers",


mapping namespaces to the stub file location

Treggats · 2024-10-16T11:22:46Z

composer.json

-        "php": "^8.2",
-        "phpstan/phpstan": "^1.10.55",
-        "illuminate/support": "^10|^11",
-        "laravel/pint": "^1.13.9"


the PHPStan and Laravel Pint packages are used during development. So made more sense to have them in the require-dev

"illuminate/support": "^10|^11", would need to stay, as without it you'd be able to install the package in Laravel < 10 (or even, not in a Laravel app)
Could also change it for illuminate/contracts, not sure what the best one would be

Signed-off-by: Tonko Mulder <[email protected]>

Treggats · 2024-10-29T09:31:35Z

Putting in draft while I work on some internal errors 🙈

Signed-off-by: Tonko Mulder <[email protected]>

it is handled by the other rule Signed-off-by: Tonko Mulder <[email protected]>

it not only covers the Illuminate Request but also the FormRequest and NovaRequest Signed-off-by: Tonko Mulder <[email protected]>

Signed-off-by: Tonko Mulder <[email protected]>

note; PHPStan throws the following error. Ignoring because I do not have the time to properly figure this out. Creating new PHPStan\Node\AnonymousClassNode is not covered by backward compatibility promise. The class might change in a minor PHPStan version. Signed-off-by: Tonko Mulder <[email protected]>

using concatenation because I had issues with formatting the test Signed-off-by: Tonko Mulder <[email protected]>

Treggats · 2024-10-31T13:58:19Z

extension.neon

+
+parameters:
+	scoperequestvalidatemethods:
+		allowedRequestMethods:


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%%' `

Treggats · 2024-10-31T13:58:50Z

extension.neon

@@ -1,4 +1,31 @@
+parametersSchema:


PHPStan requires a schema for the parameters used by the rule

Treggats · 2024-10-31T13:59:31Z

src/Rules/Validation/ScopeRequestValidateMethods.php

+{
+    public function __construct(array $allowedRequestMethods)
+    {
+        $this->allowedRequestMethods = array_unique($allowedRequestMethods);


autowired by the PHPStan config. Currently there is no overriding, only appending to this list

Treggats · 2024-10-31T14:01:06Z

src/Rules/Validation/ScopeValidationMethods.php

+            try {
+                return (new ReflectionClass($className))->getMethods();
+            } catch (ReflectionException) {
+                // @phpstan-ignore phpstanApi.constructor


ignoring because PHPStan complains that it is not backwards compatible, but otherwise we'd get a "not found" for anonymous classes.

Treggats self-assigned this Oct 4, 2024

Treggats marked this pull request as draft October 4, 2024 12:56

RobertBoes reviewed Oct 4, 2024

View reviewed changes

src/Rules/ScopeRequestValidateMethods.php Outdated Show resolved Hide resolved

SanderMuller requested changes Oct 6, 2024

View reviewed changes

src/Rules/ScopeRequestValidateMethods.php Outdated Show resolved Hide resolved

tests/Rules/ScopeRequestValidateMethodTest.php Outdated Show resolved Hide resolved

src/Rules/ScopeRequestValidateMethods.php Outdated Show resolved Hide resolved

Treggats force-pushed the feature/HPB-4006-restrict-validation-methods branch from 71e1236 to d8211ff Compare October 11, 2024 08:11

Treggats force-pushed the feature/HPB-4006-restrict-validation-methods branch from d8211ff to d6158cf Compare October 14, 2024 11:34

Treggats added 3 commits October 14, 2024 13:37

HPB-4006 add FormRequest from vendor Laravel

bd70371

HPB-4005 add stubs

aa4c010

HPB-4006 add test for using unvalidated data outside of a request class

4fdca15

Treggats force-pushed the feature/HPB-4006-restrict-validation-methods branch 3 times, most recently from 509a207 to 191c5d5 Compare October 14, 2024 12:24

Treggats and others added 6 commits October 15, 2024 09:44

HPB-4005 implement the rule

c5f6722

HPB-4006 add a controller class stub

b6524ea

HPB-4005 simplify checking the blacklist

ab806f2

Co-Authored-By: Sander Muller <[email protected]>

HPB-4006 also check sub namespace

a576dd9

Co-authored-by: Sander Muller <[email protected]> Signed-off-by: Tonko Mulder <[email protected]>

HPB-4006 update minimum version PHPStan (1.11.0)

c544a03

Signed-off-by: Tonko Mulder <[email protected]>

HPB-4006 simplify expected error message

c7df77f

Signed-off-by: Tonko Mulder <[email protected]>

Treggats force-pushed the feature/HPB-4006-restrict-validation-methods branch from dc27990 to 97fed3e Compare October 15, 2024 09:55

Treggats added 4 commits October 15, 2024 13:10

HPB-4006 update test name and separate regular request and form request

acdb7a6

Signed-off-by: Tonko Mulder <[email protected]>

HPB-4006 update method names to better reflect its purpose

dc911f1

Signed-off-by: Tonko Mulder <[email protected]>

HPB-4006 merge illuminate & form request check

b5f0922

Signed-off-by: Tonko Mulder <[email protected]>

HPB-4006 check for chained allowed methods

b7c590d

Signed-off-by: Tonko Mulder <[email protected]>

Treggats force-pushed the feature/HPB-4006-restrict-validation-methods branch from 97fed3e to b7c590d Compare October 15, 2024 11:10

Treggats added 5 commits October 15, 2024 13:24

HPB-4006 move to Validation namespace

09dbde9

Signed-off-by: Tonko Mulder <[email protected]>

HPB-4006 introduce base class

368fccf

Signed-off-by: Tonko Mulder <[email protected]>

HPB-4006 split form request and illuminate request rules

082351d

Signed-off-by: Tonko Mulder <[email protected]>

HPB-4006 update name for the illuminate request rule

da7ca65

Signed-off-by: Tonko Mulder <[email protected]>

HPB-4006 update name for the form request rule

7877685

Signed-off-by: Tonko Mulder <[email protected]>

Treggats added 7 commits October 15, 2024 16:57

HPB-4006 update name blacklist check

38ceef0

Signed-off-by: Tonko Mulder <[email protected]>

HPB-4006 move name helper to the base class

51b6479

Signed-off-by: Tonko Mulder <[email protected]>

HPB-4006 remove redundant guard clause

3bbde4f

Signed-off-by: Tonko Mulder <[email protected]>

HPB-4006 remove redundant assertion

4f76367

Signed-off-by: Tonko Mulder <[email protected]>

HPB-4006 remove outdated class

efe3ae8

Signed-off-by: Tonko Mulder <[email protected]>

HPB-4006 add support for the "validated()" method

b72726a

Signed-off-by: Tonko Mulder <[email protected]>

HPB-4006 cs & dataset update

eac95d2

Signed-off-by: Tonko Mulder <[email protected]>

Treggats commented Oct 15, 2024

View reviewed changes

Treggats marked this pull request as ready for review October 15, 2024 15:40

Treggats requested review from RobertBoes and SanderMuller October 15, 2024 15:41

Treggats added 2 commits October 16, 2024 13:15

HPB-4006 replace illuminate packages with laravel/framework

96aefae

Signed-off-by: Tonko Mulder <[email protected]>

HPB-4006 remove illuminate stub class

0817eee

Signed-off-by: Tonko Mulder <[email protected]>

Treggats commented Oct 16, 2024

View reviewed changes

HPB-4006 reorder composer packages

8c7f99a

Signed-off-by: Tonko Mulder <[email protected]>

RobertBoes approved these changes Oct 16, 2024

View reviewed changes

Treggats marked this pull request as draft October 29, 2024 09:31

Treggats added 10 commits October 29, 2024 15:59

HPB-4006 extract namespace check

7e0efbc

Signed-off-by: Tonko Mulder <[email protected]>

HPB-4006 move allowed methods to a config

4ec6942

Signed-off-by: Tonko Mulder <[email protected]>

HPB-4006 simplify mapping test stubs to namespace

d989686

Signed-off-by: Tonko Mulder <[email protected]>

HPB-4006 remove the FormRequest rule

ae6d2d7

it is handled by the other rule Signed-off-by: Tonko Mulder <[email protected]>

HPB-4006 rename rule

d15e50d

it not only covers the Illuminate Request but also the FormRequest and NovaRequest Signed-off-by: Tonko Mulder <[email protected]>

HPB-4006 add "usesValidMethod" method to check for allowed methods

5464b76

Signed-off-by: Tonko Mulder <[email protected]>

HPB-4006 update getting the node name

992249c

Signed-off-by: Tonko Mulder <[email protected]>

HPB-4006 solidify retrieving method parameter classnames

61b014a

Signed-off-by: Tonko Mulder <[email protected]>

HPB-4006 add additional info to the tip

fe37273

using concatenation because I had issues with formatting the test Signed-off-by: Tonko Mulder <[email protected]>

Treggats marked this pull request as ready for review October 31, 2024 14:41

Treggats commented Oct 31, 2024

View reviewed changes

HPB-4006 prevent unsafe request data #29

Are you sure you want to change the base?

HPB-4006 prevent unsafe request data #29

Uh oh!

Conversation

Treggats commented Oct 4, 2024

issue HPB-4006

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Treggats commented Oct 11, 2024

Line src/Rules/ScopeRequestValidateMethods.php

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RobertBoes Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Treggats commented Oct 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

RobertBoes Oct 16, 2024 •

edited

Loading