Bug #77338

Extbase validation cache allows invalid objects

Added by Alexander Stehlik almost 4 years ago. Updated over 1 year ago.

Status:
Closed
Priority:
Should have
Assignee:
-
Category:
Extbase
Target version:
-
Start date:
2016-08-01
Due date:
% Done:

100%

TYPO3 Version:
8
PHP Version:
7.0
Tags:
Complexity:
medium
Is Regression:
No
Sprint Focus:

Description

In the \TYPO3\CMS\Extbase\Validation\Validator\GenericObjectValidator is a cache for already validated object instances (validatedInstancesContainer).

This makes total sense but there is a big flaw in the concept: the validation results are not restored. This causes a problem in these scenarios:

Action forwarding with @ignorevalidation

Imagine you have a action1() and a $property1.

action1() has an @ignorevalidation annotation for $property1 and forwards to action2().

action2() has no @ignorevalidation annotation but validation errors for $property1 are still ignored because the cache in the GenericObjectValidator is not reset and the previous validation results are not loaded.

This allows the user to pass invalid data to action2().

Object relations

The second scenario would be the following. You have two method arguments $param1 and $param2.

$param1 has a relation to $param2 and because child objects are validated you get the proper validation errors for $param1.$param2.

But you won't get any validation errors for your $param2 controller argument because of the cache.

This is problematic in two ways:

  1. If $param1 has an @ignorevalidation annotation the user can submit invalid data to your action
  2. You can not display any validation errors in your form for $param2

My suggestion to solve this is to store the validation results in the cache as well and restore them if needed.

The problem is valid since 6.2 until current master.

patch_77338.diff View (2.97 KB) Bernhard Kraft, 2016-10-14 13:59

Associated revisions

Revision adb6d0ca (diff)
Added by Alexander Stehlik almost 2 years ago

[TASK] Add functional test for exbase validation caching

The tests demonstrate the issue described in the ticket
is not present anymore.

Resolves: #77338
Releases: master
Change-Id: I7002b9754f556f23850fda5026861c742f70bcdc
Reviewed-on: https://review.typo3.org/49307
Reviewed-by: Christian Kuhn <>
Tested-by: Christian Kuhn <>
Tested-by: TYPO3com <>
Reviewed-by: Anja Leichsenring <>
Tested-by: Anja Leichsenring <>

History

#1 Updated by Gerrit Code Review almost 4 years ago

  • Status changed from New to Under Review

Patch set 1 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/49307

#2 Updated by Gerrit Code Review almost 4 years ago

Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/49307

#3 Updated by Gerrit Code Review over 3 years ago

Patch set 3 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/49307

#4 Updated by Bernhard Kraft over 3 years ago

I would suggest an alternate solution. I can currently not run the functional unit tests you created because I am working on a machine running PHP5 and PHP7 is required. But in my setup this solution works for a problem similar to those described by you.

In fact the problem is buried in how the "GenericObjectValidator" gets instanced from within "ValidatorResolver". There only one instance is created for each type (class) of objects.

So if you have multiple "Blog" instances passed to your controller, or have multiple "Blog" objects in some kind of object structure the described problems will occur.

My solution is to have a "$this->resultStack" in GenericObjectValidator.

#5 Updated by Gerrit Code Review over 3 years ago

Patch set 4 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/49307

#6 Updated by Gerrit Code Review over 3 years ago

Patch set 5 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/49307

#7 Updated by Gerrit Code Review over 3 years ago

Patch set 6 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/49307

#8 Updated by Gerrit Code Review over 2 years ago

Patch set 7 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/49307

#9 Updated by Gerrit Code Review over 2 years ago

Patch set 8 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/49307

#10 Updated by Gerrit Code Review over 2 years ago

Patch set 9 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/49307

#11 Updated by Gerrit Code Review almost 2 years ago

Patch set 10 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/49307

#12 Updated by Gerrit Code Review almost 2 years ago

Patch set 11 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/49307

#13 Updated by Gerrit Code Review almost 2 years ago

Patch set 12 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/49307

#14 Updated by Gerrit Code Review almost 2 years ago

Patch set 13 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/49307

#15 Updated by Gerrit Code Review almost 2 years ago

Patch set 14 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/49307

#16 Updated by Alexander Stehlik almost 2 years ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100

#17 Updated by Benni Mack over 1 year ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF