From 9b4e92aaa3e61f8050cc635a340120302b52051a Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 3 Nov 2026 18:31:18 +0000 Subject: [PATCH] Refuse a request that names more branch patterns than MaxBranchPatternsPerRequest [patch] POST /state and GET /branches capped each pattern's length but not how many patterns one request could send. Every pattern is matched against every mirror branch synchronously, so one request with tens of thousands of patterns could hold a thread for minutes. Add MaxBranchPatternsPerRequest (default 256, validated > 0) and refuse a request over it with a 400 in TryParsePatterns, before any clone or fetch. Fixes #116 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017vfJW24ManL6mVAsf34qvT --- ...itBranchStateCacheOptionsValidatorTests.cs | 14 +++++ .../Integration/StateFlowTests.cs | 60 +++++++++++++++++++ GitBranchStateCache.Tool/appsettings.json | 1 + .../GitBranchStateCacheOptions.cs | 9 +++ .../GitBranchStateCacheOptionsValidator.cs | 5 ++ .../Endpoints/BranchStateHandler.cs | 19 +++++- README.md | 3 + 7 files changed, 109 insertions(+), 2 deletions(-) diff --git a/GitBranchStateCache.Tests/Configuration/GitBranchStateCacheOptionsValidatorTests.cs b/GitBranchStateCache.Tests/Configuration/GitBranchStateCacheOptionsValidatorTests.cs index efbfd33..6f6ffd5 100644 --- a/GitBranchStateCache.Tests/Configuration/GitBranchStateCacheOptionsValidatorTests.cs +++ b/GitBranchStateCache.Tests/Configuration/GitBranchStateCacheOptionsValidatorTests.cs @@ -152,6 +152,20 @@ public void Validate_NonPositiveMaxCachedMergeBases_Fails() Assert.Contains("MaxCachedMergeBases", result.FailureMessage); } + [TestMethod] + [DataRow(0)] + [DataRow(-1)] + public void Validate_NonPositiveMaxBranchPatternsPerRequest_Fails(int limit) + { + GitBranchStateCacheOptions options = Valid(); + options.MaxBranchPatternsPerRequest = limit; + + ValidateOptionsResult result = Validate(options); + + Assert.IsTrue(result.Failed); + Assert.Contains("MaxBranchPatternsPerRequest", result.FailureMessage); + } + [TestMethod] public void Validate_NonPositiveMaxConcurrentBranchQueries_Fails() { diff --git a/GitBranchStateCache.Tests/Integration/StateFlowTests.cs b/GitBranchStateCache.Tests/Integration/StateFlowTests.cs index 48fe198..14941b6 100644 --- a/GitBranchStateCache.Tests/Integration/StateFlowTests.cs +++ b/GitBranchStateCache.Tests/Integration/StateFlowTests.cs @@ -258,6 +258,66 @@ public async Task Post_State_WithNoBranchPatterns_Is400() Assert.AreEqual(HttpStatusCode.BadRequest, response.StatusCode); } + [TestMethod] + public async Task Post_State_MoreBranchPatternsThanTheLimit_Is400BeforeAnyFetch() + { + // Every pattern is matched against every branch, synchronously, so the count has to be + // refused before the mirror is cloned or fetched. + await using ServiceFixture fixture = await ServiceFixture.StartAsync(new() + { + ["GitBranchStateCache:MaxBranchPatternsPerRequest"] = "3", + }); + Seed(fixture.Git); + + string[] patterns = ["origin/main", "origin/release/*", "origin/a*", "origin/b*"]; + + using HttpResponseMessage response = await fixture.Client.SendAsync( + StateRequest(JsonSerializer.Serialize(new { @base = ClientBase, branchPatterns = patterns }))); + JsonElement body = await ReadAsync(response); + + Assert.AreEqual(HttpStatusCode.BadRequest, response.StatusCode); + Assert.Contains("at most 3", body.GetProperty("message").GetString()!); + Assert.AreEqual(0, fixture.Git.CountOf("clone")); + Assert.AreEqual(0, fixture.Git.CountOf("fetch")); + } + + [TestMethod] + public async Task Post_State_BranchPatternsAtTheLimit_AreAccepted() + { + await using ServiceFixture fixture = await ServiceFixture.StartAsync(new() + { + ["GitBranchStateCache:MaxBranchPatternsPerRequest"] = "2", + }); + Seed(fixture.Git); + + string[] patterns = ["origin/main", "origin/release/*"]; + + using HttpResponseMessage response = await fixture.Client.SendAsync( + StateRequest(JsonSerializer.Serialize(new { @base = ClientBase, branchPatterns = patterns }))); + + Assert.AreEqual(HttpStatusCode.OK, response.StatusCode); + } + + [TestMethod] + public async Task Get_Branches_MoreBranchPatternsThanTheLimit_Is400() + { + await using ServiceFixture fixture = await ServiceFixture.StartAsync(new() + { + ["GitBranchStateCache:MaxBranchPatternsPerRequest"] = "1", + }); + Seed(fixture.Git); + + using HttpRequestMessage request = new( + HttpMethod.Get, + "/v1/github/studio/game.git/branches?pattern=origin/main&pattern=origin/release/*"); + request.Headers.Authorization = AuthenticationHeaderValue.Parse(Credential); + + using HttpResponseMessage response = await fixture.Client.SendAsync(request); + + Assert.AreEqual(HttpStatusCode.BadRequest, response.StatusCode); + Assert.AreEqual(0, fixture.Git.CountOf("clone")); + } + [TestMethod] [DataRow("HEAD")] [DataRow("main@{1}")] diff --git a/GitBranchStateCache.Tool/appsettings.json b/GitBranchStateCache.Tool/appsettings.json index 52703b2..93d2f2b 100644 --- a/GitBranchStateCache.Tool/appsettings.json +++ b/GitBranchStateCache.Tool/appsettings.json @@ -16,6 +16,7 @@ "MaxCachedMergeBases": 20000, "MaxConcurrentBranchQueries": 4, "MaxPathsPerRequest": 20000, + "MaxBranchPatternsPerRequest": 256, "MirrorIdleMaxAge": "30.00:00:00", "MaintenanceInterval": "01:00:00" } diff --git a/GitBranchStateCache/Configuration/GitBranchStateCacheOptions.cs b/GitBranchStateCache/Configuration/GitBranchStateCacheOptions.cs index d591166..3f8703a 100644 --- a/GitBranchStateCache/Configuration/GitBranchStateCacheOptions.cs +++ b/GitBranchStateCache/Configuration/GitBranchStateCacheOptions.cs @@ -123,6 +123,15 @@ public sealed class GitBranchStateCacheOptions /// public int MaxPathsPerRequest { get; set; } = 20_000; + /// + /// Gets or sets the most branch patterns one request may name. + /// + /// + /// Every pattern is matched against every branch in the mirror, synchronously, so without a limit + /// one request can keep a thread busy for minutes. A request over it is refused before any fetch. + /// + public int MaxBranchPatternsPerRequest { get; set; } = 256; + /// /// Gets or sets how long a mirror may go unqueried before it is deleted. /// diff --git a/GitBranchStateCache/Configuration/GitBranchStateCacheOptionsValidator.cs b/GitBranchStateCache/Configuration/GitBranchStateCacheOptionsValidator.cs index f304039..b96b012 100644 --- a/GitBranchStateCache/Configuration/GitBranchStateCacheOptionsValidator.cs +++ b/GitBranchStateCache/Configuration/GitBranchStateCacheOptionsValidator.cs @@ -167,6 +167,11 @@ private static void ValidateLimits(GitBranchStateCacheOptions options, List failures) diff --git a/GitBranchStateCache/Endpoints/BranchStateHandler.cs b/GitBranchStateCache/Endpoints/BranchStateHandler.cs index 54f18e7..1d1656a 100644 --- a/GitBranchStateCache/Endpoints/BranchStateHandler.cs +++ b/GitBranchStateCache/Endpoints/BranchStateHandler.cs @@ -141,7 +141,11 @@ private async Task HandleStateAsync( + "which 'git rev-parse @{upstream}' produces without touching the network."); } - if (!TryParsePatterns(request.BranchPatterns, out List patterns, out string? patternFailure)) + if (!TryParsePatterns( + request.BranchPatterns, + options.Value.MaxBranchPatternsPerRequest, + out List patterns, + out string? patternFailure)) { return BadRequest(patternFailure!); } @@ -407,7 +411,11 @@ .. context.Request.Query["pattern"] .Select(pattern => pattern!) ]; - if (!TryParsePatterns(requested, out List patterns, out string? patternFailure)) + if (!TryParsePatterns( + requested, + options.Value.MaxBranchPatternsPerRequest, + out List patterns, + out string? patternFailure)) { return BadRequest(patternFailure!); } @@ -532,6 +540,7 @@ private static string WithoutGitSuffix(string repositoryPath) private static bool TryParsePatterns( IReadOnlyList? requested, + int limit, out List patterns, out string? failure) { @@ -544,6 +553,12 @@ private static bool TryParsePatterns( return false; } + if (requested.Count > limit) + { + failure = $"The request named {requested.Count} branch patterns, and at most {limit} may be sent."; + return false; + } + foreach (string pattern in requested) { if (!BranchPattern.TryParse(pattern, out BranchPattern? parsed, out string? reason)) diff --git a/README.md b/README.md index b955ad3..d59079f 100644 --- a/README.md +++ b/README.md @@ -144,6 +144,7 @@ Resolves wildcard patterns against the mirror's refs and returns names with tip "MaxCachedMergeBases": 20000, "MaxConcurrentBranchQueries": 4, "MaxPathsPerRequest": 20000, + "MaxBranchPatternsPerRequest": 256, "MirrorIdleMaxAge": "30.00:00:00", "Upstreams": { "github": { @@ -165,6 +166,8 @@ Refs younger than `RefsTtl` are answered from as they are. Past it, and for up t Merge bases are cached by mirror, client base and branch tip, up to `MaxCachedMergeBases`, and diffs by merge base and tip, up to `MaxCachedDiffs`. A request whose answers are all cached starts no git processes beyond reading the refs. One that is not works on up to `MaxConcurrentBranchQueries` branches at a time. Each clone and fetch also writes a commit-graph, which is what keeps an uncached merge base quick on a long history. +A request may name up to `MaxPathsPerRequest` paths and up to `MaxBranchPatternsPerRequest` branch patterns. Every pattern is matched against every branch in the mirror, so a request over either limit is refused with a 400 before anything is fetched. + ## Deployment Deploy **adjacent to the forge, not on-premises**. Its cost is round trips and its clients are worst served on residential links, which is the opposite placement from an object cache. `deploy/k8s` is a kustomize base: a StatefulSet with a read-write-once volume, a service, an ingress and a configmap. Start at one replica; each replica holds its own mirrors and diff cache, so replicas multiply fetch traffic and disk without improving the hit rate.