Answer 502, not 500, when upstream's 2xx batch body cannot be used [patch]

GitLfsCache.Tests/Batch/BatchRewriterTests.cs

@@ -2,6 +2,7 @@
 
 namespace ktsu.GitLfsCache.Tests.Batch;
 
+using System.Text.Json;
 using System.Text.Json.Nodes;
 using ktsu.Essentials.EncryptionProviders.Aes;
 using ktsu.GitLfsCache.Batch;
@@ -208,6 +209,21 @@ public void Rewrite_ActionWithNoHref_IsLeftAlone()
 		Assert.HasCount(0, FirstAction(rewritten, "download"));
 	}
 
+	[TestMethod]
+	[DataRow("""{"objects":[{"oid":1,"size":1,"actions":{"download":{"href":"https://upstream.example/x"}}}]}""")]
+	[DataRow("""{"objects":[{"oid":"aa","size":"123","actions":{"download":{"href":"https://upstream.example/x"}}}]}""")]
+	[DataRow("""{"objects":[{"oid":"aa","size":1.5,"actions":{"download":{"href":"https://upstream.example/x"}}}]}""")]
+	[DataRow("""{"objects":[{"oid":"aa","size":1,"actions":{"download":{"href":7}}}]}""")]
+	[DataRow("""{"objects":[{"oid":"aa","size":1,"actions":{"download":{"href":"https://upstream.example/x","header":{"X-Count":1}}}}]}""")]
+	public void Rewrite_FieldOfTheWrongType_IsRefusedAsMalformed(string json)
+	{
+		// Neither rewritable nor safe to pass through with upstream's credentials still in it.
+		(BatchRewriter rewriter, _) = Create();
+		JsonNode input = JsonNode.Parse(json)!;
+
+		Assert.ThrowsExactly<JsonException>(() => rewriter.Rewrite(input, Context()));
+	}
+
 	[TestMethod]
 	public void Rewrite_DoesNotMutateTheInputNode()
 	{

GitLfsCache.Tests/Integration/ProxyFlowTests.cs

@@ -121,6 +121,26 @@
 		Assert.Contains("Repository not found", await response.Content.ReadAsStringAsync());
 	}
 
+	[TestMethod]
+	[DataRow("<html><body>Sign in to continue</body></html>", "text/html")]
+	[DataRow("""{"objects":[{"oid":"aa","size":"123","actions":{"download":{"href":"https://upstream.example/x"}}}]}""", "application/vnd.git-lfs+json")]
+	[DataRow("""{"objects":[{"oid":"aa","size":1,"actions":{"download":{"href":"https://upstream.example/x","header":{"X-Count":1}}}}]}""", "application/vnd.git-lfs+json")]
+	public async Task Batch_UpstreamSuccessThatCannotBeUsed_IsABadGateway(string upstreamBody, string mediaType)
+	{
+		// A sign-in or SSO page served with 200, or a field of the wrong type, is upstream's failure.
+		// Answering 500 would point operators at the proxy instead.
+		await using ProxyFixture fixture = await ProxyFixture.StartAsync();
+		fixture.Upstream.BatchSuccessBody = (upstreamBody, mediaType);
+		(byte[] content, string oid) = Object("behind a sign-in page");
+		using HttpClient client = fixture.Client;
+
+		using StringContent body = BatchRequest("download", oid, content.Length);
+
+		using HttpResponseMessage response = await client.PostAsync($"{LfsPath}/objects/batch", body);
+
+		Assert.AreEqual(HttpStatusCode.BadGateway, response.StatusCode);
+	}
+
 	[TestMethod]
 	public async Task Download_ColdThenWarm_FetchesUpstreamOnceAndServesFromTheStore()
 	{

GitLfsCache.Tests/Integration/StubUpstream.cs

@@ -38,6 +38,12 @@ public IReadOnlyList<RecordedRequest> Requests
 	/// <summary>Gets or sets the body the batch endpoint answers with when it is not successful.</summary>
 	public string BatchFailureBody { get; set; } = """{"message":"Repository not found"}""";
 
+	/// <summary>
+	/// Gets or sets a body the batch endpoint answers a successful call with verbatim, in place of the
+	/// one it would build, used to make upstream hand back a 2xx the proxy cannot use.
+	/// </summary>
+	public (string Body, string MediaType)? BatchSuccessBody { get; set; }
+
 	/// <summary>Gets or sets the status an object fetch answers with.</summary>
 	public HttpStatusCode ObjectStatus { get; set; } = HttpStatusCode.OK;
 
@@ -288,6 +294,14 @@ private HttpResponseMessage BuildBatchResponse(string? requestBody)
 			};
 		}
 
+		if (BatchSuccessBody is (string body, string mediaType))
+		{
+			return new HttpResponseMessage(HttpStatusCode.OK)
+			{
+				Content = new StringContent(body, Encoding.UTF8, mediaType),
+			};
+		}
+
 		JsonNode parsed = JsonNode.Parse(requestBody ?? "{}")!;
 		string operation = parsed["operation"]?.GetValue<string>() ?? "download";
 		JsonArray objects = [];

GitLfsCache/Batch/BatchRewriter.cs

@@ -2,8 +2,10 @@
 
 namespace ktsu.GitLfsCache.Batch;
 
+using System.Text.Json;
 using System.Text.Json.Nodes;
 using ktsu.GitLfsCache.Configuration;
