Ревью backend-кода: архитектура и безопасность

Ревью routes/orders.js

1. Резюме

Код в текущем виде нельзя выпускать. В нём есть SQL-инъекция, нет проверки ролей и региона, нет транзакции при создании заказа, а цену товара задаёт клиент. Обе жалобы из контекста объясняются этим кодом:

• чужой заказ по ссылке: GET /orders/:id не фильтрует по региону;
• минус на складе: списание сделано как неатомарный read-modify-write без транзакции.

Главный риск — целостность денег и склада. Можно создать заказ с любой ценой, «отрицательным» количеством (это пополняет склад) и оставить в базе полусписанные заказы.

2. Замечания

🔴 Критично

К1. SQL-инъекция в промокоде

Место: POST /orders, запрос к discounts.

Чем грозит: промокод ' OR 1=1 -- вернёт первую попавшуюся активную скидку, в том числе 100% из чужого региона. Через UNION SELECT можно читать любые таблицы, к которым есть доступ у роли приложения.

До:

const discount = await AppDataSource.query(
  `SELECT * FROM discounts WHERE code = '${promoCode}' AND region_id = ${req.user.regionId}`
);
if (discount.length && discount[0].active) { ... }

После:

const [discount] = await manager.query(
  `SELECT code, percent FROM discounts
    WHERE code = $1 AND region_id = $2 AND active = true`,
  [promoCode, regionId]
);

Заодно условие active перенесено в SQL, а SELECT * заменён на явный список колонок.

К2. Нет авторизации по ролям и нет региональной изоляции (IDOR)

Место: все три маршрута.

Чем грозит:

• GET: любой аутентифицированный пользователь читает любой заказ, перебирая id. Это та самая жалоба.
• PATCH: support может менять статус, хотя по требованиям не должен; склад одного региона меняет заказы другого.
• POST: по требованиям заказы создаёт «менеджер», но какая роль ему соответствует, не определено.
• jwt импортирован, но не используется. Откуда берётся req.user и проверяется ли подпись, по коду не видно (см. блок вопросов).

После: middleware плюс фильтр по региону прямо в запросе.

// middleware/auth.js
const requireRole = (...roles) => (req, res, next) =>
  roles.includes(req.user?.role) ? next() : res.status(403).json({ error: 'forbidden' });

// scope: admin видит всё, остальные — только свой регион
const regionScope = (user) => (user.role === 'admin' ? {} : { regionId: user.regionId });

router.get('/orders/:id', requireRole('support', 'admin'), asyncHandler(async (req, res) => {
  const id = parseId(req.params.id);
  const order = await orderRepo.findOneBy({ id, ...regionScope(req.user) });
  if (!order) return res.status(404).json({ error: 'order_not_found' }); // 404, а не 403 — не раскрываем существование
  ...
}));

router.patch('/orders/:id/status', requireRole('warehouse', 'admin'), ...);

К3. Создание заказа без транзакции и гонка на складе

Место: POST /orders, цикл по items.

Чем грозит:

1. Гонка. Два запроса одновременно читают qty = 1, оба проходят проверку inv.qty < it.qty и оба сохраняют 0. Товар продан дважды. Если количества разные, save перезаписывает остаток абсолютным значением, и склад уходит в минус или «теряет» списания.
2. Частичная запись. Заказ вставляется до проверки склада. Если не хватило третьего товара:
   • в orders остаётся «мусорный» заказ со статусом new и total = 0;
   • первые два товара уже списаны и не возвращены;
   • их order_items записаны. Прямо нарушено требование «если списание не удалось, заказ создаваться не должен».
3. Падение посередине (обрыв соединения, исключение) даёт то же самое.

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

await AppDataSource.transaction(async (manager) => {
  for (const it of sortedItems) {
    const inv = await manager.getRepository('Inventory').findOne({
      where: { productId: it.productId, regionId },
      lock: { mode: 'pessimistic_write' },          // SELECT ... FOR UPDATE
    });
    if (!inv || inv.qty < it.qty) throw new OutOfStockError(it.productId);
    inv.qty -= it.qty;
    await manager.save('Inventory', inv);
  }
  // ...только после этого вставляем orders и order_items
});

В качестве страховки на уровне БД добавьте ограничение:

ALTER TABLE inventory ADD CONSTRAINT inventory_qty_non_negative CHECK (qty >= 0);

К4. Цена товара берётся из тела запроса

Место: price: it.price, total += it.price * it.qty.

Чем грозит: оператор (или любой, у кого есть токен) создаёт заказ на любую сумму: price: 0.01 или отрицательную цену. Это прямая финансовая дыра.

