Add request validation for ListBranches and ListCommits endpoints#297
Add request validation for ListBranches and ListCommits endpoints#297
Conversation
📝 WalkthroughWalkthroughRequest validation logic for git API endpoints ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@agent-manager-service/utils/utils.go`:
- Around line 647-681: The validation trims payload.Branch, payload.Path, and
payload.Author (see payload.Branch, payload.Path, payload.Author and
isValidGitHubBranch) but never updates the original fields, so whitespace-padded
values can pass validation yet be forwarded; fix by either rejecting values that
differ after trimming or normalizing by writing the trimmed strings back into
payload (e.g., set payload.Branch = &branchVal, payload.Path = &pathVal,
payload.Author = &authorVal) after successful validation so downstream code
receives the normalized values.
- Around line 583-599: The isValidGitHubBranch function uses an overly strict
allowlist regex which excludes valid characters like '@' and '+'; update
isValidGitHubBranch to switch from the current allowlist (validPattern) to an
invalidlist approach: reject branches containing forbidden characters (~ ^ : ? *
[ \ control chars space), structural violations like "..", "@{", a single "@"
token, trailing "/" or ".lock", or ending with a dot; keep the empty-string
check and use targeted string checks/regexes to detect those forbidden patterns
rather than the current `^[a-zA-Z0-9._/-]+$` pattern so valid refs such as
dependabot scoped names pass.
| // isValidGitHubBranch validates that a branch name contains only safe characters | ||
| func isValidGitHubBranch(branch string) bool { | ||
| if branch == "" { | ||
| return false | ||
| } | ||
| // Branch names can contain alphanumeric, hyphens, underscores, periods, and forward slashes | ||
| // But reject path traversal patterns | ||
| validPattern := regexp.MustCompile(`^[a-zA-Z0-9._/-]+$`) | ||
| if !validPattern.MatchString(branch) { | ||
| return false | ||
| } | ||
| // Reject path traversal patterns | ||
| if strings.Contains(branch, "..") { | ||
| return false | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
git check-ref-format valid characters allowed disallowed branch names rules
💡 Result:
git check-ref-format refname rules (what branch/tag ref names may contain)
Git treats refnames like refs/heads/<name> (branches) and refs/tags/<name> (tags). A refname is invalid if it violates any of these rules:
/is allowed for hierarchy, but no path component may start with.or end with.lock.- Must contain at least one
/(e.g.,heads/foo) unless you pass--allow-onelevel. - Must not contain
..anywhere. - Must not contain ASCII control chars, space, or any of:
~^:. - Must not contain:
?*[(except with--refspec-pattern, which allows a single*). - Must not begin or end with
/, and must not contain//(unless you use--normalizeto collapse/clean slashes). - Must not end with
. - Must not contain the sequence
@{ - Must not be exactly
@ - Must not contain backslash
\[1]
Branch-name-specific note (--branch)
git check-ref-format --branch <name> can be stricter than validating refs/heads/<name> directly; for example, a name starting with - is rejected as a branch name even though - can be valid inside a ref component. [1][2]
Quick practical “safe” character set (recommended, not required by Git): letters, digits, ., -, _, /. [3]
Sources: man page rules: [1]; Git docs on --branch being stricter: [2]; GitHub guidance on safe characters: [3].
🌐 Web query:
git branch name @ plus sign characters allowed valid refs
💡 Result:
In Git, branch names are refnames (typically under refs/heads/), so they follow the refname rules.
+ (plus sign)
+ is allowed in ref/branch names because it’s not in Git’s list of forbidden characters/sequences for refnames. Forbidden include: space, ~, ^, :, ?, *, [, \, ASCII control chars; plus a few structural rules like no .., no ending in .lock, etc. [1]
@ (at sign)
@ is mostly allowed, with key exceptions:
- A refname cannot contain the sequence
@{(because@{...}is used for reflog syntax likeHEAD@{1}). [1] - A refname cannot be exactly the single character
@(because@is a special shorthand forHEAD). [1]
Quick validation
Use:
git check-ref-format refs/heads/<name>(validates as a refname), orgit check-ref-format --branch <name>(validates as a branch name; can be a bit stricter). [1]
Source: Git documentation for git-check-ref-format. [1]
Sources
[1] git-check-ref-format documentation (git-scm.com) (git-scm.com)
Citations:
🏁 Script executed:
cd agent-manager-service && find . -name "*.go" -type f | head -20Repository: wso2/agent-manager
Length of output: 730
🏁 Script executed:
find . -path "*/utils/utils.go" -type fRepository: wso2/agent-manager
Length of output: 100
🏁 Script executed:
sed -n '583,599p' ./agent-manager-service/utils/utils.goRepository: wso2/agent-manager
Length of output: 581
🏁 Script executed:
cat > /tmp/test_branch_validation.go << 'EOF'
package main
import (
"fmt"
"regexp"
)
func isValidGitHubBranch(branch string) bool {
if branch == "" {
return false
}
validPattern := regexp.MustCompile(`^[a-zA-Z0-9._/-]+$`)
if !validPattern.MatchString(branch) {
return false
}
if contains(branch, "..") {
return false
}
return true
}
func contains(s, substr string) bool {
for i := 0; i < len(s)-len(substr)+1; i++ {
if s[i:i+len(substr)] == substr {
return true
}
}
return false
}
func main() {
testCases := []struct {
branch string
expected bool
}{
{"main", true},
{"develop", true},
{"feature/add-feature", true},
{"bugfix_with_underscore", true},
{"v1.0.0", true},
{"dependabot/npm_and_yarn/@scope/pkg-name", false}, // contains @
{"feat/+issue-123", false}, // contains +
{"release@v1", false}, // contains @
{"feat+plus", false}, // contains +
{"feature/..", false}, // contains ..
{"feature/something..else", false}, // contains ..
}
for _, tc := range testCases {
result := isValidGitHubBranch(tc.branch)
status := "✓"
if result != tc.expected {
status = "✗"
}
fmt.Printf("%s isValidGitHubBranch(%q) = %v (expected %v)\n", status, tc.branch, result, tc.expected)
}
}
EOF
go run /tmp/test_branch_validation.goRepository: wso2/agent-manager
Length of output: 776
Branch regex too restrictive; valid Git branch names will be rejected.
The allowlist pattern ^[a-zA-Z0-9._/-]+$ excludes characters that Git permits in refs, specifically @ and +. This causes rejection of valid branch names such as dependabot/npm_and_yarn/@scope/pkg-name (used by Dependabot for scoped packages) and branches with + prefixes.
According to git check-ref-format rules, both @ and + are allowed (with the exception that @{ sequences and the single character @ are forbidden). Use an invalidlist approach targeting only truly forbidden characters (~, ^, :, ?, *, [, \, control chars, space) and structural violations (.., @{, ending with / or .lock) instead of restricting to a narrow allowlist.
🤖 Prompt for AI Agents
In `@agent-manager-service/utils/utils.go` around lines 583 - 599, The
isValidGitHubBranch function uses an overly strict allowlist regex which
excludes valid characters like '@' and '+'; update isValidGitHubBranch to switch
from the current allowlist (validPattern) to an invalidlist approach: reject
branches containing forbidden characters (~ ^ : ? * [ \ control chars space),
structural violations like "..", "@{", a single "@" token, trailing "/" or
".lock", or ending with a dot; keep the empty-string check and use targeted
string checks/regexes to detect those forbidden patterns rather than the current
`^[a-zA-Z0-9._/-]+$` pattern so valid refs such as dependabot scoped names pass.
| // Validate optional branch field if provided | ||
| if payload.Branch != nil { | ||
| branchVal := strings.TrimSpace(*payload.Branch) | ||
| if branchVal == "" { | ||
| return fmt.Errorf("branch cannot be empty or whitespace") | ||
| } | ||
| if !isValidGitHubBranch(branchVal) { | ||
| return fmt.Errorf("branch contains invalid characters or path traversal patterns") | ||
| } | ||
| } | ||
|
|
||
| // Validate optional path field if provided | ||
| if payload.Path != nil { | ||
| pathVal := strings.TrimSpace(*payload.Path) | ||
| if pathVal == "" { | ||
| return fmt.Errorf("path cannot be empty or whitespace") | ||
| } | ||
| // Path can contain slashes, but reject path traversal patterns | ||
| if strings.Contains(pathVal, "..") { | ||
| return fmt.Errorf("path contains path traversal patterns") | ||
| } | ||
| } | ||
|
|
||
| // Validate optional author field if provided | ||
| if payload.Author != nil { | ||
| authorVal := strings.TrimSpace(*payload.Author) | ||
| if authorVal == "" { | ||
| return fmt.Errorf("author cannot be empty or whitespace") | ||
| } | ||
| // Author can be a username or email, so allow @ and . characters | ||
| // But still reject path traversal patterns | ||
| if strings.Contains(authorVal, "..") || strings.Contains(authorVal, "/") { | ||
| return fmt.Errorf("author contains invalid characters or path traversal patterns") | ||
| } | ||
| } |
There was a problem hiding this comment.
Trimmed values are validated but not normalized/rejected.
Branch, Path, and Author are trimmed for validation only, but the original (possibly whitespace-padded) values are still forwarded to the service. That can pass validation yet fail downstream. Either reject leading/trailing whitespace or normalize by writing the trimmed values back.
✅ Option: reject or normalize whitespace-padded values
if payload.Branch != nil {
branchVal := strings.TrimSpace(*payload.Branch)
if branchVal == "" {
return fmt.Errorf("branch cannot be empty or whitespace")
}
+ if branchVal != *payload.Branch {
+ return fmt.Errorf("branch cannot contain leading or trailing whitespace")
+ }
if !isValidGitHubBranch(branchVal) {
return fmt.Errorf("branch contains invalid characters or path traversal patterns")
}
+ *payload.Branch = branchVal
}
if payload.Path != nil {
pathVal := strings.TrimSpace(*payload.Path)
if pathVal == "" {
return fmt.Errorf("path cannot be empty or whitespace")
}
+ if pathVal != *payload.Path {
+ return fmt.Errorf("path cannot contain leading or trailing whitespace")
+ }
if strings.Contains(pathVal, "..") {
return fmt.Errorf("path contains path traversal patterns")
}
+ *payload.Path = pathVal
}
if payload.Author != nil {
authorVal := strings.TrimSpace(*payload.Author)
if authorVal == "" {
return fmt.Errorf("author cannot be empty or whitespace")
}
+ if authorVal != *payload.Author {
+ return fmt.Errorf("author cannot contain leading or trailing whitespace")
+ }
if strings.Contains(authorVal, "..") || strings.Contains(authorVal, "/") {
return fmt.Errorf("author contains invalid characters or path traversal patterns")
}
+ *payload.Author = authorVal
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Validate optional branch field if provided | |
| if payload.Branch != nil { | |
| branchVal := strings.TrimSpace(*payload.Branch) | |
| if branchVal == "" { | |
| return fmt.Errorf("branch cannot be empty or whitespace") | |
| } | |
| if !isValidGitHubBranch(branchVal) { | |
| return fmt.Errorf("branch contains invalid characters or path traversal patterns") | |
| } | |
| } | |
| // Validate optional path field if provided | |
| if payload.Path != nil { | |
| pathVal := strings.TrimSpace(*payload.Path) | |
| if pathVal == "" { | |
| return fmt.Errorf("path cannot be empty or whitespace") | |
| } | |
| // Path can contain slashes, but reject path traversal patterns | |
| if strings.Contains(pathVal, "..") { | |
| return fmt.Errorf("path contains path traversal patterns") | |
| } | |
| } | |
| // Validate optional author field if provided | |
| if payload.Author != nil { | |
| authorVal := strings.TrimSpace(*payload.Author) | |
| if authorVal == "" { | |
| return fmt.Errorf("author cannot be empty or whitespace") | |
| } | |
| // Author can be a username or email, so allow @ and . characters | |
| // But still reject path traversal patterns | |
| if strings.Contains(authorVal, "..") || strings.Contains(authorVal, "/") { | |
| return fmt.Errorf("author contains invalid characters or path traversal patterns") | |
| } | |
| } | |
| // Validate optional branch field if provided | |
| if payload.Branch != nil { | |
| branchVal := strings.TrimSpace(*payload.Branch) | |
| if branchVal == "" { | |
| return fmt.Errorf("branch cannot be empty or whitespace") | |
| } | |
| if branchVal != *payload.Branch { | |
| return fmt.Errorf("branch cannot contain leading or trailing whitespace") | |
| } | |
| if !isValidGitHubBranch(branchVal) { | |
| return fmt.Errorf("branch contains invalid characters or path traversal patterns") | |
| } | |
| *payload.Branch = branchVal | |
| } | |
| // Validate optional path field if provided | |
| if payload.Path != nil { | |
| pathVal := strings.TrimSpace(*payload.Path) | |
| if pathVal == "" { | |
| return fmt.Errorf("path cannot be empty or whitespace") | |
| } | |
| if pathVal != *payload.Path { | |
| return fmt.Errorf("path cannot contain leading or trailing whitespace") | |
| } | |
| // Path can contain slashes, but reject path traversal patterns | |
| if strings.Contains(pathVal, "..") { | |
| return fmt.Errorf("path contains path traversal patterns") | |
| } | |
| *payload.Path = pathVal | |
| } | |
| // Validate optional author field if provided | |
| if payload.Author != nil { | |
| authorVal := strings.TrimSpace(*payload.Author) | |
| if authorVal == "" { | |
| return fmt.Errorf("author cannot be empty or whitespace") | |
| } | |
| if authorVal != *payload.Author { | |
| return fmt.Errorf("author cannot contain leading or trailing whitespace") | |
| } | |
| // Author can be a username or email, so allow @ and . characters | |
| // But still reject path traversal patterns | |
| if strings.Contains(authorVal, "..") || strings.Contains(authorVal, "/") { | |
| return fmt.Errorf("author contains invalid characters or path traversal patterns") | |
| } | |
| *payload.Author = authorVal | |
| } |
🤖 Prompt for AI Agents
In `@agent-manager-service/utils/utils.go` around lines 647 - 681, The validation
trims payload.Branch, payload.Path, and payload.Author (see payload.Branch,
payload.Path, payload.Author and isValidGitHubBranch) but never updates the
original fields, so whitespace-padded values can pass validation yet be
forwarded; fix by either rejecting values that differ after trimming or
normalizing by writing the trimmed strings back into payload (e.g., set
payload.Branch = &branchVal, payload.Path = &pathVal, payload.Author =
&authorVal) after successful validation so downstream code receives the
normalized values.
1 participant
Purpose
Resolves #257
Goals
Approach
User stories
Release note
Documentation
Training
Certification
Marketing
Automation tests
Security checks
Samples
Related PRs
Migrations (if applicable)
Test environment
Learning
Summary by CodeRabbit