From 9f029fd0e58546d1380e32f2f89afe655ed7fc67 Mon Sep 17 00:00:00 2001 From: Fabien O'Carroll Date: Tue, 16 Jun 2026 11:36:57 +0000 Subject: [PATCH] Implemented read() in the S3 storage adapter fixes https://linear.app/tryghost/issue/INC-300 - read() was left unimplemented when the adapter was added (9f0c353), as files and media flows never needed it - image dimension lookups (ghost_head social meta) do call storage.read() for local-style /content/images/ URLs; with S3 configured as the images adapter this threw a critical IncorrectUsageError, flooding logs on every page render and dropping image dimensions - implemented read() as a native S3 GetObject so the existing image-size flow works unchanged; results are already cached by CachedImageSizeFromUrl, so it's one fetch per image, not per render - resizing (handle-image-sizes) still doesn't need read under S3: those requests terminate at the CDN and never reach Ghost Co-Authored-By: Claude Opus 4.8 --- .../core/server/adapters/storage/S3Storage.ts | 42 +++++-- .../adapters/storage/s3-storage.test.ts | 112 +++++++++++++++++ .../integration/importer/image-urls.test.js | 114 ++++++++++++++++++ .../lib/image/image-size-storage.test.ts | 110 +++++++++++++++++ .../adapters/storage/s3-storage.test.ts | 34 +++++- 5 files changed, 403 insertions(+), 9 deletions(-) create mode 100644 ghost/core/test/integration/adapters/storage/s3-storage.test.ts create mode 100644 ghost/core/test/integration/importer/image-urls.test.js create mode 100644 ghost/core/test/integration/lib/image/image-size-storage.test.ts diff --git a/ghost/core/core/server/adapters/storage/S3Storage.ts b/ghost/core/core/server/adapters/storage/S3Storage.ts index aec0fe146c0..3af3e820028 100644 --- a/ghost/core/core/server/adapters/storage/S3Storage.ts +++ b/ghost/core/core/server/adapters/storage/S3Storage.ts @@ -7,6 +7,7 @@ import errors from '@tryghost/errors'; import logging from '@tryghost/logging'; import { DeleteObjectCommand, + GetObjectCommand, HeadObjectCommand, NotFound, NoSuchKey, @@ -33,7 +34,8 @@ const messages = { emptyTargetPath: 'S3Storage.saveRaw requires a non-empty targetPath', emptyFileName: 'S3Storage.{method} requires a non-empty fileName', emptyRelativePath: 'S3Storage.buildKey requires a non-empty relativePath', - readNotSupported: 'read() is not supported by S3Storage. S3Storage is designed for media and files, not images. Use LocalImagesStorage for image storage.', + emptyReadPath: 'S3Storage.read requires a non-empty path', + readNotFound: 'Could not read file: {path}', multipartUploadInitFailed: 'Failed to initiate file upload.', multipartUploadPartFailed: 'Failed to upload file part {partNumber}.', multipartUploadReadFailed: 'There was an error uploading the file. The file may have been modified or removed during upload.', @@ -380,12 +382,38 @@ export default class S3Storage extends StorageBase { } /** - * Not supported - S3Storage is for media/files only. Images use LocalImagesStorage. + * Reads an object's bytes from S3. Used by image dimension lookups, which + * fall back to reading from storage for images served via the CDN. */ - async read(): Promise { - throw new errors.IncorrectUsageError({ - message: tpl(messages.readNotSupported) - }); + async read(options: {path?: string} = {}): Promise { + const relativePath = options.path; + + if (!relativePath?.trim()) { + throw new errors.IncorrectUsageError({ + message: tpl(messages.emptyReadPath) + }); + } + + const key = this.buildKey(relativePath); + + try { + const response = await this.client.send(new GetObjectCommand({ + Bucket: this.bucket, + Key: key + })); + + const bytes = await response.Body?.transformToByteArray(); + return Buffer.from(bytes ?? []); + } catch (error) { + if (this.isNotFound(error)) { + throw new errors.NotFoundError({ + err: error, + message: tpl(messages.readNotFound, {path: relativePath}) + }); + } + + throw error; + } } private buildKey(relativePath: string): string { @@ -446,7 +474,7 @@ export default class S3Storage extends StorageBase { return input.replace(/^\/+/, ''); } - private isNotFound(error: unknown): boolean { + private isNotFound(error: unknown): error is NotFound | NoSuchKey { return error instanceof NotFound || error instanceof NoSuchKey; } } diff --git a/ghost/core/test/integration/adapters/storage/s3-storage.test.ts b/ghost/core/test/integration/adapters/storage/s3-storage.test.ts new file mode 100644 index 00000000000..ed1216c1b95 --- /dev/null +++ b/ghost/core/test/integration/adapters/storage/s3-storage.test.ts @@ -0,0 +1,112 @@ +/* eslint-disable ghost/mocha/no-top-level-hooks -- false positive: the hooks are inside the describe, but the lint plugin can't see through the describe.skipIf()() gate below. (PLA-170) */ +import {describe, it, beforeAll, afterEach, afterAll} from 'vitest'; +import assert from 'node:assert/strict'; + +import S3Storage from '../../../../core/server/adapters/storage/S3Storage'; +import { + createTestS3Client, + createTestBucket, + emptyTestBucket, + deleteTestBucket, + getMinioConfig, + putObject +} from '../../../utils/minio'; + +const STATIC_PREFIX = 'content/images'; +const minioConfig = getMinioConfig(); + +// read() builds the object key as [tenantPrefix/]storagePath/relativePath, so a +// test fixture must be written at that exact key for read() to find it. +const objectKey = (relativePath: string, tenantPrefix = '') => [tenantPrefix, STATIC_PREFIX, relativePath].filter(Boolean).join('/'); + +// Skip when MinIO is unreachable. The flag is set by the integration +// globalSetup (vitest-globalsetup-services.ts), which probes MinIO once before +// the forks spawn. (PLA-170) +describe.skipIf(process.env.GHOST_TEST_MINIO_AVAILABLE !== '1')('Integration: S3Storage.read', function () { + let adminClient: ReturnType; + let bucket: string; + + const createStorage = (overrides = {}) => new S3Storage({ + ...minioConfig, + bucket, + cdnUrl: `${minioConfig.endpoint}/${bucket}`, + staticFileURLPrefix: STATIC_PREFIX, + multipartUploadThresholdBytes: 10 * 1024 * 1024, + multipartChunkSizeBytes: 10 * 1024 * 1024, + ...overrides + }); + + beforeAll(async function () { + adminClient = createTestS3Client(); + bucket = await createTestBucket(adminClient, 'test-storage'); + }); + + afterEach(async function () { + await emptyTestBucket(adminClient, bucket); + }); + + afterAll(async function () { + await deleteTestBucket(adminClient, bucket); + }); + + it('reads back the raw bytes of an object written to storage', async function () { + // Include NUL and high bytes to prove read() is binary-safe (images). + const body = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x00, 0xff, 0x0a, 0x01]); + await putObject(adminClient, bucket, objectKey('2024/06/image.jpg'), body); + + const result = await createStorage().read({path: '2024/06/image.jpg'}); + + assert.deepEqual(result, body); + }); + + it('round-trips a file referenced by its absolute storage path', async function () { + // image dimension lookups pass the local-style /content/images/ path; + // read() must map that back to the same key save() would have written. + const body = Buffer.from('saved-via-adapter'); + await putObject(adminClient, bucket, objectKey('2024/06/saved.png'), body); + + const result = await createStorage().read({path: '/content/images/2024/06/saved.png'}); + + assert.deepEqual(result, body); + }); + + it('throws NotFoundError when the object is missing', async function () { + await assert.rejects( + () => createStorage().read({path: '2024/06/missing.jpg'}), + (err: Error) => { + assert.equal((err as {errorType?: string}).errorType, 'NotFoundError'); + assert.match(err.message, /Could not read file: 2024\/06\/missing\.jpg/); + return true; + } + ); + }); + + it('throws IncorrectUsageError when no path is given', async function () { + await assert.rejects( + () => createStorage().read(), + {errorType: 'IncorrectUsageError', message: /requires a non-empty path/} + ); + }); + + describe('tenantPrefix scoping', function () { + const TENANT = 'tenant-abc'; + + it('reads an object stored under the tenant prefix', async function () { + const body = Buffer.from('tenant-scoped-bytes'); + await putObject(adminClient, bucket, objectKey('2024/06/image.jpg', TENANT), body); + + const result = await createStorage({tenantPrefix: TENANT}).read({path: '2024/06/image.jpg'}); + + assert.deepEqual(result, body); + }); + + it('does not read another tenant\'s object', async function () { + await putObject(adminClient, bucket, objectKey('2024/06/image.jpg', 'tenant-other'), Buffer.from('other')); + + await assert.rejects( + () => createStorage({tenantPrefix: TENANT}).read({path: '2024/06/image.jpg'}), + {errorType: 'NotFoundError'} + ); + }); + }); +}); diff --git a/ghost/core/test/integration/importer/image-urls.test.js b/ghost/core/test/integration/importer/image-urls.test.js new file mode 100644 index 00000000000..329f3e0f953 --- /dev/null +++ b/ghost/core/test/integration/importer/image-urls.test.js @@ -0,0 +1,114 @@ +const assert = require('node:assert/strict'); +const importer = require('../../../core/server/data/importer'); +const db = require('../../../core/server/data/db'); +const urlUtils = require('../../../core/shared/url-utils'); +const testUtils = require('../../utils'); +const {exportedBodyV5} = require('../../utils/fixtures/export/body-generator'); + +const dataImporter = importer.importers.find((instance) => { + return instance.type === 'data'; +}); + +const importOptions = { + returnImportedData: true +}; + +const LEGACY_HARDCODED_USER_ID = '1'; + +// These assertions read the RAW database columns rather than going through the +// model, because the Post / PostsMeta models re-expand __GHOST_URL__ back to an +// absolute URL on read (parse()). The thing we care about — that the importer +// stores image URLs in transform-ready (__GHOST_URL__) form — is only visible in +// the raw stored value. +describe('Importing posts with image URLs', function () { + let siteUrl; + + beforeEach(async function () { + await testUtils.teardownDb(); + await testUtils.setup('roles', 'owner')(); + siteUrl = urlUtils.urlFor('home', true).replace(/\/$/, ''); + }); + + async function importPosts(posts) { + const exportData = exportedBodyV5().db[0]; + + exportData.data.users[0] = testUtils.DataGenerator.forKnex.createUser({ + id: LEGACY_HARDCODED_USER_ID, + email: 'import-test-user@ghost.org', + slug: 'import-test-user' + }); + exportData.data.roles = [ + testUtils.DataGenerator.forKnex.createRole({name: 'Owner'}) + ]; + exportData.data.roles_users = [ + testUtils.DataGenerator.forKnex.createUsersRoles(exportData.data.users[0].id, exportData.data.roles[0].id) + ]; + + exportData.data.posts = posts.map(overrides => testUtils.DataGenerator.forKnex.createPost(Object.assign({ + authors: [{id: LEGACY_HARDCODED_USER_ID}], + published_by: LEGACY_HARDCODED_USER_ID + }, overrides))); + + await dataImporter.doImport(exportData, importOptions); + } + + const rawPost = slug => db.knex('posts').where('slug', slug).first(); + const rawMeta = postId => db.knex('posts_meta').where('post_id', postId).first(); + + it('stores a site-relative feature_image as transform-ready (__GHOST_URL__)', async function () { + // The exact shape of Tangle's migrated Cloudinary URLs (INC-300). + await importPosts([{ + slug: 'relative-feature-image', + feature_image: '/content/images/image/upload/w_728-c_limit/znxcdabd5z1cxrxclyug.jpeg' + }]); + + const post = await rawPost('relative-feature-image'); + assert.equal( + post.feature_image, + '__GHOST_URL__/content/images/image/upload/w_728-c_limit/znxcdabd5z1cxrxclyug.jpeg' + ); + }); + + it('stores an absolute site feature_image as transform-ready (__GHOST_URL__)', async function () { + await importPosts([{ + slug: 'absolute-feature-image', + feature_image: `${siteUrl}/content/images/2024/06/photo.png` + }]); + + const post = await rawPost('absolute-feature-image'); + assert.equal(post.feature_image, '__GHOST_URL__/content/images/2024/06/photo.png'); + }); + + it('stores site-relative og_image and twitter_image as transform-ready (__GHOST_URL__)', async function () { + await importPosts([{ + slug: 'relative-meta-images', + og_image: '/content/images/2024/06/og.png', + twitter_image: '/content/images/2024/06/twitter.png' + }]); + + const post = await rawPost('relative-meta-images'); + const meta = await rawMeta(post.id); + assert.equal(meta.og_image, '__GHOST_URL__/content/images/2024/06/og.png'); + assert.equal(meta.twitter_image, '__GHOST_URL__/content/images/2024/06/twitter.png'); + }); + + it('leaves an already transform-ready feature_image untouched', async function () { + await importPosts([{ + slug: 'transform-ready-feature-image', + feature_image: '__GHOST_URL__/content/images/2024/06/already.png' + }]); + + const post = await rawPost('transform-ready-feature-image'); + assert.equal(post.feature_image, '__GHOST_URL__/content/images/2024/06/already.png'); + }); + + it('leaves an external feature_image untouched', async function () { + await importPosts([{ + slug: 'external-feature-image', + feature_image: 'https://images.example.com/external/photo.png' + }]); + + const post = await rawPost('external-feature-image'); + assert.equal(post.feature_image, 'https://images.example.com/external/photo.png'); + }); +}); diff --git a/ghost/core/test/integration/lib/image/image-size-storage.test.ts b/ghost/core/test/integration/lib/image/image-size-storage.test.ts new file mode 100644 index 00000000000..bc69c56dfea --- /dev/null +++ b/ghost/core/test/integration/lib/image/image-size-storage.test.ts @@ -0,0 +1,110 @@ +/* eslint-disable ghost/mocha/no-top-level-hooks -- false positive: hooks are inside the describe, hidden behind the describe.skipIf() gate. */ +import {describe, it, beforeAll, afterEach, afterAll} from 'vitest'; +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import path from 'node:path'; + +import S3Storage from '../../../../core/server/adapters/storage/S3Storage'; +import { + createTestS3Client, + createTestBucket, + emptyTestBucket, + deleteTestBucket, + getMinioConfig, + putObject +} from '../../../utils/minio'; + +// CJS Ghost libs — required (not imported) to preserve `this` binding on the +// storageUtils namespace (isLocalImage calls this.getLocalImagesStoragePath). +const ImageSize = require('../../../../core/server/lib/image/image-size'); +const storageUtils = require('../../../../core/server/adapters/storage/utils'); +const urlUtils = require('../../../../core/shared/url-utils'); +const config = require('../../../../core/shared/config'); +const validator = require('@tryghost/validator'); +const request = require('@tryghost/request'); +const {probe} = require('../../../../core/server/lib/image/image-utils'); + +const STATIC_PREFIX = 'content/images'; +const minioConfig = getMinioConfig(); + +// 100x100 png fixture — the same one Ghost ships for favicon tests. +const FIXTURE = fs.readFileSync(path.join(process.cwd(), 'test/utils/fixtures/images/favicon.png')); +const FIXTURE_WIDTH = 100; +const FIXTURE_HEIGHT = 100; + +// This is the INC-300 flow: a meta image (cover/og/twitter) that resolves to a +// site-relative /content/images/... URL is classified "local", so the dimension +// lookup reads it from the images storage adapter. With S3 configured as that +// adapter, S3Storage.read() must return the bytes so dimensions resolve instead +// of throwing. +describe.skipIf(process.env.GHOST_TEST_MINIO_AVAILABLE !== '1')('Integration: image dimensions via S3 storage', function () { + let adminClient: ReturnType; + let bucket: string; + + const createImageSize = (overrides = {}) => { + const s3 = new S3Storage({ + ...minioConfig, + bucket, + cdnUrl: `${minioConfig.endpoint}/${bucket}`, + staticFileURLPrefix: STATIC_PREFIX, + multipartUploadThresholdBytes: 10 * 1024 * 1024, + multipartChunkSizeBytes: 10 * 1024 * 1024, + ...overrides + }); + // ImageSize asks the storage manager for the 'images' adapter. + const storage = {getStorage: () => s3}; + return new ImageSize({config, storage, storageUtils, validator, urlUtils, request, probe}); + }; + + beforeAll(async function () { + adminClient = createTestS3Client(); + bucket = await createTestBucket(adminClient, 'test-imagesize'); + }); + + afterEach(async function () { + await emptyTestBucket(adminClient, bucket); + }); + + afterAll(async function () { + await deleteTestBucket(adminClient, bucket); + }); + + it('resolves dimensions for a relative /content/images URL backed by S3', async function () { + await putObject(adminClient, bucket, 'content/images/2024/06/cover.png', FIXTURE); + + const result = await createImageSize().getImageSizeFromUrl('/content/images/2024/06/cover.png'); + + assert.equal(result.width, FIXTURE_WIDTH); + assert.equal(result.height, FIXTURE_HEIGHT); + }); + + it('resolves dimensions for an absolute site /content/images URL backed by S3', async function () { + await putObject(adminClient, bucket, 'content/images/2024/06/cover.png', FIXTURE); + + const siteUrl = urlUtils.urlFor('home', true).replace(/\/$/, ''); + const result = await createImageSize().getImageSizeFromUrl(`${siteUrl}/content/images/2024/06/cover.png`); + + assert.equal(result.width, FIXTURE_WIDTH); + assert.equal(result.height, FIXTURE_HEIGHT); + }); + + it('resolves dimensions when the adapter uses a tenant prefix', async function () { + await putObject(adminClient, bucket, 'tenant-x/content/images/2024/06/cover.png', FIXTURE); + + const result = await createImageSize({tenantPrefix: 'tenant-x'}) + .getImageSizeFromUrl('/content/images/2024/06/cover.png'); + + assert.equal(result.width, FIXTURE_WIDTH); + assert.equal(result.height, FIXTURE_HEIGHT); + }); + + it('rejects with NotFoundError when the object is missing in S3', async function () { + await assert.rejects( + () => createImageSize().getImageSizeFromUrl('/content/images/2024/06/missing.png'), + (err: Error) => { + assert.equal((err as {errorType?: string}).errorType, 'NotFoundError'); + return true; + } + ); + }); +}); 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 25d386bac83..147cec8df08 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 @@ -485,12 +485,42 @@ describe('S3Storage', function () { }, /not a valid URL/); }); - it('read() throws as it is not supported', async function () { + it('read() returns the object bytes for a relative path', async function () { + const {storage, sendStub} = createStorage(); + const {GetObjectCommand: GetObjectCmd} = await import('@aws-sdk/client-s3'); + + sendStub.resolves({ + Body: { + transformToByteArray: async () => new Uint8Array(Buffer.from('image-bytes')) + } + }); + + const result = await storage.read({path: '2024/06/image.jpg'}); + + assert.deepEqual(result, Buffer.from('image-bytes')); + + const command = sendStub.firstCall.args[0] as InstanceType; + assert.equal(command.input.Bucket, 'test-bucket'); + assert.equal(command.input.Key, 'configurable/prefix/content/files/2024/06/image.jpg'); + }); + + it('read() throws NotFoundError when the object is missing', async function () { + const {storage, sendStub} = createStorage(); + + sendStub.rejects(createNotFoundError()); + + await assert.rejects( + storage.read({path: '2024/06/missing.jpg'}), + /Could not read file: 2024\/06\/missing.jpg/ + ); + }); + + it('read() throws when no path is given', async function () { const {storage} = createStorage(); await assert.rejects( storage.read(), - /read\(\) is not supported by S3Storage/ + /requires a non-empty path/ ); });