Bug #89434

Epic #90106: [Extbase] Priority Bugfixes

Action argument values will get lost on validation error

Added by Florian Wessels 8 months ago. Updated about 1 month ago.

Status:
Closed
Priority:
Should have
Category:
Extbase
Target version:
Start date:
2019-10-16
Due date:
% Done:

100%

TYPO3 Version:
10
PHP Version:
7.2
Tags:
Complexity:
Is Regression:
Yes
Sprint Focus:

Description

Action arguments and values will get lost when given object is invalid.

How to Reproduce

Create an ActionController with at least following actions:

/**
 * @param MyObject $myObject
 * @TYPO3\CMS\Extbase\Annotation\IgnoreValidation("myObject")
 */
public function editAction(MyObject $myObject): void
{
    $this->view->assign('myObject', $myObject);
}

/**
 * @param MyObject $myObject
 */
public function updateAction(MyObject $myObject): void
{
    $this->myObjectRepository->update($myObject);
    $this->redirect('index');
}

The object itself has a validator:

/**
 * @var float
 * @TYPO3\CMS\Extbase\Annotation\Validate("NumberRange", options={"minimum": -1000, "maximum": 100})
 */
protected $value = 0;

If you now try to edit an object and set the value to 1000, following exception will be thrown:

#1298012500 TYPO3\CMS\Extbase\Mvc\Controller\Exception\RequiredArgumentMissingException
Required argument "myObject" is not set for Vendor\Example\Controller\SampleController->edit.

Backtrace:

/var/www/example/web/typo3/sysext/extbase/Classes/Mvc/Controller/AbstractController.php line 399
/var/www/example/web/typo3/sysext/extbase/Classes/Mvc/Controller/ActionController.php line 162
/var/www/example/web/typo3/sysext/extbase/Classes/Mvc/Dispatcher.php line 81
/var/www/example/web/typo3/sysext/extbase/Classes/Mvc/Web/FrontendRequestHandler.php line 92
/var/www/example/web/typo3/sysext/extbase/Classes/Core/Bootstrap.php line 181
/var/www/example/web/typo3/sysext/extbase/Classes/Core/Bootstrap.php line 171

Possible Solution

Modify the renderHiddenReferrerFields method in typo3/sysext/fluid/Classes/ViewHelpers/FormViewHelper.php:

if (!empty($request->getArguments())) {
    $actionRequest += $this->getActionArguments($request, $actionName);
}

Add this method to the same class:

/**
 * Get all valid action arguments and their values.
 *
 * @param Request $request
 * @param string  $actionName
 *
 * @return array
 * @throws \ReflectionException
 */
protected function getActionArguments(Request $request, string $actionName): array
{
    $actionArguments = [];
    $reflectionMethod = new \ReflectionMethod($request->getControllerObjectName(), $actionName . 'Action');
    $reflectionParameters = $reflectionMethod->getParameters();

    foreach ($reflectionParameters as $reflectionParameter) {
        $argumentName = $reflectionParameter->getName();
        if ($request->hasArgument($argumentName)) {
            try {
                $actionArguments[$argumentName] = $request->getArgument($argumentName);
            } catch (NoSuchArgumentException $exception) {
                // Do nothing.
            }
        }
    }

    return $actionArguments;
}

Make sure to add missing use statements.

extbase_arguments.zip (6.15 KB) Oliver Hader, 2020-02-02 18:20

Bildschirmfoto 2020-04-06 um 15.31.52.png View (32.6 KB) Torben Hansen, 2020-04-06 15:32


Related issues

Related to TYPO3 Core - Bug #89937: Insecure Deserialization when knowing encryptionKey in Extbase Closed 2019-12-13
Related to TYPO3 Core - Bug #88682: Switch to json_encode/json_decode for Extbase arguments Closed 2019-07-03
Related to TYPO3 Core - Task #88697: Remove dependencies of \TYPO3\CMS\Extbase\Mvc\Web\Request Closed 2019-07-06
Related to TYPO3 Core - Task #87550: Use controller classes when registering plugins/modules Closed 2019-01-25
Duplicated by TYPO3 Core - Bug #91134: The object of type "<Model>" given to update must be persisted already, but is new. Closed 2020-04-19

