Есть одна итальянская страница на Facebook. Называется она «Il Programmatore di Merda», что в переводе означает «Дерьмовый программист». Мне нравится эта страница.
Там часто публикуют куски отвратительного кода и мемы о программировании. Но однажды я увидел там кое-что совершенно потрясающее.
Этот кусок кода заслужил почётное звание «лучшего произведения» за неделю.
Я решил этот код разобрать, но тут так много всего неправильного, что мне сложно даже выбрать первую проблему для анализа.
Если вы — начинающий программист, то мой материал поможет вам понять то, какие ужасные ошибки совершены тем, кто писал этот код.
28 строк ошибок кода
Давайте, чтобы было удобнее, наберём этот код в редакторе:
<script>
function authenticateUser(username, password) {
var accounts = apiService.sql(
"SELECT * FROM users"
);
for (var i = 0; i < accounts.length; i++) {
var account = accounts [i];
if (account.username === username &&
account.password === password)
{
return true;
}
}
if ("true" === "true") {
return false;
}
}
$('#login').click(function() {
var username = $("#username").val();
var password = $("#password").val();
var authenticated = authenticateUser(username, password);
if (authenticated === true) {
$.cookie('loggedin', 'yes', { expires: 1 });
} else if (authenticated === false) {
$("error_message").show(LogInFailed);
}
});
</script>
И… Ну правда — не знаю, с чего начать его разбор. Но начинать, всё же, надо. Я решил разбить недостатки этого кода на три категории:
- Проблемы с безопасностью.
- Проблемы с базовыми знаниями о программировании.
- Проблемы с оформлением кода.
Проблемы с безопасностью
Этот код, наверняка, выполняется на клиенте. На это указывает то, что он заключён в тег <script> (и, кроме того, тут применяется jQuery).
Но не поймите меня неправильно. Этот код выглядел бы так же ужасно, если бы он был предназначен для сервера. Но вот выполнение его на клиенте означает то, что доступ к базе данных, которая в нём используется, будет у всех, кто способен этот код прочитать.
Взглянем на функцию authenticateUser:
function authenticateUser(username, password) {
var accounts = apiService.sql(
"SELECT * FROM users"
);
for (var i = 0; i < accounts.length; i++) {
var account = accounts [i];
if (account.username === username &&
account.password === password)
{
return true;
}
}
if ("true" === "true") {
return false;
}
}
Из анализа её кода можно сделать вывод о том, что где-то имеется объект apiService, дающий в наше распоряжение метод .sql, с помощью которого можно выполнять SQL-запросы к базе данных. Это означает, что если просматривая страницу, на которой имеется этот код, открыть консоль разработчика, можно будет выполнять любые запросы к базе данных.
Например, можно выполнить такой запрос:
apiService.sql("show tables;");
В ответ на него будет возвращён полный список таблиц базы данных. И всё это — с использованием собственного API проекта. Но, чего уж там, давайте представим себе, что это — не настоящая проблема. Давайте лучше взглянем вот на это:
if (account.username === username &&
account.password === password)
Этот код говорит мне о том, что пароли в проекте хранятся без хеширования. Отличный ход! Это значит, что я могу взять отладчик и посмотреть пароли пользователей проекта. А ещё я уверен в том, что большой процент пользователей применяет одну и ту же пару имя пользователя/пароль в социальных сетях, в сервисах электронной почты, в банковских приложениях и в прочих подобных местах.
Проблема видится мне и в том, как в этом коде устанавливаются куки loggedin:
$.cookie('loggedin', 'yes', { expires: 1 });
Оказывается, тут применяется jQuery для установки куки-файла, который сообщает веб-приложению о том, аутентифицировался ли пользователь.
Запомните: никогда не устанавливайте подобные куки с использованием JavaScript.
Если нужно хранить на клиенте информацию такого рода, то для этого чаще всего используются именно куки-файлы. Ничего плохого о подобной идее я сказать не могу. Но установка куки с использованием JavaScript означает невозможность настройки атрибута httpOnly. А это, в свою очередь, означает, что любой вредоносный скрипт может получить доступ к подобным куки-файлам.
И да, я знаю о том, что тут сохраняется лишь нечто вроде пары ключ: значение, 'loggedin': 'yes', поэтому если даже подобное прочтёт чужой скрипт, особого вреда от этого не будет. Но это, в любом случае, очень плохая практика.
Кроме того, открыв консоль Chrome, я всегда могу ввести там команду вроде $.cookie('loggedin', 'yes', { expires: 1000000000000 });. В результате окажется, что я вошёл на сайт навсегда, даже не имея при этом учётной записи на нём.
Проблемы с базовыми знаниями о программировании
О подобных проблемах, которые можно обнаружить в этом фрагменте кода, можно говорить и говорить, а времени у нас не так уж и много.
Итак, очевидно, что функция authenticateUser — это пример очень плохо написанного кода. Она демонстрирует отсутствие у того, кто её писал, некоторых базовых знаний о программировании.
function authenticateUser(username, password) {
var accounts = apiService.sql(
"SELECT * FROM users"
);
for (var i = 0; i < accounts.length; i++) {
var account = accounts [i];
if (account.username === username &&
account.password === password)
{
return true;
}
}
if ("true" === "true") {
return false;
}
}
Может, вместо того, чтобы получать полный список пользователей из базы данных, достаточно выбрать пользователя с заданным именем и паролем? А что произойдёт, если в этой базе данных хранятся сведения о миллионах пользователей?
Я уже об этом говорил, но повторюсь, задавшись следующим вопросом: «Почему они не хешируют пароли в своей базе данных?».
Давайте теперь взглянем на то, что возвращает функция authenticateUser. Исходя из того, что я вижу, я могу сделать вывод о том, что она принимает два аргумента типа string и возвращает единственное значение типа boolean.
Поэтому следующий фрагмент кода, хоть и написан он ужасно, бессмысленным назвать нельзя:
for (var i = 0; i < accounts.length; i++) {
var account = accounts [i];
if (account.username === username &&
account.password === password)
{
return true;
}
}
Если перевести его на обычный язык, то получится примерно следующее: «Существует ли пользователь с именем X и с паролем Y? Если да — вернуть true».
А теперь давайте взглянем на этот фрагмент кода:
if ("true" === "true") {
return false;
}
Бессмыслица. Почему бы функции просто не вернуть false? Зачем ей условие, которое всегда истинно?
Теперь давайте проанализируем следующий код:
$('#login').click(function() {
var username = $("#username").val();
var password = $("#password").val();
var authenticated = authenticateUser(username, password);
if (authenticated === true) {
$.cookie('loggedin', 'yes', { expires: 1 });
} else if (authenticated === false) {
$("error_message").show(LogInFailed);
}
});
Та часть этого кода, где осуществляется работа со значениями с использованием jQuery, выглядит нормально. Но проблема тут в организации работы с куки loggedin.
Мы уже говорили о том, что, даже не имея учётной записи, можно просто открыть консоль Chrome и, выполнив команду $.cookie('loggedin', 'yes', { expires: 1 });, оказаться аутентифицированным на один день.
А как эта страница узнаёт о том, кем именно является аутентифицированный пользователь? Может, она просто выводит что-то особенное, предназначенное только для тех, кто успешно прошёл проверку имени пользователя и пароля и не выводит никаких персональных данных? Этого нам не узнать.
Проблемы с оформлением кода
Оформление — это, вероятно, самая маленькая и незначительная проблема данного кода. Но она чётко указывает на то, что тот, кто этот код создавал, откуда-то скопипастил его отдельные фрагменты.
Вот пример использования двойных кавычек при оформлении строк:
var username = $("#username").val();
var password = $("#password").val();
А в другом месте используются одинарные кавычки:
$.cookie('loggedin', 'yes', { expires: 1 });
Это может показаться неважным, но это, на самом деле, говорит нам о том, что разработчик, возможно, скопировал какой-то код, скажем, со StackOverflow, и при этом даже не переписал его, следуя руководству по стилю, используемому в проекте. Конечно, это — мелочь, но она указывает на то, что разработчик не особо заботится о том, чтобы понять то, как именно работает код. Этому разработчику просто надо, чтобы код хоть как-то функционировал.
Хочу тут пояснить мою точку зрения на эту проблему. Я каждый день ищу в Google что-то, связанное с кодом, но мне кажется, что гораздо важнее понять, например, как установить куки, а не заставить работать бездумно скопированный откуда-то кусок кода. Что если по какой-то причине программа перестанет работать? Как программист, не понимающий того, что происходит в его коде, найдёт ошибку?
Итоги
Я абсолютно уверен в том, что этот фрагмент кода — фейк. Тут я впервые увидел пример синхронного выполнения SQL-запроса:
var accounts = apiService.sql(
"SELECT * FROM users"
);
Обычно подобные задачи решают так:
var accounts = apiService.sql("SELECT * FROM users", (err, res) => {
console.log(err); // ошибка
console.log(res); // результат выполнения запроса
});
Ещё их решают так:
var accounts = await apiService.sql(
"SELECT * FROM users"
);
Даже если метод apiService.sql возвращает результат в синхронном режиме (в чём я сомневаюсь), ему нужно открыть подключение к базе данных, выполнить запрос, отправить в точку вызова результат. А эти операции (как несложно догадаться) не могут выполняться синхронно.
Но если даже это — совершенно реальный код, я уверен, что он был написан начинающим программистом, джуниором. Я уверен, что я, когда работал программистом первые недели, тоже писал ужасный код (прощу прощения :D).
Но это — не ошибка программиста-джуниора.
Представим, что это — настоящий код, который где-то работает. Некий джуниор выложился бы по полной ради того, чтобы этот код заработал. Этот разработчик ещё должен научиться правильному обращению с SQL-запросами, с куки-файлами и со всем остальным. И в таком свете это — совершенно нормально.
А вот начальник этого программиста должен проконтролировать работу, указать на ошибки, добиться от начинающего понимания его ошибок. Это приведёт к тому, что столь ужасный код не попадёт в продакшн.
Я точно знаю, что есть компании, которых не заботит качество кода, уходящего в продакшн. Код решает задачу? Если так — его, без лишних вопросов, развёртывают. Код написан джуниором и даже не проверен более опытным программистом? В продакшн его!
В общем, бывают в жизни огорчения.
Дополнение от 8 августа 2020 года
Я, когда писал эту статью, полагал, что код, который я анализировал, это — фейк. Но после обсуждения материала на Reddit мне дали ссылку на эту ветку. Как оказалось, этот код используется в некоей внутренней системе, поддерживающей работу 1500 пользователей. Поэтому я, очевидно, был не прав. Это — совершенно реальный код.
А вы встречали в продакшне откровенно плохие фрагменты кода, переполненные всяческими проблемами?
Автор: ru_vds
