Match repository patterns in linear time, and refuse a timed-out match [patch]

GitBranchStateCache.Tests/Upstreams/RepositoryPatternTests.cs

@@ -60,4 +60,54 @@ 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"));
+	}
+
+	[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));
+	}
 }

GitBranchStateCache/Upstreams/RepositoryPattern.cs

@@ -18,9 +18,10 @@ public sealed class RepositoryPattern
 	/// How long a single match may run before it is abandoned.
 	/// </summary>
 	/// <remarks>
-	/// 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 <see cref="RegexOptions.NonBacktracking"/>, 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 <see cref="Matches"/> reads a timeout as no match.
 	/// </remarks>
 	private static readonly TimeSpan MatchTimeout = TimeSpan.FromMilliseconds(100);
 
@@ -48,7 +49,18 @@ private RepositoryPattern(string text, Regex expression)
 	/// <param name="parsed">The translated pattern, when it was acceptable.</param>
 	/// <param name="failure">Why the pattern was refused, when it was.</param>
 	/// <returns><see langword="true"/> when the pattern was acceptable.</returns>
-	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);
+
+	/// <summary>
+	/// Translates a configured pattern with a given match timeout, so a test can reach the timeout path.
+	/// </summary>
+	/// <param name="pattern">The pattern as configured.</param>
+	/// <param name="matchTimeout">How long a single match may run before it is abandoned.</param>
+	/// <param name="parsed">The translated pattern, when it was acceptable.</param>
+	/// <param name="failure">Why the pattern was refused, when it was.</param>
+	/// <returns><see langword="true"/> when the pattern was acceptable.</returns>
+	internal static bool TryParse(string? pattern, TimeSpan matchTimeout, out RepositoryPattern? parsed, out string? failure)
 	{
 		parsed = null;
 		failure = null;
@@ -76,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;
 	}
 
@@ -85,7 +97,21 @@ public static bool TryParse(string? pattern, out RepositoryPattern? parsed, out
 	/// </summary>
 	/// <param name="repositoryPath">The path following the upstream key, without surrounding slashes.</param>
 	/// <returns><see langword="true"/> when it matches.</returns>
-	public bool Matches(string repositoryPath) => _expression.IsMatch(repositoryPath);
+	/// <remarks>
+	/// 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.
+	/// </remarks>
+	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,19 +126,25 @@ private static bool IsLiteral(string segment) =>
 	/// replacement for the double contains no backslash, so the second replacement cannot reach inside
 	/// it.
 	/// <para>
+	/// Two single stars in one segment, as in <c>team/*-svc-*</c>, translate to adjacent
+	/// <c>[^/]*</c> runs, which a backtracking engine takes polynomial time to reject on a long
+	/// segment. The expression is therefore built with <see cref="RegexOptions.NonBacktracking"/>,
+	/// whose matching time is linear in the path whatever the pattern.
+	/// </para>
+	/// <para>
 	/// Matching is case insensitive because forge repository names are, and a pattern that fails only
 	/// because someone typed <c>Studio</c> is a support ticket rather than a control.
 	/// </para>
 	/// </remarks>
-	private static Regex Translate(string pattern)
+	private static Regex Translate(string pattern, TimeSpan matchTimeout)
 	{
 		string expression = Regex.Escape(pattern)
 			.Replace(@"\*\*", ".*", StringComparison.Ordinal)
 			.Replace(@"\*", "[^/]*", StringComparison.Ordinal);
 
 		return new Regex(
 			$"^{expression}$",
-			RegexOptions.IgnoreCase | RegexOptions.CultureInvariant,
-			MatchTimeout);
+			RegexOptions.IgnoreCase | RegexOptions.CultureInvariant | RegexOptions.NonBacktracking,
+			matchTimeout);
 	}
 }