Техпрогон
ВойтиНачать бесплатно
Код-ревью Middle Файлы и процессы обновлено

Код-ревью: конвертация файлов через внешнюю утилиту

Вопрос

Сервис конвертирует загруженные картинки внешней утилитой. Найди уязвимости и ошибки работы с процессами и файлами.

const { exec } = require('child_process');

app.post('/convert', (req, res) => {
  const input = '/tmp/' + req.body.file;
  const output = input + '.png';
  exec('convert ' + input + ' -resize ' + req.body.size + ' ' + output, (err) => {
    if (err) console.log(err);
    res.sendFile(output);
    fs.unlink(input, () => {});
  });
});

process.on('SIGTERM', () => {
  process.exit(0);
});

const PORT = process.env.PORT || 3000;
const DEBUG = process.env.DEBUG || true;

Коротко

exec передаёт строку в /bin/sh -c, поэтому любые ;, , $() в пользовательских данных становятся командами. execFile и spawn без shell: true передают аргументы массивом напрямую программе, и инъекция через метасимволы невозможна (но утилита всё ещё может интерпретировать аргументы, начинающиеся с -, поэтому значения…

Что должен найти ревьюер

  1. Безопасностьстрока 6
    Command injection: exec запускает строку через shell, а file и size приходят от клиента (; rm -rf /). Нужен execFile/spawn с массивом аргументов без shell и валидация size.
  2. Безопасностьстрока 4
    Имя файла от клиента в пути: path traversal, можно указать любой файл системы.
  3. Производительностьстрока 6
    Нет ограничения параллельных конвертаций и таймаута: десяток одновременных запросов запустит десяток тяжёлых процессов. Нужна очередь с лимитом и timeout/AbortSignal.
  4. Ошибкастрока 7
    При ошибке конвертации всё равно отправляется файл, которого нет; клиент получит 404 или старый файл вместо 500.
  5. Ошибкастрока 9
    Выходной файл никогда не удаляется: /tmp заполнится. Удалять и вход, и выход после отправки (в колбэке sendFile).
  6. Ошибкастрока 14
    Немедленный process.exit на SIGTERM обрывает запросы в работе и запущенные дочерние процессы. Нужен graceful shutdown: перестать принимать, дождаться, потом выйти.
  7. Ошибкастрока 18
    process.env.DEBUG || true всегда истинно: переменные окружения это строки, и DEBUG=false тоже truthy. Явно разбирать === "true".

Разбор

exec передаёт строку в /bin/sh -c, поэтому любые ;, |, $() в пользовательских данных становятся командами. execFile и spawn без shell: true передают аргументы массивом напрямую программе, и инъекция через метасимволы невозможна (но утилита всё ещё может интерпретировать аргументы, начинающиеся с -, поэтому значения валидируют).

Про процессы на собесе ещё спрашивают: exec буферизует весь вывод (лимит maxBuffer, по умолчанию 1 МБ), для больших выводов нужен spawn с потоками; дочерние процессы надо ограничивать по числу и времени; при остановке сервиса их надо дождаться или завершить.

Переменные окружения всегда строки (или undefined), поэтому конфиг разбирают и валидируют явно на старте.

Ответьте сами, а не прочитайте

В тренажёре вопрос попадёт в повторения и вернётся, когда начнёт забываться. Плюс билеты с зачётом: 20 вопросов, не больше 2 ошибок.

Потренироваться бесплатно

Предыдущий вопрос

Защита от path traversal при раздаче файлов

Следующий вопрос

exec, spawn и большой вывод

Урок по теме

Файлы, пути и процессы в Node.js

Термины по теме

Graceful shutdown сервера на Node.js

Ещё по теме «Файлы и процессы»

Все вопросы модуля