[Form] UrlType should not add protocol to emails

symfony · GromNaN · Oct 25, 2021 · Oct 28, 2021 · Oct 28, 2021 · Oct 25, 2021
commit 320c757aeb4e8fbc2ec3aab865c40519a513682b
diff --git a/UPGRADE-5.4.md b/UPGRADE-5.4.md
@@ -22,6 +22,7 @@ Form
 ------

 * Deprecate calling `FormErrorIterator::children()` if the current element is not iterable.
+ * Add `'default_protocol_skip_email' => true` to `UrlType` options.

 FrameworkBundle
 ---------------

diff --git a/src/Symfony/Bridge/Twig/Tests/Extension/AbstractBootstrap3LayoutTest.php b/src/Symfony/Bridge/Twig/Tests/Extension/AbstractBootstrap3LayoutTest.php
@@ -2648,7 +2648,7 @@ public function testTimezoneWithPlaceholder()
    public function testUrlWithDefaultProtocol()
    {
        $url = 'http://www.example.com?foo1=bar1&foo2=bar2';
-        $form = $this->factory->createNamed('name', 'Symfony\Component\Form\Extension\Core\Type\UrlType', $url, ['default_protocol' => 'http']);
+        $form = $this->factory->createNamed('name', 'Symfony\Component\Form\Extension\Core\Type\UrlType', $url, ['default_protocol' => 'http', 'default_protocol_skip_email' => true]);

        $this->assertWidgetMatchesXpath($form->createView(), ['attr' => ['class' => 'my&class']],
 '/input
@@ -2664,7 +2664,7 @@ public function testUrlWithDefaultProtocol()
    public function testUrlWithoutDefaultProtocol()
    {
        $url = 'http://www.example.com?foo1=bar1&foo2=bar2';
-        $form = $this->factory->createNamed('name', 'Symfony\Component\Form\Extension\Core\Type\UrlType', $url, ['default_protocol' => null]);
+        $form = $this->factory->createNamed('name', 'Symfony\Component\Form\Extension\Core\Type\UrlType', $url, ['default_protocol' => null, 'default_protocol_skip_email' => true]);

        $this->assertWidgetMatchesXpath($form->createView(), ['attr' => ['class' => 'my&class']],
            '/input

@@ -7,6 +7,7 @@ CHANGELOG
 * Deprecate calling `FormErrorIterator::children()` if the current element is not iterable.
 * Allow to pass `TranslatableMessage` objects to the `help` option
 * Add the `EnumType`
+ * Deprecate usage of `UrlType` without option `'default_protocol_skip_email' => true`, added to prevent emails from being converted to valid URLs.

 5.3
 ---

@@ -23,6 +23,7 @@
 class FixUrlProtocolListener implements EventSubscriberInterface
 {
    private $defaultProtocol;
+    private $skipEmail = false;

    /**
     * @param string|null $defaultProtocol The URL scheme to add when there is none or null to not modify the data
@@ -32,11 +33,25 @@ public function __construct(?string $defaultProtocol = 'http')
        $this->defaultProtocol = $defaultProtocol;
    }

+    /**
+     * @param bool $skipEmail the URL scheme is not added to values that match an email pattern
+     */
+    public function skipEmail(): void
+    {
+        $this->skipEmail = true;
+    }
+
    public function onSubmit(FormEvent $event)
    {
        $data = $event->getData();

        if ($this->defaultProtocol && $data && \is_string($data) && !preg_match('~^[\w+.-]+://~', $data)) {
+            if (preg_match('~^[^:/]+@[A-Za-z0-9-.]+\.[A-Za-z0-9]+$~', $data)) {
+                if ($this->skipEmail) {
+                    return;
+                }
+                trigger_deprecation('symfony/form', '5.4', 'Class "%s", will add a scheme to urls that looks like emails in 6.0. Call "setIgnoreEmail(true)"', __CLASS__);
+            }
            $event->setData($this->defaultProtocol.'://'.$data);
        }
    }

diff --git a/src/Symfony/Component/Form/Extension/Core/Type/UrlType.php b/src/Symfony/Component/Form/Extension/Core/Type/UrlType.php
@@ -27,7 +27,13 @@ class UrlType extends AbstractType
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        if (null !== $options['default_protocol']) {
-            $builder->addEventSubscriber(new FixUrlProtocolListener($options['default_protocol']));
+            $subscriber = new FixUrlProtocolListener($options['default_protocol']);
+            if ($options['default_protocol_skip_email']) {
+                $subscriber->skipEmail();
+            } else {
+                trigger_deprecation('symfony/form', '5.4', 'Type "%s" option "default_protocol_skip_email" will be "true" in 6.0.', static::class);
+            }
+            $builder->addEventSubscriber($subscriber);
        }
    }

@@ -54,9 +60,11 @@ public function configureOptions(OptionsResolver $resolver)
                    ? $previousValue
                    : 'Please enter a valid URL.';
            },
+            'default_protocol_skip_email' => false,
        ]);

        $resolver->setAllowedTypes('default_protocol', ['null', 'string']);
+        $resolver->setAllowedTypes('default_protocol_skip_email', 'bool');
    }

    /**

diff --git a/src/Symfony/Component/Form/Tests/AbstractLayoutTest.php b/src/Symfony/Component/Form/Tests/AbstractLayoutTest.php
@@ -2296,7 +2296,7 @@ public function testTimezoneWithPlaceholder()
    public function testUrlWithDefaultProtocol()
    {
        $url = 'http://www.example.com?foo1=bar1&foo2=bar2';
-        $form = $this->factory->createNamed('name', 'Symfony\Component\Form\Extension\Core\Type\UrlType', $url, ['default_protocol' => 'http']);
+        $form = $this->factory->createNamed('name', 'Symfony\Component\Form\Extension\Core\Type\UrlType', $url, ['default_protocol' => 'http', 'default_protocol_skip_email' => true]);

        $this->assertWidgetMatchesXpath($form->createView(), [],
 '/input
@@ -2311,7 +2311,7 @@ public function testUrlWithDefaultProtocol()
    public function testUrlWithoutDefaultProtocol()
    {
        $url = 'http://www.example.com?foo1=bar1&foo2=bar2';
-        $form = $this->factory->createNamed('name', 'Symfony\Component\Form\Extension\Core\Type\UrlType', $url, ['default_protocol' => null]);
+        $form = $this->factory->createNamed('name', 'Symfony\Component\Form\Extension\Core\Type\UrlType', $url, ['default_protocol' => null, 'default_protocol_skip_email' => true]);

        $this->assertWidgetMatchesXpath($form->createView(), [],
 '/input

@@ -12,6 +12,7 @@
 namespace Symfony\Component\Form\Tests\Extension\Core\EventListener;

 use PHPUnit\Framework\TestCase;
+use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait;
 use Symfony\Component\EventDispatcher\EventDispatcher;
 use Symfony\Component\Form\Extension\Core\EventListener\FixUrlProtocolListener;
 use Symfony\Component\Form\Form;
@@ -20,25 +21,16 @@

 class FixUrlProtocolListenerTest extends TestCase
 {
+    use ExpectDeprecationTrait;
+
    public function testFixHttpUrl()
    {
        $data = 'www.symfony.com';
        $form = new Form(new FormConfigBuilder('name', null, new EventDispatcher()));
        $event = new FormEvent($form, $data);

        $filter = new FixUrlProtocolListener('http');
-        $filter->onSubmit($event);
-
-        $this->assertEquals('http://www.symfony.com', $event->getData());
-    }
-
-    public function testSkipKnownUrl()
-    {
-        $data = 'http://www.symfony.com';
-        $form = new Form(new FormConfigBuilder('name', null, new EventDispatcher()));
-        $event = new FormEvent($form, $data);
-
-        $filter = new FixUrlProtocolListener('http');
+        $filter->skipEmail();
        $filter->onSubmit($event);

        $this->assertEquals('http://www.symfony.com', $event->getData());
@@ -47,11 +39,14 @@ public function testSkipKnownUrl()
    public function provideUrlsWithSupportedProtocols()
    {
        return [
+            ['http://www.symfony.com'],
            ['ftp://www.symfony.com'],
            ['chrome-extension://foo'],
            ['h323://foo'],
            ['iris.beep://foo'],
            ['foo+bar://foo'],
+            ['fabien@symfony.com'],
+            ['Contact+42@subdomain.example.com'],
        ];
    }

@@ -64,8 +59,26 @@ public function testSkipOtherProtocol($url)
        $event = new FormEvent($form, $url);

        $filter = new FixUrlProtocolListener('http');
+        $filter->skipEmail();
        $filter->onSubmit($event);

        $this->assertEquals($url, $event->getData());
    }
+
+    /**
+     * @group legacy
+     */
+    public function testDeprecatedFixEmail()
+    {
+        $this->expectDeprecation('Since symfony/form 5.4: Class "Symfony\Component\Form\Extension\Core\EventListener\FixUrlProtocolListener", will add a scheme to urls that looks like emails in 6.0. Call "setIgnoreEmail(true)"');
+
+        $data = 'fabien@symfony.com';
+        $form = new Form(new FormConfigBuilder('name', null, new EventDispatcher()));
+        $event = new FormEvent($form, $data);
+
+        $filter = new FixUrlProtocolListener('http');
+        $filter->onSubmit($event);
+
+        $this->assertEquals('http://fabien@symfony.com', $event->getData());
+    }
 }