Associated revisions

Revision f33fd60f (diff)
Added by Oliver Hader about 2 months ago

[BUGFIX] Reintroduce Extbase referring argument handling

Change https://review.typo3.org/c/Packages/TYPO3.CMS/+/61223 (v10 only)
modified Extbase referring argument handling.

This change reintroduces `$internalArguments['__referrer']['arguments']`
to be used when redirecting back to original request action in case some
validation errors occurred - otherwise those arguments are lost.

This basically reverts commit 19b5ee0f3706bf996a4287e6e89449dd71c4bef3
(adjusted for code clean up and refactorings that happened in between)

Resolves: #89434
Releases: master
Change-Id: Id83f38d20ecd482b517c37c4244d871a40ff0a89
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/63152
Tested-by: TYPO3com <>
Tested-by: Torben Hansen <>
Tested-by: Alexander Schnitzler <>
Reviewed-by: Torben Hansen <>
Reviewed-by: Alexander Schnitzler <>

Revision c0e7b296 (diff)
Added by Oliver Hader about 1 month ago

[BUGFIX] Reintroduce Extbase referring argument handling

Change https://review.typo3.org/c/Packages/TYPO3.CMS/+/62702 (v9)
modified Extbase referring argument handling.

It turned out that this approach to harden security aspects had a
couple of negative side-effects (e.g. that valid objects were not
available anymore on validation failures). Thus, plain `unserialize`
for `$internalArguments['__referrer']['arguments']` is now in place
again - which still requires to be singed with a valid HMAC.

In addition to that, additional functional tests for this scenario
have been added with this changeset.

This partly reverts commit b1626ad8fd4aebedc15e424a76f86094d78b2564
(reverts `arguments`, but keeps `@request` and `__trustedProperties`)

Resolves: #89434
Releases: 9.5
Change-Id: Id83f38d20ecd482b517c37c4244d871a40ff0a89
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/63151
Tested-by: TYPO3com <>
Tested-by: Anja Leichsenring <>
Tested-by: Alexander Schnitzler <>
Reviewed-by: Anja Leichsenring <>
Reviewed-by: Alexander Schnitzler <>

History

#1 Updated by Alexander Schnitzler 5 months ago

  • Assignee set to Alexander Schnitzler

#2 Updated by Alexander Schnitzler 5 months ago

  • Is Regression set to Yes

This bug has been introduced with this commit: https://review.typo3.org/c/Packages/TYPO3.CMS/+/61223

#3 Updated by Gerrit Code Review 5 months 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/c/Packages/TYPO3.CMS/+/62866

#4 Updated by Gerrit Code Review 5 months 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/c/Packages/TYPO3.CMS/+/62866

#5 Updated by Gerrit Code Review 5 months 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/c/Packages/TYPO3.CMS/+/62866

#6 Updated by Gerrit Code Review 5 months 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/c/Packages/TYPO3.CMS/+/62866

#7 Updated by Alexander Schnitzler 5 months ago

  • Parent task set to #90106

#8 Updated by Oliver Hader 4 months ago

Demo Extension for "object arguments"... created for v9, should show behavior in v10 as well..

#9 Updated by Oliver Hader 4 months ago

  • Related to Bug #89937: Insecure Deserialization when knowing encryptionKey in Extbase added

#10 Updated by Oliver Hader 4 months ago

  • Related to Bug #88682: Switch to json_encode/json_decode for Extbase arguments added

#11 Updated by Gerrit Code Review 4 months ago

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

#12 Updated by Gerrit Code Review 4 months ago

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

#13 Updated by Gerrit Code Review 4 months ago

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/c/Packages/TYPO3.CMS/+/63152

