diff --git a/server/Server.js b/server/Server.js index 82bdea40f..327cc94bf 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/providers/iTunes.js b/server/providers/iTunes.js index 87695c58c..6d14471b9 100644 --- a/server/providers/iTunes.js +++ b/server/providers/iTunes.js @@ -27,6 +27,55 @@ const htmlSanitizer = require('../utils/htmlSanitizer') * @property {boolean} explicit */ +/** + * Split a list of authors on commas. A piece with no space is a credential and is glued onto the previous name. + * + * @param {string} text + * @returns {string[]} + */ +function parseAuthorPieces(text) { + const authors = [] + for (const piece of (text || '').split(',')) { + const name = piece.trim() + if (!name) continue + if (!name.includes(' ') && authors.length) { + authors[authors.length - 1] = `${authors[authors.length - 1]} ${name}` + } else { + authors.push(name) + } + } + return authors +} + +function stripCommasFromName(name) { + return (name || '').replace(/,/g, '').replace(/\s+/g, ' ').trim() +} + +/** + * Apple lists authors as "Name1, Name2 & Name3", but a comma can also sit inside one name ("Rick Hanson, PhD"). + * A comma-separated piece with no space is treated as a credential and glued onto the previous name. + * The text after the last " & " is a single author, so commas there are removed. + * @see https://github.com/advplyr/audiobookshelf/issues/1022 + * @see https://github.com/advplyr/audiobookshelf/issues/2661 + * + * @param {string} artistName + * @returns {string} + */ +function cleanAuthor(artistName) { + const raw = (artistName || '').trim() + if (!raw) return '' + + const lastAmpIndex = raw.lastIndexOf(' & ') + if (lastAmpIndex === -1) { + return stripCommasFromName(raw) + } + + const authors = parseAuthorPieces(raw.slice(0, lastAmpIndex)) + const lastAuthor = stripCommasFromName(raw.slice(lastAmpIndex + 3)) + if (lastAuthor) authors.push(lastAuthor) + return authors.join(', ') +} + class iTunes { #responseTimeout = 10000 @@ -103,15 +152,11 @@ class iTunes { } cleanAudiobook(data) { - // artistName can be "Name1, Name2 & Name3" so we refactor this to "Name1, Name2, Name3" - // see: https://github.com/advplyr/audiobookshelf/issues/1022 - const author = (data.artistName || '').split(' & ').join(', ') - return { id: data.collectionId, artistId: data.artistId, title: data.collectionName, - author, + author: cleanAuthor(data.artistName), description: data.description || null, publishedYear: data.releaseDate ? data.releaseDate.split('-')[0] : null, genres: data.primaryGenreName ? [data.primaryGenreName] : null, 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/providers/iTunes.test.js b/test/server/providers/iTunes.test.js new file mode 100644 index 000000000..14f71bbd9 --- /dev/null +++ b/test/server/providers/iTunes.test.js @@ -0,0 +1,53 @@ +const iTunes = require('../../../server/providers/iTunes') +const { expect } = require('chai') + +describe('iTunes', () => { + let itunes + + beforeEach(() => { + itunes = new iTunes() + }) + + describe('cleanAudiobook', () => { + function authorFrom(artistName) { + return itunes.cleanAudiobook({ artistName }).author + } + + it('should return an empty string if artistName is missing', () => { + expect(authorFrom()).to.equal('') + expect(authorFrom('')).to.equal('') + }) + + it('should keep a single author without commas', () => { + expect(authorFrom('Joy Fielding')).to.equal('Joy Fielding') + }) + + it('should strip commas from a single author with a credential', () => { + expect(authorFrom('Rick Hanson, PhD')).to.equal('Rick Hanson PhD') + }) + + it('should split two authors joined by an ampersand', () => { + expect(authorFrom('Name1 & Name2')).to.equal('Name1, Name2') + }) + + it('should split three authors using commas and an ampersand', () => { + expect(authorFrom('Keltie Knight, Becca Tobin & Jac Vanek')).to.equal('Keltie Knight, Becca Tobin, Jac Vanek') + }) + + it('should glue a one-word credential onto the previous author before the ampersand', () => { + expect(authorFrom('Rick Hanson, PhD & Jane Doe')).to.equal('Rick Hanson PhD, Jane Doe') + }) + + it('should glue several one-word credentials in a row', () => { + expect(authorFrom('Rick Hanson, PhD, MD & Jane Doe')).to.equal('Rick Hanson PhD MD, Jane Doe') + }) + + it('should strip commas from the last author', () => { + expect(authorFrom('Rick Hanson & Jane Doe, PhD')).to.equal('Rick Hanson, Jane Doe PhD') + }) + + it('should glue credentials before the ampersand and strip commas from the last author', () => { + expect(authorFrom('Rick Hanson, PhD & Jane Doe, PhD')).to.equal('Rick Hanson PhD, Jane Doe PhD') + }) + }) +}) 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) + } + }) + }) +})