Default collation of column 'selector' in table reset_password_request (ResetPasswordRequestTrait::selector) ignored letter case

[gorenstein]

What

By default the column 'selector' created with collation '*_ci', for example 'utf8mb4_unicode_ci'.
This leads to a decrease in the code's resistance to brute-force attacks.

Why

Collation '*_ci': this causes the differences in uppercase and lowercase letters to be ignored when fetching from the database by this field.

What to do

Explicitly set collation without ignoring letter case for ResetPasswordRequestTrait::selector.

[bocharsky-bw]

Hey @gorenstein ,

You're right, good catch! I wonder if we explicitly set collation on ResetPasswordRequestTrait::selector - will it cause any potential problems for some users/systems? What collation would you recommend in this case? I can think of utf8mb4_bin, IIRC it should be case-sensitive, any thoughts about it?

Another potential fix I may think about is to add an extra check in the PHP code after the ResetPasswordRequestTrait entity is found - we can check for the selector to make sure they're matched with the case-sensitive strategy.

Any thoughts?

[gorenstein]

Hi @bocharsky-bw,

explicitly set collation on ResetPasswordRequestTrait::selector - will it cause any potential problems for some users/systems?

• after updating dependencies, need to be proactive make:migartion
• (unlikely event) for a system with table larger than the available RAM for the database, it will take a long time to rebuild the index (possible downtime)
• this will work with MySQL and MariaDB, but I have no experience how to doctrine a map collation for PostgresSQL, for example.

I can think of utf8mb4_bin

• I think this is good enough for MySQL and MariaDB (no experience for other DBs/Doctrine)

an extra check in the PHP code after the ResetPasswordRequestTrait entity is found

• I'm sure this will work either way
• and won't require any additional action after the dependencies are updated
• the developer still has the option to force collation, doctrine allows the developer to overwrite entity mapping data for extended BaseEntrty, I haven't checked if this works with trait. If it does, the documentation should mention it and give syntax examples.

[gorenstein]

Hi @bocharsky-bw,
I checked, overriding Field Mappings works with Trait as well:

<?php declare(strict_types=1);

namespace App\Entity;

use App\Repository\ResetPasswordRequestRepository;
use DateTimeInterface;
use Doctrine\DBAL\Types\Types;
use Doctrine\ORM\Mapping as ORM;

use Doctrine\ORM\Mapping\AttributeOverride;
use Doctrine\ORM\Mapping\Column;
use SymfonyCasts\Bundle\ResetPassword\Model\ResetPasswordRequestInterface;
use SymfonyCasts\Bundle\ResetPassword\Model\ResetPasswordRequestTrait;

#[ORM\Entity(repositoryClass: ResetPasswordRequestRepository::class)]
#[ORM\AttributeOverrides([
        new AttributeOverride(
            'selector',
            new Column(type: Types::STRING, length: 20, options: ['collation' => 'utf8_bin']))]
)]
class ResetPasswordRequest implements ResetPasswordRequestInterface
{
    use ResetPasswordRequestTrait;
...
}

bin/console make:migration

<?php

declare(strict_types=1);

namespace DoctrineMigrations;

use Doctrine\DBAL\Schema\Schema;
use Doctrine\Migrations\AbstractMigration;

/**
 * Auto-generated Migration: Please modify to your needs!
 */
final class Version20241003174228 extends AbstractMigration
{
    public function getDescription(): string
    {
        return '';
    }

    public function up(Schema $schema): void
    {
        // this up() migration is auto-generated, please modify it to your needs
        $this->addSql('ALTER TABLE reset_password_request CHANGE selector selector VARCHAR(20) NOT NULL COLLATE `utf8_bin`');
    }

    public function down(Schema $schema): void
    {
        // this down() migration is auto-generated, please modify it to your needs
        $this->addSql('ALTER TABLE reset_password_request CHANGE selector selector VARCHAR(20) NOT NULL');
    }
}

[bocharsky-bw]

Thanks for doing more research on it! Both solutions have pros and cons, I'm still not sure if we should force people to migrate with changing the DB collation. I'm 👍 for mentioning it in the docs at least. Probably we could start with the changes in PHP code to double-check that selector is case-sensitive.