#14 Updated by Oliver Hader 4 months ago

  • Related to Task #88697: Remove dependencies of \TYPO3\CMS\Extbase\Mvc\Web\Request added

#15 Updated by Oliver Hader 4 months ago

  • Related to Task #87550: Use controller classes when registering plugins/modules added

#16 Updated by Gerrit Code Review 4 months ago

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

#17 Updated by Torben Hansen about 2 months ago

Can I somehow help to get this issue solved? I think we really need this working with the first release of TYPO3 10 LTS, since this may be a blocker for people to update. If there is not enough time to get Ollys WIP patch finished before the release, Patchset 3 (https://review.typo3.org/c/Packages/TYPO3.CMS/+/62866/3) should be merged and a new issue about hardening referring arguments could be created for later processing.

#18 Updated by Felix Nagel about 2 months ago

@Torban I experienced problems similar to what has been describe here with my blog extension but with the release of 10.3.0 everything works as expected again.

#19 Updated by Torben Hansen about 2 months ago

felix I'm working on current master and the problem still exist. I just tested your current master branch and also there I can reproduce the problem (see screenshot). Your comment form has HTML5 required fields, which "resolves "the problem with invalid form data on client side, because the user has to enter something in the form. But as soon as you work with pure Extbase validation (remove @required="required" from your form fields), the problem persist.

#20 Updated by Felix Nagel about 2 months ago

Oh boy, thanks for pointing this out. I just did a quick test as I need to rework the email functionality for 10.x anyway and did not run in it again. Seems I was wrong. I agree, this one is a MUST HAVE for the LTS release.

#21 Updated by Alexander Schnitzler about 2 months ago

  • Target version set to 10 LTS

#22 Updated by Gerrit Code Review about 2 months 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/c/Packages/TYPO3.CMS/+/63152

#23 Updated by Gerrit Code Review about 2 months 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/c/Packages/TYPO3.CMS/+/63152

#24 Updated by Gerrit Code Review about 2 months 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/c/Packages/TYPO3.CMS/+/63152

#25 Updated by Gerrit Code Review about 2 months 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/c/Packages/TYPO3.CMS/+/63152

#26 Updated by Gerrit Code Review about 2 months 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/c/Packages/TYPO3.CMS/+/63152

#27 Updated by Gerrit Code Review about 2 months 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/c/Packages/TYPO3.CMS/+/63152

#28 Updated by Gerrit Code Review about 2 months ago

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

#29 Updated by Gerrit Code Review about 2 months 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/c/Packages/TYPO3.CMS/+/63152

#30 Updated by Gerrit Code Review about 2 months ago

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

#31 Updated by Gerrit Code Review about 2 months 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/c/Packages/TYPO3.CMS/+/63152

#32 Updated by Gerrit Code Review about 2 months 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/c/Packages/TYPO3.CMS/+/63152

#33 Updated by Gerrit Code Review about 2 months 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/c/Packages/TYPO3.CMS/+/63152

#34 Updated by Gerrit Code Review about 2 months 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/c/Packages/TYPO3.CMS/+/63152

#35 Updated by Gerrit Code Review about 2 months 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/c/Packages/TYPO3.CMS/+/63152

#36 Updated by Gerrit Code Review about 2 months ago

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

#37 Updated by Oliver Hader about 2 months ago

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

#38 Updated by Benni Mack about 1 month ago

  • Status changed from Resolved to Closed

#39 Updated by Simon Schaufelberger about 1 month ago

  • Related to Bug #91134: The object of type "<Model>" given to update must be persisted already, but is new. added

#40 Updated by Simon Schaufelberger about 1 month ago

  • Related to deleted (Bug #91134: The object of type "<Model>" given to update must be persisted already, but is new.)

#41 Updated by Simon Schaufelberger about 1 month ago

  • Duplicated by Bug #91134: The object of type "<Model>" given to update must be persisted already, but is new. added

Also available in: Atom PDF