Ревью кода на безопасность: чек-лист для pull request

Уязвимость часто появляется не в отдельной «опасной» строке, а в пропущенной проверке между слоями: пользователь передал идентификатор, сервер не проверил право доступа, а запрос вернул чужие данные. Этот чек-лист поможет провести ревью pull request по конкретным сценариям, а не по впечатлению от кода. Он не привязан к языку или фреймворку; ориентир по требованиям — OWASP ASVS 4.0.3, не утверждение о последней версии стандарта.

Сначала определите, что изменилось с точки зрения риска

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

• Какие новые входы появились: запрос, файл, webhook, сообщение очереди, импорт или переменная окружения?
• Какие действия добавлены или изменены: чтение, создание, изменение и удаление данных, экспорт, платёж, запуск фоновой задачи?
• Кто может вызвать этот путь и от чьего имени он выполняется?
• Какие данные стали доступнее, куда они записываются и кто может увидеть ошибки или логи?
• Менялись ли схема базы, права сервисной учётной записи, зависимости, конфигурация или инфраструктура?

Если на эти вопросы нельзя ответить по описанию PR, попросите автора указать поток данных и границы доступа. Иначе ревьюер вынужден угадывать, а пропущенное условие превращается в ложное одобрение.

Проверяйте не только то, что код делает при штатном запросе, но и то, что он разрешает сделать чужому пользователю, повреждённому сообщению или сбою зависимости.

Выберите глубину проверки

Одинаково тщательно разбирать каждую строку неэффективно. Выберите режим по последствиям изменения и поверхности атаки, а не по размеру diff.

На практике начинайте с diff, затем расширяйте проверку до полного потока, если изменение пересекает границу доверия. Для критичного действия задайте отдельный сценарий злоупотребления. Например: пользователь подменяет идентификатор заказа, повторяет запрос или вызывает операцию без нужной роли.

Чек-лист для pull request

1. Входные данные и границы доверия

• Проверьте, что данные валидируются на сервере. Проверка только в браузере не ограничивает запросы, отправленные напрямую.
• Уточните, какие значения допустимы: формат, длина, диапазон, обязательность и взаимосвязь полей. Проверка «поле не пустое» обычно не отвечает на вопрос, безопасно ли использовать значение дальше.
• Проследите, попадает ли ввод в SQL, шаблон, shell-команду, путь к файлу, запрос к внешнему URL или десериализатор. Для каждого такого перехода ищите подходящий безопасный механизм работы с данными, а не ручную склейку строки.
• Для загрузок проверьте ограничения размера и типа, место хранения и запрет на исполнение загруженного содержимого. Для URL, которые запрашивает сервер, проверьте ограничение адресов назначения: иначе функция может стать способом обращаться к внутренним ресурсам.

2. Аутентификация и авторизация

• Проверьте не только наличие сессии, но и право именно на запрошенный объект. Успешный вход не означает право читать любой заказ или менять любую запись.
• Проверьте права в серверном слое, который выполняет действие. Скрытая кнопка или проверка только на клиенте не защищает endpoint и фоновый обработчик.
• Убедитесь, что операции изменения состояния защищены от повторного вызова или подделки запроса там, где это важно для приложения.
• Сравните поведение для владельца объекта, другого пользователя и пользователя без нужной роли. Ответ не должен случайно раскрывать лишние данные.

3. Секреты, ошибки и логи

• Поищите ключи, пароли, токены и приватные сертификаты в diff, тестовых данных и конфигурации. Секреты должны поступать из принятого в проекте защищённого хранилища, а не из исходников.
• Проверьте, не попадают ли токены, пароли, персональные данные и содержимое запросов в логи, сообщения об ошибках и аналитику.
• Убедитесь, что внешний ответ не показывает трассировку, внутренние пути и детали инфраструктуры. При этом ошибка должна оставаться диагностируемой по защищённым серверным логам.
• Проверьте отказ внешней зависимости: таймаут, недоступность и некорректный ответ не должны приводить к обходу проверки доступа или выдаче данных не тому пользователю.

4. Зависимости и побочные эффекты

• Посмотрите, зачем добавили новую библиотеку, кто её поддерживает и какие версии фиксируются проектом. Уточните, не попала ли в обновление транзитивная зависимость с отдельным риском.
• Проверьте миграции и фоновые задачи: кому выданы права на запись, можно ли безопасно повторить задачу, что произойдёт при частичном сбое.
• Найдите соответствующие тесты на отказ в доступе, неверный ввод и ошибку зависимости. Тест только на успешный сценарий не доказывает, что новая граница защищена.

Что часто ломает хорошее ревью

• Проверка только изменённых строк. Уязвимый вызов может находиться в соседнем методе, который теперь получает недоверенные данные. Проследите путь до источника и места использования.
• Доверие к типу пользователя. Роль вроде «менеджер» не отвечает на вопрос, имеет ли человек доступ к конкретному объекту. Проверяйте объектное разрешение.
• Проверка только «чистого» пути. Добавьте в мысленную проверку пустое, слишком длинное и неожиданное значение, повторный запрос и сбой внешнего сервиса.
• Зелёные автоматические проверки как доказательство безопасности. Анализатор может подсветить подозрительный шаблон, но не всегда понимает бизнес-права и контекст. Результат нужно проверить вручную.
• Замечания без сценария. Вместо «проверьте безопасность» опишите путь: «пользователь может заменить ID в запросе и получить чужой объект; добавьте серверную проверку владельца и тест на чужой ID».

Граница доверия — место, где данные или запрос переходят от менее доверенного источника к более привилегированному компоненту: например, от браузера к серверу или от внешнего webhook к обработчику. Именно на таких переходах особенно важно проверять ввод и полномочия.

Когда подключать дополнительные проверки

Для обычного изменения часто достаточно проверки diff и связанных тестов. Расширьте ревью, если PR добавляет новый источник ввода, работает с чувствительными данными, меняет роли или затрагивает платежи. Если последствия ошибки велики, одного просмотра кода может быть мало: добавьте тест угроз, проверку конфигурации и анализ зависимостей.

Практический ориентир — не число замечаний, а проверяемые сценарии. Для каждого важного риска зафиксируйте: кто может его вызвать, какое условие должно остановить действие и каким тестом это подтверждается. Требования OWASP ASVS 4.0.3 можно использовать как основу для контроля покрытия, но конкретные проверки зависят от архитектуры и стека.

Что делать дальше

Возьмите следующий PR, который меняет входные данные или права доступа, и пройдите по списку сверху вниз. В описании изменения попросите автора указать затронутые данные, роли и негативные сценарии; в ревью оставляйте замечания с конкретным путём атаки и ожидаемым тестом. После релиза следите за ошибками авторизации, отказами внешних зависимостей и появлением чувствительных данных в логах. Проверку конкретных API и защитных механизмов сверяйте с документацией используемого языка, фреймворка и версии.