-
Notifications
You must be signed in to change notification settings - Fork 6.8k
fix(webhook): support paths in API URL regex for enterprise setups #26149
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: master
Are you sure you want to change the base?
Conversation
🔴 Preview Environment stopped on BunnyshellSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
ppapapetrou76
left a comment
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.
@HsiuChuanHsu can you please check the failing unit tests?
c5bb224 to
1804473
Compare
|
It's all green! |
| {true, "https://user@example.com/", "https://example.com/", "https+username should match https"}, | ||
| {true, "http://example.com/", "https://user@example.com/", "http should match https+username"}, | ||
| {true, "https://example.com/", "https://user@example.com/", "https should match https+username"}, | ||
| {true, "https://git.ourenterprise.com/", "https://git.ourenterprise.com/api/v3", "enterprise API URL with path should match base repo URL"}, |
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.
Would it make sense to add a test case for the common webhook shape where apiURL includes /repos/{owner}/{repo} but the generator API is the base /api/v3?
{true, "https://git.ourenterprise.com/api/v3/repos/org/repo", "https://git.ourenterprise.com/api/v3", "enterprise API URL with repo path should match base API"},
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.
For sure. Test cases with API path followed by additional repository-specific segments are added.
Signed-off-by: HsiuChuanHsu <hchsu2106@gmail.com>
Signed-off-by: HsiuChuanHsu <hchsu2106@gmail.com>
Add test cases that verify the regex accepts webhook URLs containing the base API path followed by additional repository-specific segments. Signed-off-by: HsiuChuanHsu <hchsu2106@gmail.com>
1804473 to
12ef775
Compare
WHY
Using GitHub Enterprise (or similar self-hosted Git platforms) often have API URLs with paths like https://git.company.com/api/v3 .
Since PR #21227, these setups have been failing because our webhook validator only expected URLs without paths.
HOW
Update the regex pattern in GetAPIURLRegex() to accept optional path segments.
The key change is replacing the overly strict /?$ (which only allowed a single trailing slash) with (/[\w.%/-]*)?$.
What
Files Modified:
util/webhook/webhook.go- Updated regex to support paths in API URLsutil/webhook/webhook_test.go- Added test case for enterprise API URL patternFixes: #25095
Checklist: