Lab8-Prog/code-review.md

Code Review: Lab7-Prog

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

---

🔴 CRITICAL — Требует немедленного исправления

1. server/.../commands/RemoveByIdCommand.kt:36 — Команда remove_by_id полностью сломана

Проблема: repository.deleteByOwner(id) вызывается с ID элемента, а не с ID владельца.

// Было (строка 36):
repository.deleteByOwner(id) // id — UUID элемента, а не владельца!

// Должно быть:
repository.deleteByOwner(ownerId)

Почему плохо: В БД ищутся записи, где owner_id == uuid_элемента. UUID элемента никогда не совпадёт с UUID владельца, поэтому deleteByOwner всегда возвращает 0 удалённых строк. Команда никогда ничего не удаляет.

Как исправить: Заменить id на ownerId (или existing.ownerId).

---

2. server/.../database/Repository.kt:120-124 — Обновление CarTable использует ID координат вместо ID машины

Проблема: При обновлении записи в CarTable используется UUID из колонки coordinates, а не из колонки car.

// Было (строки 120-121):
val cordsInBeing = row[HumanBeingTable.coordinates]  // это UUID координат!
CarTable.update({ CarTable.id eq cordsInBeing })      // но мы ищем Car по этому UUID!

// Должно быть:
val carId = row[HumanBeingTable.car]  // UUID машины
CarTable.update({ CarTable.id eq carId }) {
    it[CoolTable.cool] = item.car.cool
}

Почему плохо: Поле cool в машине никогда не обновляется. Вместо этого модифицируется (или создаётся) запись в CarTable с UUID, который совпадает с UUID координат. Это портит данные.

Как исправить: Использовать row[HumanBeingTable.car] вместо cordsInBeing.

---

3. server/.../auth/JWTManager.kt:14 — Секрет JDT — "random"

Проблема: Ключ подписи JWT — строка "random", которая жёстко зашита в коде.

Почему плохо: Любой, кто прочитает код, может подделать JWT-токен и войти под любым пользователем. Это полная компрометация аутентификации.

Как исправить: Вынести в переменную окружения (JWT_SECRET) или конфигурационный файл. Генерировать криптостойкий ключ:

val secret = System.getenv("JWT_SECRET") ?: error("JWT_SECRET not set")

---

4. server/.../database/DatabaseManager.kt:15-18 — Учётные данные БД жёстко зашиты в коде

Проблема: jdbcUrl, username = "mainUser", password = "superDuperPassword" в открытом виде.

Почему плохо: Адрес сервера (aklivtsov.tech:5430) и пароль доступны любому, у кого есть доступ к репозиторию. Злоумышленник может подключиться напрямую к PostgreSQL и прочитать/изменить все данные.

Как исправить: Использовать переменные окружения:

val dbUrl = System.getenv("DB_URL") ?: "jdbc:postgresql://localhost:5432/prog7_db"
val dbUser = System.getenv("DB_USER") ?: error("DB_USER not set")
val dbPass = System.getenv("DB_PASS") ?: error("DB_PASS not set")

---

🟠 HIGH — Критические баги и проблемы безопасности

5. common/.../models/HumanBeing.kt:27 — id объявлен как var в data class

Проблема: var id: UUID? = null — мутабельное поле в data class. Команда AddCommand изменяет его после создания: secured.id = repository.write(secured).

Почему плохо: Если объект с id=null добавлен в коллекцию, а потом id меняется, это ломает контракты hashCode()/equals() и TreeSet. Объект может "потеряться" в коллекции.

Как исправить: Сделать id иммутабельным. Использовать copy(id = ...) вместо прямой мутации.

---

6. common/.../models/HumanBeing.kt:50-53 — Сравнение UUID как строк

Проблема: this.id.toString().compareTo(other.id.toString()) — лексикографическое сравнение строковых представлений UUID.

Почему плохо: UUID "10000000-..." будет меньше "99999999-..." как строка, хотя как число он больше. Порядок сортировки сломан.

Как исправить: Использовать this.id!!.compareTo(other.id!!) (сравнение по битовому представлению UUID) или обработать null.

---

7. common/.../models/HumanBeing.kt:65 — NPE при hashCode() с null id

Проблема: id.hashCode() — на nullable типе это эквивалентно id!!.hashCode(). Если id == null — будет NullPointerException.

Как исправить: id?.hashCode() ?: 0

---

8. server/.../collection/CollectionManager.kt:34-43 — Гонки данных (data race)

Проблема:

• removeById(): getById() внутри synchronized, но сам collection.remove() — вне синхронизации.
• clear(): вызывается вообще без синхронизации.

Почему плохо: При конкурентном доступе: ConcurrentModificationException, потеря данных, чтение несогласованного состояния.

Как исправить: Все публичные методы должны быть обёрнуты в synchronized(collection) { ... }.

