Merge pull request #2777 from ryanfowler/fix/bugs-security-cleanup

fix: correctness bugs, security hardening, and code cleanup

Author: ryanfowler

lib/fetch.ts

@@ -64,15 +64,18 @@ export class Client {
     let redirectCount = 0;
 
     while (true) {
-      // Validate host before each request (initial + redirects)
+      // Validate host before each request (initial + redirects).
+      // Note: There is a TOCTOU window between validateHost and makeRequest
+      // where DNS could resolve to a different IP (DNS rebinding). Fully fixing
+      // this requires IP-pinning at the socket level, which is complex with TLS.
       await this.validateHost(currentUrl);
 
       const res = await this.makeRequest(currentUrl);
 
       // Check for redirect responses
       if (res.status >= 300 && res.status < 400) {
         if (redirectCount >= MAX_REDIRECTS) {
-          throw new HttpError(400, "fetch: too many redirects");
+          throw new HttpError(502, "fetch: too many redirects");
         }
 
         const location = res.headers.get("location");
@@ -99,7 +102,7 @@ export class Client {
         throw new HttpError(502, `fetch: received status ${res.status}`);
       }
       if (!res.body) {
-        throw new HttpError(400, "fetch: no body in response");
+        throw new HttpError(502, "fetch: no body in response");
       }
 
       return res;
@@ -115,7 +118,7 @@ export class Client {
         redirect: "manual",
       });
     } catch {
-      throw new HttpError(400, "fetch: unable to make request");
+      throw new HttpError(502, "fetch: unable to make request");
     }
 
     return res;

lib/image.test.ts

@@ -400,6 +400,18 @@ describe("ImageEngine", () => {
       ).rejects.toThrow(/encoding type svg is not supported/);
     });
 
