diff --git a/ghost/core/core/frontend/web/middleware/handle-image-sizes.js b/ghost/core/core/frontend/web/middleware/handle-image-sizes.js index 1fcc68e2153..a6d569c1742 100644 --- a/ghost/core/core/frontend/web/middleware/handle-image-sizes.js +++ b/ghost/core/core/frontend/web/middleware/handle-image-sizes.js @@ -111,14 +111,19 @@ module.exports = function handleImageSizes(req, res, next) { return redirectToOriginal(); } - storageInstance.exists(req.url).then((exists) => { + // Strip the leading slashes that Express + the format/size regex remove + // in the URL space so storage adapters see canonical relative paths. + const storageRelativePath = req.url.replace(/^\/+/, ''); + const originalImagePath = imagePath.replace(/^\/+/, ''); + + storageInstance.exists(storageRelativePath).then((exists) => { if (exists) { return; } - return imageSize.getOriginalImagePath(imagePath) - .then((storagePath) => { - return storageInstance.read({path: storagePath}); + return imageSize.getOriginalImagePath(originalImagePath) + .then((originalRelativePath) => { + return storageInstance.read({path: originalRelativePath}); }) .then((originalImageBuffer) => { if (originalImageBuffer.length <= 0) { @@ -132,7 +137,7 @@ module.exports = function handleImageSizes(req, res, next) { }); }) .then((resizedImageBuffer) => { - return storageInstance.saveRaw(resizedImageBuffer, req.url); + return storageInstance.saveRaw(resizedImageBuffer, storageRelativePath); }); }).then(() => { if (format) { diff --git a/ghost/core/core/server/adapters/storage/LocalStorageBase.js b/ghost/core/core/server/adapters/storage/LocalStorageBase.js index fad711adb3a..a5a786b3bd6 100644 --- a/ghost/core/core/server/adapters/storage/LocalStorageBase.js +++ b/ghost/core/core/server/adapters/storage/LocalStorageBase.js @@ -8,6 +8,7 @@ const tpl = require('@tryghost/tpl'); const errors = require('@tryghost/errors'); const urlUtils = require('../../../shared/url-utils'); const StorageBase = require('ghost-storage-base'); +const {assertCanonicalFilePath, assertCanonicalDirPath} = require('./utils'); const messages = { notFound: 'File not found', @@ -37,75 +38,32 @@ class LocalStorageBase extends StorageBase { this.siteUrl = siteUrl; this.staticFileUrl = `${siteUrl}${staticFileURLPrefix}`; 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; + // Bind the path validators to this instance's storagePath so call + // sites don't have to repeat it on every assertion. + this._assertFilePath = input => assertCanonicalFilePath(input, this.storagePath); + this._assertDirPath = input => assertCanonicalDirPath(input, this.storagePath); } /** - * 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 + * Resolve a relative-to-storage path or path pair into a full filesystem + * path inside the storage root. Inputs must already be canonical + * (validated at the public entry points); this just composes the on-disk + * path. * - * @param {string} [targetDir] absolute or relative directory - * @param {string} [fileName] file name to normalize and append - * @returns {string} resolved absolute path inside storagePath + * @param {string} [targetDir] + * @param {string} [fileName] + * @returns {string} */ - _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; + _resolveStoragePath(targetDir, fileName) { + const segments = [this.storagePath]; 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; + segments.push(targetDir); } - - // 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 || ''}) - }); + segments.push(fileName); } - - return resolvedPath; + return path.join(...segments); } /** @@ -117,19 +75,21 @@ class LocalStorageBase extends StorageBase { * @returns {Promise} */ async save(file, targetDir) { - let targetFilename; - - targetDir = targetDir - ? this._resolveAndValidateStoragePath(targetDir) - : this.getTargetDir(this.storagePath); - - const filename = await this.getUniqueFileName(file, targetDir); + if (targetDir !== undefined) { + this._assertDirPath(targetDir); + } + // Keep the dir relative through generateUnique/exists so the strict + // validator sees canonical inputs. Only resolve to absolute when we + // actually touch the filesystem. Distinguish "no targetDir" (year/month + // default) from explicit empty string (storage root). + const relativeDir = targetDir !== undefined ? targetDir : this.getTargetDir(); + const relativeFilePath = await this.getUniqueFileName(file, relativeDir); - targetFilename = filename; - await fs.mkdirs(targetDir); + const absoluteFilePath = path.join(this.storagePath, relativeFilePath); + await fs.mkdirs(path.dirname(absoluteFilePath)); try { - await fs.copy(file.path, targetFilename); + await fs.copy(file.path, absoluteFilePath); } catch (err) { if (err.code === 'ENAMETOOLONG') { throw new errors.BadRequestError({err}); @@ -144,7 +104,7 @@ class LocalStorageBase extends StorageBase { urlUtils.urlJoin('/', urlUtils.getSubdir(), this.staticFileURLPrefix, - path.relative(this.storagePath, targetFilename)) + relativeFilePath) ).replace(new RegExp(`\\${path.sep}`, 'g'), '/'); return fullUrl; @@ -157,7 +117,9 @@ class LocalStorageBase extends StorageBase { * @returns {Promise} a URL to retrieve the data */ async saveRaw(buffer, targetPath) { - const storagePath = path.join(this.storagePath, this._normalizeStorageRelativePath(targetPath)); + this._assertFilePath(targetPath); + + const storagePath = path.join(this.storagePath, targetPath); const targetDir = path.dirname(storagePath); await fs.mkdirs(targetDir); @@ -197,28 +159,40 @@ class LocalStorageBase extends StorageBase { }); } - try { - return this._normalizeStorageRelativePath(relativePath); - } catch (err) { + // Strip any leading/trailing slashes and normalise `..` segments before + // returning a canonical relative path. + const normalized = path.posix.normalize(relativePath + .replaceAll('\\', '/') + .replace(/^\/+/, '') + .replace(/\/+$/, '')); + + if (normalized === '.' || normalized === '..' || normalized.startsWith('../')) { throw new errors.IncorrectUsageError({ message: tpl(messages.invalidUrlParameter, {url}) }); } + + return normalized; } async exists(fileName, targetDir) { - let filePath; - + // exists() is a query, not a mutation: a malformed input is "no, that + // doesn't exist" rather than a programmer-error throw. Mutating methods + // (save/saveRaw/delete) still throw on the same validator failures. try { - filePath = this._resolveAndValidateStoragePath(targetDir, fileName); + this._assertFilePath(fileName); + if (targetDir !== undefined) { + this._assertDirPath(targetDir); + } } catch (err) { if (err instanceof errors.IncorrectUsageError) { return false; } - throw err; } + const filePath = this._resolveStoragePath(targetDir, fileName); + try { await fs.stat(filePath); return true; @@ -279,7 +253,12 @@ class LocalStorageBase extends StorageBase { * @returns {Promise.<*>} */ async delete(fileName, targetDir) { - const filePath = this._resolveAndValidateStoragePath(targetDir, fileName); + this._assertFilePath(fileName); + if (targetDir !== undefined) { + this._assertDirPath(targetDir); + } + + const filePath = this._resolveStoragePath(targetDir, fileName); return await fs.remove(filePath); } @@ -292,8 +271,8 @@ class LocalStorageBase extends StorageBase { async read(options) { options = options || {}; - const normalizedPath = this._normalizeStorageRelativePath(options.path); - const targetPath = path.join(this.storagePath, normalizedPath); + this._assertFilePath(options.path); + const targetPath = path.join(this.storagePath, options.path); try { return await fs.readFile(targetPath); diff --git a/ghost/core/core/server/adapters/storage/S3Storage.ts b/ghost/core/core/server/adapters/storage/S3Storage.ts index 433c1b9fce0..7f3a131e3a6 100644 --- a/ghost/core/core/server/adapters/storage/S3Storage.ts +++ b/ghost/core/core/server/adapters/storage/S3Storage.ts @@ -18,6 +18,7 @@ import { S3Client, S3ClientConfig } from '@aws-sdk/client-s3'; +import {assertCanonicalFilePath, assertCanonicalDirPath} from './utils'; // Minimum chunk size for multipart uploads (5 MiB) - required by S3/GCS // GCS limits: https://docs.cloud.google.com/storage/quotas#requests @@ -83,6 +84,12 @@ export default class S3Storage extends StorageBase { private readonly multipartChunkSizeBytes: number; + // Bound to this instance's storagePath in the constructor so call sites + // don't have to repeat it on every assertion. + private readonly _assertFilePath: (input: string) => void; + + private readonly _assertDirPath: (input: string) => void; + constructor(options: S3StorageOptions) { super(); @@ -147,10 +154,19 @@ export default class S3Storage extends StorageBase { } this.client = options.s3Client || new S3Client(clientConfig); + + this._assertFilePath = input => assertCanonicalFilePath(input, this.storagePath); + this._assertDirPath = input => assertCanonicalDirPath(input, this.storagePath); } async save(file: UploadFile, targetDir?: string): Promise { - const dir = targetDir || this.getTargetDir(); + if (targetDir !== undefined) { + this._assertDirPath(targetDir); + } + // Distinguish "no targetDir given" (use the year/month default) from + // an explicit empty string (storage root, used by the importer for + // files at the top of an import zip). + const dir = targetDir !== undefined ? targetDir : this.getTargetDir(); const relativePath = await this.getUniqueFileName(file, dir); const key = this.buildKey(relativePath); @@ -261,11 +277,7 @@ export default class S3Storage extends StorageBase { } async saveRaw(buffer: Buffer, targetPath: string): Promise { - if (!targetPath?.trim()) { - throw new errors.IncorrectUsageError({ - message: tpl(messages.emptyTargetPath) - }); - } + this._assertFilePath(targetPath); const key = this.buildKey(targetPath); @@ -320,10 +332,19 @@ export default class S3Storage extends StorageBase { } async exists(fileName: string, targetDir?: string): Promise { - if (!fileName?.trim()) { - throw new errors.IncorrectUsageError({ - message: tpl(messages.emptyFileName, {method: 'exists'}) - }); + // exists() is a query, not a mutation: a malformed input is "no, that + // doesn't exist" rather than a programmer-error throw. Mutating methods + // (save/saveRaw/delete) still throw on the same validator failures. + try { + this._assertFilePath(fileName); + if (targetDir !== undefined) { + this._assertDirPath(targetDir); + } + } catch (err) { + if (err instanceof errors.IncorrectUsageError) { + return false; + } + throw err; } const relativePath = targetDir ? path.posix.join(targetDir, fileName) : fileName; @@ -358,10 +379,9 @@ export default class S3Storage extends StorageBase { } async delete(fileName: string, targetDir?: string): Promise { - if (!fileName?.trim()) { - throw new errors.IncorrectUsageError({ - message: tpl(messages.emptyFileName, {method: 'delete'}) - }); + this._assertFilePath(fileName); + if (targetDir !== undefined) { + this._assertDirPath(targetDir); } const relativePath = targetDir ? path.posix.join(targetDir, fileName) : fileName; diff --git a/ghost/core/core/server/adapters/storage/utils.js b/ghost/core/core/server/adapters/storage/utils.js index c36fce36edb..c162dac7a1f 100644 --- a/ghost/core/core/server/adapters/storage/utils.js +++ b/ghost/core/core/server/adapters/storage/utils.js @@ -1,9 +1,74 @@ +const path = require('path'); +const errors = require('@tryghost/errors'); const config = require('../../../shared/config'); const urlUtils = require('../../../shared/url-utils'); /** * @TODO: move `events.js` to here - e.g. storageUtils.getStorage */ +/** + * Storage adapters accept a single canonical input shape: a path relative to + * the storage root, with no leading slash, no storagePath prefix, and no + * `..` segments. Validate inputs at the adapter boundary so all callers + * agree on the shape. + * + * @param {string} input - the path argument to validate + * @param {string} storagePath - the storage root prefix (e.g. 'content/images') + * @param {string} [paramName='path'] - parameter name to surface in errors + */ +/** + * Validate the *shape* of a path argument — relative, no leading slash, no + * storagePath prefix, no traversal. Used internally by both the file-path + * and directory-path validators below. + */ +function assertCanonicalShape(input, storagePath) { + if (input.startsWith('/')) { + throw new errors.IncorrectUsageError({ + message: `Storage path must be relative to the storage root, not absolute (received "${input}")` + }); + } + if (input === storagePath || input.startsWith(`${storagePath}/`)) { + throw new errors.IncorrectUsageError({ + message: `Storage path must not include the storagePath prefix "${storagePath}" (received "${input}")` + }); + } + const normalized = path.posix.normalize(input); + if (normalized === '.' || normalized === '..' || normalized.startsWith('../')) { + throw new errors.IncorrectUsageError({ + message: `Storage path must not escape the storage root (received "${input}")` + }); + } +} + +/** + * Validate a *file* path: a canonical relative path that must name a file. + * Empty is rejected — you can't read or write nothing. + */ +exports.assertCanonicalFilePath = function assertCanonicalFilePath(input, storagePath) { + if (typeof input !== 'string' || input.length === 0) { + throw new errors.IncorrectUsageError({ + message: 'Storage requires a non-empty file path' + }); + } + assertCanonicalShape(input, storagePath); +}; + +/** + * Validate a *directory* path: a canonical relative path that may be the + * empty string, representing the storage root itself. + */ +exports.assertCanonicalDirPath = function assertCanonicalDirPath(input, storagePath) { + if (typeof input !== 'string') { + throw new errors.IncorrectUsageError({ + message: 'Storage directory path must be a string' + }); + } + if (input.length === 0) { + return; + } + assertCanonicalShape(input, storagePath); +}; + /** * Sanitizes a given URL or path for an image to be readable by the local file storage * as storage needs the path without `/content/images/` prefix @@ -28,13 +93,17 @@ exports.getLocalImagesStoragePath = function getLocalImagesStoragePath(imagePath urlUtils.STATIC_IMAGE_URL_PREFIX)}` ); + let stripped; if (imagePath.match(urlRegExp)) { - return imagePath.replace(urlRegExp, ''); + stripped = imagePath.replace(urlRegExp, ''); } else if (imagePath.match(filePathRegExp)) { - return imagePath.replace(filePathRegExp, ''); + stripped = imagePath.replace(filePathRegExp, ''); } else { - return imagePath; + stripped = imagePath; } + // Storage adapters require canonical relative paths — strip any leading + // slash left over from the URL form. + return stripped.replace(/^\/+/, ''); }; /** diff --git a/ghost/core/core/server/data/importer/handlers/image.js b/ghost/core/core/server/data/importer/handlers/image.js index af9cf52a4a2..217927a1ef7 100644 --- a/ghost/core/core/server/data/importer/handlers/image.js +++ b/ghost/core/core/server/data/importer/handlers/image.js @@ -30,15 +30,17 @@ ImageHandler = { file.originalPath = noBaseDir; file.name = noGhostDirs; - file.targetDir = path.join(config.getContentPath('images'), path.dirname(noGhostDirs)); + // path.dirname returns '.' when the file is at the root of the + // import; the adapter expects either a relative subdirectory or + // the empty string (representing the storage root itself). + const dir = path.dirname(noGhostDirs); + file.targetDir = (dir === '.' || dir === '') ? '' : dir; return file; }); return Promise.all(files.map(function (image) { - return store.getUniqueFileName(image, image.targetDir).then(function (targetFilename) { - image.newPath = urlUtils.urlJoin('/', urlUtils.getSubdir(), store.staticFileURLPrefix, - path.relative(config.getContentPath('images'), targetFilename)); - + return store.getUniqueFileName(image, image.targetDir).then(function (relativeFilename) { + image.newPath = urlUtils.urlJoin('/', urlUtils.getSubdir(), store.staticFileURLPrefix, relativeFilename); return image; }); })); diff --git a/ghost/core/core/server/services/media-inliner/external-media-inliner.js b/ghost/core/core/server/services/media-inliner/external-media-inliner.js index db44850a9b1..0a945ed60bd 100644 --- a/ghost/core/core/server/services/media-inliner/external-media-inliner.js +++ b/ghost/core/core/server/services/media-inliner/external-media-inliner.js @@ -138,13 +138,8 @@ class ExternalMediaInliner { logging.warn(`No storage adapter found for file extension: ${media.extension}`); return null; } else { - // @NOTE: this is extremely convoluted and should live on a - // storage adapter level - const targetDir = storage.getTargetDir(storage.storagePath); - const uniqueFileName = await storage.getUniqueFileName({ - name: media.filename - }, targetDir); - const targetPath = path.relative(storage.storagePath, uniqueFileName); + const targetDir = storage.getTargetDir(); + const targetPath = await storage.getUniqueFileName({name: media.filename}, targetDir); const filePath = await storage.saveRaw(media.fileBuffer, targetPath); return urlUtils.toTransformReady(filePath); diff --git a/ghost/core/core/server/services/oembed/oembed-service.js b/ghost/core/core/server/services/oembed/oembed-service.js index 8c622a4119a..a07012b6a3c 100644 --- a/ghost/core/core/server/services/oembed/oembed-service.js +++ b/ghost/core/core/server/services/oembed/oembed-service.js @@ -164,9 +164,11 @@ class OEmbedService { name = store.getSanitizedFileName(path.basename(fileName)); } - let targetDir = path.join(this.config.getContentPath('images'), imageType); - const uniqueFilePath = await store.generateUnique(targetDir, name, ext, 0); - const targetPath = path.join(imageType, path.basename(uniqueFilePath)); + // targetDir must be relative so the exists() probe and the later + // saveRaw() target the same storage key. Object-storage adapters + // (e.g. S3) use path.posix.join under the hood, which preserves + // absolute paths in the second arg and produces malformed keys. + const targetPath = await store.generateUnique(imageType, name, ext, 0); const imageStoredUrl = await store.saveRaw(imageBuffer, targetPath); @@ -235,7 +237,11 @@ class OEmbedService { contentType: headers['content-type'] }; } catch (err) { - logging.error(err); + logging.error({ + event: {name: 'oembed.decode-page.error'}, + err, + pageUrl: responseUrl + }, 'Failed to decode oembed page body'); //return non decoded body anyway return { body: body.toString(), @@ -325,7 +331,11 @@ class OEmbedService { }); } catch (err) { // Log to avoid being blind to errors happening in metascraper - logging.error(err); + logging.error({ + event: {name: 'oembed.metascraper.error'}, + err, + bookmarkUrl: url + }, 'Metascraper failed to extract bookmark metadata'); return this.unknownProvider(url); } @@ -346,26 +356,44 @@ class OEmbedService { if (type === 'mention') { if (metadata.icon) { + const originalIconUrl = metadata.icon; try { await this.externalRequest.head(metadata.icon); } catch (err) { metadata.icon = 'https://static.ghost.org/v5.0.0/images/link-icon.svg'; - logging.error(err); + logging.error({ + event: {name: 'oembed.fetch-mention-icon.error'}, + err, + bookmarkUrl: url, + iconUrl: originalIconUrl + }, 'Failed HEAD probe for mention icon'); } } } else { + const originalIconUrl = metadata.icon; await this.processImageFromUrl(metadata.icon, 'icon') .then((processedImageUrl) => { metadata.icon = processedImageUrl; }).catch((err) => { metadata.icon = 'https://static.ghost.org/v5.0.0/images/link-icon.svg'; - logging.error(err); + logging.error({ + event: {name: 'oembed.fetch-bookmark-icon.error'}, + err, + bookmarkUrl: url, + iconUrl: originalIconUrl + }, 'Failed to fetch and store bookmark icon'); }); + const originalThumbnailUrl = metadata.thumbnail; await this.processImageFromUrl(metadata.thumbnail, 'thumbnail') .then((processedImageUrl) => { metadata.thumbnail = processedImageUrl; }).catch((err) => { - logging.error(err); + logging.error({ + event: {name: 'oembed.fetch-bookmark-thumbnail.error'}, + err, + bookmarkUrl: url, + thumbnailUrl: originalThumbnailUrl + }, 'Failed to fetch and store bookmark thumbnail'); }); } @@ -571,10 +599,11 @@ class OEmbedService { } // log the real error because we're going to throw a generic "Unknown provider" error - logging.error(new errors.InternalServerError({ - message: 'Encountered error when fetching oembed', - err - })); + logging.error({ + event: {name: 'oembed.fetch-oembed.error'}, + err, + oembedUrl: url + }, 'Encountered error when fetching oembed'); // default to unknown provider to avoid leaking any app specifics return this.unknownProvider(url); diff --git a/ghost/core/test/unit/frontend/web/middleware/handle-image-sizes.test.js b/ghost/core/test/unit/frontend/web/middleware/handle-image-sizes.test.js index 282c61039ae..955b4cd9c52 100644 --- a/ghost/core/test/unit/frontend/web/middleware/handle-image-sizes.test.js +++ b/ghost/core/test/unit/frontend/web/middleware/handle-image-sizes.test.js @@ -205,10 +205,10 @@ describe('handleImageSizes middleware', function () { it('returns original URL if file is empty', function (done) { dummyStorage.exists = async function (path) { - if (path === '/blank_o.png') { + if (path === 'blank_o.png') { return true; } - if (path === '/size/w1000/blank.png') { + if (path === 'size/w1000/blank.png') { return false; } }; @@ -337,14 +337,14 @@ describe('handleImageSizes middleware', function () { it('continues if file exists', function (done) { dummyStorage.exists = async function (path) { - if (path === '/size/w1000/blank.png') { + if (path === 'size/w1000/blank.png') { return true; } }; const fakeReq = { url: '/size/w1000/blank.png', - originalUrl: '/size/w1000/blank.png' + originalUrl: 'size/w1000/blank.png' }; const fakeRes = { redirect() { @@ -363,7 +363,7 @@ describe('handleImageSizes middleware', function () { it('uses unoptimizedImageExists if it exists', function (done) { dummyStorage.exists = async function (path) { - if (path === '/blank_o.png') { + if (path === 'blank_o.png') { return true; } }; @@ -385,7 +385,7 @@ describe('handleImageSizes middleware', function () { return done(err); } try { - sinon.assert.calledOnceWithExactly(spy, {path: '/blank_o.png'}); + sinon.assert.calledOnceWithExactly(spy, {path: 'blank_o.png'}); } catch (e) { return done(e); } @@ -395,7 +395,7 @@ describe('handleImageSizes middleware', function () { it('uses unoptimizedImageExists if it exists with formatting', function (done) { dummyStorage.exists = async function (path) { - if (path === '/blank_o.png') { + if (path === 'blank_o.png') { return true; } }; @@ -419,7 +419,7 @@ describe('handleImageSizes middleware', function () { return done(err); } try { - sinon.assert.calledOnceWithExactly(spy, {path: '/blank_o.png'}); + sinon.assert.calledOnceWithExactly(spy, {path: 'blank_o.png'}); sinon.assert.calledOnceWithExactly(typeStub, 'webp'); } catch (e) { return done(e); 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 index c38e543dc02..4b2cb540c06 100644 --- 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 @@ -109,7 +109,7 @@ describe('Local Storage Base', function () { await assert.rejects( localStorageBase.read({path: '../../outside-root.txt'}), - {message: 'The path "../../outside-root.txt" is not valid for this storage.'} + /must not escape the storage root/ ); }); @@ -135,7 +135,7 @@ describe('Local Storage Base', function () { await assert.rejects( localStorageBase.read({path: 'foo/..'}), - {message: 'The path "foo/.." is not valid for this storage.'} + /must not escape the storage root/ ); }); @@ -159,7 +159,7 @@ describe('Local Storage Base', function () { await assert.rejects( localStorageBase.delete(''), - {message: 'The path "" is not valid for this storage.'} + /requires a non-empty file path/ ); }); @@ -211,7 +211,7 @@ describe('Local Storage Base', function () { await assert.rejects( localStorageBase.delete('file.txt', '../../etc'), - {message: 'The path "file.txt" is not valid for this storage.'} + /must not escape the storage root/ ); }); }); diff --git a/ghost/core/test/unit/server/adapters/storage/local-images-storage.test.js b/ghost/core/test/unit/server/adapters/storage/local-images-storage.test.js index 44ad79fa732..f3b8d6b9112 100644 --- a/ghost/core/test/unit/server/adapters/storage/local-images-storage.test.js +++ b/ghost/core/test/unit/server/adapters/storage/local-images-storage.test.js @@ -157,12 +157,14 @@ describe('Local Images Storage', function () { }); }); - it('success (leading and trailing slashes)', function (done) { - localFileStore.read({path: '/ghost-logo.png/'}) - .then(function (bytes) { - assert.equal(bytes.length, 8638); - done(); - }); + it('rejects leading and trailing slashes', async function () { + // The strict adapter contract requires canonical relative paths; + // callers that historically passed slashy URL-shaped paths must + // strip them at the call site. + await assert.rejects( + localFileStore.read({path: '/ghost-logo.png/'}), + /must be relative to the storage root/ + ); }); it('image does not exist', function (done) { @@ -222,35 +224,21 @@ describe('Local Images Storage', function () { }); }); - // @TODO: remove path.join mock... describe('on Windows', function () { const truePathSep = path.sep; - let pathJoinStub; - - beforeEach(function () { - pathJoinStub = sinon.stub(path, 'join'); - sinon.stub(configUtils.config, 'getContentPath').returns('content/images/'); - }); afterEach(function () { path.sep = truePathSep; }); - it('should return url in proper format for windows', function (done) { + it('returns the URL with forward slashes regardless of platform separator', function (done) { + // The production code applies a `path.sep → /` replace at the end + // of save() so URLs never carry backslashes even when running on + // Windows. We verify by flipping path.sep mid-flight and checking + // the returned URL is still POSIX-shaped. path.sep = '\\'; - pathJoinStub.returns('content\\images\\2013\\09\\IMAGE.jpg'); - localFileStore.save(image).then(function (url) { - if (truePathSep === '\\') { - assert.equal(url, '/content/images/2013/09/IMAGE.jpg'); - } else { - // if this unit test is run on an OS that uses forward slash separators, - // localfilesystem.save() will use a path.relative() call on - // one path with backslash separators and one path with forward - // slashes and it returns a path that needs to be normalized - assert.equal(path.normalize(url), '/content/images/2013/09/IMAGE.jpg'); - } - + assert.ok(!url.includes('\\'), `URL should contain no backslashes, got "${url}"`); done(); }).catch(done); }); diff --git a/ghost/core/test/unit/server/adapters/storage/s3-storage.test.ts b/ghost/core/test/unit/server/adapters/storage/s3-storage.test.ts index 50711405c6d..667e439fd01 100644 --- a/ghost/core/test/unit/server/adapters/storage/s3-storage.test.ts +++ b/ghost/core/test/unit/server/adapters/storage/s3-storage.test.ts @@ -320,9 +320,10 @@ describe('S3Storage', function () { const {storage, sendStub} = createStorage(); const {HeadObjectCommand: HeadObjectCmd} = await import('@aws-sdk/client-s3'); - // handle-image-sizes middleware calls exists(req.url) with a single path argument + // handle-image-sizes middleware calls exists with a single canonical + // (relative) path argument sendStub.resolves({}); - const exists = await storage.exists('/size/w1200/2024/06/photo.jpg'); + const exists = await storage.exists('size/w1200/2024/06/photo.jpg'); assert.equal(exists, true, 'should resolve to true when object exists'); const command = sendStub.firstCall.args[0] as InstanceType; @@ -334,7 +335,7 @@ describe('S3Storage', function () { sendStub.resetHistory(); sendStub.rejects(createNotFoundError()); - const missing = await storage.exists('/size/w1200/2024/06/missing.jpg'); + const missing = await storage.exists('size/w1200/2024/06/missing.jpg'); assert.equal(missing, false, 'should resolve to false when object does not exist'); }); @@ -353,7 +354,7 @@ describe('S3Storage', function () { sendStub.rejects(createNotFoundError()); - await storage.delete('ghost.txt', 'content/files'); + await storage.delete('ghost.txt', '2024/06'); sinon.assert.calledOnce(sendStub); }); @@ -369,17 +370,16 @@ describe('S3Storage', function () { await assert.rejects( storage.saveRaw(Buffer.from('data'), ''), - /requires a non-empty targetPath/ + /requires a non-empty file path/ ); }); - it('exists throws when fileName is empty', async function () { + it('exists returns false when fileName is empty', async function () { + // exists() is a query — malformed inputs are treated as "doesn't + // exist" rather than thrown back at the caller. const {storage} = createStorage(); - await assert.rejects( - storage.exists('', '2024/06'), - /requires a non-empty fileName/ - ); + assert.equal(await storage.exists('', '2024/06'), false); }); it('delete throws when fileName is empty', async function () { @@ -387,7 +387,7 @@ describe('S3Storage', function () { await assert.rejects( storage.delete('', '2024/06'), - /requires a non-empty fileName/ + /requires a non-empty file path/ ); }); diff --git a/ghost/core/test/unit/server/adapters/storage/utils.test.js b/ghost/core/test/unit/server/adapters/storage/utils.test.js index f79257ad3e2..fa9ad2dd78a 100644 --- a/ghost/core/test/unit/server/adapters/storage/utils.test.js +++ b/ghost/core/test/unit/server/adapters/storage/utils.test.js @@ -31,7 +31,7 @@ describe('storage utils', function () { result = storageUtils.getLocalImagesStoragePath(url); assertExists(result); - assert.equal(result, '/2017/07/ghost-logo.png'); + assert.equal(result, '2017/07/ghost-logo.png'); }); it('should return local file storage path for absolute URL with subdirectory', function () { @@ -45,7 +45,7 @@ describe('storage utils', function () { result = storageUtils.getLocalImagesStoragePath(url); assertExists(result); - assert.equal(result, '/2017/07/ghost-logo.png'); + assert.equal(result, '2017/07/ghost-logo.png'); }); it('should return local file storage path for relative URL', function () { @@ -59,7 +59,7 @@ describe('storage utils', function () { result = storageUtils.getLocalImagesStoragePath(filePath); assertExists(result); - assert.equal(result, '/2017/07/ghost-logo.png'); + assert.equal(result, '2017/07/ghost-logo.png'); }); it('should return local file storage path for relative URL with subdirectory', function () { @@ -73,7 +73,7 @@ describe('storage utils', function () { result = storageUtils.getLocalImagesStoragePath(filePath); assertExists(result); - assert.equal(result, '/2017/07/ghost-logo.png'); + assert.equal(result, '2017/07/ghost-logo.png'); }); it('should not sanitize URL if not local file storage', function () { diff --git a/ghost/core/test/unit/server/data/importer/handlers/image.test.js b/ghost/core/test/unit/server/data/importer/handlers/image.test.js index be1cb298fb3..33a658b1c8d 100644 --- a/ghost/core/test/unit/server/data/importer/handlers/image.test.js +++ b/ghost/core/test/unit/server/data/importer/handlers/image.test.js @@ -53,7 +53,7 @@ describe('ImageHandler', function () { sinon.assert.calledOnce(storageSpy); sinon.assert.calledOnce(storeSpy); assert.equal(storeSpy.firstCall.args[0].originalPath, 'test-image.jpeg'); - assert.match(storeSpy.firstCall.args[0].targetDir, /(\/|\\)content(\/|\\)images$/); + assert.equal(storeSpy.firstCall.args[0].targetDir, ''); assert.equal(storeSpy.firstCall.args[0].newPath, '/content/images/test-image.jpeg'); }); @@ -72,7 +72,7 @@ describe('ImageHandler', function () { sinon.assert.calledOnce(storageSpy); sinon.assert.calledOnce(storeSpy); assert.equal(storeSpy.firstCall.args[0].originalPath, 'photos/my-cat.jpeg'); - assert.match(storeSpy.firstCall.args[0].targetDir, /(\/|\\)content(\/|\\)images(\/|\\)photos$/); + assert.equal(storeSpy.firstCall.args[0].targetDir, 'photos'); assert.equal(storeSpy.firstCall.args[0].newPath, '/content/images/photos/my-cat.jpeg'); }); @@ -91,7 +91,7 @@ describe('ImageHandler', function () { sinon.assert.calledOnce(storageSpy); sinon.assert.calledOnce(storeSpy); assert.equal(storeSpy.firstCall.args[0].originalPath, 'content/images/my-cat.jpeg'); - assert.match(storeSpy.firstCall.args[0].targetDir, /(\/|\\)content(\/|\\)images$/); + assert.equal(storeSpy.firstCall.args[0].targetDir, ''); assert.equal(storeSpy.firstCall.args[0].newPath, '/content/images/my-cat.jpeg'); }); @@ -112,7 +112,9 @@ describe('ImageHandler', function () { sinon.assert.calledOnce(storageSpy); sinon.assert.calledOnce(storeSpy); assert.equal(storeSpy.firstCall.args[0].originalPath, 'test-image.jpeg'); - assert.match(storeSpy.firstCall.args[0].targetDir, /(\/|\\)content(\/|\\)images$/); + // Files at the root of the import use the empty string to denote the + // storage root (the strict adapter contract requires relative paths). + assert.equal(storeSpy.firstCall.args[0].targetDir, ''); assert.equal(storeSpy.firstCall.args[0].newPath, '/subdir/content/images/test-image.jpeg'); }); @@ -141,16 +143,16 @@ describe('ImageHandler', function () { sinon.assert.calledOnce(storageSpy); sinon.assert.callCount(storeSpy, 4); assert.equal(storeSpy.firstCall.args[0].originalPath, 'testing.png'); - assert.match(storeSpy.firstCall.args[0].targetDir, /(\/|\\)content(\/|\\)images$/); + assert.equal(storeSpy.firstCall.args[0].targetDir, ''); assert.equal(storeSpy.firstCall.args[0].newPath, '/content/images/testing.png'); assert.equal(storeSpy.secondCall.args[0].originalPath, 'photo/kitten.jpg'); - assert.match(storeSpy.secondCall.args[0].targetDir, /(\/|\\)content(\/|\\)images(\/|\\)photo$/); + assert.equal(storeSpy.secondCall.args[0].targetDir, 'photo'); assert.equal(storeSpy.secondCall.args[0].newPath, '/content/images/photo/kitten.jpg'); assert.equal(storeSpy.thirdCall.args[0].originalPath, 'content/images/animated/bunny.gif'); - assert.match(storeSpy.thirdCall.args[0].targetDir, /(\/|\\)content(\/|\\)images(\/|\\)animated$/); + assert.equal(storeSpy.thirdCall.args[0].targetDir, 'animated'); assert.equal(storeSpy.thirdCall.args[0].newPath, '/content/images/animated/bunny.gif'); assert.equal(storeSpy.lastCall.args[0].originalPath, 'images/puppy.jpg'); - assert.match(storeSpy.lastCall.args[0].targetDir, /(\/|\\)content(\/|\\)images$/); + assert.equal(storeSpy.lastCall.args[0].targetDir, ''); assert.equal(storeSpy.lastCall.args[0].newPath, '/content/images/puppy.jpg'); }); }); 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 index 80e6287995e..59193566e45 100644 --- a/ghost/core/test/unit/server/lib/image/image-size.test.js +++ b/ghost/core/test/unit/server/lib/image/image-size.test.js @@ -664,7 +664,7 @@ describe('lib/image: image size', function () { await assert.rejects( imageSize.getImageSizeFromStoragePath(url), - {message: 'The path "/../../../../../outside-root.png" is not valid for this storage.'} + /must (be relative to the storage root|not escape the storage root)/ ); }); diff --git a/ghost/core/test/unit/server/services/oembed/oembed-service.test.js b/ghost/core/test/unit/server/services/oembed/oembed-service.test.js index 6e704a6be62..a126e9f2325 100644 --- a/ghost/core/test/unit/server/services/oembed/oembed-service.test.js +++ b/ghost/core/test/unit/server/services/oembed/oembed-service.test.js @@ -1,4 +1,5 @@ const assert = require('node:assert/strict'); +const path = require('node:path'); const nock = require('nock'); const got = require('got').default; const sinon = require('sinon'); @@ -264,7 +265,7 @@ describe('oembed-service', function () { describe('processImageFromUrl', function () { it('stores downloaded bookmark assets via image storage and returns stored URL', async function () { const saveRaw = sinon.stub().resolves('https://storage.ghost.is/c/6f/a3/site/content/images/thumbnail/sample.png'); - const generateUnique = sinon.stub().resolves('/tmp/content/images/thumbnail/sample.png'); + const generateUnique = sinon.stub().resolves('thumbnail/sample.png'); const getSanitizedFileName = sinon.stub().returns('sample'); const service = new OembedService({ @@ -294,13 +295,14 @@ describe('oembed-service', function () { assert.equal(storedUrl, 'https://storage.ghost.is/c/6f/a3/site/content/images/thumbnail/sample.png'); sinon.assert.calledOnce(getSanitizedFileName); sinon.assert.calledOnce(generateUnique); + assert.equal(generateUnique.firstCall.args[0], 'thumbnail'); sinon.assert.calledOnce(saveRaw); assert.equal(saveRaw.firstCall.args[1], 'thumbnail/sample.png'); }); it('works when saveRaw returns a relative path (local storage)', async function () { const saveRaw = sinon.stub().resolves('/content/images/icon/favicon.ico'); - const generateUnique = sinon.stub().resolves('/tmp/content/images/icon/favicon.ico'); + const generateUnique = sinon.stub().resolves('icon/favicon.ico'); const getSanitizedFileName = sinon.stub().returns('favicon'); const service = new OembedService({ @@ -328,9 +330,60 @@ describe('oembed-service', function () { const storedUrl = await service.processImageFromUrl('https://example.com/favicon.ico', 'icon'); assert.equal(storedUrl, '/content/images/icon/favicon.ico'); + assert.equal(generateUnique.firstCall.args[0], 'icon'); assert.equal(saveRaw.firstCall.args[1], 'icon/favicon.ico'); }); + it('passes a relative targetDir to generateUnique so the exists() probe and saveRaw target the same key', async function () { + // Object-storage adapters (e.g. S3) build keys with path.posix.join, + // which preserves an absolute second argument and produces a key + // that includes the local filesystem prefix. Asserting that + // generateUnique receives a relative dir keeps the exists() probe + // and the later saveRaw call aligned on the same key. + const observedCalls = []; + const fakeStore = { + getSanitizedFileName: name => name, + generateUnique: async (dir, name, ext) => { + observedCalls.push({op: 'generateUnique', dir, name, ext}); + return `${dir}/${name}${ext}`; + }, + saveRaw: async (_buffer, targetPath) => { + observedCalls.push({op: 'saveRaw', targetPath}); + return `https://cdn.example.com/${targetPath}`; + } + }; + + const service = new OembedService({ + config: { + // getContentPath returns an absolute filesystem path; the + // service must not let it leak into the storage targetDir. + getContentPath: () => '/var/lib/ghost/content/images/' + }, + storage: { + getStorage: () => fakeStore + }, + externalRequest: () => ({ + buffer: async () => Buffer.from('icon-bytes') + }) + }); + + await service.processImageFromUrl('https://www.theguardian.com/favicon.ico', 'icon'); + + assert.deepEqual(observedCalls, [ + {op: 'generateUnique', dir: 'icon', name: 'favicon', ext: '.ico'}, + {op: 'saveRaw', targetPath: 'icon/favicon.ico'} + ]); + + // Belt-and-braces: explicitly assert nothing absolute leaked anywhere. + for (const call of observedCalls) { + const candidate = call.dir ?? call.targetPath; + assert.ok( + !path.isAbsolute(candidate), + `${call.op} received absolute path "${candidate}"; storage adapters must receive relative paths` + ); + } + }); + it('throws when storage lacks saveRaw', async function () { const service = new OembedService({ config: {