openvibers/openvibe.tools Commits

Tools: an audio job's format can no longer write outside the temp directory; the gateway refuses backslashes in next (#29)

OP OpenVibers committed 10 Oct 2026 · b9b5b2e420 · 4 files, +42 −2

The urgent part of an independent security review of Tools. The rest follows in a separate PR.

- **HIGH, unauthenticated file write outside the temp directory:**
  - Several audio tools used the caller's `format` as the output file's extension. The jobs path (`POST /api/v1/jobs`) didn't enforce the tool's schema, so `format: "../../x"` named a path outside `tmpDir`.
  - `tmpFile()` now takes only a short alphanumeric extension and checks that the file stays inside `tmpDir`. The `audio.process` job also refuses any other format at submit.
  - The new `apps/audio/test/format-path.test.js` covers both.
- **MEDIUM, open redirect:** the gateway's `sanitizeNext` passed `/\evil.com`, which browsers read as `//evil.com`. Control characters and backslashes are now refused (the network's standard guard).

Audio jobs, audio hardening and gateway guard tests pass.

Changes

ddiff --git a/apps/audio/server/process.js b/apps/audio/server/process.js
iindex 0d0d6c4..a1afd3a 100644
---- a/apps/audio/server/process.js
++++ b/apps/audio/server/process.js
@@@ -129,6 +129,8 @@ function defineJobs(system) {
                  return `Tool "${id}" takes exactly one file`;
              }
              for (const [k, v] of Object.entries(input)) if (v != null && typeof v === 'object') return `Option ${k} must be a string or a number`;
++            // The output format becomes the file extension (ffmpeg-helper tmpFile): a short alphanumeric name only.
++            if (input.format != null && !/^[a-z0-9]{1,8}$/i.test(String(input.format))) return 'format must be a short audio format name such as mp3, wav or flac';
              return null;
          },
          async run({ input, files, progress, signal }) {
ddiff --git a/apps/audio/server/tools/ffmpeg-helper.js b/apps/audio/server/tools/ffmpeg-helper.js
iindex 5f47268..0b632f5 100644
---- a/apps/audio/server/tools/ffmpeg-helper.js
++++ b/apps/audio/server/tools/ffmpeg-helper.js
@@@ -16,10 +16,16 @@ const tmpDir = path.resolve(config.dataDir, 'tmp');
  if (!fs.existsSync(tmpDir)) fs.mkdirSync(tmpDir, { recursive: true });
  
  /**
-- * Generate a temp file path with the given extension.
++ * Generate a temp file path with the given extension. The extension often comes from a request (an output `format`),
++ * so only a short alphanumeric one is accepted: anything with a path separator, a dot or `..` would write outside
++ * tmpDir. The resolved path is checked to stay inside tmpDir as well.
   */
  function tmpFile(ext) {
--    return path.join(tmpDir, `${crypto.randomBytes(16).toString('hex')}.${ext}`);
++    const e = String(ext == null ? '' : ext).toLowerCase();
++    if (!/^[a-z0-9]{1,8}$/.test(e)) throw Object.assign(new Error('unsupported output format'), { status: 400, code: 'tools.bad_format' });
++    const file = path.join(tmpDir, `${crypto.randomBytes(16).toString('hex')}.${e}`);
++    if (path.dirname(file) !== tmpDir) throw Object.assign(new Error('unsupported output format'), { status: 400, code: 'tools.bad_format' });
++    return file;
  }
  
  /**
ddiff --git a/apps/audio/test/format-path.test.js b/apps/audio/test/format-path.test.js
nnew file mode 100644
iindex 0000000..710f31a
---- /dev/null
++++ b/apps/audio/test/format-path.test.js
@@@ -0,0 +1,30 @@
++'use strict';
++/**
++ * The output format becomes a temp file's extension, so it can never name a path: tmpFile() takes only a short
++ * alphanumeric extension and stays inside tmpDir, and the audio.process job refuses any other format at submit.
++ * (Security review 2026-10-10: `format: "../../x"` through POST /api/v1/jobs wrote outside the temp directory.)
++ */
++const assert = require('assert');
++const fs = require('fs');
++const os = require('os');
++const path = require('path');
++
++process.env.DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), 'ov-audio-format-'));
++const { tmpFile, tmpDir } = require('../server/tools/ffmpeg-helper');
++const { defineJobs } = require('../server/process');
++
++assert.strictEqual(path.dirname(tmpFile('mp3')), tmpDir);
++assert.ok(tmpFile('FLAC').endsWith('.flac'));
++for (const bad of ['../../etc/x', '../x', 'mp3/../../x', 'a.b', '', 'x'.repeat(9), '..', 'm p3']) {
++    assert.throws(() => tmpFile(bad), (e) => e.status === 400 && e.code === 'tools.bad_format', `${JSON.stringify(bad)} is refused`);
++}
++
++let job = null;
++defineJobs({ define: (d) => { job = d; } });
++const file = [{ path: '/tmp/a.mp3', name: 'a.mp3', size: 1 }];
++assert.strictEqual(job.validate({ tool: 'convert', format: 'wav' }, file), null);
++assert.match(job.validate({ tool: 'convert', format: '../../x' }, file) || '', /format must be a short audio format name/);
++assert.match(job.validate({ tool: 'extract', format: 'mp3/../../../tmp/pwned' }, file) || '', /format must be/);
++
++fs.rmSync(process.env.DATA_DIR, { recursive: true, force: true });
++console.log('audio format path: only short alphanumeric formats, inside the temp directory');
ddiff --git a/apps/gateway/server/auth/routes.js b/apps/gateway/server/auth/routes.js
iindex 0d338a6..7be927b 100644
---- a/apps/gateway/server/auth/routes.js
++++ b/apps/gateway/server/auth/routes.js
@@@ -245,6 +245,8 @@ function createAuthRoutes(config, auth) {
       */
      function sanitizeNext(next) {
          if (!next || typeof next !== 'string') return '/';
++        // Browsers drop tabs and newlines and read \ as /, so "/\evil.com" or "/<TAB>/evil.com" would leave the site.
++        if (/[\u0000-\u001f\u007f\\]/.test(next)) return '/';
          if (/^\/(?!\/)/.test(next)) return next; // relative path, not protocol-relative
          try {
              const u = new URL(next);