Tak bardzo źle… dotnet/corefx EmailAddressAttribute.cs

W pracy programisty są takie błędy, których znalezienie jest tak zaskakujące, że wydaje się aż niemożliwe aby one istniały. Do takich sytuacji należy również implementacja dotnetowego EmailAddressAttribute.


Ta historia zaczyna się niewinnie. Zgłoszenie od testera błędu o przybliżonej treści „Można utworzyć użytkownika z niepoprawnym adresem email”. Po kilku minutach krótkiej prezentacji przy komputerze okazuje się, że:

– podanie adresu adres@com kończy się poprawnym zapisem
– podanie adresu adres.com kończy się wyświetleniem błędu zapisu

Wydawałoby się na pierwszy rzut oka, że weryfikacja adresu email odbywa się tylko poprzez sprawdzenie czy ciąg znaków zawiera znak @.

Zaraz po prezentacji okazało się, że walidacja danych odbywa się z użyciem standardowego atrybutu pochodzącego z jednej z bibliotek .NET Core. Można wyobrazić sobie moje zaskoczenie, gdy okazało się, że rzeczywiście kod firmy Microsoft sprawdza adresy email poprzez proste przeszukiwanie ciągu znaków.

// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

namespace System.ComponentModel.DataAnnotations
{
    [AttributeUsage(AttributeTargets.Property | AttributeTargets.Field | AttributeTargets.Parameter,
        AllowMultiple = false)]
    public sealed class EmailAddressAttribute : DataTypeAttribute
    {
        public EmailAddressAttribute()
            : base(DataType.EmailAddress)
        {
            // Set DefaultErrorMessage not ErrrorMessage, allowing user to set
            // ErrorMessageResourceType and ErrorMessageResourceName to use localized messages.
            DefaultErrorMessage = SR.EmailAddressAttribute_Invalid;
        }

        public override bool IsValid(object value)
        {
            if (value == null)
            {
                return true;
            }

            if (!(value is string valueAsString))
            {
                return false;
            }

            // only return true if there is only 1 '@' character
            // and it is neither the first nor the last character
            bool found = false;
            for (int i = 0; i < valueAsString.Length; i++)
            {
                if (valueAsString[i] == '@')
                {
                    if (found || i == 0 || i == valueAsString.Length - 1)
                    {
                        return false;
                    }
                    found = true;
                }
            }

            return found;
        }
    }
}

Autor kodu nawet pokusił się o krótki komentarz, który głosi, że adresem email (wg tego atrybutu) jest „ciąg znaków zawierający tylko jeden znak @, który nie znajduje się na początku lub końcu ciągu”. Podczas gdy RFC 2822 określa adres taki jak asd\@asd@asd.com jako poprawny mimo tego, że zawiera dwa znaki @. Co więcej, niepoprawny adres email asd@asd zostanie przedstawiony jako poprawny.

Wiele osób od razu w takich przypadkach zastanawia się, czy takie sprawdzenie jest bezpieczne? Szczerze mówiąc, ciężko mi odpowiedzieć na takie pytania. Znając życie, prędzej czy później znajdzie się „koks”, który w skrajnie hardcorowy sposób wykorzysta tą implementację.

Z punktu widzenia programisty czuję się oszukany. Biblioteka kłamie mi w oczy mówiąc, że otrzymałem poprawny adres email, który dla mnie sprawdzili. Co więcej, fałszywie odrzuca adres email mojego kolegi o perwersyjnym upodobaniu do dziwnych, w pełni poprawnych, adresów (kto i po co tworzy sobie konta o takich adresach). „Czysty kod” jako jedną z najważniejszych zasad podaje to, że metoda (w tym przypadku atrybut) nie może kłamać. Jak dla mnie ta zasada została karygodnie złamana. W tym przypadku można pomyśleć, że przedstawiona klasa powinna mieć inną nazwę. Może przykładowo coś w stylu: StringWithAtCharacterButFirstAndLastCharIsNotAtCharacterAttribute.

Należy jednak się zastanowić, czy są jakieś specjalne warunki usprawiedliwiające takie niechlubne postępowanie. Przykładowo, czy istnieje kwestia optymalizacji czasowej/obliczeniowej dla tego problemu. Jest to możliwe z uwagi na to, że atrybut aż zachęca do wykorzystania go do walidacji danych dostarczonych do API. Tylko czemu, w takim wypadku nie zostało to odpowiednio udokumentowane…?

Dzięki rewolucji, która zadziała się przy wydaniu .NET Core byłem w stanie zgłosić swoje spostrzeżenia (github). Ciekawe czy jest jakieś inne wytłumaczenie dla tego kodu.

EDIT: 2018-11-17
Na chwilę obecną sprawa ucichła. Głównym elementem sporu jest zakres w jakim mechanizm walidacji powinien działać. Nie ma pewności czy warto wspierać w całości RFC 2822 z uwagi na to, że taka walidacja może być dość powolna.

Dodaj komentarz

Twój adres email nie zostanie opublikowany. Pola, których wypełnienie jest wymagane, są oznaczone symbolem *