1

だから私は自分のサイトのためにこのスクリプトを書こうとしています。それはかなりめちゃくちゃで壊れているように見えます。多分誰かが私がそれを少し片付けて、何が間違っているかもしれないかを説明するのを手伝ってくれるでしょう。また、それを短くする方法はありますか、私には少し危険に見えます。ありがとうございました。

<?php

class Register
{
    private $username;
    private $password;
    private $password2;
    private $passmd5;
    private $email;
    private $email2;

    private $errors;
    private $rtoken;

    public function __construct()
    {
        $this->errors = array();

        $this->username  = $this->filter($_POST['ruser']);
        $this->password  = $this->filter($_POST['rpass']);
        $this->password2 = $this->filter($_POST['rpass2']);
        $this->email     = $this->filter($_POST['remail']);
        $this->email2    = $this->filter($_POST['remail2']);
        $this->rtoken    = $_POST['rtoken'];

        $this->passmd5 = md5($this->password);
    }

    public function process()
    {
        if ($this->valid_rtoken() && $this->valid_data())
            $this->register();

        return count($this->errors) ? 0 : 1;
    }

    public function filter($var)
    {
        return preg_replace('/[^a-zA-Z0-9@.]/', '', $var);
    }
    public function register()
    {
        mysql_query("INSERT INTO users(username,password,email) VALUES ('{$this->username}','{$this->passmd5}','{$this->email}')");

        if (mysql_affected_rows() < 1)
            $this->errors[] = '<font color="red">Database error</font>';
    }

    public function user_exists()
    {
        $data = mysql_query("SELECT ID FROM users WHERE username = '{$this->username}'");

        return mysql_num_rows($data) ? 1 : 0;
    }

    public function email_exists()
    {
        $data = mysql_query("SELECT ID FROM users WHERE email = '{$this->email}'");

        return mysql_num_rows($data) ? 1 : 0;
    }

    public function show_errors()
    {
        echo "";

        foreach ($this->errors as $key => $value)
            echo $value . "<br>";
    }

    public function valid_data()
    {
        if ($this->user_exists())
            $this->errors[] = '<font color="red">Username Exists</font>';
        if ($this->email_exists())
            $this->errors[] = '<font color="red">email exists</font>';
        if (empty($this->username))
            $this->errors[] = '<font color="red">check your username</font>';
        if (empty($this->password))
            $this->errors[] = '<font color="red">check your password</font>';
        if ($this->password != $this->password2)
            $this->errors[] = '<font color="red">Passwords do not match</font>';
        if (empty($this->email) || !eregi('^[a-zA-Z0-9._-]+@[a-zA-Z0-9._-]+\.[a-zA-Z]{2,4}$', $this->email))
            $this->errors[] = '<font color="red">Check your email</font>';
        if ($this->email != $this->email2)
            $this->errors[] = '<font color="red">Emails do not match</font>';

        return count($this->errors) ? 0 : 1;
    }


    public function valid_rtoken()
    {
        if (!isset($_SESSION['rtoken']) || $this->rtoken != $_SESSION['rtoken'])
            $this->errors[] = '<font color="red">Check</font>';

        return count($this->errors) ? 0 : 1;
    }
}

?>
4

1 に答える 1

3

コードを書くためのより良い方法は常にあります。コードを書き直すのではなく、なぜコードが長いのか、なぜ安全ではないのかを再考するようにあなたに挑戦したいと考えています。うまくいけば、この方法でより多くを学ぶことができます。

そもそも、MD5 でパスワードをハッシュすることは完全に安全ではありません。MD5 がかつて存在したことを忘れてください。SHA1 でさえ安全なものには使用しないでください。代わりに、bcrypt または PBKDF2 を使用する必要があります (SHA2 または Whirlpool と ~2000+ ラウンド)。より良いセキュリティの実装に役立つ記事やライブラリへのヒントとリンクについては、PHP パスワードのセキュア ハッシュとソルトに対する私の回答を参照することをお勧めします。

次に、mysql_* 関数はPHP 5.5 で非推奨になりました。MySQLi を使用すれば問題は解決しますが、PDOなどの一貫したインターフェイスを使用して接続を処理し、クエリのエスケープ/フィルタリングを行う必要があります。PHP 5.5 でソフトウェアを構築していない場合でも、ホストが PHP のバージョンをアップグレードすることを決定したかどうかを常に制御できるとは限りません。可能な限り将来の互換性のために最適化します。さらに、PDO には、そのドキュメントで説明されている便利な機能がいくつかあります。

第三に、クエリ変数を「フィルタリング」するために正規表現を使用しないでください。あなたができる最も安全な方法は、任意の数値で intval/floatval を使用し、mysql_escape_string (またはmysqli_real_escape_string )などの使用する DB ライブラリを介して残りをエスケープするか、準備済みステートメント(変数をサニタイズします) を使用することです。

第 4 に、表示ロジックをオブジェクトに入れています。考えてみてください: このオブジェクトはどのような目的/役割を果たしますか? 登録ロジックを処理しますか? 登録データは保存されますか？ここでは、単一責任の原則を使用することをお勧めします。オブジェクトは、プレゼンテーション情報を含むハイブリッド モデル コントローラーのように動作するはずです。これを RegistrationModel および RegistrationController クラスに拡張して、データの一時的な保存を処理したり、別のことを行うことを決定したりすることもできます。しかし覚えておいてほしいのは、あなたのクラスがより多くの責任を負うほど、より多くの方法で破らなければならないということです。

また、Register のすべての属性を非公開にすることで、複数の方法で登録することができなくなります。OAuth (Twitter や Facebook など) 経由でログインするなど、プロセスへのショートカットが必要であるが、Register のロジックの一部を再利用する必要がある場合はどうすればよいでしょうか? これらの属性は、継承できるように少なくとも保護する必要があります。または、別のオブジェクトがそれらとやり取りできるように公開する必要があります (ユーザーに登録が成功したことを通知するオブジェクトなど)。

于 2013-02-13T18:19:30.537 に答える