Ocena kodu logowania.

Question

Witam, proszę Was o ocenę kodu logowania napisanego przeze mnie w PHP. Jeśli jest coś źle zrobione proszę o napisanie co mógłbym poprawić.

<?php



    session_start();

    $message = "Wszystko OK!";

    $conn = mysqli_connect("host", "nick", "hasło", "nazwa bazy danych");



    if ($conn->connect_error) {

        die($conn->connect_error);

    } else {  

        if (isset($_SESSION['user_id'])) {

            header('Location: user_panel.php');

        } else {  

            if (!empty($_POST["Name"]) && mysqli_real_escape_string($conn, $_POST["Name"]) == $_POST["Name"] || !empty($_POST["Password"]) && mysqli_real_escape_string($conn, $_POST["Name"]) == $_POST["Name"]) {

                $name = $_POST["Name"];

                $sql = "SELECT * FROM `players` WHERE BINARY name = '$name'";

                $results = mysqli_fetch_assoc(mysqli_query($conn, $sql));

                

                if (!($results == null)) {

                    if (count($results) > 0 && password_verify($_POST["Name"], $results["password"])) {

                        $_SESSION["user_id"] = $results["id"];

                        header('Location: user_panel.php');

                    } else {

                        $message = "Nieprawidłowe dane.";

                    }

                } else {

                    $message = "Nieprawidłowe dane.";

                }

            } else {

                $message = "Wszystko OK!";

            }

        }

    }



?>

 

Answer 1

• Skoro stosujesz die, to możesz się pozbyć tego else. Zawsze to jeden poziom zagnieżdżenia mniej, a kod będzie działał tak samo (bo jak wejdzie do ifa z die, to i tak dalej nie pójdzie).
• Jak już jesteśmy przy tym die – po co userowi techniczne info o błędzie? Takie coś jest przeznaczone dla programisty i powinno trafić do logów, a user powinien po prostu dostać komunikat, ze coś nie działa.
• Zresztą podobny zabieg można zrobić ze sprawdzaniem S_SESSION. Na dobrą sprawę to sprawdzanie powinieneś zrobić na samym początku, bo do tego Ci nawet połączenie z bazą niepotrzebne. Jak zmienna sesyjna istnieje, to header + die i tyle. Nie potrzeba else.
• Czemu porównujesz wprowadzone przez usera dane do wyniku przepuszczenia ich przez mysqli_real_escape_string? To mocno ogranicza tak naprawdę bezpieczeństwo hasła, w którym mogą być przeróżne znaki. Pierwsze losowo wygenerowane hasło z generatora haseł online: -vx"`%Z2#m2r/&%%. Powinieneś używać prepared statements. Inna rzecz: po co to porównanie robisz dla hasła, skoro go nawet nie umieszczasz w zapytaniu?
• W password_verify chyba powinieneś używać $_POST['Password'] zamiast $_POST['Name'].
• Przy zastosowaniu tzw. guard clauses całą tę drabinkę ifów można by znacząco uprościć.

Answer 2

https://pl.wikipedia.org/wiki/SQL_injection

WHERE BINARY name = '$name'";