После: цена читается из БД внутри той же транзакции и фиксируется в order_items.price как снимок на момент заказа.

const product = await manager.getRepository('Product').findOneBy({ id: it.productId });
if (!product) throw new ValidationError(`unknown product ${it.productId}`);
const price = product.price; // см. вопрос о том, где хранится цена

К5. Отрицательное или дробное qty и отсутствие валидации входа

Место: POST /orders, PATCH /orders/:id/status.

Чем грозит:

• qty: -100 проходит проверку inv.qty < it.qty (например, 5 < -100 ложно). Строка inv.qty -= -100 пополняет склад на 100, а total становится отрицательным.
• items не массив или отсутствует — TypeError, запрос виснет (см. К6).
• В PATCH можно записать любую строку в status, например "hacked", или сделать delivered → new.
• Дубли productId в items обрабатываются как отдельные позиции. После исправления К3 это безопасно, но лучше агрегировать.

После: валидация на входе до бизнес-логики, например через zod.

const CreateOrderSchema = z.object({
  email: z.string().email(),
  promoCode: z.string().trim().max(64).optional(),
  items: z.array(z.object({
    productId: z.number().int().positive(),
    qty: z.number().int().positive().max(1000),
  })).min(1).max(100),
}).strict(); // price больше не принимаем вообще

const parsed = CreateOrderSchema.safeParse(req.body);
if (!parsed.success) return res.status(400).json({ error: 'validation', details: parsed.error.flatten() });

Для статусов нужна явная машина переходов и атомарная смена статуса:

const TRANSITIONS = {
  new:       ['paid', 'cancelled'],
  paid:      ['shipped', 'cancelled'],
  shipped:   ['delivered'],
  delivered: [],
  cancelled: [],
};

router.patch('/orders/:id/status', requireRole('warehouse', 'admin'), asyncHandler(async (req, res) => {
  const id = parseId(req.params.id);
  const { status: next } = StatusSchema.parse(req.body); // z.enum([...])

  const result = await AppDataSource.transaction(async (manager) => {
    const order = await manager.getRepository('Order').findOne({
      where: { id, ...regionScope(req.user) },
      lock: { mode: 'pessimistic_write' },
    });
    if (!order) return { code: 404, body: { error: 'order_not_found' } };
    if (!TRANSITIONS[order.status].includes(next)) {
      return { code: 409, body: { error: 'invalid_transition', from: order.status, to: next } };
    }
    order.status = next;
    await manager.save('Order', order);
    // TODO: при cancelled — вернуть товар на склад? (см. вопросы)
    return { code: 200, body: order };
  });
  res.status(result.code).json(result.body);
}));

К6. Проглоченные исключения и зависающие запросы

Место:

• catch (e) { console.log('order create failed'); } в POST: ответ не отправляется, клиент ждёт до таймаута, текст ошибки и стек теряются.
• GET и PATCH вообще без обработки. Express 4 не ловит rejected promise из async-хендлера, поэтому:
  • order === null (несуществующий id) даёт TypeError: Cannot read properties of null;
  • возникает unhandledRejection, и запрос висит;
  • в Node 20 по умолчанию процесс падает, если нет обработчика unhandledRejection. То есть запрос по несуществующему id может уронить сервис. Это тривиальный DoS.
• То же происходит, если product удалён: product.name на null.

После:

const asyncHandler = (fn) => (req, res, next) => Promise.resolve(fn(req, res, next)).catch(next);

// app.js — единый обработчик
app.use((err, req, res, next) => {
  if (err instanceof OutOfStockError) return res.status(409).json({ error: 'out_of_stock', productId: err.productId });
  if (err instanceof ValidationError) return res.status(400).json({ error: 'validation', message: err.message });
  req.log.error({ err, orderId: req.params?.id }, 'unhandled error'); // стек + контекст, без PII
  res.status(500).json({ error: 'internal' });
});

Вместо обёртки можно подключить пакет express-async-errors.

🟠 Важно

В1. PII в логах

Место: console.log('not enough stock for', it, 'user email', email).

Чем грозит: email клиента попадает в лог-агрегатор, где хранится дольше и доступен шире, чем БД. Для диагностики нехватки товара email не нужен.

После:

logger.warn({ productId: it.productId, requested: it.qty, available: inv?.qty, regionId }, 'out of stock');

В2. Email клиента принимается, но нигде не сохраняется

