Безопасно ли такая авторизация и последующее открытие страниц?

Question

[Имя Фамилия]

Безопасно ли такая авторизация и последующее открытие страниц?
Если залогинен, то присваивается статус logged - true и пользователь находиться на сайте:

if($logged) {
// для авторизованого
}

Функция подключения к базе:

function connectdbpdo() {
      $dbh = new PDO('mysql:dbname=111111;host=localhost;charset=UTF8', '111111', '111111');
      $dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
      return $dbh;
}

$logged = false;

if(isset($_COOKIE['user_id']) AND isset($_COOKIE['hid'])) {
      include "/logged.php";
} else {
      $logged = false;
}

Если есть куки инклюд файла для статуса logged: false или true

$cookie_user_id = preg_replace('/[^a-zA-Z0-9]/ui', '',$_COOKIE['user_id']);
      $cookie_hid = preg_replace('/[^a-zA-Z0-9]/ui', '',$_COOKIE['hid']);
      $checkUsers = $dbpdo->prepare("SELECT * FROM user WHERE alias = :alias and user_hid = :user_hid");
      $checkUsers->execute(array('alias' => $cookie_user_id, 'user_hid' => $cookie_hid));
      $rows = $checkUsers->fetch(PDO::FETCH_ASSOC);
            if($rows) {
            $logged = true;
            } else {
            $logged = false; 
            }
}

Авторизация:

spoiler

include "/functions.php";
$dbpdo = connectdbpdo();
ini_set('error_reporting', E_ALL);
ini_set('display_errors', 1);
ini_set('display_startup_errors', 1);

if($_GET['act'] == 'gologin') {
   // принимаем данные и обрабатываем
   $pass = preg_replace('/[^a-zA-Z0-9]/ui', '',$_POST['password']);
   $pass = trim($pass);
   $pass = preg_replace('/[\s]{2,}/', ' ', $pass);
   $pass = md5($pass);
   $user_email = preg_replace('/[^a-zA-Z0-9@._-]/ui', '',$_POST['email']);
   $user_email = trim($user_email);
		
		// проверяем email на валидность и пропускаем дальше если норм
		if(filter_var($user_email, FILTER_VALIDATE_EMAIL) == false && strlen($user_email > 3)) {
			echo json_encode(array('1','<div class="err_show">The email address was entered incorrectly</div>'));
			die();
		}
		// проверяем пароль  на валидность и пропускаем дальше если норм
		elseif (strlen($pass) <= 5){
			echo json_encode(array('2','<div class="err_show">Password must be 6 characters</div>'));
			die();
		}

		else {
			// если всё норм то делаем запрос в базу на наличие данных
			$resjd = $dbpdo->prepare("SELECT * FROM user WHERE user_email = :user_email and user_password = :user_password");
			$resjd->execute(array('user_email' => $user_email, 'user_password' => $pass));
			$rowndd = $resjd->fetch(PDO::FETCH_ASSOC);
				// если есть совпадение - авторизуем
				if($rowndd) {
					$hid = md5(time().$pass).md5($pass);
					$go = $dbpdo->prepare("UPDATE user SET user_hid = :user_hid WHERE user_email = :user_email");
					$go->execute(array('user_hid' => $hid, 'user_email' => $user_email));
					setcookie("user_id", $rowndd['alias'],time()+360000, "/","", 0);
					setcookie("hid", $hid, time()[phone], "/","", 0);
					// возвращаем урл юзера, на который он переходит автоматически
					echo json_encode(array('4', $rowndd['alias']));
					
						} else {
							// юзера такого нет, отправояем ему сообщение
							echo json_encode(array('3','<div class="err_show">E-mail or password entered incorrectly.</div>'));
							die();
						}
		}
die();
} 
else {
header('HTTP/1.0 404 Not Found'); die();

Спасибо.

Comments

[Дмитрий]

Обычно используют сессии. Хотя сто процентной защиты нет. Ну и с временем по итогу можно обмануться
Лучше один раз time() сохраните в переменную, и где нужно используйте

[Дмитрий]

Ну и рекомендую не экономить на пустых строках и отступах

[Имя Фамилия]

Дмитрий, пустые строки и коменты убрал, чтобы более сжато сюда выложить. На таких костылях можно ходить?

[FanatPHP]

нет нельзя.
без отступов слева вообще нельзя код ни писать ни выкладывать

[FanatPHP]

какой-то код бессмысленный.
почему

if(isset($_COOKIE['user_id']) AND isset($_COOKIE['hid'])) {

повторяется два раза?

[Имя Фамилия]

FanatPHP, учту, спасибо.

[FanatPHP]

не "учту" а "уже иду редактировать вопрос".

в частности именно из-за отсутствия отступов alexalexes обознался и решил что $logged внутри connectdbpdo()
потому что в этой тупой портянке действительно хрен поймешь, где заканчивается функция.

[FanatPHP]

что лежит в $_COOKIE['user_id']? пример

Answer 1

[alexalexes]

Эта конструкция небезопасна, если у программиста кривые руки.

if($rows) {
$logged = true;
} else {
$logged = false; 
}
} 
else {
$logged = false;
}

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

// Секция, где присваиваются значения по умолчанию основных переменных
$logged = false;
.....
....
// начинаем проверку
if(первое условие)
{
  if(второе условие)
  {
    if(еще какое-то супер-пупер условие)
    {
       $logged = true; // вот теперь, пройдя все медные трубы, мы изменяем статус
    }
  }
}

Comments

[Имя Фамилия]

спасибо за дельный совет. Обязательно учту. Спасибо.

[FanatPHP]

Если сам не ориентируешься в ООП, то не не давать дурацких советов
про бессмысленный класс в котором connectdbpdo() имеет отношение к переменной $logged
Вот блин я не понимаю как можно писать настолько бессмысленный код?
Ну вот у меня иногда складывается впечатление что вы пишете код вообще не понимая, что он делает. Просто механически вбивая знакомые операторы.

И где ты там вообще увидел $logged внутри connectdbpdo() ?

[FanatPHP]

убери ради бога "момент второй" - будет нормальный ответ

[alexalexes]

Убрал.
Я просто хотел показать, чтобы избегали использование глобальных переменных.
Если нужно хранить где-то какое-то глобальное состояние, то для этого хотя бы сделать один класс-монолит, который не будет отвечать действительной модели приложения, но поможет описывать переменные состояния в одном месте.

[FanatPHP]

один класс-монолит - это абсолютно та же самая глобальная переменная, только возведенная на новый уровень.
И современные фреймворки, в том числе Yii3, от неё в ужасе отказываются, в пользу внедрения зависимостей.
Нужна тебе переменная? Передавай в параметрах функции.

[alexalexes]

Хорошо, буду изучать современные подходы.