Merge pull request #108 from flashcatcloud/fix/incident-comment-shell-safe

fix(incident): take the comment from a file and verify it landed, instead of shell-quoting it and assuming

Author: ysyneu

internal/cli/alert.go

@@ -19,8 +19,7 @@ func newAlertCmd() *cobra.Command {
 	cmd.AddCommand(newAlertGetCmd())
 	cmd.AddCommand(newAlertEventsCmd())
 	cmd.AddCommand(newAlertTimelineCmd())
-	// merge is registered via the generated layer (positional alert-ids fold to
-	// alert_ids). Flag-name change: --incident (curated) → --incident-id (generated).
+	cmd.AddCommand(newAlertMergeCmd())
 	return cmd
 }
 
@@ -280,6 +279,104 @@ func newAlertTimelineCmd() *cobra.Command {
 	return cmd
 }
 
+// newAlertMergeCmd curates the generated alert-write-merge command solely to
+// replace its inline --comment string with --comment-file, closing the same
+// shell-interpolation exposure that motivated incident comment's
+// --comment-file (see newIncidentCommentCmd): an LLM-authored merge comment
+// containing backticks, $(...), or quotes must never be typed into a shell
+// argument. Everything else (positional alert IDs, --alert-ids, --incident-id,
+// --owner-id, --title, --data) mirrors the generated command byte-for-byte via
+// the same genAssembleBody/genBindBody request-building helpers, so this is a
+// pure flag-source swap, not a behavior change. Registering it here (curated
+// commands are added before registerGenerated runs, see root.go) makes
+// genAddLeaf skip attaching the generated "merge" leaf under this same name.
+func newAlertMergeCmd() *cobra.Command {
+	var dataJSON string
+	var fAlertIDs []string
+	var fCommentFile string
+	var fIncidentID string
+	var fOwnerID int64
+	var fTitle string
+
+	cmd := &cobra.Command{
+		Use:   "merge <alert-id> [<id2>...]",
+		Short: "Merge alerts into an incident",
+		Long: `Merge alerts into an incident.
+
+Associate one or more alerts with an existing incident. If a source alert previously belonged to a different incident and that incident becomes empty after the merge, it will be automatically closed.
+
+An optional comment on the merge action is read verbatim from --comment-file
+(or from stdin, when the value is "-"); it is never parsed by a shell, so
+backticks, $(...), and quotes inside it reach the API exactly as written.
+
+API: POST /alert/merge (alert-write-merge)
+
+Request fields:
+  --alert-ids []string (required) — Alert IDs to merge.
+  --comment-file string — Path to a file containing an optional comment on the merge action (- reads stdin).
+  --incident-id string (required) — Target incident ID.
+  --owner-id int — Optional new owner for the target incident.
+  --title string — Optional new title for the target incident.
+`,
+		Args:    requireBodyFieldOrArgs("alert_ids", "alert-ids"),
+		Example: `  flashduty alert merge <alert-id1> <alert-id2> --incident-id <incident-id> --comment-file ./comment.txt`,
+		RunE: func(cmd *cobra.Command, args []string) error {
+			return runCommand(cmd, args, func(ctx *RunContext) error {
+				body, err := genAssembleBody(dataJSON, func(body map[string]any) error {
+					if err := genFoldPositional(args, body, "alert_ids", "slice"); err != nil {
+						return err
+					}
+					if cmd.Flags().Changed("alert-ids") {
+						body["alert_ids"] = fAlertIDs
+					}
+					if cmd.Flags().Changed("comment-file") {
+						comment, err := resolveCommentFile(fCommentFile)
+						if err != nil {
+							return err
+						}
+						body["comment"] = comment
+					}
+					if cmd.Flags().Changed("incident-id") {
+						body["incident_id"] = fIncidentID
+					}
+					if cmd.Flags().Changed("owner-id") {
+						body["owner_id"] = fOwnerID
+					}
+					if cmd.Flags().Changed("title") {
+						body["title"] = fTitle
+					}
+					return nil
+				})
+				if err != nil {
+					return err
+				}
+				req := new(flashduty.AlertMergeRequest)
+				if err := genBindBody(body, req); err != nil {
+					return err
+				}
+				resp, err := ctx.Client.Alerts.WriteMerge(cmdContext(ctx.Cmd), req)
+				if err != nil {
+					return err
+				}
+				if resp != nil && len(resp.Raw) > 0 {
+					return ctx.WriteRaw(resp.Raw)
+				}
+				ctx.WriteResult("OK: POST /alert/merge")
+				return nil
+			})
+		},
+	}
+
+	cmd.Flags().StringSliceVar(&fAlertIDs, "alert-ids", nil, "Alert IDs to merge. (required)")
+	cmd.Flags().StringVar(&fCommentFile, "comment-file", "", "Path to a file containing an optional comment on the merge action (- reads stdin)")
+	cmd.Flags().StringVar(&fIncidentID, "incident-id", "", "Target incident ID. (required)")
+	cmd.Flags().Int64Var(&fOwnerID, "owner-id", 0, "Optional new owner for the target incident.")
+	cmd.Flags().StringVar(&fTitle, "title", "", "Optional new title for the target incident.")
+	cmd.Flags().StringVar(&dataJSON, "data", "", "Full request body as JSON; positional arguments and typed flags override its fields. Accepts inline JSON, or - to read stdin.")
+
+	return cmd
+}
+
 // resolveAlertFeedOperators resolves the actor (creator) person IDs of
 // alert-feed items to display names via /person/infos, replicating the
 // operator-name enrichment the legacy SDK did server-side. Best-effort: a

