fix: serve image bytes from the API for bulk download

src/static/aws/s3.service.ts

@@ -35,14 +35,33 @@ export class AWSS3Service implements Static {
   }
 
   async getImage(fileName: string): Promise<PNGWithMetadata> {
+    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<Buffer | null> {
     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;
     }
   }
 

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/);
+  });
+});

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<string> {
@@ -47,6 +59,21 @@ export class HddService implements Static {
     }
   }
 
+  async getImageBuffer(imageName: string): Promise<Buffer | null> {
+    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<boolean> {
     if (!imageName) return;
     return new Promise((resolvePromise) => {

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>(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();
+    }
+  );
+});

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 `<img>` 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);
+  }
 }

src/static/static.interface.ts

@@ -3,6 +3,7 @@ import { PNGWithMetadata } from 'pngjs';
 export interface Static {
   saveImage(type: 'screenshot' | 'diff' | 'baseline', imageBuffer: Buffer): Promise<string>;
   getImage(fileName: string): Promise<PNGWithMetadata>;
+  getImageBuffer(fileName: string): Promise<Buffer | null>;
   deleteImage(imageName: string): Promise<boolean>;
   getImageUrl(imageName: string): Promise<string>;
 }

src/static/static.service.ts

@@ -20,6 +20,10 @@ export class StaticService {
     return this.staticService.getImage(imageName);
   }
 
+  async getImageBuffer(imageName: string): Promise<Buffer | null> {
+    return this.staticService.getImageBuffer(imageName);
+  }
+
   async deleteImage(imageName: string): Promise<boolean> {
     return this.staticService.deleteImage(imageName);
   }