# Code Review: Lab7-Prog

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

---

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

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

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

```kotlin
// Было (строка 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`.

```kotlin
// Было (строки 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`) или конфигурационный файл. Генерировать криптостойкий ключ:

```kotlin
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 и прочитать/изменить все данные.

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

```kotlin
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?`:

```kotlin
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` со всех серверных команд + мёртвый код