В схеме orders нет колонки для email или клиента, а user_id заполняется req.user.userId, то есть id менеджера, а не клиента. Непонятно, как потом связать заказ с покупателем. Это вопрос к схеме (см. блок вопросов). Если email нужен, его стоит хранить, но не возвращать в ответах шире, чем требуется.

В3. regionId в токене может отсутствовать

По описанию в JWT есть только userId и role, а код использует req.user.regionId. Если его там нет:

• заказы создаются с region_id = NULL;
• в старом SQL получалось region_id = undefined, то есть синтаксическая ошибка;
• региональная изоляция из К2 молча превращается в фильтр regionId: undefined. TypeORM 0.3 игнорирует undefined в where, и фильтр пропадёт. Это ещё один путь к IDOR.

Правка: явно проверять наличие региона в auth-middleware и отвечать 401/403, если у не-админа его нет:

if (req.user.role !== 'admin' && !Number.isInteger(req.user.regionId)) {
  return res.status(403).json({ error: 'no_region' });
}

В4. Деньги в float

Место: total = total * (1 - percent / 100).

Чем грозит: 19.99 * 3 * 0.85 даёт 50.974499999.... Копейки «плывут», и суммы в отчётах не сходятся.

После: считать в целых копейках и округлять один раз, явно. Колонки total и price должны быть numeric(12,2) или integer в копейках, но не real/double (см. вопросы).

const subtotalKop = lines.reduce((s, l) => s + l.priceKop * l.qty, 0);
const totalKop = discount ? Math.round(subtotalKop * (100 - discount.percent) / 100) : subtotalKop;

Добавьте в БД CHECK (percent BETWEEN 0 AND 100) на discounts.

В5. Эндпоинт POST /orders/:id/discount отсутствует

В описании он есть, в коде нет. Когда будете его писать:

• проверка роли support или admin и региона заказа;
• запрет повторного применения: колонка discount_code в orders и условие WHERE discount_code IS NULL;
• применение только к заказам в статусе new;
• пересчёт от исходной суммы, а не от уже сниженного total;
• транзакция с блокировкой строки заказа.

Иначе промокод можно применить N раз подряд и довести сумму почти до нуля.

В6. Ответ POST возвращает сырую сущность

res.json(order) отдаёт всё, что есть в entity, включая будущие служебные поля. Лучше явный DTO: { id, status, total, items, createdAt }. То же в GET: { ...order } плюс { ...it } отдаёт orderId и все внутренние поля.

🟡 Желательно

Ж1. N+1 в GET /orders/:id

Для заказа из N позиций выполняется 2+N запросов. При 200 заказах в день это не проблема производительности, но и один запрос не сложнее:

const items = await AppDataSource.query(
  `SELECT oi.id, oi.product_id AS "productId", oi.qty, oi.price, p.name AS "productName"
     FROM order_items oi
     LEFT JOIN products p ON p.id = oi.product_id
    WHERE oi.order_id = $1`,
  [order.id]
);

LEFT JOIN нужен, чтобы удалённый товар не ронял ответ. Альтернатива — relation OrderItem → Product и relations: { items: { product: true } }.

Ж2. Индексы под реальные выборки

Проверьте наличие (схему нужно запросить):

CREATE INDEX IF NOT EXISTS order_items_order_id_idx ON order_items(order_id);
-- inventory: PK или UNIQUE (product_id, region_id) — нужен и для корректности, и для FOR UPDATE по точке
ALTER TABLE inventory ADD PRIMARY KEY (product_id, region_id);   -- если ещё нет
CREATE UNIQUE INDEX IF NOT EXISTS discounts_code_region_uq ON discounts(code, region_id);
CREATE INDEX IF NOT EXISTS orders_region_created_idx ON orders(region_id, created_at DESC); -- под списки оператора

Ж3. Бизнес-логика в роутере

Расчёт суммы, списание склада, машина статусов и промокоды лежат в хендлере. Предлагаемая раскладка:

• routes/orders.js — только разбор запроса, валидация и маппинг ошибок в HTTP;
• services/orderService.js — createOrder, changeStatus, applyDiscount (транзакции живут здесь);
• domain/orderStatus.js — TRANSITIONS.

Это упрощает тесты: гонку склада можно проверить интеграционным тестом на сервисе без HTTP.

Ж4. Мелочи

• Неиспользуемый require('jsonwebtoken') удалить.
• Репозитории по строковому имени (getRepository('Order')) лучше заменить на импорт entity-класса: так опечатки ловятся сразу, а не в рантайме.
• req.params.id нужно валидировать как int или uuid. Сейчас GET /orders/abc даёт ошибку PostgreSQL invalid input syntax, то есть 500 вместо 400/404.
• Идемпотентность POST /orders: менеджер может нажать «Создать» дважды при медленной сети. Если это реальная проблема, стоит добавить заголовок Idempotency-Key с уникальным индексом.

