theopensystemslab · jessicamcinchak · Dec 3, 2024 · Dec 3, 2024 · Dec 3, 2024
diff --git a/.env.example b/.env.example
@@ -22,7 +22,6 @@ MICROSOFT_CLIENT_SECRET=👻
 
 # AWS credentials for uploading user files from local and pull request environments to a staging S3 bucket
 AWS_S3_REGION=eu-west-2
-AWS_S3_ACL=public-read
 AWS_S3_BUCKET=👻
 AWS_ACCESS_KEY=👻
 AWS_SECRET_KEY=👻

diff --git a/api.planx.uk/modules/file/file.test.ts b/api.planx.uk/modules/file/file.test.ts
@@ -5,41 +5,45 @@ import app from "../../server.js";
 import { deleteFilesByURL } from "./service/deleteFile.js";
 import { authHeader } from "../../tests/mockJWT.js";
 
+import type * as s3Client from "@aws-sdk/client-s3";
+import { getSignedUrl } from "@aws-sdk/s3-request-presigner";
+
 let mockPutObject: Mocked<() => void>;
 let mockGetObject: Mocked<() => void>;
 let mockDeleteObjects: Mocked<() => void>;
 let getObjectResponse = {};
 
-const mockGetSignedUrl = vi.fn(() => {
-  const randomFolderName = "nanoid";
-  const modifiedKey = "modified%20key";
-  return `
-    https://test-bucket.s3.eu-west-2.amazonaws.com/${randomFolderName}/${modifiedKey}?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-SignedHeaders=host
-  `;
-});
+vi.mock("@aws-sdk/s3-request-presigner", () => ({
+  getSignedUrl: vi.fn(() => {
+    const randomFolderName = "nanoid";
+    const modifiedKey = "modified%20key";
+    return `https://test-bucket.s3.eu-west-2.amazonaws.com/${randomFolderName}/${modifiedKey}?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-SignedHeaders=host`;
+  }),
+}));
 
 const s3Mock = () => {
   return {
     putObject: mockPutObject,
     getObject: mockGetObject,
-    getSignedUrl: mockGetSignedUrl,
     deleteObjects: mockDeleteObjects,
   };
 };
 
