diff --git a/pkg/compose/publish.go b/pkg/compose/publish.go index b3616030dfe..55ebe8cf485 100644 --- a/pkg/compose/publish.go +++ b/pkg/compose/publish.go @@ -199,7 +199,7 @@ func (s *composeService) createLayers(ctx context.Context, project *types.Projec for _, file := range project.ComposeFiles { data, err := processFile(ctx, file, project, extFiles, envFiles) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to process compose file %s: %w", file, err) } layerDescriptor := oci.DescriptorForComposeFile(file, data) @@ -235,7 +235,7 @@ func processExtends(ctx context.Context, project *types.Project, extFiles map[st for xf, hash := range extFiles { data, err := processFile(ctx, xf, project, moreExtFiles, envFiles) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to process compose file %s: %w", xf, err) } layerDescriptor := oci.DescriptorForComposeFile(hash, data) diff --git a/pkg/compose/publish_test.go b/pkg/compose/publish_test.go index 79b3884da57..5036c0a966f 100644 --- a/pkg/compose/publish_test.go +++ b/pkg/compose/publish_test.go @@ -112,6 +112,47 @@ services: }, cmp.Ignore())) } +// a processFile failure must say which file it came from: a project can +// carry many compose files and its extends references add more, so an +// unattributed error leaves the user guessing which one to fix. The +// flow-style file makes the assertions meaningful: the rejection itself +// does not name the file, so only the wrap satisfies them. +func Test_createLayers_error_includes_file_name(t *testing.T) { + dir := t.TempDir() + flowFile := filepath.Join(dir, "flow.yaml") + assert.NilError(t, os.WriteFile(flowFile, []byte(`{services: {test: {env_file: old.env}}}`), 0o600)) + // the env file must exist, or the loader rejects it before the + // flow-style rewrite gets a chance to fail + assert.NilError(t, os.WriteFile(filepath.Join(dir, "old.env"), []byte("FOO=bar\n"), 0o600)) + + project := &types.Project{ + Name: "test", + WorkingDir: dir, + Environment: types.Mapping{}, + ComposeFiles: []string{flowFile}, + } + + service := &composeService{} + _, err := service.createLayers(t.Context(), project, api.PublishOptions{}) + assert.ErrorContains(t, err, "failed to process compose file "+flowFile) + assert.ErrorContains(t, err, "flow style") +} + +func Test_processExtends_error_includes_file_name(t *testing.T) { + dir := t.TempDir() + flowFile := filepath.Join(dir, "flow.yaml") + assert.NilError(t, os.WriteFile(flowFile, []byte(`{services: {test: {env_file: old.env}}}`), 0o600)) + assert.NilError(t, os.WriteFile(filepath.Join(dir, "old.env"), []byte("FOO=bar\n"), 0o600)) + + _, err := processExtends(t.Context(), &types.Project{ + Name: "test", + WorkingDir: dir, + Environment: types.Mapping{}, + }, map[string]string{flowFile: "deadbeef"}) + assert.ErrorContains(t, err, "failed to process compose file "+flowFile) + assert.ErrorContains(t, err, "flow style") +} + func Test_preChecks_sensitive_data_detected_decline(t *testing.T) { dir := t.TempDir() envPath := dir + "/secrets.env" diff --git a/pkg/compose/transform/replace.go b/pkg/compose/transform/replace.go index d15dc1d0bc7..d41d27bcc03 100644 --- a/pkg/compose/transform/replace.go +++ b/pkg/compose/transform/replace.go @@ -58,7 +58,10 @@ func ReplaceExtendsFile(in []byte, serviceName string, value string) ([]byte, er } // we've found target `file` yaml node. Let's replace value in stream at node position - return replace(in, file.Line, file.Column, value), nil + if isFlowStyle(root, services, target, extends, file) { + return nil, fmt.Errorf("service %q: extends.file is in a flow-style mapping, which cannot be rewritten while preserving formatting", serviceName) + } + return replace(in, file.Line, file.Column, value) } // ReplaceEnvFile changes value for service.extends.env_file in input yaml stream, preserving formatting @@ -93,6 +96,12 @@ func ReplaceEnvFile(in []byte, serviceName string, i int, value string) ([]byte, // env_file can be either a string, sequence of strings, or sequence of mappings with path attribute if envFile.Kind == yaml.SequenceNode { + if i < 0 || i >= len(envFile.Content) { + return nil, fmt.Errorf("service %q: env_file has %d entries, cannot rewrite entry %d", serviceName, len(envFile.Content), i) + } + if isFlowStyle(root, services, target, envFile) || isFlowStyle(envFile.Content[i]) { + return nil, fmt.Errorf("service %q: env_file is in flow style, which cannot be rewritten while preserving formatting", serviceName) + } envFile = envFile.Content[i] if envFile.Kind == yaml.MappingNode { envFile, err = getMapping(envFile, "path") @@ -100,9 +109,12 @@ func ReplaceEnvFile(in []byte, serviceName string, i int, value string) ([]byte, return nil, err } } - return replace(in, envFile.Line, envFile.Column, value), nil + return replace(in, envFile.Line, envFile.Column, value) } else { - return replace(in, envFile.Line, envFile.Column, value), nil + if isFlowStyle(root, services, target, envFile) { + return nil, fmt.Errorf("service %q: env_file is in flow style, which cannot be rewritten while preserving formatting", serviceName) + } + return replace(in, envFile.Line, envFile.Column, value) } } @@ -123,20 +135,34 @@ func getMapping(root *yaml.Node, key string) (*yaml.Node, error) { } // replace changes yaml node value in stream at position, preserving content -func replace(in []byte, line int, column int, value string) []byte { +func replace(in []byte, line int, column int, value string) ([]byte, error) { var out []byte l := 1 pos := 0 - for _, b := range in { - if b == '\n' { - l++ - if l == line { - break + // the scan looks for the newline that precedes the target line; line 1 + // has none, so without the guard pos would run to the end of the stream + // and the column offset below would slice out of range + if line > 1 { + for _, b := range in { + if b == '\n' { + l++ + if l == line { + break + } } + pos++ } - pos++ + // pos stopped at the newline before the target line, one byte before + // the line start; columns are 1-based, so the value starts at + // line-start + column - 1, which is pos + column + pos += column + } else { + // line 1 starts at offset 0, so the 1-based column maps to column - 1 + pos = column - 1 + } + if pos > len(in) { + return nil, fmt.Errorf("cannot locate line %d column %d in stream", line, column) } - pos += column out = append(out, in[0:pos]...) out = append(out, []byte(value)...) for ; pos < len(in); pos++ { @@ -145,5 +171,21 @@ func replace(in []byte, line int, column int, value string) []byte { } } out = append(out, in[pos:]...) - return out + return out, nil +} + +// isFlowStyle reports whether any node on the path to a rewritten value is +// in flow style. replace rewrites from the value's start to the end of its +// line — anything after the value on that line, a trailing comment +// included, is dropped — so it is only safe when the value is the sole +// content of its line: flow style puts closing braces, commas and sibling +// keys after the value on the same line, and swallowing them would corrupt +// the stream. +func isFlowStyle(nodes ...*yaml.Node) bool { + for _, n := range nodes { + if n != nil && n.Style&yaml.FlowStyle != 0 { + return true + } + } + return false } diff --git a/pkg/compose/transform/replace_test.go b/pkg/compose/transform/replace_test.go index 6f7477e0350..68b3b1184e1 100644 --- a/pkg/compose/transform/replace_test.go +++ b/pkg/compose/transform/replace_test.go @@ -83,3 +83,34 @@ func TestReplace(t *testing.T) { }) } } + +// A flow-style document puts every node on line 1: the position scan in +// replace never sees a newline preceding the target, pos runs to the end of +// the stream, and the column offset then slices out of range. compose +// publish feeds the user's original file bytes into this path +// (processFile), so the flow style is reachable input, not a hypothetical. +func TestReplaceFlowStyle(t *testing.T) { + t.Run("extends file", func(t *testing.T) { + _, err := ReplaceExtendsFile([]byte(`{services: {test: {extends: {file: old.yaml}}}}`), "test", "REPLACED") + assert.ErrorContains(t, err, "flow-style") + }) + t.Run("env_file", func(t *testing.T) { + _, err := ReplaceEnvFile([]byte(`{services: {test: {env_file: old.env}}}`), "test", 0, "REPLACED") + assert.ErrorContains(t, err, "flow style") + }) + // on main this silently corrupted the stream: rewriting an entry of a + // flow sequence swallowed the rest of the line, closing bracket included + t.Run("env_file flow sequence", func(t *testing.T) { + _, err := ReplaceEnvFile([]byte(`{services: {test: {env_file: [a.env, b.env]}}}`), "test", 1, "REPLACED") + assert.ErrorContains(t, err, "flow style") + }) + t.Run("env_file out-of-range entry", func(t *testing.T) { + _, err := ReplaceEnvFile([]byte(`services: + test: + env_file: + - a.env + - b.env +`), "test", 5, "REPLACED") + assert.ErrorContains(t, err, "cannot rewrite entry 5") + }) +}