feat:wildcard repository url#2733
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for glob and regex patterns when matching and validating repository URLs. The review feedback highlights several critical issues: the regex implementation is vulnerable to ReDoS and uses incorrect substring matching, the validation logic is too permissive because literal strings often compile as valid patterns, and URL normalization needs to be more robust. It is recommended to remove regex support in favor of glob matching, improve normalization using strings.TrimSpace and strings.TrimRight, and refine the GitHub path segment check to only trigger when wildcards are absent.
| // matchTarget checks if a branch matches a target pattern using glob matching. | ||
| // Supports both exact string matching and glob patterns. | ||
| func matchRepo(repo, target string) (bool, error) { | ||
| if target == repo { | ||
| return true, nil | ||
| } | ||
| // Check unix glob match | ||
| globPattern, err := glob.Compile(target) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| if globPattern.Match(repo) { | ||
| return true, nil | ||
| } | ||
| // Check regex match | ||
| if reMatch, err := regexp.MatchString(target, repo); err != nil { | ||
| return false, err | ||
| } else if reMatch { | ||
| return true, nil | ||
| } | ||
| return false, nil | ||
| } |
There was a problem hiding this comment.
The implementation of matchRepo has several critical issues:
- Incorrect Matching Logic:
regexp.MatchStringperforms substring matching by default. For repository matching, you should use full-string matching. - Redundancy and Security: The
globlibrary is sufficient and safer. Regular expressions are prone to ReDoS. Per project rules, if regular expressions are used, they must be sanitized at a centralized point, but removing regex entirely in favor of glob is preferred here. - Performance: Compiling patterns inside a loop for every event is inefficient.
I suggest removing the regexp support and sticking to glob for wildcard matching.
// matchRepo checks if a repository URL matches a target pattern using glob matching.
// Supports both exact string matching and glob patterns.
func matchRepo(repo, target string) (bool, error) {
if target == repo {
return true, nil
}
// Check unix glob match
globPattern, err := glob.Compile(target)
if err != nil {
return false, fmt.Errorf("invalid glob pattern %q: %w", target, err)
}
return globPattern.Match(repo), nil
}References
- Sanitize user-provided input for components like regular expressions at a centralized point to avoid redundant sanitization logic.
30da1fa to
679d058
Compare
679d058 to
b064dc4
Compare
📝 Description of the Change
This change introduce posibility to match repository over glob or regex pattern making able to setup a single repository cr to handle set of repositories
🔗 Linked GitHub Issue
Fixes #1246
🧪 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.