@@ -22,9 +22,9 @@ public function testSubmitNull($expected = null, $norm = null, $view = null)

    public function testSubmitNullReturnsNullWithEmptyDataAsString()
    {
-        $form = $this->factory->create(static::TESTED_TYPE, 'name', [
+        $form = $this->factory->create(static::TESTED_TYPE, 'name', array_merge($this->getTestOptions(), [
            'empty_data' => '',
-        ]);
+        ]));

        $form->submit(null);
        $this->assertSame('', $form->getData());
@@ -48,9 +48,9 @@ public function provideZeros()
     */
    public function testSetDataThroughParamsWithZero($data, $dataAsString)
    {
-        $form = $this->factory->create(static::TESTED_TYPE, null, [
+        $form = $this->factory->create(static::TESTED_TYPE, null, array_merge($this->getTestOptions(), [
            'data' => $data,
-        ]);
+        ]));
        $view = $form->createView();

        $this->assertFalse($form->isEmpty());

diff --git a/src/Symfony/Component/Form/Tests/Extension/Core/Type/UrlTypeLegacyTest.php b/src/Symfony/Component/Form/Tests/Extension/Core/Type/UrlTypeLegacyTest.php
@@ -0,0 +1,36 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Form\Tests\Extension\Core\Type;
+
+/**
+ * @group legacy
+ */
+class UrlTypeLegacyTest extends UrlTypeTest
+{
+    /**
+     * Legacy behavior. Replace test in parent class.
+     */
+    public function testSubmitAddsNoDefaultProtocolToEmail()
+    {
+        $form = $this->factory->create(static::TESTED_TYPE, 'name', $this->getTestOptions());
+
+        $form->submit('contact@domain.com');
+
+        $this->assertSame('http://contact@domain.com', $form->getData());
+        $this->assertSame('http://contact@domain.com', $form->getViewData());
+    }
+
+    protected function getTestOptions(): array
+    {
+        return [];
+    }
+}
@@ -19,19 +19,29 @@ class UrlTypeTest extends TextTypeTest

    public function testSubmitAddsDefaultProtocolIfNoneIsIncluded()
    {
-        $form = $this->factory->create(static::TESTED_TYPE, 'name');
+        $form = $this->factory->create(static::TESTED_TYPE, 'name', $this->getTestOptions());

        $form->submit('www.domain.com');

        $this->assertSame('http://www.domain.com', $form->getData());
        $this->assertSame('http://www.domain.com', $form->getViewData());
    }

+    public function testSubmitAddsNoDefaultProtocolToEmail()
+    {
+        $form = $this->factory->create(static::TESTED_TYPE, 'name', $this->getTestOptions());
+
+        $form->submit('contact@domain.com');
+
+        $this->assertSame('contact@domain.com', $form->getData());
+        $this->assertSame('contact@domain.com', $form->getViewData());
+    }
+
    public function testSubmitAddsNoDefaultProtocolIfAlreadyIncluded()
    {
-        $form = $this->factory->create(static::TESTED_TYPE, null, [
+        $form = $this->factory->create(static::TESTED_TYPE, null, array_merge($this->getTestOptions(), [
            'default_protocol' => 'http',
-        ]);
+        ]));

        $form->submit('ftp://www.domain.com');

@@ -41,9 +51,9 @@ public function testSubmitAddsNoDefaultProtocolIfAlreadyIncluded()

    public function testSubmitAddsNoDefaultProtocolIfEmpty()
    {
-        $form = $this->factory->create(static::TESTED_TYPE, null, [
+        $form = $this->factory->create(static::TESTED_TYPE, null, array_merge($this->getTestOptions(), [
            'default_protocol' => 'http',
-        ]);
+        ]));

        $form->submit('');

@@ -53,9 +63,9 @@ public function testSubmitAddsNoDefaultProtocolIfEmpty()

    public function testSubmitAddsNoDefaultProtocolIfNull()
    {
-        $form = $this->factory->create(static::TESTED_TYPE, null, [
+        $form = $this->factory->create(static::TESTED_TYPE, null, array_merge($this->getTestOptions(), [
            'default_protocol' => 'http',
-        ]);
+        ]));

        $form->submit(null);

@@ -65,9 +75,9 @@ public function testSubmitAddsNoDefaultProtocolIfNull()

    public function testSubmitAddsNoDefaultProtocolIfSetToNull()
    {
-        $form = $this->factory->create(static::TESTED_TYPE, null, [
+        $form = $this->factory->create(static::TESTED_TYPE, null, array_merge($this->getTestOptions(), [
            'default_protocol' => null,
-        ]);
+        ]));

        $form->submit('www.domain.com');

@@ -85,14 +95,21 @@ public function testThrowExceptionIfDefaultProtocolIsInvalid()

    public function testSubmitNullUsesDefaultEmptyData($emptyData = 'empty', $expectedData = 'http://empty')
    {
-        $form = $this->factory->create(static::TESTED_TYPE, null, [
+        $form = $this->factory->create(static::TESTED_TYPE, null, array_merge($this->getTestOptions(), [
            'empty_data' => $emptyData,
-        ]);
+        ]));
        $form->submit(null);

        // listener normalizes data on submit
        $this->assertSame($expectedData, $form->getViewData());
        $this->assertSame($expectedData, $form->getNormData());
        $this->assertSame($expectedData, $form->getData());
    }
+
+    protected function getTestOptions(): array
+    {
+        return [
+            'default_protocol_skip_email' => true,
+        ];
+    }
 }
-Original file line number
+Diff line change
@@ -23,6 +23,7 @@
     class FixUrlProtocolListener implements EventSubscriberInterface
     {
         private $defaultProtocol;
+        private $skipEmail = false;
         /**
          * @param string|null $defaultProtocol The URL scheme to add when there is none or null to not modify the data
@@ -32,11 +33,25 @@ public function __construct(?string $defaultProtocol = 'http')
             $this->defaultProtocol = $defaultProtocol;
         }
+        /**
+         * @param bool $skipEmail the URL scheme is not added to values that match an email pattern
+         */
+        public function skipEmail(): void
             Copy link

  
      
    
  

  
      

  
  Member


      

  

  
    
      

      
            nicolas-grekas
  

      

      

      


        Oct 28, 2021


      
    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    Choose a reason for hiding this comment

    
      The reason will be displayed to describe this comment to others. Learn more.
    

    
      
      


  


  
    
      I would suggest to replace this setter by a constructor argument
    
  
  


    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  


          
      
  
    
    
            GromNaN reacted with thumbs up emoji
      
        
            
    All reactions
  


          
              
                👍
                  1 reaction
+        {
+            $this->skipEmail = true;
+        }
         public function onSubmit(FormEvent $event)
         {
             $data = $event->getData();
             if ($this->defaultProtocol && $data && \is_string($data) && !preg_match('~^[\w+.-]+://~', $data)) {
+                if (preg_match('~^[^:/]+@[A-Za-z0-9-.]+\.[A-Za-z0-9]+$~', $data)) {
             Copy link

  
      
    
  

  
      

  
    Contributor


      

  

  
    
      

      
            jvasseur
  

      

      

      


        Oct 27, 2021


      
    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    Choose a reason for hiding this comment

    
      The reason will be displayed to describe this comment to others. Learn more.
    

    
      
      


  


  
    
      Maybe we could be even more aggressive and simply skip adding the default protocol if there is an userinfo part in the URL. Since entering an URL with userinfo is quite rare, forcing to specify the protocol in this case would be a big decrease in user experience.
    
  
  


    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  


          
      
  
    
    
            ThomasLandauer reacted with thumbs up emoji
      
        
            
    All reactions
  


          
              
                👍
                  1 reaction
              
          
        
      
    

    



  
        
  
    
        
    
  


      
          
  
      
            Copy link

  
      
    
  

  
      

  
    Contributor


      

  

  
    
      

      
            ThomasLandauer
  

      

      

      


        Oct 27, 2021


      
    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    Choose a reason for hiding this comment

    
      The reason will be displayed to describe this comment to others. Learn more.
    

    
      
      


  


  
    
      You mean "would not be a big decrease in user experience"
    
  
  


    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  


          
      
  
    
    
            jvasseur reacted with thumbs up emoji
      
        
            
    All reactions
  


          
              
                👍
                  1 reaction
              
          
        
      
    

    



  
        
  
    
        
    
  


      
          
  
      
            Copy link

  
      
    
  

  
      

  
  Member


      

  

  
    
      

      
            nicolas-grekas
  

      

      

      


        Oct 28, 2021


      
  •

  
    
      
        
          edited
          
        
        
      
    
    
      
  
        
      Loading


  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  
    
  


    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    Choose a reason for hiding this comment

    
      The reason will be displayed to describe this comment to others. Learn more.
    

    
      
      


  


  
    
      ~^[^:/?@]++@~ would fit
    
  
  


    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  


          
      
  
    
    
            GromNaN reacted with thumbs up emoji
      
        
            
    All reactions
  


          
              
                👍
                  1 reaction
              
          
        
      
    

    



  
        
  
    
        
    
  


      
          
  
      
            Copy link

  
      
    
  

  
      

  
  Member


      

  Author


  

  
    
      

      
            GromNaN
  

      

      

      


        Oct 28, 2021


      
    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    Choose a reason for hiding this comment

    
      The reason will be displayed to describe this comment to others. Learn more.
    

    
      
      


  


  
    
      Could be ~^([^:/?@]++@|[^./]+$)~ to exclude values like no, that would be converted to a valid url.
    
  
  


    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  


          
      
  
    
    
      
        
            
    All reactions
+                    if ($this->skipEmail) {
+                        return;
+                    }
+                    trigger_deprecation('symfony/form', '5.4', 'Class "%s", will add a scheme to urls that looks like emails in 6.0. Call "setIgnoreEmail(true)"', __CLASS__);
+                }
                 $event->setData($this->defaultProtocol.'://'.$data);
             }
         }
-          Expand Down