1. PHP / Говнокод #11570

    +52

    1. 01
    2. 02
    3. 03
    4. 04
    5. 05
    6. 06
    7. 07
    8. 08
    9. 09
    10. 10
    11. 11
    12. 12
    13. 13
    14. 14
    15. 15
    $hash = md5($eshopId."::".
                        $orderId."::".
                	    $_REQUEST["serviceName"]."::".
                        $_REQUEST["eshopAccount"]."::".
                        $_REQUEST["recipientAmount"]."::".
            			$_REQUEST["recipientCurrency"]."::".
            			$_REQUEST["paymentStatus"]."::".
            			$_REQUEST["userName"]."::".
                        $_REQUEST["userEmail"]."::".
            			$_REQUEST["paymentData"]."::".
            			$secretKey);
            if (strtoupper($_REQUEST["hash"]) != strtoupper($hash)) {
                die('Err: wrong hash.');
            }
......

    я конечно уважаю modx но ето через чур
    http://bezumkin.ru/modx/minishop/extra/intellectmoney.html

    Запостил: Sulik78, 11 Августа 2012

    Комментарии (11) RSS

    • ava bormand 11.08.2012 04:44 # +2

      А что не так, кроме отступов?

      P.S. Разве что на implode("::", array(..тут все поля...)) заменить.
      Ответить

      • ava bormand 11.08.2012 04:53 # 0

        P.P.S. Предложите, пожалуйста, свой вариант этого кода.
        Ответить

        • ava Sulik78 11.08.2012 06:21 # −2

          логика тупа. зачем вообще такой тупой хэш?
          ну или хотя бы уже
          implode('::', $_REQUEST)
          Ответить

          • ava dev6alexander 11.08.2012 07:26 # +2

            Не пойдет если по мимо стандартной информации о проплате они пришлют вам еще кастомные поля к примеру ID пользователя проплатившего заказ. Тогда ваша подпись будет не верна.
            Ответить

          • ava Elvenfighter 11.08.2012 10:52 # +2

            А в PHP есть что-то типа hash-slice? Например в Perl это бы звучало приблизительно как:
            join('::', @_REQUEST{serviceName, eshopAccount, recipientAmount, recipienCurrency, paymentStatus, userName, userEmail, paymentData}, $secretKey);
            Ответить

          • ava bormand 11.08.2012 13:25 # 0

            > Зачем вообще такой тупой хэш.
            Ну авторы платежной системы указали в документации, что считать его нужно именно так. И обсуждать "почему именно так" бессмысленно. Это требование платежной системы. Да и по опыту работы с одной из них скажу - та же хрень, только в профиль, поля немного другие.

            > implode('::', $_REQUEST)
            Ни в коем случае. В хеше важен порядок, и некоторые поля (да тот же $secretKey) в реквесте не размещены.
            Ответить

            • ava bormand 11.08.2012 13:41 # +3

              Комментарии Капитана Очевидность:

              Данная схема - типичный MAC (http://en.wikipedia.org/wiki/Message_authentication_code) основанный на хэш функции. $secretKey известен только данному серверу и серверу платежной системы. Злоумышленник не знает $secretKey, поэтому он не сможет сформировать поддельное сообщение.

              P.S. Смысла упрощать данный код не вижу. В данном состоянии (если причесать отступы, и сделать implode("::", array(...)) вместо конкатенации) он вполне читаем и верифицируем.
              Ответить

          • ava Lowezar 12.08.2012 23:02 # +1

            Вы понимаете, что сделав просто implode('::', $_REQUEST) вы не гарантируете
            1. порядок полей;
            2. даже если лишних параметров не пришлют - сюда же попадёт присланный $_REQUEST['hash'], с которым, собственно, сравнить надо;
            3. не видно, какие поля должны использоваться.

            ...Я мог, наверное, промолчать и дать разобраться "зачем?" самому, но у нас как раз на днях тоже так "оптимизировали" не подумав. За день до релиза.
            Работает - не трожь. ©
            Ответить

            • ava Sulik78 13.08.2012 21:34 # −2

              я написал абстрактно. и не думал что мое сообщение примут буквально.
              имел ввиду сформированный "ручками" массив вместо $_REQUEST.
              Ответить

        • ava Sulik78 14.08.2012 02:24 # −1

          ну если уже и придется делать "тупой" хэш
          $parsed = array_map(function($key) {
        return isset($_REQUEST[$key]) ? $_REQUEST[$key] : $key;
        }, array(
                 $eshopId, $orderId, 'serviceName', 'eshopAccount', 'recipientAmount',
                   'recipienCurrency', 'paymentStatus', 'userName', 'userEmail',
                   'paymentData', $secretKey
                   ));
    
    var_dump(implode('::', $parsed));

          ну а раз уж modx то вообще надо в массив обернуть функцию и засунуть туда класс.
          return $this->getOption($key, $_REQUEST, $key);
          Ответить

          • ava bormand 14.08.2012 05:36 # +2

            Прием попахивает (в данном случае он, конечно, будет работать, но в других может и поломаться) - если одна из переменных (к примеру $secretKey) будет равна чему-то, что есть в $_REQUEST, результат будет неправильным.

            Плюс ко всему - этот вариант "поедает" предупреждения интерпретатора - в оригинальном коде, если бы мы опечатались или платежники убрали поле, в лог бы выдалось предупреждение, здесь же мы предупреждения не получим, и код молча прохеширует имя поля вместо его значения.
            Ответить

    Добавить комментарий