From 3370d8e7ada4c7992ba2cb15a26c9f1e1b595ef8 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 31 Oct 2026 00:27:28 +0000 Subject: [PATCH 1/2] Match repository patterns in linear time, and refuse a timed-out match [patch] A pattern with two stars in one segment, such as team/*-svc-*, became adjacent [^/]* runs that the backtracking engine took polynomial time to reject. The allow-list runs before admission, so an unauthenticated long path could time the match out and get a 500. The expression is now built with RegexOptions.NonBacktracking, and a match that still times out counts as not allowed. Fixes #111 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017vfJW6ERvyATBYDH9pPgb7 --- .../Upstreams/RepositoryPatternTests.cs | 35 +++++++++++++++++++ .../Upstreams/RepositoryPattern.cs | 31 +++++++++++++--- 2 files changed, 61 insertions(+), 5 deletions(-) diff --git a/GitBranchStateCache.Tests/Upstreams/RepositoryPatternTests.cs b/GitBranchStateCache.Tests/Upstreams/RepositoryPatternTests.cs index 8a76fff..6c35158 100644 --- a/GitBranchStateCache.Tests/Upstreams/RepositoryPatternTests.cs +++ b/GitBranchStateCache.Tests/Upstreams/RepositoryPatternTests.cs @@ -60,4 +60,39 @@ public void Matches_DotIsLiteral() Assert.IsFalse(pattern!.Matches("studio/gameXgit")); } + + [TestMethod] + [DataRow("studio/*-*-*", "studio", 1000)] + [DataRow("studio/*-*", "studio", 4000)] + [DataRow("team/*-svc-*", "team", 4000)] + public void Matches_SeveralStarsInOneSegment_RejectsALongNonMatchingPathWithoutTimingOut(string text, string owner, int dashes) + { + // Two stars in one segment are adjacent [^/]* runs, which a backtracking engine takes + // polynomial time to reject. The allow-list runs before admission, so an unauthenticated + // caller could otherwise time the match out and get a 500 instead of a 404 (#111). + Assert.IsTrue(RepositoryPattern.TryParse(text, out RepositoryPattern? pattern, out _)); + string path = $"{owner}/{new string('-', dashes)}/x"; + + // Warm the expression up on a short path first, so the timing below measures the match rather + // than the engine building its automaton. + _ = pattern!.Matches($"{owner}/a/x"); + + System.Diagnostics.Stopwatch stopwatch = System.Diagnostics.Stopwatch.StartNew(); + bool matched = pattern.Matches(path); + stopwatch.Stop(); + + Assert.IsFalse(matched); + Assert.IsLessThan(50, stopwatch.ElapsedMilliseconds); + } + + [TestMethod] + public void Matches_SeveralStarsInOneSegment_StillMatchesWhatItShould() + { + Assert.IsTrue(RepositoryPattern.TryParse("team/*-svc-*", out RepositoryPattern? pattern, out _)); + + Assert.IsTrue(pattern!.Matches("team/billing-svc-api")); + Assert.IsTrue(pattern.Matches("Team/Billing-SVC-Api")); + Assert.IsFalse(pattern.Matches("team/billing-api")); + Assert.IsFalse(pattern.Matches("team/billing-svc-api/extra")); + } } diff --git a/GitBranchStateCache/Upstreams/RepositoryPattern.cs b/GitBranchStateCache/Upstreams/RepositoryPattern.cs index 331aed8..620f052 100644 --- a/GitBranchStateCache/Upstreams/RepositoryPattern.cs +++ b/GitBranchStateCache/Upstreams/RepositoryPattern.cs @@ -18,9 +18,10 @@ public sealed class RepositoryPattern /// How long a single match may run before it is abandoned. /// /// - /// The translated patterns have no backtracking construct, so this cannot fire in practice. It is - /// set because a regular expression built from configuration and run against a request path is - /// exactly the shape that should carry a timeout. + /// The patterns are matched with , which runs in time + /// linear in the path, so this should not fire. It is kept because a regular expression built from + /// configuration and run against a request path is exactly the shape that should carry a timeout, + /// and reads a timeout as no match. /// private static readonly TimeSpan MatchTimeout = TimeSpan.FromMilliseconds(100); @@ -85,7 +86,21 @@ public static bool TryParse(string? pattern, out RepositoryPattern? parsed, out /// /// The path following the upstream key, without surrounding slashes. /// when it matches. - public bool Matches(string repositoryPath) => _expression.IsMatch(repositoryPath); + /// + /// This runs before admission, on a path an unauthenticated caller chose, so a match that runs out + /// of time is refused as not allowed rather than surfacing as a server error. + /// + public bool Matches(string repositoryPath) + { + try + { + return _expression.IsMatch(repositoryPath); + } + catch (RegexMatchTimeoutException) + { + return false; + } + } private static bool IsLiteral(string segment) => segment.Length > 0 && !segment.Contains('*', StringComparison.Ordinal); @@ -100,6 +115,12 @@ private static bool IsLiteral(string segment) => /// replacement for the double contains no backslash, so the second replacement cannot reach inside /// it. /// + /// Two single stars in one segment, as in team/*-svc-*, translate to adjacent + /// [^/]* runs, which a backtracking engine takes polynomial time to reject on a long + /// segment. The expression is therefore built with , + /// whose matching time is linear in the path whatever the pattern. + /// + /// /// Matching is case insensitive because forge repository names are, and a pattern that fails only /// because someone typed Studio is a support ticket rather than a control. /// @@ -112,7 +133,7 @@ private static Regex Translate(string pattern) return new Regex( $"^{expression}$", - RegexOptions.IgnoreCase | RegexOptions.CultureInvariant, + RegexOptions.IgnoreCase | RegexOptions.CultureInvariant | RegexOptions.NonBacktracking, MatchTimeout); } } From 52e9328b8882744613d30e8deb81c205e10a37a5 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 31 Oct 2026 00:36:53 +0000 Subject: [PATCH 2/2] Cover the timed-out match path with a test [patch] Adds an internal TryParse overload that takes the match timeout, so a test can make a match time out and check that Matches reads it as not allowed rather than throwing. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017vfJW6ERvyATBYDH9pPgb7 --- .../Upstreams/RepositoryPatternTests.cs | 15 +++++++++++++++ .../Upstreams/RepositoryPattern.cs | 19 +++++++++++++++---- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/GitBranchStateCache.Tests/Upstreams/RepositoryPatternTests.cs b/GitBranchStateCache.Tests/Upstreams/RepositoryPatternTests.cs index 6c35158..5299819 100644 --- a/GitBranchStateCache.Tests/Upstreams/RepositoryPatternTests.cs +++ b/GitBranchStateCache.Tests/Upstreams/RepositoryPatternTests.cs @@ -95,4 +95,19 @@ public void Matches_SeveralStarsInOneSegment_StillMatchesWhatItShould() Assert.IsFalse(pattern.Matches("team/billing-api")); Assert.IsFalse(pattern.Matches("team/billing-svc-api/extra")); } + + [TestMethod] + public void Matches_WhenTheMatchTimesOut_IsNotAllowedRatherThanThrowing() + { + // The allow-list runs before admission, so a timeout must read as "not allowed" (a 404) and + // never escape as an unhandled RegexMatchTimeoutException (a 500). + Assert.IsTrue(RepositoryPattern.TryParse( + "studio/**", + TimeSpan.FromTicks(1), + out RepositoryPattern? pattern, + out _)); + string path = $"studio/{new string('a', 4_000_000)}"; + + Assert.IsFalse(pattern!.Matches(path)); + } } diff --git a/GitBranchStateCache/Upstreams/RepositoryPattern.cs b/GitBranchStateCache/Upstreams/RepositoryPattern.cs index 620f052..59b8063 100644 --- a/GitBranchStateCache/Upstreams/RepositoryPattern.cs +++ b/GitBranchStateCache/Upstreams/RepositoryPattern.cs @@ -49,7 +49,18 @@ private RepositoryPattern(string text, Regex expression) /// The translated pattern, when it was acceptable. /// Why the pattern was refused, when it was. /// when the pattern was acceptable. - public static bool TryParse(string? pattern, out RepositoryPattern? parsed, out string? failure) + public static bool TryParse(string? pattern, out RepositoryPattern? parsed, out string? failure) => + TryParse(pattern, MatchTimeout, out parsed, out failure); + + /// + /// Translates a configured pattern with a given match timeout, so a test can reach the timeout path. + /// + /// The pattern as configured. + /// How long a single match may run before it is abandoned. + /// The translated pattern, when it was acceptable. + /// Why the pattern was refused, when it was. + /// when the pattern was acceptable. + internal static bool TryParse(string? pattern, TimeSpan matchTimeout, out RepositoryPattern? parsed, out string? failure) { parsed = null; failure = null; @@ -77,7 +88,7 @@ public static bool TryParse(string? pattern, out RepositoryPattern? parsed, out return false; } - parsed = new RepositoryPattern(pattern, Translate(trimmed)); + parsed = new RepositoryPattern(pattern, Translate(trimmed, matchTimeout)); return true; } @@ -125,7 +136,7 @@ private static bool IsLiteral(string segment) => /// because someone typed Studio is a support ticket rather than a control. /// /// - private static Regex Translate(string pattern) + private static Regex Translate(string pattern, TimeSpan matchTimeout) { string expression = Regex.Escape(pattern) .Replace(@"\*\*", ".*", StringComparison.Ordinal) @@ -134,6 +145,6 @@ private static Regex Translate(string pattern) return new Regex( $"^{expression}$", RegexOptions.IgnoreCase | RegexOptions.CultureInvariant | RegexOptions.NonBacktracking, - MatchTimeout); + matchTimeout); } }