fix(storage): support S3 buckets with object ACLs disabled (#3432)

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<string, StoredObject>();
+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(`<Error><Code>${code}</Code><Message>${code}</Message></Error>`);
+	};
+	// 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<typeof import("@reactive-resume/api/features/storage").getStorageService>;
+let handleUpload: typeof import("./uploads").handleUpload;
+
+beforeAll(async () => {
+	await new Promise<void>((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<void>((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);
+});

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<void> {
+	async write({ key, data, contentType }: StorageWriteInput): Promise<void> {
+		// 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,
 		});