Наговнокодил, на сколько все плачевно?

Question

[Валера Иванов]

Решил поговнокодить, велосипед рабочий, но на сколько все плачевно и как можно упростить мой велосипед?

if(isset($_REQUEST['success'])){
	$active = htmlspecialchars($_POST['active']);
	$pass = htmlspecialchars($_POST['password']);
	
	$mysql = mysqli_query($base, "SELECT `email`,`login`,`password` FROM `profile` WHERE `email` = '".$active."' OR `login` = '".$active."' and `password`='".md5($pass)."' LIMIT 1");
	$sql = mysqli_fetch_array($mysql);
	
	if($active == $sql['login']){
		$namecookie = 'login';
	}elseif($active == $sql['email']){
		$namecookie = 'email';
	}
	
	if(empty($active)) msg('Введите email или логин');
	elseif(empty($pass)) msg('Введите пароль');
	elseif($sql == 0) msg('Такой пользователь не найден ');
	else {
		
		setcookie(''.$namecookie.'', $active, time()+86400*365, '/');
		setcookie('password', md5($pass), time()+86400*365, '/');
		header('location: /main.php');
	}
}

Answer 1

[Алексей Уколов]

Классическая sql-инъекция.
Пароль в md5.
Сначала делается запрос, а потом проверяется, что логин и пароль заполнены.
Сначала используются данные запроса, потом проверяется, что он был успешен.
Блоки условий не обёрнуты в скобки.
Пароль и логин зачем-то сохраняются в куки.
У кук нет флага http-only.

Спасать здесь нечего, нужно выбросить и сделать нормально в соответствии с современными стандартами и практиками.

Answer 2

[Олег]

select "valid" as a from personal WHERE '1'='1' or '1'='2' and '3'='4'
вернет все записи.
это я к чему,
если есть в базе пользователь с емайлом или телефоном, то пароль в вашем варианте можно вводит любой.
нужны скобки
(email = or phone= ) and password=

Answer 3

[Анатолий Цивилёв]

у вас в запросе: select email... where email - это как "чему равен Икс, если Икс равен трём?"

Про куки - уже написали, но у вас все же реализация хотя бы с хешем пароля в куках.

Этакий эмбрион авторизации)

Изучите чьё-нибудь готовое решение на каком-нибудь ресурсе типа хабра, а не на форумах, где решения пишут новички вашего уровня, и переделайте под себя. Увидите, сколько там проверок будет.

Так, ради шутки вот сколько у меня приватных и публичных методов в черновом неготовом классе, который занимается только авторизацией и больше ничем

У меня например по куке вообще не поймешь - от какого она юзера. Генерируется на основе узерагента, подсети, айди, соли, хеша пароля - но и то я жопой чую, что чето не так у меня =) если код свой тут выложу - ой засмеют)

Answer 4

[zenaku]

Скажу сразу, пхп почти забыл.
Но сделал бы я иначе, и через ФВ хотя бы. Хотя тут зависит от проекта. Если это был бы код ФБ, то все очень плохо. А если сайта с фотками твоих любимых кошей - и так сойдет.

Прям в коде не разбирался, но что бросается мне в глаза:
мд5, отсутствие ООП, неадаптивность(mysql), устаревшие функции(mysql_query), сомнительная проверка данных, отсутствие подготовленных запросов в БД, что за лимит 1(то есть можно создать копию чужого аккаунта? )

if($active == $sql['login']){
    $namecookie = 'login';
  }elseif($active == $sql['email']){
    $namecookie = 'email';
  }

Зачем это делать? Можно же с помощью 1/0 указать. Или более красиво. ИМХО

Почему проверка на пустые значения происходит после обращения к БД и указания названия куки? То есть с помощью чтения кук я могу шерстить базу данных на существования пользователя с таким ником или почтой? Кука же создаться, если совпадет. А доступа не будет, да.
Почему хэш пароля в куке хранится - табу. Почему не айди сессии или токен?
Почему переадресация на главную страницу, а не предыдущую страницу перед требование авторизации?

Answer 5

[Barmunk]

1. Валидируйте POST в самом начале скрипта. Если пользователь ничего не ввел, сразу выбрасывайте с ошибкой. Проверяйте как минимум на пустоту.

2. На втором шаге передавайте $_POST в подготовленный запрос.

3. С куками беда. Сохранять в них пароль нельзя. Самый простой вариант сохранять в сессию id авторизованного пользователя или генерировать токен, записывать его в куки и в бд.