Address review feedback for PR #249

Author: data-douser

client/cmd/code_scanning_apply.go

@@ -28,12 +28,14 @@ type applyAction struct {
 }
 
 type applySummary struct {
-	TotalAlerts   int  `json:"totalAlerts"`
-	DismissCount  int  `json:"dismissCount"`
-	NoChangeCount int  `json:"noChangeCount"`
-	AppliedCount  int  `json:"appliedCount"`
-	ErrorCount    int  `json:"errorCount"`
-	DryRun        bool `json:"dryRun"`
+	TotalAlerts              int  `json:"totalAlerts"`
+	DismissCount             int  `json:"dismissCount"`
+	AuthorizedDismissCount   int  `json:"authorizedDismissCount"`
+	UnauthorizedDismissCount int  `json:"unauthorizedDismissCount"`
+	NoChangeCount            int  `json:"noChangeCount"`
+	AppliedCount             int  `json:"appliedCount"`
+	ErrorCount               int  `json:"errorCount"`
+	DryRun                   bool `json:"dryRun"`
 }
 
 type applyPlan struct {
@@ -95,14 +97,25 @@ func buildApplyPlan(assessed []assessedAlert, opts applyOptions) applyPlan {
 		}
 	}
 
+	var authorizedCount, unauthorizedCount int
+	for _, a := range actions {
+		if a.Authorized {
+			authorizedCount++
+		} else {
+			unauthorizedCount++
+		}
+	}
+
 	return applyPlan{
 		GeneratedAt: time.Now().UTC().Format(time.RFC3339),
 		Actions:     actions,
 		Summary: applySummary{
-			TotalAlerts:   len(assessed),
-			DismissCount:  len(actions),
-			NoChangeCount: noChange,
-			DryRun:        opts.dryRun,
+			TotalAlerts:              len(assessed),
+			DismissCount:             len(actions),
+			AuthorizedDismissCount:   authorizedCount,
+			UnauthorizedDismissCount: unauthorizedCount,
+			NoChangeCount:            noChange,
+			DryRun:                   opts.dryRun,
 		},
 	}
 }

client/cmd/code_scanning_apply_test.go

@@ -154,3 +154,65 @@ func TestBuildApplyPlan_ReviewWithoutAcceptIsNotAuthorized(t *testing.T) {
 		t.Errorf("action = %q, want dismiss", plan.Actions[0].Action)
 	}
 }
+
+func TestBuildApplyPlan_AuthorizedVsUnauthorizedCounts(t *testing.T) {
+	assessed := []assessedAlert{
+		{Number: 1, State: "open", Rule: ruleEntry{ID: "r1"}, Recommendation: "discard"},
+		{Number: 2, State: "open", Rule: ruleEntry{ID: "r2"}, Recommendation: "review"},
+		{Number: 3, State: "open", Rule: ruleEntry{ID: "r1"}, Recommendation: "discard"},
+	}
+
+	// Only accept r1 — so alerts 1 and 3 are authorized, alert 2 is not
+	plan := buildApplyPlan(assessed, applyOptions{
+		dryRun:               true,
+		acceptChangeForRules: []string{"r1"},
+	})
+
+	if plan.Summary.DismissCount != 3 {
+		t.Errorf("dismissCount = %d, want 3 (total proposed)", plan.Summary.DismissCount)
+	}
+	if plan.Summary.AuthorizedDismissCount != 2 {
+		t.Errorf("authorizedDismissCount = %d, want 2", plan.Summary.AuthorizedDismissCount)
+	}
+	if plan.Summary.UnauthorizedDismissCount != 1 {
+		t.Errorf("unauthorizedDismissCount = %d, want 1", plan.Summary.UnauthorizedDismissCount)
+	}
+}
+
+func TestBuildApplyPlan_AllAuthorizedWhenAcceptAll(t *testing.T) {
+	assessed := []assessedAlert{
+		{Number: 1, State: "open", Rule: ruleEntry{ID: "r1"}, Recommendation: "discard"},
+		{Number: 2, State: "open", Rule: ruleEntry{ID: "r2"}, Recommendation: "review"},
+	}
+
+	plan := buildApplyPlan(assessed, applyOptions{
+		dryRun:           true,
+		acceptAllChanges: true,
+	})
+
+	if plan.Summary.AuthorizedDismissCount != 2 {
+		t.Errorf("authorizedDismissCount = %d, want 2", plan.Summary.AuthorizedDismissCount)
+	}
+	if plan.Summary.UnauthorizedDismissCount != 0 {
+		t.Errorf("unauthorizedDismissCount = %d, want 0", plan.Summary.UnauthorizedDismissCount)
+	}
+}
+
+func TestBuildApplyPlan_DiscardAutoAuthorizedCountsCorrectly(t *testing.T) {
+	assessed := []assessedAlert{
+		{Number: 1, State: "open", Rule: ruleEntry{ID: "r1"}, Recommendation: "discard"},
+	}
+
+	// No rule filter set — discard should be auto-authorized
+	plan := buildApplyPlan(assessed, applyOptions{dryRun: true})
+
+	if plan.Summary.DismissCount != 1 {
+		t.Errorf("dismissCount = %d, want 1", plan.Summary.DismissCount)
+	}
+	if plan.Summary.AuthorizedDismissCount != 1 {
+		t.Errorf("authorizedDismissCount = %d, want 1", plan.Summary.AuthorizedDismissCount)
+	}
+	if plan.Summary.UnauthorizedDismissCount != 0 {
+		t.Errorf("unauthorizedDismissCount = %d, want 0", plan.Summary.UnauthorizedDismissCount)
+	}
+}

