diff --git a/README.md b/README.md index b1208ec..01da578 100644 --- a/README.md +++ b/README.md @@ -155,7 +155,7 @@ runs without configuration; the rest are `off` until you enable them: | --- | --- | --- | | [`correctness`](https://bare-devcontainer.github.io/decolint/rules/#correctness) | `error` | 13 | | [`security`](https://bare-devcontainer.github.io/decolint/rules/#security) | `off` | 11 | -| [`reproducibility`](https://bare-devcontainer.github.io/decolint/rules/#reproducibility) | `off` | 5 | +| [`reproducibility`](https://bare-devcontainer.github.io/decolint/rules/#reproducibility) | `off` | 6 | | [`style`](https://bare-devcontainer.github.io/decolint/rules/#style) | `off` | 2 | diff --git a/containerdef/containerdef.go b/containerdef/containerdef.go index d315b8b..d7879e6 100644 --- a/containerdef/containerdef.go +++ b/containerdef/containerdef.go @@ -3,8 +3,8 @@ // and nothing else — resolving it is the caller's, whether that is the merge fetching what the // declaration names or a lint rule reading it from the linted directory. // -// Each declaration carries the byte offset of the property name declaring it, so a caller can -// anchor what it reports at where the declaration is written. +// Each declaration carries the byte offsets of the property name declaring it and of its value, so +// a caller can anchor what it reports at whichever the reader of its output expects to see. package containerdef import ( @@ -25,6 +25,8 @@ type ImageDef struct { Ref string // KeyOffset is the byte offset of the "image" property name. KeyOffset int + // ValueOffset is the byte offset of the reference. + ValueOffset int } func (*ImageDef) containerDef() {} @@ -62,7 +64,7 @@ func image(obj *hujson.Object) *ImageDef { if !isLit || lit.Kind() != '"' { return nil } - return &ImageDef{Ref: lit.String(), KeyOffset: m.Name.StartOffset} + return &ImageDef{Ref: lit.String(), KeyOffset: m.Name.StartOffset, ValueOffset: m.Value.StartOffset} } // BuildDef is the Dockerfile build a devcontainer.json declares: the Dockerfile it builds and @@ -74,6 +76,8 @@ type BuildDef struct { // DockerfileKeyOffset is the byte offset of the property naming the Dockerfile, whichever of the // two names it. DockerfileKeyOffset int + // DockerfileValueOffset is the byte offset of that property's value. + DockerfileValueOffset int // Args are the arguments passed to the build, nil when it declares none. Args map[string]string // Target is the stage the build stops at, empty when it declares none. @@ -89,11 +93,11 @@ type BuildDef struct { // config.build.dockerfile). The options are always the "build" object's, which the legacy top-level // form carries alongside it. func build(obj *hujson.Object) *BuildDef { - path, keyOffset, ok := dockerfilePath(obj) + path, keyOffset, valueOffset, ok := dockerfilePath(obj) if !ok { return nil } - config := &BuildDef{Dockerfile: path, DockerfileKeyOffset: keyOffset} + config := &BuildDef{Dockerfile: path, DockerfileKeyOffset: keyOffset, DockerfileValueOffset: valueOffset} build := buildObject(obj) if build == nil { return config @@ -123,20 +127,20 @@ func build(obj *hujson.Object) *BuildDef { // dockerfilePath returns the path the configuration names its Dockerfile by, in either form, with // the byte offset of the naming property. -func dockerfilePath(obj *hujson.Object) (string, int, bool) { +func dockerfilePath(obj *hujson.Object) (path string, keyOffset, valueOffset int, ok bool) { if m := memberNamed(obj, "dockerFile"); m != nil { if lit, isLit := m.Value.Value.(hujson.Literal); isLit && lit.Kind() == '"' { - return lit.String(), m.Name.StartOffset, true + return lit.String(), m.Name.StartOffset, m.Value.StartOffset, true } } if build := buildObject(obj); build != nil { if m := memberNamed(build, "dockerfile"); m != nil { if lit, isLit := m.Value.Value.(hujson.Literal); isLit && lit.Kind() == '"' { - return lit.String(), m.Name.StartOffset, true + return lit.String(), m.Name.StartOffset, m.Value.StartOffset, true } } } - return "", 0, false + return "", 0, 0, false } // ComposeDef is the Compose declaration of a devcontainer.json: the files it names and the @@ -149,6 +153,8 @@ type ComposeDef struct { Files []string // FilesKeyOffset is the byte offset of the "dockerComposeFile" property name. FilesKeyOffset int + // FilesValueOffset is the byte offset of that property's value. + FilesValueOffset int // Service is the service the dev container runs in, empty when the configuration names none or // names it as something other than a string. Service string @@ -186,7 +192,7 @@ func compose(obj *hujson.Object) *ComposeDef { } } } - config.FilesKeyOffset = m.Name.StartOffset + config.FilesKeyOffset, config.FilesValueOffset = m.Name.StartOffset, m.Value.StartOffset if m := memberNamed(obj, "service"); m != nil { if lit, isLit := m.Value.Value.(hujson.Literal); isLit && lit.Kind() == '"' { diff --git a/containerdef/containerdef_test.go b/containerdef/containerdef_test.go index 2a4d7c6..cb9b825 100644 --- a/containerdef/containerdef_test.go +++ b/containerdef/containerdef_test.go @@ -224,9 +224,9 @@ func TestDefs_Compose(t *testing.T) { } } -// TestDefs_KeyOffsets checks that each form is located at the property declaring it, which is where -// a caller anchors what it reports. -func TestDefs_KeyOffsets(t *testing.T) { +// TestDefs_Offsets checks that each form is located at the property declaring it and at that +// property's value, which is where a caller anchors what it reports. +func TestDefs_Offsets(t *testing.T) { t.Parallel() // One configuration declaring every form, so each offset is read from the same source. @@ -239,6 +239,9 @@ func TestDefs_KeyOffsets(t *testing.T) { if want := strings.Index(src, `"dockerComposeFile"`); compose.FilesKeyOffset != want { t.Errorf("FilesKeyOffset = %d, want %d", compose.FilesKeyOffset, want) } + if want := strings.Index(src, `"c.yml"`); compose.FilesValueOffset != want { + t.Errorf("FilesValueOffset = %d, want %d", compose.FilesValueOffset, want) + } build := defOf[*containerdef.BuildDef](t, src) if build == nil { @@ -247,6 +250,9 @@ func TestDefs_KeyOffsets(t *testing.T) { if want := strings.Index(src, `"dockerfile"`); build.DockerfileKeyOffset != want { t.Errorf("DockerfileKeyOffset = %d, want %d", build.DockerfileKeyOffset, want) } + if want := strings.Index(src, `"Dockerfile"`); build.DockerfileValueOffset != want { + t.Errorf("DockerfileValueOffset = %d, want %d", build.DockerfileValueOffset, want) + } image := defOf[*containerdef.ImageDef](t, src) if image == nil { @@ -255,14 +261,17 @@ func TestDefs_KeyOffsets(t *testing.T) { if want := strings.Index(src, `"image"`); image.KeyOffset != want { t.Errorf("KeyOffset = %d, want %d", image.KeyOffset, want) } + if want := strings.Index(src, `"ubuntu:24.04"`); image.ValueOffset != want { + t.Errorf("ValueOffset = %d, want %d", image.ValueOffset, want) + } } -// TestDefs_LegacyDockerfileKeyOffset checks that the legacy form anchors at its own property, not at +// TestDefs_LegacyDockerfileOffsets checks that the legacy form anchors at its own property, not at // the "build" object the options come from. -func TestDefs_LegacyDockerfileKeyOffset(t *testing.T) { +func TestDefs_LegacyDockerfileOffsets(t *testing.T) { t.Parallel() - const src = `{"dockerFile": "Dockerfile", "build": {"target": "dev"}}` + const src = `{"dockerFile": "top", "build": {"dockerfile": "nested"}}` got := defOf[*containerdef.BuildDef](t, src) if got == nil { @@ -271,6 +280,9 @@ func TestDefs_LegacyDockerfileKeyOffset(t *testing.T) { if want := strings.Index(src, `"dockerFile"`); got.DockerfileKeyOffset != want { t.Errorf("DockerfileKeyOffset = %d, want %d", got.DockerfileKeyOffset, want) } + if want := strings.Index(src, `"top"`); got.DockerfileValueOffset != want { + t.Errorf("DockerfileValueOffset = %d, want %d", got.DockerfileValueOffset, want) + } } // TestDefs_Order checks the order the forms come in, which is the order the reference implementation diff --git a/feature/metadata.go b/feature/metadata.go index ee53d91..0337d6a 100644 --- a/feature/metadata.go +++ b/feature/metadata.go @@ -11,7 +11,7 @@ import ( type Metadata struct { // ID is the Feature's declared identifier, or "" when it declares none (as image-metadata // entries do; see [contributor.hasID]). - ID string + ID string Version string // DependsOn lists the Features this Feature depends on, in declaration order. Dependencies are // installed before the Feature and contribute properties of their own. diff --git a/go.mod b/go.mod index 9a25c8c..a792b39 100644 --- a/go.mod +++ b/go.mod @@ -11,6 +11,7 @@ require ( github.com/opencontainers/image-spec v1.1.1 github.com/spf13/pflag v1.0.10 github.com/tailscale/hujson v0.0.0-20260727124030-b80ff77dac4f + go.yaml.in/yaml/v3 v3.0.4 golang.org/x/sys v0.47.0 golang.org/x/term v0.45.0 oras.land/oras-go/v2 v2.6.2 @@ -200,7 +201,6 @@ require ( go.opentelemetry.io/otel/sdk/metric v1.44.0 // indirect go.opentelemetry.io/otel/trace v1.44.0 // indirect go.uber.org/automaxprocs v1.5.3 // indirect - go.yaml.in/yaml/v3 v3.0.4 // indirect go.yaml.in/yaml/v4 v4.0.0-rc.4 // indirect gocloud.dev v0.45.0 // indirect golang.org/x/crypto v0.54.0 // indirect diff --git a/rules/compose.go b/rules/compose.go new file mode 100644 index 0000000..1f5c58c --- /dev/null +++ b/rules/compose.go @@ -0,0 +1,183 @@ +package rules + +import ( + "path" + "strings" + + "github.com/bare-devcontainer/decolint/linter" + "go.yaml.in/yaml/v3" +) + +// composeSource is what the Compose service a dev container runs is made from: the image it pulls, +// or the build that produces one. At most one is set; neither is when the service names an image +// this cannot resolve (see [composeServiceSource]). +type composeSource struct { + image string + build *composeBuild +} + +// composeBuild is a Compose service's "build", reduced to what a rule reading its Dockerfile needs. +// Exactly one of dockerfile and inline is set. +type composeBuild struct { + // dockerfile is the Dockerfile's path, relative to the directory being linted. + dockerfile string + // inline is the Dockerfile's content, for a build that gives it as "dockerfile_inline". + inline string + // target is the stage "target" names, empty when it names none. + target string + // args are the build arguments, which a FROM of the Dockerfile is expanded against. + args map[string]string +} + +// composeService is the part of a Compose service definition that says what the service runs, or +// that the definition is not all in this file. +type composeService struct { + Image string `yaml:"image"` + // Build is untyped because Compose writes it two ways: the build context as a string, or an + // object of build options. See [composeServiceBuild]. + Build any `yaml:"build"` + Extends any `yaml:"extends"` +} + +// composeDoc is the part of a Compose file that defines the services, or pulls definitions in from +// files of its own. +type composeDoc struct { + Services map[string]composeService `yaml:"services"` + Include any `yaml:"include"` +} + +// composeServiceSource returns what the named Compose service is made from, reading the files at +// paths in the order they are declared, each later one overriding the earlier ones as Compose merges +// them. +// +// This reads the declared files and nothing else, which is narrower than the resolution the merge +// performs through compose-go (see feature's loadComposeService: it applies "extends" and "include" +// and interpolates variables, reading files outside the linted directory and an environment a rule +// does not have). ok is therefore false for everything this cannot settle from the files +// themselves, so that what it does report is what the full resolution would report too: +// +// - a file that cannot be read (see [readConfigFile]) or does not parse; +// - a file declaring "include", or a service declaring "extends", either of which can define or +// override the service from a file not named here; +// - a service none of the files defines; +// - a service more than one file gives a "build", which Compose merges option by option. +// +// A service whose image or build context is written with a variable resolves to neither an image nor +// a build: the value comes from the environment. The same is true of a build context naming a remote +// repository, which is no path in the linted directory. +func composeServiceSource(dir linter.Dir, paths []string, service string) (composeSource, bool) { + var src composeSource + var found, built bool + for _, p := range paths { + data, ok := readConfigFile(dir, p) + if !ok { + return composeSource{}, false + } + var doc composeDoc + if err := yaml.Unmarshal(data, &doc); err != nil || doc.Include != nil { + return composeSource{}, false + } + svc, ok := doc.Services[service] + if !ok { + continue + } + found = true + if svc.Extends != nil { + return composeSource{}, false + } + if svc.Image != "" { + src.image = svc.Image + } + if svc.Build == nil { + continue + } + if built { + return composeSource{}, false + } + built = true + src.build = composeServiceBuild(svc.Build, path.Dir(p)) + } + if !found { + return composeSource{}, false + } + if src.build != nil { + // The "image" of a service that builds names what the build produces, not what it starts + // from, so the build is the whole answer. + return composeSource{build: src.build}, true + } + // Both "${VAR}" and the bare "$VAR" Compose accepts leave the image unresolved here. + if strings.Contains(src.image, "$") { + src.image = "" + } + return src, true +} + +// composeServiceBuild reads a service's "build" in either of the forms Compose writes it, resolving +// the Dockerfile against baseDir, the directory of the Compose file declaring the build, as Compose +// resolves it against the file it is written in. It returns nil for a build whose Dockerfile is not +// a path in the linted directory. +// +// The Dockerfile defaults to "Dockerfile" in the build context, and the context to the Compose +// file's own directory. +func composeServiceBuild(value any, baseDir string) *composeBuild { + var context, dockerfile, inline, target string + var args map[string]string + switch v := value.(type) { + case string: + // The short form is the build context alone. + context = v + case map[string]any: + context, _ = v["context"].(string) + dockerfile, _ = v["dockerfile"].(string) + inline, _ = v["dockerfile_inline"].(string) + target, _ = v["target"].(string) + args = composeBuildArgs(v["args"]) + default: + return nil + } + + if inline != "" { + return &composeBuild{inline: inline, target: target, args: args} + } + // A context naming a remote repository, or one written as a variable, is no path the Dockerfile + // can be read through. + if strings.Contains(context, "://") || strings.Contains(context, "$") { + return nil + } + if dockerfile == "" { + dockerfile = "Dockerfile" + } + if strings.Contains(dockerfile, "$") { + return nil + } + return &composeBuild{dockerfile: path.Join(baseDir, context, dockerfile), target: target, args: args} +} + +// composeBuildArgs reads a build's "args", which Compose writes as a mapping of names to values or +// as a list of "NAME=value" entries. An entry with no value takes it from the environment, which is +// not the configuration's to give, and is left out along with any value that is not a string. +func composeBuildArgs(value any) map[string]string { + args := map[string]string{} + switch v := value.(type) { + case map[string]any: + for name, raw := range v { + if s, ok := raw.(string); ok { + args[name] = s + } + } + case []any: + for _, raw := range v { + entry, ok := raw.(string) + if !ok { + continue + } + if name, val, found := strings.Cut(entry, "="); found { + args[name] = val + } + } + } + if len(args) == 0 { + return nil + } + return args +} diff --git a/rules/compose_test.go b/rules/compose_test.go new file mode 100644 index 0000000..15551a3 --- /dev/null +++ b/rules/compose_test.go @@ -0,0 +1,211 @@ +package rules + +import ( + "testing" + "testing/fstest" + + "github.com/bare-devcontainer/decolint/linter" + "github.com/google/go-cmp/cmp" +) + +// TestComposeServiceSource covers what a service is read as — an image, a build, or neither — since +// which one it is decides whether the Compose rule or the Dockerfile rules report on it. +func TestComposeServiceSource(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + files map[string]string + paths []string + wantOK bool + wantImage string + wantBuild *composeBuild + }{ + { + name: "an image", + files: map[string]string{"docker-compose.yml": "services:\n app:\n image: ubuntu:24.04\n"}, + paths: []string{"docker-compose.yml"}, + wantOK: true, + wantImage: "ubuntu:24.04", + }, + { + name: "a build in the long form", + files: map[string]string{"docker-compose.yml": "services:\n app:\n build:\n context: .\n dockerfile: Dockerfile\n"}, + paths: []string{"docker-compose.yml"}, + wantOK: true, + wantBuild: &composeBuild{dockerfile: "Dockerfile"}, + }, + { + name: "a build in the short form defaults the Dockerfile", + files: map[string]string{"docker-compose.yml": "services:\n app:\n build: .\n"}, + paths: []string{"docker-compose.yml"}, + wantOK: true, + wantBuild: &composeBuild{dockerfile: "Dockerfile"}, + }, + { + name: "a build resolves against the Compose file's own directory", + files: map[string]string{"compose/docker-compose.yml": "services:\n app:\n build:\n context: ..\n dockerfile: build/Dockerfile\n"}, + paths: []string{"compose/docker-compose.yml"}, + wantOK: true, + wantBuild: &composeBuild{dockerfile: "build/Dockerfile"}, + }, + { + name: "a build carries its args", + files: map[string]string{"docker-compose.yml": "services:\n app:\n build:\n context: .\n args:\n VARIANT: \"24.04\"\n"}, + paths: []string{"docker-compose.yml"}, + wantOK: true, + wantBuild: &composeBuild{dockerfile: "Dockerfile", args: map[string]string{"VARIANT": "24.04"}}, + }, + { + // Compose accepts a list of "NAME=value" entries as readily as a mapping. + name: "a build carries its args written as a list", + files: map[string]string{"docker-compose.yml": "services:\n app:\n build:\n context: .\n args:\n - VARIANT=24.04\n - FROM_ENV\n"}, + paths: []string{"docker-compose.yml"}, + wantOK: true, + wantBuild: &composeBuild{dockerfile: "Dockerfile", args: map[string]string{"VARIANT": "24.04"}}, + }, + { + name: "a build arg that is not a string is left out", + files: map[string]string{"docker-compose.yml": "services:\n app:\n build:\n context: .\n args:\n - 42\n"}, + paths: []string{"docker-compose.yml"}, + wantOK: true, + wantBuild: &composeBuild{dockerfile: "Dockerfile"}, + }, + { + name: "a build carries its target", + files: map[string]string{"docker-compose.yml": "services:\n app:\n build:\n context: .\n target: dev\n"}, + paths: []string{"docker-compose.yml"}, + wantOK: true, + wantBuild: &composeBuild{dockerfile: "Dockerfile", target: "dev"}, + }, + { + name: "an inline Dockerfile is its own content", + files: map[string]string{"docker-compose.yml": "services:\n app:\n build:\n dockerfile_inline: |\n FROM ubuntu:latest\n"}, + paths: []string{"docker-compose.yml"}, + wantOK: true, + wantBuild: &composeBuild{inline: "FROM ubuntu:latest\n"}, + }, + { + name: "a build overrides an image", + files: map[string]string{"docker-compose.yml": "services:\n app:\n image: built:latest\n build: .\n"}, + paths: []string{"docker-compose.yml"}, + wantOK: true, + wantBuild: &composeBuild{dockerfile: "Dockerfile"}, + }, + { + name: "a later file overriding the image wins", + files: map[string]string{ + "a.yml": "services:\n app:\n image: ubuntu:latest\n", + "b.yml": "services:\n app:\n image: ubuntu:24.04\n", + }, + paths: []string{"a.yml", "b.yml"}, + wantOK: true, + wantImage: "ubuntu:24.04", + }, + { + // Compose merges a build option by option across files, which this does not model. + name: "a build declared by two files is not resolved", + files: map[string]string{ + "a.yml": "services:\n app:\n build: .\n", + "b.yml": "services:\n app:\n build:\n target: dev\n", + }, + paths: []string{"a.yml", "b.yml"}, + wantOK: false, + }, + { + name: "a context naming a repository is no path", + files: map[string]string{"docker-compose.yml": "services:\n app:\n build: https://example.invalid/repo.git\n"}, + paths: []string{"docker-compose.yml"}, + wantOK: true, + wantBuild: nil, + }, + { + name: "a context written as a variable is not resolved", + files: map[string]string{"docker-compose.yml": "services:\n app:\n build: ${CONTEXT}\n"}, + paths: []string{"docker-compose.yml"}, + wantOK: true, + wantBuild: nil, + }, + { + name: "a Dockerfile written as a variable is not resolved", + files: map[string]string{"docker-compose.yml": "services:\n app:\n build:\n context: .\n dockerfile: ${DOCKERFILE}\n"}, + paths: []string{"docker-compose.yml"}, + wantOK: true, + wantBuild: nil, + }, + { + // Compose writes a build as its context or as an object of options, and as neither of + // those a build says nothing about a Dockerfile. + name: "a build that is neither form is not resolved", + files: map[string]string{"docker-compose.yml": "services:\n app:\n build:\n - .\n"}, + paths: []string{"docker-compose.yml"}, + wantOK: true, + wantBuild: nil, + }, + { + name: "an image written as a variable is not resolved", + files: map[string]string{"docker-compose.yml": "services:\n app:\n image: ubuntu:$TAG\n"}, + paths: []string{"docker-compose.yml"}, + wantOK: true, + wantImage: "", + }, + { + name: "a service none of the files defines", + files: map[string]string{"docker-compose.yml": "services:\n web:\n image: ubuntu:24.04\n"}, + paths: []string{"docker-compose.yml"}, + wantOK: false, + }, + { + name: "a service extending another", + files: map[string]string{"docker-compose.yml": "services:\n app:\n extends:\n service: base\n"}, + paths: []string{"docker-compose.yml"}, + wantOK: false, + }, + { + name: "a file pulling in others", + files: map[string]string{"docker-compose.yml": "include:\n - other.yml\nservices:\n app:\n image: ubuntu:24.04\n"}, + paths: []string{"docker-compose.yml"}, + wantOK: false, + }, + { + name: "a file that does not parse", + files: map[string]string{"docker-compose.yml": "services:\n app:\n image: [\n"}, + paths: []string{"docker-compose.yml"}, + wantOK: false, + }, + { + name: "a missing file", + files: map[string]string{}, + paths: []string{"docker-compose.yml"}, + wantOK: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + fsys := fstest.MapFS{} + for name, content := range tt.files { + fsys[name] = &fstest.MapFile{Data: []byte(content)} + } + got, ok := composeServiceSource(linter.Dir{FS: fsys}, tt.paths, "app") + if ok != tt.wantOK { + t.Fatalf("composeServiceSource ok = %v, want %v", ok, tt.wantOK) + } + if !ok { + return + } + if got.image != tt.wantImage { + t.Errorf("image = %q, want %q", got.image, tt.wantImage) + } + switch { + case tt.wantBuild == nil && got.build != nil: + t.Errorf("build = %+v, want none", *got.build) + case tt.wantBuild != nil && got.build == nil: + t.Errorf("build = none, want %+v", *tt.wantBuild) + case tt.wantBuild != nil && !cmp.Equal(*got.build, *tt.wantBuild, cmp.AllowUnexported(composeBuild{})): + t.Errorf("build = %+v, want %+v", *got.build, *tt.wantBuild) + } + }) + } +} diff --git a/rules/dockerfile.go b/rules/dockerfile.go new file mode 100644 index 0000000..57c90f0 --- /dev/null +++ b/rules/dockerfile.go @@ -0,0 +1,229 @@ +package rules + +import ( + "bytes" + "slices" + "strconv" + "strings" + + "github.com/moby/buildkit/frontend/dockerfile/instructions" + dflinter "github.com/moby/buildkit/frontend/dockerfile/linter" + "github.com/moby/buildkit/frontend/dockerfile/parser" + "github.com/moby/buildkit/frontend/dockerfile/shell" +) + +// dockerfilePulledImages returns the images a build of the Dockerfile in src pulls when target is +// built with args: the one each stage's FROM builds on, and the ones its COPY and RUN --mount +// instructions read through "--from". They come in the order the instructions name them, one entry +// per instruction. An empty target builds the last stage, as "docker build" does. +// +// A FROM is expanded against the Dockerfile's global ARGs, which args overrides, as BuildKit expands +// it (buildMetaArgs and buildDispatchStates in dockerfile2llb) — so "FROM ubuntu:${VARIANT}" names +// the image the declared VARIANT resolves to. A "--from" is not expanded, BuildKit rejecting a +// variable there outright. +// +// Only the stages the build actually reaches are considered, since a stage nothing depends on is +// never built and its images never pulled. Within them, a reference naming another stage is left +// out, being no image at all, as are the references [isPulledImage] rejects. +// +// It returns nothing for a Dockerfile that does not parse, or a target it does not define, leaving +// a rule with nothing to report rather than a guess. +func dockerfilePulledImages(src []byte, args map[string]string, target string) []string { + result, err := parser.Parse(bytes.NewReader(src)) + if err != nil { + return nil + } + // A Dockerfile may configure buildkit's own linter through a "# check=..." comment, which is + // merged onto the one passed here — a nil one is dereferenced, so pass a linter that reports + // nothing instead. Its zero Config leaves Warn nil, which is what turns the warnings off. + stages, metaArgs, err := instructions.Parse(result.AST, dflinter.New(&dflinter.Config{})) + if err != nil { + return nil + } + + lex := shell.NewLex(result.EscapeToken) + env := buildArgEnv(lex, metaArgs, args) + built := builtStages(stages, target) + var images []string + for i := range stages { + if !built[i] { + continue + } + if _, isStage := stageBase(stages, i); !isStage { + if base, ok := expand(lex, env, stages[i].BaseName); ok && isPulledImage(base) { + images = append(images, base) + } + } + for _, from := range stageFroms(stages, i) { + // A "--from" carrying a variable fails the build ("variable expansion is not supported + // for --from"), so it names no image to report on. + if from.stage < 0 && !strings.Contains(from.ref, "$") && isPulledImage(from.ref) { + images = append(images, from.ref) + } + } + } + return images +} + +// isPulledImage reports whether ref, a reference naming no stage, names an image the build pulls. +// Left out are the empty reference and "scratch", the empty base, which BuildKit recognizes in that +// spelling alone. +func isPulledImage(ref string) bool { + return ref != "" && ref != "scratch" +} + +// buildArgEnv returns the values a FROM is expanded against: the Dockerfile's global ARGs, each +// taking its value from args when that declares one and from its own default otherwise, with a +// default itself expanded against the ARGs before it. An arg args gives but the Dockerfile never +// declares is left out, as it is out of scope for a FROM. +func buildArgEnv(lex *shell.Lex, metaArgs []instructions.ArgCommand, args map[string]string) shell.EnvGetter { + var env []string + for _, cmd := range metaArgs { + for _, arg := range cmd.Args { + if value, ok := args[arg.Key]; ok { + env = append(env, arg.Key+"="+value) + continue + } + if arg.Value == nil { + continue + } + if value, ok := expand(lex, shell.EnvsFromSlice(env), *arg.Value); ok { + env = append(env, arg.Key+"="+value) + } + } + } + return shell.EnvsFromSlice(env) +} + +// expand resolves the variables in word against env. ok is false when a variable has no value there: +// BuildKit expands it to nothing, leaving a reference that names no image, so a rule has nothing to +// report on rather than a truncated reference to report wrongly. +func expand(lex *shell.Lex, env shell.EnvGetter, word string) (string, bool) { + result, err := lex.ProcessWordWithMatches(word, env) + if err != nil || len(result.Unmatched) > 0 { + return "", false + } + return result.Result, true +} + +// builtStages returns the indexes of the stages a build of target reaches: the target stage itself, +// the stages it builds on, and the ones it copies from, transitively. An empty target starts from +// the last stage, as "docker build" does. It returns nothing when target names no stage, since such +// a build does not run at all. +func builtStages(stages []instructions.Stage, target string) map[int]bool { + if len(stages) == 0 { + return nil + } + start := len(stages) - 1 + if target != "" { + // A target names a stage and never a position, and BuildKit lower-cases it before the + // lookup, so "DEV" reaches the stage declared "AS dev". + i, ok := stageNamed(stages, strings.ToLower(target)) + if !ok { + return nil + } + start = i + } + + built := map[int]bool{} + for queue := []int{start}; len(queue) > 0; queue = queue[1:] { + i := queue[0] + if built[i] { + continue + } + built[i] = true + queue = append(queue, stageDeps(stages, i)...) + } + return built +} + +// stageDeps returns the indexes of the stages the stage at i is built from: the one its FROM builds +// on, and the ones its instructions read through "--from", each only when it names a stage rather +// than an image. +func stageDeps(stages []instructions.Stage, i int) []int { + var deps []int + if j, ok := stageBase(stages, i); ok { + deps = append(deps, j) + } + for _, from := range stageFroms(stages, i) { + if from.stage >= 0 { + deps = append(deps, from.stage) + } + } + return deps +} + +// stageFrom is a "--from" value of a COPY or a RUN --mount, resolved against the Dockerfile's +// stages: stage is the index of the stage it names, or -1 for a value naming an image, which the +// build pulls like a FROM base. +type stageFrom struct { + ref string + stage int +} + +// stageFroms returns the "--from" values the instructions of the stage at i read, in the order they +// are written. A value naming neither a stage nor an image — a COPY's position that is out of range, +// which fails the build — is left out. +// +// The two instructions resolve a value differently: a COPY's is a stage position when it parses as +// an integer, while a RUN --mount's is always a name. Both are matched against the stage names +// case-insensitively, and against every stage rather than only the earlier ones, since BuildKit +// resolves them once the whole Dockerfile is read. +func stageFroms(stages []instructions.Stage, i int) []stageFrom { + byName := func(ref string) stageFrom { + if j, ok := stageNamed(stages, strings.ToLower(ref)); ok { + return stageFrom{ref: ref, stage: j} + } + return stageFrom{ref: ref, stage: -1} + } + + var froms []stageFrom + for _, cmd := range stages[i].Commands { + switch c := cmd.(type) { + case *instructions.CopyCommand: + if c.From == "" { + continue + } + if j, err := strconv.Atoi(c.From); err == nil { + if j >= 0 && j < len(stages) { + froms = append(froms, stageFrom{ref: c.From, stage: j}) + } + continue + } + froms = append(froms, byName(c.From)) + case *instructions.RunCommand: + for _, mount := range instructions.GetMounts(c) { + if mount.From == "" { + continue + } + froms = append(froms, byName(mount.From)) + } + } + } + return froms +} + +// stageBase returns the index of the stage the FROM of the stage at i builds on, and reports whether +// it names one rather than an image. BuildKit matches a base name against the stages declared before +// it only, and matches it as written against names the parser has already lower-cased — so +// "FROM Builder" after "AS builder" names an image, as its "repository name must be lowercase" +// failure shows. +func stageBase(stages []instructions.Stage, i int) (int, bool) { + return stageNamed(stages[:i], stages[i].BaseName) +} + +// stageNamed returns the index of the last stage named ref and reports whether one is. Several +// stages may share a name, which BuildKit only warns about, and it keeps one stage per name as it +// registers them in turn, so a reference reaches the last of them. A caller whose reference BuildKit +// lower-cases before the lookup passes it lower-cased; stage names need no folding, the parser +// having lower-cased them already. A name cannot begin with a digit, so no reference written as a +// position reaches a stage here. +func stageNamed(stages []instructions.Stage, ref string) (int, bool) { + for i, stage := range slices.Backward(stages) { + // A stage left unnamed has no name to be reached by, whatever ref is. + if stage.Name != "" && stage.Name == ref { + return i, true + } + } + return 0, false +} diff --git a/rules/images.go b/rules/images.go new file mode 100644 index 0000000..78d8692 --- /dev/null +++ b/rules/images.go @@ -0,0 +1,94 @@ +package rules + +import ( + "fmt" + + "github.com/bare-devcontainer/decolint/containerdef" + "github.com/bare-devcontainer/decolint/linter" + "github.com/tailscale/hujson" +) + +// pulledImage is an image a configuration pulls, wherever it is named. +type pulledImage struct { + // ref is the image reference as written. + ref string + // source locates the image for a finding that continues with the reference, e.g. + // `Dockerfile "Dockerfile": `. It is empty for the "image" property, which the reference alone + // already names. + source string + // offset is the byte offset of the property the finding anchors at, which is the one the + // devcontainer.json declares — the Dockerfile and the Compose file are not the linted file. + offset int +} + +// configImages returns every image a build of obj, a devcontainer.json, pulls: the one "image" +// names, the ones the Dockerfile it builds pulls, and, for a Compose-based configuration, the image +// its service runs or the ones the Dockerfile that service builds from pulls. +// +// Every form the configuration declares is read, not only the one the tooling would build, so that +// an unpinned image is reported wherever it is written; a configuration declaring more than one form +// is reported by [ConflictingContainerDef]. An image this cannot resolve is left out rather than +// guessed at; see [dockerfilePulledImages] and [composeServiceSource] for what each leaves behind. +func configImages(dir linter.Dir, obj *hujson.Object) []pulledImage { + var images []pulledImage + for def := range containerdef.Defs(obj) { + switch def := def.(type) { + case *containerdef.ImageDef: + images = append(images, pulledImage{ref: def.Ref, offset: def.ValueOffset}) + case *containerdef.BuildDef: + images = append(images, dockerfileImages(dir, def)...) + case *containerdef.ComposeDef: + images = append(images, composeImages(dir, def)...) + } + } + return images +} + +// dockerfileImages returns the images the Dockerfile def names pulls, anchored at the property +// naming it. +func dockerfileImages(dir linter.Dir, def *containerdef.BuildDef) []pulledImage { + src, ok := readConfigFile(dir, def.Dockerfile) + if !ok { + return nil + } + images := dockerfilePulledImages(src, def.Args, def.Target) + return locate(images, fmt.Sprintf("Dockerfile %q: ", def.Dockerfile), def.DockerfileValueOffset) +} + +// composeImages returns the images the Compose service the dev container runs pulls: the one it +// runs, or the ones the Dockerfile it builds from pulls. +func composeImages(dir linter.Dir, def *containerdef.ComposeDef) []pulledImage { + if !def.Usable() { + return nil + } + service, offset := def.Service, def.FilesValueOffset + source, ok := composeServiceSource(dir, def.Files, service) + if !ok { + return nil + } + if source.build == nil { + if source.image == "" { + return nil + } + return []pulledImage{{ref: source.image, source: fmt.Sprintf("compose service %q: ", service), offset: offset}} + } + + src := []byte(source.build.inline) + where := fmt.Sprintf("compose service %q inline Dockerfile: ", service) + if source.build.dockerfile != "" { + if src, ok = readConfigFile(dir, source.build.dockerfile); !ok { + return nil + } + where = fmt.Sprintf("Dockerfile %q: ", source.build.dockerfile) + } + return locate(dockerfilePulledImages(src, source.build.args, source.build.target), where, offset) +} + +// locate pairs each reference with where it was found and the offset to report it at. +func locate(refs []string, source string, offset int) []pulledImage { + images := make([]pulledImage, 0, len(refs)) + for _, ref := range refs { + images = append(images, pulledImage{ref: ref, source: source, offset: offset}) + } + return images +} diff --git a/rules/no_image_latest.go b/rules/no_image_latest.go index bf39586..9547c2a 100644 --- a/rules/no_image_latest.go +++ b/rules/no_image_latest.go @@ -7,22 +7,27 @@ import ( "github.com/tailscale/hujson" ) -// NoImageLatest reports the "image" property when it references a container image without an -// explicit tag or with the "latest" tag. Such references are not reproducible: the image they -// resolve to changes over time. +// NoImageLatest reports an image the configuration pulls without an explicit tag or with the +// "latest" tag, wherever it is named (see [configImages]). Such references are not reproducible: the +// image they resolve to changes over time. var NoImageLatest = &linter.Rule{ ID: "no-image-latest", Description: `disallow container images without an explicit tag or with the "latest" tag`, LongDescription: `A reference with no tag resolves to "latest", and "latest" is just the tag a publisher moves as they release. Either way the configuration says "whatever is current", so the same devcontainer.json builds a different environment next month, and a build that broke cannot be reproduced from the file alone. Name -the version the project was tested against.`, +the version the project was tested against. + +Every image a container of this configuration pulls is checked, whichever way the configuration names +it: the "image" property, the "FROM" and "COPY --from" of the Dockerfile it builds from, and, for a +Compose-based configuration, the image its service runs or the Dockerfile that service builds from.`, References: []string{ `https://containers.dev/implementors/json_reference/#image-specific`, + `https://containers.dev/implementors/spec/#dockerfile-based`, }, Category: linter.CategoryReproducibility, FileTypes: []linter.FileType{linter.Devcontainer}, - Paths: []string{"/image"}, + Paths: []string{""}, Example: linter.Example{ Bad: linter.Snippet{ Files: []linter.ExampleFile{ @@ -40,29 +45,34 @@ the version the project was tested against.`, `}, }, }, + Note: "A `FROM` written with a variable is resolved against the Dockerfile's `ARG` defaults\n" + + "and the `build.args` the configuration passes, as a build resolves it. One whose value\n" + + "neither declares is left unchecked, naming no image the configuration settles.", }, Check: checkNoImageLatest, } -func checkNoImageLatest(_ *linter.Context, node *linter.Node) []linter.Finding { - lit, ok := node.Value.Value.(hujson.Literal) - if !ok || lit.Kind() != '"' { +func checkNoImageLatest(ctx *linter.Context, node *linter.Node) []linter.Finding { + obj, ok := node.Value.Value.(*hujson.Object) + if !ok { return nil } - image := lit.String() - tag, hasTag := refTag(image) - switch { - case !hasTag: - return []linter.Finding{{ - Message: fmt.Sprintf("image %q has no explicit tag; pin a specific version", image), - Offset: node.Value.StartOffset, - }} - case tag == "latest": - return []linter.Finding{{ - Message: fmt.Sprintf("image %q uses the \"latest\" tag; pin a specific version", image), - Offset: node.Value.StartOffset, - }} + var findings []linter.Finding + for _, image := range configImages(ctx.Dir, obj) { + tag, hasTag := refTag(image.ref) + switch { + case !hasTag: + findings = append(findings, linter.Finding{ + Message: fmt.Sprintf("%simage %q has no explicit tag; pin a specific version", image.source, image.ref), + Offset: image.offset, + }) + case tag == "latest": + findings = append(findings, linter.Finding{ + Message: fmt.Sprintf("%simage %q uses the \"latest\" tag; pin a specific version", image.source, image.ref), + Offset: image.offset, + }) + } } - return nil + return findings } diff --git a/rules/no_image_latest_compose_test.go b/rules/no_image_latest_compose_test.go new file mode 100644 index 0000000..db51f75 --- /dev/null +++ b/rules/no_image_latest_compose_test.go @@ -0,0 +1,156 @@ +package rules_test + +import ( + "testing" + "testing/fstest" + + "github.com/bare-devcontainer/decolint/linter" + "github.com/bare-devcontainer/decolint/rules" +) + +func TestNoImageLatest_Compose(t *testing.T) { + t.Parallel() + + // Every case declares one Compose file, whose path starts at column 23, so the findings all + // anchor there. + const src = `{"dockerComposeFile": "docker-compose.yml", "service": "app"}` + issue := func(message string) []linter.Issue { + return []linter.Issue{{Path: "devcontainer.json", Line: 1, Col: 23, RuleID: "no-image-latest", Message: message}} + } + + tests := []struct { + name string + compose string + want []linter.Issue + }{ + { + "untagged image", + "services:\n app:\n image: ubuntu\n", + issue(`compose service "app": image "ubuntu" has no explicit tag; pin a specific version`), + }, + { + "latest image", + "services:\n app:\n image: ubuntu:latest\n", + issue(`compose service "app": image "ubuntu:latest" uses the "latest" tag; pin a specific version`), + }, + {"pinned tag", "services:\n app:\n image: ubuntu:24.04\n", nil}, + {"pinned digest", "services:\n app:\n image: ubuntu@sha256:abc123\n", nil}, + { + // Only the service the dev container runs in is the container's image. + "another service is not the dev container", + "services:\n app:\n image: ubuntu:24.04\n db:\n image: postgres:latest\n", + nil, + }, + { + // A service that builds names in "image" what the build produces, not what it starts + // from; the Dockerfile rules cover the base image. + "a service that builds its own image reports nothing", + "services:\n app:\n build: .\n image: myapp:latest\n", + nil, + }, + { + "an image written as a variable is not resolved", + "services:\n app:\n image: ubuntu:${TAG}\n", + nil, + }, + { + // Compose accepts the bare form as readily as "${VAR}". + "an image written as a bare variable is not resolved", + "services:\n app:\n image: $IMAGE\n", + nil, + }, + { + // The definition continues in a file this does not read, so what is here may not be the + // image the service ends up running. + "a service extending another reports nothing", + "services:\n app:\n extends:\n file: base.yml\n service: base\n image: ubuntu:latest\n", + nil, + }, + { + "a file pulling in others reports nothing", + "include:\n - other.yml\nservices:\n app:\n image: ubuntu:latest\n", + nil, + }, + {"a service defined in no file reports nothing", "services:\n web:\n image: ubuntu:latest\n", nil}, + {"a service without an image reports nothing", "services:\n app:\n command: sleep infinity\n", nil}, + {"a file that does not parse reports nothing", "services:\n app:\n image: [\n", nil}, + {"an empty file reports nothing", "", nil}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + dir := linter.Dir{FS: fstest.MapFS{"docker-compose.yml": {Data: []byte(tt.compose)}}} + assertIssuesInDir(t, rules.NoImageLatest, linter.SeverityError, "devcontainer.json", linter.Devcontainer, src, dir, tt.want) + }) + } +} + +func TestNoImageLatest_Compose_ComposeFileList(t *testing.T) { + t.Parallel() + + dir := linter.Dir{FS: fstest.MapFS{ + "docker-compose.yml": {Data: []byte("services:\n app:\n image: ubuntu:latest\n")}, + "docker-compose.override.yml": {Data: []byte("services:\n app:\n image: ubuntu:24.04\n")}, + "command.yml": {Data: []byte("services:\n app:\n command: sleep infinity\n")}, + }} + + tests := []struct { + name string + src string + want []linter.Issue + }{ + { + // Compose applies the files in order, so the last one to name an image wins. + "a later file overriding the image is the one read", + `{"dockerComposeFile": ["docker-compose.yml", "docker-compose.override.yml"], "service": "app"}`, + nil, + }, + { + "a later file leaving the image alone does not clear it", + `{"dockerComposeFile": ["docker-compose.yml", "command.yml"], "service": "app"}`, + []linter.Issue{{Path: "devcontainer.json", Line: 1, Col: 23, RuleID: "no-image-latest", Message: `compose service "app": image "ubuntu:latest" uses the "latest" tag; pin a specific version`}}, + }, + { + "an earlier file overridden by a later one is not reported", + `{"dockerComposeFile": ["docker-compose.override.yml", "docker-compose.yml"], "service": "app"}`, + []linter.Issue{{Path: "devcontainer.json", Line: 1, Col: 23, RuleID: "no-image-latest", Message: `compose service "app": image "ubuntu:latest" uses the "latest" tag; pin a specific version`}}, + }, + { + "no dockerComposeFile property", + `{"image": "ubuntu:latest", "service": "app"}`, + []linter.Issue{{Path: "devcontainer.json", Line: 1, Col: 11, RuleID: "no-image-latest", Message: `image "ubuntu:latest" uses the "latest" tag; pin a specific version`}}, + }, + {"no service property", `{"dockerComposeFile": "docker-compose.yml"}`, nil}, + {"an empty file list reports nothing", `{"dockerComposeFile": [], "service": "app"}`, nil}, + {"a non-string entry reports nothing", `{"dockerComposeFile": [42], "service": "app"}`, nil}, + {"a non-string dockerComposeFile reports nothing", `{"dockerComposeFile": 42, "service": "app"}`, nil}, + {"an object dockerComposeFile reports nothing", `{"dockerComposeFile": {}, "service": "app"}`, nil}, + {"a non-string service reports nothing", `{"dockerComposeFile": "docker-compose.yml", "service": 42}`, nil}, + {"a document that is not an object reports nothing", `["docker-compose.yml"]`, nil}, + {"a missing Compose file reports nothing", `{"dockerComposeFile": "absent.yml", "service": "app"}`, nil}, + { + // Configuration under .devcontainer is read through a root confined to it, so a Compose + // file above that directory is not decolint's to open. + "a path leading outside the directory reports nothing", + `{"dockerComposeFile": "../docker-compose.yml", "service": "app"}`, + nil, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assertIssuesInDir(t, rules.NoImageLatest, linter.SeverityError, "devcontainer.json", linter.Devcontainer, tt.src, dir, tt.want) + }) + } + + t.Run("unreadable directory reports nothing", func(t *testing.T) { + t.Parallel() + src := `{"dockerComposeFile": "docker-compose.yml", "service": "app"}` + assertIssuesInDir(t, rules.NoImageLatest, linter.SeverityError, "devcontainer.json", linter.Devcontainer, src, linter.Dir{FS: errFS{}}, nil) + }) + + t.Run("nil directory reports nothing", func(t *testing.T) { + t.Parallel() + assertIssues(t, rules.NoImageLatest, linter.SeverityError, `{"dockerComposeFile": "docker-compose.yml", "service": "app"}`, nil) + }) +} diff --git a/rules/no_image_latest_dockerfile_test.go b/rules/no_image_latest_dockerfile_test.go new file mode 100644 index 0000000..e812fe0 --- /dev/null +++ b/rules/no_image_latest_dockerfile_test.go @@ -0,0 +1,533 @@ +package rules_test + +import ( + "testing" + "testing/fstest" + + "github.com/bare-devcontainer/decolint/linter" + "github.com/bare-devcontainer/decolint/rules" +) + +func TestNoImageLatest_Dockerfile(t *testing.T) { + t.Parallel() + + // Every case declares the Dockerfile at "build.dockerfile", whose value starts at column 26, so + // the findings all anchor there. + const src = `{"build": {"dockerfile": "Dockerfile"}}` + issue := func(message string) []linter.Issue { + return []linter.Issue{{Path: "devcontainer.json", Line: 1, Col: 26, RuleID: "no-image-latest", Message: message}} + } + + tests := []struct { + name string + dockerfile string + want []linter.Issue + }{ + { + "untagged base image", + "FROM ubuntu\n", + issue(`Dockerfile "Dockerfile": image "ubuntu" has no explicit tag; pin a specific version`), + }, + { + "latest base image", + "FROM ubuntu:latest\n", + issue(`Dockerfile "Dockerfile": image "ubuntu:latest" uses the "latest" tag; pin a specific version`), + }, + {"pinned tag", "FROM ubuntu:24.04\n", nil}, + {"pinned digest", "FROM ubuntu@sha256:abc123\n", nil}, + {"scratch is not an image", "FROM scratch\nCOPY app /app\n", nil}, + { + "a later stage building on an earlier one is not an image", + "FROM golang:1.24 AS builder\nRUN go build\n\nFROM builder AS final\n", + nil, + }, + { + // A base name reaching the last stage declared under it leaves the first one unbuilt. + "a base name reaches the last stage declared under it", + "FROM ubuntu:latest AS base\n\nFROM ubuntu:24.04 AS base\n\nFROM base AS final\n", + nil, + }, + { + // The parser lower-cases every stage name, so a "FROM" reaches one only in lower case. + "a stage name is reached in the case the parser gives it", + "FROM golang:1.24 AS Builder\n\nFROM builder\n", + nil, + }, + { + // BuildKit reads a base name it cannot match as an image, which is why "FROM BUILDER" + // fails with "repository name must be lowercase" rather than building on the stage. + "a base name in another case is an image", + "FROM golang:1.24 AS builder\n\nFROM BUILDER\n", + issue(`Dockerfile "Dockerfile": image "BUILDER" has no explicit tag; pin a specific version`), + }, + { + // A stage name cannot begin with a digit, so a "FROM" naming a position names an image; + // the stage at that position is not built and its own base never pulled. + "a base name written as a position is an image", + "FROM golang:latest\n\nFROM 0\n", + issue(`Dockerfile "Dockerfile": image "0" has no explicit tag; pin a specific version`), + }, + { + // BuildKit expands a FROM against the global ARGs, so the image the default names is the + // one the build pulls. + "an ARG default resolves the image", + "ARG VARIANT=latest\nFROM ubuntu:${VARIANT}\n", + issue(`Dockerfile "Dockerfile": image "ubuntu:latest" uses the "latest" tag; pin a specific version`), + }, + { + "a resolved image that is pinned reports nothing", + "ARG VARIANT=24.04\nFROM ubuntu:${VARIANT}\n", + nil, + }, + { + // Nothing declares the variable, so BuildKit expands it to nothing and the reference + // names no image. + "an undeclared variable leaves no image", + "FROM ubuntu:${VARIANT}\n", + nil, + }, + { + // An ARG with no default takes its value from the build, which passes none here. + "an ARG left without a value leaves no image", + "ARG VARIANT\nFROM ubuntu:${VARIANT}\n", + nil, + }, + { + // The first ARG's default cannot be resolved, which leaves the ARGs after it alone. + "an ARG default of its own variable does not settle the ones after it", + "ARG BASE=${UNDECLARED}\nARG VARIANT=latest\nFROM ubuntu:${VARIANT}\n", + issue(`Dockerfile "Dockerfile": image "ubuntu:latest" uses the "latest" tag; pin a specific version`), + }, + { + // A variable in a "--from" fails the build outright. + "a variable in a copy reports nothing", + "FROM ubuntu:24.04\nCOPY --from=$TOOLS /x /x\n", + nil, + }, + { + "each unpinned stage is reported", + "FROM golang:latest AS builder\nRUN go build\n\nFROM ubuntu\nCOPY --from=builder /app /app\n", + []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 26, RuleID: "no-image-latest", Message: `Dockerfile "Dockerfile": image "golang:latest" uses the "latest" tag; pin a specific version`}, + {Path: "devcontainer.json", Line: 1, Col: 26, RuleID: "no-image-latest", Message: `Dockerfile "Dockerfile": image "ubuntu" has no explicit tag; pin a specific version`}, + }, + }, + { + "the same unpinned image in several stages is reported once", + "FROM ubuntu:latest AS a\n\nFROM ubuntu:latest AS b\nCOPY --from=a /x /x\n", + issue(`Dockerfile "Dockerfile": image "ubuntu:latest" uses the "latest" tag; pin a specific version`), + }, + {"a Dockerfile whose instructions do not parse reports nothing", "FROM\n", nil}, + {"a Dockerfile that does not tokenize reports nothing", "FROM ubuntu\nRUN < maxConfigFileBytes { + return nil, false + } + return data, true +} + // holdsFeatureRefs reports whether pointer names the property that holds Feature references in a // file of the given type: "features" in a devcontainer.json, "dependsOn" in a Feature. A rule // declares its paths for every file type it applies to, so one covering both properties is offered diff --git a/rules/util_test.go b/rules/util_test.go index 8648f2e..2cc5d3e 100644 --- a/rules/util_test.go +++ b/rules/util_test.go @@ -2,7 +2,9 @@ package rules import ( "slices" + "strings" "testing" + "testing/fstest" "github.com/bare-devcontainer/decolint/feature" "github.com/bare-devcontainer/decolint/linter" @@ -98,3 +100,32 @@ func TestHoldsFeatureRefs(t *testing.T) { }) } } + +// TestReadConfigFile_SizeCap covers the boundary of the size cap: a file at it is read, and one over +// it is refused outright, so the rules reading a Dockerfile or a Compose file report nothing on it +// rather than on the part of it that fit. +func TestReadConfigFile_SizeCap(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + size int + want bool + }{ + {"at the cap", maxConfigFileBytes, true}, + {"over the cap", maxConfigFileBytes + 1, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + dir := linter.Dir{FS: fstest.MapFS{"Dockerfile": {Data: []byte(strings.Repeat("#", tt.size))}}} + src, ok := readConfigFile(dir, "Dockerfile") + if ok != tt.want { + t.Fatalf("readConfigFile of a %d-byte file: ok = %v, want %v", tt.size, ok, tt.want) + } + if ok && len(src) != tt.size { + t.Errorf("read %d bytes, want %d", len(src), tt.size) + } + }) + } +}