Может ли этот скрипт быть улучшен?

Question

Хорошо, мне интересно, можно ли улучшить этот код, хранящийся в user_login.php, или если я делаю это неправильно. Я смущен, потому что весь мой скрипт в приложении длинный примерно 30-40 строк, и мне было интересно, не хватает ли я чего-то.

Этот скрипт вызывается с вызовом ajax, как и все остальные в моем приложении, за исключением файлов шаблонов.

<?php 

# Ignore 
if (!defined('APP_ON')) { die('Silence...'); } 

# Gets the variables sent 
$user_name = post('user_name'); 
$user_password = extra_crypt(post('user_password')); 

# Check if the user exists 
if (!user::check($user_name, $user_password)) { template::bad($lang['incorrect_login']); } 

# Logging in 
$id = user::get_id($user_name, $user_password); 
$u = new user($id); 
$u->login(); 
template::good($lang['correct_login']); 

?> 

Я собираюсь объяснить это:

# Ignore 
if (!defined('APP_ON')) { die('Silence...'); } 

В основном это проверить, что файл не вызывается непосредственно. Каждый URL-адрес перенаправляется в файл index.php, который управляет URL-адресом (es: www.mysite.com/user/view/id/1) и включает правильный файл. В первых строках этого файла index.php есть define('APP_ON', true);. Индексный файл также инициализирует приложение, вызывающее некоторый набор функций и некоторые классы.

# Gets the variables sent 
$user_name = post('user_name'); 
$user_password = extra_crypt(post('user_password')); 

Функция post() управлять востановление $ _POST [ 'user_name'] делает некоторые проверки. Функция extra_crypt() просто шифрует пароль, используя sha1 и пользовательский алгоритм. Я использую подготовленный оператор для sql-запросов, поэтому не беспокойтесь об экранировании пост-переменных.

# Check if the user exists 
if (!user::check($user_name, $user_password)) { template::bad($lang['incorrect_login']); 

user класс имеет как статические, так и обычные методы. Для статических не требуется идентификатор, из которого выполняются обычные методы. Например, user::check(); проверяет, существует ли имя пользователя и пароль в базе данных. Класс template имеет только два статических метода (template::bad() и template::good()), которые управляют быстрым диалоговым окном для отправки пользователю без верхнего или нижнего колонтитула. Вместо этого, если вы экземпляр класса $t = new template('user_view'), шаблон user_view_body.php называется, и вы можете управлять этой страницей с $t->assign(), что назначить статический ВАР в шаблон, или с $t->loop(), которые начинаются петлей и т.д. Наконец $lang представляет собой массив, имеющий некоторые общие строки в языка, установленного пользователем.

# Logging in 
$id = user::get_id($user_name, $user_password); 
$u = new user($id); 
$u->login(); 
template::good($lang['correct_login']); 

В конце мы имеем фактический логин. Создается класс user и восстанавливается идентификатор. Пользователь с этим идентификатором входит в систему, и мы возвращаем пользователю сообщение template::good(). Для любого пояснения напишите комментарий выше.

Answer 1

С таким количеством функций, которые вы не предоставили, трудно сказать, нужно ли что-то исправлять. Вы уверены, что вход чист в post()? Безопасны ли ваши базы данных в user::check()? Используете ли вы файлы cookie сеансов для упрощения пользовательского префикса/информационного хранилища? Является ли ваш класс template хорошо написанным? Когда вы создаете нового пользователя, регистрируете ли вы всю необходимую информацию (время входа, IP-адрес, если необходимо, и т. Д.)? Обеспечиваете ли вы, чтобы пользователь не был дважды зарегистрирован, если это важно здесь?

Единственная конкретная вещь, которую я могу рассказать вам о том, что вы написали, это то, что sha1 algorithm is completely broken, и вместо этого вы должны использовать что-то другое; см. комментарии ниже для предложений.

Answer 2

Прежде всего функция дайджеста сообщения, такая как sha1, не является функцией шифрования. Поэтому, пожалуйста, удалите crypt из имени функции, чтобы избежать путаницы.Более того, вся эта идея «пользовательского алгоритма» для хранения паролей пугает меня. sha256 - лучший выбор, чем sha1, но sha1 не так уж плох, после того как все еще была утвержденная функция NIST. В отличие от md5 никто не смог создать столкновение для sha1 (хотя это произойдет в последние несколько лет). Если вы поедете с sha1, убедитесь, что соль является префиксом, чтобы предотвратить префиксную атаку.

Проверка ввода должна всегда выполняться во время использования. функция post() должна не нести ответственность за любой подтверждение ввода или выход. Это всего лишь воплощение magic_quotes_gpc, которое снимается, потому что его поезд по безопасности работает.

Файлы с параметризованным запросом, такие как PDO и ADODB, очень хороши, и я рекомендовал его использовать. Это прекрасный пример дезинфекции во время использования.

Ваш шаблон assign() метод может использоваться для дезинфекции для XSS. Smarty поставляется с модулем выходного фильтра htmlspecialchars, который делает это.