client/cmd/code_scanning_assess.go

@@ -123,6 +123,16 @@ func assessAlerts(alerts []alertEntry) []assessedAlert {
 						"potential duplicate of dismissed alert #%d from rule %s",
 						peer.Number, peer.Rule.ID)
 				}
+
+				// If both alerts are open, mark the higher-numbered one as discard
+				if a.State == "open" && peer.State == "open" && assessed.Recommendation == "keep" {
+					if a.Number > peer.Number {
+						assessed.Recommendation = "discard"
+						assessed.RecommendReason = fmt.Sprintf(
+							"duplicate of canonical alert #%d from rule %s at same location",
+							peer.Number, peer.Rule.ID)
+					}
+				}
 			}
 		}
 

client/cmd/code_scanning_assess_test.go

@@ -155,3 +155,96 @@ func TestBuildAssessReport_ReviewCountSeparateFromKeep(t *testing.T) {
 		t.Errorf("keepDismissedCount = %d, want 1", assessReport.Summary.KeepDismissed)
 	}
 }
+
+func TestAssessAlerts_DiscardForOverlappingOpenAlerts(t *testing.T) {
+	// Two open alerts from different rules at the same location.
+	// The higher-numbered alert should be marked "discard" (lower number is canonical).
+	alerts := []alertEntry{
+		{Number: 1, State: "open", Rule: ruleEntry{ID: "js/sql-injection", Severity: "8.8"}, Location: locationEntry{Path: "src/db.js", StartLine: 42}},
+		{Number: 5, State: "open", Rule: ruleEntry{ID: "js/sql-injection-v2", Severity: "8.8"}, Location: locationEntry{Path: "src/db.js", StartLine: 42}},
+	}
+
+	assessed := assessAlerts(alerts)
+
+	var discardCount int
+	for _, a := range assessed {
+		if a.Recommendation == "discard" {
+			discardCount++
+			if a.Number != 5 {
+				t.Errorf("expected alert #5 to be discard, got alert #%d", a.Number)
+			}
+		}
+	}
+	if discardCount != 1 {
+		t.Errorf("expected 1 discard recommendation, got %d", discardCount)
+	}
+
+	// The canonical alert (lower number) keeps "keep" recommendation
+	for _, a := range assessed {
+		if a.Number == 1 && a.Recommendation != "keep" {
+			t.Errorf("canonical alert #1 recommendation = %q, want keep", a.Recommendation)
+		}
+	}
+}
+
+func TestAssessAlerts_DiscardPreservesOverlapMetadata(t *testing.T) {
+	alerts := []alertEntry{
+		{Number: 1, State: "open", Rule: ruleEntry{ID: "js/sql-injection"}, Location: locationEntry{Path: "src/db.js", StartLine: 42}},
+		{Number: 5, State: "open", Rule: ruleEntry{ID: "js/sql-injection-v2"}, Location: locationEntry{Path: "src/db.js", StartLine: 42}},
+	}
+
+	assessed := assessAlerts(alerts)
+
+	for _, a := range assessed {
+		if a.Number == 5 {
+			if len(a.OverlappingAlerts) == 0 {
+				t.Error("discarded alert #5 should list overlapping alerts")
+			}
+			if a.RecommendReason == "" {
+				t.Error("discarded alert #5 should have a recommend reason")
+			}
+		}
+	}
+}
+
+func TestAssessAlerts_DiscardNotAppliedToMixedStateOverlaps(t *testing.T) {
+	// When an open alert overlaps with a dismissed alert, it should be "review" not "discard".
+	// Discard only applies when all overlapping alerts are open.
+	alerts := []alertEntry{
+		{Number: 1, State: "dismissed", Rule: ruleEntry{ID: "js/sql-injection"}, Location: locationEntry{Path: "src/db.js", StartLine: 42},
+			DismissedReason: strPtr("false positive")},
+		{Number: 5, State: "open", Rule: ruleEntry{ID: "js/sql-injection-v2"}, Location: locationEntry{Path: "src/db.js", StartLine: 42}},
+	}
+
+	assessed := assessAlerts(alerts)
+
+	for _, a := range assessed {
+		if a.Number == 5 {
+			if a.Recommendation == "discard" {
+				t.Error("alert #5 overlapping with dismissed alert should be 'review', not 'discard'")
+			}
+			if a.Recommendation != "review" {
+				t.Errorf("alert #5 recommendation = %q, want review", a.Recommendation)
+			}
+		}
+	}
+}
+
+func TestBuildAssessReport_DiscardCount(t *testing.T) {
+	// Two open alerts at same location → one should be discarded
+	alerts := []alertEntry{
+		{Number: 1, State: "open", Rule: ruleEntry{ID: "js/sql-injection"}, Location: locationEntry{Path: "src/db.js", StartLine: 42}},
+		{Number: 5, State: "open", Rule: ruleEntry{ID: "js/sql-injection-v2"}, Location: locationEntry{Path: "src/db.js", StartLine: 42}},
+	}
+
+	report := buildReport("test/repo", nil, alerts)
+	assessed := assessAlerts(alerts)
+	assessReport := buildAssessReport(report, assessed)
+
+	if assessReport.Summary.DiscardCount != 1 {
+		t.Errorf("discardCount = %d, want 1", assessReport.Summary.DiscardCount)
+	}
+	if assessReport.Summary.KeepCount != 1 {
+		t.Errorf("keepCount = %d, want 1 (canonical alert kept)", assessReport.Summary.KeepCount)
+	}
+}

