Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
frank9999 opened this issue Sep 26, 2023 · 4 comments
Open

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

frank9999 opened this issue Sep 26, 2023 · 4 comments
Labels
bug Something isn't working Priority: Low Low priority issue

Comments

@frank9999
Copy link
Member

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": {}
}
@frank9999 frank9999 added bug Something isn't working Priority: Low Low priority issue labels Sep 26, 2023
@Schnoop
Copy link
Contributor

Schnoop commented Sep 26, 2023

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
Copy link
Contributor

Schnoop commented Sep 26, 2023

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
Copy link
Member Author

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
Copy link

Zanaras commented Apr 24, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Priority: Low Low priority issue
Projects
None yet
Development

No branches or pull requests

3 participants