From 86602d66428484ef85596f09db533c98e661487f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 09:29:16 +0000 Subject: [PATCH] Forward the client's refspec on cached lock walks and probes Lock snapshots are keyed by the ?refspec= a client listed under, because the locking API treats the ref as an authentication input and upstream may answer differently per ref. But BuildLockListRequest had no refspec, so both the walk that fills a snapshot and the one-page probe that admits a caller to it asked upstream the unscoped question. With the cache warm, a ref-scoped listing got the no-ref answer, and a credential upstream would refuse for that ref was admitted anyway. BuildLockListRequest now takes the refspec and LockListRefresher passes key.Ref from both RefreshAsync and ProbeAsync. CredentialAdmission keys on the ref as well, so an admission earned under one ref does not admit the same credential to another ref's snapshot without its own probe. Fixes #47 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016wsoxnwaqMuAzvkm2xnzYh --- .../Integration/LockListCachingTests.cs | 35 ++++++++++++ GitLfsCache.Tests/Integration/StubUpstream.cs | 6 +- .../Locks/CredentialAdmissionTests.cs | 56 ++++++++++++------- GitLfsCache/Locks/CredentialAdmission.cs | 22 +++++--- GitLfsCache/Locks/ICredentialAdmission.cs | 12 +++- GitLfsCache/Locks/LockListRefresher.cs | 2 + GitLfsCache/Locks/LockListService.cs | 6 +- GitLfsCache/Upstreams/UpstreamRequests.cs | 11 ++++ 8 files changed, 115 insertions(+), 35 deletions(-) diff --git a/GitLfsCache.Tests/Integration/LockListCachingTests.cs b/GitLfsCache.Tests/Integration/LockListCachingTests.cs index 01e30c6..13bc65f 100644 --- a/GitLfsCache.Tests/Integration/LockListCachingTests.cs +++ b/GitLfsCache.Tests/Integration/LockListCachingTests.cs @@ -215,6 +215,41 @@ public async Task CreatingALock_InvalidatesTheSnapshot() Assert.HasCount(2, (await ListLocksAsync(fixture))["locks"]!.AsArray()); } + [TestMethod] + [DataRow("?refspec=refs/heads/main", "refspec=refs%2Fheads%2Fmain", DisplayName = "With a refspec")] + [DataRow("", null, DisplayName = "Without a refspec")] + public async Task CachedListing_ForwardsTheClientsRefspecOnTheWalkAndTheProbe(string query, string? forwarded) + { + // The snapshot is keyed by ref because upstream may answer differently per ref, so what fills it + // and what admits a caller to it have to ask upstream about that same ref. + await using ProxyFixture fixture = await ProxyFixture.StartAsync(); + fixture.Upstream.Locks.Add("a"); + + await ListLocksAsync(fixture, query, Credential); + await ListLocksAsync(fixture, query, OtherCredential); + + StubUpstream.RecordedRequest[] listings = + [ + .. fixture.Upstream.Requests.Where(request => request.Path.EndsWith("/locks", StringComparison.Ordinal)), + ]; + + // The first caller's walk and the second caller's probe. + Assert.HasCount(2, listings); + Assert.Contains("limit=1", listings[1].Query, "The second request should be the one-page probe."); + + foreach (StubUpstream.RecordedRequest listing in listings) + { + if (forwarded is null) + { + Assert.DoesNotContain("refspec", listing.Query); + } + else + { + Assert.Contains(forwarded, listing.Query); + } + } + } + [TestMethod] public async Task LocksDisabled_RelaysExactlyAsBefore() { diff --git a/GitLfsCache.Tests/Integration/StubUpstream.cs b/GitLfsCache.Tests/Integration/StubUpstream.cs index 6f3562f..4bcb8e6 100644 --- a/GitLfsCache.Tests/Integration/StubUpstream.cs +++ b/GitLfsCache.Tests/Integration/StubUpstream.cs @@ -91,7 +91,8 @@ protected override async Task SendAsync( request.Headers.TryGetValues("Authorization", out IEnumerable? authorization) ? string.Join(",", authorization) : null, - request.Headers.Range?.ToString())); + request.Headers.Range?.ToString(), + request.RequestUri!.Query)); } if (path.EndsWith("/unlock", StringComparison.Ordinal)) @@ -431,5 +432,6 @@ private HttpResponseMessage BuildUploadResponse( /// The absolute path. /// The Authorization header, or null when absent. /// The Range header, or null when absent. - internal sealed record RecordedRequest(string Method, string Path, string? Authorization, string? Range); + /// The query string, including its leading ?, or empty when absent. + internal sealed record RecordedRequest(string Method, string Path, string? Authorization, string? Range, string Query); } diff --git a/GitLfsCache.Tests/Locks/CredentialAdmissionTests.cs b/GitLfsCache.Tests/Locks/CredentialAdmissionTests.cs index 403fb7d..552b108 100644 --- a/GitLfsCache.Tests/Locks/CredentialAdmissionTests.cs +++ b/GitLfsCache.Tests/Locks/CredentialAdmissionTests.cs @@ -33,7 +33,7 @@ public void IsAdmitted_BeforeAnyUpstreamSuccess_IsFalse() // The whole point: nothing is admitted until upstream actually said yes. (CredentialAdmission admission, _) = Build(); - Assert.IsFalse(admission.IsAdmitted("github", Repository, Credential)); + Assert.IsFalse(admission.IsAdmitted("github", Repository, null, Credential)); } [TestMethod] @@ -41,9 +41,9 @@ public void IsAdmitted_AfterAdmit_IsTrue() { (CredentialAdmission admission, _) = Build(); - admission.Admit("github", Repository, Credential); + admission.Admit("github", Repository, null, Credential); - Assert.IsTrue(admission.IsAdmitted("github", Repository, Credential)); + Assert.IsTrue(admission.IsAdmitted("github", Repository, null, Credential)); } [TestMethod] @@ -51,12 +51,12 @@ public void IsAdmitted_AfterTheTtl_IsFalseAgain() { (CredentialAdmission admission, FakeTimeProvider time) = Build(TimeSpan.FromMinutes(1)); - admission.Admit("github", Repository, Credential); + admission.Admit("github", Repository, null, Credential); time.Advance(TimeSpan.FromMinutes(1)); // This is the window in which a credential revoked upstream still reads listings. It has to // actually close. - Assert.IsFalse(admission.IsAdmitted("github", Repository, Credential)); + Assert.IsFalse(admission.IsAdmitted("github", Repository, null, Credential)); } [TestMethod] @@ -64,10 +64,10 @@ public void IsAdmitted_JustBeforeTheTtl_IsStillTrue() { (CredentialAdmission admission, FakeTimeProvider time) = Build(TimeSpan.FromMinutes(1)); - admission.Admit("github", Repository, Credential); + admission.Admit("github", Repository, null, Credential); time.Advance(TimeSpan.FromSeconds(59)); - Assert.IsTrue(admission.IsAdmitted("github", Repository, Credential)); + Assert.IsTrue(admission.IsAdmitted("github", Repository, null, Credential)); } [TestMethod] @@ -75,9 +75,9 @@ public void IsAdmitted_ADifferentCredential_IsNotAdmitted() { (CredentialAdmission admission, _) = Build(); - admission.Admit("github", Repository, Credential); + admission.Admit("github", Repository, null, Credential); - Assert.IsFalse(admission.IsAdmitted("github", Repository, "Basic c29tZW9uZTplbHNl")); + Assert.IsFalse(admission.IsAdmitted("github", Repository, null, "Basic c29tZW9uZTplbHNl")); } [TestMethod] @@ -86,9 +86,9 @@ public void IsAdmitted_ADifferentRepository_IsNotAdmitted() // Admission is per repository. Read access to one proves nothing about another. (CredentialAdmission admission, _) = Build(); - admission.Admit("github", Repository, Credential); + admission.Admit("github", Repository, null, Credential); - Assert.IsFalse(admission.IsAdmitted("github", "owner/other.git/info/lfs", Credential)); + Assert.IsFalse(admission.IsAdmitted("github", "owner/other.git/info/lfs", null, Credential)); } [TestMethod] @@ -96,9 +96,25 @@ public void IsAdmitted_ADifferentUpstream_IsNotAdmitted() { (CredentialAdmission admission, _) = Build(); - admission.Admit("github", Repository, Credential); + admission.Admit("github", Repository, null, Credential); - Assert.IsFalse(admission.IsAdmitted("ado", Repository, Credential)); + Assert.IsFalse(admission.IsAdmitted("ado", Repository, null, Credential)); + } + + [TestMethod] + [DataRow("refs/heads/feature", DisplayName = "Another ref")] + [DataRow(null, DisplayName = "No ref")] + [DataRow("", DisplayName = "An empty ref")] + public void IsAdmitted_ADifferentRef_IsNotAdmitted(string? reference) + { + // The locking API treats the ref as an authentication input, so upstream accepting a + // credential under one ref says nothing about another. + (CredentialAdmission admission, _) = Build(); + + admission.Admit("github", Repository, "refs/heads/main", Credential); + + Assert.IsTrue(admission.IsAdmitted("github", Repository, "refs/heads/main", Credential)); + Assert.IsFalse(admission.IsAdmitted("github", Repository, reference, Credential)); } [TestMethod] @@ -108,9 +124,9 @@ public void Key_CannotBeConfusedAcrossFields() // "b" would hash identically and one repository's admission would serve another's. (CredentialAdmission admission, _) = Build(); - admission.Admit("github", "a/b", Credential); + admission.Admit("github", "a/b", null, Credential); - Assert.IsFalse(admission.IsAdmitted("github/a", "b", Credential)); + Assert.IsFalse(admission.IsAdmitted("github/a", "b", null, Credential)); } [TestMethod] @@ -121,9 +137,9 @@ public void IsAdmitted_NoCredential_IsNeverAdmitted(string? authorization) // Admitting an anonymous caller would mean serving a listing to someone who proved nothing. (CredentialAdmission admission, _) = Build(); - admission.Admit("github", Repository, authorization); + admission.Admit("github", Repository, null, authorization); - Assert.IsFalse(admission.IsAdmitted("github", Repository, authorization)); + Assert.IsFalse(admission.IsAdmitted("github", Repository, null, authorization)); } [TestMethod] @@ -131,11 +147,11 @@ public void Admit_Twice_ExtendsTheWindowFromTheSecondTime() { (CredentialAdmission admission, FakeTimeProvider time) = Build(TimeSpan.FromMinutes(1)); - admission.Admit("github", Repository, Credential); + admission.Admit("github", Repository, null, Credential); time.Advance(TimeSpan.FromSeconds(50)); - admission.Admit("github", Repository, Credential); + admission.Admit("github", Repository, null, Credential); time.Advance(TimeSpan.FromSeconds(50)); - Assert.IsTrue(admission.IsAdmitted("github", Repository, Credential)); + Assert.IsTrue(admission.IsAdmitted("github", Repository, null, Credential)); } } diff --git a/GitLfsCache/Locks/CredentialAdmission.cs b/GitLfsCache/Locks/CredentialAdmission.cs index 3b6468b..f18a65a 100644 --- a/GitLfsCache/Locks/CredentialAdmission.cs +++ b/GitLfsCache/Locks/CredentialAdmission.cs @@ -3,6 +3,7 @@ namespace ktsu.GitLfsCache.Locks; using System.Collections.Concurrent; +using System.Globalization; using System.Security.Cryptography; using System.Text; using ktsu.GitLfsCache.Configuration; @@ -42,7 +43,7 @@ public sealed class CredentialAdmission( private readonly Lock _hashGate = new(); /// - public bool IsAdmitted(string upstream, string repositoryPath, string? authorization) + public bool IsAdmitted(string upstream, string repositoryPath, string? reference, string? authorization) { // An anonymous caller is never admitted. Upstream would refuse it, and admitting it here would // mean a listing served to someone who never proved anything. @@ -51,7 +52,7 @@ public bool IsAdmitted(string upstream, string repositoryPath, string? authoriza return false; } - string key = Key(upstream, repositoryPath, authorization); + string key = Key(upstream, repositoryPath, reference, authorization); if (!_admitted.TryGetValue(key, out DateTimeOffset expiry)) { @@ -70,14 +71,14 @@ public bool IsAdmitted(string upstream, string repositoryPath, string? authoriza } /// - public void Admit(string upstream, string repositoryPath, string? authorization) + public void Admit(string upstream, string repositoryPath, string? reference, string? authorization) { if (string.IsNullOrEmpty(authorization)) { return; } - _admitted[Key(upstream, repositoryPath, authorization)] = + _admitted[Key(upstream, repositoryPath, reference, authorization)] = timeProvider.GetUtcNow() + options.Value.Locks.AdmissionTtl; if (_admitted.Count > SweepThreshold) @@ -106,13 +107,18 @@ private void Sweep() /// Derives the entry key for one credential and repository. /// /// - /// The three parts are separated by a character that cannot appear in an upstream key, so no two - /// different triples can produce the same input. Without that, an upstream and repository could be + /// The parts are separated by a character that cannot appear in an upstream key, so no two + /// different combinations can produce the same input. Without that, an upstream and repository could be /// re-split to match a different pair. /// - private string Key(string upstream, string repositoryPath, string authorization) + private string Key(string upstream, string repositoryPath, string? reference, string authorization) { - byte[] input = Encoding.UTF8.GetBytes($"{upstream}\n{repositoryPath}\n{authorization}"); + // The ref goes last and length-prefixed, since unlike the other parts it comes straight from a + // query string and may contain the separator. -1 keeps "no ref" apart from an empty one. + string scope = reference is null + ? "-1:" + : string.Create(CultureInfo.InvariantCulture, $"{reference.Length}:{reference}"); + byte[] input = Encoding.UTF8.GetBytes($"{upstream}\n{repositoryPath}\n{authorization}\n{scope}"); // HMACSHA256 holds mutable state across ComputeHash, so one shared instance needs a gate. The // alternative, an instance per call, allocates on a path taken on every lock request. diff --git a/GitLfsCache/Locks/ICredentialAdmission.cs b/GitLfsCache/Locks/ICredentialAdmission.cs index 0e66b58..e8ddd28 100644 --- a/GitLfsCache/Locks/ICredentialAdmission.cs +++ b/GitLfsCache/Locks/ICredentialAdmission.cs @@ -23,9 +23,13 @@ public interface ICredentialAdmission /// /// The upstream key. /// The repository the credential was accepted for. + /// + /// The ref the credential was presented with, or null for none. Upstream may accept a credential + /// for one ref and refuse it for another, so an admission for one does not carry over. + /// /// The client's Authorization header, exactly as sent. /// when an unexpired admission exists. - public bool IsAdmitted(string upstream, string repositoryPath, string? authorization); + public bool IsAdmitted(string upstream, string repositoryPath, string? reference, string? authorization); /// /// Records that upstream accepted this credential for this repository. @@ -36,6 +40,10 @@ public interface ICredentialAdmission /// /// The upstream key. /// The repository the credential was accepted for. + /// + /// The ref the credential was presented with, or null for none. Upstream may accept a credential + /// for one ref and refuse it for another, so an admission for one does not carry over. + /// /// The client's Authorization header, exactly as sent. - public void Admit(string upstream, string repositoryPath, string? authorization); + public void Admit(string upstream, string repositoryPath, string? reference, string? authorization); } diff --git a/GitLfsCache/Locks/LockListRefresher.cs b/GitLfsCache/Locks/LockListRefresher.cs index 5077d75..0567d3c 100644 --- a/GitLfsCache/Locks/LockListRefresher.cs +++ b/GitLfsCache/Locks/LockListRefresher.cs @@ -52,6 +52,7 @@ public async Task ProbeAsync( using HttpRequestMessage request = UpstreamRequests.BuildLockListRequest( upstreamBase, key.RepositoryPath, + key.Ref, cursor: null, limit: 1, authorization); @@ -83,6 +84,7 @@ public async Task RefreshAsync( using HttpRequestMessage request = UpstreamRequests.BuildLockListRequest( upstreamBase, key.RepositoryPath, + key.Ref, cursor, limit: null, authorization); diff --git a/GitLfsCache/Locks/LockListService.cs b/GitLfsCache/Locks/LockListService.cs index b287f85..8929ded 100644 --- a/GitLfsCache/Locks/LockListService.cs +++ b/GitLfsCache/Locks/LockListService.cs @@ -65,7 +65,7 @@ public async Task ResolveAsync( // A caller already admitted, with a snapshot still inside its lifetime, is the steady state and // costs upstream nothing at all. This is the whole point of the subsystem. - if (usable && admission.IsAdmitted(key.Upstream, key.RepositoryPath, authorization)) + if (usable && admission.IsAdmitted(key.Upstream, key.RepositoryPath, key.Ref, authorization)) { metrics.RecordLockListHit(key.Upstream); return LockListOutcome.Serve(current!); @@ -110,7 +110,7 @@ private async Task ProbeAsync( return LockListOutcome.Relay(); } - admission.Admit(key.Upstream, key.RepositoryPath, authorization); + admission.Admit(key.Upstream, key.RepositoryPath, key.Ref, authorization); return LockListOutcome.Serve(current); } @@ -155,7 +155,7 @@ private async Task RefreshAsync( snapshots.Publish(key, result.Snapshot!); // The walk succeeding is itself upstream's answer that this caller may read these locks. - admission.Admit(key.Upstream, key.RepositoryPath, authorization); + admission.Admit(key.Upstream, key.RepositoryPath, key.Ref, authorization); ticket.Complete(published: true); return LockListOutcome.Serve(result.Snapshot!); diff --git a/GitLfsCache/Upstreams/UpstreamRequests.cs b/GitLfsCache/Upstreams/UpstreamRequests.cs index 9d08680..944fc13 100644 --- a/GitLfsCache/Upstreams/UpstreamRequests.cs +++ b/GitLfsCache/Upstreams/UpstreamRequests.cs @@ -84,6 +84,11 @@ public static HttpRequestMessage BuildBatchRequest( /// /// The configured upstream base URL. /// The path between the upstream key and /locks. + /// + /// The ref the client listed under, forwarded as refspec, or null when it sent none. The + /// locking API treats the ref as an authentication input, so a walk or probe made on a client's + /// behalf has to present the same one the client did. + /// /// Upstream's cursor for the page to fetch, or null for the first. /// A page size to request, or null to let upstream choose. /// The client's Authorization header, forwarded unchanged. @@ -91,6 +96,7 @@ public static HttpRequestMessage BuildBatchRequest( public static HttpRequestMessage BuildLockListRequest( Uri upstreamBase, string repositoryPath, + string? refspec, string? cursor, int? limit, string? authorization) @@ -100,6 +106,11 @@ public static HttpRequestMessage BuildLockListRequest( List query = []; + if (refspec is not null) + { + query.Add($"refspec={Uri.EscapeDataString(refspec)}"); + } + if (!string.IsNullOrEmpty(cursor)) { query.Add($"cursor={Uri.EscapeDataString(cursor)}");