Итоговый вариант POST /orders (К1, К3, К4, К5, К6, В1, В4)

router.post('/orders', requireRole('support', 'admin' /* уточнить роль менеджера */),
  asyncHandler(async (req, res) => {
    const { email, items, promoCode } = CreateOrderSchema.parse(req.body);
    const regionId = req.user.regionId;

    // агрегируем дубли и сортируем — стабильный порядок блокировок, без дедлоков
    const merged = [...items.reduce((m, it) => m.set(it.productId, (m.get(it.productId) ?? 0) + it.qty), new Map())]
      .map(([productId, qty]) => ({ productId, qty }))
      .sort((a, b) => a.productId - b.productId);

    const order = await AppDataSource.transaction(async (manager) => {
      const lines = [];
      for (const it of merged) {
        const inv = await manager.getRepository('Inventory').findOne({
          where: { productId: it.productId, regionId },
          lock: { mode: 'pessimistic_write' },
        });
        if (!inv || inv.qty < it.qty) throw new OutOfStockError(it.productId);

        const product = await manager.getRepository('Product').findOneBy({ id: it.productId });
        if (!product) throw new ValidationError(`unknown product ${it.productId}`);

        inv.qty -= it.qty;
        await manager.save('Inventory', inv);
        lines.push({ productId: it.productId, qty: it.qty, priceKop: product.priceKop });
      }

      const subtotal = lines.reduce((s, l) => s + l.priceKop * l.qty, 0);
      let total = subtotal;
      if (promoCode) {
        const [d] = await manager.query(
          `SELECT percent FROM discounts WHERE code = $1 AND region_id = $2 AND active = true`,
          [promoCode, regionId]
        );
        if (!d) throw new ValidationError('invalid_promo'); // явная ошибка лучше молчаливого игнора
        total = Math.round(subtotal * (100 - d.percent) / 100);
      }

      const saved = await manager.save('Order', {
        userId: req.user.userId, regionId, status: 'new', total, /* customerEmail: email — см. вопросы */
      });
      await manager.save('OrderItem', lines.map((l) => ({
        orderId: saved.id, productId: l.productId, qty: l.qty, price: l.priceKop,
      })));
      return { id: saved.id, status: saved.status, total, items: lines };
    });

    res.status(201).json(order);
  }));

Если любой шаг бросает исключение, транзакция откатывается целиком: ни заказа, ни списаний не остаётся.

Как поступать с неверным промокодом — ошибка или тихое игнорирование — продуктовое решение. Сейчас он молча игнорируется, и менеджер не узнаёт, что скидка не применилась.

3. Что сделано хорошо

• Регион берётся из токена, а не из тела запроса. Правильная идея: клиент не может подставить чужой регион. Осталось применять её последовательно (GET, PATCH) и гарантировать наличие regionId в токене.
• Промокод фильтруется по региону. Бизнес-правило учтено. Нужно лишь параметризовать запрос и перенести active в SQL.
• Код короткий и линейный. В нём легко увидеть поток данных, поэтому перенос в сервисный слой с транзакцией — механическая работа, а не переписывание.

Что запросить для окончательного вердикта

1. Auth-middleware. Где проверяется подпись JWT (алгоритм, секрет, exp)? Откуда берётся req.user.regionId, если в токене по описанию только userId и role?
2. DDL таблиц. Типы total, price, percent (numeric, integer или float), существующие PK, UNIQUE и индексы (особенно inventory(product_id, region_id)), наличие FK order_items.order_id → orders.id.
3. Где хранится цена товара. В схеме products не упомянута, хотя код её использует. Есть ли там price, и в каких единицах?
4. Кто такой orders.user_id: менеджер, создавший заказ, или клиент? Где должен храниться email клиента?
5. Какая роль создаёт заказы (support, admin или отдельная manager)? Видит ли admin все регионы?
6. Отмена заказа. Должен ли переход в cancelled возвращать товар на склад, и из каких статусов отмена разрешена (например, после shipped)?
7. Код POST /orders/:id/discount. Он заявлен, но не приложен, а риск повторного применения скидки там высокий.
8. Уровень изоляции транзакций и настройки пула в data-source.js: extra.max, statement_timeout, idle_in_transaction_session_timeout. Последний важен, чтобы зависшая транзакция не держала блокировки склада.