Return 400 for image requests that cannot be parsed

src/protagonist/DLCS.Model/DLCS.Model.csproj

@@ -11,7 +11,7 @@
   </ItemGroup>
 
   <ItemGroup>
-    <PackageReference Include="iiif-net" Version="0.2.5" />
+    <PackageReference Include="iiif-net" Version="0.2.6" />
   </ItemGroup>
 
 </Project>

src/protagonist/DLCS.Repository/DLCS.Repository.csproj

@@ -8,7 +8,7 @@
 
   <ItemGroup>
     <PackageReference Include="Dapper" Version="2.0.123" />
-    <PackageReference Include="iiif-net" Version="0.2.5" />
+    <PackageReference Include="iiif-net" Version="0.2.6" />
     <PackageReference Include="LazyCache" Version="2.4.0" />
     <PackageReference Include="Microsoft.EntityFrameworkCore.Design" Version="6.0.5">
       <PrivateAssets>all</PrivateAssets>

src/protagonist/DLCS.Web.Tests/Requests/AssetDelivery/AssetDeliveryPathParserTests.cs

@@ -3,6 +3,7 @@
 using DLCS.Model.PathElements;
 using DLCS.Web.Requests.AssetDelivery;
 using FakeItEasy;
+using Microsoft.Extensions.Logging.Abstractions;
 
 namespace DLCS.Web.Tests.Requests.AssetDelivery;
 
@@ -14,7 +15,7 @@ public class AssetDeliveryPathParserTests
     public AssetDeliveryPathParserTests()
     {
         pathCustomerRepository = A.Fake<IPathCustomerRepository>();
-        sut = new AssetDeliveryPathParser(pathCustomerRepository);
+        sut = new AssetDeliveryPathParser(pathCustomerRepository, new NullLogger<AssetDeliveryPathParser>());
     }
 
     [Fact]
@@ -227,8 +228,8 @@ public async Task Parse_VersionedImageRequest_SetsNormalisedPath(string version,
 
     [Theory]
     [InlineData("/iiif-img/full/!800,400/0/default.jpg")]
-    [InlineData("/iiif-av/test-customer/1/the-astronaut/full/full/max/max/0/default.mp3")]
-    public async Task Parse_ImageRequest_ThrowsFormatException_IfPathInUnknownFormat(string path)
+    [InlineData("/iiif-img/customer-test/1,2,3/full/!800,400/0/default.jpg")]
+    public async Task Parse_ImageRequest_ThrowsFormatException_IfBaseAssetPathInUnknownFormat(string path)
     {
         // Act
         Func<Task> action = () => sut.Parse<ImageAssetDeliveryRequest>(path);
@@ -237,6 +238,18 @@ public async Task Parse_ImageRequest_ThrowsFormatException_IfPathInUnknownFormat
         await action.Should().ThrowAsync<FormatException>();
     }
 
+    [Theory]
+    [InlineData("/iiif-img/test-customer/1/the-astronaut/max/0/default.jpg")]
+    [InlineData("/iiif-av/test-customer/1/the-astronaut/full/full/max/max/0/default.mp3")]
+    public async Task Parse_ImageRequest_ThrowsArgumentException_IfPathInUnknownFormat(string path)
+    {
+        // Act
+        Func<Task> action = () => sut.Parse<ImageAssetDeliveryRequest>(path);
+
+        // Assert
+        await action.Should().ThrowAsync<ArgumentException>();
+    }
+
     [Fact]
     public async Task Parse_TimeBasedRequest_WithCustomerName_FullParse()
     {

src/protagonist/DLCS.Web/DLCS.Web.csproj

@@ -12,7 +12,7 @@
   </ItemGroup>
 
   <ItemGroup>
-    <PackageReference Include="iiif-net" Version="0.2.5" />
+    <PackageReference Include="iiif-net" Version="0.2.6" />
     <PackageReference Include="Microsoft.AspNetCore.Http" Version="2.2.2" />
     <PackageReference Include="Microsoft.AspNetCore.Http.Abstractions" Version="2.2.0" />
     <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="6.0.1" />

src/protagonist/DLCS.Web/Middleware/StatusCodeExceptionHandlerMiddleware.cs

@@ -1,4 +1,6 @@
-using System.Threading.Tasks;
+using System;
+using System.Net;
+using System.Threading.Tasks;
 using DLCS.Core.Exceptions;
 using DLCS.Web.Response;
 using Microsoft.AspNetCore.Http;
@@ -23,6 +25,10 @@ public async Task InvokeAsync(HttpContext httpContext)
         {
             await next(httpContext);
         }
+        catch (ArgumentException)
+        {
+            httpContext.Response.StatusCode = (int)HttpStatusCode.BadRequest;
+        }
         catch (HttpException ex)
         {
             await HandleExceptionAsync(httpContext, ex);

src/protagonist/DLCS.Web/Requests/AssetDelivery/AssetDeliveryPathParser.cs

@@ -1,11 +1,14 @@
 using System;
+using System.Collections.Generic;
 using System.Linq;
 using System.Net;
 using System.Text;
 using System.Text.RegularExpressions;
 using System.Threading.Tasks;
+using DLCS.Core.Exceptions;
 using DLCS.Model.PathElements;
 using IIIF.ImageApi;
+using Microsoft.Extensions.Logging;
 
 namespace DLCS.Web.Requests.AssetDelivery;
 
@@ -21,21 +24,58 @@ public interface IAssetDeliveryPathParser
     /// <typeparam name="T">Type of <see cref="BaseAssetRequest"/> to return.</typeparam>
     /// <returns>Populated <see cref="ImageAssetDeliveryRequest"/> object</returns>
     Task<T> Parse<T>(string path) where T : BaseAssetRequest, new();
+
+    /// <summary>
+    /// Parse asset delivery requests to <see cref="BaseAssetRequest"/>, any exceptions thrown during parsing are
+    /// converted to an HttpException for ease of handling.
+    /// </summary>
+    /// <param name="path">Full asset request path</param>
+    /// <typeparam name="T">Type of <see cref="BaseAssetRequest"/> to return.</typeparam>
+    /// <returns>Populated <see cref="ImageAssetDeliveryRequest"/> object</returns>
+    /// <exception cref="HttpException">Raised with appropriate status code depending on error</exception>
+    Task<T> ParseForHttp<T>(string path) where T : BaseAssetRequest, new();
 }
 
 public class AssetDeliveryPathParser : IAssetDeliveryPathParser
 {
     private readonly IPathCustomerRepository pathCustomerRepository;
-
+    private readonly ILogger<AssetDeliveryPathParser> logger;
+
     // regex to clean query params and hashmark from asset id
     private static readonly Regex AssetIdClean = new(@"[\?\#].*$", RegexOptions.Compiled);
 
     // regex to match v1, v2 etc but not a v23
     private static readonly Regex VersionRegex = new("^(v\\d)$", RegexOptions.Compiled);
 
-    public AssetDeliveryPathParser(IPathCustomerRepository pathCustomerRepository)
+    public AssetDeliveryPathParser(IPathCustomerRepository pathCustomerRepository,
+        ILogger<AssetDeliveryPathParser> logger)
     {
         this.pathCustomerRepository = pathCustomerRepository;
+        this.logger = logger;
+    }
+
+    public async Task<T> ParseForHttp<T>(string path)
+        where T : BaseAssetRequest, new()
+    {
+        try
+        {
+            return await Parse<T>(path);
+        }
+        catch (KeyNotFoundException ex)
+        {
+            logger.LogError(ex, "Could not find Customer/Space from '{Path}'", path);
+            throw new HttpException(HttpStatusCode.NotFound, $"Could not find Customer/Space from '{path}'", ex);
+        }
+        catch (FormatException ex)
+        {
+            logger.LogError(ex, "Error parsing path '{Path}'", path);
+            throw new HttpException(HttpStatusCode.BadRequest, $"Error parsing path '{path}'", ex);
+        }
+        catch (Exception ex)
+        {
+            logger.LogError(ex, "Error parsing path '{Path}'", path);
+            throw new HttpException(HttpStatusCode.BadRequest, $"Error parsing path '{path}'", ex);
+        }
     }
 
     public async Task<T> Parse<T>(string path) 
@@ -47,7 +87,7 @@ public async Task<T> Parse<T>(string path)
 
         if (assetRequest is ImageAssetDeliveryRequest imageAssetRequest)
         {
-            imageAssetRequest.IIIFImageRequest = ImageRequest.Parse(escapedPath, assetRequest.BasePath);
+            imageAssetRequest.IIIFImageRequest = ImageRequest.Parse(escapedPath, assetRequest.BasePath, true);
         }
         else if (assetRequest is TimeBasedAssetDeliveryRequest timebasedAssetRequest)
         {

src/protagonist/Engine/Engine.csproj

@@ -7,7 +7,7 @@
     </PropertyGroup>
 
     <ItemGroup>
-        <PackageReference Include="iiif-net" Version="0.2.5" />
+        <PackageReference Include="iiif-net" Version="0.2.6" />
         <PackageReference Include="Microsoft.AspNet.WebApi.Client" Version="5.2.9" />
         <PackageReference Include="Serilog.AspNetCore" Version="5.0.0" />
         <PackageReference Include="AWSSDK.SecurityToken" Version="3.7.1.150" />

src/protagonist/Orchestrator.Tests/Features/Images/ImageRequestHandlerTests.cs

@@ -3,6 +3,7 @@
 using System.Net;
 using System.Text.Json.Nodes;
 using System.Threading;
+using DLCS.Core.Exceptions;
 using DLCS.Core.Settings;
 using DLCS.Core.Types;
 using DLCS.Model.Assets.CustomHeaders;
@@ -40,7 +41,7 @@ public ImageRequestHandlerTests()
         assetDeliveryPathParser = A.Fake<IAssetDeliveryPathParser>();
         customerRepository = A.Fake<IPathCustomerRepository>();
         accessValidator = A.Fake<IAssetAccessValidator>();
-        assetDeliveryPathParserImpl = new AssetDeliveryPathParser(customerRepository);
+        assetDeliveryPathParserImpl = new AssetDeliveryPathParser(customerRepository, new NullLogger<AssetDeliveryPathParser>());
         customHeaderRepository = A.Fake<ICustomHeaderRepository>();
 
         scopeFactory = A.Fake<IServiceScopeFactory>();
@@ -80,11 +81,11 @@ private static OrchestratorSettings CreateOrchestratorSettings()
     }
 
     [Fact]
-    public async Task HandleRequest_Returns404_IfAssetPathParserThrowsKeyNotFound()
+    public async Task HandleRequest_Returns404_IfAssetPathParserThrowsHttpException_NotFound()
     {
         // Arrange
-        A.CallTo(() => assetDeliveryPathParser.Parse<ImageAssetDeliveryRequest>(A<string>._))
-            .ThrowsAsync(new KeyNotFoundException());
+        A.CallTo(() => assetDeliveryPathParser.ParseForHttp<ImageAssetDeliveryRequest>(A<string>._))
+            .ThrowsAsync(new HttpException(HttpStatusCode.NotFound, "Could not find Customer/Space"));
         var sut = GetImageRequestHandlerWithMockPathParser(true);
 
         // Act
@@ -95,30 +96,13 @@ public async Task HandleRequest_Returns404_IfAssetPathParserThrowsKeyNotFound()
     }
 
     [Fact]
-    public async Task HandleRequest_Returns400_IfAssetPathParserThrowsFormatException()
+    public async Task HandleRequest_Returns400_IfAssetPathParserThrowsHttpException_BadRequest()
     {
         // NOTE - routes should prevent this from ever happening
 
         // Arrange
-        A.CallTo(() => assetDeliveryPathParser.Parse<ImageAssetDeliveryRequest>(A<string>._))
-            .ThrowsAsync(new FormatException());
-        var sut = GetImageRequestHandlerWithMockPathParser(true);
-
-        // Act
-        var result = await sut.HandleRequest(new DefaultHttpContext());
-
-        // Assert
-        result.Should().BeOfType<StatusCodeResult>().Which.StatusCode.Should().Be(HttpStatusCode.BadRequest);
-    }
-
-    [Fact]
-    public async Task HandleRequest_Returns400_IfAssetPathParserThrowsException()
-    {
-        // NOTE - routes should prevent this from ever happening
-
-        // Arrange
-        A.CallTo(() => assetDeliveryPathParser.Parse<ImageAssetDeliveryRequest>(A<string>._))
-            .ThrowsAsync(new ApplicationException());
+        A.CallTo(() => assetDeliveryPathParser.ParseForHttp<ImageAssetDeliveryRequest>(A<string>._))
+            .ThrowsAsync(new HttpException(HttpStatusCode.BadRequest, "Error parsing path"));
         var sut = GetImageRequestHandlerWithMockPathParser(true);
 
         // Act

src/protagonist/Orchestrator.Tests/Integration/ImageHandlingTests.cs

@@ -1120,6 +1120,8 @@ public async Task Get_UnknownImage_Returns404()
     [Theory]
     [InlineData("iiif-img/99/1/my-image/full/all/0/default.jpg")]
     [InlineData("iiif-img/99/1/my-image/!200,200/full/0/default.jpg")]
+    [InlineData("iiif-img/99/1/my-image/full////max/0/default.jpg")]
+    [InlineData("iiif-img/99/1/my-image/full/0/default.jpg")]
     public async Task Get_MalformedImageRequest_Returns400(string path)
     {
         // Act

src/protagonist/Orchestrator/Infrastructure/AssetRequestProcessor.cs

@@ -2,6 +2,7 @@
 using System.Collections.Generic;
 using System.Net;
 using System.Threading.Tasks;
+using DLCS.Core.Exceptions;
 using DLCS.Web.Requests.AssetDelivery;
 using DLCS.Web.Response;
 using Microsoft.AspNetCore.Http;
@@ -41,25 +42,13 @@ public class AssetRequestProcessor
         try
         {
             var assetRequest =
-                await assetDeliveryPathParser.Parse<T>(httpContext.Request.Path);
+                await assetDeliveryPathParser.ParseForHttp<T>(httpContext.Request.Path);
 
             return (assetRequest, null);
         }
-        catch (KeyNotFoundException ex)
+        catch (HttpException ex)
         {
-            logger.LogError(ex, "Could not find Customer/Space from '{Path}'", httpContext.Request.Path);
-            return (null, HttpStatusCode.NotFound);
-        }
-        catch (FormatException ex)
-        {
-            logger.LogError(ex, "Error parsing path '{Path}'", httpContext.Request.Path);
-            return (null, HttpStatusCode.BadRequest);
-        }
-        catch (Exception ex)
-        {
-            // TODO - is this the correct status?
-            logger.LogError(ex, "Error parsing path '{Path}'", httpContext.Request.Path);
-            return (null, HttpStatusCode.BadRequest);
+            return (null, ex.StatusCode);
         }
     }
 

src/protagonist/Portal/Portal.csproj

@@ -10,7 +10,7 @@
       <PackageReference Include="AWSSDK.SecurityToken" Version="3.7.1.150" />
       <PackageReference Include="CsvHelper" Version="30.0.1" />
       <PackageReference Include="Destructurama.Attributed" Version="2.0.0" />
-      <PackageReference Include="iiif-net" Version="0.2.5" />
+      <PackageReference Include="iiif-net" Version="0.2.6" />
       <PackageReference Include="MediatR" Version="10.0.1" />
       <PackageReference Include="MediatR.Extensions.Microsoft.DependencyInjection" Version="10.0.1" />
       <PackageReference Include="Microsoft.AspNetCore.Mvc.Razor.RuntimeCompilation" Version="6.0.5" />

src/protagonist/Thumbs/Thumbs.csproj

@@ -10,7 +10,7 @@
   <ItemGroup>
     <PackageReference Include="AspNetCore.HealthChecks.NpgSql" Version="6.0.2" />
     <PackageReference Include="AWSSDK.SecurityToken" Version="3.7.1.150" />
-    <PackageReference Include="iiif-net" Version="0.2.5" />
+    <PackageReference Include="iiif-net" Version="0.2.6" />
     <PackageReference Include="LazyCache.AspNetCore" Version="2.4.0" />
     <PackageReference Include="Microsoft.Extensions.Logging.Debug" Version="6.0.0" />
     <PackageReference Include="Serilog.AspNetCore" Version="5.0.0" />

src/protagonist/Thumbs/ThumbsMiddleware.cs

@@ -50,7 +50,7 @@ public class ThumbsMiddleware
         try
         {
             var pathValue = context.Request.Path.Value.ThrowIfNullOrEmpty(nameof(context.Request.Path.Value));
-            var thumbnailRequest = await parser.Parse<ImageAssetDeliveryRequest>(pathValue);
+            var thumbnailRequest = await parser.ParseForHttp<ImageAssetDeliveryRequest>(pathValue);
             context.Response.SetAssetIdResponseHeader(thumbnailRequest.GetAssetId());
             if (thumbnailRequest.IIIFImageRequest.IsBase)
             {