Merge pull request #5686 from mikiher/fix-upload-auth

Stop unauthenticated multipart uploads from leaving temp files on disk

server/Server.js

@@ -4,7 +4,6 @@ const express = require('express')
 const http = require('http')
 const util = require('util')
 const fs = require('./libs/fsExtra')
-const fileUpload = require('./libs/expressFileupload')
 const cookieParser = require('cookie-parser')
 const axios = require('axios')
 
@@ -13,6 +12,7 @@ const { version } = require('../package.json')
 // Utils
 const is = require('./libs/requestIp/isJs')
 const fileUtils = require('./utils/fileUtils')
+const { sweepUploadTempFiles } = require('./utils/fileUploadMiddleware')
 const { toNumber } = require('./utils/index')
 const { getRequestOrigin } = require('./utils/requestUtils')
 const Logger = require('./Logger')
@@ -199,6 +199,7 @@ class Server {
 
     await this.cleanUserData() // Remove invalid user item progress
     await CacheManager.ensureCachePaths()
+    await sweepUploadTempFiles()
 
     await ShareManager.init()
     await this.backupManager.init()
@@ -336,16 +337,6 @@ class Server {
 
     this.server = http.createServer(app)
 
-    // Skip file upload parsing for internal-api routes (Next.js proxies read multipart bodies).
-    router.use(
-      /^(?!\/internal-api).*/,
-      fileUpload({
-        defCharset: 'utf8',
-        defParamCharset: 'utf8',
-        useTempFiles: true,
-        tempFileDir: Path.join(global.MetadataPath, 'tmp')
-      })
-    )
     router.use(express.urlencoded({ extended: true, limit: '5mb' }))
 
     // Skip JSON parsing for internal-api routes

server/libs/expressFileupload/processMultipart.js

@@ -26,6 +26,18 @@ const waitFlushProperty = Symbol('wait flush property symbol');
  */
 module.exports = (options, req, res, next) => {
   req.files = null;
+  const pendingCleanups = [];
+  const runPendingCleanups = () => {
+    while (pendingCleanups.length) {
+      const cleanupFn = pendingCleanups.pop();
+      try {
+        cleanupFn();
+      } catch (err) {
+        debugLog(options, `Temp file cleanup failed: ${err}`);
+      }
+    }
+  };
+  res.once('close', runPendingCleanups);
 
   // Build busboy options and init busboy instance.
   const busboyOptions = buildOptions(options, { headers: req.headers });
@@ -62,6 +74,7 @@ module.exports = (options, req, res, next) => {
     } = options.useTempFiles
         ? tempFileHandler(options, field, filename) // Upload into temporary file.
         : memHandler(options, field, filename);     // Upload into RAM.
+    pendingCleanups.push(cleanup);
 
     const writePromise = options.useTempFiles
       ? getWritePromise().catch(err => {
@@ -161,6 +174,7 @@ module.exports = (options, req, res, next) => {
 
   busboy.on('error', (err) => {
     debugLog(options, `Busboy error`);
+    runPendingCleanups();
     next(err);
   });
 

server/libs/expressFileupload/tempFileHandler.js

@@ -28,6 +28,18 @@ module.exports = (options, fieldname, filename) => {
       reject(err);
     });
   });
+  // cleanup() can run from an error path and again on response close.
+  let cleanedUp = false;
+
+  // Windows cannot unlink while the write stream still holds the file.
+  const closeWriteStream = () => new Promise((resolve) => {
+    if (writeStream.destroyed || writeStream.closed) {
+      return resolve();
+    }
+    writeStream.once('close', resolve);
+    writeStream.once('error', resolve);
+    writeStream.end();
+  });
 
   return {
     dataHandler: (data) => {
@@ -51,13 +63,21 @@ module.exports = (options, fieldname, filename) => {
       return Buffer.concat([]);
     },
     cleanup: () => {
+      if (cleanedUp) return Promise.resolve();
+      cleanedUp = true;
       completed = true;
       debugLog(options, `Cleaning up temporary file ${tempFilePath}...`);
-      writeStream.end();
-      deleteFile(tempFilePath, err => (err 
-        ? debugLog(options, `Cleaning up temporary file ${tempFilePath} failed: ${err}`)
-        : debugLog(options, `Cleaning up temporary file ${tempFilePath} done.`)
-      ));
+      return closeWriteStream().then(() => new Promise((resolve) => {
+        // ENOENT: handler already mv()'d the file; that is success.
+        deleteFile(tempFilePath, err => {
+          if (err && err.code !== 'ENOENT') {
+            debugLog(options, `Cleaning up temporary file ${tempFilePath} failed: ${err}`);
+          } else {
+            debugLog(options, `Cleaning up temporary file ${tempFilePath} done.`);
+          }
+          resolve();
+        });
+      }));
     },
     getWritePromise: () => writePromise
   };

server/routers/ApiRouter.js

@@ -11,6 +11,7 @@ const date = require('../libs/dateAndTime')
 
 const CacheManager = require('../managers/CacheManager')
 const RssFeedManager = require('../managers/RssFeedManager')
+const { fileUploadParser, requireUploadPermission } = require('../utils/fileUploadMiddleware')
 
 const LibraryController = require('../controllers/LibraryController')
 const UserController = require('../controllers/UserController')
@@ -110,7 +111,7 @@ class ApiRouter {
     this.router.get('/items/:id/download', LibraryItemController.middleware.bind(this), LibraryItemController.download.bind(this))
     this.router.patch('/items/:id/media', LibraryItemController.middleware.bind(this), LibraryItemController.updateMedia.bind(this))
     this.router.get('/items/:id/cover', LibraryItemController.getCover.bind(this))
-    this.router.post('/items/:id/cover', LibraryItemController.middleware.bind(this), LibraryItemController.uploadCover.bind(this))
+    this.router.post('/items/:id/cover', LibraryItemController.middleware.bind(this), requireUploadPermission, fileUploadParser, LibraryItemController.uploadCover.bind(this))
     this.router.patch('/items/:id/cover', LibraryItemController.middleware.bind(this), LibraryItemController.updateCover.bind(this))
     this.router.delete('/items/:id/cover', LibraryItemController.middleware.bind(this), LibraryItemController.removeCover.bind(this))
     this.router.post('/items/:id/match', LibraryItemController.middleware.bind(this), LibraryItemController.match.bind(this))
@@ -202,7 +203,7 @@ class ApiRouter {
     this.router.delete('/backups/:id', BackupController.middleware.bind(this), BackupController.delete.bind(this))
     this.router.get('/backups/:id/download', BackupController.middleware.bind(this), BackupController.download.bind(this))
     this.router.get('/backups/:id/apply', BackupController.middleware.bind(this), BackupController.apply.bind(this))
-    this.router.post('/backups/upload', BackupController.middleware.bind(this), BackupController.upload.bind(this))
+    this.router.post('/backups/upload', BackupController.middleware.bind(this), fileUploadParser, BackupController.upload.bind(this))
     this.router.patch('/backups/path', BackupController.middleware.bind(this), BackupController.updatePath.bind(this))
 
     //
@@ -343,7 +344,7 @@ class ApiRouter {
     //
     // Misc Routes
     //
-    this.router.post('/upload', MiscController.handleUpload.bind(this))
+    this.router.post('/upload', requireUploadPermission, fileUploadParser, MiscController.handleUpload.bind(this))
     this.router.get('/tasks', MiscController.getTasks.bind(this))
     this.router.patch('/settings', MiscController.updateServerSettings.bind(this))
     this.router.patch('/sorting-prefixes', MiscController.updateSortingPrefixes.bind(this))

server/utils/fileUploadMiddleware.js

@@ -0,0 +1,76 @@
+const Path = require('path')
+const fs = require('../libs/fsExtra')
+const fileUpload = require('../libs/expressFileupload')
+const Logger = require('../Logger')
+
+/**
+ * Multipart parser for the three routes that read req.files.
+ * Reads MetadataPath per request so route registration does not depend on it.
+ *
+ * @param {import('express').Request} req
+ * @param {import('express').Response} res
+ * @param {import('express').NextFunction} next
+ */
+function fileUploadParser(req, res, next) {
+  fileUpload({
+    defCharset: 'utf8',
+    defParamCharset: 'utf8',
+    useTempFiles: true,
+    tempFileDir: Path.join(global.MetadataPath, 'tmp')
+  })(req, res, next)
+}
+module.exports.fileUploadParser = fileUploadParser
+
+/**
+ * Reject users without upload permission before the multipart parser writes temp files.
+ *
+ * @param {import('express').Request} req
+ * @param {import('express').Response} res
+ * @param {import('express').NextFunction} next
+ */
+function requireUploadPermission(req, res, next) {
+  if (!req.user?.canUpload) {
+    Logger.warn(`User "${req.user?.username || 'unknown'}" attempted to upload without permission`)
+    return res.sendStatus(403)
+  }
+  next()
+}
+module.exports.requireUploadPermission = requireUploadPermission
+
+/**
+ * Remove leftover upload temp files (tmp-*) from metadata/tmp.
+ * Comic extraction uses the same directory with other names; those stay.
+ *
+ * @returns {Promise<void>}
+ */
+async function sweepUploadTempFiles() {
+  const tmpDir = Path.join(global.MetadataPath, 'tmp')
+  let names
+  try {
+    names = await fs.readdir(tmpDir)
+  } catch (error) {
+    if (error.code === 'ENOENT') return
+    Logger.warn(`[FileUpload] Failed to read upload temp directory "${tmpDir}": ${error.message}`)
+    return
+  }
+
+  let removed = 0
+  for (const name of names) {
+    if (!name.startsWith('tmp-')) continue
+    const fullPath = Path.join(tmpDir, name)
+    try {
+      const stat = await fs.lstat(fullPath)
+      if (!stat.isFile()) continue
+      await fs.unlink(fullPath)
+      removed++
+    } catch (error) {
+      if (error.code === 'ENOENT') continue
+      Logger.warn(`[FileUpload] Failed to remove leftover upload temp file "${fullPath}": ${error.message}`)
+    }
+  }
+
+  if (removed) {
+    Logger.info(`[FileUpload] Removed ${removed} leftover upload temp file(s) from "${tmpDir}"`)
+  }
+}
+module.exports.sweepUploadTempFiles = sweepUploadTempFiles

test/server/utils/fileUploadMiddleware.test.js

@@ -0,0 +1,221 @@
+const { expect } = require('chai')
+const express = require('express')
+const fs = require('fs')
+const http = require('http')
+const os = require('os')
+const path = require('path')
+const sinon = require('sinon')
+
+const Logger = require('../../../server/Logger')
+const { fileUploadParser, requireUploadPermission, sweepUploadTempFiles } = require('../../../server/utils/fileUploadMiddleware')
+
+function listTmpFiles(dir) {
+  if (!fs.existsSync(dir)) return []
+  return fs.readdirSync(dir).filter((name) => name.startsWith('tmp-'))
+}
+
+function sendMultipart(port, urlPath, extraHeaders = {}) {
+  return new Promise((resolve, reject) => {
+    const boundary = '----AbsUploadTestBoundary'
+    const payload = [
+      `--${boundary}`,
+      'Content-Disposition: form-data; name="f"; filename="probe.txt"',
+      'Content-Type: text/plain',
+      '',
+      'probe',
+      `--${boundary}--`,
+      ''
+    ].join('\r\n')
+
+    const req = http.request(
+      {
+        hostname: '127.0.0.1',
+        port,
+        path: urlPath,
+        method: 'POST',
+        headers: {
+          'Content-Type': `multipart/form-data; boundary=${boundary}`,
+          'Content-Length': Buffer.byteLength(payload),
+          ...extraHeaders
+        }
+      },
+      (res) => {
+        res.resume()
+        res.on('end', () => resolve(res.statusCode))
+      }
+    )
+    req.on('error', reject)
+    req.write(payload)
+    req.end()
+  })
+}
+
+function listen(app) {
+  const server = http.createServer(app)
+  return new Promise((resolve) => {
+    server.listen(0, '127.0.0.1', () => {
+      resolve({ server, port: server.address().port })
+    })
+  })
+}
+
+function closeServer(server) {
+  return new Promise((resolve, reject) => {
+    server.close((err) => (err ? reject(err) : resolve()))
+  })
+}
+
+async function waitFor(predicate, timeoutMs = 2000) {
+  const started = Date.now()
+  while (Date.now() - started < timeoutMs) {
+    if (predicate()) return
+    await new Promise((resolve) => setTimeout(resolve, 25))
+  }
+  throw new Error('Timed out waiting for condition')
+}
+
+describe('fileUploadMiddleware', () => {
+  let metadataDir
+  let tmpDir
+  let previousMetadataPath
+
+  beforeEach(() => {
+    previousMetadataPath = global.MetadataPath
+    metadataDir = fs.mkdtempSync(path.join(os.tmpdir(), 'abs-upload-meta-'))
+    tmpDir = path.join(metadataDir, 'tmp')
+    global.MetadataPath = metadataDir
+    sinon.stub(Logger, 'info')
+    sinon.stub(Logger, 'warn')
+  })
+
+  afterEach(() => {
+    sinon.restore()
+    global.MetadataPath = previousMetadataPath
+    fs.rmSync(metadataDir, { recursive: true, force: true })
+  })
+
+  describe('requireUploadPermission', () => {
+    it('returns 403 when the user cannot upload', () => {
+      const req = { user: { canUpload: false, username: 'guest' } }
+      const res = { sendStatus: sinon.stub() }
+      const next = sinon.stub()
+
+      requireUploadPermission(req, res, next)
+
+      expect(res.sendStatus.calledOnceWithExactly(403)).to.equal(true)
+      expect(next.called).to.equal(false)
+    })
+
+    it('calls next when the user can upload', () => {
+      const req = { user: { canUpload: true, username: 'uploader' } }
+      const res = { sendStatus: sinon.stub() }
+      const next = sinon.stub()
+
+      requireUploadPermission(req, res, next)
+
+      expect(next.calledOnce).to.equal(true)
+      expect(res.sendStatus.called).to.equal(false)
+    })
+  })
+
+  describe('sweepUploadTempFiles', () => {
+    it('removes leftover tmp-* files and leaves other names', async () => {
+      fs.mkdirSync(tmpDir, { recursive: true })
+      fs.writeFileSync(path.join(tmpDir, 'tmp-1-123'), 'stale')
+      fs.writeFileSync(path.join(tmpDir, 'tmp-2-456'), 'stale')
+      fs.writeFileSync(path.join(tmpDir, 'comic-extract-keep'), 'keep')
+      fs.mkdirSync(path.join(tmpDir, 'tmp-not-a-file'))
+
+      await sweepUploadTempFiles()
+
+      expect(fs.existsSync(path.join(tmpDir, 'tmp-1-123'))).to.equal(false)
+      expect(fs.existsSync(path.join(tmpDir, 'tmp-2-456'))).to.equal(false)
+      expect(fs.existsSync(path.join(tmpDir, 'comic-extract-keep'))).to.equal(true)
+      expect(fs.existsSync(path.join(tmpDir, 'tmp-not-a-file'))).to.equal(true)
+    })
+
+    it('returns when the temp directory does not exist', async () => {
+      await sweepUploadTempFiles()
+      expect(fs.existsSync(tmpDir)).to.equal(false)
+    })
+  })
+
+  describe('upload parser mounting', () => {
+    it('does not write tmp-* files for a route without the parser', async () => {
+      const app = express()
+      app.use((req, res) => res.sendStatus(404))
+      const { server, port } = await listen(app)
+
+      try {
+        const status = await sendMultipart(port, '/not-a-route')
+        expect(status).to.equal(404)
+        expect(listTmpFiles(tmpDir)).to.deep.equal([])
+      } finally {
+        await closeServer(server)
+      }
+    })
+
+    it('does not write tmp-* files when permission is rejected before parsing', async () => {
+      const app = express()
+      app.use((req, res, next) => {
+        req.user = { canUpload: false, username: 'guest' }
+        next()
+      })
+      app.post('/upload', requireUploadPermission, fileUploadParser, (req, res) => {
+        res.sendStatus(200)
+      })
+      const { server, port } = await listen(app)
+
+      try {
+        const status = await sendMultipart(port, '/upload')
+        expect(status).to.equal(403)
+        expect(listTmpFiles(tmpDir)).to.deep.equal([])
+      } finally {
+        await closeServer(server)
+      }
+    })
+
+    it('removes the temp file when the handler rejects without moving it', async () => {
+      const app = express()
+      app.use((req, res, next) => {
+        req.user = { canUpload: true, username: 'uploader' }
+        next()
+      })
+      app.post('/upload', requireUploadPermission, fileUploadParser, (req, res) => {
+        res.status(400).send('Invalid request no file or url')
+      })
+      const { server, port } = await listen(app)
+
+      try {
+        const status = await sendMultipart(port, '/upload')
+        expect(status).to.equal(400)
+        await waitFor(() => listTmpFiles(tmpDir).length === 0)
+      } finally {
+        await closeServer(server)
+      }
+    })
+
+    it('keeps a moved file and leaves no tmp-* leftover', async () => {
+      const dest = path.join(metadataDir, 'moved.txt')
+      const app = express()
+      app.use((req, res, next) => {
+        req.user = { canUpload: true, username: 'uploader' }
+        next()
+      })
+      app.post('/upload', requireUploadPermission, fileUploadParser, async (req, res) => {
+        await req.files.f.mv(dest)
+        res.sendStatus(200)
+      })
+      const { server, port } = await listen(app)
+
+      try {
+        const status = await sendMultipart(port, '/upload')
+        expect(status).to.equal(200)
+        expect(fs.readFileSync(dest, 'utf8')).to.equal('probe')
+        await waitFor(() => listTmpFiles(tmpDir).length === 0)
+      } finally {
+        await closeServer(server)
+      }
+    })
+  })
+})