🔒 Made names for uploaded files more secure and resilient to filesystems' limits

ghost/core/core/server/adapters/storage/LocalStorageBase.js

@@ -43,23 +43,30 @@ class LocalStorageBase extends StorageBase {
      * Saves the file to storage (the file system)
      * - returns a promise which ultimately returns the full url to the uploaded file
      *
-     * @param {StorageBase.Image} file
+     * @param {Object} file
+     * @param {String} [file.path] -- The original path of the file
+     * @param {String} [file.name] -- The original name of the file
+     * @param {Boolean} [file.keepOriginalName] -- If true, skip generating a new filename
      * @param {String} targetDir
      * @returns {Promise<String>}
      */
     async save(file, targetDir) {
-        let targetFilename;
-
-        // NOTE: the base implementation of `getTargetDir` returns the format this.storagePath/YYYY/MM
-        targetDir = targetDir || this.getTargetDir(this.storagePath);
-
-        const filename = await this.getUniqueFileName(file, targetDir);
-
-        targetFilename = filename;
-        await fs.mkdirs(targetDir);
+        // The base implementation of `getTargetDir` returns the format this.storagePath/YYYY/MM, e.g. /content/images/2025/01
+        const directory = targetDir || this.getTargetDir(this.storagePath);
+        const originalFilePath = file.path;
+
+        // If the `keepOriginalName` flag is set, don't generate a new filename
+        // Otherwise, generate a unique secure filename, composed of a 16-character random hash and truncated to be under 255 bytes, e.g. image-a1b2c3d4e5f6g789.png
+        let targetFilePath;
+        if (file.keepOriginalName) {
+            targetFilePath = path.join(directory, file.name);
+        } else {
+            targetFilePath = this.getUniqueSecureFilePath(file, directory);
+        }
 
         try {
-            await fs.copy(file.path, targetFilename);
+            await fs.mkdirs(directory);
+            await fs.copy(originalFilePath, targetFilePath);
         } catch (err) {
             if (err.code === 'ENAMETOOLONG') {
                 throw new errors.BadRequestError({err});
@@ -74,7 +81,7 @@ class LocalStorageBase extends StorageBase {
             urlUtils.urlJoin('/',
                 urlUtils.getSubdir(),
                 this.staticFileURLPrefix,
-                path.relative(this.storagePath, targetFilename))
+                path.relative(this.storagePath, targetFilePath))
         ).replace(new RegExp(`\\${path.sep}`, 'g'), '/');
 
         return fullUrl;

ghost/core/core/server/api/endpoints/images.js

@@ -71,7 +71,8 @@ const controller = {
                 await store.save({
                     ...frame.file,
                     path: originalPath,
-                    name: imageTransform.generateOriginalImageName(processedImageName)
+                    name: imageTransform.generateOriginalImageName(processedImageName),
+                    keepOriginalName: true // Don't generate a new filename on save
                 }, processedImageDir);
 
                 return processedImageUrl;

ghost/core/core/server/services/themes/storage.js

@@ -46,7 +46,7 @@ module.exports = {
         });
     },
     setFromZip: async (zip) => {
-        const themeName = getStorage().getSanitizedFileName(zip.name.split('.zip')[0]);
+        const themeName = getStorage().sanitizeFileName(zip.name.split('.zip')[0]);
         const backupName = `${themeName}_${ObjectID()}`;
 
         // check if zip name matches one of the default themes
@@ -63,6 +63,7 @@ module.exports = {
         try {
             checkedTheme = await validate.checkSafe(themeName, zip, true);
             const themeExists = await getStorage().exists(themeName);
+
             // CASE: move the existing theme to a backup folder
             if (themeExists) {
                 debug('setFromZip Theme exists already');
@@ -73,7 +74,8 @@ module.exports = {
             // CASE: store extracted theme
             await getStorage().save({
                 name: themeName,
-                path: checkedTheme.path
+                path: checkedTheme.path,
+                keepOriginalName: true
             });
 
             // CASE: loads the theme from the fs & sets the theme on the themeList

ghost/core/package.json

@@ -189,7 +189,7 @@
     "express-queue": "0.0.13",
     "express-session": "1.18.1",
     "fs-extra": "11.2.0",
-    "ghost-storage-base": "1.0.0",
+    "ghost-storage-base": "1.1.1",
     "glob": "8.1.0",
     "got": "11.8.6",
     "gscan": "4.46.0",

ghost/core/test/e2e-api/admin/files.test.js

@@ -29,7 +29,7 @@ describe('Files API', function () {
             .attach('file', path.join(__dirname, '/../../utils/fixtures/images/loadingcat_square.gif'))
             .expect(201);
 
-        res.body.files[0].url.should.match(new RegExp(`${config.get('url')}/content/files/\\d+/\\d+/loadingcat_square.gif`));
+        res.body.files[0].url.should.match(new RegExp(`${config.get('url')}/content/files/\\d+/\\d+/loadingcat_square-\\w{16}\\.gif`));
         res.body.files[0].ref.should.equal('934203942');
 
         files.push(res.body.files[0].url.replace(config.get('url'), ''));

ghost/core/test/e2e-api/admin/images.test.js

@@ -13,6 +13,7 @@ const {anyErrorId} = matchers;
 const {imageSize} = require('../../../core/server/lib/image');
 const configUtils = require('../../utils/configUtils');
 const logging = require('@tryghost/logging');
+const crypto = require('crypto');
 
 const images = [];
 let agent, frontendAgent, ghostServer;
@@ -49,26 +50,31 @@ const uploadImageRequest = ({fileContents, filename, contentType, ref}) => {
  * @param {string} options.filename
  * @param {string} options.contentType
  * @param {string} [options.expectedFileName]
- * @param {string} [options.expectedOriginalFileName]
  * @param {string} [options.ref]
  * @param {boolean} [options.skipOriginal]
  * @returns
  */
-const uploadImageCheck = async ({path, filename, contentType, expectedFileName, expectedOriginalFileName, ref, skipOriginal}) => {
+const uploadImageCheck = async ({path, filename, contentType, expectedFileName, ref, skipOriginal}) => {
     const fileContents = await fs.readFile(path);
     const {body} = await uploadImageRequest({fileContents, filename, contentType, ref}).expectStatus(201);
+
     expectedFileName = expectedFileName || filename;
+    const parsedFileName = p.parse(expectedFileName);
+    const expectedFileNameRegex = `${parsedFileName.name}-\\w{16}${parsedFileName.ext}`;
 
-    assert.match(body.images[0].url, new RegExp(`${urlUtils.urlFor('home', true)}content/images/\\d+/\\d+/${expectedFileName}`));
+    assert.match(body.images[0].url, new RegExp(`${urlUtils.urlFor('home', true)}content/images/\\d+/\\d+/${expectedFileNameRegex}`));
     assert.equal(body.images[0].ref, ref === undefined ? null : ref);
 
     const relativePath = body.images[0].url.replace(urlUtils.urlFor('home', true), '/');
     const filePath = config.getContentPath('images') + relativePath.replace('/content/images/', '');
     images.push(filePath);
 
     // Get original image path
-    const originalFilePath = skipOriginal ? filePath : (expectedOriginalFileName ? filePath.replace(expectedFileName, expectedOriginalFileName) : imageTransform.generateOriginalImageName(filePath));
-    images.push(originalFilePath);
+    let originalFilePath = filePath;
+    if (!skipOriginal) {
+        originalFilePath = imageTransform.generateOriginalImageName(filePath);
+        images.push(originalFilePath);
+    }
 
     // Check the image is saved to disk
     const saved = await fs.readFile(originalFilePath);
@@ -242,20 +248,6 @@ describe('Images API', function () {
         await uploadImageCheck({path: originalFilePath, filename: 'loadingcat_square.gif', contentType: 'image/gif'});
     });
 
-    it('Will error when filename is too long', async function () {
-        const originalFilePath = p.join(__dirname, '/../../utils/fixtures/images/ghost-logo.png');
-        const fileContents = await fs.readFile(originalFilePath);
-        const loggingStub = sinon.stub(logging, 'error');
-        await uploadImageRequest({fileContents, filename: `${'a'.repeat(300)}.png`, contentType: 'image/png'})
-            .expectStatus(400)
-            .matchBodySnapshot({
-                errors: [{
-                    id: anyErrorId
-                }]
-            });
-        sinon.assert.calledOnce(loggingStub);
-    });
-
     it('Can not upload a json file', async function () {
         const originalFilePath = p.join(__dirname, '/../../utils/fixtures/data/redirects.json');
         const fileContents = await fs.readFile(originalFilePath);
@@ -312,12 +304,21 @@ describe('Images API', function () {
         sinon.assert.calledOnce(loggingStub);
     });
 
+    it('Can upload a file with a long name and the filename will be truncated to be under 253 bytes', async function () {
+        const originalFilePath = p.join(__dirname, '/../../utils/fixtures/images/ghost-logo.png');
+        const ext = '.png';
+        const hash = `-${crypto.randomBytes(8).toString('hex')}`;
+        const truncatedNameLength = 253 - hash.length - ext.length;
+
+        await uploadImageCheck({path: originalFilePath, filename: `${'a'.repeat(300)}.png`, expectedFileName: `${'a'.repeat(truncatedNameLength)}.png`, contentType: 'image/png'});
+    });
+
     it('Can upload multiple images with the same name', async function () {
         const originalFilePath = p.join(__dirname, '/../../utils/fixtures/images/ghost-logo.png');
         const originalFilePath2 = p.join(__dirname, '/../../utils/fixtures/images/ghosticon.jpg');
 
         await uploadImageCheck({path: originalFilePath, filename: 'a.png', contentType: 'image/png'});
-        await uploadImageCheck({path: originalFilePath2, filename: 'a.png', contentType: 'image/png', expectedFileName: 'a-1.png', expectedOriginalFileName: 'a-1_o.png'});
+        await uploadImageCheck({path: originalFilePath2, filename: 'a.png', contentType: 'image/png'});
     });
 
     it('Can upload image with number suffix', async function () {
@@ -327,12 +328,12 @@ describe('Images API', function () {
 
     it('Trims _o suffix from uploaded files', async function () {
         const originalFilePath = p.join(__dirname, '/../../utils/fixtures/images/ghost-logo.png');
-        await uploadImageCheck({path: originalFilePath, filename: 'a-3_o.png', contentType: 'image/png', expectedFileName: 'a-3.png', expectedOriginalFileName: 'a-3_o.png'});
+        await uploadImageCheck({path: originalFilePath, filename: 'a-3_o.png', contentType: 'image/png', expectedFileName: 'a-3.png'});
     });
 
     it('Can use _o in uploaded file name, as long as it is not at the end', async function () {
         const originalFilePath = p.join(__dirname, '/../../utils/fixtures/images/ghost-logo.png');
-        await uploadImageCheck({path: originalFilePath, filename: 'a_o-3.png', contentType: 'image/png', expectedFileName: 'a_o-3.png', expectedOriginalFileName: 'a_o-3_o.png'});
+        await uploadImageCheck({path: originalFilePath, filename: 'a_o-3.png', contentType: 'image/png', expectedFileName: 'a_o-3.png'});
     });
 
     it('Can upload around midnight of month change', async function () {

ghost/core/test/e2e-api/admin/media.test.js

@@ -38,8 +38,8 @@ describe('Media API', function () {
                 .attach('thumbnail', path.join(__dirname, '/../../utils/fixtures/images/ghost-logo.png'))
                 .expect(201);
 
-            res.body.media[0].url.should.match(new RegExp(`${config.get('url')}/content/media/\\d+/\\d+/sample_640x360.mp4`));
-            res.body.media[0].thumbnail_url.should.match(new RegExp(`${config.get('url')}/content/media/\\d+/\\d+/sample_640x360_thumb.png`));
+            res.body.media[0].url.should.match(new RegExp(`${config.get('url')}/content/media/\\d+/\\d+/sample_640x360-\\w{16}\\.mp4`));
+            res.body.media[0].thumbnail_url.should.match(new RegExp(`${config.get('url')}/content/media/\\d+/\\d+/sample_640x360_thumb-\\w{16}\\.png`));
             res.body.media[0].ref.should.equal('https://ghost.org/sample_640x360.mp4');
 
             media.push(res.body.media[0].url.replace(config.get('url'), ''));
@@ -54,7 +54,7 @@ describe('Media API', function () {
                 .attach('file', path.join(__dirname, '/../../utils/fixtures/media/sample_640x360.webm'))
                 .expect(201);
 
-            res.body.media[0].url.should.match(new RegExp(`${config.get('url')}/content/media/\\d+/\\d+/sample_640x360.webm`));
+            res.body.media[0].url.should.match(new RegExp(`${config.get('url')}/content/media/\\d+/\\d+/sample_640x360-\\w{16}\\.webm`));
             should(res.body.media[0].thumbnail_url).eql(null);
             res.body.media[0].ref.should.equal('https://ghost.org/sample_640x360.webm');
 
@@ -70,7 +70,7 @@ describe('Media API', function () {
                 .attach('thumbnail', path.join(__dirname, '/../../utils/fixtures/images/ghost-logo.png'))
                 .expect(201);
 
-            res.body.media[0].url.should.match(new RegExp(`${config.get('url')}/content/media/\\d+/\\d+/sample_640x360.ogv`));
+            res.body.media[0].url.should.match(new RegExp(`${config.get('url')}/content/media/\\d+/\\d+/sample_640x360-\\w{16}\\.ogv`));
             res.body.media[0].ref.should.equal('https://ghost.org/sample_640x360.ogv');
 
             media.push(res.body.media[0].url.replace(config.get('url'), ''));
@@ -84,7 +84,7 @@ describe('Media API', function () {
                 .attach('file', path.join(__dirname, '/../../utils/fixtures/media/sample.mp3'))
                 .expect(201);
 
-            res.body.media[0].url.should.match(new RegExp(`${config.get('url')}/content/media/\\d+/\\d+/sample.mp3`));
+            res.body.media[0].url.should.match(new RegExp(`${config.get('url')}/content/media/\\d+/\\d+/sample-\\w{16}\\.mp3`));
             res.body.media[0].ref.should.equal('audio_file_123');
 
             media.push(res.body.media[0].url.replace(config.get('url'), ''));
@@ -98,7 +98,7 @@ describe('Media API', function () {
                 .attach('file', path.join(__dirname, '/../../utils/fixtures/media/sample.m4a'), {filename: 'audio-mp4.m4a', contentType: 'audio/mp4'})
                 .expect(201);
 
-            res.body.media[0].url.should.match(new RegExp(`${config.get('url')}/content/media/\\d+/\\d+/audio-mp4.m4a`));
+            res.body.media[0].url.should.match(new RegExp(`${config.get('url')}/content/media/\\d+/\\d+/audio-mp4-\\w{16}\\.m4a`));
             res.body.media[0].ref.should.equal('audio_file_mp4');
 
             media.push(res.body.media[0].url.replace(config.get('url'), ''));
@@ -112,7 +112,7 @@ describe('Media API', function () {
                 .attach('file', path.join(__dirname, '/../../utils/fixtures/media/sample.m4a'), {filename: 'audio-x-m4a.m4a', contentType: 'audio/x-m4a'})
                 .expect(201);
 
-            res.body.media[0].url.should.match(new RegExp(`${config.get('url')}/content/media/\\d+/\\d+/audio-x-m4a.m4a`));
+            res.body.media[0].url.should.match(new RegExp(`${config.get('url')}/content/media/\\d+/\\d+/audio-x-m4a-\\w{16}\\.m4a`));
             res.body.media[0].ref.should.equal('audio_file_x_m4a');
 
             media.push(res.body.media[0].url.replace(config.get('url'), ''));
@@ -188,7 +188,7 @@ describe('Media API', function () {
                 .attach('thumbnail', path.join(__dirname, '/../../utils/fixtures/images/ghost-logo.png'))
                 .expect(201);
 
-            res.body.media[0].ref.should.equal('https://ghost.org/sample_640x360.mp4');
+            res.body.media[0].ref.should.match(/https:\/\/ghost\.org\/sample_640x360.mp4/);
 
             media.push(res.body.media[0].url.replace(config.get('url'), ''));
             media.push(res.body.media[0].thumbnail_url.replace(config.get('url'), ''));
@@ -201,8 +201,8 @@ describe('Media API', function () {
                 .attach('file', path.join(__dirname, '/../../utils/fixtures/images/ghosticon.jpg'))
                 .expect(200);
 
-            const thumbnailUrl = res.body.media[0].url.replace('.mp4', '_thumb.jpg');
-            thumbnailRes.body.media[0].url.should.equal(thumbnailUrl);
+            const mediaUrlWithoutExt = res.body.media[0].url.replace('.mp4', '');
+            thumbnailRes.body.media[0].url.should.match(new RegExp(`${mediaUrlWithoutExt}_thumb-\\w{16}\\.jpg`));
             thumbnailRes.body.media[0].ref.should.equal('updated_thumbnail');
             media.push(thumbnailRes.body.media[0].url.replace(config.get('url'), ''));
         });
@@ -225,8 +225,8 @@ describe('Media API', function () {
                 .attach('file', path.join(__dirname, '/../../utils/fixtures/images/ghosticon.jpg'))
                 .expect(200);
 
-            const thumbnailUrl = res.body.media[0].url.replace('.mp4', '_thumb.jpg');
-            thumbnailRes.body.media[0].url.should.equal(thumbnailUrl);
+            const mediaUrlWithoutExt = res.body.media[0].url.replace('.mp4', '');
+            thumbnailRes.body.media[0].url.should.match(new RegExp(`${mediaUrlWithoutExt}_thumb-\\w{16}\\.jpg`));
             thumbnailRes.body.media[0].ref.should.equal('updated_thumbnail_2');
 
             media.push(thumbnailRes.body.media[0].url.replace(config.get('url'), ''));