From 450a46acc715e999f51eb6a141f8ff74ce80a897 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 3 Nov 2026 18:36:19 +0000 Subject: [PATCH 1/2] Skip a branch whose name is not UTF-8 instead of failing every ref listing [patch] RefResolver.ListAsync read for-each-ref output through GitRunner's strict UTF-8 decoding, so one ref name git allows but UTF-8 cannot read failed the whole listing, and every /state and /branches request for that repository returned 502 refs-unreadable until the branch was deleted. GitInvocation.ReadStandardOutputAsBytes now returns stdout one character per byte. RefResolver decodes each line on its own, skips a line that is not UTF-8, and logs a warning with the hex of the ref name. Every other branch is still served. Fixes #114 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017vfJW24ManL6mVAsf34qvT --- GitBranchStateCache.Tests/Fakes/ListLogger.cs | 31 +++++++++ .../Git/RealRepositoryTests.cs | 32 +++++++++ .../Integration/ScriptedGit.cs | 22 +++++- .../Integration/StateFlowTests.cs | 27 ++++++++ .../Refs/RefResolverTests.cs | 15 +++++ GitBranchStateCache/Git/GitInvocation.cs | 12 ++++ GitBranchStateCache/Git/GitRunner.cs | 2 +- GitBranchStateCache/Refs/RefLog.cs | 17 +++++ GitBranchStateCache/Refs/RefResolver.cs | 67 ++++++++++++++++++- 9 files changed, 221 insertions(+), 4 deletions(-) create mode 100644 GitBranchStateCache.Tests/Fakes/ListLogger.cs create mode 100644 GitBranchStateCache/Refs/RefLog.cs diff --git a/GitBranchStateCache.Tests/Fakes/ListLogger.cs b/GitBranchStateCache.Tests/Fakes/ListLogger.cs new file mode 100644 index 0000000..e36ac70 --- /dev/null +++ b/GitBranchStateCache.Tests/Fakes/ListLogger.cs @@ -0,0 +1,31 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.GitBranchStateCache.Tests.Fakes; + +using Microsoft.Extensions.Logging; + +/// +/// A logger that records every formatted message it is given. +/// +/// The category. +/// Where the messages go. +internal sealed class ListLogger(List messages) : ILogger +{ + public IDisposable? BeginScope(TState state) + where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log( + LogLevel logLevel, + EventId eventId, + TState state, + Exception? exception, + Func formatter) + { + lock (messages) + { + messages.Add(formatter(state, exception)); + } + } +} diff --git a/GitBranchStateCache.Tests/Git/RealRepositoryTests.cs b/GitBranchStateCache.Tests/Git/RealRepositoryTests.cs index a264efa..3bab47d 100644 --- a/GitBranchStateCache.Tests/Git/RealRepositoryTests.cs +++ b/GitBranchStateCache.Tests/Git/RealRepositoryTests.cs @@ -9,6 +9,7 @@ namespace ktsu.GitBranchStateCache.Tests.Git; using ktsu.GitBranchStateCache.Git; using ktsu.GitBranchStateCache.Mirrors; using ktsu.GitBranchStateCache.Refs; +using ktsu.GitBranchStateCache.Tests.Fakes; using Microsoft.Extensions.Options; using Microsoft.VisualStudio.TestTools.UnitTesting; @@ -109,6 +110,37 @@ public async Task RefResolver_ReadsEveryBranchWithItsTip() Assert.IsTrue(branches!.Any(branch => branch.Name == "origin/feature/ui" && branch.Tip == _featureTip)); } + [TestMethod] + public async Task RefResolver_ABranchNameThatIsNotUtf8_IsSkipped() + { + // A copy of the mirror, so the bad ref does not leak into the other tests. The ref goes in + // through packed-refs because a file name with a lone 0xE9 byte cannot be spelled from .NET. + string copy = Path.Combine(Path.GetDirectoryName(_mirror)!, $"latin1-{Guid.NewGuid():N}.git"); + Git(Path.GetDirectoryName(_mirror)!, "clone", "--bare", "--quiet", _mirror, copy); + byte[] line = [.. Encoding.ASCII.GetBytes($"{_featureTip} refs/heads/feature/caf"), 0xE9, (byte)'\n']; + string packedRefs = Path.Combine(copy, "packed-refs"); + + // The appended line is out of order, so the header must stop promising a sorted file, or git + // binary-searches past it. + string existing = (await File.ReadAllTextAsync(packedRefs)).Replace(" sorted", string.Empty, StringComparison.Ordinal); + await File.WriteAllBytesAsync(packedRefs, [.. Encoding.ASCII.GetBytes(existing), .. line]); + Assert.Contains("caf", Git(copy, "for-each-ref", "refs/heads/feature/"), "git did not take the fixture ref."); + + List skipped = []; + ListLogger logger = new(skipped); + + IReadOnlyList? branches = await new RefResolver(Runner, Options.Create(Settings), logger) + .ListAsync(copy, CancellationToken.None); + + Assert.IsNotNull(branches, "One undecodable branch failed the whole listing."); + Assert.IsTrue(branches!.Any(branch => branch.Name == "origin/main" && branch.Tip == _mainTip)); + Assert.IsTrue(branches!.Any(branch => branch.Name == "origin/feature/ui")); + Assert.IsFalse(branches!.Any(branch => branch.Name.Contains("caf", StringComparison.Ordinal))); + Assert.HasCount(1, skipped); + Assert.Contains("72656673", skipped[0]); + Assert.Contains("636166E9", skipped[0]); + } + [TestMethod] public async Task RefResolver_ABrokenMirrorInsideAnotherRepository_ReadsNothing() { diff --git a/GitBranchStateCache.Tests/Integration/ScriptedGit.cs b/GitBranchStateCache.Tests/Integration/ScriptedGit.cs index 83bea40..8f4da23 100644 --- a/GitBranchStateCache.Tests/Integration/ScriptedGit.cs +++ b/GitBranchStateCache.Tests/Integration/ScriptedGit.cs @@ -3,6 +3,7 @@ namespace ktsu.GitBranchStateCache.Tests.Integration; using System.Collections.Concurrent; +using System.Text; using ktsu.GitBranchStateCache.Git; using Testably.Abstractions.Testing; @@ -34,6 +35,12 @@ internal sealed class ScriptedGit(MockFileSystem fileSystem) : IGitRunner /// Gets the commits the mirror holds. public HashSet Commits { get; } = new(StringComparer.Ordinal); + /// + /// Gets extra for-each-ref lines, one character per byte, for refs whose names are not UTF-8 + /// and so cannot be held in . + /// + public List RawRefLines { get; } = []; + /// Gets or sets whether an ls-remote succeeds. public bool AdmitsCredentials { get; set; } = true; @@ -76,7 +83,7 @@ public async Task RunAsync(GitInvocation invocation, CancellationToke "commit-graph" => CommitGraphFails ? new GitResult(128, string.Empty, "fatal: could not write commit-graph", TimedOut: false) : Success(string.Empty), - "for-each-ref" => Success(ForEachRef()), + "for-each-ref" => ForEachRef(invocation), "cat-file" => CatFile(arguments), "merge-base" => MergeBase(arguments), "diff-tree" => Diff(arguments), @@ -94,6 +101,19 @@ private GitResult Clone(IReadOnlyList arguments) return Success(string.Empty); } + private GitResult ForEachRef(GitInvocation invocation) + { + if (invocation.ReadStandardOutputAsBytes) + { + return Success(Encoding.Latin1.GetString(Encoding.UTF8.GetBytes(ForEachRef())) + string.Concat(RawRefLines)); + } + + // What GitRunner's strict decoding does with a byte that is not UTF-8. + return RawRefLines.Count > 0 + ? new GitResult(-1, string.Empty, "git produced output that is not valid UTF-8.", TimedOut: false) + : Success(ForEachRef()); + } + private string ForEachRef() => string.Concat(Branches.Select(branch => $"{branch.Value} refs/heads/{branch.Key}\n")); diff --git a/GitBranchStateCache.Tests/Integration/StateFlowTests.cs b/GitBranchStateCache.Tests/Integration/StateFlowTests.cs index 48fe198..ed45b2f 100644 --- a/GitBranchStateCache.Tests/Integration/StateFlowTests.cs +++ b/GitBranchStateCache.Tests/Integration/StateFlowTests.cs @@ -258,6 +258,33 @@ public async Task Post_State_WithNoBranchPatterns_Is400() Assert.AreEqual(HttpStatusCode.BadRequest, response.StatusCode); } + [TestMethod] + public async Task ABranchNameThatIsNotUtf8_IsSkipped_AndEveryOtherBranchIsStillServed() + { + // "caf\xe9": a Latin-1 name git accepts. It used to fail the whole ref listing, so every request + // for the repository was a 502 until the branch was deleted upstream (#114). + await using ServiceFixture fixture = await ServiceFixture.StartAsync(); + Seed(fixture.Git); + fixture.Git.RawRefLines.Add($"{MainTip} refs/heads/feature/caf\u00e9\n"); + + using HttpRequestMessage branchesRequest = new( + HttpMethod.Get, + "/v1/github/studio/game.git/branches?pattern=origin/*"); + branchesRequest.Headers.Authorization = AuthenticationHeaderValue.Parse(Credential); + + using HttpResponseMessage branches = await fixture.Client.SendAsync(branchesRequest); + JsonElement branchesBody = await ReadAsync(branches); + + Assert.AreEqual(HttpStatusCode.OK, branches.StatusCode); + string?[] names = [.. branchesBody.GetProperty("branches").EnumerateArray() + .Select(branch => branch.GetProperty("name").GetString())]; + CollectionAssert.AreEquivalent(new[] { "origin/main", "origin/release/1.0" }, names); + + using HttpResponseMessage state = await fixture.Client.SendAsync(StateRequest(Body())); + + Assert.AreEqual(HttpStatusCode.OK, state.StatusCode); + } + [TestMethod] [DataRow("HEAD")] [DataRow("main@{1}")] diff --git a/GitBranchStateCache.Tests/Refs/RefResolverTests.cs b/GitBranchStateCache.Tests/Refs/RefResolverTests.cs index fc29f7a..32f0103 100644 --- a/GitBranchStateCache.Tests/Refs/RefResolverTests.cs +++ b/GitBranchStateCache.Tests/Refs/RefResolverTests.cs @@ -22,6 +22,21 @@ [.. patterns.Select(pattern => return parsed!; })]; + [TestMethod] + public void DecodeLines_DropsOnlyTheLineThatIsNotUtf8() + { + // One character per byte: "caf\u00e9" is the single byte 0xE9, and "\u00c3\u00a9" is the UTF-8 + // encoding of é, which must still come through as é. + string output = $"{MainTip} refs/heads/main\n{ReleaseTip} refs/heads/caf\u00e9\n{ReleaseTip} refs/heads/caf\u00c3\u00a9\n"; + List skipped = []; + + string decoded = RefResolver.DecodeLines(output, skipped.Add); + + Assert.AreEqual($"{MainTip} refs/heads/main\n{ReleaseTip} refs/heads/café\n", decoded); + Assert.HasCount(1, skipped); + Assert.AreEqual("726566732F68656164732F636166E9", skipped[0]); + } + [TestMethod] public void Parse_ReportsBranchesUnderTheRemoteName() { diff --git a/GitBranchStateCache/Git/GitInvocation.cs b/GitBranchStateCache/Git/GitInvocation.cs index 33451d2..dbe2010 100644 --- a/GitBranchStateCache/Git/GitInvocation.cs +++ b/GitBranchStateCache/Git/GitInvocation.cs @@ -50,6 +50,18 @@ public sealed class GitInvocation /// public bool DiscardStandardOutput { get; init; } + /// + /// Gets whether standard output is returned undecoded, as one character per byte. + /// + /// + /// For a command whose output holds several independent records, any of which may not be UTF-8. + /// Strict decoding of the whole output would let one record fail every other one, so the caller + /// splits the output into records first and decodes each one itself + /// (ktsu-dev/GitBranchStateCache#114). Each character's value is the byte git wrote, so + /// turns the text back into those bytes exactly. + /// + public bool ReadStandardOutputAsBytes { get; init; } + /// Gets how long the command may run before it is killed. public required TimeSpan Timeout { get; init; } } diff --git a/GitBranchStateCache/Git/GitRunner.cs b/GitBranchStateCache/Git/GitRunner.cs index c833211..bf48941 100644 --- a/GitBranchStateCache/Git/GitRunner.cs +++ b/GitBranchStateCache/Git/GitRunner.cs @@ -83,7 +83,7 @@ public async Task RunAsync(GitInvocation invocation, CancellationToke : new( chunk => standardOutput.Append(chunk), chunk => standardError.Append(chunk), - StrictUtf8); + invocation.ReadStandardOutputAsBytes ? Encoding.Latin1 : StrictUtf8); CommandOptions commandOptions = new() { diff --git a/GitBranchStateCache/Refs/RefLog.cs b/GitBranchStateCache/Refs/RefLog.cs new file mode 100644 index 0000000..02c0c1d --- /dev/null +++ b/GitBranchStateCache/Refs/RefLog.cs @@ -0,0 +1,17 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.GitBranchStateCache.Refs; + +using Microsoft.Extensions.Logging; + +/// +/// Source-generated log messages for reading refs. +/// +internal static partial class RefLog +{ + [LoggerMessage( + EventId = 4000, + Level = LogLevel.Warning, + Message = "Skipped a ref in the mirror at '{Directory}' whose name is not valid UTF-8 (bytes {RefNameHex}). Every other branch is still served; rename or delete it upstream to serve it.")] + public static partial void UndecodableRefSkipped(ILogger logger, string directory, string refNameHex); +} diff --git a/GitBranchStateCache/Refs/RefResolver.cs b/GitBranchStateCache/Refs/RefResolver.cs index 9d5df30..7ff7fa4 100644 --- a/GitBranchStateCache/Refs/RefResolver.cs +++ b/GitBranchStateCache/Refs/RefResolver.cs @@ -2,8 +2,11 @@ namespace ktsu.GitBranchStateCache.Refs; +using System.Text; using ktsu.GitBranchStateCache.Configuration; using ktsu.GitBranchStateCache.Git; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Options; /// @@ -17,10 +20,20 @@ namespace ktsu.GitBranchStateCache.Refs; /// /// Runs git. /// The configured options. -public sealed class RefResolver(IGitRunner runner, IOptions options) : IRefResolver +/// Where a skipped ref is reported, or null to report nothing. +public sealed class RefResolver( + IGitRunner runner, + IOptions options, + ILogger? logger = null) : IRefResolver { private const string HeadPrefix = "refs/heads/"; + private static readonly Encoding StrictUtf8 = new UTF8Encoding( + encoderShouldEmitUTF8Identifier: false, + throwOnInvalidBytes: true); + + private readonly ILogger _logger = logger ?? NullLogger.Instance; + /// public async Task?> ListAsync(string directory, CancellationToken cancellationToken) { @@ -33,6 +46,10 @@ public sealed class RefResolver(IGitRunner runner, IOptions RefLog.UndecodableRefSkipped(_logger, directory, undecodable)); + + return Parse(decoded, options.Value.RemoteName); + } + + /// + /// Decodes for-each-ref output read as bytes, one line at a time, dropping any line that is + /// not UTF-8. + /// + /// + /// A name that cannot be decoded cannot be written into a response faithfully either, so the ref + /// is skipped rather than served under a name with a replacement character in it, which would look + /// fine and match nothing the client has. + /// + /// The output, one character per byte. + /// Told the hex of each ref name that was dropped. + /// The lines that decoded, each followed by a line feed. + internal static string DecodeLines(string output, Action skipped) + { + Ensure.NotNull(output); + Ensure.NotNull(skipped); + + StringBuilder decoded = new(); + + foreach (string line in output.Split('\n')) + { + if (line.Length == 0) + { + continue; + } + + byte[] bytes = Encoding.Latin1.GetBytes(line); + + try + { + decoded.Append(StrictUtf8.GetString(bytes)).Append('\n'); + } + catch (DecoderFallbackException) + { + int separator = Array.IndexOf(bytes, (byte)' '); + skipped(Convert.ToHexString(bytes, separator + 1, bytes.Length - separator - 1)); + } + } + + return decoded.ToString(); } /// From 6bbde1242005db5d0d81ad04d20e6b8ecec28539 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 3 Nov 2026 18:38:15 +0000 Subject: [PATCH 2/2] Move the #114 end-to-end test to the end of StateFlowTests Keeps it clear of the lines #117 adds, so the two PRs merge in either order. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017vfJW24ManL6mVAsf34qvT --- .../Integration/StateFlowTests.cs | 54 +++++++++---------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/GitBranchStateCache.Tests/Integration/StateFlowTests.cs b/GitBranchStateCache.Tests/Integration/StateFlowTests.cs index ed45b2f..d5d6b1d 100644 --- a/GitBranchStateCache.Tests/Integration/StateFlowTests.cs +++ b/GitBranchStateCache.Tests/Integration/StateFlowTests.cs @@ -258,33 +258,6 @@ public async Task Post_State_WithNoBranchPatterns_Is400() Assert.AreEqual(HttpStatusCode.BadRequest, response.StatusCode); } - [TestMethod] - public async Task ABranchNameThatIsNotUtf8_IsSkipped_AndEveryOtherBranchIsStillServed() - { - // "caf\xe9": a Latin-1 name git accepts. It used to fail the whole ref listing, so every request - // for the repository was a 502 until the branch was deleted upstream (#114). - await using ServiceFixture fixture = await ServiceFixture.StartAsync(); - Seed(fixture.Git); - fixture.Git.RawRefLines.Add($"{MainTip} refs/heads/feature/caf\u00e9\n"); - - using HttpRequestMessage branchesRequest = new( - HttpMethod.Get, - "/v1/github/studio/game.git/branches?pattern=origin/*"); - branchesRequest.Headers.Authorization = AuthenticationHeaderValue.Parse(Credential); - - using HttpResponseMessage branches = await fixture.Client.SendAsync(branchesRequest); - JsonElement branchesBody = await ReadAsync(branches); - - Assert.AreEqual(HttpStatusCode.OK, branches.StatusCode); - string?[] names = [.. branchesBody.GetProperty("branches").EnumerateArray() - .Select(branch => branch.GetProperty("name").GetString())]; - CollectionAssert.AreEquivalent(new[] { "origin/main", "origin/release/1.0" }, names); - - using HttpResponseMessage state = await fixture.Client.SendAsync(StateRequest(Body())); - - Assert.AreEqual(HttpStatusCode.OK, state.StatusCode); - } - [TestMethod] [DataRow("HEAD")] [DataRow("main@{1}")] @@ -567,4 +540,31 @@ public async Task Post_State_ReportsHowOldTheRefsAre() Assert.AreEqual(fixture.Time.GetUtcNow(), body.GetProperty("refsAsOf").GetDateTimeOffset()); } + + [TestMethod] + public async Task ABranchNameThatIsNotUtf8_IsSkipped_AndEveryOtherBranchIsStillServed() + { + // "caf\xe9": a Latin-1 name git accepts. It used to fail the whole ref listing, so every request + // for the repository was a 502 until the branch was deleted upstream (#114). + await using ServiceFixture fixture = await ServiceFixture.StartAsync(); + Seed(fixture.Git); + fixture.Git.RawRefLines.Add($"{MainTip} refs/heads/feature/caf\u00e9\n"); + + using HttpRequestMessage branchesRequest = new( + HttpMethod.Get, + "/v1/github/studio/game.git/branches?pattern=origin/*"); + branchesRequest.Headers.Authorization = AuthenticationHeaderValue.Parse(Credential); + + using HttpResponseMessage branches = await fixture.Client.SendAsync(branchesRequest); + JsonElement branchesBody = await ReadAsync(branches); + + Assert.AreEqual(HttpStatusCode.OK, branches.StatusCode); + string?[] names = [.. branchesBody.GetProperty("branches").EnumerateArray() + .Select(branch => branch.GetProperty("name").GetString())]; + CollectionAssert.AreEquivalent(new[] { "origin/main", "origin/release/1.0" }, names); + + using HttpResponseMessage state = await fixture.Client.SendAsync(StateRequest(Body())); + + Assert.AreEqual(HttpStatusCode.OK, state.StatusCode); + } }