Skip to content

Commit 5b60bc4

Browse files
sallyomclaude
andcommitted
PR Review suggestions
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> Signed-off-by: sallyom <[email protected]>
1 parent 9b7b5a7 commit 5b60bc4

File tree

13 files changed

+407
-59
lines changed

13 files changed

+407
-59
lines changed

components/backend/crd/bugfix.go

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package crd
33
import (
44
"context"
55
"fmt"
6+
"log"
67

78
"ambient-code-backend/types"
89

@@ -113,8 +114,16 @@ func GetProjectBugFixWorkflowCR(dyn dynamic.Interface, project, id string) (*typ
113114

114115
spec, found, _ := unstructured.NestedMap(obj.Object, "spec")
115116
if found {
117+
// Parse githubIssueNumber with type safety - handle both int64 and float64
118+
// JSON unmarshaling can produce either depending on the source
116119
if val, ok := spec["githubIssueNumber"].(int64); ok {
117120
workflow.GithubIssueNumber = int(val)
121+
} else if val, ok := spec["githubIssueNumber"].(float64); ok {
122+
workflow.GithubIssueNumber = int(val)
123+
} else if spec["githubIssueNumber"] != nil {
124+
// Log warning if type assertion fails - helps debug unexpected types
125+
log.Printf("Warning: githubIssueNumber has unexpected type %T in BugFixWorkflow %s/%s",
126+
spec["githubIssueNumber"], project, id)
118127
}
119128
if val, ok := spec["githubIssueURL"].(string); ok {
120129
workflow.GithubIssueURL = val
@@ -144,16 +153,22 @@ func GetProjectBugFixWorkflowCR(dyn dynamic.Interface, project, id string) (*typ
144153
workflow.LastSyncedAt = &val
145154
}
146155

147-
// Parse implementationRepo
156+
// Parse implementationRepo (required field)
148157
if implMap, ok := spec["implementationRepo"].(map[string]interface{}); ok {
149158
repo := types.GitRepository{}
150-
if url, ok := implMap["url"].(string); ok {
159+
if url, ok := implMap["url"].(string); ok && url != "" {
151160
repo.URL = url
161+
} else {
162+
// Critical field missing - return error instead of zero value
163+
return nil, fmt.Errorf("BugFixWorkflow %s/%s missing required field: implementationRepo.url", project, id)
152164
}
153165
if branch, ok := implMap["branch"].(string); ok && branch != "" {
154166
repo.Branch = &branch
155167
}
156168
workflow.ImplementationRepo = repo
169+
} else {
170+
// implementationRepo is required
171+
return nil, fmt.Errorf("BugFixWorkflow %s/%s missing required field: implementationRepo", project, id)
157172
}
158173
}
159174

components/backend/git/operations.go

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,11 @@ func CheckRepoSeeding(ctx context.Context, repoURL string, branch *string, githu
206206

207207
// ParseGitHubURL extracts owner and repo from a GitHub URL
208208
func ParseGitHubURL(gitURL string) (owner, repo string, err error) {
209+
// Only HTTPS URLs are supported (no SSH git@ URLs)
210+
if !strings.HasPrefix(gitURL, "https://") {
211+
return "", "", fmt.Errorf("invalid URL scheme: must be https://")
212+
}
213+
209214
gitURL = strings.TrimSuffix(gitURL, ".git")
210215

211216
if strings.Contains(gitURL, "github.com") {
@@ -218,7 +223,15 @@ func ParseGitHubURL(gitURL string) (owner, repo string, err error) {
218223
if len(pathParts) < 2 {
219224
return "", "", fmt.Errorf("invalid GitHub URL path")
220225
}
221-
return pathParts[0], pathParts[1], nil
226+
227+
// Validate owner and repo are non-empty
228+
owner = pathParts[0]
229+
repo = pathParts[1]
230+
if owner == "" || repo == "" {
231+
return "", "", fmt.Errorf("owner and repository name cannot be empty")
232+
}
233+
234+
return owner, repo, nil
222235
}
223236

224237
return "", "", fmt.Errorf("not a GitHub URL")

components/backend/handlers/bugfix/create.go

Lines changed: 98 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"fmt"
55
"log"
66
"net/http"
7+
"regexp"
78
"strings"
89
"time"
910

@@ -24,6 +25,42 @@ var (
2425
GetProjectSettingsResource func() schema.GroupVersionResource
2526
)
2627

28+
// Input validation constants to prevent oversized inputs
29+
const (
30+
MaxTitleLength = 200 // GitHub Issue title limit is 256, use 200 for safety
31+
MaxSymptomsLength = 10000 // ~10KB for symptoms description
32+
MaxReproductionStepsLength = 10000 // ~10KB for reproduction steps
33+
MaxExpectedBehaviorLength = 5000 // ~5KB for expected behavior
34+
MaxActualBehaviorLength = 5000 // ~5KB for actual behavior
35+
MaxAdditionalContextLength = 10000 // ~10KB for additional context
36+
MaxBranchNameLength = 255 // Git branch name limit
37+
)
38+
39+
// validBranchNameRegex defines allowed characters in branch names
40+
// Allows: letters, numbers, hyphens, underscores, forward slashes, and dots
41+
// Prevents: shell metacharacters, backticks, quotes, semicolons, pipes, etc.
42+
var validBranchNameRegex = regexp.MustCompile(`^[a-zA-Z0-9/_.-]+$`)
43+
44+
// validateBranchName checks if a branch name is safe to use in git operations
45+
// Returns error if the branch name contains potentially dangerous characters
46+
func validateBranchName(branchName string) error {
47+
if branchName == "" {
48+
return fmt.Errorf("branch name cannot be empty")
49+
}
50+
if !validBranchNameRegex.MatchString(branchName) {
51+
return fmt.Errorf("branch name contains invalid characters (allowed: a-z, A-Z, 0-9, /, _, -, .)")
52+
}
53+
// Prevent branch names that start with special characters
54+
if strings.HasPrefix(branchName, ".") || strings.HasPrefix(branchName, "-") {
55+
return fmt.Errorf("branch name cannot start with '.' or '-'")
56+
}
57+
// Prevent branch names with ".." (path traversal) or "//" (double slashes)
58+
if strings.Contains(branchName, "..") || strings.Contains(branchName, "//") {
59+
return fmt.Errorf("branch name cannot contain '..' or '//'")
60+
}
61+
return nil
62+
}
63+
2764
// CreateProjectBugFixWorkflow handles POST /api/projects/:projectName/bugfix-workflows
2865
// Creates a new BugFix Workspace from either GitHub Issue URL or text description
2966
func CreateProjectBugFixWorkflow(c *gin.Context) {
@@ -94,18 +131,37 @@ func CreateProjectBugFixWorkflow(c *gin.Context) {
94131
c.JSON(http.StatusBadRequest, gin.H{"error": "Title is required"})
95132
return
96133
}
97-
if len(strings.TrimSpace(td.Title)) < 10 {
134+
titleLen := len(strings.TrimSpace(td.Title))
135+
if titleLen < 10 {
98136
c.JSON(http.StatusBadRequest, gin.H{"error": "Title must be at least 10 characters"})
99137
return
100138
}
139+
if titleLen > MaxTitleLength {
140+
c.JSON(http.StatusBadRequest, gin.H{
141+
"error": fmt.Sprintf("Title exceeds maximum length of %d characters", MaxTitleLength),
142+
"current": titleLen,
143+
"max": MaxTitleLength,
144+
})
145+
return
146+
}
147+
101148
if strings.TrimSpace(td.Symptoms) == "" {
102149
c.JSON(http.StatusBadRequest, gin.H{"error": "Symptoms are required"})
103150
return
104151
}
105-
if len(strings.TrimSpace(td.Symptoms)) < 20 {
152+
symptomsLen := len(strings.TrimSpace(td.Symptoms))
153+
if symptomsLen < 20 {
106154
c.JSON(http.StatusBadRequest, gin.H{"error": "Symptoms must be at least 20 characters"})
107155
return
108156
}
157+
if symptomsLen > MaxSymptomsLength {
158+
c.JSON(http.StatusBadRequest, gin.H{
159+
"error": fmt.Sprintf("Symptoms exceed maximum length of %d characters", MaxSymptomsLength),
160+
"current": symptomsLen,
161+
"max": MaxSymptomsLength,
162+
})
163+
return
164+
}
109165
if strings.TrimSpace(td.TargetRepository) == "" {
110166
c.JSON(http.StatusBadRequest, gin.H{"error": "Target repository is required"})
111167
return
@@ -118,6 +174,40 @@ func CreateProjectBugFixWorkflow(c *gin.Context) {
118174
return
119175
}
120176

177+
// Validate optional field lengths
178+
if td.ReproductionSteps != nil && len(*td.ReproductionSteps) > MaxReproductionStepsLength {
179+
c.JSON(http.StatusBadRequest, gin.H{
180+
"error": fmt.Sprintf("Reproduction steps exceed maximum length of %d characters", MaxReproductionStepsLength),
181+
"current": len(*td.ReproductionSteps),
182+
"max": MaxReproductionStepsLength,
183+
})
184+
return
185+
}
186+
if td.ExpectedBehavior != nil && len(*td.ExpectedBehavior) > MaxExpectedBehaviorLength {
187+
c.JSON(http.StatusBadRequest, gin.H{
188+
"error": fmt.Sprintf("Expected behavior exceeds maximum length of %d characters", MaxExpectedBehaviorLength),
189+
"current": len(*td.ExpectedBehavior),
190+
"max": MaxExpectedBehaviorLength,
191+
})
192+
return
193+
}
194+
if td.ActualBehavior != nil && len(*td.ActualBehavior) > MaxActualBehaviorLength {
195+
c.JSON(http.StatusBadRequest, gin.H{
196+
"error": fmt.Sprintf("Actual behavior exceeds maximum length of %d characters", MaxActualBehaviorLength),
197+
"current": len(*td.ActualBehavior),
198+
"max": MaxActualBehaviorLength,
199+
})
200+
return
201+
}
202+
if td.AdditionalContext != nil && len(*td.AdditionalContext) > MaxAdditionalContextLength {
203+
c.JSON(http.StatusBadRequest, gin.H{
204+
"error": fmt.Sprintf("Additional context exceeds maximum length of %d characters", MaxAdditionalContextLength),
205+
"current": len(*td.AdditionalContext),
206+
"max": MaxAdditionalContextLength,
207+
})
208+
return
209+
}
210+
121211
// Generate issue body from template
122212
reproSteps := ""
123213
if td.ReproductionSteps != nil {
@@ -162,9 +252,14 @@ func CreateProjectBugFixWorkflow(c *gin.Context) {
162252
}
163253

164254
// Auto-generate branch name if not provided
165-
branch := "main"
255+
var branch string
166256
if req.BranchName != nil && *req.BranchName != "" {
167257
branch = *req.BranchName
258+
// Validate user-provided branch name for security
259+
if err := validateBranchName(branch); err != nil {
260+
c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid branch name", "details": err.Error()})
261+
return
262+
}
168263
} else {
169264
// Auto-generate branch name: bugfix/gh-{issue-number}
170265
branch = fmt.Sprintf("bugfix/gh-%d", githubIssue.Number)

components/backend/handlers/bugfix/handlers_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ func TestCreateBugFixWorkflow(t *testing.T) {
1111
// Test cases:
1212
// 1. Valid GitHub Issue URL -> 201 Created
1313
// 2. Invalid GitHub Issue URL -> 400 Bad Request
14-
// 3. Missing umbrellaRepo -> 400 Bad Request
14+
// 3. Missing implementationRepo -> 400 Bad Request
1515
// 4. Duplicate workspace (same issue number) -> 409 Conflict
1616
// 5. Text description with valid targetRepository -> 201 Created
1717
// 6. Both githubIssueURL and textDescription -> 400 Bad Request

components/backend/handlers/bugfix/jira_sync.go

Lines changed: 14 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"encoding/json"
77
"fmt"
88
"io"
9+
"log"
910
"net/http"
1011
"strings"
1112
"time"
@@ -152,6 +153,7 @@ func SyncProjectBugFixWorkflowToJira(c *gin.Context) {
152153
req.Header.Set("Content-Type", "application/json")
153154
req.Header.Set("Accept", "application/json")
154155

156+
// Use context from request for proper cancellation propagation
155157
client := &http.Client{Timeout: 30 * time.Second}
156158
resp, err := client.Do(req)
157159
if err != nil {
@@ -187,26 +189,29 @@ func SyncProjectBugFixWorkflowToJira(c *gin.Context) {
187189
return
188190
case 404:
189191
websocket.BroadcastBugFixJiraSyncFailed(workflowID, workflow.GithubIssueNumber, "Jira project not found")
190-
c.JSON(http.StatusBadRequest, gin.H{"error": "Jira project not found", "details": string(body)})
192+
// Don't expose Jira error details to user - may contain sensitive info
193+
log.Printf("Jira 404 error details: %s", string(body))
194+
c.JSON(http.StatusBadRequest, gin.H{"error": "Jira project not found"})
191195
return
192196
default:
193-
websocket.BroadcastBugFixJiraSyncFailed(workflowID, workflow.GithubIssueNumber, fmt.Sprintf("Jira API error: %s", string(body)))
194-
c.JSON(http.StatusServiceUnavailable, gin.H{"error": fmt.Sprintf("Failed to create Jira issue (status %d)", resp.StatusCode), "details": string(body)})
197+
websocket.BroadcastBugFixJiraSyncFailed(workflowID, workflow.GithubIssueNumber, "Jira API error")
198+
// Log details for debugging, but don't expose to user
199+
log.Printf("Jira API error (status %d): %s", resp.StatusCode, string(body))
200+
c.JSON(http.StatusServiceUnavailable, gin.H{"error": fmt.Sprintf("Failed to create Jira issue (status %d)", resp.StatusCode)})
195201
return
196202
}
197203

198204
// Parse JSON response
199205
var result map[string]interface{}
200206
if err := json.Unmarshal(body, &result); err != nil {
201-
// Log the raw response for debugging
202-
fmt.Printf("ERROR: Failed to parse Jira response as JSON: %v\n", err)
207+
// Log the raw response for debugging (server-side only)
208+
log.Printf("ERROR: Failed to parse Jira response as JSON: %v", err)
203209
bodyLen := len(body)
204-
fmt.Printf("Response body (first 500 chars): %s\n", string(body[:min(500, bodyLen)]))
210+
log.Printf("Response body (first 500 chars): %s", string(body[:min(500, bodyLen)]))
205211
websocket.BroadcastBugFixJiraSyncFailed(workflowID, workflow.GithubIssueNumber, "Invalid Jira response")
212+
// Don't expose Jira response body to user - may contain sensitive details
206213
c.JSON(http.StatusInternalServerError, gin.H{
207-
"error": "Failed to parse Jira response",
208-
"details": err.Error(),
209-
"responsePreview": string(body[:min(200, bodyLen)]),
214+
"error": "Failed to parse Jira response",
210215
})
211216
return
212217
}
@@ -422,40 +427,6 @@ func formatGitHubJiraLinkComment(jiraTaskKey, jiraTaskURL string, workflow *type
422427
return comment.String()
423428
}
424429

425-
// formatGitHubJiraUpdateComment formats the comment to post on GitHub Issue when updating Jira task
426-
func formatGitHubJiraUpdateComment(jiraTaskKey, jiraTaskURL string, workflow *types.BugFixWorkflow) string {
427-
var comment strings.Builder
428-
429-
comment.WriteString("## 🔄 Jira Task Updated\n\n")
430-
comment.WriteString(fmt.Sprintf("Jira task [**%s**](%s) has been updated with the latest information.\n\n", jiraTaskKey, jiraTaskURL))
431-
432-
// Add links to analysis documents if available
433-
if workflow.Annotations != nil {
434-
hasGists := false
435-
if bugReviewGist := workflow.Annotations["bug-review-gist-url"]; bugReviewGist != "" {
436-
if !hasGists {
437-
comment.WriteString("### 📄 Latest Analysis\n\n")
438-
hasGists = true
439-
}
440-
comment.WriteString(fmt.Sprintf("- [Bug Review & Assessment](%s)\n", bugReviewGist))
441-
}
442-
if implGist := workflow.Annotations["implementation-gist-url"]; implGist != "" {
443-
if !hasGists {
444-
comment.WriteString("### 📄 Latest Analysis\n\n")
445-
hasGists = true
446-
}
447-
comment.WriteString(fmt.Sprintf("- [Implementation Details](%s)\n", implGist))
448-
}
449-
if hasGists {
450-
comment.WriteString("\n")
451-
}
452-
}
453-
454-
comment.WriteString("*Synchronized by vTeam BugFix Workspace*")
455-
456-
return comment.String()
457-
}
458-
459430
// getSuccessMessage returns appropriate success message
460431
func getSuccessMessage(created bool, jiraTaskKey string) string {
461432
if created {

0 commit comments

Comments
 (0)