fix: bound image attachment downloads (#1625)

src/github/utils/image-downloader.ts

@@ -34,6 +34,8 @@ const SIGNED_URL_HOST = "private-user-images.githubusercontent.com";
 const SIGNED_URL_PATH_REGEX =
   /^\/[^/]+\/[^/]*-([0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})(?:\.[a-z0-9]+)?$/i;
 
+const DEFAULT_IMAGE_DOWNLOAD_TIMEOUT_MS = 30_000;
+
 function extractSignedUrlAssetGuid(signedUrl: string): string | undefined {
   let parsed: URL;
   try {
@@ -85,13 +87,19 @@ export type CommentWithImages =
   | IssueBody
   | PullRequestBody;
 
+type ImageDownloadOptions = {
+  timeoutMs?: number;
+};
+
 export async function downloadCommentImages(
   octokits: Octokits,
   owner: string,
   repo: string,
   comments: CommentWithImages[],
+  options: ImageDownloadOptions = {},
 ): Promise<Map<string, string>> {
   const urlToPathMap = new Map<string, string>();
+  const timeoutMs = options.timeoutMs ?? DEFAULT_IMAGE_DOWNLOAD_TIMEOUT_MS;
   const downloadsDir = "/tmp/github-images";
 
   await fs.mkdir(downloadsDir, { recursive: true });
@@ -241,15 +249,7 @@ export async function downloadCommentImages(
         try {
           console.log(`Downloading ${originalUrl}...`);
 
-          const imageResponse = await fetch(signedUrl);
+          const buffer = await fetchImage(signedUrl, timeoutMs);
-          if (!imageResponse.ok) {
-            throw new Error(
-              `HTTP ${imageResponse.status}: ${imageResponse.statusText}`,
-            );
-          }
-
-          const arrayBuffer = await imageResponse.arrayBuffer();
-          const buffer = Buffer.from(arrayBuffer);
 
           // GitHub user-attachment URLs (/user-attachments/assets/<uuid>) carry
           // no file extension, so the URL-based guess silently falls back to
@@ -289,6 +289,37 @@ export async function downloadCommentImages(
   return urlToPathMap;
 }
 
+async function fetchImage(url: string, timeoutMs: number): Promise<Buffer> {
+  const controller = new AbortController();
+  let timeoutHandle: ReturnType<typeof setTimeout> | undefined;
+  const timeoutPromise = new Promise<never>((_, reject) => {
+    timeoutHandle = setTimeout(() => {
+      controller.abort();
+      reject(new Error(`Image download timed out after ${timeoutMs}ms`));
+    }, timeoutMs);
+  });
+
+  try {
+    const response = await Promise.race([
+      fetch(url, { signal: controller.signal }),
+      timeoutPromise,
+    ]);
+    if (!response.ok) {
+      throw new Error(`HTTP ${response.status}: ${response.statusText}`);
+    }
+
+    const arrayBuffer = await Promise.race([
+      response.arrayBuffer(),
+      timeoutPromise,
+    ]);
+    return Buffer.from(arrayBuffer);
+  } finally {
+    if (timeoutHandle !== undefined) {
+      clearTimeout(timeoutHandle);
+    }
+  }
+}
+
 function getImageExtension(url: string): string {
   const urlParts = url.split("/");
   const filename = urlParts[urlParts.length - 1];

test/image-downloader.test.ts

@@ -148,7 +148,9 @@ describe("downloadCommentImages", () => {
       mediaType: { format: "full+json" },
     });
 
-    expect(fetchSpy).toHaveBeenCalledWith(signedUrl);
+    expect(fetchSpy).toHaveBeenCalledWith(signedUrl, {
+      signal: expect.any(AbortSignal),
+    });
     expect(fsWriteFileSpy).toHaveBeenCalledWith(
       "/tmp/github-images/image-1704067200000-0.png",
       Buffer.from(mockArrayBuffer),
@@ -481,8 +483,12 @@ describe("downloadCommentImages", () => {
     );
 
     expect(fetchSpy).toHaveBeenCalledTimes(2);
-    expect(fetchSpy).toHaveBeenNthCalledWith(1, signedUrl1);
+    expect(fetchSpy).toHaveBeenNthCalledWith(1, signedUrl1, {
-    expect(fetchSpy).toHaveBeenNthCalledWith(2, signedUrl2);
+      signal: expect.any(AbortSignal),
+    });
+    expect(fetchSpy).toHaveBeenNthCalledWith(2, signedUrl2, {
+      signal: expect.any(AbortSignal),
+    });
     expect(result.get(imageUrl1)).toBe(
       "/tmp/github-images/image-1704067200000-0.png",
     );
@@ -523,7 +529,9 @@ describe("downloadCommentImages", () => {
       comments,
     );
 
-    expect(fetchSpy).toHaveBeenCalledWith(signedUrl);
+    expect(fetchSpy).toHaveBeenCalledWith(signedUrl, {
+      signal: expect.any(AbortSignal),
+    });
     expect(result.get(imageUrl)).toBe(
       "/tmp/github-images/image-1704067200000-0.png",
     );
@@ -766,6 +774,95 @@ describe("downloadCommentImages", () => {
     );
   });
 
+  test("should skip an image when the fetch times out", async () => {
+    const mockOctokit = createMockOctokit();
+    const imageUrl = assetUrl(GUID_1);
+    const signedUrl = signedUrlFor(GUID_1, ".png");
+    let signal: AbortSignal | null | undefined;
+
+    // @ts-expect-error Mock implementation doesn't match full type signature
+    mockOctokit.rest.issues.getComment = jest.fn().mockResolvedValue({
+      data: {
+        body_html: `<img src="${signedUrl}">`,
+      },
+    });
+
+    fetchSpy = spyOn(global, "fetch");
+    fetchSpy.mockImplementation((_input: unknown, init?: RequestInit) => {
+      signal = init?.signal;
+      return new Promise<Response>(() => {});
+    });
+
+    const result = await downloadCommentImages(
+      mockOctokit,
+      "owner",
+      "repo",
+      [
+        {
+          type: "issue_comment",
+          id: "445",
+          body: `Stalled image: ![stalled](${imageUrl})`,
+        },
+      ],
+      { timeoutMs: 5 },
+    );
+
+    expect(result.size).toBe(0);
+    expect(signal?.aborted).toBe(true);
+    expect(consoleErrorSpy).toHaveBeenCalledWith(
+      expect.stringContaining("Failed to download"),
+      expect.objectContaining({
+        message: "Image download timed out after 5ms",
+      }),
+    );
+  });
+
+  test("should time out while reading a response body", async () => {
+    const mockOctokit = createMockOctokit();
+    const imageUrl = assetUrl(GUID_1);
+    const signedUrl = signedUrlFor(GUID_1, ".png");
+    let signal: AbortSignal | null | undefined;
+
+    // @ts-expect-error Mock implementation doesn't match full type signature
+    mockOctokit.rest.issues.getComment = jest.fn().mockResolvedValue({
+      data: {
+        body_html: `<img src="${signedUrl}">`,
+      },
+    });
+
+    fetchSpy = spyOn(global, "fetch");
+    fetchSpy.mockImplementation((_input: unknown, init?: RequestInit) => {
+      signal = init?.signal;
+      return Promise.resolve({
+        ok: true,
+        arrayBuffer: () => new Promise<ArrayBuffer>(() => {}),
+      } as Response);
+    });
+
+    const result = await downloadCommentImages(
+      mockOctokit,
+      "owner",
+      "repo",
+      [
+        {
+          type: "issue_comment",
+          id: "446",
+          body: `Stalled body: ![stalled](${imageUrl})`,
+        },
+      ],
+      { timeoutMs: 5 },
+    );
+
+    expect(result.size).toBe(0);
+    expect(signal?.aborted).toBe(true);
+    expect(consoleErrorSpy).toHaveBeenCalledWith(
+      expect.stringContaining("Failed to download"),
+      expect.objectContaining({
+        message: "Image download timed out after 5ms",
+      }),
+    );
+  });
+
   test("should handle API errors gracefully", async () => {
     const mockOctokit = createMockOctokit();
     const imageUrl = assetUrl(GUID_1);
@@ -936,7 +1033,9 @@ describe("downloadCommentImages", () => {
       mediaType: { format: "full+json" },
     });
 
-    expect(fetchSpy).toHaveBeenCalledWith(signedUrl);
+    expect(fetchSpy).toHaveBeenCalledWith(signedUrl, {
+      signal: expect.any(AbortSignal),
+    });
     expect(fsWriteFileSpy).toHaveBeenCalledWith(
       "/tmp/github-images/image-1704067200000-0.png",
       Buffer.from(mockArrayBuffer),