internal/cli/alert_test.go

@@ -1,6 +1,7 @@
 package cli
 
 import (
+	"strings"
 	"testing"
 )
 
@@ -60,3 +61,73 @@ func TestCommandAlertListNoStatusFilterOmitsIsActive(t *testing.T) {
 		t.Errorf("ever_muted should be omitted without --muted, got %#v", stub.lastBody["ever_muted"])
 	}
 }
+
+// TestCommandAlertMergeCommentFileReachesWireByteForByte guards the same
+// shell-interpolation fix applied to incident comment (see
+// TestCommandIncidentCommentPreservesShellMetacharactersByteForByte): alert
+// merge's comment must come from --comment-file, never an inline shell
+// argument, so backticks, $(...), and quotes inside an LLM-authored comment
+// reach the API exactly as written.
+func TestCommandAlertMergeCommentFileReachesWireByteForByte(t *testing.T) {
+	saveAndResetGlobals(t)
+	stub := newGFStub(t)
+
+	malicious := "Root cause: restart via `kubectl rollout restart deploy/api`.\n" +
+		"Then ran $(rm -rf /tmp/scratch) to clean up staging state.\n" +
+		"Quotes: \"double\" and 'single' and it's mine.\n"
+	commentFile := writeCommentFile(t, malicious)
+
+	out, err := execCommand("alert", "merge", "alert-1", "alert-2",
+		"--incident-id", "inc-1", "--comment-file", commentFile)
+	if err != nil {
+		t.Fatalf("[alert-merge-comment-file] unexpected error: %v", err)
+	}
+	if stub.lastPath != "/alert/merge" {
+		t.Fatalf("[alert-merge-comment-file] expected /alert/merge, got %q", stub.lastPath)
+	}
+	if stub.lastBody["comment"] != malicious {
+		t.Fatalf("[alert-merge-comment-file] comment reached the API mangled:\nwant: %q\n got: %q", malicious, stub.lastBody["comment"])
+	}
+	if got, want := strings.Join(stringsField(stub.lastBody, "alert_ids"), ","), "alert-1,alert-2"; got != want {
+		t.Fatalf("[alert-merge-comment-file] expected alert_ids %q, got %q", want, got)
+	}
+	if stub.lastBody["incident_id"] != "inc-1" {
+		t.Fatalf("[alert-merge-comment-file] expected incident_id %q, got %#v", "inc-1", stub.lastBody["incident_id"])
+	}
+	if !strings.Contains(out, "OK: POST /alert/merge") {
+		t.Fatalf("[alert-merge-comment-file] unexpected output:\n%s", out)
+	}
+}
+
+// TestCommandAlertMergeWithoutCommentFileOmitsComment guards that the merge
+// comment stays optional now that it is sourced from a file: not passing
+// --comment-file must not send an empty "comment" field.
+func TestCommandAlertMergeWithoutCommentFileOmitsComment(t *testing.T) {
+	saveAndResetGlobals(t)
+	stub := newGFStub(t)
+
+	if _, err := execCommand("alert", "merge", "alert-1", "--incident-id", "inc-1"); err != nil {
+		t.Fatalf("[alert-merge-no-comment] unexpected error: %v", err)
+	}
+	if _, ok := stub.lastBody["comment"]; ok {
+		t.Fatalf("[alert-merge-no-comment] comment should be omitted, got %#v", stub.lastBody["comment"])
+	}
+}
+
+// TestCommandAlertMergeEmptyCommentFileGivesCleanError guards routing alert
+// merge's optional --comment-file through the same resolveCommentFile helper
+// incident comment uses: an explicit but empty --comment-file value must fail
+// with resolveCommentFile's clean "must not be empty" message, not the raw
+// os.ReadFile("") error.
+func TestCommandAlertMergeEmptyCommentFileGivesCleanError(t *testing.T) {
+	saveAndResetGlobals(t)
+	newGFStub(t)
+
+	_, err := execCommand("alert", "merge", "alert-1", "--incident-id", "inc-1", "--comment-file", "")
+	if err == nil {
+		t.Fatal("[alert-merge-empty-comment-file] expected an error, got nil")
+	}
+	if !strings.Contains(err.Error(), "--comment-file must not be empty") {
+		t.Fatalf("[alert-merge-empty-comment-file] expected the clean resolveCommentFile message, got: %v", err)
+	}
+}

internal/cli/automation.go

@@ -2,7 +2,6 @@ package cli
 
 import (
 	"fmt"
-	"io"
 	"os"
 	"strconv"
 	"strings"
@@ -527,15 +526,7 @@ func resolveAutomationPrompt(cmd *cobra.Command, prompt, promptFile string) (str
 		if promptFile == "" {
 			return "", fmt.Errorf("--prompt-file must not be empty")
 		}
-		var (
-			b   []byte
-			err error
-		)
-		if promptFile == "-" {
-			b, err = io.ReadAll(stdinReader)
-		} else {
-			b, err = os.ReadFile(promptFile)
-		}
+		b, err := readPathOrStdin(promptFile)
 		if err != nil {
 			return "", fmt.Errorf("failed to read prompt file: %w", err)
 		}