Completed
Push — master ( 5247c6...b8f763 )
by Craig
10:35 queued 03:41
created

AbstractEditHandler::fetchInputData()   B

Complexity

Conditions 6
Paths 4

Size

Total Lines 16
Code Lines 7

Duplication

Lines 0
Ratio 0 %

Importance

Changes 0
Metric Value
cc 6
eloc 7
nc 4
nop 1
dl 0
loc 16
rs 8.8571
c 0
b 0
f 0
1
<?php
2
/**
3
 * Routes.
4
 *
5
 * @copyright Zikula contributors (Zikula)
6
 * @license http://www.gnu.org/licenses/lgpl.html GNU Lesser General Public License
7
 * @author Zikula contributors <[email protected]>.
8
 * @link http://www.zikula.org
9
 * @link http://zikula.org
10
 * @version Generated by ModuleStudio 0.7.1 (http://modulestudio.de).
11
 */
12
13
namespace Zikula\RoutesModule\Form\Handler\Common\Base;
14
15
use Psr\Log\LoggerInterface;
16
use RuntimeException;
17
use Symfony\Component\Form\AbstractType;
18
use Symfony\Component\Form\FormFactoryInterface;
19
use Symfony\Component\HttpFoundation\RedirectResponse;
20
use Symfony\Component\HttpFoundation\Request;
21
use Symfony\Component\HttpFoundation\RequestStack;
22
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
23
use Symfony\Component\Routing\RouterInterface;
24
use Symfony\Component\Security\Core\Exception\AccessDeniedException;
25
use Zikula\Bundle\CoreBundle\HttpKernel\ZikulaHttpKernelInterface;
26
use Zikula\Common\Translator\TranslatorInterface;
27
use Zikula\Common\Translator\TranslatorTrait;
28
use Zikula\Core\Doctrine\EntityAccess;
29
use Zikula\PageLockModule\Api\LockingApi;
30
use Zikula\PermissionsModule\Api\PermissionApi;
31
use Zikula\RoutesModule\Entity\Factory\RoutesFactory;
32
use Zikula\RoutesModule\Helper\ControllerHelper;
33
use Zikula\RoutesModule\Helper\ModelHelper;
34
use Zikula\RoutesModule\Helper\SelectionHelper;
35
use Zikula\RoutesModule\Helper\WorkflowHelper;
36
use Zikula\UsersModule\Api\CurrentUserApi;
37
38
/**
39
 * This handler class handles the page events of editing forms.
40
 * It collects common functionality required by different object types.
41
 */
