From 3c094e413b339fd9758d37f120bdf2d6b7142858 Mon Sep 17 00:00:00 2001 From: NopeCharon <115794254+NopeCharon@users.noreply.github.com> Date: Sun, 25 Oct 2026 01:14:57 +0530 Subject: [PATCH] fix: return errors instead of panicking on malformed layers parseLayers() validated the directives of each layer with unchecked type assertions on the raw YAML: the layer body and the 'from' value were asserted to be yaml.MapSlice and every directive key was asserted to be a string. A stacker file such as foo: from: docker://ubuntu:latest or a layer with an empty body or a non-string key therefore crashed stacker with "interface conversion" panics instead of reporting what was wrong with the file. Similarly, an empty string entry in 'imports' (which can result from a substitution with an empty default, e.g. "- ${{EXTRA:}}") made Layer.absolutify() slice the path with a negative index and panic. Check every assertion and return a descriptive error naming the layer, and reject empty import paths when parsing the import entry, the same way an import map without a 'path' is already rejected. Add unit tests covering each malformed input. Fixes #807 Signed-off-by: NopeCharon <115794254+NopeCharon@users.noreply.github.com> --- pkg/types/layer.go | 46 ++++++++++++++++++++++++------- pkg/types/layer_import_test.go | 3 ++ pkg/types/types_test.go | 50 ++++++++++++++++++++++++++++++++++ 3 files changed, 89 insertions(+), 10 deletions(-) diff --git a/pkg/types/layer.go b/pkg/types/layer.go index b8369faf..c54b5a4a 100644 --- a/pkg/types/layer.go +++ b/pkg/types/layer.go @@ -268,39 +268,62 @@ func parseLayers(referenceDirectory string, lms yaml.MapSlice, requireHash bool) // Let's make sure that all the things people supplied in the layers are // actually things this stacker understands. for _, e := range lms { - for _, directive := range e.Value.(yaml.MapSlice) { + directives, ok := e.Value.(yaml.MapSlice) + if !ok { + return nil, errors.Errorf("stackerfile: layer '%v' must be a map of directives, found: %#v", + e.Key, e.Value) + } + + for _, directive := range directives { + key, ok := directive.Key.(string) + if !ok { + return nil, errors.Errorf("stackerfile: layer '%v': directive %v is not a string", + e.Key, directive.Key) + } + found := false for _, field := range layerFields { - if directive.Key.(string) == field { + if key == field { found = true break } } if !found { - return nil, errors.Errorf("stackerfile: unknown directive %s", directive.Key.(string)) + return nil, errors.Errorf("stackerfile: unknown directive %s", key) } - if directive.Key.(string) == "from" { - for _, sourceDirective := range directive.Value.(yaml.MapSlice) { + if key == "from" { + sourceDirectives, ok := directive.Value.(yaml.MapSlice) + if !ok { + return nil, errors.Errorf("stackerfile: layer '%v': 'from' must be a map with a 'type' "+ + "and a 'url' or 'tag', found: %#v", e.Key, directive.Value) + } + + for _, sourceDirective := range sourceDirectives { + sourceKey, ok := sourceDirective.Key.(string) + if !ok { + return nil, errors.Errorf("stackerfile: layer '%v': image source directive %v "+ + "is not a string", e.Key, sourceDirective.Key) + } + found = false for _, field := range imageSourceFields { - if sourceDirective.Key.(string) == field { + if sourceKey == field { found = true break } } if !found { - return nil, errors.Errorf("stackerfile: unknown image source directive %s", - sourceDirective.Key.(string)) + return nil, errors.Errorf("stackerfile: unknown image source directive %s", sourceKey) } } } - if directive.Key.(string) == "os" || directive.Key.(string) == "arch" { + if key == "os" || key == "arch" { if directive.Value == nil { - return nil, errors.Errorf("stackerfile: %q value cannot be empty", directive.Key.(string)) + return nil, errors.Errorf("stackerfile: %q value cannot be empty", key) } } } @@ -456,6 +479,9 @@ func getImportFromInterface(v interface{}) (Import, error) { // if it is a simple string, that is the path s, ok := v.(string) if ok { + if s == "" { + return Import{}, errors.Errorf("import path cannot be empty") + } ret.Path = s return ret, nil } diff --git a/pkg/types/layer_import_test.go b/pkg/types/layer_import_test.go index e7b298cc..b8636bef 100644 --- a/pkg/types/layer_import_test.go +++ b/pkg/types/layer_import_test.go @@ -31,6 +31,9 @@ func TestGetImportFromInterface(t *testing.T) { {desc: "relative string", val: "path/to/file", expected: Import{Path: "path/to/file", Uid: eUGid, Gid: eUGid}}, + {desc: "string cannot be empty", + val: "", + errstr: "import path cannot be empty"}, {desc: "dict no dest", val: map[interface{}]interface{}{ "path": "/path/to/file", diff --git a/pkg/types/types_test.go b/pkg/types/types_test.go index c741150f..f0846b0e 100644 --- a/pkg/types/types_test.go +++ b/pkg/types/types_test.go @@ -2,7 +2,9 @@ package types import ( "os" + "path/filepath" "reflect" + "strings" "testing" ) @@ -51,6 +53,54 @@ func TestDockerFrom(t *testing.T) { } } +func TestMalformedLayers(t *testing.T) { + tables := []struct { + desc string + content string + errstr string + }{ + {desc: "layer is a string", + content: "foo: bar\n", + errstr: "must be a map of directives"}, + {desc: "layer is empty", + content: "foo:\n", + errstr: "must be a map of directives"}, + {desc: "directive is not a string", + content: "foo:\n 1: bar\n", + errstr: "directive 1 is not a string"}, + {desc: "from is a string", + content: "foo:\n from: docker://ubuntu:latest\n", + errstr: "'from' must be a map"}, + {desc: "from is empty", + content: "foo:\n from:\n", + errstr: "'from' must be a map"}, + {desc: "image source directive is not a string", + content: "foo:\n from:\n 1: docker\n", + errstr: "image source directive 1 is not a string"}, + {desc: "import path is empty", + content: "foo:\n from:\n type: scratch\n imports:\n - \"\"\n", + errstr: "import path cannot be empty"}, + } + + for _, tt := range tables { + t.Run(tt.desc, func(t *testing.T) { + path := filepath.Join(t.TempDir(), "stacker.yaml") + if err := os.WriteFile(path, []byte(tt.content), 0644); err != nil { + t.Fatalf("couldn't write stackerfile: %s", err) + } + + _, err := NewStackerfile(path, false, nil) + if err == nil { + t.Fatalf("expected an error parsing %q", tt.content) + } + + if !strings.Contains(err.Error(), tt.errstr) { + t.Fatalf("expected error containing %q, got %q", tt.errstr, err) + } + }) + } +} + func TestDependencyOrder(t *testing.T) { content := `first: from: