Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
38 changes: 37 additions & 1 deletion ghost/core/core/server/adapters/storage/S3Storage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,7 @@ export default class S3Storage extends StorageBase {
});
}

const pathWithStorage = path.posix.join(this.storagePath, relativePath);
const pathWithStorage = path.posix.join(this.storagePath, this.toCanonicalRelativePath(relativePath));

if (!pathWithStorage.startsWith(this.storagePath + '/') && pathWithStorage !== this.storagePath) {
throw new errors.IncorrectUsageError({
Expand All @@ -410,6 +410,42 @@ export default class S3Storage extends StorageBase {
return `${this.tenantPrefix}/${pathWithStorage}`;
}

private toCanonicalRelativePath(input: string): string {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could implement the path transformations inline here, because they all boil down to "get everything after the storage path" or "remove leading slash" - something like:

toCanonicalRelativePath(input) {
    const marker = this.storagePath + '/';                                                                                                                                               
    const parts = input.split(marker);       
    if (parts.length > 1) {                                                                                                                                                              
        return parts[parts.length - 1];
    }                                                                                                                                                                                    
    return input.replace(/^\/+/, '');
}

Or I guess the lastIndexOf approach works here too - I think it's effectively the same

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally had this implementation, but I found myself having to document the different cases and outcomes in comments so I wanted to see if I could encode the accepted inputs and their outputs into the code itself, hence the different function names etc.

Its more verbose in the code, but also more explicit and literal, so we don't have to rely on comments to explain what we accept or transform or for convention to remember what we accept or pass.

The other method of achieving the same goal is to not allow "invalid" input which would require this kind of manipulation, but that would require changing all the call sites, which I wanted to avoid (in this specific design).

Regarding the suggestion vs the current implementation, we could go with either, I think the outcome is the same. I'll hold off actioning anything right now, I'll reply to the outer suggestion about updating call-sites first.

return this.fromAbsoluteFilesystemPath(input)
?? this.fromStoragePathPrefixed(input)
?? this.fromLeadingSlashPath(input)
?? input;
}

private fromAbsoluteFilesystemPath(input: string): string | null {
if (!path.posix.isAbsolute(input)) {
return null;
}
const marker = `/${this.storagePath}/`;
const idx = input.lastIndexOf(marker);
if (idx !== -1) {
return input.slice(idx + marker.length);
}
if (input.endsWith(`/${this.storagePath}`)) {
return '';
}
return null;
}

private fromStoragePathPrefixed(input: string): string | null {
if (input === this.storagePath || input.startsWith(`${this.storagePath}/`)) {
return path.posix.relative(this.storagePath, input);
}
return null;
}

private fromLeadingSlashPath(input: string): string | null {
if (!path.posix.isAbsolute(input)) {
return null;
}
return input.replace(/^\/+/, '');
}
Comment thread
rob-ghost marked this conversation as resolved.

private isNotFound(error: unknown): boolean {
return error instanceof NotFound || error instanceof NoSuchKey;
}
Expand Down
96 changes: 96 additions & 0 deletions ghost/core/test/unit/server/adapters/storage/s3-storage.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,102 @@ describe('S3Storage', function () {
);
});

describe('absolute and pre-prefixed paths are normalised before building keys', function () {
// Some legacy callers build their target by joining `getContentPath(...)`
// with an extra segment, producing an absolute filesystem path. Without
// normalisation the absolute prefix gets concatenated into the bucket
// key. These tests pin the behaviour so the keys produced for an
// absolute or pre-prefixed input match the keys for the equivalent
// relative input, mirroring `LocalStorageBase`.

it('strips an absolute filesystem prefix that contains the storagePath segment', function () {
const {storage} = createStorage();

const fromAbsolute = (storage as any).buildKey('/var/lib/ghost/content/files/2024/06/image.jpg');
const fromRelative = (storage as any).buildKey('2024/06/image.jpg');

assert.equal(fromAbsolute, fromRelative);
assert.equal(fromAbsolute, 'configurable/prefix/content/files/2024/06/image.jpg');
});

it('strips a leading <storagePath>/ when callers pass it via `getTargetDir(storagePath)`', function () {
const {storage} = createStorage();

// External-media-inliner shape: targetDir = `storagePath/year/month`,
// joined with a filename and handed to exists()/save().
const fromPrefixed = (storage as any).buildKey('content/files/2024/06/image.jpg');
const fromRelative = (storage as any).buildKey('2024/06/image.jpg');

assert.equal(fromPrefixed, fromRelative);
});

it('preserves the historical leading-slash-as-decoration behaviour when the path has no storagePath segment', function () {
// handle-image-sizes calls exists(req.url) with values like
// `/size/w1200/...`. These are conceptually relative to the storage
// root and must continue to map to the same key as before.
const {storage} = createStorage();

const key = (storage as any).buildKey('/size/w1200/2024/06/photo.jpg');

assert.equal(key, 'configurable/prefix/content/files/size/w1200/2024/06/photo.jpg');
});

it('exists() probes the same key for absolute, pre-prefixed and relative inputs', async function () {
const {storage, sendStub} = createStorage();
const {HeadObjectCommand: HeadObjectCmd} = await import('@aws-sdk/client-s3');

sendStub.rejects(createNotFoundError());

await storage.exists('image.jpg', '/var/lib/ghost/content/files/2024/06');
await storage.exists('image.jpg', 'content/files/2024/06');
await storage.exists('image.jpg', '2024/06');

const keys = sendStub.getCalls().map(call => (call.args[0] as InstanceType<typeof HeadObjectCmd>).input.Key);
assert.equal(keys.length, 3);
assert.ok(keys.every(k => k === 'configurable/prefix/content/files/2024/06/image.jpg'),
`expected all three exists() probes to target the same key, got: ${JSON.stringify(keys)}`);
});

it('save() writes to the same key whether targetDir is absolute, pre-prefixed or relative', async function () {
const {storage, sendStub} = createStorage();

sinon.stub(storage, 'exists').resolves(false);
sinon.stub(fs.promises, 'stat').resolves({size: 256} as fs.Stats);
sinon.stub(fs.promises, 'readFile').resolves(Buffer.from('image-bytes'));

const targetDirs = [
'/var/lib/ghost/content/files/2024/06',
'content/files/2024/06',
'2024/06'
];

const keys: (string | undefined)[] = [];
for (const targetDir of targetDirs) {
sendStub.resetHistory();
await storage.save({path: '/tmp/image.jpg', name: 'image.jpg'}, targetDir);
const command = sendStub.firstCall.args[0] as PutObjectCommand;
keys.push(command.input.Key);
}

assert.ok(keys.every(k => k === 'configurable/prefix/content/files/2024/06/image.jpg'),
`expected all save()s to land at the same key, got: ${JSON.stringify(keys)}`);
});

it('still rejects path traversal that would escape the storage root', function () {
const {storage} = createStorage();

// Belt and braces: the existing `..` protection still fires when
// the input passes through normalisation.
assert.throws(() => {
(storage as any).buildKey('/var/lib/ghost/content/files/../../../etc/passwd');
}, /not a valid URL/);

assert.throws(() => {
(storage as any).buildKey('content/files/../../etc/passwd');
}, /not a valid URL/);
});
});

describe('Multipart Upload', function () {
function createMockReadStream(fileContent: Buffer) {
return Readable.from(fileContent);
Expand Down
Loading