+    test("honors effort: 0 for WebP", async () => {
+      // effort: 0 is valid for WebP (range 0-6), must not be replaced by preset default
+      const result = await engine.perform({
+        data: jpegBuffer,
+        format: ImageType.Webp,
+        effort: 0,
+      });
+
+      expect(result.format).toBe(ImageType.Webp);
+      expect(detectImageFormat(result.data)).toBe(ImageType.Webp);
+    });
+
     test("handles lossless WebP", async () => {
       const result = await engine.perform({
         data: pngBuffer,

lib/image.ts

@@ -166,8 +166,8 @@ export class ImageEngine {
 
     return {
       format: input,
-      width: meta.autoOrient?.width || meta.width,
-      height: meta.autoOrient?.height || meta.height,
+      width: meta.autoOrient?.width ?? meta.width,
+      height: meta.autoOrient?.height ?? meta.height,
       size: ops.data.length,
       space: meta.space,
       channels: meta.channels,
@@ -198,8 +198,8 @@ function applyFormat(img: sharp.Sharp, ops: ImageOptions): sharp.Sharp {
   switch (ops.format) {
     case ImageType.Avif:
       return img.avif({
-        quality: ops.quality || preset.quality,
-        effort: ops.effort || preset.effort,
+        quality: ops.quality ?? preset.quality,
+        effort: ops.effort ?? preset.effort,
         chromaSubsampling: preset.chromaSubsampling,
         lossless: ops.lossless,
       });
@@ -208,23 +208,23 @@ function applyFormat(img: sharp.Sharp, ops: ImageOptions): sharp.Sharp {
     case ImageType.Heic:
       return img.heif({
         compression: "hevc",
-        quality: ops.quality || preset.quality,
-        effort: ops.effort || preset.effort,
+        quality: ops.quality ?? preset.quality,
+        effort: ops.effort ?? preset.effort,
         chromaSubsampling: preset.chromaSubsampling,
         lossless: ops.lossless,
       });
     case ImageType.Jpeg:
       return img.jpeg({
-        quality: ops.quality || preset.quality,
-        progressive: ops.progressive || preset.progressive,
+        quality: ops.quality ?? preset.quality,
+        progressive: ops.progressive ?? preset.progressive,
         chromaSubsampling: preset.chromaSubsampling,
         mozjpeg: preset.mozjpeg,
         optimiseCoding: preset.optimiseCoding,
       });
     case ImageType.JpegXL:
       return img.jxl({
-        quality: ops.quality || preset.quality,
-        effort: ops.effort || preset.effort,
+        quality: ops.quality ?? preset.quality,
+        effort: ops.effort ?? preset.effort,
         decodingTier: preset.decodingTier,
         lossless: ops.lossless,
       });
@@ -245,15 +245,15 @@ function applyFormat(img: sharp.Sharp, ops: ImageOptions): sharp.Sharp {
       throw new HttpError(400, "image: encoding type svg is not supported");
     case ImageType.Tiff:
       return img.tiff({
-        quality: ops.quality || preset.quality,
+        quality: ops.quality ?? preset.quality,
         compression: preset.compression,
         predictor: preset.predictor,
       });
     case ImageType.Webp:
       return img.webp({
-        quality: ops.quality || preset.quality,
+        quality: ops.quality ?? preset.quality,
         lossless: ops.lossless,
-        effort: ops.effort || preset.effort,
+        effort: ops.effort ?? preset.effort,
         smartSubsample: preset.smartSubsample,
         smartDeblock: preset.smartDeblock,
         alphaQuality: preset.alphaQuality,
@@ -316,21 +316,6 @@ export function detectImageFormat(buf: Uint8Array): ImageType {
     return ImageType.Tiff;
   }
 
-  // BMP
-  // if (buf[0] === 0x42 && buf[1] === 0x4d) {
-  //   return ImageType.Bmp;
-  // }
-
-  // ICO
-  // if (buf[0] === 0x00 && buf[1] === 0x00 && buf[2] === 0x01 && buf[3] === 0x00) {
-  //   return ImageType.Ico;
-  // }
-
-  // PSD
-  // if (buf[0] === 0x38 && buf[1] === 0x42 && buf[2] === 0x50 && buf[3] === 0x53) {
-  //   return ImageType.Psd;
-  // }
-
   // JPEG XL
   if (
     (buf[0] === 0xff && buf[1] === 0x0a) ||
@@ -358,8 +343,6 @@ export function detectImageFormat(buf: Uint8Array): ImageType {
     if (brand.startsWith("heic")) return ImageType.Heic;
     if (brand.startsWith("heix")) return ImageType.Heic;
     if (brand.startsWith("hevc")) return ImageType.Heic;
-    // if (brand.startsWith("heif")) return ImageType.HEIF;
-    // if (brand.startsWith("mif1")) return ImageType.HEIF;
   }
 
   // PDF

lib/pipeline.test.ts

@@ -1,7 +1,7 @@
 import { describe, test, expect, mock } from "bun:test";
 import { PipelineExecutor } from "./pipeline.ts";
 import { HttpError, type PipelineConfig, type S3Config, ImageType } from "./types.ts";
-import { getMimetype } from "./validation.ts";
+import { getMimetype, validateContentType } from "./validation.ts";
 
 // Mock S3 config for testing
 const mockS3Config: S3Config = {
@@ -72,6 +72,48 @@ describe("getMimetype", () => {
   });
 });
 
+describe("validateContentType", () => {
+  test("accepts image/jpeg", () => {
+    expect(validateContentType("image/jpeg", "test")).toBe("image/jpeg");
+  });
+
+  test("accepts image/png", () => {
+    expect(validateContentType("image/png", "test")).toBe("image/png");
+  });
+
+  test("accepts image/webp", () => {
+    expect(validateContentType("image/webp", "test")).toBe("image/webp");
+  });
+
+  test("accepts application/pdf", () => {
+    expect(validateContentType("application/pdf", "test")).toBe("application/pdf");
+  });
+
+  test("accepts application/octet-stream", () => {
+    expect(validateContentType("application/octet-stream", "test")).toBe(
+      "application/octet-stream",
+    );
+  });
+
+  test("rejects text/html", () => {
+    expect(() => validateContentType("text/html", "test")).toThrow(
+      "content type 'text/html' is not allowed",
+    );
+  });
+
+  test("rejects text/javascript", () => {
+    expect(() => validateContentType("text/javascript", "test")).toThrow(
+      "content type 'text/javascript' is not allowed",
+    );
+  });
+
+  test("rejects arbitrary content type", () => {
+    expect(() => validateContentType("application/json", "test")).toThrow(
+      "content type 'application/json' is not allowed",
+    );
+  });
+});
+
 describe("PipelineExecutor", () => {
   // Create mock dependencies
   const createMockEngine = () => ({

lib/pipeline.ts

@@ -13,6 +13,7 @@ import {
 } from "./types.ts";
 import {
   getMimetype,
+  validateContentType,
   validateBlurStrict,
   validateBooleanStrict,
   validateDimensionStrict,
@@ -229,7 +230,9 @@ export class PipelineExecutor {
       });
 
       // Upload to S3
-      const contentType = task.output.contentType || getMimetype(result.format);
+      const contentType = task.output.contentType
+        ? validateContentType(task.output.contentType, `task '${task.id}'`)
+        : getMimetype(result.format);
       await uploadToS3(
         result.data,
         task.output.bucket,

lib/s3.ts

@@ -52,7 +52,7 @@ export async function uploadToS3(
 ): Promise<void> {
   const file = client.file(key, {
     bucket,
-    acl: options.acl as undefined, // Hack the type system
+    ...(options.acl != null && { acl: options.acl as "private" }),
     type: options.contentType,
   });
 

lib/validation.ts

@@ -99,6 +99,30 @@ export function normalizeFormat(value: string): string {
   return v;
 }
 
+// Allowed content types for S3 uploads
+const ALLOWED_CONTENT_TYPES = new Set([
+  "image/avif",
+  "image/gif",
+  "image/heic",
+  "image/jpeg",
+  "image/jxl",
+  "image/png",
+  "image/svg+xml",
+  "image/tiff",
+  "image/webp",
+  "application/pdf",
+  "application/octet-stream",
+]);
+
+// Validate that a content type is an allowed image MIME type.
+// Returns the validated content type, or throws HttpError for disallowed types.
+export function validateContentType(contentType: string, prefix: string): string {
+  if (!ALLOWED_CONTENT_TYPES.has(contentType)) {
+    throw new HttpError(400, `${prefix}: content type '${contentType}' is not allowed`);
+  }
+  return contentType;
+}
+
 // Get mime type for image format
 export function getMimetype(format: ImageType): string {
   switch (format) {