diff --git a/server/Server.js b/server/Server.js index 30d2c8c39..0b2a0dd66 100644 --- a/server/Server.js +++ b/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 diff --git a/server/libs/expressFileupload/processMultipart.js b/server/libs/expressFileupload/processMultipart.js index 32fb690a7..317194ebf 100644 --- a/server/libs/expressFileupload/processMultipart.js +++ b/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); }); diff --git a/server/libs/expressFileupload/tempFileHandler.js b/server/libs/expressFileupload/tempFileHandler.js index bfaca8b4f..473a62179 100644 --- a/server/libs/expressFileupload/tempFileHandler.js +++ b/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 }; diff --git a/server/routers/ApiRouter.js b/server/routers/ApiRouter.js index 7c89126b2..9a184aa62 100644 --- a/server/routers/ApiRouter.js +++ b/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)) diff --git a/server/utils/fileUploadMiddleware.js b/server/utils/fileUploadMiddleware.js new file mode 100644 index 000000000..de78eca81 --- /dev/null +++ b/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} + */ +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 diff --git a/test/server/utils/fileUploadMiddleware.test.js b/test/server/utils/fileUploadMiddleware.test.js new file mode 100644 index 000000000..2e8701de6 --- /dev/null +++ b/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) + } + }) + }) +})