-vi.mock("aws-sdk/clients/s3", () => ({
-  default: vi.fn().mockImplementation(() => {
-    return s3Mock();
-  }),
-}));
+vi.mock("@aws-sdk/client-s3", async (importOriginal) => {
+  const actualS3Client = await importOriginal<typeof s3Client>();
+  return {
+    ...actualS3Client,
+    S3: vi.fn().mockImplementation(() => {
+      return s3Mock();
+    }),
+  };
+});
 
 describe("File upload", () => {
   beforeEach(() => {
     vi.clearAllMocks();
 
-    mockPutObject = vi.fn(() => ({
-      promise: () => Promise.resolve(),
-    }));
+    mockPutObject = vi.fn(() => Promise.resolve());
   });
 
   describe("Private", () => {
@@ -70,26 +74,27 @@ describe("File upload", () => {
     });
 
     it("should upload file", async () => {
+      vi.stubEnv("API_URL_EXT", "https://api.editor.planx.dev");
+      vi.stubEnv("AWS_S3_BUCKET", "myBucketName");
+
       await supertest(app)
         .post(ENDPOINT)
         .field("filename", "some_file.txt")
         .attach("file", Buffer.from("some data"), "some_file.txt")
         .then((res) => {
           expect(res.body).toEqual({
             fileType: "text/plain",
-            fileUrl: expect.stringContaining(
-              "/file/private/nanoid/modified%20key",
-            ),
+            // Bucket name stripped from URL
+            fileUrl:
+              "https://api.editor.planx.dev/file/private/nanoid/modified%20key",
           });
         });
       expect(mockPutObject).toHaveBeenCalledTimes(1);
-      expect(mockGetSignedUrl).toHaveBeenCalledTimes(1);
+      expect(getSignedUrl).toHaveBeenCalledTimes(1);
     });
 
     it("should handle S3 error", async () => {
-      mockPutObject = vi.fn(() => ({
-        promise: () => Promise.reject(new Error("S3 error!")),
-      }));
+      mockPutObject = vi.fn(() => Promise.reject(new Error("S3 error!")));
 
       await supertest(app)
         .post("/file/private/upload")
@@ -101,6 +106,25 @@ describe("File upload", () => {
         });
       expect(mockPutObject).toHaveBeenCalledTimes(1);
     });
+
+    it("should generate a correct URL on production", async () => {
+      vi.stubEnv("API_URL_EXT", "https://api.editor.planx.uk");
+      vi.stubEnv("NODE_ENV", "production");
+
+      await supertest(app)
+        .post(ENDPOINT)
+        .field("filename", "some_file.txt")
+        .attach("file", Buffer.from("some data"), "some_file.txt")
+        .then((res) => {
+          expect(res.body).toEqual({
+            fileType: "text/plain",
+            fileUrl:
+              "https://api.editor.planx.uk/file/private/nanoid/modified%20key",
+          });
+        });
+      expect(mockPutObject).toHaveBeenCalledTimes(1);
+      expect(getSignedUrl).toHaveBeenCalledTimes(1);
+    });
   });
 
   describe("Public", () => {
@@ -160,13 +184,11 @@ describe("File upload", () => {
           });
         });
       expect(mockPutObject).toHaveBeenCalledTimes(1);
-      expect(mockGetSignedUrl).toHaveBeenCalledTimes(1);
+      expect(getSignedUrl).toHaveBeenCalledTimes(1);
     });
 
     it("should handle S3 error", async () => {
-      mockPutObject = vi.fn(() => ({
-        promise: () => Promise.reject(new Error("S3 error!")),
-      }));
+      mockPutObject = vi.fn(() => Promise.reject(new Error("S3 error!")));
 
       await supertest(app)
         .post(ENDPOINT)
@@ -185,7 +207,7 @@ describe("File upload", () => {
 describe("File download", () => {
   beforeEach(() => {
     getObjectResponse = {
-      Body: Buffer.from("some data"),
+      Body: { transformToByteArray: () => new ArrayBuffer(24) },
       ContentLength: "633",
       ContentDisposition: "inline;filename='some_file.txt'",
       ContentEncoding: "undefined",
@@ -197,9 +219,7 @@ describe("File download", () => {
     };
     vi.clearAllMocks();
 
-    mockGetObject = vi.fn(() => ({
-      promise: () => Promise.resolve(getObjectResponse),
-    }));
+    mockGetObject = vi.fn(() => Promise.resolve(getObjectResponse));
   });
 
   describe("Public", () => {
@@ -235,20 +255,33 @@ describe("File download", () => {
     });
 
     it("should handle S3 error", async () => {
-      mockGetObject = vi.fn(() => ({
-        promise: () => Promise.reject(new Error("S3 error!")),
-      }));
+      mockGetObject = vi.fn(() => Promise.reject(new Error("S3 error!")));
 
       await supertest(app)
         .get("/file/public/someKey/someFile.txt")
-        .field("filename", "some_file.txt")
-        .attach("file", Buffer.from("some data"), "some_file.txt")
         .expect(500)
         .then((res) => {
           expect(res.body.error).toMatch(/S3 error!/);
         });
       expect(mockGetObject).toHaveBeenCalledTimes(1);
     });
+
+    it("should handle an empty file body", async () => {
+      mockGetObject = vi.fn(() =>
+        Promise.resolve({
+          ...getObjectResponse,
+          Body: undefined,
+        }),
+      );
+
+      await supertest(app)
+        .get("/file/public/someKey/someFile.txt")
+        .expect(500)
+        .then((res) => {
+          expect(res.body.error).toMatch(/Missing body from S3 file/);
+        });
+      expect(mockGetObject).toHaveBeenCalledTimes(1);
+    });
   });
 
   describe("Private", () => {
@@ -313,9 +346,7 @@ describe("File download", () => {
     });
 
     it("should handle S3 error", async () => {
-      mockGetObject = vi.fn(() => ({
-        promise: () => Promise.reject(new Error("S3 error!")),
-      }));
+      mockGetObject = vi.fn(() => Promise.reject(new Error("S3 error!")));
 
       await supertest(app)
         .get("/file/private/someKey/someFile.txt")
@@ -337,9 +368,8 @@ describe("File delete", () => {
   });
 
   it("deletes files by URL", async () => {
-    mockDeleteObjects = vi.fn(() => ({
-      promise: () => Promise.resolve(),
-    }));
+    mockDeleteObjects = vi.fn(() => Promise.resolve());
+
     const fileURLs = [
       "https://api.planx.dev/file/private/abc/123",
       "https://api.planx.dev/file/private/def/456",
@@ -361,11 +391,10 @@ describe("File delete", () => {
   });
 
   it("throw an error if S3 fails to delete the file", async () => {
-    mockDeleteObjects = vi.fn(() => ({
-      promise: () => {
-        throw Error();
-      },
-    }));
+    mockDeleteObjects = vi.fn(() => {
+      throw Error();
+    });
+
     const fileURLs = [
       "https://api.planx.dev/file/private/abc/123",
       "https://api.planx.dev/file/private/def/456",

diff --git a/api.planx.uk/modules/file/service/deleteFile.ts b/api.planx.uk/modules/file/service/deleteFile.ts
@@ -1,4 +1,4 @@
-import type { DeleteObjectsRequest } from "aws-sdk/clients/s3.js";
+import type { DeleteObjectsCommandInput } from "@aws-sdk/client-s3";
 import { getS3KeyFromURL, s3Factory } from "./utils.js";
 
 export const deleteFilesByURL = async (
@@ -13,14 +13,14 @@ export const deleteFilesByKey = async (keys: string[]): Promise<string[]> => {
   const s3 = s3Factory();
   const params = getDeleteFilesParams(keys);
   try {
-    await s3.deleteObjects(params).promise();
+    await s3.deleteObjects(params);
     return keys;
   } catch (error) {
     throw Error(`Failed to delete S3 files: ${error}`);
   }
 };
 
-const getDeleteFilesParams = (keys: string[]): DeleteObjectsRequest => ({
+const getDeleteFilesParams = (keys: string[]): DeleteObjectsCommandInput => ({
   Bucket: process.env.AWS_S3_BUCKET!,
   Delete: {
     Objects: keys.map((key) => ({ Key: key })),

diff --git a/api.planx.uk/modules/file/service/getFile.ts b/api.planx.uk/modules/file/service/getFile.ts
@@ -1,25 +1,30 @@
-import type S3 from "aws-sdk/clients/s3.js";
+import type { PutObjectCommandInput } from "@aws-sdk/client-s3";
 import { s3Factory } from "./utils.js";
 
 export const getFileFromS3 = async (fileId: string) => {
   const s3 = s3Factory();
 
-  const params = {
+  const params: PutObjectCommandInput = {
     Key: fileId,
-  } as S3.PutObjectRequest;
+    Bucket: process.env.AWS_S3_BUCKET,
+  };
+
+  const file = await s3.getObject(params);
+
+  if (!file.Body) throw Error(`Missing body from S3 file ${fileId}`);
 
-  const file = await s3.getObject(params).promise();
+  const body = Buffer.from(await file.Body.transformToByteArray());
 
   return {
-    body: file.Body as Buffer,
+    body,
     isPrivate: file.Metadata?.is_private === "true",
     headers: {
       "Content-Type": file.ContentType,
       "Content-Length": file.ContentLength,
       "Content-Disposition": file.ContentDisposition,
       "Content-Encoding": file.ContentEncoding,
       "Cache-Control": file.CacheControl,
-      Expires: file.Expires,
+      Expires: file.ExpiresString,
       "Last-Modified": file.LastModified,
       ETag: file.ETag,
       "cross-origin-resource-policy": "cross-site",

diff --git a/api.planx.uk/modules/file/service/uploadFile.ts b/api.planx.uk/modules/file/service/uploadFile.ts
@@ -1,8 +1,12 @@
-import type S3 from "aws-sdk/clients/s3.js";
-import { customAlphabet } from "nanoid";
+import {
+  GetObjectCommand,
+  type PutObjectCommandInput,
+} from "@aws-sdk/client-s3";
+import { getSignedUrl } from "@aws-sdk/s3-request-presigner";
 import mime from "mime";
-import { s3Factory } from "./utils.js";
+import { customAlphabet } from "nanoid";
 import { isLiveEnv } from "../../../helpers.js";
+import { s3Factory } from "./utils.js";
 const nanoid = customAlphabet("1234567890abcdefghijklmnopqrstuvwxyz", 8);
 
 export const uploadPublicFile = async (
@@ -14,8 +18,8 @@ export const uploadPublicFile = async (
 
   const { params, key, fileType } = generateFileParams(file, filename, filekey);
 
-  await s3.putObject(params).promise();
-  const fileUrl = buildFileUrl(key, "public");
+  await s3.putObject(params);
+  const fileUrl = await buildFileUrl(key, "public");
 
   return {
     fileType,
@@ -36,8 +40,8 @@ export const uploadPrivateFile = async (
     is_private: "true",
   };
 
-  await s3.putObject(params).promise();
-  const fileUrl = buildFileUrl(key, "private");
+  await s3.putObject(params);
+  const fileUrl = await buildFileUrl(key, "private");
 
   return {
     fileType,
@@ -46,37 +50,39 @@ export const uploadPrivateFile = async (
 };
 
 // Construct an API URL for the uploaded file
-const buildFileUrl = (key: string, path: "public" | "private") => {
+const buildFileUrl = async (key: string, path: "public" | "private") => {
   const s3 = s3Factory();
-  const s3Url = new URL(s3.getSignedUrl("getObject", { Key: key }));
-  let s3Pathname = s3Url.pathname;
+  const s3Url = await getSignedUrl(
+    s3,
+    new GetObjectCommand({ Key: key, Bucket: process.env.AWS_S3_BUCKET }),
+  );
+  let s3Pathname = new URL(s3Url).pathname;
   // Minio returns a pathname with bucket name prepended, remove this
   if (!isLiveEnv())
     s3Pathname = s3Pathname.replace(`/${process.env.AWS_S3_BUCKET}`, "");
-  // URL.pathname has a leading "/"
-  const fileUrl = `${process.env.API_URL_EXT}/file/${path}${s3Pathname}`;
-  return fileUrl;
+  return `${process.env.API_URL_EXT}/file/${path}${s3Pathname}`;
 };
 
 export function generateFileParams(
   file: Express.Multer.File,
   filename: string,
   filekey?: string,
 ): {
-  params: S3.PutObjectRequest;
+  params: PutObjectCommandInput;
   fileType: string | null;
   key: string;
 } {
   const fileType = mime.getType(filename);
   const key = `${filekey || nanoid()}/${filename}`;
 
-  const params = {
-    ACL: process.env.AWS_S3_ACL,
+  const params: PutObjectCommandInput = {
+    ACL: "public-read",
+    Bucket: process.env.AWS_S3_BUCKET,
     Key: key,
-    Body: file?.buffer || JSON.stringify(file),
+    Body: file.buffer,
     ContentDisposition: `inline;filename="${filename}"`,
-    ContentType: file?.mimetype || "application/json",
-  } as S3.PutObjectRequest;
+    ContentType: file.mimetype,
+  };
 
   return {
     fileType,