FrankProjects / UltimateWarfare

Ultimate Warfare is an online multiplayer browser based strategy game written in PHP 8.3 and Symfony 7
https://ultimate-warfare.com
MIT License
32 stars 13 forks source link

Input validation bug: Must be of type string, null given #171

Open frank9999 opened 1 year ago

frank9999 commented 1 year ago

Several TypeErrors have been reported in server log. I'm able to reproduce these errors by using spaces as input. Several forms impacted, tested and reproduced on both contact form and registration form.

{
  "message": "Uncaught PHP Exception Symfony\\Component\\PropertyAccess\\Exception\\InvalidArgumentException: \"Expected argument of type \"string\", \"null\" given at property path \"name\".\" at /var/www/prod.ultimate-warfare.com/releases/11/vendor/symfony/property-access/PropertyAccessor.php line 211",
  "context": {
    "exception": {
      "class": "Symfony\\Component\\PropertyAccess\\Exception\\InvalidArgumentException",
      "message": "Expected argument of type \"string\", \"null\" given at property path \"name\".",
      "code": 0,
      "file": "/var/www/prod.ultimate-warfare.com/releases/11/vendor/symfony/property-access/PropertyAccessor.php:211",
      "previous": {
        "class": "TypeError",
        "message": "FrankProjects\\UltimateWarfare\\Entity\\Contact::setName(): Argument #1 ($name) must be of type string, null given, called in /var/www/prod.ultimate-warfare.com/releases/11/vendor/symfony/property-access/PropertyAccessor.php on line 531",
        "code": 0,
        "file": "/var/www/prod.ultimate-warfare.com/releases/11/src/Entity/Contact.php:39"
      }
    }
  },
  "level": 500,
  "level_name": "CRITICAL",
  "channel": "request",
  "datetime": "",
  "extra": {}
}
Schnoop commented 1 year ago

It's no just if you enter spaces - it's also a problem if a browser does not respect the HTML5 validation. In fact there is no server-side validation of any user input on any form.

Schnoop commented 1 year ago

There are tow options to "fix" this problem:

a) Make the property nullable in Contact model. This will break the clean code which you try to archive via PHPStan. b) Force the form to send an empty string as default value if no value has been entered. (empty_data in form)

Additionally the server side validation should be added. https://symfony.com/doc/current/validation.html#constraints-in-form-classes

frank9999 commented 1 year ago

Good catch, not sure how I missed the form validation. I'll put it on my list to add proper server side validation to all forms :).

And I prefer option B, empty string as default if no value has been entered. I don't like to make the datamodel less strict by allowing null, as that should never be an accepted value. More strict and clean is always better

Zanaras commented 6 months ago

So I kind of stumbled into your project while looking up other Symfony browser games on GitHub, and thought I'd drop one fo the ways my project fixed this, which follows the Symfony guides from v5 (confirmed to work with v6), using your registration form as an example:

# src/Form/RegistrationType.php

use Symfony\Component\Validator\Constraints\Length;

...

    public function buildForm(FormBuilderInterface $builder, array $options): void
    {
        $builder
            ->add(
                'email',
                EmailType::class,
                [
                    'label' => 'label.email'
                ]
            )
            ->add(
                'username',
                TextType::class,
                [
                    'label' => 'label.username'
                ]
            )
            ->add(
                'plainPassword',
                RepeatedType::class,
                [
                    'type' => PasswordType::class,
                    'first_options' => [
                        'label' => 'label.password'
                    ],
                    'second_options' => [
                        'label' => 'label.password_repeat'
                    ],
                    'constraints' => [
                        new Length([
                             'min' => 8,
                             'minMessage' => 'password.must.be.eight.char.min',
                             'max' => 4096,
                        ]),
                    ],
                ]
            )
            ->add(
                'agreeTerms',
                CheckboxType::class,
                [
                    'mapped' => false,
                    'label' => 'label.accept_rules'
                ]
            )
            ->add(
                'captcha',
                ReCaptchaType::class,
                [
                    'mapped' => false,
                    'type' => 'checkbox' // (invisible, checkbox)
                ]
            )
            ->add(
                'submit',
                SubmitType::class,
                [
                    'label' => 'label.register'
                ]
            );
    }

This would be validated by the $form->isValid() check automatically, and limit your efforts to just the forms, and not also the controllers.

Max length is 4096 per https://symfony.com/doc/current/security/passwords.html#creating-a-custom-password-hasher