diff --git a/src/static/aws/s3.service.ts b/src/static/aws/s3.service.ts index 50da45bb..9b527027 100644 --- a/src/static/aws/s3.service.ts +++ b/src/static/aws/s3.service.ts @@ -35,14 +35,33 @@ export class AWSS3Service implements Static { } async getImage(fileName: string): Promise { + if (!fileName) return null; + 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 get 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}`); + // 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..860195e1 --- /dev/null +++ b/src/static/hdd/hdd.service.spec.ts @@ -0,0 +1,33 @@ +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$/); + }); + + // 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/); + } + ); + + 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 ca3c7066..30a48deb 100644 --- a/src/static/hdd/hdd.service.ts +++ b/src/static/hdd/hdd.service.ts @@ -19,7 +19,19 @@ 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 + // 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); + 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; } getImageUrl(imageName: string): Promise { @@ -47,6 +59,21 @@ 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}`); + // 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; + } + } + async deleteImage(imageName: string): Promise { if (!imageName) return; return new Promise((resolvePromise) => { 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 01bec303..ab42616f 100644 --- a/src/static/static.controller.ts +++ b/src/static/static.controller.ts @@ -1,6 +1,7 @@ -import { Controller, Get, Logger, 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') @@ -20,4 +21,32 @@ 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) { + // a stored image is always a plain file name, so anything carrying a path + // 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'); + } + + 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); }