From 35cecf9c91ed75b215506d888efa24a2e134b35b Mon Sep 17 00:00:00 2001 From: Amruth Pillai Date: Sat, 5 Sep 2026 07:29:42 -0700 Subject: [PATCH] fix(storage): support S3 buckets with object ACLs disabled (#3432) --- apps/server/src/static/uploads-s3.test.ts | 110 +++++++++++++++++++ packages/api/src/features/storage/service.ts | 5 +- 2 files changed, 113 insertions(+), 2 deletions(-) create mode 100644 apps/server/src/static/uploads-s3.test.ts diff --git a/apps/server/src/static/uploads-s3.test.ts b/apps/server/src/static/uploads-s3.test.ts new file mode 100644 index 000000000..3fb68ffde --- /dev/null +++ b/apps/server/src/static/uploads-s3.test.ts @@ -0,0 +1,110 @@ +import type { IncomingHttpHeaders } from "node:http"; +import { createServer } from "node:http"; +import { afterAll, beforeAll, beforeEach, expect, it, vi } from "vitest"; + +const envMock = vi.hoisted(() => ({ + APP_URL: "https://resume.example.com", + S3_ACCESS_KEY_ID: "test-access-key", + S3_SECRET_ACCESS_KEY: "test-secret-key", + S3_REGION: "us-east-1", + S3_ENDPOINT: "", + S3_BUCKET: "test-bucket", + S3_FORCE_PATH_STYLE: true, +})); +vi.mock("@reactive-resume/env/server", () => ({ env: envMock })); + +type StoredObject = { data: Buffer; contentType: string }; +type StorageRequest = { method: string; path: string; headers: IncomingHttpHeaders }; +const objects = new Map(); +const requests: StorageRequest[] = []; + +// Wire-contract stub, not an AWS emulator. It applies the documented BucketOwnerEnforced +// PUT rule to real SDK requests: no ACL or bucket-owner-full-control is accepted. +// https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-ownership-error-responses.html +const server = createServer(async (request, response) => { + const path = new URL(request.url ?? "/", "http://localhost").pathname; + requests.push({ method: request.method ?? "", path, headers: request.headers }); + const fail = (status: number, code: string) => { + response.writeHead(status, { "Content-Type": "application/xml" }); + response.end(`${code}${code}`); + }; + // Only checks that the SDK authenticates its requests; this stub does not verify signatures. + if (!request.headers.authorization?.startsWith("AWS4-HMAC-SHA256 ")) return fail(403, "AccessDenied"); + if (request.method === "PUT") { + const chunks: Buffer[] = []; + for await (const chunk of request) chunks.push(Buffer.from(chunk)); + const acl = request.headers["x-amz-acl"]; + if (acl && acl !== "bucket-owner-full-control") return fail(400, "AccessControlListNotSupported"); + objects.set(path, { + data: Buffer.concat(chunks), + contentType: request.headers["content-type"] ?? "application/octet-stream", + }); + response.writeHead(200, { ETag: '"test-etag"' }); + return response.end(); + } + if (request.method === "DELETE") { + objects.delete(path); + response.writeHead(204); + return response.end(); + } + const object = objects.get(path); + if (!object) return fail(404, "NoSuchKey"); + response.writeHead(200, { "Content-Type": object.contentType, "Content-Length": object.data.length }); + response.end(object.data); +}); + +let storage: ReturnType; +let handleUpload: typeof import("./uploads").handleUpload; + +beforeAll(async () => { + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + const address = server.address(); + if (!address || typeof address === "string") throw new Error("Missing stub TCP address"); + envMock.S3_ENDPOINT = `http://127.0.0.1:${address.port}`; + storage = (await import("@reactive-resume/api/features/storage")).getStorageService(); + ({ handleUpload } = await import("./uploads")); +}); + +beforeEach(() => { + objects.clear(); + requests.length = 0; +}); + +afterAll(async () => { + server.closeAllConnections(); + await new Promise((resolve, reject) => server.close((error) => (error ? reject(error) : resolve()))); +}); + +it("keeps the ACL-disabled storage health check healthy", async () => { + expect(await storage.healthcheck()).toMatchObject({ status: "healthy", type: "s3" }); + expect(requests.map(({ method }) => method)).toEqual(["PUT", "DELETE"]); + expect(objects.size).toBe(0); +}); + +it("stores images without ACLs and serves them through the signed application proxy", async () => { + const key = "uploads/user-1/pictures/photo.png"; + const data = new Uint8Array([137, 80, 78, 71]); + await storage.write({ key, data, contentType: "image/png" }); + expect(requests[0]?.headers["x-amz-acl"]).toBeUndefined(); + const direct = await fetch(`${envMock.S3_ENDPOINT}/${envMock.S3_BUCKET}/${key}`); + expect(direct.status).toBe(403); + const response = await handleUpload(new Request(`${envMock.APP_URL}/api/${key}`)); + expect(response.status).toBe(200); + expect(response.headers.get("Content-Type")).toBe("image/png"); + expect(new Uint8Array(await response.arrayBuffer())).toEqual(data); + expect(requests.at(-1)?.headers.authorization).toMatch(/^AWS4-HMAC-SHA256 /); +}); + +it("stores private attachments without ACLs while keeping them outside the public proxy", async () => { + const key = "uploads/user-1/agent/thread-1/private.txt"; + const data = new TextEncoder().encode("private attachment"); + await storage.write({ key, data, contentType: "text/plain", private: true }); + expect(requests[0]?.headers["x-amz-acl"]).toBeUndefined(); + const requestCount = requests.length; + const response = await handleUpload(new Request(`${envMock.APP_URL}/api/${key}`)); + expect(response.status).toBe(404); + expect(requests).toHaveLength(requestCount); + const direct = await fetch(`${envMock.S3_ENDPOINT}/${envMock.S3_BUCKET}/${key}`); + expect(direct.status).toBe(403); + expect((await storage.read(key))?.data).toEqual(data); +}); diff --git a/packages/api/src/features/storage/service.ts b/packages/api/src/features/storage/service.ts index 63905cb87..989367440 100644 --- a/packages/api/src/features/storage/service.ts +++ b/packages/api/src/features/storage/service.ts @@ -251,12 +251,13 @@ class S3StorageService implements StorageService { return response.Contents.map((object) => object.Key ?? ""); } - async write({ key, data, contentType, private: isPrivate }: StorageWriteInput): Promise { + async write({ key, data, contentType }: StorageWriteInput): Promise { + // BucketOwnerEnforced rejects object ACLs. Public files use the application proxy + // with authenticated S3 reads; private attachments retain their access checks. const command = new PutObjectCommand({ Bucket: this.bucket, Key: key, Body: data, - ACL: isPrivate ? "private" : "public-read", ContentType: contentType, });