fix(webhook): prevent duplicate repository CR on trailing slash#2683
fix(webhook): prevent duplicate repository CR on trailing slash#2683zakisk merged 1 commit intotektoncd:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the repository URL validation logic within the webhook to prevent the creation of duplicate repository resources that differ only by trailing slashes. The changes include updating the comparison logic in checkIfRepoExist and adding comprehensive unit and integration tests. Feedback suggests improving the robustness of the URL normalization by using strings.TrimRight and strings.TrimSpace to handle multiple trailing slashes and whitespace, ensuring a more secure and consistent validation process.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2683 +/- ##
=======================================
Coverage 58.85% 58.85%
=======================================
Files 204 204
Lines 20149 20149
=======================================
Hits 11859 11859
Misses 7525 7525
Partials 765 765 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/lgtm just fix it to trimright as the bot suggest and that would be good to merge |
![pipelines-as-code[bot]](https://avatars.githubusercontent.com/in/112592?s=60&v=4){kind=small}
There was a problem hiding this comment.
Congrats @zakisk your PR Has been approved 🎉
✅ Pull Request Approved
Approval Status:
- Required Approvals: 1
- Current Approvals: 1
👥 Reviewers Who Approved:
| Reviewer | Permission Level | Approval Status |
|---|---|---|
| @chmouel | admin |
✅ |
📝 Next Steps
- Ensure all required checks pass
- Comply with branch protection rules
- Request a maintainer to merge using the
/mergecommand (or merge it
directly if you have repository permission).
Automated by the PAC Boussole 🧭
The webhook admission controller's checkIfRepoExist used exact string equality to compare repository URLs. This allowed users to create a second Repository CR with the same URL by simply appending a trailing slash (e.g. https://github.com/org/repo vs https://github.com/org/repo/), bypassing the uniqueness validation. - pkg/webhook/validation.go: - Normalize URLs with strings.TrimSuffix before comparison in checkIfRepoExist to treat trailing slash variants as identical - pkg/webhook/validation_test.go: - Add test case for rejecting repository URL with trailing slash when a matching repo already exists - test/repository_webhook_test.go: - Add e2e test TestOthersRepositoryCreationWithTrailingSlash to verify the webhook rejects duplicate repos created via trailing slash bypass Signed-off-by: Zaki Shaikh <zashaikh@redhat.com>
zakisk
force-pushed
the
fix-duplicate-repo-url-bypass
branch
from
537396e to
31e17c1
Compare
|
@aThorp96 I think this PR would fix https://redhat.atlassian.net/browse/SRVKP-11295 somehow? |
|
/lgtm |
There was a problem hiding this comment.
Congrats @zakisk your PR Has been approved 🎉
✅ Pull Request Approved
Approval Status:
- Required Approvals: 1
- Current Approvals: 1
👥 Reviewers Who Approved:
| Reviewer | Permission Level | Approval Status |
|---|---|---|
| @chmouel | admin |
✅ |
📝 Next Steps
- Ensure all required checks pass
- Comply with branch protection rules
- Request a maintainer to merge using the
/mergecommand (or merge it
directly if you have repository permission).
Automated by the PAC Boussole 🧭
| for i := len(repositories) - 1; i >= 0; i-- { | ||
| repoFromCluster := repositories[i] | ||
| if repoFromCluster.Spec.URL == repo.Spec.URL && | ||
| if strings.TrimRight(strings.TrimSpace(repoFromCluster.Spec.URL), "/") == strings.TrimRight(strings.TrimSpace(repo.Spec.URL), "/") && |
There was a problem hiding this comment.
@zakisk ah I think this is a likely culprit for the issue you linked.
We could probably make this normalization a bit more consistent using something like the below (playground link), but this should work as well I think. Though if strings.TrimSpace() is necessary then it would still be necessary in addition to path.Clean().
func normalizeUrl(repoUrl string) (string, err) {
u, err := url.Parse(repoUrl)
if err != nil {
return "", err
}
u.Path = path.Clean(u.Path)
return u.String(), nil
}There was a problem hiding this comment.
yeah, @aThorp96 makes sense! feel free to raise PR or you want me to do?
4 participants
The webhook admission controller's checkIfRepoExist used exact string equality to compare repository URLs. This allowed users to create a second Repository CR with the same URL by simply appending a trailing slash (e.g. https://github.com/org/repo vs https://github.com/org/repo/), bypassing the uniqueness validation.
And in matching logic we compare repository CRs by trimming suffix slash
"/"which was facilitating this flaw. if there are two repository CRs with URLs https://github.com/pac/repo and https://github.com/pac/repo/ then in matching the repo with slash would be matchedpkg/webhook/validation.go:
pkg/webhook/validation_test.go:
test/repository_webhook_test.go:
🔗 Linked GitHub Issue
Fixes #
JIRA
https://redhat.atlassian.net/browse/SRVKP-11295
🧪 Testing Strategy
🤖 AI Assistance
AI assistance can be used for various tasks, such as code generation,
documentation, or testing.
Please indicate whether you have used AI assistance
for this PR and provide details if applicable.
Important
Slop will be simply rejected, if you are using AI assistance you need to make sure you
understand the code generated and that it meets the project's standards. you
need at least know how to run the code and deploy it (if needed). See
startpaac to make it easy
to deploy and test your code changes.
If the majority of the code in this PR was generated by an AI, please add a
Co-authored-bytrailer to your commit message.For example:
Co-authored-by: Claude noreply@anthropic.com
✅ Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix anyissues. For an efficient workflow, I have considered installing
pre-commit and running
pre-commit installtoautomate these checks.