fix: webhook URL matching edge cases #7981
Conversation
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
Codecov Report
@@ Coverage Diff @@
## master #7981 +/- ##
=======================================
Coverage 41.43% 41.44%
=======================================
Files 173 173
Lines 22570 22575 +5
=======================================
+ Hits 9353 9357 +4
Misses 11886 11886
- Partials 1331 1332 +1
Continue to review full report at Codecov.
|
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
|
|
||
| regexEscapedHostname := regexp.QuoteMeta(urlObj.Hostname()) | ||
| regexEscapedPath := regexp.QuoteMeta(urlObj.Path[1:]) | ||
| regexpStr := fmt.Sprintf(`(?i)^(http://|https://|\w+@|ssh://(\w+@)?)%s(:[0-9]+|)[:/]%s(\.git)?$`, regexEscapedHostname, regexEscapedPath) |
There was a problem hiding this comment.
The only change to the pattern (besides escaping the inputs) is to add ^ and $.
|
|
||
| // Standard cases. | ||
| {true, "https://example.com/org/repo", "https://example.com/org/repo", "exact match should match"}, | ||
| {false, "https://example.com/org/repo", "https://example.com/org/repo-2", "partial match should not match"}, |
There was a problem hiding this comment.
The ^/$ change makes this pass.
| {false, "https://example.com/org/a..d", "https://example.com/org/abcd", "dots in repo names should not be treated as wildcards"}, | ||
| {false, "https://an.example.com/org/repo", "https://an-example.com/org/repo", "dots in domain names should not be treated as wildcards"}, |
There was a problem hiding this comment.
The QuoteMeta changes make these pass.
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
| } | ||
|
|
||
| regexEscapedHostname := regexp.QuoteMeta(urlObj.Hostname()) | ||
| regexEscapedPath := regexp.QuoteMeta(urlObj.Path[1:]) |
There was a problem hiding this comment.
Just a question for my understanding: Why do we quote Path[1:] and not the whole Path here?
There was a problem hiding this comment.
Good question. I think the [1:] index has been there since Jesse first committed the regex 4 years ago. My assumption has been that it's stripping the leading / from the path.
There was a problem hiding this comment.
Ah right - I see that it's already been so in the original. Yeah, in the context of the complete regexp, it makes sense, because URLs could be in different formats, including scp-format (e.g. git@foo:bar/baz)
This fixes two edge cases when matching git hook payloads to targetRevisions.
Dots are now interpreted literally. Previously,
example.com:org/some.repowould match targetRevisionexample.com:org/some-repo.The current implementation would technically allow any regex pattern in the org or repo name. But practically speaking, dots are the only control characters that show up in org, repo, and domain names.
Partial matches no longer count. Previously
example.com:org/awould match targetRevisionexample.com:org/apple.