RememberMe Mapper must be refactored to avoid sql injections problem

[peuh]

Hi,

Concatenating strings to build a query is dangerous! The user could use it's cookie value to inject malicious code.

In Mapper\RememberMe:

public function removeAll($userId)
{
    $dql = sprintf("DELETE %s u WHERE u.user_id = %s", $this->options->getRememberMeEntityClass(), $userId);
    $query = $this->em->createQuery($dql);
    $query->getResult();
}

public function removeSerie($userId, $serieId)
{
    $dql = sprintf("DELETE %s u WHERE u.user_id = %s AND u.sid = '%s'", $this->options->getRememberMeEntityClass(), $userId, $serieId);
    $query = $this->em->createQuery($dql);
    $query->getResult();
}

Should be replaced by :

public function removeAll($userId)
{
    $er = $this->em->getRepository($this->options->getRememberMeEntityClass());
    return $er->deleteByUid($userId);
}

public function removeSerie($userId, $serieId)
{
    $er = $this->em->getRepository($this->options->getRememberMeEntityClass());
    return $er->deleteByUidAndSid($userId,$serieId);
}

Then in our entity repository we add :

public function deleteByUid($uid)
{
    $qb = $this->getEntityManager()->createQueryBuilder();
    $qb->delete('RememberMe','r')
        ->where('r.uid = :uid')
        ->setParameter('uid', $uid);
    return $qb->getQuery()->getSingleScalarResult();
}

public function deleteByUidAndSid($uid,$sid)
{
    $qb = $this->getEntityManager()->createQueryBuilder();
    $qb->delete('RememberMe','r')
    ->where('r.uid = :uid')
    ->andWhere('r.sid = :sid')
    ->setParameter('uid', $uid)
    ->setParameter('sid', $sid);
    return $qb->getQuery()->getSingleScalarResult();
}

The main advantage of moving the query building work to the repository is that it makes the mapper db-agnostic as it relies on the entity repository for every call. Not just for the select calls, as it is today in your version. It's more coherent.

[pdobrigkeit]

I made a new commit. But once again, I do not use this module myself, so I am not able to test it right away. I would need some time to build a complete application to be able to test this out, but I believe I made the required changes. If you could verify?

[peuh]

Hi @pdobrigkeit , I've looked at the changes and it looks good for me. I do not use the module myself either. I've just integrated some pieces of the logic in my application.
I think we can now close this issue.