Improved storage path validation (#27217)

kevinansfield · web-flow · commit 770d438fd9d3 · 2026-04-08T09:44:00.000Z
ref https://linear.app/ghost/issue/ONC-1595/ - shared path normalization across local storage reads and URL conversion so image path handling stays consistent
diff --git a/ghost/core/core/server/adapters/storage/LocalStorageBase.js b/ghost/core/core/server/adapters/storage/LocalStorageBase.js
@@ -13,7 +13,8 @@ const messages = {
     notFound: 'File not found',
     notFoundWithRef: 'File not found: {file}',
     cannotRead: 'Could not read file: {file}',
-    invalidUrlParameter: `The URL "{url}" is not a valid URL for this site.`
+    invalidUrlParameter: `The URL "{url}" is not a valid URL for this site.`,
+    invalidPathParameter: 'The path "{path}" is not valid for this storage.'
 };
 
 class LocalStorageBase extends StorageBase {
@@ -38,6 +39,75 @@ class LocalStorageBase extends StorageBase {
         this.errorMessages = errorMessages || messages;
     }
 
+    /**
+     * Normalizes a relative storage path and rejects traversal outside the storage root.
+     *
+     * @param {string} filePath
+     * @returns {string}
+     */
+    _normalizeStorageRelativePath(filePath) {
+        const normalized = path.posix.normalize(String(filePath || '')
+            .replaceAll('\\', '/')
+            .replace(/^\/+/, '')
+            .replace(/\/+$/, ''));
+
+        if (normalized === '.' || normalized === '..' || normalized.startsWith('../')) {
+            throw new errors.IncorrectUsageError({
+                message: tpl(messages.invalidPathParameter, {path: filePath})
+            });
+        }
+
+        return normalized;
+    }
+
+    /**
+     * Resolves a target directory and optional file name into a full path,
+     * validating the result is inside the storage root.
+     *
+     * Supports relative paths (preferred) and absolute paths (legacy).
+     * TODO: remove absolute path support once all callers pass relative paths
+     *
+     * @param {String} [targetDir] absolute or relative directory
+     * @param {String} [fileName] file name to normalize and append
+     * @returns {String} resolved absolute path inside storagePath
+     */
+    _resolveAndValidateStoragePath(targetDir, fileName) {
+        const resolvedRoot = path.resolve(this.storagePath);
+
+        // Resolve targetDir: if already inside storage root use as-is, otherwise treat as relative
+        let resolvedBase;
+        if (targetDir) {
+            const resolvedTargetDir = path.resolve(targetDir);
+            const relToRoot = path.relative(resolvedRoot, resolvedTargetDir);
+            if (relToRoot === '' || (!relToRoot.startsWith('..') && !path.isAbsolute(relToRoot))) {
+                resolvedBase = resolvedTargetDir;
+            } else {
+                resolvedBase = path.resolve(this.storagePath, targetDir);
+            }
+        } else {
+            resolvedBase = resolvedRoot;
+        }
+
+        // If fileName provided, normalize and resolve
+        let resolvedPath;
+        if (fileName) {
+            const normalizedFileName = this._normalizeStorageRelativePath(fileName);
+            resolvedPath = path.resolve(resolvedBase, normalizedFileName);
+        } else {
+            resolvedPath = resolvedBase;
+        }
+
+        // Validate the resolved path is strictly inside the storage root (not equal to it)
+        const relative = path.relative(resolvedRoot, resolvedPath);
+        if (relative === '' || relative === '..' || relative.startsWith('..' + path.sep) || path.isAbsolute(relative)) {
+            throw new errors.IncorrectUsageError({
+                message: tpl(messages.invalidPathParameter, {path: fileName || targetDir || ''})
+            });
+        }
+
+        return resolvedPath;
+    }
+
     /**
      * Saves the file to storage (the file system)
      * - returns a promise which ultimately returns the full url to the uploaded file
@@ -49,13 +119,9 @@ class LocalStorageBase extends StorageBase {
     async save(file, targetDir) {
         let targetFilename;
 
-        // Supports relative paths (preferred) and absolute paths (legacy)
-        // TODO: remove absolute path support once all callers pass relative paths
-        if (targetDir && !targetDir.startsWith(this.storagePath)) {
-            targetDir = path.join(this.storagePath, targetDir);
-        }
-
-        targetDir = targetDir || this.getTargetDir(this.storagePath);
+        targetDir = targetDir
+            ? this._resolveAndValidateStoragePath(targetDir)
+            : this.getTargetDir(this.storagePath);
 
         const filename = await this.getUniqueFileName(file, targetDir);
 
@@ -91,7 +157,7 @@ class LocalStorageBase extends StorageBase {
      * @returns {Promise<String>} a URL to retrieve the data
      */
     async saveRaw(buffer, targetPath) {
-        const storagePath = path.join(this.storagePath, targetPath);
+        const storagePath = path.join(this.storagePath, this._normalizeStorageRelativePath(targetPath));
         const targetDir = path.dirname(storagePath);
 
         await fs.mkdirs(targetDir);
@@ -131,31 +197,34 @@ class LocalStorageBase extends StorageBase {
             });
         }
 
-        const normalized = path.posix.normalize(relativePath.replace(/^\//, ''));
-        if (normalized.startsWith('..')) {
+        try {
+            return this._normalizeStorageRelativePath(relativePath);
+        } catch (err) {
             throw new errors.IncorrectUsageError({
                 message: tpl(messages.invalidUrlParameter, {url})
             });
         }
-
-        return normalized;
     }
 
-    exists(fileName, targetDir) {
-        // Supports relative paths (preferred) and absolute paths (legacy)
-        // TODO: remove absolute path support once all callers pass relative paths
-        if (targetDir && !targetDir.startsWith(this.storagePath)) {
-            targetDir = path.join(this.storagePath, targetDir);
-        }
-        const filePath = path.join(targetDir || this.storagePath, fileName);
+    async exists(fileName, targetDir) {
+        let filePath;
 
-        return fs.stat(filePath)
-            .then(() => {
-                return true;
-            })
-            .catch(() => {
+        try {
+            filePath = this._resolveAndValidateStoragePath(targetDir, fileName);
+        } catch (err) {
+            if (err instanceof errors.IncorrectUsageError) {
                 return false;
-            });
+            }
+
+            throw err;
+        }
+
+        try {
+            await fs.stat(filePath);
+            return true;
+        } catch {
+            return false;
+        }
     }
 
     /**
@@ -210,12 +279,7 @@ class LocalStorageBase extends StorageBase {
      * @returns {Promise.<*>}
      */
     async delete(fileName, targetDir) {
-        // Supports relative paths (preferred) and absolute paths (legacy)
-        // TODO: remove absolute path support once all callers pass relative paths
-        if (targetDir && !targetDir.startsWith(this.storagePath)) {
-            targetDir = path.join(this.storagePath, targetDir);
-        }
-        const filePath = path.join(targetDir || this.storagePath, fileName);
+        const filePath = this._resolveAndValidateStoragePath(targetDir, fileName);
         return await fs.remove(filePath);
     }
 
@@ -225,41 +289,35 @@ class LocalStorageBase extends StorageBase {
      *
      * @param options
      */
-    read(options) {
+    async read(options) {
         options = options || {};
 
-        // remove trailing slashes
-        options.path = (options.path || '').replace(/\/$|\\$/, '');
+        const normalizedPath = this._normalizeStorageRelativePath(options.path);
+        const targetPath = path.join(this.storagePath, normalizedPath);
 
-        const targetPath = path.join(this.storagePath, options.path);
-
-        return new Promise((resolve, reject) => {
-            fs.readFile(targetPath, (err, bytes) => {
-                if (err) {
-                    if (err.code === 'ENOENT' || err.code === 'ENOTDIR') {
-                        return reject(new errors.NotFoundError({
-                            err: err,
-                            message: tpl(this.errorMessages.notFoundWithRef, {file: options.path})
-                        }));
-                    }
-
-                    if (err.code === 'ENAMETOOLONG') {
-                        return reject(new errors.BadRequestError({err: err}));
-                    }
+        try {
+            return await fs.readFile(targetPath);
+        } catch (err) {
+            if (err.code === 'ENOENT' || err.code === 'ENOTDIR') {
+                throw new errors.NotFoundError({
+                    err: err,
+                    message: tpl(this.errorMessages.notFoundWithRef, {file: options.path})
+                });
+            }
 
-                    if (err.code === 'EACCES') {
-                        return reject(new errors.NoPermissionError({err: err}));
-                    }
+            if (err.code === 'ENAMETOOLONG') {
+                throw new errors.BadRequestError({err: err});
+            }
 
-                    return reject(new errors.InternalServerError({
-                        err: err,
-                        message: tpl(this.errorMessages.cannotRead, {file: options.path})
-                    }));
-                }
+            if (err.code === 'EACCES') {
+                throw new errors.NoPermissionError({err: err});
+            }
 
-                resolve(bytes);
+            throw new errors.InternalServerError({
+                err: err,
+                message: tpl(this.errorMessages.cannotRead, {file: options.path})
             });
-        });
+        }
     }
 }
 
diff --git a/ghost/core/test/unit/server/adapters/storage/local-base-storage.test.js b/ghost/core/test/unit/server/adapters/storage/local-base-storage.test.js
@@ -2,6 +2,8 @@ const assert = require('assert/strict');
 const path = require('path');
 const http = require('http');
 const express = require('express');
+const sinon = require('sinon');
+const fs = require('fs-extra');
 const LocalStorageBase = require('../../../../../core/server/adapters/storage/LocalStorageBase');
 
 describe('Local Storage Base', function () {
@@ -95,4 +97,121 @@ describe('Local Storage Base', function () {
             }, {message: 'The URL "http://anothersite.com/blog/content/media/2021/11/media.mp4" is not a valid URL for this site.'});
         });
     });
+
+    describe('path validation', function () {
+        it('read rejects if the path resolves outside the storage root', async function () {
+            const localStorageBase = new LocalStorageBase({
+                storagePath: '/media-storage/path/',
+                staticFileURLPrefix: 'content/media',
+                siteUrl: 'http://example.com/blog/'
+            });
+
+            await assert.rejects(
+                localStorageBase.read({path: '../../outside-root.txt'}),
+                {message: 'The path "../../outside-root.txt" is not valid for this storage.'}
+            );
+        });
+
+        it('exists returns false if the path resolves outside the storage root', async function () {
+            const localStorageBase = new LocalStorageBase({
+                storagePath: '/media-storage/path/',
+                staticFileURLPrefix: 'content/media',
+                siteUrl: 'http://example.com/blog/'
+            });
+
+            await assert.doesNotReject(async function () {
+                const exists = await localStorageBase.exists('../../outside-root.txt');
+                assert.equal(exists, false);
+            });
+        });
+
+        it('read rejects dot-equivalent paths', async function () {
+            const localStorageBase = new LocalStorageBase({
+                storagePath: '/media-storage/path/',
+                staticFileURLPrefix: 'content/media',
+                siteUrl: 'http://example.com/blog/'
+            });
+
+            await assert.rejects(
+                localStorageBase.read({path: 'foo/..'}),
+                {message: 'The path "foo/.." is not valid for this storage.'}
+            );
+        });
+
+        it('exists returns false for dot-equivalent paths', async function () {
+            const localStorageBase = new LocalStorageBase({
+                storagePath: '/media-storage/path/',
+                staticFileURLPrefix: 'content/media',
+                siteUrl: 'http://example.com/blog/'
+            });
+
+            const exists = await localStorageBase.exists('.');
+            assert.equal(exists, false);
+        });
+
+        it('delete rejects dot-equivalent paths', async function () {
+            const localStorageBase = new LocalStorageBase({
+                storagePath: '/media-storage/path/',
+                staticFileURLPrefix: 'content/media',
+                siteUrl: 'http://example.com/blog/'
+            });
+
+            await assert.rejects(
+                localStorageBase.delete(''),
+                {message: 'The path "" is not valid for this storage.'}
+            );
+        });
+
+        it('exists rejects when targetDir resolves outside storage root via traversal', async function () {
+            // Stub fs.stat to always succeed so we can detect traversal
+            // rather than having it masked by file-not-found
+            const statStub = sinon.stub(fs, 'stat').resolves({});
+
+            try {
+                const localStorageBase = new LocalStorageBase({
+                    storagePath: '/media-storage/path/',
+                    staticFileURLPrefix: 'content/media',
+                    siteUrl: 'http://example.com/blog/'
+                });
+
+                // targetDir traverses out of storage root — should return false, not attempt access
+                const exists = await localStorageBase.exists('file.txt', '../../etc');
+                assert.equal(exists, false, 'should be false because the path escapes the storage root');
+            } finally {
+                statStub.restore();
+            }
+        });
+
+        it('exists rejects when targetDir prefix-matches but is not inside storage root', async function () {
+            const statStub = sinon.stub(fs, 'stat').resolves({});
+
+            try {
+                const localStorageBase = new LocalStorageBase({
+                    storagePath: '/media-storage/path',
+                    staticFileURLPrefix: 'content/media',
+                    siteUrl: 'http://example.com/blog/'
+                });
+
+                // targetDir starts with storagePath string but is a sibling directory
+                // naive startsWith would treat this as already absolute + inside root
+                const exists = await localStorageBase.exists('file.txt', '/media-storage/path-evil');
+                assert.equal(exists, false, 'should be false because path-evil is not inside path/');
+            } finally {
+                statStub.restore();
+            }
+        });
+
+        it('delete rejects when targetDir resolves outside storage root', async function () {
+            const localStorageBase = new LocalStorageBase({
+                storagePath: '/media-storage/path/',
+                staticFileURLPrefix: 'content/media',
+                siteUrl: 'http://example.com/blog/'
+            });
+
+            await assert.rejects(
+                localStorageBase.delete('file.txt', '../../etc'),
+                {message: 'The path "file.txt" is not valid for this storage.'}
+            );
+        });
+    });
 });
diff --git a/ghost/core/test/unit/server/lib/image/image-size.test.js b/ghost/core/test/unit/server/lib/image/image-size.test.js