+using ktsu.GitLfsCache.Locks;
 using ktsu.GitLfsCache.Tokens;
 using Microsoft.Extensions.Options;
 
@@ -34,6 +36,11 @@ public sealed class BatchRewriter(
 	/// <param name="upstreamResponse">The parsed upstream response.</param>
 	/// <param name="context">The request context.</param>
 	/// <returns>A new node tree with rewritten hrefs.</returns>
+	/// <exception cref="JsonException">
+	/// An object's <c>oid</c> or <c>size</c>, or an action's <c>href</c> or header value, has the wrong
+	/// JSON type. Such an entry can be neither rewritten nor safely passed through with upstream's
+	/// credentials still in it, so the whole response is refused.
+	/// </exception>
 	public JsonNode Rewrite(JsonNode upstreamResponse, BatchRewriteContext context)
 	{
 		Ensure.NotNull(upstreamResponse);
@@ -78,14 +85,14 @@ private void RewriteObject(
 			return;
 		}
 
-		string? oid = batchObject["oid"]?.GetValue<string>();
+		string? oid = ReadString(batchObject["oid"], "oid");
 
 		if (string.IsNullOrEmpty(oid))
 		{
 			return;
 		}
 
-		long size = batchObject["size"]?.GetValue<long>() ?? 0;
+		long size = ReadSize(batchObject["size"]);
 
 		foreach (string actionName in RewrittenActions)
 		{
@@ -106,7 +113,7 @@ private void RewriteAction(
 		DateTimeOffset expiresAt,
 		int expiresInSeconds)
 	{
-		string? upstreamHref = action["href"]?.GetValue<string>();
+		string? upstreamHref = ReadString(action["href"], "href");
 
 		if (string.IsNullOrEmpty(upstreamHref))
 		{
@@ -121,7 +128,7 @@ private void RewriteAction(
 			{
 				if (value is not null)
 				{
-					headers[name] = value.GetValue<string>();
+					headers[name] = ReadString(value, $"header {name}")!;
 				}
 			}
 		}
@@ -149,6 +156,32 @@ private void RewriteAction(
 		action["expires_in"] = expiresInSeconds;
 	}
 
+	/// <summary>
+	/// Reads a string field, treating an absent one as null and any other type as malformed.
+	/// </summary>
+	private static string? ReadString(JsonNode? node, string field) =>
+		node is null
+			? null
+			: JsonValues.String(node) ?? throw new JsonException($"The batch response's {field} is not a string.");
+
+	/// <summary>
+	/// Reads an object's size, treating an absent one as zero and anything but an integer as malformed.
+	/// </summary>
+	private static long ReadSize(JsonNode? node)
+	{
+		if (node is null)
+		{
+			return 0;
+		}
+
+		if (node.GetValueKind() == JsonValueKind.Number && node is JsonValue value && value.TryGetValue(out long size))
+		{
+			return size;
+		}
+
+		throw new JsonException("The batch response's size is not an integer.");
+	}
+
 	private static string BuildProxyHref(
 		string actionName,
 		string oid,

GitLfsCache/Endpoints/ObjectRouteHandler.cs

@@ -34,7 +34,7 @@
 /// <param name="metrics">Cache counters.</param>
 /// <param name="options">The configured options.</param>
 /// <param name="logger">Logger.</param>
 internal sealed class ObjectRouteHandler(
 	IUpstreamClient upstreamClient,
 	IHrefTokenCodec codec,
 	BatchRewriter rewriter,
@@ -84,30 +84,42 @@
 		}
 
 		JsonNode? upstreamBody;
+		JsonNode rewritten;
 
 		Stream batchBody = await response.Content
 			.ReadAsStreamAsync(cancellationToken)
 			.ConfigureAwait(false);
 
-		await using (batchBody.ConfigureAwait(false))
+		// A success status with a body this proxy cannot use (a sign-in page served with 200, or a
+		// field of the wrong type) is upstream's failure, not the proxy's, so it is a 502 rather than
+		// an unhandled exception and a 500 that points operators at the wrong component.
+		try
 		{
-			upstreamBody = await JsonNode.ParseAsync(batchBody, cancellationToken: cancellationToken)
-				.ConfigureAwait(false);
-		}
+			await using (batchBody.ConfigureAwait(false))
+			{
+				upstreamBody = await JsonNode.ParseAsync(batchBody, cancellationToken: cancellationToken)
+					.ConfigureAwait(false);
+			}
 
-		if (upstreamBody is null)
+			if (upstreamBody is null)
+			{
+				context.Response.StatusCode = StatusCodes.Status502BadGateway;
+				return;
+			}
+
+			rewritten = rewriter.Rewrite(upstreamBody, new BatchRewriteContext
+			{
+				Upstream = route.Upstream,
+				RepositoryPath = route.RepositoryPath,
+				PublicBaseUrl = publicUrls.Resolve(context.Request),
+			});
+		}
+		catch (System.Text.Json.JsonException)
 		{
 			context.Response.StatusCode = StatusCodes.Status502BadGateway;
 			return;
 		}
 
-		JsonNode rewritten = rewriter.Rewrite(upstreamBody, new BatchRewriteContext
-		{
-			Upstream = route.Upstream,
-			RepositoryPath = route.RepositoryPath,
-			PublicBaseUrl = publicUrls.Resolve(context.Request),
-		});
-
 		context.Response.StatusCode = StatusCodes.Status200OK;
 		context.Response.ContentType = UpstreamRequests.LfsMediaType;
 		await context.Response