Refuse a locks/batch ref that is not an object with 400 instead of 500 [patch]

GitLfsCache.Tests/Integration/LockFanOutTests.cs

@@ -57,6 +57,21 @@ public async Task ManyPaths_BecomeOneClientRoundTripAndManyUpstreamCalls()
 		Assert.AreEqual(50, fixture.Upstream.LockChangeRequests);
 	}
 
+	[TestMethod]
+	public async Task RefThatIsNotAnObject_IsRefusedWithoutCallingUpstream()
+	{
+		// The listing endpoint takes the ref as a refspec string, so flattening it here is an easy
+		// mistake for a client to make. It is a malformed request, not a proxy failure.
+		await using ProxyFixture fixture = await ProxyFixture.StartAsync();
+
+		using HttpResponseMessage response = await PostBatchAsync(
+			fixture,
+			"""{"operation":"lock","paths":["Content/A.uasset"],"ref":"refs/heads/main"}""");
+
+		Assert.AreEqual(HttpStatusCode.BadRequest, response.StatusCode);
+		Assert.AreEqual(0, fixture.Upstream.LockChangeRequests);
+	}
+
 	[TestMethod]
 	public async Task EveryCall_CarriesTheCallersOwnCredential()
 	{

GitLfsCache.Tests/Locks/LockFanOutRequestTests.cs

@@ -0,0 +1,45 @@
+// Copyright (c) 2023-2026 ktsu-dev contributors
+
+namespace ktsu.GitLfsCache.Tests.Locks;
+
+using System.Text.Json.Nodes;
+using ktsu.GitLfsCache.Locks;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+[TestClass]
+public class LockFanOutRequestTests
+{
+	private static JsonNode Parse(string json) => JsonNode.Parse(json)!;
+
+	[TestMethod]
+	public void TryParse_RefObject_ReadsItsName()
+	{
+		JsonNode body = Parse("""{"operation":"lock","paths":["a.uasset"],"ref":{"name":"refs/heads/main"}}""");
+
+		Assert.IsTrue(LockFanOutRequest.TryParse(body, out LockFanOutRequest? request));
+		Assert.AreEqual("refs/heads/main", request.Ref);
+	}
+
+	[TestMethod]
+	public void TryParse_NoRef_HasNoRef()
+	{
+		JsonNode body = Parse("""{"operation":"lock","paths":["a.uasset"]}""");
+
+		Assert.IsTrue(LockFanOutRequest.TryParse(body, out LockFanOutRequest? request));
+		Assert.IsNull(request.Ref);
+	}
+
+	[TestMethod]
+	[DataRow("\"refs/heads/main\"")]
+	[DataRow("1")]
+	[DataRow("true")]
+	[DataRow("[\"refs/heads/main\"]")]
+	public void TryParse_RefThatIsNotAnObject_IsRefused(string refJson)
+	{
+		// Taking the lock with no ref would not be the lock the client asked for.
+		JsonNode body = Parse($$"""{"operation":"lock","paths":["a.uasset"],"ref":{{refJson}}}""");
+
+		Assert.IsFalse(LockFanOutRequest.TryParse(body, out LockFanOutRequest? request));
+		Assert.IsNull(request);
+	}
+}

GitLfsCache/Locks/LockFanOutRequest.cs

@@ -56,10 +56,15 @@ public static bool TryParse(JsonNode? body, [NotNullWhen(true)] out LockFanOutRe
 			return false;
 		}
 
+		if (!TryReadRef(root["ref"], out string? refName))
+		{
+			return false;
+		}
+
 		request = new LockFanOutRequest(
 			operation,
 			targets,
-			JsonValues.String(root["ref"]?["name"]),
+			refName,
 			JsonValues.Bool(root["force"]) ?? false);
 
 		return true;
@@ -73,6 +78,32 @@ private static LockFanOutOperation ReadOperation(JsonObject root) =>
 			_ => LockFanOutOperation.Unknown,
 		};
 
+	/// <summary>
+	/// Reads the ref's name, treating an absent ref as none.
+	/// </summary>
+	/// <remarks>
+	/// A ref that is present but not an object (a client that flattened it to the ref name, as the
+	/// listing endpoint's <c>refspec</c> query takes it) refuses the whole body. Taking the lock with no
+	/// ref would not be the lock the client asked for, and indexing into a non-object throws.
+	/// </remarks>
+	private static bool TryReadRef(JsonNode? node, out string? name)
+	{
+		name = null;
+
+		if (node is null)
+		{
+			return true;
+		}
+
+		if (node is not JsonObject refObject)
+		{
+			return false;
+		}
+
+		name = JsonValues.String(refObject["name"]);
+		return true;
+	}
+
 	/// <summary>
 	/// Reads the paths, and for a release the ids as well.
 	/// </summary>