Код-ревью queries.js — гонки, транзакции, безопасность

Дата: 2026-05-30

🔴 Race condition 1 — обход лимита (TOCTOU, критично)

createEntry: между SELECT COUNT(*) (проверка лимита) и INSERT нет транзакции и блокировки. Два параллельных запроса от одной компании оба прочитают cnt = 14, оба пройдут проверку cnt >= 15, оба вставят запись → 16 записей при лимите 15. То же самое позволяет вставить две пересекающиеся/дублирующие записи одновременно (проверка existing тоже вне транзакции).

Фикс — обернуть всю операцию в транзакцию с блокировкой строки компании (SELECT ... FOR UPDATE сериализует параллельные вставки в рамках одной компании):

async function createEntry(companyId, rawValue, comment, userEmail) {
  const { cidr, wasNormalized } = validate(rawValue);
  const client = await pool.connect();
  try {
    await client.query('BEGIN');
    // блокируем строку компании — параллельные createEntry этой компании встают в очередь
    const company = (await client.query(
      'SELECT * FROM companies WHERE id = $1 FOR UPDATE', [companyId]
    )).rows[0];
    if (!company) throw new Error('Компания не найдена');

    const limit = await getLimit(company);
    const cnt = (await client.query(
      'SELECT COUNT(*)::int AS c FROM whitelist_entries WHERE company_id = $1 AND deleted_at IS NULL',
      [companyId]
    )).rows[0].c;
    if (cnt >= limit) throw new Error(`Лимит исчерпан: ${cnt} из ${limit}`);

    const existing = (await client.query(
      'SELECT value_cidr FROM whitelist_entries WHERE company_id = $1 AND deleted_at IS NULL',
      [companyId]
    )).rows;
    for (const row of existing) {
      if (row.value_cidr === cidr) throw new Error('Такой адрес уже существует');
      if (overlaps(cidr, row.value_cidr))
        throw new Error(`Пересечение с существующей записью ${row.value_cidr}`);
    }

    const res = await client.query(
      `INSERT INTO whitelist_entries (company_id, value_cidr, comment, created_by)
       VALUES ($1, $2, $3, $4) RETURNING *`,
      [companyId, cidr, comment || null, userEmail]
    );
    await logAudit(userEmail, companyId, 'CREATE', null, cidr, res.rows[0].id, client);
    await client.query('COMMIT');
    return { entry: res.rows[0], wasNormalized };
  } catch (e) {
    await client.query('ROLLBACK');
    throw e;
  } finally {
    client.release();
  }
}

🔴 Race condition 2 — то же в updateEntry

updateEntry имеет идентичную проблему: проверка пересечений (existing) и UPDATE не в транзакции. Параллельное обновление двух записей в пересекающиеся CIDR пройдёт обе проверки. Обернуть так же: BEGIN → SELECT ... FOR UPDATE строки компании → проверки → UPDATE → logAudit(...,client) → COMMIT/ROLLBACK.

🔴 Race condition 3 — getOrCreateCompany (дубли компаний)

getOrCreateCompany: между SELECT и INSERT нет защиты. Два первых запроса новой компании оба не найдут строку и оба сделают INSERT. Спасает только UNIQUE на client_id в схеме (второй упадёт), но ошибка вылетит наружу некрасиво. Фикс — атомарный upsert:

async function getOrCreateCompany(clientId, companyName) {
  const res = await pool.query(
    `INSERT INTO companies (client_id, name) VALUES ($1, $2)
     ON CONFLICT (client_id) DO UPDATE SET name = COALESCE(companies.name, EXCLUDED.name)
     RETURNING *`,
    [clientId, companyName || clientId]
  );
  return res.rows[0];
}

🟡 audit_log пишется вне транзакции

logAudit использует глобальный pool, а не клиента транзакции. Если INSERT записи прошёл, а logAudit упал — запись есть, аудита нет (или наоборот при будущих изменениях). Аудит обязателен по ТЗ. Передавать клиента транзакции:

async function logAudit(userEmail, companyId, action, oldValue, newValue, entryId, db = pool) {
  await db.query(
    `INSERT INTO audit_log (user_email, company_id, action, old_value, new_value, entry_id)
     VALUES ($1, $2, $3, $4, $5, $6)`,
    [userEmail, companyId, action, oldValue, newValue, entryId || null]
  );
}

🟡 deleteEntry — UPDATE без company_id в WHERE

await pool.query(
  'UPDATE whitelist_entries SET deleted_by = $1, deleted_at = NOW() WHERE id = $2',
  [userEmail, entryId]
);

old уже проверен по company_id, поэтому изоляция сейчас не нарушается. Но WHERE по одному id хрупкий — при рефакторинге легко потерять привязку. Дублировать company_id в WHERE для глубины защиты:

await pool.query(
  'UPDATE whitelist_entries SET deleted_by = $1, deleted_at = NOW() WHERE id = $2 AND company_id = $3',
  [userEmail, entryId, companyId]
);

Также deleteEntry не в транзакции с logAudit — обернуть аналогично create/update.

🟡 getLimit — custom_limit = 0 игнорируется

return company.custom_limit || defaultLimit;

Если админ задал custom_limit = 0 (запретить компании добавлять), 0 || 15 вернёт 15. ТЗ разрешает снижать лимит. Фикс:

return company.custom_limit != null ? company.custom_limit : defaultLimit;

🟢 SQL-инъекций нет

Все запросы параметризованы ($1, $2...). Конкатенации с пользовательским вводом нет. listEntries/getAudit строят SQL из булевых флагов, не из ввода — безопасно.

🟢 Изоляция по company_id

updateEntry и deleteEntry проверяют company_id при выборке old — пользователь компании А не затронет записи компании Б. Корректно (но см. замечание по deleteEntry WHERE).

Итог: SQL-инъекций и утечек между компаниями нет. Главная проблема — отсутствие транзакций: 3 эксплуатируемые гонки (обход лимита, дубли/пересечения, дубли компаний) + риск рассинхрона аудита. Все чинятся обёрткой в транзакцию с FOR UPDATE и передачей клиента в logAudit. Плюс мелкий баг с custom_limit = 0.

7.3 KiB Raw Blame History