---

9. server/.../commands/RemoveByIdCommand.kt:33 — Сравнение UUID как строк (2)

Проблема: existing.ownerId.toString() != userId — строковое сравнение. userId из JWT совпадает по смыслу, но типобезопасность потеряна.

Как исправить: Хранить userId как UUID и сравнивать: existing.ownerId == UUID.fromString(userId).

---

10. client/.../network/NetworkManager.kt:77 — " no id" с пробелом

Проблема: private var id: String = " no id" — если getUUID() вызван до аутентификации, UUID.fromString(" no id") упадёт с IllegalArgumentException.

Как исправить: Использовать null и UUID?:

private var id: UUID? = null
fun getUUID(): UUID = id ?: error("Not authenticated")

---

11. client/.../network/NetworkManager.kt:78 — Не проверяется возврат DatagramChannel.write()

Проблема: Метод write() возвращает количество записанных байт. Для UDP при превышении MTU часть данных может быть отброшена, но код не проверяет, что все байты отправлены.

Как исправить: Использовать DatagramChannel.send() с готовым DatagramPacket, или проверять возвращаемое значение.

---

12. client/.../input/IOManager.kt:17-19 — Падение при отсутствии терминала

Проблема: TerminalBuilder.builder().system(true).build() упадёт с исключением, если stdin не терминал (CI/CD, пайп, Docker без TTY).

Как исправить: Обработать исключение и упасть на Scanner(System.in).

---

13. client/.../commands/AuthCommand.kt:19-20 — ArrayIndexOutOfBoundsException

Проблема: Проверяется только args.isEmpty(), но не args.size < 3. Если пользователь введёт auth login, то args = ["login"] (size = 1), и args[1] вызовет crash.

Как исправить: args.size < 3 → вернуть ошибку.

---

14. client/.../commands/ExecuteScriptCommand.kt:13 — Та же проблема

Проблема: args[0] без проверки args.isNotEmpty().

Как исправить: Проверить args.isNotEmpty().

---

15. server/.../commands/SyncCommand.kt:41 — Обсценная лексика в ответе сервера

Проблема: message = "хуй" — нецензурное выражение возвращается клиенту.

Почему плохо: Неприемлемо для любого кода, даже учебного. Это будет видно пользователю при синхронизации команд.

Как исправить: Заменить на "Commands synchronized" или "Команды синхронизированы".

---

16. client/.../commands/SyncCommand.kt:24 — Отладочный print(res) в продакшене

Проблема: В коде остался print(res), который выводит сырой Response-объект в stdout.

Как исправить: Удалить строку.

---

17. server/.../collection/CollectionManager.kt:34-38 — TOCTOU race в removeById

