-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix: expand commit hash regex to support SHA-256 (64-char) hashes #4347
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ public static class WellKnownRegularExpressions | |
| public const String Email = nameof(Email); | ||
| public const String IPv4Address = nameof(IPv4Address); | ||
| public const String SHA1 = nameof(SHA1); | ||
| public const String CommitHash = nameof(CommitHash); | ||
| public const String Url = nameof(Url); | ||
|
|
||
| /// <summary> | ||
|
|
@@ -24,7 +25,8 @@ public static Lazy<Regex> GetRegex(String regexType) | |
| case IPv4Address: | ||
| return s_validIPv4Address; | ||
| case SHA1: | ||
| return s_validSha1; | ||
| case CommitHash: | ||
| return s_validCommitHash; | ||
| case Url: | ||
| return s_validUrl; | ||
| default: | ||
|
|
@@ -46,9 +48,9 @@ public static Lazy<Regex> GetRegex(String regexType) | |
| ) | ||
| ); | ||
|
|
||
| // 40 hex characters | ||
| private static readonly Lazy<Regex> s_validSha1 = new Lazy<Regex>(() => new Regex( | ||
| @"\b[0-9a-f]{40}\b", | ||
| // 40 or 64 hex characters (SHA-1 or SHA-256 commit hash) | ||
| private static readonly Lazy<Regex> s_validCommitHash = new Lazy<Regex>(() => new Regex( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The regex change looks correct, but Would be good if we could add a test class (e.g.
Would be good to avoid a regression in the future, in-case a future change breaks 64-char support to avoid it going undetected since nothing in CI directly tests it. |
||
| @"\b(?:[0-9a-f]{40}|[0-9a-f]{64})\b", | ||
| RegexOptions.IgnoreCase | RegexOptions.CultureInvariant | RegexOptions.Compiled, RegexUtility.GetRegexTimeOut() | ||
| ) | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| using GitHub.DistributedTask.Pipelines.Expressions; | ||
| using Xunit; | ||
|
|
||
| namespace GitHub.Runner.Common.Tests.Sdk | ||
| { | ||
| public sealed class WellKnownRegularExpressionsL0 | ||
| { | ||
| [Fact] | ||
| [Trait("Level", "L0")] | ||
| [Trait("Category", "Sdk")] | ||
| public void SHA1_Key_Returns_CommitHash_Regex() | ||
| { | ||
| var regex = WellKnownRegularExpressions.GetRegex(WellKnownRegularExpressions.SHA1); | ||
|
|
||
| Assert.NotNull(regex); | ||
| } | ||
|
|
||
| [Fact] | ||
| [Trait("Level", "L0")] | ||
| [Trait("Category", "Sdk")] | ||
| public void CommitHash_Key_Returns_CommitHash_Regex() | ||
| { | ||
| var regex = WellKnownRegularExpressions.GetRegex(WellKnownRegularExpressions.CommitHash); | ||
|
|
||
| Assert.NotNull(regex); | ||
| } | ||
|
|
||
| [Fact] | ||
| [Trait("Level", "L0")] | ||
| [Trait("Category", "Sdk")] | ||
| public void SHA1_And_CommitHash_Return_Same_Regex() | ||
| { | ||
| var sha1Regex = WellKnownRegularExpressions.GetRegex(WellKnownRegularExpressions.SHA1); | ||
| var commitHashRegex = WellKnownRegularExpressions.GetRegex(WellKnownRegularExpressions.CommitHash); | ||
|
|
||
| Assert.Same(sha1Regex, commitHashRegex); | ||
| } | ||
|
|
||
| [Fact] | ||
| [Trait("Level", "L0")] | ||
| [Trait("Category", "Sdk")] | ||
| public void Matches_40_Char_Hex() | ||
| { | ||
| var regex = WellKnownRegularExpressions.GetRegex(WellKnownRegularExpressions.CommitHash); | ||
|
|
||
| Assert.Matches(regex.Value, new string((char)97, 40)); | ||
| } | ||
|
|
||
| [Fact] | ||
| [Trait("Level", "L0")] | ||
| [Trait("Category", "Sdk")] | ||
| public void Matches_64_Char_Hex() | ||
| { | ||
| var regex = WellKnownRegularExpressions.GetRegex(WellKnownRegularExpressions.CommitHash); | ||
|
|
||
| Assert.Matches(regex.Value, new string((char)97, 64)); | ||
| } | ||
|
|
||
| [Fact] | ||
| [Trait("Level", "L0")] | ||
| [Trait("Category", "Sdk")] | ||
| public void Does_Not_Match_63_Char_Hex() | ||
| { | ||
| var regex = WellKnownRegularExpressions.GetRegex(WellKnownRegularExpressions.CommitHash); | ||
|
|
||
| Assert.DoesNotMatch(regex.Value, new string((char)97, 63)); | ||
| } | ||
|
|
||
| [Fact] | ||
| [Trait("Level", "L0")] | ||
| [Trait("Category", "Sdk")] | ||
| public void Does_Not_Match_65_Char_Hex() | ||
| { | ||
| var regex = WellKnownRegularExpressions.GetRegex(WellKnownRegularExpressions.CommitHash); | ||
|
|
||
| Assert.DoesNotMatch(regex.Value, new string((char)97, 65)); | ||
| } | ||
|
|
||
| [Fact] | ||
| [Trait("Level", "L0")] | ||
| [Trait("Category", "Sdk")] | ||
| public void Matches_Mixed_Case_64_Char() | ||
| { | ||
| var regex = WellKnownRegularExpressions.GetRegex(WellKnownRegularExpressions.CommitHash); | ||
| var value = new string((char)65, 32) + new string((char)98, 32); | ||
|
|
||
| Assert.Matches(regex.Value, value); | ||
| } | ||
|
|
||
| [Fact] | ||
| [Trait("Level", "L0")] | ||
| [Trait("Category", "Sdk")] | ||
| public void Unknown_Key_Returns_Null() | ||
| { | ||
| var regex = WellKnownRegularExpressions.GetRegex("UnknownType"); | ||
|
|
||
| Assert.Null(regex); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to remove the SHA1 and update all the caller to use the CommitHash?