diff --git a/packages/api-client/src/schema.ts b/packages/api-client/src/schema.ts index d3a7b87e..53ad0df4 100644 --- a/packages/api-client/src/schema.ts +++ b/packages/api-client/src/schema.ts @@ -282,6 +282,12 @@ export interface components { GitBranch: string; /** @description SHA256 hash */ Sha256Hash: string; + /** @description Screenshot file to upload */ + ScreenshotUploadRequest: { + key: components["schemas"]["Sha256Hash"]; + /** @description Content type of the snapshot file */ + contentType: string; + }; /** * @description A unique identifier for the build * @example 12345 @@ -390,8 +396,8 @@ export interface components { } | null; pwTraceKey?: string | null; threshold?: number | null; - /** @default image/png */ - contentType: string; + /** @description Content type of the snapshot file */ + contentType?: string; }; /** @description Build metadata */ BuildMetadata: { @@ -479,6 +485,8 @@ export interface components { /** @enum {string} */ state: "pending" | "running" | "success" | "failed" | "canceled"; }; + /** Format: uri */ + url: string; } | null; }; /** @description Git reference */ @@ -789,8 +797,6 @@ export interface operations { commit: components["schemas"]["Sha1Hash"]; /** @description The branch the build is running on */ branch: components["schemas"]["GitBranch"]; - /** @description Keys of screenshot files */ - screenshotKeys: components["schemas"]["Sha256Hash"][]; /** @description Keys of Playwright trace files */ pwTraceKeys?: components["schemas"]["Sha256Hash"][]; /** @description The name of the build (for multi-build setups) */ @@ -830,7 +836,18 @@ export interface operations { * This is useful when a build is created from an incomplete test suite where some tests are skipped. */ subset?: boolean | null; - }; + } & ({ + /** + * @deprecated + * @description Keys of screenshot files + */ + screenshotKeys: components["schemas"]["Sha256Hash"][]; + screenshots?: unknown; + } | { + /** @description Screenshot files to upload */ + screenshots: components["schemas"]["ScreenshotUploadRequest"][]; + screenshotKeys?: unknown; + }); }; }; responses: { @@ -842,16 +859,38 @@ export interface operations { content: { "application/json": { build: components["schemas"]["Build"]; - screenshots: { + screenshots: ({ key: string; - /** Format: uri */ + /** + * Format: uri + * @deprecated + * @description Deprecated. Use postUrl and fields instead. + */ putUrl: string; - }[]; - pwTraces: { + } | { key: string; /** Format: uri */ + postUrl: string; + fields: { + [key: string]: string; + }; + })[]; + pwTraces: ({ + key: string; + /** + * Format: uri + * @deprecated + * @description Deprecated. Use postUrl and fields instead. + */ putUrl: string; - }[]; + } | { + key: string; + /** Format: uri */ + postUrl: string; + fields: { + [key: string]: string; + }; + })[]; }; }; }; @@ -1446,6 +1485,15 @@ export interface operations { "application/json": components["schemas"]["Error"]; }; }; + /** @description Not found */ + 404: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["Error"]; + }; + }; /** @description Server error */ 500: { headers: { @@ -1455,6 +1503,15 @@ export interface operations { "application/json": components["schemas"]["Error"]; }; }; + /** @description Service unavailable */ + 503: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["Error"]; + }; + }; }; }; getAuthProject: { @@ -1880,10 +1937,18 @@ export interface operations { content: { "application/json": { /** - * @description Overall review conclusion for the build: "APPROVE" or "REQUEST_CHANGES" + * @description Review event to apply to the build: "APPROVE", "REJECT" or "COMMENT". Required when `conclusion` is not provided. + * @enum {string} + */ + event?: "APPROVE" | "REJECT" | "COMMENT"; + /** + * @deprecated + * @description Deprecated: use `event` instead. Overall review conclusion for the build: "APPROVE" or "REQUEST_CHANGES". * @enum {string} */ - conclusion: "APPROVE" | "REQUEST_CHANGES"; + conclusion?: "APPROVE" | "REQUEST_CHANGES"; + /** @description Optional comment to attach to the review. Expected as the JSON representation of a rich-text document. */ + body?: unknown; /** * @description Optional per-snapshot review decisions. When omitted, only the build-level review is recorded. * @default [] @@ -1910,10 +1975,19 @@ export interface operations { "application/json": { id: string; /** @enum {string} */ - state: "approved" | "rejected"; + state: "approved" | "rejected" | "commented" | "pending"; }; }; }; + /** @description Invalid parameters */ + 400: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["Error"]; + }; + }; /** @description Unauthorized */ 401: { headers: { diff --git a/packages/core/mocks/handlers/createBuild.ts b/packages/core/mocks/handlers/createBuild.ts index 72373077..9679d3c0 100644 --- a/packages/core/mocks/handlers/createBuild.ts +++ b/packages/core/mocks/handlers/createBuild.ts @@ -3,7 +3,8 @@ import { http, HttpResponse } from "msw"; type CreateBuildParams = never; type CreateBuildRequestBody = { commit: string; - screenshotKeys: string[]; + screenshots: { key: string; contentType: string }[]; + pwTraceKeys?: string[]; branch?: string | null; name?: string | null; parallel?: boolean | null; @@ -18,7 +19,13 @@ type CreateBuildResponseBody = { }; screenshots: { key: string; - putUrl: string; + postUrl: string; + fields: Record; + }[]; + pwTraces: { + key: string; + postUrl: string; + fields: Record; }[]; }; @@ -27,15 +34,27 @@ export const createBuild = http.post< CreateBuildRequestBody, CreateBuildResponseBody >("https://api.argos-ci.dev/builds", async ({ request }) => { - const { screenshotKeys } = await request.json(); + const body = await request.json(); return HttpResponse.json({ build: { id: "123", url: "https://app.argos-ci.dev/builds/123", }, - screenshots: screenshotKeys.map((key) => ({ + screenshots: body.screenshots.map((screenshot) => ({ + key: screenshot.key, + postUrl: `https://api.s3.dev/upload/${screenshot.key}`, + fields: { + key: screenshot.key, + "Content-Type": screenshot.contentType, + }, + })), + pwTraces: (body.pwTraceKeys ?? []).map((key) => ({ key, - putUrl: `https://api.s3.dev/upload/${key}`, + postUrl: `https://api.s3.dev/upload/${key}`, + fields: { + key, + "Content-Type": "application/zip", + }, })), }); }); diff --git a/packages/core/mocks/handlers/uploadScreenshot.ts b/packages/core/mocks/handlers/uploadScreenshot.ts index b480c541..e20b50dd 100644 --- a/packages/core/mocks/handlers/uploadScreenshot.ts +++ b/packages/core/mocks/handlers/uploadScreenshot.ts @@ -1,8 +1,12 @@ import { http } from "msw"; -export const uploadScreenshot = http.put( +export const uploadScreenshot = http.post( "https://api.s3.dev/upload/*", - async () => { - return new Response(null, { status: 201 }); + async ({ request }) => { + const formData = await request.formData(); + if (!formData.has("file")) { + return new Response(null, { status: 400 }); + } + return new Response(null, { status: 204 }); }, ); diff --git a/packages/core/src/s3.test.ts b/packages/core/src/s3.test.ts index b9ec9568..96d6851e 100644 --- a/packages/core/src/s3.test.ts +++ b/packages/core/src/s3.test.ts @@ -2,8 +2,9 @@ import { describe, it } from "vitest"; import { join } from "node:path"; // import { fileURLToPath } from "node:url"; -import { setupMockServer } from "../mocks/server"; -import { uploadFile } from "./s3"; +import { server, setupMockServer } from "../mocks/server"; +import { uploadFile, uploadFileWithPresignedPost } from "./s3"; +import { http } from "msw"; // const __dirname = fileURLToPath(new URL(".", import.meta.url)); @@ -11,6 +12,7 @@ setupMockServer(); describe("#upload", () => { it("uploads", async () => { + mockPutUpload(); await uploadFile({ path: join(__dirname, "../../../__fixtures__/screenshots/penelope.png"), url: "https://api.s3.dev/upload/123", @@ -19,10 +21,31 @@ describe("#upload", () => { }); it("uploads big images", async () => { + mockPutUpload(); await uploadFile({ path: join(__dirname, "../../../__fixtures__/png-10mb.png"), url: "https://api.s3.dev/upload/123", contentType: "image/png", }); }); + + it("uploads with presigned POST fields", async () => { + await uploadFileWithPresignedPost({ + path: join(__dirname, "../../../__fixtures__/screenshots/penelope.png"), + url: "https://api.s3.dev/upload/123", + contentType: "image/png", + fields: { + key: "123", + "Content-Type": "image/png", + }, + }); + }); }); + +function mockPutUpload() { + server.use( + http.put("https://api.s3.dev/upload/*", async () => { + return new Response(null, { status: 201 }); + }), + ); +} diff --git a/packages/core/src/s3.ts b/packages/core/src/s3.ts index 4e6a2351..eb4d04b8 100644 --- a/packages/core/src/s3.ts +++ b/packages/core/src/s3.ts @@ -1,4 +1,5 @@ import { readFile } from "node:fs/promises"; +import { basename } from "node:path"; interface UploadInput { url: string; @@ -6,6 +7,10 @@ interface UploadInput { contentType: string; } +interface PresignedPostUploadInput extends UploadInput { + fields: Record; +} + export async function uploadFile(input: UploadInput): Promise { const file = await readFile(input.path); const response = await fetch(input.url, { @@ -22,3 +27,29 @@ export async function uploadFile(input: UploadInput): Promise { ); } } + +export async function uploadFileWithPresignedPost( + input: PresignedPostUploadInput, +): Promise { + const file = await readFile(input.path); + const formData = new FormData(); + for (const [key, value] of Object.entries(input.fields)) { + formData.append(key, value); + } + formData.append( + "file", + new Blob([new Uint8Array(file)], { type: input.contentType }), + basename(input.path), + ); + + const response = await fetch(input.url, { + method: "POST", + signal: AbortSignal.timeout(30_000), + body: formData, + }); + if (!response.ok) { + throw new Error( + `Failed to upload file to ${input.url}: ${response.status} ${response.statusText}`, + ); + } +} diff --git a/packages/core/src/skip.ts b/packages/core/src/skip.ts index d2becd8f..117e734f 100644 --- a/packages/core/src/skip.ts +++ b/packages/core/src/skip.ts @@ -49,7 +49,7 @@ export async function skip( runId: config.runId, runAttempt: config.runAttempt, skipped: true, - screenshotKeys: [], + screenshots: [], pwTraceKeys: [], parentCommits: [], }, diff --git a/packages/core/src/upload.test.ts b/packages/core/src/upload.test.ts index b7a1889d..d00ce4e1 100644 --- a/packages/core/src/upload.test.ts +++ b/packages/core/src/upload.test.ts @@ -85,6 +85,82 @@ describe("#upload", () => { }); }); + it("uses secure uploads", () => { + return server.boundary(async () => { + const received: { + createBuildBody?: { + screenshots?: { key: string; contentType: string }[]; + screenshotKeys?: string[]; + }; + upload?: { + keys: string[]; + contentType: FormDataEntryValue | null; + policy: FormDataEntryValue | null; + file: FormDataEntryValue | null; + }; + } = {}; + + server.use( + http.post("https://api.argos-ci.dev/builds", async ({ request }) => { + received.createBuildBody = (await request.json()) as { + screenshots?: { key: string; contentType: string }[]; + screenshotKeys?: string[]; + }; + const screenshot = received.createBuildBody.screenshots?.[0]; + if (!screenshot) { + return HttpResponse.json({ error: "Bad Request" }, { status: 400 }); + } + return HttpResponse.json({ + build: { id: "123", url: "https://app.argos-ci.dev/builds/123" }, + screenshots: [ + { + key: screenshot.key, + postUrl: `https://api.s3.dev/upload/${screenshot.key}`, + fields: { + key: screenshot.key, + "Content-Type": screenshot.contentType, + policy: "test-policy", + }, + }, + ], + pwTraces: [], + }); + }), + http.post("https://api.s3.dev/upload/*", async ({ request }) => { + const formData = await request.formData(); + received.upload = { + keys: Array.from(formData.keys()), + contentType: formData.get("Content-Type"), + policy: formData.get("policy"), + file: formData.get("file"), + }; + return new Response(null, { status: 204 }); + }), + ); + + await upload({ + branch: "main", + apiBaseUrl: "https://api.argos-ci.dev", + root: join(__dirname, "../../../__fixtures__/screenshots"), + files: ["penelope.png"], + commit: "f16f980bd17cccfa93a1ae7766727e67950773d0", + token: "92d832e0d22ab113c8979d73a87a11130eaa24a9", + }); + + expect(received.createBuildBody?.screenshotKeys).toBeUndefined(); + expect(received.createBuildBody?.screenshots).toEqual([ + { + key: expect.stringMatching(/^[A-Fa-f0-9]{64}$/), + contentType: "image/png", + }, + ]); + expect(received.upload?.contentType).toBe("image/png"); + expect(received.upload?.policy).toBe("test-policy"); + expect(received.upload?.keys.at(-1)).toBe("file"); + expect(received.upload?.file).toBeInstanceOf(Blob); + })(); + }); + it("retries", () => { return server.boundary(async () => { let reqCount = 0; diff --git a/packages/core/src/upload.ts b/packages/core/src/upload.ts index 1a5eb354..25dca5d8 100644 --- a/packages/core/src/upload.ts +++ b/packages/core/src/upload.ts @@ -5,7 +5,7 @@ import { discoverSnapshots } from "./discovery"; import { optimizeScreenshot } from "./optimize"; import { hashFile } from "./hashing"; import { resolveArgosToken } from "./auth"; -import { uploadFile } from "./s3"; +import { uploadFileWithPresignedPost } from "./s3"; import { debug, debugTime, debugTimeEnd } from "./debug"; import { chunk } from "./util/chunk"; import { @@ -23,7 +23,13 @@ import { skip } from "./skip"; */ const CHUNK_SIZE = 10; +const PLAYWRIGHT_TRACE_CONTENT_TYPE = "application/zip"; + type BuildMetadata = ArgosAPISchema.components["schemas"]["BuildMetadata"]; +type CreateBuildResponse = + ArgosAPISchema.operations["createBuild"]["responses"][201]["content"]["application/json"]; +type CreateBuildUpload = CreateBuildResponse["screenshots"][number]; +type SecureUploadFileInput = Parameters[0]; export interface UploadParameters { /** @@ -307,18 +313,19 @@ export async function upload(params: UploadParameters): Promise<{ // Create build debug("Creating build"); - const [pwTraceKeys, snapshotKeys] = snapshots.reduce( - ([pwTraceKeys, snapshotKeys], snapshot) => { - if (snapshot.pwTrace && !pwTraceKeys.includes(snapshot.pwTrace.hash)) { - pwTraceKeys.push(snapshot.pwTrace.hash); - } - if (!snapshotKeys.includes(snapshot.hash)) { - snapshotKeys.push(snapshot.hash); - } - return [pwTraceKeys, snapshotKeys]; - }, - [[], []] as [string[], string[]], - ); + const pwTraceKeys: string[] = []; + const screenshots: { key: string; contentType: string }[] = []; + for (const snapshot of snapshots) { + if (snapshot.pwTrace && !pwTraceKeys.includes(snapshot.pwTrace.hash)) { + pwTraceKeys.push(snapshot.pwTrace.hash); + } + if (!screenshots.some((item) => item.key === snapshot.hash)) { + screenshots.push({ + key: snapshot.hash, + contentType: snapshot.contentType, + }); + } + } const createBuildResponse = await apiClient.POST("/builds", { body: { @@ -328,7 +335,7 @@ export async function upload(params: UploadParameters): Promise<{ mode: config.mode, parallel: config.parallel, parallelNonce: config.parallelNonce, - screenshotKeys: snapshotKeys, + screenshots, pwTraceKeys, prNumber: config.prNumber, prHeadCommit: config.prHeadCommit, @@ -354,29 +361,29 @@ export async function upload(params: UploadParameters): Promise<{ debug("Got uploads url", result); const uploadFiles = [ - ...result.screenshots.map(({ key, putUrl }) => { - const snapshot = snapshots.find((s) => s.hash === key); + ...result.screenshots.map((upload) => { + const snapshot = snapshots.find((s) => s.hash === upload.key); if (!snapshot) { - throw new Error(`Invariant: snapshot with hash ${key} not found`); + throw new Error( + `Invariant: snapshot with hash ${upload.key} not found`, + ); } - return { - url: putUrl, + return createSecureUploadFileInput(upload, { path: snapshot.optimizedPath, contentType: snapshot.contentType, - }; + }); }), - ...(result.pwTraces?.map(({ key, putUrl }) => { + ...(result.pwTraces?.map((upload) => { const snapshot = snapshots.find( - (s) => s.pwTrace && s.pwTrace.hash === key, + (s) => s.pwTrace && s.pwTrace.hash === upload.key, ); if (!snapshot || !snapshot.pwTrace) { - throw new Error(`Invariant: trace with ${key} not found`); + throw new Error(`Invariant: trace with ${upload.key} not found`); } - return { - url: putUrl, + return createSecureUploadFileInput(upload, { path: snapshot.pwTrace.path, - contentType: "application/json", - }; + contentType: PLAYWRIGHT_TRACE_CONTENT_TYPE, + }); }) ?? []), ]; @@ -416,9 +423,7 @@ export async function upload(params: UploadParameters): Promise<{ return { build: uploadBuildResponse.data.build, screenshots: snapshots }; } -async function uploadFilesToS3( - files: { url: string; path: string; contentType: string }[], -) { +async function uploadFilesToS3(files: SecureUploadFileInput[]) { debug(`Split files in chunks of ${CHUNK_SIZE}`); const chunks = chunk(files, CHUNK_SIZE); @@ -433,19 +438,26 @@ async function uploadFilesToS3( if (!chunk) { throw new Error(`Invariant: chunk ${i} is empty`); } - await Promise.all( - chunk.map(async ({ url, path, contentType }) => { - await uploadFile({ - url, - path, - contentType, - }); - }), - ); + await Promise.all(chunk.map((file) => uploadFileWithPresignedPost(file))); debugTimeEnd(timeLabel); } } +function createSecureUploadFileInput( + upload: CreateBuildUpload, + file: { path: string; contentType: string }, +): SecureUploadFileInput { + if (!("postUrl" in upload)) { + throw new Error(`Invariant: expected secure upload for ${upload.key}`); + } + + return { + url: upload.postUrl, + fields: upload.fields, + ...file, + }; +} + /** * Format the preview URL. */