Image for analysis now validates analysis ID and ensures the image is a real image

Author: JudahGabriel

apps/pwabuilder/Controllers/ImagesController.cs

@@ -1,10 +1,9 @@
-using System.Drawing;
-using System.Text.Json;
 using Microsoft.AspNetCore.Mvc;
 using PuppeteerSharp;
 using PWABuilder.Common;
 using PWABuilder.Models;
 using PWABuilder.Services;
+using PWABuilder.Validations.Services;
 
 namespace PWABuilder.Controllers;
 
@@ -27,34 +26,41 @@ public ImagesController(IHttpClientFactory httpClientFactory, ILogger<ImagesCont
     /// </summary>
     /// <param name="analysisId">The ID of the analysis whose manifest contains the image.</param>
     /// <param name="puppeteer">The Puppeteer service.</param>
+    /// <param name="redis">The Redis cache.</param>
+    /// <param name="imageValidation">The image validation service.</param>
     /// <param name="cancelToken">The cancellation token.</param>
     /// <param name="imageUrl">The URL of the image to proxy.</param>
     /// <returns></returns>
     [HttpGet("getSafeImageForAnalysis")]
-    public async Task<IActionResult> GetSafeImageForAnalysis([FromQuery] string analysisId, [FromQuery] Uri imageUrl, [FromServices] IPuppeteerService puppeteer, [FromServices] IRedisCache db, CancellationToken cancelToken)
+    public async Task<IActionResult> GetSafeImageForAnalysis([FromQuery] string analysisId, [FromQuery] Uri imageUrl, [FromServices] IPuppeteerService puppeteer, [FromServices] IRedisCache redis, [FromServices] IImageValidationService imageValidation, CancellationToken cancelToken)
     {
         if (!imageUrl.IsAbsoluteInternetHttps())
         {
             return BadRequest("Image URL must be an absolute HTTPS URI pointing to a non-local address.");
         }
 
-        HttpClientExtensions.LimitedReadStreamWithMediaType imageStream;
+        // Ensure the analysis exists.
+        var analysis = await redis.GetByIdAsync<Analysis>(analysisId);
+        if (analysis == null)
+        {
+            logger.LogWarning("Image proxy endpoint failed to find analysis {analysisId} for image URL {imageUrl}", analysisId, imageUrl);
+            return NotFound($"Analysis with ID {analysisId} not found.");
+        }
+
         try
         {
-            imageStream = await http.GetImageAsync(imageUrl, maxSize, cancelToken);
-            Response.RegisterForDispose(imageStream);
-            return File(imageStream, string.IsNullOrWhiteSpace(imageStream.MediaType) ? "image/png" : imageStream.MediaType);
+            using var imageStream = await http.GetImageAsync(imageUrl, maxSize, cancelToken);
+            return await ValidateImageAsync(imageStream, imageStream.MediaType ?? "image/png", imageValidation, cancelToken);
         }
         catch (Exception error)
         {
             // Download via C# HttpClient failed. Try to load the image via Puppeteer.
             if (!string.IsNullOrEmpty(analysisId))
             {
-                var puppeteerImageStream = await TryDownloadImageViaPuppeteer(analysisId, imageUrl, puppeteer, db);
+                using var puppeteerImageStream = await TryDownloadImageViaPuppeteer(analysisId, imageUrl, puppeteer, redis);
                 if (puppeteerImageStream != null)
                 {
-                    Response.RegisterForDispose(puppeteerImageStream);
-                    return File(puppeteerImageStream, puppeteerImageStream.MediaType ?? "image/png");
+                    return await ValidateImageAsync(puppeteerImageStream, puppeteerImageStream.MediaType ?? "image/png", imageValidation, cancelToken);
                 }
             }
 
@@ -211,4 +217,31 @@ public async Task<IActionResult> GenerateStoreImages([FromForm] StoreImageCreati
             await resultHandle.DisposeAsync();
         }
     }
+
+    private async Task<IActionResult> ValidateImageAsync(Stream imageStream, string contentType, IImageValidationService imageValidation, CancellationToken cancellationToken)
+    {
+        // First, we need to copy the stream because the image stream may not be seekable.
+        var memStream = new MemoryStream();
+        await imageStream.CopyToAsync(memStream, cancellationToken);
+        memStream.Position = 0;
+
+        // Validate that it's a real image. This is needed to prevent crafted attacks where the server response is not an image.
+        var imageStatus = await imageValidation.IsValidImageAsync(memStream, cancellationToken);
+        if (imageStatus.Error != null)
+        {
+            logger.LogWarning("Image validation failed for proxied image with content type {contentType}. Error: {error}", contentType, imageStatus.Error);
+            memStream.Dispose();
+            return BadRequest($"You didn't pass a valid image endpoint.");
+        }
+        if (imageStatus.Value == false)
+        {
+            logger.LogWarning("Image validation failed for proxied image with content type {contentType}. The stream is not a valid image.", contentType);
+            memStream.Dispose();
+            return BadRequest($"You didn't pass a valid image endpoint.");
+        }
+
+        memStream.Position = 0;
+        Response.RegisterForDispose(memStream);
+        return File(memStream, contentType);
+    }
 }

apps/pwabuilder/Services/ImageValidationService.cs

@@ -35,6 +35,14 @@ public interface IImageValidationService
     /// <param name="cancelToken">The cancellation token.</param>
     /// <returns>True if the actual image dimensions match any of the declared sizes, otherwise the result will contain an exception.</returns>
     Task<Result<bool>> TryValidateImageSizeAsync(Uri imageUri, string? declaredSizes, CancellationToken cancelToken);
+
+    /// <summary>
+    /// Attempts to load the image to see if it's a valid image.
+    /// </summary>
+    /// <param name="imageStream">The stream to validate as an image.</param>
+    /// <param name="cancellationToken">The cancellation token.</param>
+    /// <returns>True if the image is valid, otherwise false with any error details.</returns>
+    Task<Result<bool>> IsValidImageAsync(Stream imageStream, CancellationToken cancellationToken);
 }
 
 /// <summary>
@@ -257,6 +265,32 @@ public async Task<Result<bool>> TryValidateImageSizeAsync(Uri imageUri, string?
         }
     }
 
+    /// <inheritdoc/>
+    public async Task<Result<bool>> IsValidImageAsync(Stream imageStream, CancellationToken cancellationToken)
+    {
+        try
+        {
+            var imageInfo = await Image.IdentifyAsync(imageStream, cancellationToken);
+            if (imageInfo == null)
+            {
+                logger.LogInformation("Attempted to identify image from stream, but it couldn't be identified.");
+                return new ArgumentException("Image couldn't be identified.", nameof(imageStream));
+            }
+
+            return true;
+        }
+        catch (UnknownImageFormatException unknownFormatError)
+        {
+            logger.LogInformation(unknownFormatError, "Attempted to identify image from stream, but it was in an unknown format.");
+            return unknownFormatError;
+        }
+        catch (Exception error)
+        {
+            logger.LogInformation(error, "Attempted to identify image from stream, but an error occurred.");
+            return error;
+        }
+    }
+
     private Result<bool> EnsureImageContentType(Uri imageUrl, string? contentType)
     {
         // If we have no content type, we can't validate it. Assume it's valid and hope for the best.