diff --git a/README.md b/README.md index 4e2f8ae..a378aec 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` | 4 | +| [`reproducibility`](https://bare-devcontainer.github.io/decolint/rules/#reproducibility) | `off` | 9 | | [`style`](https://bare-devcontainer.github.io/decolint/rules/#style) | `off` | 2 | diff --git a/go.mod b/go.mod index 91d6c2b..1475b0f 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-20260302212456-ecc657c15afd + 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.53.0 // indirect diff --git a/rules/dockerfile.go b/rules/dockerfile.go new file mode 100644 index 0000000..1259070 --- /dev/null +++ b/rules/dockerfile.go @@ -0,0 +1,262 @@ +package rules + +import ( + "bytes" + "slices" + "strconv" + "strings" + + "github.com/bare-devcontainer/decolint/linter" + "github.com/moby/buildkit/frontend/dockerfile/instructions" + dflinter "github.com/moby/buildkit/frontend/dockerfile/linter" + "github.com/moby/buildkit/frontend/dockerfile/parser" + "github.com/tailscale/hujson" +) + +// dockerfileRef locates the Dockerfile a devcontainer.json builds from: its path as written, and +// the byte offset of the value declaring it, which is where a rule reporting the Dockerfile's +// contents anchors its findings. +// +// The specification defines two mutually exclusive forms, the top-level "dockerFile" and the nested +// "build.dockerfile". The top-level one is preferred, as the reference implementation prefers it; +// the merge resolves the same two the same way, in feature's dockerfilePath. +func dockerfileRef(obj *hujson.Object) (path string, offset 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.Value.StartOffset, true + } + } + if m := memberNamed(obj, "build"); m != nil { + if build, isObj := m.Value.Value.(*hujson.Object); isObj { + if d := memberNamed(build, "dockerfile"); d != nil { + if lit, isLit := d.Value.Value.(hujson.Literal); isLit && lit.Kind() == '"' { + return lit.String(), d.Value.StartOffset, true + } + } + } + } + return "", 0, false +} + +// buildTarget returns the stage "build.target" names, or "" when the configuration names none and +// the build produces the Dockerfile's last stage. +func buildTarget(obj *hujson.Object) string { + m := memberNamed(obj, "build") + if m == nil { + return "" + } + build, ok := m.Value.Value.(*hujson.Object) + if !ok { + return "" + } + target, _ := stringMember(build, "target") + return target +} + +// dockerfileImage is an image a build of a Dockerfile pulls, and the instruction form that reaches +// it, which is all a rule needs to name it in a finding. +type dockerfileImage struct { + ref string + // base distinguishes the image a stage's FROM builds on from one a COPY or a RUN --mount pulls + // through "--from". + base bool +} + +// verb describes how the Dockerfile reaches the image, for a message that continues with the image: +// `Dockerfile "Dockerfile" builds from image "ubuntu"`. +func (img dockerfileImage) verb() string { + if img.base { + return "builds from" + } + return "pulls" +} + +// dockerfilePulledImages returns the images a build of the Dockerfile in src pulls when target is +// built: 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. +// +// 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, target string) []dockerfileImage { + 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, _, err := instructions.Parse(result.AST, dflinter.New(&dflinter.Config{})) + if err != nil { + return nil + } + + built := builtStages(stages, target) + var images []dockerfileImage + for i := range stages { + if !built[i] { + continue + } + if _, isStage := stageBase(stages, i); !isStage && isPulledImage(stages[i].BaseName) { + images = append(images, dockerfileImage{ref: stages[i].BaseName, base: true}) + } + for _, from := range stageFroms(stages, i) { + if from.stage < 0 && isPulledImage(from.ref) { + images = append(images, dockerfileImage{ref: 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; +// - "scratch", the empty base, which BuildKit recognizes in that spelling alone; +// - a reference containing a variable, whose value comes from "build.args" or an ARG default and +// is not the linter's to resolve. +func isPulledImage(ref string) bool { + return ref != "" && ref != "scratch" && !strings.Contains(ref, "$") +} + +// 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 +} + +// dockerfileBuildImages returns the images the build the Dockerfile that obj, a devcontainer.json, +// declares pulls (see [dockerfilePulledImages]), along with the Dockerfile's path as written and the +// offset to anchor findings at. ok is false when obj declares no Dockerfile, or when the file +// cannot be read (see [readConfigFile]). +func dockerfileBuildImages(dir linter.Dir, obj *hujson.Object) (images []dockerfileImage, path string, offset int, ok bool) { + path, offset, ok = dockerfileRef(obj) + if !ok { + return nil, "", 0, false + } + src, ok := readConfigFile(dir, path) + if !ok { + return nil, "", 0, false + } + return dockerfilePulledImages(src, buildTarget(obj)), path, offset, true +} diff --git a/rules/no_compose_image_latest.go b/rules/no_compose_image_latest.go new file mode 100644 index 0000000..7cfd25e --- /dev/null +++ b/rules/no_compose_image_latest.go @@ -0,0 +1,193 @@ +package rules + +import ( + "fmt" + "strings" + + "github.com/bare-devcontainer/decolint/linter" + "github.com/tailscale/hujson" + "go.yaml.in/yaml/v3" +) + +// NoComposeImageLatest reports the Compose service a devcontainer.json attaches to when it runs an +// image without an explicit tag or with the "latest" tag. It is [NoImageLatest] for the +// Compose-based form, where the container's image is named in a Compose file rather than in the +// "image" property. +var NoComposeImageLatest = &linter.Rule{ + ID: "no-compose-image-latest", + Description: `disallow a Compose service that runs an image without an explicit tag or with the "latest" tag`, + LongDescription: `The service named by "service" is the dev container: it is the one editors attach to and lifecycle +scripts run in. Its "image:" is therefore the environment the project works in, and an entry with no tag, +or with "latest", pulls whatever the publisher last released — a container that changes from one +"docker compose up" to the next while the repository stays the same.`, + References: []string{ + `https://containers.dev/implementors/spec/#docker-compose-based`, + `https://containers.dev/implementors/json_reference/#compose-specific`, + }, + Category: linter.CategoryReproducibility, + FileTypes: []linter.FileType{linter.Devcontainer}, + Paths: []string{""}, + Example: linter.Example{ + Bad: linter.Snippet{ + Files: []linter.ExampleFile{ + {Path: `devcontainer.json`, Content: `{ + "name": "api", + "dockerComposeFile": "docker-compose.yml", + "service": "app", + "workspaceFolder": "/workspace" +} +`}, + {Path: `docker-compose.yml`, Content: `services: + app: + image: mcr.microsoft.com/devcontainers/base:latest + command: sleep infinity +`}, + }, + }, + Good: linter.Snippet{ + Files: []linter.ExampleFile{ + {Path: `devcontainer.json`, Content: `{ + "name": "api", + "dockerComposeFile": "docker-compose.yml", + "service": "app", + "workspaceFolder": "/workspace" +} +`}, + {Path: `docker-compose.yml`, Content: `services: + app: + image: mcr.microsoft.com/devcontainers/base:ubuntu-24.04 + command: sleep infinity +`}, + }, + }, + Note: "Only the service the dev container runs in is checked, and only when it runs a\n" + + "published image: the base image of a service that builds its own image is not checked,\n" + + "and neither is an image written as a `${...}` variable, whose value is not in the\n" + + "configuration.", + }, + Check: checkNoComposeImageLatest, +} + +func checkNoComposeImageLatest(ctx *linter.Context, node *linter.Node) []linter.Finding { + obj, ok := node.Value.Value.(*hujson.Object) + if !ok { + return nil + } + paths, offset, ok := composeFilePaths(obj) + if !ok || len(paths) == 0 { + return nil + } + service, ok := stringMember(obj, "service") + if !ok { + return nil + } + image, ok := composeServiceImage(ctx.Dir, paths, service) + if !ok { + return nil + } + + tag, hasTag := refTag(image) + switch { + case !hasTag: + return []linter.Finding{{ + Message: fmt.Sprintf("compose service %q runs image %q, which has no explicit tag; pin a specific version", service, image), + Offset: offset, + }} + case tag == "latest": + return []linter.Finding{{ + Message: fmt.Sprintf("compose service %q runs image %q, which uses the \"latest\" tag; pin a specific version", service, image), + Offset: offset, + }} + } + return nil +} + +// composeFilePaths returns the Compose file paths obj declares, with the byte offset of the value +// declaring them. The property is a single path or an array of paths, later ones overriding earlier +// ones; the merge reads the same property in feature's composeFilePaths. +func composeFilePaths(obj *hujson.Object) (paths []string, offset int, ok bool) { + m := memberNamed(obj, "dockerComposeFile") + if m == nil { + return nil, 0, false + } + switch v := m.Value.Value.(type) { + case hujson.Literal: + if v.Kind() != '"' { + return nil, 0, false + } + paths = []string{v.String()} + case *hujson.Array: + for _, e := range v.Elements { + lit, isLit := e.Value.(hujson.Literal) + if !isLit || lit.Kind() != '"' { + return nil, 0, false + } + paths = append(paths, lit.String()) + } + default: + return nil, 0, false + } + return paths, m.Value.StartOffset, true +} + +// composeService is the part of a Compose service definition that says which image the service +// runs, or that the definition is not all in this file. +type composeService struct { + Image string `yaml:"image"` + 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"` +} + +// composeServiceImage returns the image the named Compose service runs, 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 that declares "build", whose "image" names what the build produces rather than what +// it starts from; +// - an image written with a variable, whose value comes from the environment. +func composeServiceImage(dir linter.Dir, paths []string, service string) (string, bool) { + var image string + var found bool + for _, p := range paths { + src, ok := readConfigFile(dir, p) + if !ok { + return "", false + } + var doc composeDoc + if err := yaml.Unmarshal(src, &doc); err != nil || doc.Include != nil { + return "", false + } + svc, ok := doc.Services[service] + if !ok { + continue + } + found = true + if svc.Build != nil || svc.Extends != nil { + return "", false + } + if svc.Image != "" { + image = svc.Image + } + } + // Both "${VAR}" and the bare "$VAR" Compose accepts leave the image unresolved here. + if !found || image == "" || strings.Contains(image, "$") { + return "", false + } + return image, true +} diff --git a/rules/no_compose_image_latest_test.go b/rules/no_compose_image_latest_test.go new file mode 100644 index 0000000..c202263 --- /dev/null +++ b/rules/no_compose_image_latest_test.go @@ -0,0 +1,152 @@ +package rules_test + +import ( + "testing" + "testing/fstest" + + "github.com/bare-devcontainer/decolint/linter" + "github.com/bare-devcontainer/decolint/rules" +) + +func TestNoComposeImageLatest(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-compose-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" runs image "ubuntu", which has no explicit tag; pin a specific version`), + }, + { + "latest image", + "services:\n app:\n image: ubuntu:latest\n", + issue(`compose service "app" runs image "ubuntu:latest", which 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.NoComposeImageLatest, linter.SeverityError, "devcontainer.json", linter.Devcontainer, src, dir, tt.want) + }) + } +} + +func TestNoComposeImageLatest_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-compose-image-latest", Message: `compose service "app" runs image "ubuntu:latest", which 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-compose-image-latest", Message: `compose service "app" runs image "ubuntu:latest", which uses the "latest" tag; pin a specific version`}}, + }, + {"no dockerComposeFile property", `{"image": "ubuntu:latest", "service": "app"}`, nil}, + {"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.NoComposeImageLatest, 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.NoComposeImageLatest, 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.NoComposeImageLatest, linter.SeverityError, `{"dockerComposeFile": "docker-compose.yml", "service": "app"}`, nil) + }) +} diff --git a/rules/no_dockerfile_image_latest.go b/rules/no_dockerfile_image_latest.go new file mode 100644 index 0000000..768d3f9 --- /dev/null +++ b/rules/no_dockerfile_image_latest.go @@ -0,0 +1,98 @@ +package rules + +import ( + "fmt" + + "github.com/bare-devcontainer/decolint/linter" + "github.com/tailscale/hujson" +) + +// NoDockerfileImageLatest reports an image the Dockerfile a devcontainer.json builds from pulls +// without an explicit tag or with the "latest" tag. It is [NoImageLatest] for the Dockerfile-based +// form, where the images are named in the Dockerfile rather than in the "image" property. +var NoDockerfileImageLatest = &linter.Rule{ + ID: "no-dockerfile-image-latest", + Description: `disallow a Dockerfile that pulls an image without an explicit tag or with the "latest" tag`, + LongDescription: `A configuration that builds from a Dockerfile still starts from a base image, and pinning the +devcontainer.json says nothing about what that image is: a "FROM" with no tag, or with "latest", resolves +to whatever the publisher last released. The container then changes from one rebuild to the next while +every file in the repository stays the same. Name the version in the "FROM" the way you would in "image". + +A "COPY --from" or a "RUN --mount=from" naming an image pulls one just as a "FROM" does, and what it +brings into the container moves under an unpinned reference the same way, so those are named too.`, + 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{""}, + Example: linter.Example{ + Bad: linter.Snippet{ + Files: []linter.ExampleFile{ + {Path: `devcontainer.json`, Content: `{ + "name": "api", + "build": { + "dockerfile": "Dockerfile" + } +} +`}, + {Path: `Dockerfile`, Content: `FROM mcr.microsoft.com/devcontainers/base:latest + +RUN apt-get update && apt-get install -y --no-install-recommends jq +`}, + }, + }, + Good: linter.Snippet{ + Files: []linter.ExampleFile{ + {Path: `devcontainer.json`, Content: `{ + "name": "api", + "build": { + "dockerfile": "Dockerfile" + } +} +`}, + {Path: `Dockerfile`, Content: `FROM mcr.microsoft.com/devcontainers/base:ubuntu-24.04 + +RUN apt-get update && apt-get install -y --no-install-recommends jq +`}, + }, + }, + Note: "The images a build of the Dockerfile pulls are checked: the base image of each stage the\n" + + "build reaches, and the images its `COPY --from` and `RUN --mount=from` instructions name.\n" + + "An image written with a `$` variable is not checked, since its value can come from\n" + + "`build.args`.\n" + + "The finding is reported at the property naming the Dockerfile, since that is what the\n" + + "devcontainer.json says about the image; the fix belongs in the Dockerfile.", + }, + Check: checkNoDockerfileImageLatest, +} + +func checkNoDockerfileImageLatest(ctx *linter.Context, node *linter.Node) []linter.Finding { + obj, ok := node.Value.Value.(*hujson.Object) + if !ok { + return nil + } + images, path, offset, ok := dockerfileBuildImages(ctx.Dir, obj) + if !ok { + return nil + } + + var findings []linter.Finding + for _, image := range images { + tag, hasTag := refTag(image.ref) + switch { + case !hasTag: + findings = append(findings, linter.Finding{ + Message: fmt.Sprintf("Dockerfile %q %s image %q, which has no explicit tag; pin a specific version", path, image.verb(), image.ref), + Offset: offset, + }) + case tag == "latest": + findings = append(findings, linter.Finding{ + Message: fmt.Sprintf("Dockerfile %q %s image %q, which uses the \"latest\" tag; pin a specific version", path, image.verb(), image.ref), + Offset: offset, + }) + } + } + return findings +} diff --git a/rules/no_dockerfile_image_latest_test.go b/rules/no_dockerfile_image_latest_test.go new file mode 100644 index 0000000..2bac41b --- /dev/null +++ b/rules/no_dockerfile_image_latest_test.go @@ -0,0 +1,354 @@ +package rules_test + +import ( + "testing" + "testing/fstest" + + "github.com/bare-devcontainer/decolint/linter" + "github.com/bare-devcontainer/decolint/rules" +) + +func TestNoDockerfileImageLatest(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-dockerfile-image-latest", Message: message}} + } + + tests := []struct { + name string + dockerfile string + want []linter.Issue + }{ + { + "untagged base image", + "FROM ubuntu\n", + issue(`Dockerfile "Dockerfile" builds from image "ubuntu", which has no explicit tag; pin a specific version`), + }, + { + "latest base image", + "FROM ubuntu:latest\n", + issue(`Dockerfile "Dockerfile" builds from image "ubuntu:latest", which 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" builds from image "BUILDER", which 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" builds from image "0", which has no explicit tag; pin a specific version`), + }, + { + "an image reached through a variable is not resolved", + "ARG VARIANT=24.04\nFROM ubuntu:${VARIANT}\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-dockerfile-image-latest", Message: `Dockerfile "Dockerfile" builds from image "golang:latest", which uses the "latest" tag; pin a specific version`}, + {Path: "devcontainer.json", Line: 1, Col: 26, RuleID: "no-dockerfile-image-latest", Message: `Dockerfile "Dockerfile" builds from image "ubuntu", which 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" builds from image "ubuntu:latest", which 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 +// each of them in each file — including the combinations the specification does not define. +func holdsFeatureRefs(fileType linter.FileType, pointer string) bool { + switch fileType { + case linter.Devcontainer: + return pointer == "/features" + case linter.Feature: + return pointer == "/dependsOn" + default: + return false + } +} + +// featureRef is an OCI Feature reference, as written for a key of a devcontainer.json "features" or +// a Feature's "dependsOn", with the byte offset of that key. +type featureRef struct { + ref string + offset int +} + +// ociFeatureRefs returns the OCI Feature references the members of v are keyed by, for a v that is +// an object of them. It returns none for a value that is not one. +// +// The local path and tarball URI forms are left out: neither carries a version to pin. See +// [isLocalFeature] and [isTarballFeature]. +func ociFeatureRefs(v *hujson.Value) []featureRef { + obj, ok := v.Value.(*hujson.Object) + if !ok { + return nil + } + var refs []featureRef + for _, m := range obj.Members { + name, ok := m.Name.Value.(hujson.Literal) + if !ok || name.Kind() != '"' { + continue + } + ref := name.String() + if isLocalFeature(ref) || isTarballFeature(ref) { + continue + } + refs = append(refs, featureRef{ref: ref, offset: m.Name.StartOffset}) + } + return refs +} + +// isLocalFeature reports whether ref names a Feature by a relative path, which has no version tag +// to pin. +func isLocalFeature(ref string) bool { + return strings.HasPrefix(ref, "./") || strings.HasPrefix(ref, "../") +} + +// isTarballFeature reports whether ref names a Feature by a direct HTTP(S) URI to a tarball, which +// has no version tag to pin. +func isTarballFeature(ref string) bool { + return strings.HasPrefix(ref, "http://") || strings.HasPrefix(ref, "https://") +} + +// unpinnedFeatureVersion describes how ref fails to name a specific Feature version, or "" if it +// names one. The text completes a message that begins with the reference, e.g. +// `feature "ghcr.io/devcontainers/features/go" has no explicit version; ...`. +func unpinnedFeatureVersion(ref string) string { + tag, hasTag := refTag(ref) + switch { + case !hasTag: + return "has no explicit version; pin a specific version" + case tag == "latest": + return `uses the "latest" version; pin a specific version` + default: + return "" + } +} + // refTag extracts the tag from an OCI-style reference, e.g. a container image or Feature reference. // A reference pinned by digest (e.g. "ref@sha256:...") is treated as tagged. The colon in a // registry host with a port (e.g. "localhost:5000/img") is not a tag separator. diff --git a/rules/util_test.go b/rules/util_test.go new file mode 100644 index 0000000..9ec7d25 --- /dev/null +++ b/rules/util_test.go @@ -0,0 +1,66 @@ +package rules + +import ( + "strings" + "testing" + "testing/fstest" + + "github.com/bare-devcontainer/decolint/linter" +) + +// TestHoldsFeatureRefs covers every file type, including the one no rule declaring these paths +// applies to, since the answer for it is part of the contract rather than a case that cannot arise. +func TestHoldsFeatureRefs(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + fileType linter.FileType + pointer string + want bool + }{ + {"devcontainer features", linter.Devcontainer, "/features", true}, + {"devcontainer dependsOn", linter.Devcontainer, "/dependsOn", false}, + {"feature dependsOn", linter.Feature, "/dependsOn", true}, + {"feature features", linter.Feature, "/features", false}, + {"template features", linter.Template, "/features", false}, + {"template dependsOn", linter.Template, "/dependsOn", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if got := holdsFeatureRefs(tt.fileType, tt.pointer); got != tt.want { + t.Errorf("holdsFeatureRefs(%q, %q) = %v, want %v", tt.fileType, tt.pointer, got, tt.want) + } + }) + } +} + +// 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) + } + }) + } +}