Skip to content

🐛 Fixed bookmark favicon storage on object-storage adapters #27670

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
rob-ghost wants to merge 2 commits into from
Closed

🐛 Fixed bookmark favicon storage on object-storage adapters #27670

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c81e280
🐛 Fixed bookmark favicon storage by passing relative dir to adapter
rob-ghost May 5, 2026
5768626
🐛 Hardened storage adapter contract with canonical-path validation
rob-ghost May 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions ghost/core/core/frontend/web/middleware/handle-image-sizes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down
141 changes: 60 additions & 81 deletions ghost/core/core/server/adapters/storage/LocalStorageBase.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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);
}

/**
Expand All @@ -117,19 +75,21 @@ class LocalStorageBase extends StorageBase {
* @returns {Promise<String>}
*/
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});
Expand All @@ -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;
Expand All @@ -157,7 +117,9 @@ class LocalStorageBase extends StorageBase {
* @returns {Promise<String>} 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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}

Expand All @@ -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);
Expand Down
48 changes: 34 additions & 14 deletions ghost/core/core/server/adapters/storage/S3Storage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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<string> {
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);
Expand Down Expand Up @@ -261,11 +277,7 @@ export default class S3Storage extends StorageBase {
}

async saveRaw(buffer: Buffer, targetPath: string): Promise<string> {
if (!targetPath?.trim()) {
throw new errors.IncorrectUsageError({
message: tpl(messages.emptyTargetPath)
});
}
this._assertFilePath(targetPath);

const key = this.buildKey(targetPath);

Expand Down Expand Up @@ -320,10 +332,19 @@ export default class S3Storage extends StorageBase {
}

async exists(fileName: string, targetDir?: string): Promise<boolean> {
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;
Expand Down Expand Up @@ -358,10 +379,9 @@ export default class S3Storage extends StorageBase {
}

async delete(fileName: string, targetDir?: string): Promise<void> {
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;
Expand Down
Loading
Loading