Мой первый сайт на PHP

А вот не надо. Я всегда на стороне упорных новичков, которые что-то пытаются делать.

Автор, что-то в админке не работает кнопка "ответить".
Ну и CSS надо все же получше подучить, подпричесать все это дело. Дихайн ну очень сильно хромает пока что. Функциональность же простенькая, да. Но для начала неплохо.

 

@Dimon2x
слушай, неплохо.

 

Вот тебе небольшое ревью по коду:

1)

PHP:

1. die('Database error: '.$e->getMessage().'<br>');

Не надо так делать. Незачем на пользователя это вываливать. Пиши это в лог себе. А пользователю просто покажи нейтральную надпись. мол, так и так, неполадки, так и так, исправим.

2) После таких вещей, то бишь переадресации

PHP:

1. if(!empty($_SESSION['user'])){
2.             header('Location: ?interface-admin=1');
3.         }

Обычно сразу можно дергать die или exit. Ты уже выкинул заголовок переадресации. Все остальное, что может выдать сервер, уже никому не нужно стало. Этот запрос объявлен закрытым.

3)

PHP:

1. $user = "SELECT `login`, `password` FROM users WHERE login = '" . $login ."' AND password = '". $password ."'";
2.  
3.         $resUser = $this->db->prepare($user);
4.         $resUser->execute();

Ты неправильно используешь prepare. У тебя код уязвим к SQL_инъекции. Читай-вникай. Недостаточно просто собрать строку запроса и сделать prepare. Для машины это просто строка текста. Что в ней подготавливать? По каким правилам? Как она разберется, чего ты от нее хочешь? Там все не просто так. Ну и да, по традиции, подумай, нужен ли тебе PDO как таковой.

4)

PHP:

1. function getListAdmin() {
2.         $sql = "SELECT login, password, id FROM users";
3.         $allUser = $this->db->query($sql);
4.         $allUser->setFetchMode(PDO::FETCH_ASSOC);
5.         return $allUser->fetchAll();
6.     }

Полные выборки лучше не делать. Думай сразу наперед. Что будешь делать, когда будет миллион пользователей? То-то и оно. Лимиты. Условия. Это вот все надо применять сразу.

5)

PHP:

1. $login = strip_tags(trim($l));
2.         $password = strip_tags(trim($p));

Не надо так делать. Это порча данных. Во-первых, ник человека будет не тот, какой он ввел. А это его личное дело, в общем-то. Не хочешь, чтобы его ник сломал верстку - просто при выводе данных прогоняй их через htmlspecialchars();
Во-вторых, как думаешь, что случится с крайне сильным паролем H<DSу2128dfHDGQUWD@^%#R&*&)0r9y32ufdq3>76 после функции strip_tags? Он превратится в H76. Это не круто. Ты подставил пользователя. Да и какая разница, что он там в пароле написал? Чем тебе там угловые скобки не угодили?

6)
Ты не хэшируешь пароли и хранишь их в открытом виде. Это неправильно. Так делать нельзя. Как минимум - это не этично. Как максимум - не безопасно. Люди, регаясь у тебя, доверяют тебе свой пароль - это очень ценная вещь. Твоя святая обязанность - защитить его от утечки наружу. И, в первую очередь, оградить себя/своих админов от соблазна их использования. Пользователь доверяет тебе. Соответствуй. Читай вникай.

7)

PHP:

1. $input_name = trim(htmlspecialchars(strip_tags($name)));

Почитай в отдельности про каждую функцию. И подумай, почему эта конструкция - бред. Да да, ты ее подглядел где-то в интернете/видеокурсах/на каком-то форуме. Но это не отменяет того факта, что это бред. Эта конструкция гуляет по интернетам и уже стала своего рода лакмусовой бумажкой. И она у тебя повсюду. Знаешь, что самое забавное? Она не дает вообще никакой защиты. Корёжит входящие данные, подменяет в них символы, увеличивая вес данных, но никак не делает их безопасными для вставки в БД.

И опять же, strip_tags - вандальная функция. Она варварски портит данные лишь на основании наличия в них угловых скобок. Никогда ей не пользуйся. Вот прям буквально вот никогда. Вообще. Вообще никогда.

8)
При этом ты пользуешься "коробочной" фильтрацией для емейла. Молодец. Это правильно. Не стал городить огороды из двадцатиэтажных регулярок.

9)
У тебя в all-question.php(и еще в ряде файлов, отвечающих за генерацию view) есть целая микробиблиотека на JS. Почему бы не вынести все это в отдельный файл? Увеличишь скорость загрузки сайта.

10)Здесь и далее

PHP:

1. <a href="?interface-admin=1&del-id='.$admin['id'].'">Удалить</a>'

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

--------------------------

На первый взгляд это вот то, что бросилось в глаза.
Глубже ревьювить нет времени, не обессудь.

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

Главное руки не опускать.

При этом код, что интересно, написан аккуратно, с определенной логикой, читабельностью и понятностью. Ты в PHP пришел с другого языка?

 

Что я такого могу сделать на своём сайте, имея пароль пользователя, чего не могу сделать, не имея его? Разве что попытать счастья и взломать почту пользователя, надеясь, что он везде ставит один пароль. Утечка - да, а вот защищать от себя - я и так имею доступ ко всему, что пользователь ввёл, обычно

 

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

Раз ты спрашивает, значит не понимаешь. Спроси, что не понимаешь или проверь своё понимание. Плейсхолдеры тоже могут быть опасными.

И кавычки забыл на 3 и 4 строке последнего варианта.

 

В плане защиты от данных, ломающих запрос (а именно этим являются SQL-инъекции) именованные и неименованные плейсхолдеры одинаковы.
--- Добавлено ---
ТС, если ты уж пишешь MVC, то модель не должна ничего редиректить, выводить какие-то сообщения, заниматься чем-то, кроме работы с данными. Классы не разбиты по сущностям, бесконечные, делают слишком много работы. Аутентификация - это одно, вывод вопросов - это другое. Тоже и с моделью. Админка и фронт в одном классе контроллера - это уже вообще ни о чём.

 

В 95% случаев так оно и есть. А там можно и до финансов добраться, если клиент не очень любит двухфакторные методы аутентификации.

 

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

А на него тебе уже ответили.

Но вот Игорь туманно намекнул на то, что и подготовленные выражения могут быть опасными, а раскрыть мысль забыл. Речь шла об инъекциях второго порядка. Это атаки через кодировки, или через доверие разработчика к данным, взятым из своей же БД. То есть на кавычках экранированных проблемы не кончаются. Это лишь один из факторов. Самый часто используемый, да, но не единственный.

 

@_ne_scaju_
почитай про инъекции в википедии, потом про PDO и плейсхолдеры и всё такое. Подумай. Потом задай вопрос =) Я буду рад ответить.

 

Согласен. Просто раз уж человек называет свои классы "Модель", "Контроллер", то из этого следует, что он что-то где-то об этом прочёл (слышал), что типа "круто". А значит не мешает подсказать, как это делать правильнее
--- Добавлено ---

Тебе обязательно переспросить надо то, на что и так очень ясно ответили. Ещё раз, дал ты имена плейсхолдерам (типа :login) или не дал (оставил просто ?), на безопасность запросов не влияет. Другое дело, что именованные удобнее для программиста

 

Если php >= 5.4, то только квадратные скобки. Вариант с array () был единственным в более ранних версиях. На скорость никак не влияет

 

Лучше читаются именованные. И удобнее выражения подставлять. А вообще, на вкус и цвет

 

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