220 lines
18 KiB
Markdown
220 lines
18 KiB
Markdown
# Отчёт по ошибкам и рискам проекта Magistr
|
||
|
||
Дата проверки: 2026-07-03
|
||
|
||
Проверено вручную: backend Spring Boot, мультитенантность, авторизация, генерация расписания, основные frontend JS-файлы. Автотесты не запущены: в окружении нет `mvn` и `./mvnw`.
|
||
|
||
## Нужно исправить в первую очередь
|
||
|
||
### 1. Высокая — JWT имеет небезопасные production-дефолты
|
||
|
||
**Файлы:**
|
||
- `backend/src/main/java/com/magistr/app/config/auth/JwtProperties.java:13-17`
|
||
- `backend/src/main/resources/application.properties:18-22`
|
||
- `backend/src/main/java/com/magistr/app/controller/AuthController.java:162-168`
|
||
|
||
**Проблема:** если на проде не задан `JWT_SECRET`, приложение использует известный дефолтный секрет `dev-only-change-this-jwt-secret-32-bytes-minimum`. При этом `JWT_REFRESH_COOKIE_SECURE` по умолчанию `false`, и refresh-cookie может уходить без флага `Secure`.
|
||
|
||
**Риск:** подделка access-токенов при известном секрете; утечка refresh-cookie при ошибочной HTTP/прокси-конфигурации.
|
||
|
||
**Как исправить:**
|
||
- На production-профиле падать при отсутствии `JWT_SECRET` и при секрете короче требуемой длины.
|
||
- Для production сделать `app.jwt.refresh-cookie-secure=true` обязательным.
|
||
- Разделить dev/prod профили: дефолтный секрет оставить только в `application-dev.properties`.
|
||
|
||
---
|
||
|
||
### 2. Высокая — race condition при ротации refresh-токена
|
||
|
||
**Файл:** `backend/src/main/java/com/magistr/app/config/auth/RefreshTokenService.java:50-78`
|
||
|
||
**Проблема:** `rotate()` читает refresh-токен через `findByTokenHash()`, проверяет активность, затем отзывает старый и создаёт новый. Нет блокировки строки или атомарного `UPDATE ... WHERE revoked_at IS NULL`. Два параллельных запроса с одним refresh-токеном могут оба пройти проверку и создать две активные сессии.
|
||
|
||
**Риск:** повторное использование одного refresh-токена, раздвоение сессий, некорректная ротация `rotatedToTokenHash`.
|
||
|
||
**Как исправить:**
|
||
- Добавить pessimistic lock в репозиторий (`@Lock(PESSIMISTIC_WRITE)` для поиска по `tokenHash`) внутри транзакции.
|
||
- Либо сделать атомарное обновление: `UPDATE auth_refresh_tokens SET revoked_at = now, rotated_to_token_hash = :newHash WHERE token_hash = :hash AND revoked_at IS NULL AND expires_at > now` и создавать новый токен только если обновлена 1 строка.
|
||
- Добавить тест на два параллельных refresh-запроса.
|
||
|
||
---
|
||
|
||
### 3. Высокая — `AuthContext` может протечь между запросами при `403`
|
||
|
||
**Файл:** `backend/src/main/java/com/magistr/app/config/auth/AuthorizationInterceptor.java:35-49`
|
||
|
||
**Проблема:** пользователь кладётся в `AuthContext` на строке 45, а при нехватке роли метод возвращает `false` на строке 49. В таком сценарии `afterCompletion()` текущего interceptor'а может не выполниться, и `ThreadLocal` останется в потоке до следующей очистки.
|
||
|
||
**Риск:** утечка контекста пользователя между запросами в servlet thread pool. Сейчас это в основном риск безопасности и будущих багов, но его лучше устранить сразу.
|
||
|
||
**Как исправить:** перед каждым `return false` после `AuthContext.setCurrentUser(user)` вызывать `AuthContext.clear()`. Ещё лучше — не устанавливать `AuthContext`, пока не пройдена ролевая проверка, или обернуть обработку отказов в безопасный helper.
|
||
|
||
---
|
||
|
||
### 4. Высокая — точечные изменения расписания не проверяются на конфликты
|
||
|
||
**Файл:** `backend/src/main/java/com/magistr/app/controller/ScheduleOverrideController.java:71-127`
|
||
|
||
**Проблема:** при `MOVE`/`REPLACE` валидируется только существование нового преподавателя, аудитории и слота. Не проверяется, свободны ли преподаватель/аудитория/группа в выбранную дату и пару.
|
||
|
||
**Риск:** учебный отдел может создать override, который назначит преподавателя или аудиторию на две пары одновременно. Основные правила расписания конфликты проверяют, а override — нет.
|
||
|
||
**Как исправить:**
|
||
- Перед сохранением override построить расписание на `lessonDate` и проверить занятость нового `timeSlot`, `teacher`, `classroom`, групп/подгрупп базового слота.
|
||
- Переиспользовать/вынести конфликтную логику из `ScheduleRuleAdminController` в сервис.
|
||
- Вернуть `409 Conflict` с описанием конфликтующего занятия.
|
||
|
||
---
|
||
|
||
### 5. Средняя/высокая — расписание преподавателя не показывает пары, где он назначен через override
|
||
|
||
**Файлы:**
|
||
- `backend/src/main/java/com/magistr/app/service/ScheduleQueryService.java:47-58`
|
||
- `backend/src/main/java/com/magistr/app/repository/ScheduleRuleRepository.java:36-54`
|
||
|
||
**Проблема:** если поиск выполняется только по `teacherId`, сервис строит расписание через `buildScheduleForTeacher(teacherId)`, то есть берёт только базовые правила, где преподаватель указан в слоте. Если override заменил преподавателя на этого teacherId, базовое правило в выборку не попадёт, и после `applyOverrides()` такая пара не появится.
|
||
|
||
**Риск:** преподаватель не увидит замену/переназначенную ему пару в своём расписании.
|
||
|
||
**Как исправить:**
|
||
- При поиске по `teacherId` дополнительно учитывать overrides с `newTeacher.id = teacherId` за период.
|
||
- Либо строить расписание по группам для затронутых override-слотов и после применения override фильтровать по преподавателю.
|
||
- Добавить тест: базовый преподаватель A, override `newTeacher=B`, поиск по `teacherId=B` должен вернуть пару.
|
||
|
||
---
|
||
|
||
### 6. Средняя — обновление существующего тенанта может удалить рабочее подключение и сохранить нерабочее
|
||
|
||
**Файлы:**
|
||
- `backend/src/main/java/com/magistr/app/controller/DatabaseController.java:104-116`
|
||
- `backend/src/main/java/com/magistr/app/config/tenant/TenantRoutingDataSource.java:135-148`
|
||
- `backend/src/main/java/com/magistr/app/config/tenant/TenantConfigWatcher.java:147-158`
|
||
|
||
**Проблема:** при добавлении тенанта с уже существующим доменом старый DataSource сначала удаляется и закрывается (`removeTenant()`), затем добавляется новый. Hikari настроен с `setInitializationFailTimeout(-1)`, поэтому невалидная БД может быть добавлена без ошибки. `initDatabaseForTenant()` ловит исключения Flyway и не пробрасывает их наружу, поэтому API может вернуть успех и записать нерабочий конфиг.
|
||
|
||
**Риск:** админ одной ошибкой в JDBC URL/пароле может выключить рабочий tenant и разнести нерабочую конфигурацию через ConfigMap.
|
||
|
||
**Как исправить:**
|
||
- Сначала создать и проверить новый DataSource во временном объекте (`testConnection`, Flyway migrate) и только после успеха атомарно заменить старый.
|
||
- `initDatabaseForTenant()` должен возвращать результат или бросать исключение, чтобы `DatabaseController` не писал невалидный ConfigMap.
|
||
- При ошибке оставлять старое подключение активным.
|
||
|
||
---
|
||
|
||
### 7. Средняя — watcher не применяет изменения URL/логина/пароля существующего тенанта
|
||
|
||
**Файл:** `backend/src/main/java/com/magistr/app/config/tenant/TenantConfigWatcher.java:102-125`
|
||
|
||
**Проблема:** `syncTenants()` добавляет только новые домены и удаляет исчезнувшие. Если в `tenants.json` изменились `url`, `username` или `password` для уже существующего `domain`, текущий DataSource не заменяется.
|
||
|
||
**Риск:** после обновления ConfigMap часть pod'ов продолжит ходить в старую БД/со старыми credentials до рестарта.
|
||
|
||
**Как исправить:** сравнивать весь `TenantConfig` для существующих доменов. При отличии — безопасно пересоздавать DataSource после успешной проверки нового подключения.
|
||
|
||
---
|
||
|
||
### 8. Средняя — отключена проверка TLS-сертификата Kubernetes API
|
||
|
||
**Файл:** `backend/src/main/java/com/magistr/app/config/tenant/ConfigMapUpdater.java:74-76,104-119`
|
||
|
||
**Проблема:** `createInsecureClient()` доверяет любому сертификату (`checkServerTrusted` пустой). Комментарий объясняет это self-signed CA, но в Kubernetes правильный CA доступен в serviceaccount volume.
|
||
|
||
**Риск:** MITM внутри сети кластера может подменить Kubernetes API и получить serviceaccount token/подменить ConfigMap.
|
||
|
||
**Как исправить:** использовать CA из `/var/run/secrets/kubernetes.io/serviceaccount/ca.crt` и стандартную проверку hostname/cert chain. Не использовать trust-all клиент.
|
||
|
||
---
|
||
|
||
### 9. Средняя — кабинет кафедры может перезаписать дисциплину другой кафедры при импорте
|
||
|
||
**Файл:** `backend/src/main/java/com/magistr/app/controller/DepartmentWorkspaceController.java:80-90`
|
||
|
||
**Проблема:** импорт ищет дисциплину только по глобальному имени (`findByName`) и затем без проверки меняет `departmentId` на кафедру текущего пользователя. Если дисциплина с таким названием уже принадлежит другой кафедре, она будет переназначена.
|
||
|
||
**Риск:** потеря принадлежности дисциплины и связей расписания/календарей у другой кафедры.
|
||
|
||
**Как исправить:**
|
||
- Не менять `departmentId` существующей дисциплины другой кафедры.
|
||
- Сделать уникальность по `(department_id, lower(name))`, если одинаковые названия допустимы у разных кафедр.
|
||
- При конфликте возвращать понятную ошибку или создавать отдельную запись в рамках кафедры.
|
||
|
||
---
|
||
|
||
### 10. Средняя — кафедра может смотреть workload по чужим кафедрам
|
||
|
||
**Файл:** `backend/src/main/java/com/magistr/app/controller/WorkloadController.java:43-130`
|
||
|
||
**Проблема:** endpoints `/api/workload/*` доступны роли `DEPARTMENT` и принимают произвольный `departmentId`, но не сверяют его с `AuthContext.departmentId`. Также при `departmentId` отсутствующем запрос строится по всем группам/кафедрам.
|
||
|
||
**Риск:** оператор кафедры может получить загруженность преподавателей/аудиторий/кафедр за другие подразделения, если это не было задумано бизнес-правилами.
|
||
|
||
**Как исправить:** для роли `DEPARTMENT` принудительно использовать `AuthContext.getCurrentUser().departmentId()` и игнорировать/запрещать чужой `departmentId`. Аналогично проверить `ScheduleSearchController`, если расписание не должно быть глобально видимым.
|
||
|
||
---
|
||
|
||
### 11. Средняя — семестры можно создать вне учебного года или с пересечениями
|
||
|
||
**Файлы:**
|
||
- `backend/src/main/java/com/magistr/app/controller/AcademicCalendarAdminController.java:95-129,201-211`
|
||
- `backend/src/main/java/com/magistr/app/service/AcademicDateService.java:32-34`
|
||
- `backend/src/main/java/com/magistr/app/repository/SemesterRepository.java:13`
|
||
|
||
**Проблема:** при создании/обновлении семестра проверяется только `endDate >= startDate`. Нет проверки, что семестр лежит внутри своего учебного года и не пересекается с другим семестром. При этом расписание ищет `findFirstByStartDateLessThanEqualAndEndDateGreaterThanEqual()`, то есть при пересечении дата будет сопоставляться с произвольным первым семестром.
|
||
|
||
**Риск:** генерация расписания и чётность недель будут работать некорректно на пересекающихся датах.
|
||
|
||
**Как исправить:**
|
||
- Валидировать границы семестра относительно `AcademicYear.startDate/endDate`.
|
||
- Запрещать пересечение семестров в рамках учебного года.
|
||
- Желательно добавить DB-level exclusion constraint/range constraint или хотя бы сервисную проверку и тест.
|
||
|
||
---
|
||
|
||
### 12. Низкая/средняя — access-токен хранится в `localStorage`
|
||
|
||
**Файл:** `frontend/admin/js/api.js:5-7,155-163`
|
||
|
||
**Проблема:** access JWT хранится в `localStorage`, откуда его может украсть любой XSS в админке. Refresh-токен уже вынесен в HttpOnly cookie, но access-токен остаётся доступным JS.
|
||
|
||
**Риск:** при XSS злоумышленник получает Bearer token и может выполнять API-запросы до истечения TTL.
|
||
|
||
**Как исправить:**
|
||
- По возможности держать access token в памяти и обновлять через HttpOnly refresh cookie.
|
||
- Усилить CSP (`script-src` без inline), запретить небезопасные вставки HTML.
|
||
- Провести отдельный XSS-аудит всех `innerHTML`.
|
||
|
||
---
|
||
|
||
### 13. Низкая — frontend местами вставляет текст ошибки через `innerHTML` без экранирования
|
||
|
||
**Файл:** `frontend/admin/settings/js/views/database.js:37-49`
|
||
|
||
**Проблема:** `e.message` вставляется в `innerHTML` без `escapeHtml()`. Сейчас большинство серверных ошибок фиксированные, но часть ошибок может включать текст из внешних систем/драйверов.
|
||
|
||
**Риск:** потенциальный XSS в настройках БД при попадании HTML в сообщение ошибки.
|
||
|
||
**Как исправить:** заменить на `textContent` или оборачивать `escapeHtml(e.message)`.
|
||
|
||
---
|
||
|
||
### 14. Низкая — backend image скачивает OpenTelemetry javaagent по `latest`
|
||
|
||
**Файл:** `backend/Dockerfile:7`
|
||
|
||
**Проблема:** сборка всегда скачивает `latest` javaagent с GitHub без pin версии/checksum.
|
||
|
||
**Риск:** невоспроизводимые сборки; внезапные несовместимости; supply-chain риск.
|
||
|
||
**Как исправить:** закрепить конкретную версию javaagent и проверять checksum.
|
||
|
||
## Дополнительно проверить после исправлений
|
||
|
||
1. Добавить Maven Wrapper (`mvnw`), чтобы проверки запускались одинаково в CI и локально.
|
||
2. Прогнать `mvn test` и добавить тесты на:
|
||
- параллельную ротацию refresh-токена;
|
||
- override с конфликтом преподавателя/аудитории;
|
||
- расписание преподавателя, назначенного через override;
|
||
- обновление существующего tenant config;
|
||
- запрет чужого `departmentId` для роли `DEPARTMENT`.
|
||
3. Не изменять существующие Flyway-миграции. Если нужны DB constraints/indexes — добавлять новую миграцию `V3__...sql`.
|