Выше описано (#8), но это отдельный опасный случай: между проверкой getById() и удалением коллекция может измениться.

---

18. server/.../database/tables/UserTable.kt:7 — Комментарий // change про пароль

Автор сам знает, что пароль хранится небезопасно (MD5 без соли). MD5 неприемлем для хранения паролей — взламывается миллиардами хэшей в секунду на обычном GPU.

Как исправить: Использовать bcrypt/Argon2.

---

19. client/.../commands/AuthCommand.kt:38-41 — MD5 + pass-the-hash

Проблема: MD5-хэш отправляется по сети и используется сервером как пароль. Перехватчик трафика получает готовый хэш, который можно просто отправить серверу (pass-the-hash).

Как исправить: Использовать TLS/DTLS для транспорта. Сервер должен хэшировать пароль повторно с солью.

---

20. gateway/.../core/GatewayManager.kt:55-64, 120-127 — Утечка сокетов

Проблема: При ошибке в probeServer() или ProxyTask.compute() DatagramSocket не закрывается. При каждом неудачном health check утекает файловый дескриптор.

Как исправить: Использовать try {} finally { socket.close() } или use {}.

---

🟡 MEDIUM — Важно, но не критично

#	Файл	Строка	Проблема	Исправление
21	server/.../NetworkManager.kt:24-25	Использование ForkJoinPool для I/O	Заменить на ExecutorService/thread pool
22	server/.../NetworkManager.kt:88	Ошибка сервера возвращает e.message клиенту	Логировать на сервере, клиенту — "Внутренняя ошибка"
23	server/.../commands/UpdateCommand.kt:38	ownerId в теле запроса может отличаться от JWT (теоретически)	Принудительно устанавливать ownerId из JWT в команде
24	server/.../repository/Repository.kt:146-156	Мёртвый метод delete(id: UUID) — нигде не вызывается	Удалить, или реализовать корректно (с удалением coordinates/car)
25	server/.../repository/Repository.kt:162	Пароль сравнивается напрямую в SQL-запросе	Использовать server-side hashing с солью
26	server/.../database/DatabaseManager.kt:44-54	Мёртвый метод test()	Удалить
27	server/.../database/tables/CoordinatesTable.kt:5-6	Колонки NULLABLE в БД, NON-NULL в модели	Добавить NOT NULL в DDL
28	server/.../database/tables/CarTable.kt:6		То же
29	server/.../runner/CommandInvoker.kt:51-52	getHistory() не синхронизирован	Синхронизировать или использовать CopyOnWriteArrayList
30	server/.../commands/ClearCommand.kt:26-27	TOCTOU между getAll() и итерацией	Блокировать коллекцию на всё время операции
31	server/.../auth/JWTManager.kt:38-49	validateJWT не использует параметр userId	Проверять, что subject токена == ожидаемому userId
32	client/.../runner/CommandInvoker.kt:45-47	Ловится только IllegalStateException	Добавить общий catch для непредвиденных ошибок
33	client/.../runner/CommandInvoker.kt:7	Импортировано 2 разных logging-библиотеки	Оставить только io.github.oshai.kotlinlogging
34	client/.../input/IOManager.kt:15	isInteractive всегда true	Отключать интерактивный режим при execute_script
35	client/.../input/HumanBeingBuilder.kt:65-165	Массовое дублирование в read-методах	Вынести в generic-функцию
36	client/.../input/HumanBeingBuilder.kt:171-173	Мёртвый метод printPrompt	Удалить или использовать
37	gateway/.../core/GatewayManager.kt:55,120	Создание нового DatagramSocket на каждый запрос	Переиспользовать сокеты
38	gateway/.../core/GatewayManager.kt:135-142	Бесконечные ретраи при флапе бэкендов	Добавить лимит retry

---

🟢 LOW — Косметика, стиль, конвенции

#	Файл	Строка	Проблема	Исправление
39	common/.../network/AppJson.kt:5	AppJson — нейминг нарушает конвенции Kotlin	APP_JSON (UPPER_SNAKE_CASE)
40	common/.../network/AuthResponse.kt:6	Лишний пробел в class AuthResponse (	class AuthResponse(
41	common/.../models/Coordinates.kt:11-12	Пустая строка между KDoc и аннотацией	Убрать пустую строку
42	common/.../commands/CommandArgType.kt:5	Комментарий // why there's only two types? yes.	Удалить
43	server/.../Main.kt:28	Комментарий // let's pretend that i'm in	Удалить
44	client/.../Main.kt:16	Комментарий // Name by Boris Bosenko	Удалить
45	client/.../Main.kt:47	Пропущен пробел после запятой	invoker: CommandInvoker, client: NetworkManager
46	client/.../input/IOManager.kt:73	print() вызывает println()	Переименовать или исправить
47	client/.../input/IOManager.kt:27	Закомментированный код	Удалить или раскомментировать
48	server/.../commands/AddIfMaxCommand.kt:13	val manager не private	Добавить private
49	server/.../commands/ClearCommand.kt:14	var repository вместо val	Заменить на val
50	Все server/commands	@Serializable + @Contextual на классах команд — не используются	Удалить (минимум 10 файлов)
51	Везде	Нет форматтера (ktlint, spotless)	Добавить плагин в Gradle
52	Везде	Нет тестов	Написать хотя бы unit-тесты на команды
53	gateway/.../core/GatewayManager.kt:73	AtomicInteger переполнится через ~2 млрд запросов	Использовать AtomicLong или обнуление

---

📋 Итого по категориям

Категория	Кол-во	Самые важные
🔴 Critical	4	RemoveByIdCommand сломан (#1), CarTable не обновляется (#2), JWT secret (#3), credentials в коде (#4)
🟠 High	16	var id (#5), сравнение UUID (#6), NPE в hashCode (#7), data races (#8), утечка сокетов (#20), мат в ответе (#15), MD5 (#14)
🟡 Medium	18	Мёртвый код, ForkJoinPool для I/O, утечка деталей в ошибках, отсутствие тестов
🟢 Low	15	Косметика, нейминг, комментарии, стиль

---

💡 Рекомендации по очерёдности исправления

1. Сначала: Critical #1, #2 — баги, делающие команды нерабочими
2. Сразу после: Critical #3, #4 + High #14, #15 — безопасность
3. Затем: High #5, #6, #7, #8 — стабильность и корректность данных
4. Потом: Остальные Medium/Low — по желанию

Топ-5 исправлений, которые дадут 80% улучшения:

1. RemoveByIdCommand.kt:36 — repository.deleteByOwner(id) → repository.deleteByOwner(ownerId)
2. Repository.kt:120-124 — заменить cordsInBeing на carId
3. Вынести секреты (JWT, БД) в переменные окружения
4. Синхронизировать CollectionManager (removeById, clear, size)
5. Удалить @Serializable / @Contextual со всех серверных команд + мёртвый код