From 544c91a449ba76ee0102ace9b406c747127c8543 Mon Sep 17 00:00:00 2001 From: "mykola.gervasyuk" Date: Thu, 20 Aug 2026 15:11:44 +0300 Subject: [PATCH 1/3] fix: serve image bytes from the API for bulk download Downloading images for the selected rows builds a zip in the browser, which means fetching each image. With S3 storage, GET /images/:fileName redirects to a pre-signed URL that answers without CORS headers, so every fetch is blocked and the download fails. An tag is unaffected, which is why the images still display. Add GET /images/:fileName/download, which reads the bytes through the storage service and answers from the API's own origin, and keep the redirect for display so image traffic still bypasses the API. --- src/static/aws/s3.service.ts | 14 +++++++++++++- src/static/hdd/hdd.service.ts | 10 ++++++++++ src/static/static.controller.ts | 22 +++++++++++++++++++++- src/static/static.interface.ts | 1 + src/static/static.service.ts | 4 ++++ 5 files changed, 49 insertions(+), 2 deletions(-) diff --git a/src/static/aws/s3.service.ts b/src/static/aws/s3.service.ts index 50da45bb..b1d7dd7e 100644 --- a/src/static/aws/s3.service.ts +++ b/src/static/aws/s3.service.ts @@ -35,14 +35,26 @@ export class AWSS3Service implements Static { } async getImage(fileName: string): Promise { + if (!fileName) return null; + const imageBuffer = await this.getImageBuffer(fileName); + if (!imageBuffer) return undefined; + try { + return PNG.sync.read(imageBuffer); + } catch (ex) { + this.logger.error(`Error from read : Cannot decode image: ${fileName}. ${ex}`); + } + } + + async getImageBuffer(fileName: string): Promise { if (!fileName) return null; try { const command = new GetObjectCommand({ Bucket: this.AWS_S3_BUCKET_NAME, Key: fileName }); const s3Response = await this.s3Client.send(command); const stream = s3Response.Body as Readable; - return PNG.sync.read(Buffer.concat(await stream.toArray())); + return Buffer.concat(await stream.toArray()); } catch (ex) { this.logger.error(`Error from read : Cannot get image: ${fileName}. ${ex}`); + return null; } } diff --git a/src/static/hdd/hdd.service.ts b/src/static/hdd/hdd.service.ts index ca3c7066..7fc98abe 100644 --- a/src/static/hdd/hdd.service.ts +++ b/src/static/hdd/hdd.service.ts @@ -47,6 +47,16 @@ export class HddService implements Static { } } + async getImageBuffer(imageName: string): Promise { + if (!imageName) return null; + try { + return readFileSync(this.getImagePath(imageName)); + } catch (ex) { + this.logger.error(`Cannot get image: ${imageName}. ${ex}`); + return null; + } + } + async deleteImage(imageName: string): Promise { if (!imageName) return; return new Promise((resolvePromise) => { diff --git a/src/static/static.controller.ts b/src/static/static.controller.ts index 01bec303..9e830ee1 100644 --- a/src/static/static.controller.ts +++ b/src/static/static.controller.ts @@ -1,4 +1,4 @@ -import { Controller, Get, Logger, Param, Res } from '@nestjs/common'; +import { Controller, Get, Logger, NotFoundException, Param, Res } from '@nestjs/common'; import { Response } from 'express'; import { ApiOkResponse, ApiTags } from '@nestjs/swagger'; import { StaticService } from './static.service'; @@ -20,4 +20,24 @@ export class StaticController { res.status(500).send('Error occurred while getting the file.'); } } + + /** + * Serves the image bytes from this origin instead of redirecting to storage. + * Redirected pre-signed S3 URLs carry no CORS headers, so a browser `fetch` + * (used to build the bulk download zip) is blocked; an `` tag is not, + * which is why display keeps using the redirect above. + */ + @Get('/:fileName/download') + @ApiOkResponse() + async download(@Param('fileName') fileName: string, @Res() res: Response) { + const imageBuffer = await this.staticService.getImageBuffer(fileName); + if (!imageBuffer) { + throw new NotFoundException(`Image not found: ${fileName}`); + } + res.set({ + 'Content-Type': 'image/png', + 'Content-Disposition': `attachment; filename="${fileName}"`, + }); + res.send(imageBuffer); + } } diff --git a/src/static/static.interface.ts b/src/static/static.interface.ts index 906d4716..9e0a0629 100644 --- a/src/static/static.interface.ts +++ b/src/static/static.interface.ts @@ -3,6 +3,7 @@ import { PNGWithMetadata } from 'pngjs'; export interface Static { saveImage(type: 'screenshot' | 'diff' | 'baseline', imageBuffer: Buffer): Promise; getImage(fileName: string): Promise; + getImageBuffer(fileName: string): Promise; deleteImage(imageName: string): Promise; getImageUrl(imageName: string): Promise; } diff --git a/src/static/static.service.ts b/src/static/static.service.ts index 062379b3..ec3d6c11 100644 --- a/src/static/static.service.ts +++ b/src/static/static.service.ts @@ -20,6 +20,10 @@ export class StaticService { return this.staticService.getImage(imageName); } + async getImageBuffer(imageName: string): Promise { + return this.staticService.getImageBuffer(imageName); + } + async deleteImage(imageName: string): Promise { return this.staticService.deleteImage(imageName); } From 6d1015d363fb0ffcaac54be8907c2a61780e7050 Mon Sep 17 00:00:00 2001 From: "mykola.gervasyuk" Date: Thu, 20 Aug 2026 16:25:00 +0300 Subject: [PATCH 2/3] fix(static): keep image names inside the image directory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The download route reads the file itself, so a request-controlled name went straight into path.resolve — a traversal value such as ..%2F..%2Fetc%2Fpasswd could read any file the process can see. The redirect route was not exposed this way because express serves those bytes. Reject names that carry a path before touching storage, and keep a containment check in HddService.getImagePath as the backstop for every other caller. Retrieval now also returns null only for a genuinely absent image (ENOENT / NoSuchKey) so that a permission or network failure cannot read as a missing image; the comparison pipeline keeps treating an unreadable image as a missing baseline. --- src/static/aws/s3.service.ts | 15 +++++++++++---- src/static/hdd/hdd.service.spec.ts | 24 ++++++++++++++++++++++++ src/static/hdd/hdd.service.ts | 17 +++++++++++++++-- src/static/static.controller.ts | 9 ++++++++- 4 files changed, 58 insertions(+), 7 deletions(-) create mode 100644 src/static/hdd/hdd.service.spec.ts diff --git a/src/static/aws/s3.service.ts b/src/static/aws/s3.service.ts index b1d7dd7e..9b527027 100644 --- a/src/static/aws/s3.service.ts +++ b/src/static/aws/s3.service.ts @@ -36,12 +36,14 @@ export class AWSS3Service implements Static { async getImage(fileName: string): Promise { if (!fileName) return null; - const imageBuffer = await this.getImageBuffer(fileName); - if (!imageBuffer) return undefined; try { + // the comparison pipeline treats an unreadable image as a missing + // baseline, so storage failures stay contained here + const imageBuffer = await this.getImageBuffer(fileName); + if (!imageBuffer) return undefined; return PNG.sync.read(imageBuffer); } catch (ex) { - this.logger.error(`Error from read : Cannot decode image: ${fileName}. ${ex}`); + this.logger.error(`Error from read : Cannot get image: ${fileName}. ${ex}`); } } @@ -54,7 +56,12 @@ export class AWSS3Service implements Static { return Buffer.concat(await stream.toArray()); } catch (ex) { this.logger.error(`Error from read : Cannot get image: ${fileName}. ${ex}`); - return null; + // only a missing object means "no image"; credentials, throttling and + // network failures have to stay errors instead of reading as absence + if (ex?.name === 'NoSuchKey' || ex?.$metadata?.httpStatusCode === 404) { + return null; + } + throw ex; } } diff --git a/src/static/hdd/hdd.service.spec.ts b/src/static/hdd/hdd.service.spec.ts new file mode 100644 index 00000000..da944409 --- /dev/null +++ b/src/static/hdd/hdd.service.spec.ts @@ -0,0 +1,24 @@ +import { HddService } from './hdd.service'; + +describe('HddService', () => { + const service = new HddService(); + + it('resolves a plain image name inside the image directory', () => { + expect(service.getImagePath('ABC123.screenshot.png')).toMatch(/imageUploads\/ABC123\.screenshot\.png$/); + }); + + it.each(['../../etc/passwd', '../outside.png', '/etc/passwd', 'nested/../../outside.png', '..'])( + 'rejects an image name pointing outside the image directory: %s', + (imageName) => { + expect(() => service.getImagePath(imageName)).toThrow(/outside of the image directory/); + } + ); + + it('returns null for a missing image', async () => { + await expect(service.getImageBuffer('definitely-missing.png')).resolves.toBeNull(); + }); + + it('propagates a traversal attempt instead of reading the file', async () => { + await expect(service.getImageBuffer('../../etc/passwd')).rejects.toThrow(/outside of the image directory/); + }); +}); diff --git a/src/static/hdd/hdd.service.ts b/src/static/hdd/hdd.service.ts index 7fc98abe..9f635628 100644 --- a/src/static/hdd/hdd.service.ts +++ b/src/static/hdd/hdd.service.ts @@ -19,7 +19,15 @@ export class HddService implements Static { getImagePath(imageName: string): string { this.ensureDirectoryExistence(HDD_IMAGE_PATH); - return path.resolve(HDD_IMAGE_PATH, imageName); + const root = path.resolve(HDD_IMAGE_PATH); + const imagePath = path.resolve(root, imageName); + // image names reach here straight from the request, so a traversal value + // would otherwise read any file the process can see + const relativeToRoot = path.relative(root, imagePath); + if (!relativeToRoot || relativeToRoot.startsWith('..') || path.isAbsolute(relativeToRoot)) { + throw new Error(`Image name outside of the image directory: ${imageName}`); + } + return imagePath; } getImageUrl(imageName: string): Promise { @@ -53,7 +61,12 @@ export class HddService implements Static { return readFileSync(this.getImagePath(imageName)); } catch (ex) { this.logger.error(`Cannot get image: ${imageName}. ${ex}`); - return null; + // an absent file is the only case that means "no image"; a permission or + // I/O failure has to stay an error rather than read as a missing image + if (ex?.code === 'ENOENT') { + return null; + } + throw ex; } } diff --git a/src/static/static.controller.ts b/src/static/static.controller.ts index 9e830ee1..2abe871c 100644 --- a/src/static/static.controller.ts +++ b/src/static/static.controller.ts @@ -1,6 +1,7 @@ -import { Controller, Get, Logger, NotFoundException, Param, Res } from '@nestjs/common'; +import { BadRequestException, Controller, Get, Logger, NotFoundException, Param, Res } from '@nestjs/common'; import { Response } from 'express'; import { ApiOkResponse, ApiTags } from '@nestjs/swagger'; +import { basename } from 'path'; import { StaticService } from './static.service'; @ApiTags('images') @@ -30,6 +31,12 @@ export class StaticController { @Get('/:fileName/download') @ApiOkResponse() async download(@Param('fileName') fileName: string, @Res() res: Response) { + // a stored image is always a plain file name, so anything carrying a path + // is rejected before it reaches storage + if (!fileName || fileName !== basename(fileName)) { + throw new BadRequestException('Invalid image name'); + } + const imageBuffer = await this.staticService.getImageBuffer(fileName); if (!imageBuffer) { throw new NotFoundException(`Image not found: ${fileName}`); From 01487d3b0dd6b069c452cc10f807670f994e184f Mon Sep 17 00:00:00 2001 From: "mykola.gervasyuk" Date: Fri, 21 Aug 2026 17:45:12 +0300 Subject: [PATCH 3/3] fix(static): only reject names that climb out of the image directory The containment check turned away any name whose resolved path started with two dots, which includes a file called ..thumbnail.png that sits inside the directory. Match a parent-directory step instead. The controller's check leaned on basename, which returns '.' and '..' unchanged, so both reached storage: the HDD backend then failed the containment check and answered 500, and S3 was asked for the key. Name them alongside the path check so they are turned away as bad requests. Both are covered by tests, including a new spec for the controller, since it is where the request-controlled name is first seen. --- src/static/hdd/hdd.service.spec.ts | 11 ++++- src/static/hdd/hdd.service.ts | 6 ++- src/static/static.controller.spec.ts | 61 ++++++++++++++++++++++++++++ src/static/static.controller.ts | 6 ++- 4 files changed, 80 insertions(+), 4 deletions(-) create mode 100644 src/static/static.controller.spec.ts diff --git a/src/static/hdd/hdd.service.spec.ts b/src/static/hdd/hdd.service.spec.ts index da944409..860195e1 100644 --- a/src/static/hdd/hdd.service.spec.ts +++ b/src/static/hdd/hdd.service.spec.ts @@ -7,7 +7,16 @@ describe('HddService', () => { expect(service.getImagePath('ABC123.screenshot.png')).toMatch(/imageUploads\/ABC123\.screenshot\.png$/); }); - it.each(['../../etc/passwd', '../outside.png', '/etc/passwd', 'nested/../../outside.png', '..'])( + // a name may begin with dots without climbing out of the directory + it.each(['..thumbnail.png', '...png', '.hidden.png'])( + 'resolves a dotted name inside the directory: %s', + (imageName) => { + expect(service.getImagePath(imageName)).toContain('imageUploads'); + expect(() => service.getImagePath(imageName)).not.toThrow(); + } + ); + + it.each(['../../etc/passwd', '../outside.png', '/etc/passwd', 'nested/../../outside.png', '..', '.'])( 'rejects an image name pointing outside the image directory: %s', (imageName) => { expect(() => service.getImagePath(imageName)).toThrow(/outside of the image directory/); diff --git a/src/static/hdd/hdd.service.ts b/src/static/hdd/hdd.service.ts index 9f635628..30a48deb 100644 --- a/src/static/hdd/hdd.service.ts +++ b/src/static/hdd/hdd.service.ts @@ -23,8 +23,12 @@ export class HddService implements Static { const imagePath = path.resolve(root, imageName); // image names reach here straight from the request, so a traversal value // would otherwise read any file the process can see + // only a leading parent-directory step means the name climbs out: a file + // whose name merely starts with dots stays inside const relativeToRoot = path.relative(root, imagePath); - if (!relativeToRoot || relativeToRoot.startsWith('..') || path.isAbsolute(relativeToRoot)) { + const climbsOut = + relativeToRoot === '..' || relativeToRoot.startsWith(`..${path.sep}`) || path.isAbsolute(relativeToRoot); + if (!relativeToRoot || climbsOut) { throw new Error(`Image name outside of the image directory: ${imageName}`); } return imagePath; diff --git a/src/static/static.controller.spec.ts b/src/static/static.controller.spec.ts new file mode 100644 index 00000000..11dce6d5 --- /dev/null +++ b/src/static/static.controller.spec.ts @@ -0,0 +1,61 @@ +import { Test, TestingModule } from '@nestjs/testing'; +import { BadRequestException, NotFoundException } from '@nestjs/common'; +import { Response } from 'express'; +import { StaticController } from './static.controller'; +import { StaticService } from './static.service'; + +const initController = async (getImageBufferMock = jest.fn()) => { + const module: TestingModule = await Test.createTestingModule({ + controllers: [StaticController], + providers: [ + { + provide: StaticService, + useValue: { getImageBuffer: getImageBufferMock, getImageUrl: jest.fn() }, + }, + ], + }).compile(); + + return module.get(StaticController); +}; + +const responseMock = () => { + const res = { set: jest.fn(), send: jest.fn() }; + return res as unknown as Response & { set: jest.Mock; send: jest.Mock }; +}; + +describe('download', () => { + it('answers with the image bytes', async () => { + const imageBuffer = Buffer.from([1, 2, 3]); + const getImageBufferMock = jest.fn().mockResolvedValueOnce(imageBuffer); + const controller = await initController(getImageBufferMock); + const res = responseMock(); + + await controller.download('image.png', res); + + expect(getImageBufferMock).toHaveBeenCalledWith('image.png'); + expect(res.set).toHaveBeenCalledWith({ + 'Content-Type': 'image/png', + 'Content-Disposition': 'attachment; filename="image.png"', + }); + expect(res.send).toHaveBeenCalledWith(imageBuffer); + }); + + it('answers 404 for an image that is not there', async () => { + const getImageBufferMock = jest.fn().mockResolvedValueOnce(null); + const controller = await initController(getImageBufferMock); + + await expect(controller.download('missing.png', responseMock())).rejects.toThrow(NotFoundException); + }); + + // a name carrying a path could otherwise read any file the process can see + it.each(['../../etc/passwd', '..', '.', '', 'nested/image.png', '/etc/passwd'])( + 'rejects %p without touching storage', + async (fileName) => { + const getImageBufferMock = jest.fn(); + const controller = await initController(getImageBufferMock); + + await expect(controller.download(fileName, responseMock())).rejects.toThrow(BadRequestException); + expect(getImageBufferMock).not.toHaveBeenCalled(); + } + ); +}); diff --git a/src/static/static.controller.ts b/src/static/static.controller.ts index 2abe871c..ab42616f 100644 --- a/src/static/static.controller.ts +++ b/src/static/static.controller.ts @@ -32,8 +32,10 @@ export class StaticController { @ApiOkResponse() async download(@Param('fileName') fileName: string, @Res() res: Response) { // a stored image is always a plain file name, so anything carrying a path - // is rejected before it reaches storage - if (!fileName || fileName !== basename(fileName)) { + // is rejected before it reaches storage. basename keeps '.' and '..' as + // they are, so both are named here rather than reaching a backend that + // would answer 500 for them, or ask storage for a directory. + if (!fileName || fileName === '.' || fileName === '..' || fileName !== basename(fileName)) { throw new BadRequestException('Invalid image name'); }