feat!: применять авторизацию и права по спеке, закрыть 5xx и дыры доступа - #18
Merged
Conversation
Правки из рабочего дерева, не связанные с фиксами ниже: мёртвый root.test.js (vitest его не запускал — include только .ts, а маршрута / нет), неиспользуемый openapi() с импортом v2, `make routes` на несуществующий файл, .PHONY, outDir без emit.
@useAuth(BearerAuth) стоял у пяти операций /users, но jwtVerify() в routes/api/users.ts не вызывался ни разу: список, правка и удаление любого пользователя работали без токена (проверено — 200/200/200/204). Спека уже знает, где нужна авторизация, поэтому её применяет glue через securityHandlers: имя обработчика совпадает с именем схемы безопасности. Ручные jwtVerify() из courses и lessons убраны — забыть их больше негде. Заодно удалён декоратор authenticate: он не имел ни одного вызова. Тест перечисляет защищённые операции и проверяет 401 без токена и с битым токеном, а публичные — что они остались доступны.
Две правки в одном коммите намеренно: по отдельности получается состояние, где /tokens отдаёт статус, которого нет в контракте. ensure() вызывал httpErrors.createError, который ошибку только создаёт. 404 не наступал никогда: показ отсутствующей записи отдавал 200 с пустым телом, удаление — 204, а /tokens с неизвестным email падал в 500 при чтении поля у undefined. Сигнатура asserts при этом заставляла tsc ручаться за проверку, которой не происходило. Теперь ensure() бросает, а параметр reply ушёл: httpErrors берётся из @fastify/sensible напрямую. Пароль в /tokens не проверялся вовсе — токен выдавался на любой пароль, потому что колонки для него не было в схеме. Добавлены password_digest, хеширование scrypt из node:crypto (без новых зависимостей) и сверка при выдаче токена. Ответ на неизвестный email и на неверный пароль совпадает, иначе эндпоинт превращается в перебор зарегистрированных адресов. Хеш не должен уезжать в ответ: у User в контракте нет additionalProperties: false, поэтому схема ответа лишнее поле не отсечёт. Запросы к users ограничены публичной проекцией из db/projections.ts. В контракте: password в UserCreateDTO/UserEditDTO (@secret, minLength 8) и UnauthorizedError у tokensCreate. Это ломающее изменение v1 — версия нигде не опубликована, а AuthInfo и так обещал пароль, которого не существовало. Обработчик ошибок теперь рендерит problem+json для всех HTTP-ошибок, а не только для ZodError. Текст 5xx наружу не уходит. Побочно: seed() был асинхронным, но не ожидался в plugins/drizzle.ts — с хешированием это уже настоящая гонка. Миграция добавляет NOT NULL без DEFAULT: на пустой базе это работает, на живой понадобится трёхшаговая (добавить nullable, заполнить, ужесточить).
Обработчики требовали токен, но не смотрели, чей это токен: любой аутентифицированный пользователь правил и удалял чужие курсы и дописывал в них уроки (проверено — 204 на чужом курсе). policies/CoursePolicy при этом был пустым классом без единого вызова. Правка и удаление теперь читают курс до изменения — иначе проверять владельца не на чем, — и отвечают 403. Создание урока проверяет тот же признак по курсу: урок меняет чужой курс. Несуществующий курс там раньше упирался в ограничение внешнего ключа и давал 500, теперь это 404. В контракте у coursesUpdate/coursesDestroy/coursesLessonsCreate появился ForbiddenError — модель была описана в main.tsp, но не использовалась.
updatedAt не обновлялся никогда: обработчики его не писали, а DEFAULT срабатывает только на INSERT. Теперь его ведёт $onUpdate, и он есть у всех трёх таблиц, а не у одних users. createdAt был text с default (unixepoch()), то есть в текстовую колонку клалось число. Тип сменён на integer с mode: timestamp — наружу теперь уходит дата, а не строка вида "1787349516". SELECT'ы в миграции поправлены руками: drizzle-kit сгенерировал перенос updated_at из courses и course_lessons, где такой колонки не было, и миграция падала. Ровно поэтому миграции и не входят в generate-check.
Боевые параметры scrypt — ~230 мс на хеш, а сиды заводят трёх пользователей на каждый build(). Прогон тестов от этого вырос на десятки секунд. Стоимость читается из SCRYPT_COST и хранится внутри дайджеста (scrypt$N$salt$hash), поэтому её смена не обесценивает уже выданные хеши — проверка берёт параметры из самой строки. В vitest выставлено 1024. Заодно include в vitest расширен на .js: раньше .js-тест молча не запускался.
…раница JWT-секрет был зашит в код строкой "supersecret". Теперь конфиг проверяется схемой @fastify/env на старте: без JWT_SECRET длиной от 32 символов приложение не поднимается. plugins/jwt объявляет зависимость от env явно, а не рассчитывает на алфавитный порядок autoload. Добавлены штатные плагины: helmet, cors и rate-limit — все три настраиваются из того же конфига. CSP у helmet выключен: страницу документации он ломает. Документация — @scalar/fastify-api-reference на /docs поверх /openapi.json, который отдаётся из той же спеки, по которой glue регистрирует маршруты. pino-pretty переехал в devDependencies: test/helper.ts настраивает его транспортом, а в package.json его не было — работал только потому, что всплыл в node_modules из fastify-cli. Локально нужен .env, шаблон в .env.example.
release-please — вторая половина того, что уже было настроено: pr-title.yml требовал conventional commit в заголовке PR именно затем, чтобы по нему определялся разряд версии, но выпускать было нечему. make migration-check ловит изменённую схему без миграции: перегенерирует и падает, если что-то появилось. drizzle-kit check вызывается флагами, а не через конфиг — читая drizzle.config.ts, он принимает dialect за параметр AWS Data API и падает. Покрытие с порогами 95/95/80/95 при фактических 100/100/85/97. Тесты собирают приложение напрямую вместо helper из fastify-cli. Тот грузил app.ts в обход трансформации vite, из-за чего app в тестах был any, а покрытие показывало 28% по tokens.ts при четырёх прицельных тестах на него. Реальные цифры видны только теперь. concurrency отменяет прошлый прогон ветки: пуш в ветку с открытым PR запускал сборку дважды. На main не отменяет. deps-update вызывает ncu через pnpm exec: пакет в devDependencies, а npx при его отсутствии тянул бы из сети другую версию.
…ммита
redocly lint встроен в make lint. Он нашёл настоящий разрыв: пять защищённых
операций (все users и coursesCreate) могут ответить 401, но в контракте этого
не было — фикс авторизации сделал разрыв наблюдаемым. Добавлены
UnauthorizedError, summary у всех четырнадцати операций, сервер и лицензия.
Осталось 0 ошибок против 21.
Отключения правил лежат поимённо в redocly.yaml с причинами, а не в
ignore-файле: тот замораживает конкретные находки, и новые такие же проходят
молча. security-defined снят — публичные эндпоинты здесь осознанный дизайн.
oasdiff показывает ломающие правки контракта в PR, но сборку не роняет: v1
нигде не опубликован, и жёсткий гейт обещал бы совместимость, которой
репозиторий не даёт.
lefthook гоняет oxfmt и oxlint по staged-файлам до коммита и tsc перед пушем.
Отдельный nano-staged не нужен — у lefthook есть {staged_files}.
make contract-test поднимает приложение и гоняет по нему schemathesis: тот сам генерирует запросы из OpenAPI и сверяет ответы со спекой. На первом же прогоне 23 падения, все настоящие. Пять 500 на краевых входах: - POST /users с fullName в один символ. Запрос такое принимал, а модель ответа требует от 2 до 100 символов, и валидатор ответа ронял запрос уже после записи в базу. Границы добавлены в UserCreateDTO и UserEditDTO. - GET /courses?page=-1.79e+308. page был numeric без границ, и (page - 1) * perPage переполнялся. Теперь int32 с minValue(1) — заодно перестали приниматься дробные страницы. Идентификаторы в путях тоже int32. - PUT с пустым телом: все поля EditDTO необязательные, а drizzle на пустом set бросает «No values to set». Теперь запись возвращается без изменений. - DELETE пользователя, у которого есть курсы, и курса, у которого есть уроки, — нарушение внешнего ключа. Добавлен каскад: контракт обещает у DELETE только 204. - POST /courses с валидным токеном удалённого пользователя. Токен остаётся подписанным и не истёкшим, запрос доходил до обработчика и падал на внешнем ключе. Проверка существования пользователя добавлена в securityHandlers — туда же, где применяется сама авторизация. Плюс 400 и 429 не были описаны ни у одной операции, хотя их отдают валидация запроса и ограничитель частоты. Добавлены как CommonErrors. На каждую находку есть тест. После правок четыре прогона подряд с разными seed — 1000+ кейсов, ноль падений.
openapi-ts из той же спеки генерирует не только типы обработчиков и zod-схемы, но и типизированный клиент. CRUD-тесты по курсам, пользователям, урокам и токенам переведены на него: URL, методы и формы тел больше не переписываются в тестах руками и не могут разойтись с контрактом. Клиент ходит через fetch, поэтому buildClient() поднимает настоящий сокет на случайном порту. Покрытие от этого не пострадало (100% строк, ветки даже чуть выросли — 86% против 85%), пять прогонов подряд стабильны и в один поток, и с параллелизмом. Тесты на нарушения контракта — страница вне диапазона, слишком короткое имя, запрос без токена — остались на app.inject(): клиент типизирован по спеке и выразить такие запросы не даёт, а проверять их надо. Правило записано в test/helper.ts.
Оба описывали состояние до правок: app.js вместо app.ts, node --test вместо vitest, совет вынести JWT-секрет в окружение, которого уже нет в коде. Не хватало половины целей Makefile, шага с .env и правила, по которому тесты разделены между сгенерированным клиентом и app.inject(). Заодно сужен glob форматтера в lefthook: oxfmt форматирует только ts/js и падает, если в наборе staged-файлов подходящих не осталось — коммит из одних markdown-файлов не проходил.
У всех, у кого .env уже есть, эта команда затирает его содержимое. Добавление строки безопаснее и решает ту же задачу.
Шаг был зелёным только из-за continue-on-error, а на деле падал с «failed to load base spec: /tmp/openapi.base.json: no such file or directory». oasdiff запускается docker-действием, и внутрь контейнера монтируется только workspace — файл из /tmp раннера там не виден. Заодно test -s: без него редирект создавал пустой файл даже при неудачном git show, и fallback не срабатывал.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Зачем
В репозитории контракт первичен, но часть того, что спека уже знает, код
повторял руками — и руками написанная копия разошлась. Все находки ниже
воспроизведены до правки и проверены после.
GET/PUT/DELETE /usersбез токенаGET /users/999999/tokensс неизвестным email/tokensс любым паролемPOST /usersсfullName: "A"GET /courses?page=-1.79e+308PUTс телом{}Первые пять строк — из чтения кода, последние пять нашёл
schemathesis: их невидели ни
tsc, ни валидаторы fastify. На первом прогоне 23 падения, сейчас 0на 1000+ кейсов при разных seed.
Как это больше не повторится
@useAuthиз спеки применяетfastify-openapi-glueчерезsecurityHandlers—
jwtVerify()в обработчиках больше нет, забыть его негде.ensure()бросает ошибку (раньшеcreateErrorтолько создавал её, асигнатура
assertsзаставлялаtscза это ручаться).make contract-test— schemathesis по спеке, отдельным workflow.make lintлинтует контракт: 21 ошибка → 0. Заодно нашёлся настоящий разрыв— пять защищённых операций могли ответить 401, которого не было в спеке.
make migration-check— схема не менялась без миграции.приложение грузилось через хелпер fastify-cli в обход vite. Реальные цифры —
100% строк, 86% ветвей.
release-please— вторая половина того, что уже было настроено:pr-title.ymlтребовал conventional commit именно под него.
lefthookгоняет формат и линт до коммита.остались на
app.inject()— клиент типизирован и выразить их не даёт.Тестов 15 → 45.
Решения, которые стоит обсудить
passwordстал обязательным вUserCreateDTO,pageиid—int32сminValue(1)(page=1.5давал 200, теперь 400).AuthInfoи так обещал пароль, которого не существовало, а@added(Versions.v2)сделал бы фичу мёртвой: приложение работает на v1.ON DELETE CASCADE. Удаление пользователя удаляет его курсы и их уроки.Это делает обещанный контрактом 204 честным, но альтернатива —
задокументированный 409.
securityHandlersстоит одногозапроса к базе на каждый авторизованный запрос.
Прочее
.env:echo "JWT_SECRET=$(openssl rand -hex 32)" >> .env.Без секрета от 32 символов приложение осознанно не поднимается.
serializers/UserSerializer.tsостался мёртвым — его работу забралdb/projections.ts. Не удалял: каталог похож на часть задуманнойархитектуры.
oasdiffпоказывает ломающие правки, но сборку не роняет: v1 нигде неопубликован.
Локально зелёные
make lint,generate-check,migration-check,test-coverage,contract-test. CI-файлы (contract.yml,openapi-diff.yml,release-please.yml) в этом PR выполняются впервые.🤖 Generated with Claude Code