client/cmd/code_scanning_report.go

@@ -194,6 +194,10 @@ func runReport(cmd *cobra.Command, _ []string) error {
 		return err
 	}
 
+	if err := validatePerPage(reportFlags.perPage); err != nil {
+		return err
+	}
+
 	client, err := gh.NewClient()
 	if err != nil {
 		return err

client/cmd/helpers.go

@@ -6,10 +6,26 @@ import (
 )
 
 // parseRepo splits an "owner/repo" string into owner and repo components.
+// It rejects path traversal sequences and extra path separators.
 func parseRepo(nwo string) (string, string, error) {
 	parts := strings.SplitN(nwo, "/", 2)
 	if len(parts) != 2 || parts[0] == "" || parts[1] == "" {
 		return "", "", fmt.Errorf("invalid repo format %q: expected owner/repo", nwo)
 	}
-	return parts[0], parts[1], nil
+	owner, repo := parts[0], parts[1]
+	if strings.Contains(owner, "..") || strings.Contains(repo, "..") {
+		return "", "", fmt.Errorf("invalid repo format %q: path traversal not allowed", nwo)
+	}
+	if strings.Contains(repo, "/") {
+		return "", "", fmt.Errorf("invalid repo format %q: expected owner/repo", nwo)
+	}
+	return owner, repo, nil
+}
+
+// validatePerPage checks that a per-page value is within the GitHub API limit.
+func validatePerPage(perPage int) error {
+	if perPage < 1 || perPage > 100 {
+		return fmt.Errorf("invalid per-page value %d: must be between 1 and 100", perPage)
+	}
+	return nil
 }

client/cmd/helpers_test.go

@@ -1,6 +1,9 @@
 package cmd
 
-import "testing"
+import (
+	"fmt"
+	"testing"
+)
 
 func TestParseRepo_Valid(t *testing.T) {
 	owner, repo, err := parseRepo("example-owner/example-repo")
@@ -30,6 +33,67 @@ func TestParseRepo_Invalid(t *testing.T) {
 	}
 }
 
+func TestParseRepo_RejectsPathTraversal(t *testing.T) {
+	tests := []struct {
+		input string
+		desc  string
+	}{
+		{"owner/../../etc", "dot-dot in repo"},
+		{"../owner/repo", "dot-dot in owner"},
+		{"owner/../repo", "dot-dot in owner with slash"},
+		{"owner/repo/../../path", "dot-dot beyond repo"},
+		{"owner/repo/../secret", "dot-dot in repo component"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.desc, func(t *testing.T) {
+			_, _, err := parseRepo(tt.input)
+			if err == nil {
+				t.Errorf("parseRepo(%q) should reject path traversal", tt.input)
+			}
+		})
+	}
+}
+
+func TestParseRepo_RejectsPathSeparators(t *testing.T) {
+	tests := []struct {
+		input string
+		desc  string
+	}{
+		{"owner/sub/repo", "extra slash in repo"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.desc, func(t *testing.T) {
+			_, _, err := parseRepo(tt.input)
+			if err == nil {
+				t.Errorf("parseRepo(%q) should reject extra path separators", tt.input)
+			}
+		})
+	}
+}
+
+func TestValidatePerPage(t *testing.T) {
+	tests := []struct {
+		value   int
+		wantErr bool
+	}{
+		{1, false},
+		{50, false},
+		{100, false},
+		{0, true},
+		{-1, true},
+		{101, true},
+		{200, true},
+	}
+	for _, tt := range tests {
+		t.Run(fmt.Sprintf("perPage=%d", tt.value), func(t *testing.T) {
+			err := validatePerPage(tt.value)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("validatePerPage(%d) error = %v, wantErr %v", tt.value, err, tt.wantErr)
+			}
+		})
+	}
+}
+
 // strPtr returns a pointer to s.
 func strPtr(s string) *string {
 	return &s