Change this code to not construct class or method names directly from user-controlled data

[epeliberty (Ellia Passinge)]

Hello,

I have twice error on my controller : Change this code to not construct class or method names directly from user-controlled data.

We get field by $request->get('field') before create getter and setter dynamicly.

However, for fix it, i add this code

private array $fieldsAllowed = [
    'active',
    'agemax',
    'agemin',
    ...
];


Webmozart\Assert\Assert::inArray($request->get('field'), $this->fieldsAllowed);

With my code, I assume and list the field names that are allowed, but I still get an error…
My code is in my main branch, and I don’t understand why I still get this error.

Does anyone have any ideas?

Thank you very much.

[Colin]

Hey there!

Thanks for the report.

Would mind sharing the complete file to reproduce the issue, or a minimal reproducer? I tried on my end but can’t get the vulnerability to raise based on your code sample.

And just to confirm – you’re using SonarQube Cloud, that’s correct?

[epeliberty (Ellia Passinge)]

Hello,

Here is the cleaned file to reproduce the false positive and i confirm that we use Sonar Qube Cloud.

<?php

namespace App\Controller;

use App\Exception\EntityClassNotFoundException;
use App\Exception\MissingGetterMethodException;
use App\Exception\MissingSetterMethodException;
use Doctrine\ORM\EntityNotFoundException;
use Symfony\Component\HttpFoundation\JsonResponse;
use Symfony\Component\HttpFoundation\Request;
use Webmozart\Assert\Assert;

class DefaultController extends BaseController
{
    private array $fieldsAllowed = [
        'active',
        'agemax',
        'agemin',
        'city',
        'clientname',
        'country',
        'currency',
        'depotpossible',
        'email',
        'externalid',
        'externalversion',
        'firstname',
        'isadditionnalonly',
        'isinsurance',
        'issupport',
        'label',
        'lastname',
        'maxage',
        'minage',
        'name',
        'photorequired',
        'predatingpossible',
        'reservedforcontract',
        'shortname',
        'username',
        'zipcode',
    ];

    public function celleditAction(Request $request): JsonResponse
    {
        try {
            $newValue   = $request->get('newvalue');
            $entityName = ucfirst($request->get('entity'));

            Assert::inArray($request->get('field'), $this->fieldsAllowed);

            $field = ucfirst($request->get('field'));
            $id    = $request->get('id');

            if (empty($entityName) || empty($field) || null === $id) {
                throw new \InvalidArgumentException('Entity, field, and ID must be provided.');
            }

            if (\is_string($newValue)) {
                $newValue = match (strtolower($newValue)) {
                    'true'  => true,
                    'false' => false,
                    default => $newValue, // return original if not 'true' or 'false'
                };
            }

            $entityNamespacedName = \sprintf('App\Entity\%s', $entityName);

            if (!class_exists($entityNamespacedName)) {
                throw new EntityClassNotFoundException(\sprintf('Entity class %s does not exist.', $entityNamespacedName));
            }

            $entity = $this->getRepository($entityNamespacedName)->find($id);

            if (!$entity) {
                throw new EntityNotFoundException(\sprintf('Entity %s wit ID %s not found.', $entityName, $id));
            }

            $setter = 'set'.$field;
            $getter = 'get'.$field;

            if (!method_exists($entity, $setter)) {
                throw new MissingSetterMethodException(\sprintf('Setter method %s does not exist on entity %s.', $setter, $entityName));
            }

            $entity->$setter($newValue);

            $this->persist($entity);
            $this->flush();

            if (!method_exists($entity, $getter)) {
                throw new MissingGetterMethodException(\sprintf('Getter method %s does not exist on entity %s.', $getter, $entityName));
            }

            return new JsonResponse($entity->$getter());
        } catch (\Exception $e) {
            return new JsonResponse(['error' => $e->getMessage()], 400);
        }
    }
}

This call works

curl 'https://project.epassinge.com/admin/celledit/' \
  -H 'Accept: text/html, */*; q=0.01' \
  -H 'Accept-Language: fr-FR,fr;q=0.9,en-US;q=0.8,en;q=0.7' \
  -H 'Connection: keep-alive' \
  -H 'Content-Type: application/x-www-form-urlencoded; charset=UTF-8' \
  -H 'Origin: https://project.epassinge.com' \
  -H 'Referer: https://project.epassinge.com/admin/catalogs' \
  --data-raw 'newvalue=false&field=active&id=22596&entity=catalog'

however this call throws an exception

curl 'https://project.epassinge.com/admin/celledit/' \
  -H 'Accept: text/html, */*; q=0.01' \
  -H 'Accept-Language: fr-FR,fr;q=0.9,en-US;q=0.8,en;q=0.7' \
  -H 'Connection: keep-alive' \
  -H 'Content-Type: application/x-www-form-urlencoded; charset=UTF-8' \
  -H 'Origin: https://project.epassinge.com' \
  -H 'Referer: https://project.epassinge.com/admin/catalogs' \
  --data-raw 'newvalue=true&field=visibility&id=22596&entity=catalog'

The expected behavior is therefore correct, and it is no longer possible to have unauthorized getters or setters. With 25 fields available for approximately 8 differents entities, I couldn’t see myself doing a mapping per entity

Thanks

[Loris (Loris Sierra)]

Hi @epeliberty, perfect. thanks a lot for your work, this is great. I’m sincerely hyper grateful because we rarely get feedback for this rule.

I am adding this to my current batch of investigations. I’ll come back to you once I am done, with an explanation.

Cheers!

Loris

(And welcome to these forums!)

[epeliberty (Ellia Passinge)]

Hello,

Thank you very much
I’ll keep an eye out, as the error may also be on my end

Have a nice day.

[Loris (Loris Sierra)]

Hi Ellia, just a small status update to tell you that I did not blackhole this, this is still in the works

[epeliberty (Ellia Passinge)]

Hello Loris,

No worries

[Loris (Loris Sierra)]

I thought it would not be a low hanging fruit to investigate this, but I have to say your reproducer is actually great.

It’s rare enough for us to get “actually self-contained” pieces of code that it needs to be acknowledged !

I managed to solve part of the issue, but unfortunately the second part is not a low-hanging fruit, so we are going to need a bit more time (And I cannot throw my team under the bus by committing on an ETA)

In the meantime, you can totally close these SQCloud issues as false positives !

Thanks a lot for the work you put into your feedback!!

Regards,

Loris