diff --git a/.changeset/big-laws-jam.md b/.changeset/big-laws-jam.md new file mode 100644 index 000000000..3bc4808d8 --- /dev/null +++ b/.changeset/big-laws-jam.md @@ -0,0 +1,5 @@ +--- +"@opennextjs/aws": major +--- + +Improved error handling in image optimization adapter for S3 ListBucket permission errors diff --git a/packages/open-next/src/adapters/image-optimization-adapter.ts b/packages/open-next/src/adapters/image-optimization-adapter.ts index 1058e4dd0..9ecce6d21 100644 --- a/packages/open-next/src/adapters/image-optimization-adapter.ts +++ b/packages/open-next/src/adapters/image-optimization-adapter.ts @@ -28,6 +28,7 @@ import { emptyReadableStream, toReadableStream } from "utils/stream.js"; import type { OpenNextHandlerOptions } from "types/overrides.js"; import { createGenericHandler } from "../core/createGenericHandler.js"; import { resolveImageLoader } from "../core/resolve.js"; +import { IgnorableError } from "../utils/error.js"; import { debug, error } from "./logger.js"; import { optimizeImage } from "./plugins/image-optimization/image-optimization.js"; import { setNodeEnv } from "./util.js"; @@ -114,10 +115,19 @@ export async function defaultHandler( ); return buildSuccessResponse(result, options?.streamCreator, etag); } catch (e: any) { - error("Failed to optimize image", e); + // All image-related errors should be treated as client errors + // Extract status code from error or default to 400 Bad Request + const statusCode = e.statusCode || 400; + const errorMessage = e.message || "Failed to process image request"; + + // Create an IgnorableError for proper monitoring classification + const clientError = new IgnorableError(errorMessage, statusCode); + error("Failed to optimize image", clientError); + return buildFailureResponse( - "Internal server error", + errorMessage, options?.streamCreator, + statusCode, // Use the appropriate status code (preserving original when available) ); } } @@ -238,50 +248,140 @@ async function downloadHandler( // directly. debug("downloadHandler url", url); - // Reads the output from the Writable and writes to the response - const pipeRes = (w: Writable, res: ServerResponse) => { - w.pipe(res) + /** + * Helper function to handle image errors consistently with appropriate response + * @param e The error object + * @param res The server response object + * @param isInternalImage Whether the error is from an internal image (S3) or external image + */ + function handleImageError( + e: any, + res: ServerResponse, + isInternalImage: boolean, + ) { + let originalStatus = e.statusCode || e.$metadata?.httpStatusCode || 500; + let message = e.message || "Failed to process image request"; + + // Special handling for S3 ListBucket permission errors + // AWS SDK v3 nests error details deeply within the error object + const isListBucketError = + (message.includes("s3:ListBucket") && message.includes("AccessDenied")) || + e.error?.message?.includes("s3:ListBucket") || + (e.Code === "AccessDenied" && e.Message?.includes("s3:ListBucket")); + + if (isListBucketError) { + message = "Image not found or access denied"; + // For S3 ListBucket errors, ensure we're using 403 (the actual AWS error) + if (originalStatus === 500 && e.$metadata?.httpStatusCode === 403) { + originalStatus = 403; + } + + // Log using IgnorableError to classify as client error + const clientError = new IgnorableError(message, originalStatus); + error("S3 ListBucket permission error", clientError); + } else { + // Log all other errors as client errors + const clientError = new IgnorableError(message, originalStatus); + error("Failed to process image", clientError); + } + + // For external images, throw if not ListBucket error + // Next.js will preserve the status code for external images + if (!isInternalImage && !isListBucketError) { + const formattedError = new Error(message); + // @ts-ignore: Add statusCode property to Error + formattedError.statusCode = originalStatus >= 500 ? 400 : originalStatus; + throw formattedError; + } + + // Different handling for internal vs external images + const finalStatus = originalStatus >= 500 ? 400 : originalStatus; + res.statusCode = finalStatus; + + // For internal images, we want to trigger Next.js's "internal response invalid" message + if (isInternalImage) { + // For internal images, don't set Content-Type to trigger Next.js's default error handling + // This should result in "url parameter is valid but internal response is invalid" + + // Still include error details in headers for debugging only + const errorMessage = isListBucketError ? "Access denied" : message; + res.setHeader("x-nextjs-internal-error", errorMessage); + res.end(); + } else { + // For external images, maintain existing behavior with text/plain + res.setHeader("Content-Type", "text/plain"); + + if (isListBucketError) { + res.end("Access denied"); + } else { + res.end(message); + } + } + } + + // Pipes data from a writable stream to the server response + function pipeStream( + stream: Writable, + res: ServerResponse, + isInternalImage: boolean, + ) { + stream + .pipe(res) .once("close", () => { res.statusCode = 200; res.end(); }) .once("error", (err) => { - error("Failed to get image", err); - res.statusCode = 400; - res.end(); + error("Error streaming image data", err); + handleImageError(err, res, isInternalImage); }); - }; + } + // Main handler logic with clearer error paths try { - // Case 1: remote image URL => download the image from the URL + // EXTERNAL IMAGE HANDLING if (url.href.toLowerCase().match(/^https?:\/\//)) { - pipeRes(https.get(url), res); + try { + pipeStream(https.get(url), res, false); + } catch (e: any) { + handleImageError(e, res, false); + } + return; } - // Case 2: local image => download the image from S3 - else { - // Download image from S3 - // note: S3 expects keys without leading `/` + // INTERNAL IMAGE HANDLING (S3) + try { const response = await loader.load(url.href); + // Handle empty response body if (!response.body) { - throw new Error("Empty response body from the S3 request."); - } + const message = "Empty response body from the S3 request."; + const clientError = new IgnorableError(message, 400); + error("Empty response from S3", clientError); - // @ts-ignore - pipeRes(response.body, res); + res.statusCode = 400; + res.setHeader("Content-Type", "text/plain"); + res.end(message); + return; + } - // Respect the bucket file's content-type and cache-control - // imageOptimizer will use this to set the results.maxAge + // Set headers from the response if (response.contentType) { res.setHeader("Content-Type", response.contentType); } if (response.cacheControl) { res.setHeader("Cache-Control", response.cacheControl); } + + // Stream the image to the client + // @ts-ignore + pipeStream(response.body, res, true); + } catch (e: any) { + // Direct response for all internal image errors + handleImageError(e, res, true); } } catch (e: any) { - error("Failed to download image", e); - throw e; + // Catch-all for any unexpected errors + handleImageError(e, res, true); } } diff --git a/packages/open-next/src/utils/error.ts b/packages/open-next/src/utils/error.ts index c1e8dc9c0..352dea36a 100644 --- a/packages/open-next/src/utils/error.ts +++ b/packages/open-next/src/utils/error.ts @@ -3,6 +3,7 @@ export interface BaseOpenNextError { readonly canIgnore: boolean; // 0 - debug, 1 - warn, 2 - error readonly logLevel: 0 | 1 | 2; + readonly statusCode?: number; } // This is an error that can be totally ignored @@ -11,9 +12,12 @@ export class IgnorableError extends Error implements BaseOpenNextError { readonly __openNextInternal = true; readonly canIgnore = true; readonly logLevel = 0; - constructor(message: string) { + readonly statusCode: number; + + constructor(message: string, statusCode = 400) { super(message); this.name = "IgnorableError"; + this.statusCode = statusCode; } } @@ -23,9 +27,12 @@ export class RecoverableError extends Error implements BaseOpenNextError { readonly __openNextInternal = true; readonly canIgnore = true; readonly logLevel = 1; - constructor(message: string) { + readonly statusCode: number; + + constructor(message: string, statusCode = 400) { super(message); this.name = "RecoverableError"; + this.statusCode = statusCode; } } @@ -34,9 +41,12 @@ export class FatalError extends Error implements BaseOpenNextError { readonly __openNextInternal = true; readonly canIgnore = false; readonly logLevel = 2; - constructor(message: string) { + readonly statusCode: number; + + constructor(message: string, statusCode = 500) { super(message); this.name = "FatalError"; + this.statusCode = statusCode; } }