42
abstract class AbstractEditHandler
43
{
44
    use TranslatorTrait;
45
46
    /**
47
     * Name of treated object type.
48
     *
49
     * @var string
50
     */
51
    protected $objectType;
52
53
    /**
54
     * Name of treated object type starting with upper case.
55
     *
56
     * @var string
57
     */
58
    protected $objectTypeCapital;
59
60
    /**
61
     * Lower case version.
62
     *
63
     * @var string
64
     */
65
    protected $objectTypeLower;
66
67
    /**
68
     * Permission component based on object type.
69
     *
70
     * @var string
71
     */
72
    protected $permissionComponent;
73
74
    /**
75
     * Reference to treated entity instance.
76
     *
77
     * @var EntityAccess
78
     */
79
    protected $entityRef = null;
80
81
    /**
82
     * List of identifier names.
83
     *
84
     * @var array
85
     */
86
    protected $idFields = [];
87
88
    /**
89
     * List of identifiers of treated entity.
90
     *
91
     * @var array
92
     */
93
    protected $idValues = [];
94
95
    /**
96
     * Code defining the redirect goal after command handling.
97
     *
98
     * @var string
99
     */
100
    protected $returnTo = null;
101
102
    /**
103
     * Whether a create action is going to be repeated or not.
104
     *
105
     * @var boolean
106
     */
107
    protected $repeatCreateAction = false;
108
109
    /**
110
     * Url of current form with all parameters for multiple creations.
111
     *
112
     * @var string
113
     */
114
    protected $repeatReturnUrl = null;
115
116
    /**
117
     * Whether an existing item is used as template for a new one.
118
     *
119
     * @var boolean
120
     */
121
    protected $hasTemplateId = false;
122
123
    /**
124
     * Whether the PageLock extension is used for this entity type or not.
125
     *
126
     * @var boolean
127
     */
128
    protected $hasPageLockSupport = false;
129
130
    /**
131
     * @var ZikulaHttpKernelInterface
132
     */
133
    protected $kernel;
134
135
    /**
136
     * @var FormFactoryInterface
137
     */
138
    protected $formFactory;
139
140
    /**
141
     * The current request.
142
     *
143
     * @var Request
144
     */
145
    protected $request;
146
147
    /**
148
     * The router.
149
     *
150
     * @var RouterInterface
151
     */
152
    protected $router;
153
154
    /**
155
     * @var LoggerInterface
156
     */
157
    protected $logger;
158
159
    /**
160
     * @var PermissionApi
161
     */
162
    protected $permissionApi;
163
164
    /**
165
     * @var CurrentUserApi
166
     */
167
    protected $currentUserApi;
168
169
    /**
170
     * @var RoutesFactory
171
     */
172
    protected $entityFactory;
173
174
    /**
175
     * @var ControllerHelper
176
     */
177
    protected $controllerHelper;
178
179
    /**
180
     * @var ModelHelper
181
     */
182
    protected $modelHelper;
183
184
    /**
185
     * @var SelectionHelper
186
     */
187
    protected $selectionHelper;
188
189
    /**
190
     * @var WorkflowHelper
191
     */
192
    protected $workflowHelper;
193
194
    /**
195
     * Reference to optional locking api.
196
     *
197
     * @var LockingApi
198
     */
199
    protected $lockingApi = null;
200
201
    /**
202
     * The handled form type.
203
     *
204
     * @var AbstractType
205
     */
206
    protected $form;
207
208
    /**
209
     * Template parameters.
210
     *
211
     * @var array
212
     */
213
    protected $templateParameters = [];
214
215
    /**
216
     * EditHandler constructor.
217
     *
218
     * @param ZikulaHttpKernelInterface $kernel Kernel service instance
219
     * @param TranslatorInterface $translator Translator service instance
220
     * @param FormFactoryInterface $formFactory FormFactory service instance
221
     * @param RequestStack $requestStack RequestStack service instance
222
     * @param RouterInterface $router Router service instance
223
     * @param LoggerInterface $logger Logger service instance
224
     * @param PermissionApi $permissionApi PermissionApi service instance
225
     * @param CurrentUserApi $currentUserApi CurrentUserApi service instance
226
     * @param RoutesFactory $entityFactory RoutesFactory service instance
227
     * @param ControllerHelper $controllerHelper ControllerHelper service instance
228
     * @param ModelHelper $modelHelper ModelHelper service instance
229
     * @param SelectionHelper $selectionHelper SelectionHelper service instance
230
     * @param WorkflowHelper $workflowHelper WorkflowHelper service instance
231
     */
232
    public function __construct(
233
        ZikulaHttpKernelInterface $kernel,
234
        TranslatorInterface $translator,
235
        FormFactoryInterface $formFactory,
236
        RequestStack $requestStack,
237
        RouterInterface $router,
238
        LoggerInterface $logger,
239
        PermissionApi $permissionApi,
240
        CurrentUserApi $currentUserApi,
241
        RoutesFactory $entityFactory,
242
        ControllerHelper $controllerHelper,
243
        ModelHelper $modelHelper,
244
        SelectionHelper $selectionHelper,
245
        WorkflowHelper $workflowHelper)
246
    {
247
        $this->kernel = $kernel;
248
        $this->setTranslator($translator);
249
        $this->formFactory = $formFactory;
250
        $this->request = $requestStack->getCurrentRequest();
251
        $this->router = $router;
252
        $this->logger = $logger;
253
        $this->permissionApi = $permissionApi;
254
        $this->currentUserApi = $currentUserApi;
255
        $this->entityFactory = $entityFactory;
256
        $this->controllerHelper = $controllerHelper;
257
        $this->modelHelper = $modelHelper;
258
        $this->selectionHelper = $selectionHelper;
259
        $this->workflowHelper = $workflowHelper;
260
    }
261
262
    /**
263
     * Sets the translator.
264
     *
265
     * @param TranslatorInterface $translator Translator service instance
266
     */
267
    public function setTranslator(/*TranslatorInterface */$translator)
268
    {
269
        $this->translator = $translator;
0 ignored issues
show
Documentation Bug introduced by
$translator is of type object<Zikula\Common\Tra...or\TranslatorInterface>, but the property $translator was declared to be of type object<Zikula\Common\Translator\Translator>. Are you sure that you always receive this specific sub-class here, or does it make sense to add an instanceof check?

Our type inference engine has found a suspicous assignment of a value to a property. This check raises an issue when a value that can be of a given class or a super-class is assigned to a property that is type hinted more strictly.

Either this assignment is in error or an instanceof check should be added for that assignment.

class Alien {}

class Dalek extends Alien {}

class Plot
{
    /** @var  Dalek */
    public $villain;
}

$alien = new Alien();
$plot = new Plot();
if ($alien instanceof Dalek) {
    $plot->villain = $alien;
}
Loading history...
270
    }
271
272
    /**
273
     * Initialise form handler.
274
     *
275
     * This method takes care of all necessary initialisation of our data and form states.
276
     *
277
     * @param array $templateParameters List of preassigned template variables
278
     *
279
     * @return boolean False in case of initialisation errors, otherwise true
280
     *
281
     * @throws NotFoundHttpException Thrown if item to be edited isn't found
282
     * @throws RuntimeException      Thrown if the workflow actions can not be determined
283
     */
284
    public function processForm(array $templateParameters)
285
    {
286
        $this->templateParameters = $templateParameters;
287
        $this->templateParameters['inlineUsage'] = $this->request->query->getBoolean('raw', false);
288
289
        // initialise redirect goal
290
        $this->returnTo = $this->request->query->get('returnTo', null);
291
        if (null === $this->returnTo) {
292
            // default to referer
293
            if ($this->request->getSession()->has('zikularoutesmoduleReferer')) {
294
                $this->returnTo = $this->request->getSession()->get('zikularoutesmoduleReferer');
295
            } elseif ($this->request->headers->has('zikularoutesmoduleReferer')) {
296
                $this->returnTo = $this->request->headers->get('zikularoutesmoduleReferer');
0 ignored issues
show
Documentation Bug introduced by
It seems like $this->request->headers-...laroutesmoduleReferer') can also be of type array. However, the property $returnTo is declared as type string. Maybe add an additional type check?

Our type inference engine has found a suspicous assignment of a value to a property. This check raises an issue when a value that can be of a mixed type is assigned to a property that is type hinted more strictly.

For example, imagine you have a variable $accountId that can either hold an Id object or false (if there is no account id yet). Your code now assigns that value to the id property of an instance of the Account class. This class holds a proper account, so the id value must no longer be false.

Either this assignment is in error or a type check should be added for that assignment.

class Id
{
    public $id;

    public function __construct($id)
    {
        $this->id = $id;
    }

}

class Account
{
    /** @var  Id $id */
    public $id;
}

$account_id = false;

if (starsAreRight()) {
    $account_id = new Id(42);
}

$account = new Account();
if ($account instanceof Id)
{
    $account->id = $account_id;
}
Loading history...
297
                $this->request->getSession()->set('zikularoutesmoduleReferer', $this->returnTo);
298
            } elseif ($this->request->server->has('HTTP_REFERER')) {
299
                $this->returnTo = $this->request->server->get('HTTP_REFERER');
300
                $this->request->getSession()->set('zikularoutesmoduleReferer', $this->returnTo);
301
            }
302
        }
303
        // store current uri for repeated creations
304
        $this->repeatReturnUrl = $this->request->getSchemeAndHttpHost() . $this->request->getBasePath() . $this->request->getPathInfo();
305
306
        $this->permissionComponent = 'ZikulaRoutesModule:' . $this->objectTypeCapital . ':';
307
308
        $this->idFields = $this->selectionHelper->getIdFields($this->objectType);
309
310
        // retrieve identifier of the object we wish to view
311
        $this->idValues = $this->controllerHelper->retrieveIdentifier($this->request, [], $this->objectType, $this->idFields);
312
        $hasIdentifier = $this->controllerHelper->isValidIdentifier($this->idValues);
313
314
        $entity = null;
0 ignored issues
show
Unused Code introduced by
$entity is not used, you could remove the assignment.

This check looks for variable assignements that are either overwritten by other assignments or where the variable is not used subsequently.

$myVar = 'Value';
$higher = false;

if (rand(1, 6) > 3) {
    $higher = true;
} else {
    $higher = false;
}

Both the $myVar assignment in line 1 and the $higher assignment in line 2 are dead. The first because $myVar is never used and the second because $higher is always overwritten for every possible time line.

Loading history...
315
        $this->templateParameters['mode'] = $hasIdentifier ? 'edit' : 'create';
316
317
        if ($this->templateParameters['mode'] == 'edit') {
318 View Code Duplication
            if (!$this->permissionApi->hasPermission($this->permissionComponent, $this->createCompositeIdentifier() . '::', ACCESS_EDIT)) {
0 ignored issues
show
Duplication introduced by
This code seems to be duplicated across your project.

Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation.

You can also find more detailed suggestions in the “Code” section of your repository.

Loading history...
319
                throw new AccessDeniedException();
320
            }
321
322
            $entity = $this->initEntityForEditing();
323
            if (!is_object($entity)) {
324
                return false;
325
            }
326
327
            if (true === $this->hasPageLockSupport && $this->kernel->isBundle('ZikulaPageLockModule') && null !== $this->lockingApi) {
328
                // try to guarantee that only one person at a time can be editing this entity
329
                $lockName = 'ZikulaRoutesModule' . $this->objectTypeCapital . $this->createCompositeIdentifier();
330
                $this->lockingApi->addLock($lockName, $this->getRedirectUrl(null));
0 ignored issues
show
Bug introduced by
It seems like you code against a specific sub-type and not the parent class Zikula\RoutesModule\Form...ase\AbstractEditHandler as the method getRedirectUrl() does only exist in the following sub-classes of Zikula\RoutesModule\Form...ase\AbstractEditHandler: Zikula\RoutesModule\Form...ase\AbstractEditHandler, Zikula\RoutesModule\Form\Handler\Route\EditHandler. Maybe you want to instanceof check for one of these explicitly?

Let’s take a look at an example:

abstract class User
{
    /** @return string */
    abstract public function getPassword();
}

class MyUser extends User
{
    public function getPassword()
    {
        // return something
    }

    public function getDisplayName()
    {
        // return some name.
    }
}

class AuthSystem
{
    public function authenticate(User $user)
    {
        $this->logger->info(sprintf('Authenticating %s.', $user->getDisplayName()));
        // do something.
    }
}

In the above example, the authenticate() method works fine as long as you just pass instances of MyUser. However, if you now also want to pass a different sub-classes of User which does not have a getDisplayName() method, the code will break.

Available Fixes

  1. Change the type-hint for the parameter:

    class AuthSystem
    {
        public function authenticate(MyUser $user) { /* ... */ }
    }
    
  2. Add an additional type-check:

    class AuthSystem
    {
        public function authenticate(User $user)
        {
            if ($user instanceof MyUser) {
                $this->logger->info(/** ... */);
            }
    
            // or alternatively
            if ( ! $user instanceof MyUser) {
                throw new \LogicException(
                    '$user must be an instance of MyUser, '
                   .'other instances are not supported.'
                );
            }
    
        }
    }
    
Note: PHP Analyzer uses reverse abstract interpretation to narrow down the types inside the if block in such a case.
  1. Add the method to the parent class:

    abstract class User
    {
        /** @return string */
        abstract public function getPassword();
    
        /** @return string */
        abstract public function getDisplayName();
    }
    
Loading history...
331
            }
332 View Code Duplication
        } else {
0 ignored issues
show
Duplication introduced by
This code seems to be duplicated across your project.

Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation.

You can also find more detailed suggestions in the “Code” section of your repository.

Loading history...
333
            if (!$this->permissionApi->hasPermission($this->permissionComponent, '::', ACCESS_EDIT)) {
334
                throw new AccessDeniedException();
335
            }
336
337
            $entity = $this->initEntityForCreation();
338
        }
339
340
        // save entity reference for later reuse
341
        $this->entityRef = $entity;
342
343
344
        $actions = $this->workflowHelper->getActionsForObject($entity);
0 ignored issues
show
Bug introduced by
It seems like $entity can be null; however, getActionsForObject() does not accept null, maybe add an additional type check?

Unless you are absolutely sure that the expression can never be null because of other conditions, we strongly recommend to add an additional type check to your code:

/** @return stdClass|null */
function mayReturnNull() { }

function doesNotAcceptNull(stdClass $x) { }

// With potential error.
function withoutCheck() {
    $x = mayReturnNull();
    doesNotAcceptNull($x); // Potential error here.
}

// Safe - Alternative 1
function withCheck1() {
    $x = mayReturnNull();
    if ( ! $x instanceof stdClass) {
        throw new \LogicException('$x must be defined.');
    }
    doesNotAcceptNull($x);
}

// Safe - Alternative 2
function withCheck2() {
    $x = mayReturnNull();
    if ($x instanceof stdClass) {
        doesNotAcceptNull($x);
    }
}
Loading history...
345
        if (false === $actions || !is_array($actions)) {
346
            $this->request->getSession()->getFlashBag()->add('error', $this->__('Error! Could not determine workflow actions.'));
347
            $logArgs = ['app' => 'ZikulaRoutesModule', 'user' => $this->currentUserApi->get('uname'), 'entity' => $this->objectType, 'id' => $entity->createCompositeIdentifier()];
348
            $this->logger->error('{app}: User {user} tried to edit the {entity} with id {id}, but failed to determine available workflow actions.', $logArgs);
349
            throw new \RuntimeException($this->__('Error! Could not determine workflow actions.'));
350
        }
351
352
        $this->templateParameters['actions'] = $actions;
353
354
        $this->form = $this->createForm();
0 ignored issues
show
Bug introduced by
Are you sure the assignment to $this->form is correct as $this->createForm() (which targets Zikula\RoutesModule\Form...itHandler::createForm()) seems to always return null.

This check looks for function or method calls that always return null and whose return value is assigned to a variable.

class A
{
    function getObject()
    {
        return null;
    }

}

$a = new A();
$object = $a->getObject();

The method getObject() can return nothing but null, so it makes no sense to assign that value to a variable.

The reason is most likely that a function or method is imcomplete or has been reduced for debug purposes.

Loading history...
355
        if (!is_object($this->form)) {
356
            return false;
357
        }
358
359
        // handle form request and check validity constraints of edited entity
360
        if ($this->form->handleRequest($this->request) && $this->form->isSubmitted()) {
361
            if ($this->form->isValid()) {
362
                $result = $this->handleCommand();
363
                if (false === $result) {
364
                    $this->templateParameters['form'] = $this->form->createView();
365
                }
366
367
                return $result;
368
            }
369
            if ($this->form->get('cancel')->isClicked()) {
370
                return new RedirectResponse($this->getRedirectUrl(['commandName' => 'cancel']), 302);
0 ignored issues
show
Bug introduced by
It seems like you code against a specific sub-type and not the parent class Zikula\RoutesModule\Form...ase\AbstractEditHandler as the method getRedirectUrl() does only exist in the following sub-classes of Zikula\RoutesModule\Form...ase\AbstractEditHandler: Zikula\RoutesModule\Form...ase\AbstractEditHandler, Zikula\RoutesModule\Form\Handler\Route\EditHandler. Maybe you want to instanceof check for one of these explicitly?

Let’s take a look at an example:

abstract class User
{
    /** @return string */
    abstract public function getPassword();
}

class MyUser extends User
{
    public function getPassword()
    {
        // return something
    }

    public function getDisplayName()
    {
        // return some name.
    }
}

class AuthSystem
{
    public function authenticate(User $user)
    {
        $this->logger->info(sprintf('Authenticating %s.', $user->getDisplayName()));
        // do something.
    }
}

In the above example, the authenticate() method works fine as long as you just pass instances of MyUser. However, if you now also want to pass a different sub-classes of User which does not have a getDisplayName() method, the code will break.

Available Fixes

  1. Change the type-hint for the parameter:

    class AuthSystem
    {
        public function authenticate(MyUser $user) { /* ... */ }
    }
    
  2. Add an additional type-check:

    class AuthSystem
    {
        public function authenticate(User $user)
        {
            if ($user instanceof MyUser) {
                $this->logger->info(/** ... */);
            }
    
            // or alternatively
            if ( ! $user instanceof MyUser) {
                throw new \LogicException(
                    '$user must be an instance of MyUser, '
                   .'other instances are not supported.'
                );
            }
    
        }
    }
    
Note: PHP Analyzer uses reverse abstract interpretation to narrow down the types inside the if block in such a case.
  1. Add the method to the parent class:

    abstract class User
    {
        /** @return string */
        abstract public function getPassword();
    
        /** @return string */
        abstract public function getDisplayName();
    }
    
Loading history...
Security Cross-Site Scripting introduced by
$this->getRedirectUrl(ar...mandName' => 'cancel')) can contain request data and is used in output context(s) leading to a potential security vulnerability.

3 paths for user data to reach this point

  1. Path: HeaderBag::get() returns tainted data, and AbstractEditHandler::$returnTo is assigned in src/system/RoutesModule/Form/Handler/Common/Base/AbstractEditHandler.php on line 296
  1. HeaderBag::get() returns tainted data, and AbstractEditHandler::$returnTo is assigned
    in src/system/RoutesModule/Form/Handler/Common/Base/AbstractEditHandler.php on line 296
  2. Tainted property AbstractEditHandler::$returnTo is read
    in src/system/RoutesModule/Form/Handler/Route/Base/AbstractEditHandler.php on line 121
  3. AbstractEditHandler::getDefaultReturnUrl() returns tainted data
    in src/system/RoutesModule/Form/Handler/Route/Base/AbstractEditHandler.php on line 257
  4. AbstractEditHandler::getRedirectUrl() returns tainted data
    in src/system/RoutesModule/Form/Handler/Common/Base/AbstractEditHandler.php on line 370
  2. Path: ParameterBag::get() returns tainted data, and AbstractEditHandler::$returnTo is assigned in src/system/RoutesModule/Form/Handler/Common/Base/AbstractEditHandler.php on line 290
  1. ParameterBag::get() returns tainted data, and AbstractEditHandler::$returnTo is assigned
    in src/system/RoutesModule/Form/Handler/Common/Base/AbstractEditHandler.php on line 290
  2. Tainted property AbstractEditHandler::$returnTo is read
    in src/system/RoutesModule/Form/Handler/Route/Base/AbstractEditHandler.php on line 121
  3. AbstractEditHandler::getDefaultReturnUrl() returns tainted data
    in src/system/RoutesModule/Form/Handler/Route/Base/AbstractEditHandler.php on line 257
  4. AbstractEditHandler::getRedirectUrl() returns tainted data
    in src/system/RoutesModule/Form/Handler/Common/Base/AbstractEditHandler.php on line 370
  3. Path: Session::get() returns tainted data, and AbstractEditHandler::$returnTo is assigned in src/system/RoutesModule/Form/Handler/Common/Base/AbstractEditHandler.php on line 294
  1. Session::get() returns tainted data, and AbstractEditHandler::$returnTo is assigned
    in src/system/RoutesModule/Form/Handler/Common/Base/AbstractEditHandler.php on line 294
  2. Tainted property AbstractEditHandler::$returnTo is read
    in src/system/RoutesModule/Form/Handler/Route/Base/AbstractEditHandler.php on line 121
  3. AbstractEditHandler::getDefaultReturnUrl() returns tainted data
    in src/system/RoutesModule/Form/Handler/Route/Base/AbstractEditHandler.php on line 257
  4. AbstractEditHandler::getRedirectUrl() returns tainted data
    in src/system/RoutesModule/Form/Handler/Common/Base/AbstractEditHandler.php on line 370

Used in output context

  1. RedirectResponse::__construct() uses RedirectResponse::setTargetUrl() ($url)
    in vendor/src/Symfony/Component/HttpFoundation/RedirectResponse.php on line 39
  2. RedirectResponse::setTargetUrl() uses Response::setContent() ($content)
    in vendor/src/Symfony/Component/HttpFoundation/RedirectResponse.php on line 86
  3. Response::setContent() uses property Response::$content for writing
    in vendor/src/Symfony/Component/HttpFoundation/Response.php on line 447
  4. Property Response::$content is used in echo
    in vendor/src/Symfony/Component/HttpFoundation/Response.php on line 406

Preventing Cross-Site-Scripting Attacks

Cross-Site-Scripting allows an attacker to inject malicious code into your website - in particular Javascript code, and have that code executed with the privileges of a visiting user. This can be used to obtain data, or perform actions on behalf of that visiting user.

In order to prevent this, make sure to escape all user-provided data:

// for HTML
$sanitized = htmlentities($tainted, ENT_QUOTES);

// for URLs
$sanitized = urlencode($tainted);

General Strategies to prevent injection

In general, it is advisable to prevent any user-data to reach this point. This can be done by white-listing certain values:

if ( ! in_array($value, array('this-is-allowed', 'and-this-too'), true)) {
    throw new \InvalidArgumentException('This input is not allowed.');
}

For numeric data, we recommend to explicitly cast the data:

$sanitized = (integer) $tainted;
Loading history...
371
            }
372
        }
373
374
        $this->templateParameters['form'] = $this->form->createView();
375
376
        // everything okay, no initialisation errors occured
377
        return true;
378
    }
379
380
    /**
381
     * Creates the form type.
382
     */
383
    protected function createForm()
384
    {
385
        // to be customised in sub classes
386
        return null;
387
    }
388
389
    /**
390
     * Returns the template parameters.
391
     *
392
     * @return array
393
     */
394
    public function getTemplateParameters()
395
    {
396
        return $this->templateParameters;
397
    }
398
399
    /**
400
     * Create concatenated identifier string (for composite keys).
401
     *
402
     * @return String concatenated identifiers
403
     */
404
    protected function createCompositeIdentifier()
405
    {
406
        $itemId = '';
407
        foreach ($this->idFields as $idField) {
408
            if (!empty($itemId)) {
409
                $itemId .= '_';
410
            }
411
            $itemId .= $this->idValues[$idField];
412
        }
413
414
        return $itemId;
415
    }
416
417
    /**
418
     * Initialise existing entity for editing.
419
     *
420
     * @return EntityAccess|null Desired entity instance or null
421
     *
422
     * @throws NotFoundHttpException Thrown if item to be edited isn't found
423
     */
424
    protected function initEntityForEditing()
425
    {
426
        $entity = $this->selectionHelper->getEntity($this->objectType, $this->idValues);
427
        if (null === $entity) {
428
            throw new NotFoundHttpException($this->__('No such item.'));
429
        }
430
431
        $entity->initWorkflow();
432
433
        return $entity;
434
    }
435
436
    /**
437
     * Initialise new entity for creation.
438
     *
439
     * @return EntityAccess|null Desired entity instance or null
440
     *
441
     * @throws NotFoundHttpException Thrown if item to be cloned isn't found
442
     */
443
    protected function initEntityForCreation()
444
    {
445
        $this->hasTemplateId = false;
446
        $templateId = $this->request->query->get('astemplate', '');
447
        $entity = null;
448
449
        if (!empty($templateId)) {
450
            $templateIdValueParts = explode('_', $templateId);
451
            $this->hasTemplateId = count($templateIdValueParts) == count($this->idFields);
452
453
            if (true === $this->hasTemplateId) {
454
                $templateIdValues = [];
455
                $i = 0;
456
                foreach ($this->idFields as $idField) {
457
                    $templateIdValues[$idField] = $templateIdValueParts[$i];
458
                    $i++;
459
                }
460
                // reuse existing entity
461
                $entityT = $this->selectionHelper->getEntity($this->objectType, $templateIdValues);
462
                if (null === $entityT) {
463
                    throw new NotFoundHttpException($this->__('No such item.'));
464
                }
465
                $entity = clone $entityT;
466
            }
467
        }
468
469
        if (null === $entity) {
470
            $createMethod = 'create' . ucfirst($this->objectType);
471
            $entity = $this->entityFactory->$createMethod();
472
        }
473
474
        return $entity;
475
    }
476
477
    /**
478
     * Get list of allowed redirect codes.
479
     *
480
     * @return array list of possible redirect codes
481
     */
482
    protected function getRedirectCodes()
483
    {
484
        $codes = [];
485
486
        // to be filled by subclasses
487
488
        return $codes;
489
    }
490
491
    /**
492
     * Command event handler.
493
     *
494
     * @param array $args List of arguments
495
     *
496
     * @return mixed Redirect or false on errors
497
     */
498
    public function handleCommand($args = [])
499
    {
500
        // build $args for BC (e.g. used by redirect handling)
501 View Code Duplication
        foreach ($this->templateParameters['actions'] as $action) {
0 ignored issues
show
Duplication introduced by
This code seems to be duplicated across your project.

Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation.

You can also find more detailed suggestions in the “Code” section of your repository.

Loading history...
502
            if ($this->form->get($action['id'])->isClicked()) {
0 ignored issues
show
Bug introduced by
The method get() does not seem to exist on object<Symfony\Component\Form\AbstractType>.

This check looks for calls to methods that do not seem to exist on a given type. It looks for the method on the type itself as well as in inherited classes or implemented interfaces.

This is most likely a typographical error or the method has been renamed.

Loading history...
503
                $args['commandName'] = $action['id'];
504
            }
505
        }
506
        if ($this->form->get('cancel')->isClicked()) {
0 ignored issues
show
Bug introduced by
The method get() does not seem to exist on object<Symfony\Component\Form\AbstractType>.

This check looks for calls to methods that do not seem to exist on a given type. It looks for the method on the type itself as well as in inherited classes or implemented interfaces.

This is most likely a typographical error or the method has been renamed.

Loading history...
507
            $args['commandName'] = 'cancel';
508
        }
509
510
        $action = $args['commandName'];
511
        $isRegularAction = !in_array($action, ['delete', 'cancel']);
512
513
        if ($isRegularAction || $action == 'delete') {
514
            $this->fetchInputData($args);
515
        }
516
517
        // get treated entity reference from persisted member var
518
        $entity = $this->entityRef;
0 ignored issues
show
Unused Code introduced by
$entity is not used, you could remove the assignment.

This check looks for variable assignements that are either overwritten by other assignments or where the variable is not used subsequently.

$myVar = 'Value';
$higher = false;

if (rand(1, 6) > 3) {
    $higher = true;
} else {
    $higher = false;
}

Both the $myVar assignment in line 1 and the $higher assignment in line 2 are dead. The first because $myVar is never used and the second because $higher is always overwritten for every possible time line.

Loading history...
519
520
        if ($isRegularAction || $action == 'delete') {
521
            $success = $this->applyAction($args);
522
            if (!$success) {
523
                // the workflow operation failed
524
                return false;
525
            }
526
        }
527
528
        if (true === $this->hasPageLockSupport && $this->templateParameters['mode'] == 'edit' && $this->kernel->isBundle('ZikulaPageLockModule') && null !== $this->lockingApi) {
529
            $lockName = 'ZikulaRoutesModule' . $this->objectTypeCapital . $this->createCompositeIdentifier();
530
            $this->lockingApi->releaseLock($lockName);
531
        }
532
533
        return new RedirectResponse($this->getRedirectUrl($args), 302);
0 ignored issues
show
Bug introduced by
It seems like you code against a specific sub-type and not the parent class Zikula\RoutesModule\Form...ase\AbstractEditHandler as the method getRedirectUrl() does only exist in the following sub-classes of Zikula\RoutesModule\Form...ase\AbstractEditHandler: Zikula\RoutesModule\Form...ase\AbstractEditHandler, Zikula\RoutesModule\Form\Handler\Route\EditHandler. Maybe you want to instanceof check for one of these explicitly?

Let’s take a look at an example:

abstract class User
{
    /** @return string */
    abstract public function getPassword();
}

class MyUser extends User
{
    public function getPassword()
    {
        // return something
    }

    public function getDisplayName()
    {
        // return some name.
    }
}

class AuthSystem
{
    public function authenticate(User $user)
    {
        $this->logger->info(sprintf('Authenticating %s.', $user->getDisplayName()));
        // do something.
    }
}

In the above example, the authenticate() method works fine as long as you just pass instances of MyUser. However, if you now also want to pass a different sub-classes of User which does not have a getDisplayName() method, the code will break.

Available Fixes

  1. Change the type-hint for the parameter:

    class AuthSystem
    {
        public function authenticate(MyUser $user) { /* ... */ }
    }
    
  2. Add an additional type-check:

    class AuthSystem
    {
        public function authenticate(User $user)
        {
            if ($user instanceof MyUser) {
                $this->logger->info(/** ... */);
            }
    
            // or alternatively
            if ( ! $user instanceof MyUser) {
                throw new \LogicException(
                    '$user must be an instance of MyUser, '
                   .'other instances are not supported.'
                );
            }
    
        }
    }
    
Note: PHP Analyzer uses reverse abstract interpretation to narrow down the types inside the if block in such a case.
  1. Add the method to the parent class:

    abstract class User
    {
        /** @return string */
        abstract public function getPassword();
    
        /** @return string */
        abstract public function getDisplayName();
    }
    
Loading history...
Security Cross-Site Scripting introduced by
$this->getRedirectUrl($args) can contain request data and is used in output context(s) leading to a potential security vulnerability.

3 paths for user data to reach this point

  1. Path: HeaderBag::get() returns tainted data, and AbstractEditHandler::$returnTo is assigned in src/system/RoutesModule/Form/Handler/Common/Base/AbstractEditHandler.php on line 296
  1. HeaderBag::get() returns tainted data, and AbstractEditHandler::$returnTo is assigned
    in src/system/RoutesModule/Form/Handler/Common/Base/AbstractEditHandler.php on line 296
  2. Tainted property AbstractEditHandler::$returnTo is read
    in src/system/RoutesModule/Form/Handler/Route/Base/AbstractEditHandler.php on line 121
  3. AbstractEditHandler::getDefaultReturnUrl() returns tainted data
    in src/system/RoutesModule/Form/Handler/Route/Base/AbstractEditHandler.php on line 257
  4. AbstractEditHandler::getRedirectUrl() returns tainted data
    in src/system/RoutesModule/Form/Handler/Common/Base/AbstractEditHandler.php on line 533
  2. Path: ParameterBag::get() returns tainted data, and AbstractEditHandler::$returnTo is assigned in src/system/RoutesModule/Form/Handler/Common/Base/AbstractEditHandler.php on line 290
  1. ParameterBag::get() returns tainted data, and AbstractEditHandler::$returnTo is assigned
    in src/system/RoutesModule/Form/Handler/Common/Base/AbstractEditHandler.php on line 290
  2. Tainted property AbstractEditHandler::$returnTo is read
    in src/system/RoutesModule/Form/Handler/Route/Base/AbstractEditHandler.php on line 121
  3. AbstractEditHandler::getDefaultReturnUrl() returns tainted data
    in src/system/RoutesModule/Form/Handler/Route/Base/AbstractEditHandler.php on line 257
  4. AbstractEditHandler::getRedirectUrl() returns tainted data
    in src/system/RoutesModule/Form/Handler/Common/Base/AbstractEditHandler.php on line 533
  3. Path: Session::get() returns tainted data, and AbstractEditHandler::$returnTo is assigned in src/system/RoutesModule/Form/Handler/Common/Base/AbstractEditHandler.php on line 294
  1. Session::get() returns tainted data, and AbstractEditHandler::$returnTo is assigned
    in src/system/RoutesModule/Form/Handler/Common/Base/AbstractEditHandler.php on line 294
  2. Tainted property AbstractEditHandler::$returnTo is read
    in src/system/RoutesModule/Form/Handler/Route/Base/AbstractEditHandler.php on line 121
  3. AbstractEditHandler::getDefaultReturnUrl() returns tainted data
    in src/system/RoutesModule/Form/Handler/Route/Base/AbstractEditHandler.php on line 257
  4. AbstractEditHandler::getRedirectUrl() returns tainted data
    in src/system/RoutesModule/Form/Handler/Common/Base/AbstractEditHandler.php on line 533

Used in output context

  1. RedirectResponse::__construct() uses RedirectResponse::setTargetUrl() ($url)
    in vendor/src/Symfony/Component/HttpFoundation/RedirectResponse.php on line 39
  2. RedirectResponse::setTargetUrl() uses Response::setContent() ($content)
    in vendor/src/Symfony/Component/HttpFoundation/RedirectResponse.php on line 86
  3. Response::setContent() uses property Response::$content for writing
    in vendor/src/Symfony/Component/HttpFoundation/Response.php on line 447
  4. Property Response::$content is used in echo
    in vendor/src/Symfony/Component/HttpFoundation/Response.php on line 406

Preventing Cross-Site-Scripting Attacks

Cross-Site-Scripting allows an attacker to inject malicious code into your website - in particular Javascript code, and have that code executed with the privileges of a visiting user. This can be used to obtain data, or perform actions on behalf of that visiting user.

In order to prevent this, make sure to escape all user-provided data:

// for HTML
$sanitized = htmlentities($tainted, ENT_QUOTES);

// for URLs
$sanitized = urlencode($tainted);

General Strategies to prevent injection

In general, it is advisable to prevent any user-data to reach this point. This can be done by white-listing certain values:

if ( ! in_array($value, array('this-is-allowed', 'and-this-too'), true)) {
    throw new \InvalidArgumentException('This input is not allowed.');
}

For numeric data, we recommend to explicitly cast the data:

$sanitized = (integer) $tainted;
Loading history...
534
    }
535
536
    /**
537
     * Get success or error message for default operations.
538
     *
539
     * @param array   $args    arguments from handleCommand method
540
     * @param Boolean $success true if this is a success, false for default error
541
     *
542
     * @return String desired status or error message
543
     */
544
    protected function getDefaultMessage($args, $success = false)
545
    {
546
        $message = '';
547
        switch ($args['commandName']) {
548 View Code Duplication
            case 'create':
0 ignored issues
show
Duplication introduced by
This code seems to be duplicated across your project.

Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation.

You can also find more detailed suggestions in the “Code” section of your repository.

Loading history...
549
                if (true === $success) {
550
                    $message = $this->__('Done! Item created.');
551
                } else {
552
                    $message = $this->__('Error! Creation attempt failed.');
553
                }
554
                break;
555 View Code Duplication
            case 'update':
0 ignored issues
show
Duplication introduced by
This code seems to be duplicated across your project.

Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation.

You can also find more detailed suggestions in the “Code” section of your repository.

Loading history...
556
                if (true === $success) {
557
                    $message = $this->__('Done! Item updated.');
558
                } else {
559
                    $message = $this->__('Error! Update attempt failed.');
560
                }
561
                break;
562 View Code Duplication
            case 'delete':
0 ignored issues
show
Duplication introduced by
This code seems to be duplicated across your project.

Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation.

You can also find more detailed suggestions in the “Code” section of your repository.

Loading history...
563
                if (true === $success) {
564
                    $message = $this->__('Done! Item deleted.');
565
                } else {
566
                    $message = $this->__('Error! Deletion attempt failed.');
567
                }
568
                break;
569
        }
570
571
        return $message;
572
    }
573
574
    /**
575
     * Add success or error message to session.
576
     *
577
     * @param array   $args    arguments from handleCommand method
578
     * @param Boolean $success true if this is a success, false for default error
579
     *
580
     * @throws RuntimeException Thrown if executing the workflow action fails
581
     */
582
    protected function addDefaultMessage($args, $success = false)
583
    {
584
        $message = $this->getDefaultMessage($args, $success);
585
        if (empty($message)) {
586
            return;
587
        }
588
589
        $flashType = true === $success ? 'status' : 'error';
590
        $this->request->getSession()->getFlashBag()->add($flashType, $message);
591
        $logArgs = ['app' => 'ZikulaRoutesModule', 'user' => $this->currentUserApi->get('uname'), 'entity' => $this->objectType, 'id' => $this->entityRef->createCompositeIdentifier()];
0 ignored issues
show
Bug introduced by
It seems like you code against a specific sub-type and not the parent class Zikula\Core\Doctrine\EntityAccess as the method createCompositeIdentifier() does only exist in the following sub-classes of Zikula\Core\Doctrine\EntityAccess: Zikula\RoutesModule\Enti...ase\AbstractRouteEntity, Zikula\RoutesModule\Entity\RouteEntity. Maybe you want to instanceof check for one of these explicitly?

Let’s take a look at an example:

abstract class User
{
    /** @return string */
    abstract public function getPassword();
}

class MyUser extends User
{
    public function getPassword()
    {
        // return something
    }

    public function getDisplayName()
    {
        // return some name.
    }
}

class AuthSystem
{
    public function authenticate(User $user)
    {
        $this->logger->info(sprintf('Authenticating %s.', $user->getDisplayName()));
        // do something.
    }
}

In the above example, the authenticate() method works fine as long as you just pass instances of MyUser. However, if you now also want to pass a different sub-classes of User which does not have a getDisplayName() method, the code will break.

Available Fixes

  1. Change the type-hint for the parameter:

    class AuthSystem
    {
        public function authenticate(MyUser $user) { /* ... */ }
    }
    
  2. Add an additional type-check:

    class AuthSystem
    {
        public function authenticate(User $user)
        {
            if ($user instanceof MyUser) {
                $this->logger->info(/** ... */);
            }
    
            // or alternatively
            if ( ! $user instanceof MyUser) {
                throw new \LogicException(
                    '$user must be an instance of MyUser, '
                   .'other instances are not supported.'
                );
            }
    
        }
    }
    
Note: PHP Analyzer uses reverse abstract interpretation to narrow down the types inside the if block in such a case.
  1. Add the method to the parent class:

    abstract class User
    {
        /** @return string */
        abstract public function getPassword();
    
        /** @return string */
        abstract public function getDisplayName();
    }
    
Loading history...
592
        if (true === $success) {
593
            $this->logger->notice('{app}: User {user} updated the {entity} with id {id}.', $logArgs);
594
        } else {
595
            $this->logger->error('{app}: User {user} tried to update the {entity} with id {id}, but failed.', $logArgs);
596
        }
597
    }
598
599
    /**
600
     * Input data processing called by handleCommand method.
601
     *
602
     * @param array $args Additional arguments
603
     */
604
    public function fetchInputData($args)
0 ignored issues
show
Unused Code introduced by
The parameter $args is not used and could be removed.

This check looks from parameters that have been defined for a function or method, but which are not used in the method body.

Loading history...
605
    {
606
        // fetch posted data input values as an associative array
607
        $formData = $this->form->getData();
0 ignored issues
show
Bug introduced by
The method getData() does not seem to exist on object<Symfony\Component\Form\AbstractType>.

This check looks for calls to methods that do not seem to exist on a given type. It looks for the method on the type itself as well as in inherited classes or implemented interfaces.

This is most likely a typographical error or the method has been renamed.

Loading history...
608
609
        if ($this->templateParameters['mode'] == 'create' && isset($this->form['repeatCreation']) && $this->form['repeatCreation']->getData() == 1) {
610
            $this->repeatCreateAction = true;
611
        }
612
613
        if (isset($this->form['additionalNotificationRemarks']) && $this->form['additionalNotificationRemarks']->getData() != '') {
614
            $this->request->getSession()->set('ZikulaRoutesModuleAdditionalNotificationRemarks', $this->form['additionalNotificationRemarks']->getData());
615
        }
616
617
        // return remaining form data
618
        return $formData;
619
    }
620
621
    /**
622
     * This method executes a certain workflow action.
623
     *
624
     * @param array $args Arguments from handleCommand method
625
     *
626
     * @return bool Whether everything worked well or not
627
     */
628
    public function applyAction(array $args = [])
629
    {
630
        // stub for subclasses
631
        return false;
632
    }
633
634
    /**
635
     * Sets optional locking api reference.
636
     *
637
     * @param LockingApi $lockingApi
638
     */
639
    public function setLockingApi(LockingApi $lockingApi)
640
    {
641
        $this->lockingApi = $lockingApi;
642
    }
643
}
644