From 2525b1f1a58da52f2d873f97be70957af671bcba Mon Sep 17 00:00:00 2001
From: Claude <noreply@anthropic.com>
Date: Thu, 8 Oct 2026 06:28:35 +0000
Subject: [PATCH] Refuse a locks/batch ref that is not an object with 400
 instead of 500 [patch]

LockFanOutRequest.TryParse read the ref with root["ref"]?["name"], and
JsonNode's string indexer throws InvalidOperationException when the node is
a string, number, bool or array. Nothing caught it, so a client that sent
"ref": "refs/heads/main" got an unhandled 500.

The ref is now read with a type check. A ref that is present but not an
object refuses the whole body, matching the parser's rule that every
malformed shape is refused, so a lock is never taken without the ref the
client meant to send.

Fixes #77

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Md2Tr7FWcbqPdwq2BjeG79
---
 .../Integration/LockFanOutTests.cs            | 15 +++++++
 .../Locks/LockFanOutRequestTests.cs           | 45 +++++++++++++++++++
 GitLfsCache/Locks/LockFanOutRequest.cs        | 33 +++++++++++++-
 3 files changed, 92 insertions(+), 1 deletion(-)
 create mode 100644 GitLfsCache.Tests/Locks/LockFanOutRequestTests.cs

diff --git a/GitLfsCache.Tests/Integration/LockFanOutTests.cs b/GitLfsCache.Tests/Integration/LockFanOutTests.cs
index 8f81f3b..0c153dc 100644
--- a/GitLfsCache.Tests/Integration/LockFanOutTests.cs
+++ b/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()
 	{
diff --git a/GitLfsCache.Tests/Locks/LockFanOutRequestTests.cs b/GitLfsCache.Tests/Locks/LockFanOutRequestTests.cs
new file mode 100644
index 0000000..ba24d35
--- /dev/null
+++ b/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);
+	}
+}
diff --git a/GitLfsCache/Locks/LockFanOutRequest.cs b/GitLfsCache/Locks/LockFanOutRequest.cs
index 0918bcb..7041f66 100644
--- a/GitLfsCache/Locks/LockFanOutRequest.cs
+++ b/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>