From 80a2708f65438acaf10d9f72e6cd744ea5cb2101 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 8 Oct 2026 07:35:48 +0000 Subject: [PATCH] Store objects for an upstream key with a dot in it, such as gitlab.com [patch] ObjectStore accepted only upstream keys made of [A-Za-z0-9_-] and threw ArgumentException for anything else, though startup validation, the registry and the route parser all accept such a key. With an upstream named gitlab.com the batch call worked and then every object download and upload returned 500. The store now maps a key to its directory name instead of rejecting it. A plain key is its own name, so existing stores stay readable. Any other key has each UTF-8 byte outside that set written as %XX, which is deterministic and reversible, cannot collide with a plain name since it always contains a %, and leaves no separator or parent reference that could reach outside the root. Enumerate maps the name back to the key. Only an empty key is still refused. Fixes #56 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WY7QGzbH6kE2oceF1hP8fh --- .../Integration/ProxyFlowTests.cs | 32 ++++++++++ GitLfsCache.Tests/Storage/ObjectStoreTests.cs | 56 ++++++++++++++-- GitLfsCache/Storage/ObjectStore.cs | 64 +++++++++++++++++-- .../specs/2026-08-18-gitlfscache-design.md | 2 + 4 files changed, 143 insertions(+), 11 deletions(-) diff --git a/GitLfsCache.Tests/Integration/ProxyFlowTests.cs b/GitLfsCache.Tests/Integration/ProxyFlowTests.cs index 76c30d8..e665f96 100644 --- a/GitLfsCache.Tests/Integration/ProxyFlowTests.cs +++ b/GitLfsCache.Tests/Integration/ProxyFlowTests.cs @@ -332,6 +332,38 @@ public async Task Upload_IsRelayedUpstreamAndStoredOnTheWayThrough() Assert.IsTrue(fixture.Store.Exists("github", oid), "A pushed object should be cached for the next fetch."); } + [TestMethod] + public async Task DottedUpstreamKey_DownloadsAndUploadsAreStored() + { + const string dottedPath = "/gitlab.com/owner/repo.git/info/lfs"; + await using ProxyFixture fixture = await ProxyFixture.StartAsync(new(StringComparer.Ordinal) + { + ["GitLfsCache:Upstreams:gitlab.com:BaseUrl"] = "https://upstream.example", + ["GitLfsCache:Upstreams:gitlab.com:Repositories:0"] = "**", + }); + using HttpClient client = fixture.Client; + + (byte[] downloaded, string downloadOid) = Object("fetched through a dotted upstream"); + fixture.Upstream.AddObject(downloadOid, downloaded); + JsonNode downloadBatch = await PostBatchAsync(fixture, "download", downloadOid, downloaded.Length, lfsPath: dottedPath); + + using HttpResponseMessage download = await client.GetAsync(Relative(HrefOf(downloadBatch, "download"))); + + Assert.AreEqual(HttpStatusCode.OK, download.StatusCode); + CollectionAssert.AreEqual(downloaded, await download.Content.ReadAsByteArrayAsync()); + Assert.IsTrue(fixture.Store.Exists("gitlab.com", downloadOid)); + + (byte[] uploaded, string uploadOid) = Object("pushed through a dotted upstream"); + JsonNode uploadBatch = await PostBatchAsync(fixture, "upload", uploadOid, uploaded.Length, lfsPath: dottedPath); + using ByteArrayContent body = new(uploaded); + + using HttpResponseMessage upload = await client.PutAsync(Relative(HrefOf(uploadBatch, "upload")), body); + + Assert.AreEqual(HttpStatusCode.OK, upload.StatusCode); + CollectionAssert.AreEqual(uploaded, fixture.Upstream.Uploaded[uploadOid]); + Assert.IsTrue(fixture.Store.Exists("gitlab.com", uploadOid)); + } + /// /// Puts a file where the upstream's staging directory belongs, so no staging file can be opened. /// diff --git a/GitLfsCache.Tests/Storage/ObjectStoreTests.cs b/GitLfsCache.Tests/Storage/ObjectStoreTests.cs index 653d220..331d5ad 100644 --- a/GitLfsCache.Tests/Storage/ObjectStoreTests.cs +++ b/GitLfsCache.Tests/Storage/ObjectStoreTests.cs @@ -263,14 +263,62 @@ public void TryOpenRead_MalformedOid_ReturnsFalse(string oid) } [TestMethod] + public void OpenStaging_EmptyUpstream_Throws() + { + (ObjectStore store, _, _) = Create(); + + Assert.ThrowsExactly(() => store.OpenStaging(string.Empty)); + } + + [TestMethod] + [DataRow("gitlab.com")] [DataRow("../escape")] [DataRow("with/slash")] - [DataRow("")] - public void OpenStaging_MalformedUpstream_Throws(string upstream) + [DataRow("with\\backslash")] + [DataRow("..")] + [DataRow("dé.example")] + public async Task AnyNonEmptyUpstream_IsStoredInOneDirectoryUnderTheRootAndReadBack(string upstream) { - (ObjectStore store, _, _) = Create(); + (ObjectStore store, MockFileSystem fileSystem, _) = Create(); + + string oid = await StoreAsync(store, upstream, $"stored under {upstream}"); + + Assert.IsTrue(store.Exists(upstream, oid)); + using (Stream? read = store.OpenRead(upstream, oid, out _)) + { + Assert.IsNotNull(read); + } + + string directory = fileSystem.Directory.EnumerateDirectories(Root).Single(); + Assert.AreEqual(Root, fileSystem.Path.GetDirectoryName(directory), "The key must not name a directory outside the root."); + Assert.AreEqual(ObjectStore.DirectoryNameFor(upstream), fileSystem.Path.GetFileName(directory)); + Assert.AreEqual(upstream, store.Enumerate().Single().Upstream); + } + + [TestMethod] + public void DirectoryNameFor_DottedKey_EscapesOnlyTheDot() + { + Assert.AreEqual("gitlab%2Ecom", ObjectStore.DirectoryNameFor("gitlab.com")); + } + + [TestMethod] + [DataRow("github")] + [DataRow("My_Upstream-2")] + public void DirectoryNameFor_PlainKey_IsTheKeyItself(string upstream) + { + // The layout a store written before escaping existed already has, so it stays readable. + Assert.AreEqual(upstream, ObjectStore.DirectoryNameFor(upstream)); + } + + [TestMethod] + public void DirectoryNameFor_KeysThatLookAlikeOnceEscaped_StayDistinct() + { + string[] keys = ["gitlab.com", "gitlab%2Ecom", "gitlab%252Ecom", "gitlab_com", "gitlab-com"]; + + string[] names = [.. keys.Select(ObjectStore.DirectoryNameFor)]; - Assert.ThrowsExactly(() => store.OpenStaging(upstream)); + Assert.HasCount(keys.Length, names.Distinct(StringComparer.Ordinal).ToList()); + CollectionAssert.AreEqual(keys, names.Select(ObjectStore.UpstreamFor).ToArray()); } [TestMethod] diff --git a/GitLfsCache/Storage/ObjectStore.cs b/GitLfsCache/Storage/ObjectStore.cs index 2b1cf07..fa27c04 100644 --- a/GitLfsCache/Storage/ObjectStore.cs +++ b/GitLfsCache/Storage/ObjectStore.cs @@ -3,7 +3,9 @@ namespace ktsu.GitLfsCache.Storage; using System.Diagnostics.CodeAnalysis; +using System.Globalization; using System.IO.Abstractions; +using System.Text; using ktsu.GitLfsCache.Configuration; using ktsu.Semantics.Paths; using ktsu.Semantics.Strings; @@ -15,7 +17,8 @@ namespace ktsu.GitLfsCache.Storage; /// /// /// Layout is {root}/{upstream}/objects/{first two}/{next two}/{oid} with staging under -/// {root}/{upstream}/staging. Staging shares the volume with the objects so publishing is an +/// {root}/{upstream}/staging, where {upstream} is the key's directory name (see +/// ). Staging shares the volume with the objects so publishing is an /// atomic rename, and the two-level fan-out mirrors the git-lfs client's own layout, which keeps /// directory sizes reasonable into the hundreds of thousands of objects. /// @@ -225,7 +228,7 @@ public IEnumerable Enumerate() foreach (string upstreamDirectory in fileSystem.Directory.EnumerateDirectories(_root)) { - string upstream = fileSystem.Path.GetFileName(upstreamDirectory); + string upstream = UpstreamFor(fileSystem.Path.GetFileName(upstreamDirectory)); string objectsDirectory = fileSystem.Path.Combine(upstreamDirectory, ObjectsDirectoryName); if (!fileSystem.Directory.Exists(objectsDirectory)) @@ -346,11 +349,11 @@ private bool TryDeleteFile(AbsoluteFilePath path) } private AbsoluteFilePath ObjectPath(string upstream, string oid) => fileSystem.Path - .Combine(_root, upstream, ObjectsDirectoryName, oid[..2], oid[2..4], oid) + .Combine(_root, DirectoryNameFor(upstream), ObjectsDirectoryName, oid[..2], oid[2..4], oid) .As(); private AbsoluteDirectoryPath StagingDirectory(string upstream) => fileSystem.Path - .Combine(_root, upstream, StagingDirectoryName) + .Combine(_root, DirectoryNameFor(upstream), StagingDirectoryName) .As(); /// @@ -364,9 +367,56 @@ private AbsoluteDirectoryPath StagingDirectory(string upstream) => fileSystem.Pa private static bool IsValidOid([NotNullWhen(true)] string? oid) => oid is not null && oid.Length == OidLength && oid.All(char.IsAsciiHexDigitLower); - private static bool IsValidUpstream([NotNullWhen(true)] string? upstream) => - !string.IsNullOrEmpty(upstream) && upstream.All(IsUpstreamCharacter); + private static bool IsValidUpstream([NotNullWhen(true)] string? upstream) => !string.IsNullOrEmpty(upstream); - private static bool IsUpstreamCharacter(char character) => + /// + /// Maps an upstream key to the name of the directory its tree lives in. + /// + /// + /// A key made only of ASCII letters, digits, - and _ is its own directory name, which + /// is the layout every store written before other keys were supported already has. Any other key, + /// such as the natural gitlab.com, has each byte of its UTF-8 form outside that set written + /// as %XX. That is deterministic and reversible, so no two keys share a directory, and an + /// escaped name always contains a %, which a plain one never does. It also leaves nothing a + /// filesystem would read as a separator or a parent reference, so no key can name a directory + /// outside the store root. + /// + /// The upstream key. + /// The directory name. + internal static string DirectoryNameFor(string upstream) + { + if (upstream.All(IsPlainCharacter)) + { + return upstream; + } + + StringBuilder name = new(upstream.Length * 3); + + foreach (byte value in Encoding.UTF8.GetBytes(upstream)) + { + if (value < 0x80 && IsPlainCharacter((char)value)) + { + name.Append((char)value); + } + else + { + name.Append('%').Append(value.ToString("X2", CultureInfo.InvariantCulture)); + } + } + + return name.ToString(); + } + + /// + /// Recovers the upstream key from a directory name produced. + /// + /// The directory name. + /// The upstream key. + internal static string UpstreamFor(string directoryName) => + directoryName.Contains('%', StringComparison.Ordinal) + ? Uri.UnescapeDataString(directoryName) + : directoryName; + + private static bool IsPlainCharacter(char character) => char.IsAsciiLetterOrDigit(character) || character is '-' or '_'; } diff --git a/docs/superpowers/specs/2026-08-18-gitlfscache-design.md b/docs/superpowers/specs/2026-08-18-gitlfscache-design.md index 6248d92..e147eb7 100644 --- a/docs/superpowers/specs/2026-08-18-gitlfscache-design.md +++ b/docs/superpowers/specs/2026-08-18-gitlfscache-design.md @@ -125,6 +125,8 @@ Layout, one tree per upstream key, with `ktsu.Semantics.Paths` types for the roo //staging/.tmp ``` +`` is the key itself when it is made only of ASCII letters, digits, `-` and `_`. Any other key, such as `gitlab.com`, has each UTF-8 byte outside that set written as `%XX` (`gitlab%2Ecom`), which is reversible, cannot collide with a plain name, and cannot reach outside the root. + Staging sits on the same volume as the objects so publishing is an atomic rename. The two-level fan-out mirrors the git-lfs client's own layout and keeps directory sizes reasonable into the hundreds of thousands of objects. Namespacing per upstream duplicates bytes when two upstreams hold the same object, and content addressing means those bytes are identical. The duplication is accepted to bound the blast radius if the token codec is ever wrong. A single shared tree is recorded as a deferred option, not a configuration flag.