From a1229221ff2861c4b8e461d6cccf1fbd5e485840 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 2 Aug 2026 10:11:33 +0000 Subject: [PATCH 01/13] feat(rules): add five reproducibility rules for unpinned references The reproducibility rules only looked at a devcontainer.json's "image", "features", and "customizations", so the same moving reference went unreported wherever else it is written: a Dockerfile-based or Compose-based configuration escaped image pinning entirely, and a Feature's own dependencies were never checked. - no-dockerfile-image-latest and pin-dockerfile-image-digest read the Dockerfile named by "build.dockerfile" (or the legacy "dockerFile") and judge each FROM. A reference to an earlier stage, "scratch", and one containing a variable are left out: none names an image the configuration pins. - no-compose-image-latest reads the "image" of the Compose service the dev container runs in. A service that builds its own image, an image written as a variable, and a service no declared file defines are left out. - pin-depends-on-version checks a Feature's "dependsOn", where an unpinned reference installs a moving dependency into every project using the Feature, with no way for those projects to pin it. - pin-feature-exact-version requires a full "major.minor.patch", since the "major" and "major.minor" tags are reassigned on release. It stands to pin-feature-version as pin-image-digest stands to no-image-latest. A file another configuration file names is read through the directory the linted file was discovered in, so a path leading outside that boundary reports nothing rather than reaching for it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Y7ohBnPuPSezRvwrkzaRaA --- README.md | 2 +- go.mod | 2 +- rules/dockerfile.go | 88 +++++++++++ rules/no_compose_image_latest.go | 180 ++++++++++++++++++++++ rules/no_compose_image_latest_test.go | 134 ++++++++++++++++ rules/no_dockerfile_image_latest.go | 92 +++++++++++ rules/no_dockerfile_image_latest_test.go | 139 +++++++++++++++++ rules/pin_depends_on_version.go | 74 +++++++++ rules/pin_depends_on_version_test.go | 46 ++++++ rules/pin_dockerfile_image_digest.go | 84 ++++++++++ rules/pin_dockerfile_image_digest_test.go | 77 +++++++++ rules/pin_feature_exact_version.go | 89 +++++++++++ rules/pin_feature_exact_version_test.go | 91 +++++++++++ rules/pin_feature_version.go | 47 +----- rules/rules.go | 5 + rules/util.go | 97 ++++++++++++ 16 files changed, 1205 insertions(+), 42 deletions(-) create mode 100644 rules/dockerfile.go create mode 100644 rules/no_compose_image_latest.go create mode 100644 rules/no_compose_image_latest_test.go create mode 100644 rules/no_dockerfile_image_latest.go create mode 100644 rules/no_dockerfile_image_latest_test.go create mode 100644 rules/pin_depends_on_version.go create mode 100644 rules/pin_depends_on_version_test.go create mode 100644 rules/pin_dockerfile_image_digest.go create mode 100644 rules/pin_dockerfile_image_digest_test.go create mode 100644 rules/pin_feature_exact_version.go create mode 100644 rules/pin_feature_exact_version_test.go 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..d3c490c --- /dev/null +++ b/rules/dockerfile.go @@ -0,0 +1,88 @@ +package rules + +import ( + "bytes" + "strings" + + "github.com/bare-devcontainer/decolint/linter" + "github.com/moby/buildkit/frontend/dockerfile/instructions" + "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 +} + +// dockerfileBaseImages returns the images the Dockerfile in src builds from, in the order its FROM +// instructions name them, keeping a repeated image once per FROM. It returns nothing for a +// Dockerfile that does not parse, leaving a rule with nothing to report rather than a guess. +// +// Only FROMs naming an image outside the build are returned. Left out are: +// - a reference to an earlier stage of the same Dockerfile, which is not an image at all; +// - "scratch", the empty base; +// - a reference containing a variable, whose value comes from "build.args" or an ARG default and +// is not the linter's to resolve. +func dockerfileBaseImages(src []byte) []string { + result, err := parser.Parse(bytes.NewReader(src)) + if err != nil { + return nil + } + // The linter argument reports the lint warnings buildkit itself defines; a nil one turns them + // off, which is what a caller reading the stages wants. + stages, _, err := instructions.Parse(result.AST, nil) + if err != nil { + return nil + } + + var images []string + stageNames := map[string]struct{}{} + for _, stage := range stages { + base := stage.BaseName + _, isStage := stageNames[strings.ToLower(base)] + if base != "" && base != "scratch" && !isStage && !strings.Contains(base, "$") { + images = append(images, base) + } + if stage.Name != "" { + stageNames[strings.ToLower(stage.Name)] = struct{}{} + } + } + return images +} + +// dockerfileBuildImages returns the images the Dockerfile that obj, a devcontainer.json, declares +// builds from (see [dockerfileBaseImages]), 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 []string, 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 dockerfileBaseImages(src), 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..9c31b1d --- /dev/null +++ b/rules/no_compose_image_latest.go @@ -0,0 +1,180 @@ +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. A service that builds its own\n" + + "image is left to the Dockerfile rules, and a service whose image is written as a\n" + + "`${...}` variable is not reported: the value is not in the 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. +type composeService struct { + Image string `yaml:"image"` + Build any `yaml:"build"` +} + +// 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. +// +// ok is false whenever the answer is not in the files themselves, so that the caller reports +// nothing rather than reporting on a service it has only partly resolved: +// - a file that cannot be read (see [readConfigFile]) or does not parse; +// - a service none of the files defines, which "extends" or "include" may bring in from a file +// decolint does not follow; +// - 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 struct { + Services map[string]composeService `yaml:"services"` + } + if err := yaml.Unmarshal(src, &doc); err != nil { + return "", false + } + svc, ok := doc.Services[service] + if !ok { + continue + } + found = true + if svc.Build != nil { + return "", false + } + if svc.Image != "" { + image = svc.Image + } + } + 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..65715bc --- /dev/null +++ b/rules/no_compose_image_latest_test.go @@ -0,0 +1,134 @@ +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, + }, + {"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..d863132 --- /dev/null +++ b/rules/no_dockerfile_image_latest.go @@ -0,0 +1,92 @@ +package rules + +import ( + "fmt" + + "github.com/bare-devcontainer/decolint/linter" + "github.com/tailscale/hujson" +) + +// NoDockerfileImageLatest reports a FROM instruction of the Dockerfile a devcontainer.json builds +// from that names an image without an explicit tag or with the "latest" tag. It is [NoImageLatest] +// for the Dockerfile-based form, where the base image is named in the Dockerfile rather than in the +// "image" property. +var NoDockerfileImageLatest = &linter.Rule{ + ID: "no-dockerfile-image-latest", + Description: `disallow a Dockerfile that builds from 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".`, + 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 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) + switch { + case !hasTag: + findings = append(findings, linter.Finding{ + Message: fmt.Sprintf("Dockerfile %q builds from image %q, which has no explicit tag; pin a specific version", path, image), + Offset: offset, + }) + case tag == "latest": + findings = append(findings, linter.Finding{ + Message: fmt.Sprintf("Dockerfile %q builds from image %q, which uses the \"latest\" tag; pin a specific version", path, image), + 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..9bc51c7 --- /dev/null +++ b/rules/no_dockerfile_image_latest_test.go @@ -0,0 +1,139 @@ +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 stage name is matched case-insensitively", + "FROM golang:1.24 AS Builder\n\nFROM builder\n", + nil, + }, + { + "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\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 +} + +// 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. From 58ed5021c12b1d296198939d8ab5b806768e1ae5 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 21:22:50 +0000 Subject: [PATCH 02/13] fix(rules): correct the Dockerfile and Compose readers Three defects found in review, each confirmed end-to-end: - instructions.Parse panics on a nil buildkit linter, which a Dockerfile reaches through a "# check=..." comment: the merge of that comment's config dereferences the receiver. Every rule reading such a Dockerfile reported "rule panicked" instead of its findings. Pass a linter whose Warn is nil, which reports nothing without being nil itself. - The Compose reader guarded only "${VAR}", so the bare "$VAR" form reached the tag check and was reported as an image with no tag. - "build.target" was ignored, so stages the build never reaches were reported. Only the target stage and what it builds on and copies from are read now, and with no target the last stage, as "docker build" does. The Compose reader also stops at "extends" and "include", which can define or override a service from a file it does not read: it now reports only what compose-go's full resolution (see feature's loadComposeService) would report too. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Y7ohBnPuPSezRvwrkzaRaA --- rules/dockerfile.go | 132 +++++++++++++++++++--- rules/no_compose_image_latest.go | 40 ++++--- rules/no_compose_image_latest_test.go | 18 +++ rules/no_dockerfile_image_latest_test.go | 92 ++++++++++++++- rules/pin_dockerfile_image_digest_test.go | 2 +- rules/pin_feature_exact_version.go | 4 +- 6 files changed, 253 insertions(+), 35 deletions(-) diff --git a/rules/dockerfile.go b/rules/dockerfile.go index d3c490c..36d172d 100644 --- a/rules/dockerfile.go +++ b/rules/dockerfile.go @@ -2,10 +2,12 @@ package rules import ( "bytes" + "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" ) @@ -35,42 +37,140 @@ func dockerfileRef(obj *hujson.Object) (path string, offset int, ok bool) { return "", 0, false } -// dockerfileBaseImages returns the images the Dockerfile in src builds from, in the order its FROM -// instructions name them, keeping a repeated image once per FROM. It returns nothing for a -// Dockerfile that does not parse, leaving a rule with nothing to report rather than a guess. +// 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 +} + +// dockerfileBaseImages returns the images the Dockerfile in src builds from when target is built, +// in the order its FROM instructions name them, keeping a repeated image once per FROM. An empty +// target builds the last stage, as "docker build" does. // -// Only FROMs naming an image outside the build are returned. Left out are: +// Only the stages the build actually reaches are considered, since a stage nothing depends on is +// never built and its base image never pulled. Of those, only FROMs naming an image outside the +// build are returned. Left out are: // - a reference to an earlier stage of the same Dockerfile, which is not an image at all; // - "scratch", the empty base; // - a reference containing a variable, whose value comes from "build.args" or an ARG default and // is not the linter's to resolve. -func dockerfileBaseImages(src []byte) []string { +// +// 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 dockerfileBaseImages(src []byte, target string) []string { result, err := parser.Parse(bytes.NewReader(src)) if err != nil { return nil } - // The linter argument reports the lint warnings buildkit itself defines; a nil one turns them - // off, which is what a caller reading the stages wants. - stages, _, err := instructions.Parse(result.AST, 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 []string - stageNames := map[string]struct{}{} - for _, stage := range stages { + for i, stage := range stages { + if !built[i] { + continue + } base := stage.BaseName - _, isStage := stageNames[strings.ToLower(base)] - if base != "" && base != "scratch" && !isStage && !strings.Contains(base, "$") { - images = append(images, base) + if base == "" || base == "scratch" || strings.Contains(base, "$") { + continue } - if stage.Name != "" { - stageNames[strings.ToLower(stage.Name)] = struct{}{} + if j, isStage := stageIndex(stages, base); isStage && j < i { + continue } + images = append(images, base) } return images } +// 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 != "" { + i, ok := stageIndex(stages, 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 + for _, dep := range stageDeps(stages, i) { + queue = append(queue, dep) + } + } + return built +} + +// stageDeps returns the indexes of the stages the stage at i is built from: the one its FROM names, +// and the ones its instructions read through "--from", each only when it is a stage defined earlier +// rather than an image. +func stageDeps(stages []instructions.Stage, i int) []int { + var deps []int + add := func(ref string) { + if j, ok := stageIndex(stages, ref); ok && j < i { + deps = append(deps, j) + } + } + + add(stages[i].BaseName) + for _, cmd := range stages[i].Commands { + if copyCmd, ok := cmd.(*instructions.CopyCommand); ok { + add(copyCmd.From) + } + if runCmd, ok := cmd.(*instructions.RunCommand); ok { + for _, mount := range instructions.GetMounts(runCmd) { + add(mount.From) + } + } + } + return deps +} + +// stageIndex returns the index of the stage ref names, by its name or by its position, and reports +// whether it names one at all. Stage names are matched case-insensitively, as the Dockerfile parser +// matches them. +func stageIndex(stages []instructions.Stage, ref string) (int, bool) { + if ref == "" { + return 0, false + } + for i, stage := range stages { + if stage.Name != "" && strings.EqualFold(stage.Name, ref) { + return i, true + } + } + if i, err := strconv.Atoi(ref); err == nil && i >= 0 && i < len(stages) { + return i, true + } + return 0, false +} + // dockerfileBuildImages returns the images the Dockerfile that obj, a devcontainer.json, declares // builds from (see [dockerfileBaseImages]), 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 @@ -84,5 +184,5 @@ func dockerfileBuildImages(dir linter.Dir, obj *hujson.Object) (images []string, if !ok { return nil, "", 0, false } - return dockerfileBaseImages(src), path, offset, true + return dockerfileBaseImages(src, buildTarget(obj)), path, offset, true } diff --git a/rules/no_compose_image_latest.go b/rules/no_compose_image_latest.go index 9c31b1d..ca1efe6 100644 --- a/rules/no_compose_image_latest.go +++ b/rules/no_compose_image_latest.go @@ -130,23 +130,36 @@ func composeFilePaths(obj *hujson.Object) (paths []string, offset int, ok bool) } // composeService is the part of a Compose service definition that says which image the service -// runs. +// runs, or that the definition is not all in this file. type composeService struct { - Image string `yaml:"image"` - Build any `yaml:"build"` + 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. // -// ok is false whenever the answer is not in the files themselves, so that the caller reports -// nothing rather than reporting on a service it has only partly resolved: +// 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 service none of the files defines, which "extends" or "include" may bring in from a file -// decolint does not follow; +// - 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. +// - 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 @@ -155,10 +168,8 @@ func composeServiceImage(dir linter.Dir, paths []string, service string) (string if !ok { return "", false } - var doc struct { - Services map[string]composeService `yaml:"services"` - } - if err := yaml.Unmarshal(src, &doc); err != nil { + var doc composeDoc + if err := yaml.Unmarshal(src, &doc); err != nil || doc.Include != nil { return "", false } svc, ok := doc.Services[service] @@ -166,14 +177,15 @@ func composeServiceImage(dir linter.Dir, paths []string, service string) (string continue } found = true - if svc.Build != nil { + if svc.Build != nil || svc.Extends != nil { return "", false } if svc.Image != "" { image = svc.Image } } - if !found || image == "" || strings.Contains(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 index 65715bc..c202263 100644 --- a/rules/no_compose_image_latest_test.go +++ b/rules/no_compose_image_latest_test.go @@ -53,6 +53,24 @@ func TestNoComposeImageLatest(t *testing.T) { "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}, diff --git a/rules/no_dockerfile_image_latest_test.go b/rules/no_dockerfile_image_latest_test.go index 9bc51c7..f30c412 100644 --- a/rules/no_dockerfile_image_latest_test.go +++ b/rules/no_dockerfile_image_latest_test.go @@ -61,11 +61,46 @@ func TestNoDockerfileImageLatest(t *testing.T) { }, { "the same unpinned image in several stages is reported once", - "FROM ubuntu:latest AS a\n\nFROM ubuntu:latest AS b\n", + "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 < Date: Tue, 4 Aug 2026 23:43:03 +0000 Subject: [PATCH 03/13] fix(rules): check every image a Dockerfile build pulls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Dockerfile rules read only each stage's FROM, so an image a "COPY --from" or a "RUN --mount=from" names went unreported. BuildKit pulls those too: a "--from" naming no stage becomes a dispatch state of its own, whose base image is resolved with the rest. A Dockerfile whose FROM is digest-pinned but which copies a tool from "ghcr.io/…/uv:latest" was reported clean. The stage lookup also matched more than BuildKit does. A FROM base is matched against the stages declared before it, as written, so "FROM Builder" after "AS builder" names an image; only a COPY's "--from" accepts a stage position, while "build.target" and a RUN --mount's "--from" are names BuildKit lower-cases first. Reporting the earlier stage's image for "FROM 0" named an image the build never pulls, and hid the one it does. Also state in the Compose rule's example what is left unchecked, rather than attributing it to the Dockerfile rules, which read a Compose service's "build" nowhere; and cover the two branches the suite reached by neither input: a Dockerfile of global ARGs alone, which parses to no stage at all, and a file over the size cap. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Y7ohBnPuPSezRvwrkzaRaA --- rules/dockerfile.go | 169 ++++++++++++++++------ rules/no_compose_image_latest.go | 7 +- rules/no_dockerfile_image_latest.go | 24 +-- rules/no_dockerfile_image_latest_test.go | 81 ++++++++++- rules/pin_dockerfile_image_digest.go | 21 ++- rules/pin_dockerfile_image_digest_test.go | 15 ++ rules/util.go | 6 +- rules/util_test.go | 38 +++++ 8 files changed, 288 insertions(+), 73 deletions(-) create mode 100644 rules/util_test.go diff --git a/rules/dockerfile.go b/rules/dockerfile.go index 36d172d..c4acee5 100644 --- a/rules/dockerfile.go +++ b/rules/dockerfile.go @@ -52,21 +52,36 @@ func buildTarget(obj *hujson.Object) string { return target } -// dockerfileBaseImages returns the images the Dockerfile in src builds from when target is built, -// in the order its FROM instructions name them, keeping a repeated image once per FROM. An empty -// target builds the last stage, as "docker build" does. +// 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 base image never pulled. Of those, only FROMs naming an image outside the -// build are returned. Left out are: -// - a reference to an earlier stage of the same Dockerfile, which is not an image at all; -// - "scratch", the empty base; -// - a reference containing a variable, whose value comes from "build.args" or an ARG default and -// is not the linter's to resolve. +// 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 dockerfileBaseImages(src []byte, target string) []string { +func dockerfilePulledImages(src []byte, target string) []dockerfileImage { result, err := parser.Parse(bytes.NewReader(src)) if err != nil { return nil @@ -80,23 +95,33 @@ func dockerfileBaseImages(src []byte, target string) []string { } built := builtStages(stages, target) - var images []string - for i, stage := range stages { + var images []dockerfileImage + for i := range stages { if !built[i] { continue } - base := stage.BaseName - if base == "" || base == "scratch" || strings.Contains(base, "$") { - continue + if _, isStage := stageBase(stages, i); !isStage && isPulledImage(stages[i].BaseName) { + images = append(images, dockerfileImage{ref: stages[i].BaseName, base: true}) } - if j, isStage := stageIndex(stages, base); isStage && j < i { - continue + for _, from := range stageFroms(stages, i) { + if from.stage < 0 && isPulledImage(from.ref) { + images = append(images, dockerfileImage{ref: from.ref}) + } } - images = append(images, base) } 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 @@ -107,7 +132,9 @@ func builtStages(stages []instructions.Stage, target string) map[int]bool { } start := len(stages) - 1 if target != "" { - i, ok := stageIndex(stages, 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 } @@ -128,54 +155,100 @@ func builtStages(stages []instructions.Stage, target string) map[int]bool { return built } -// stageDeps returns the indexes of the stages the stage at i is built from: the one its FROM names, -// and the ones its instructions read through "--from", each only when it is a stage defined earlier -// rather than an image. +// 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 - add := func(ref string) { - if j, ok := stageIndex(stages, ref); ok && j < i { - deps = append(deps, j) + 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} } - add(stages[i].BaseName) + var froms []stageFrom for _, cmd := range stages[i].Commands { - if copyCmd, ok := cmd.(*instructions.CopyCommand); ok { - add(copyCmd.From) - } - if runCmd, ok := cmd.(*instructions.RunCommand); ok { - for _, mount := range instructions.GetMounts(runCmd) { - add(mount.From) + 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 deps + return froms } -// stageIndex returns the index of the stage ref names, by its name or by its position, and reports -// whether it names one at all. Stage names are matched case-insensitively, as the Dockerfile parser -// matches them. -func stageIndex(stages []instructions.Stage, ref string) (int, bool) { - if ref == "" { - return 0, false - } +// 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 stage named ref and reports whether one is. 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 stages { - if stage.Name != "" && strings.EqualFold(stage.Name, ref) { + // A stage left unnamed has no name to be reached by, whatever ref is. + if stage.Name != "" && stage.Name == ref { return i, true } } - if i, err := strconv.Atoi(ref); err == nil && i >= 0 && i < len(stages) { - return i, true - } return 0, false } -// dockerfileBuildImages returns the images the Dockerfile that obj, a devcontainer.json, declares -// builds from (see [dockerfileBaseImages]), along with the Dockerfile's path as written and the +// 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 []string, path string, offset int, ok bool) { +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 @@ -184,5 +257,5 @@ func dockerfileBuildImages(dir linter.Dir, obj *hujson.Object) (images []string, if !ok { return nil, "", 0, false } - return dockerfileBaseImages(src, buildTarget(obj)), path, offset, true + return dockerfilePulledImages(src, buildTarget(obj)), path, offset, true } diff --git a/rules/no_compose_image_latest.go b/rules/no_compose_image_latest.go index ca1efe6..7cfd25e 100644 --- a/rules/no_compose_image_latest.go +++ b/rules/no_compose_image_latest.go @@ -60,9 +60,10 @@ or with "latest", pulls whatever the publisher last released — a container tha `}, }, }, - Note: "Only the service the dev container runs in is checked. A service that builds its own\n" + - "image is left to the Dockerfile rules, and a service whose image is written as a\n" + - "`${...}` variable is not reported: the value is not in the configuration.", + 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, } diff --git a/rules/no_dockerfile_image_latest.go b/rules/no_dockerfile_image_latest.go index d863132..a43b7da 100644 --- a/rules/no_dockerfile_image_latest.go +++ b/rules/no_dockerfile_image_latest.go @@ -7,17 +7,19 @@ import ( "github.com/tailscale/hujson" ) -// NoDockerfileImageLatest reports a FROM instruction of the Dockerfile a devcontainer.json builds -// from that names an image without an explicit tag or with the "latest" tag. It is [NoImageLatest] -// for the Dockerfile-based form, where the base image is named in the Dockerfile rather than in the -// "image" property. +// 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 builds from an image without an explicit tag or with the "latest" tag`, + 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".`, +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`, @@ -56,7 +58,9 @@ RUN apt-get update && apt-get install -y --no-install-recommends jq `}, }, }, - Note: "The finding is reported at the property naming the Dockerfile, since that is what the\n" + + Note: "Every image a build of the Dockerfile pulls is checked: the base image of each stage the\n" + + "build reaches, and the images its `COPY --from` and `RUN --mount=from` instructions name.\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, @@ -74,16 +78,16 @@ func checkNoDockerfileImageLatest(ctx *linter.Context, node *linter.Node) []lint var findings []linter.Finding for _, image := range images { - tag, hasTag := refTag(image) + tag, hasTag := refTag(image.ref) switch { case !hasTag: findings = append(findings, linter.Finding{ - Message: fmt.Sprintf("Dockerfile %q builds from image %q, which has no explicit tag; pin a specific version", path, image), + 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 builds from image %q, which uses the \"latest\" tag; pin a specific version", path, image), + Message: fmt.Sprintf("Dockerfile %q %s image %q, which uses the \"latest\" tag; pin a specific version", path, image.verb(), image.ref), Offset: offset, }) } diff --git a/rules/no_dockerfile_image_latest_test.go b/rules/no_dockerfile_image_latest_test.go index f30c412..5eef590 100644 --- a/rules/no_dockerfile_image_latest_test.go +++ b/rules/no_dockerfile_image_latest_test.go @@ -42,10 +42,25 @@ func TestNoDockerfileImageLatest(t *testing.T) { nil, }, { - "a stage name is matched case-insensitively", + // 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", @@ -95,12 +110,47 @@ func TestNoDockerfileImageLatest(t *testing.T) { "FROM golang:latest\n\nFROM ubuntu:24.04\nCOPY --from=0 /app /app\n", issue(`Dockerfile "Dockerfile" builds from image "golang:latest", which uses the "latest" tag; pin a specific version`), }, + { + "an image copied from is reported", + "FROM ubuntu:24.04\nCOPY --from=ghcr.io/astral-sh/uv:latest /uv /bin/\n", + issue(`Dockerfile "Dockerfile" pulls image "ghcr.io/astral-sh/uv:latest", which uses the "latest" tag; pin a specific version`), + }, + { + "an image mounted from is reported", + "FROM ubuntu:24.04\nRUN --mount=from=busybox,target=/b /b/bin/echo hi\n", + issue(`Dockerfile "Dockerfile" pulls image "busybox", which has no explicit tag; pin a specific version`), + }, + { + // A mount naming no source mounts the build context, and one naming no stage is matched + // by name alone, so neither reaches the stage at that position. + "a mount is not a position and needs no source", + "FROM golang:latest\n\nFROM ubuntu:24.04\nRUN --mount=target=/b --mount=from=0,target=/c true\n", + issue(`Dockerfile "Dockerfile" pulls image "0", which has no explicit tag; pin a specific version`), + }, + { + // Out of range, the position names no stage at all and the build fails on it, so there + // is no image to report either. + "a position no stage occupies is not an image", + "FROM ubuntu:24.04\nCOPY --from=9 /app /app\n", + nil, + }, + { + "an ordinary copy pulls nothing", + "FROM ubuntu:24.04\nCOPY app /app\nRUN --mount=type=cache,target=/c true\n", + nil, + }, + { + "a copy from a variable is not resolved", + "FROM ubuntu:24.04\nCOPY --from=$BUILDER /app /app\n", + nil, + }, { "a stage reached twice is read once", "FROM ubuntu:latest AS base\n\nFROM base AS mid\nCOPY --from=base /x /x\n", issue(`Dockerfile "Dockerfile" builds from image "ubuntu:latest", which uses the "latest" tag; pin a specific version`), }, {"a Dockerfile with no stage at all reports nothing", "# nothing to build here\n", nil}, + {"a Dockerfile of global ARGs alone reports nothing", "ARG VERSION=1.0\n", nil}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -148,7 +198,19 @@ COPY --from=tools /go /go `{"build": {"dockerfile": "Dockerfile"}}`, toolsIssue, }, + { + "a target is matched case-insensitively", + `{"build": {"dockerfile": "Dockerfile", "target": "DEV"}}`, + toolsIssue, + }, {"a target naming no stage reports nothing", `{"build": {"dockerfile": "Dockerfile", "target": "absent"}}`, nil}, + { + // A target names a stage and never a position, and a stage name cannot begin with a + // digit, so the build fails rather than building the stage at that position. + "a target written as a position reports nothing", + `{"build": {"dockerfile": "Dockerfile", "target": "0"}}`, + nil, + }, {"a non-string target is no target", `{"build": {"dockerfile": "Dockerfile", "target": 42}}`, toolsIssue}, { // The legacy top-level property names the Dockerfile; a "build" beside it that is not @@ -166,6 +228,23 @@ COPY --from=tools /go /go } } +// TestNoDockerfileImageLatest_ForwardStageReference checks that a stage copied from before it is +// declared is read as the stage it names rather than as an image: BuildKit resolves a "--from" +// once the whole Dockerfile is parsed, so the order the two stages are written in does not matter. +func TestNoDockerfileImageLatest_ForwardStageReference(t *testing.T) { + t.Parallel() + + const dockerfile = `FROM ubuntu:24.04 AS dev +COPY --from=tools /go /go + +FROM golang:latest AS tools +` + dir := linter.Dir{FS: fstest.MapFS{"Dockerfile": {Data: []byte(dockerfile)}}} + src := `{"build": {"dockerfile": "Dockerfile", "target": "dev"}}` + want := []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`}} + assertIssuesInDir(t, rules.NoDockerfileImageLatest, linter.SeverityError, "devcontainer.json", linter.Devcontainer, src, dir, want) +} + func TestNoDockerfileImageLatest_DockerfileLocation(t *testing.T) { t.Parallel() diff --git a/rules/pin_dockerfile_image_digest.go b/rules/pin_dockerfile_image_digest.go index 936579f..58ee5cd 100644 --- a/rules/pin_dockerfile_image_digest.go +++ b/rules/pin_dockerfile_image_digest.go @@ -7,17 +7,20 @@ import ( "github.com/tailscale/hujson" ) -// PinDockerfileImageDigest reports a FROM instruction of the Dockerfile a devcontainer.json builds -// from that names an image without a content digest. It is [PinImageDigest] for the -// Dockerfile-based form, and stands to [NoDockerfileImageLatest] as that rule stands to -// [NoImageLatest]: any unpinned reference is reported, not only a missing or "latest" tag. +// PinDockerfileImageDigest reports an image the Dockerfile a devcontainer.json builds from pulls +// without a content digest. It is [PinImageDigest] for the Dockerfile-based form, and stands to +// [NoDockerfileImageLatest] as that rule stands to [NoImageLatest]: any unpinned reference is +// reported, not only a missing or "latest" tag. var PinDockerfileImageDigest = &linter.Rule{ ID: "pin-dockerfile-image-digest", - Description: `disallow a Dockerfile that builds from an image not pinned by content digest (e.g. "FROM image@sha256:...")`, + Description: `disallow a Dockerfile that pulls an image not pinned by content digest (e.g. "FROM image@sha256:...")`, LongDescription: `A "FROM" with a fixed tag still resolves through a mutable pointer: the publisher can move the tag to different bits, so two builds of the same Dockerfile can start from different images. Writing the digest ("FROM image:tag@sha256:...") names the content itself, and the build verifies what it pulled against it. -Keeping the tag alongside the digest leaves the reference readable.`, +Keeping the tag alongside the digest leaves the reference readable. + +An image a "COPY --from" or a "RUN --mount=from" names is pulled through the same mutable pointer, so +it takes a digest too.`, References: []string{ `https://containers.dev/implementors/spec/#dockerfile-based`, `https://github.com/opencontainers/image-spec/blob/main/descriptor.md#digests`, @@ -56,6 +59,8 @@ RUN apt-get update && apt-get install -y --no-install-recommends jq `}, }, }, + Note: "Every image a build of the Dockerfile pulls is checked: the base image of each stage the\n" + + "build reaches, and the images its `COPY --from` and `RUN --mount=from` instructions name.", }, Check: checkPinDockerfileImageDigest, } @@ -72,11 +77,11 @@ func checkPinDockerfileImageDigest(ctx *linter.Context, node *linter.Node) []lin var findings []linter.Finding for _, image := range images { - if digestSuffix.MatchString(image) { + if digestSuffix.MatchString(image.ref) { continue } findings = append(findings, linter.Finding{ - Message: fmt.Sprintf("Dockerfile %q builds from image %q, which is not pinned by digest; add an \"@sha256:...\" digest", path, image), + Message: fmt.Sprintf("Dockerfile %q %s image %q, which is not pinned by digest; add an \"@sha256:...\" digest", path, image.verb(), image.ref), Offset: offset, }) } diff --git a/rules/pin_dockerfile_image_digest_test.go b/rules/pin_dockerfile_image_digest_test.go index e54cab1..e1f8290 100644 --- a/rules/pin_dockerfile_image_digest_test.go +++ b/rules/pin_dockerfile_image_digest_test.go @@ -48,6 +48,21 @@ func TestPinDockerfileImageDigest(t *testing.T) { {Path: "devcontainer.json", Line: 1, Col: 26, RuleID: "pin-dockerfile-image-digest", Message: `Dockerfile "Dockerfile" builds from image "ubuntu:24.04", which is not pinned by digest; add an "@sha256:..." digest`}, }, }, + { + "an image copied from is reported", + "FROM ubuntu:24.04@sha256:abc123\nCOPY --from=ghcr.io/astral-sh/uv:0.9.7 /uv /bin/\n", + issue(`Dockerfile "Dockerfile" pulls image "ghcr.io/astral-sh/uv:0.9.7", which is not pinned by digest; add an "@sha256:..." digest`), + }, + { + "an image mounted from is reported", + "FROM ubuntu:24.04@sha256:abc123\nRUN --mount=from=busybox:1.37,target=/b /b/bin/echo hi\n", + issue(`Dockerfile "Dockerfile" pulls image "busybox:1.37", which is not pinned by digest; add an "@sha256:..." digest`), + }, + { + "an image copied from by digest is pinned", + "FROM ubuntu:24.04@sha256:abc123\nCOPY --from=busybox@sha256:def456 /bin/busybox /bin/\n", + nil, + }, {"a Dockerfile that does not parse reports nothing", "FROM\n", nil}, } for _, tt := range tests { diff --git a/rules/util.go b/rules/util.go index d08e919..d3ad4a2 100644 --- a/rules/util.go +++ b/rules/util.go @@ -238,9 +238,9 @@ const maxConfigFileBytes = 4 << 20 // 4 MB // // It reports false rather than an error because a rule reads such a file to say something about it, // and can say nothing when it is absent, unreadable, or too large (see maxConfigFileBytes). A path -// leading outside the directory is also not read: access is confined to the boundary the file was -// discovered through (see [discovery.VisitConfigs]), so a rule reports nothing on configuration -// that names a file decolint may not open. +// leading outside the directory is also not read: access is confined to the boundary discovery hands +// the rule the directory through, so a rule reports nothing on configuration that names a file +// decolint may not open. func readConfigFile(dir linter.Dir, name string) ([]byte, bool) { if dir.FS == nil { return nil, false diff --git a/rules/util_test.go b/rules/util_test.go new file mode 100644 index 0000000..32f92e8 --- /dev/null +++ b/rules/util_test.go @@ -0,0 +1,38 @@ +package rules + +import ( + "strings" + "testing" + "testing/fstest" + + "github.com/bare-devcontainer/decolint/linter" +) + +// 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) + } + }) + } +} From 825667cee6b09d6b55aea64fdba985cd7e3d0a2f Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 00:09:04 +0000 Subject: [PATCH 04/13] fix(rules): reach the last stage sharing a name, as a build does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Several stages may be declared under one name. BuildKit only warns about that and keeps one stage per name as it registers them in turn, so a reference reaches the last of them; the lookup here returned the first, which is a different stage whenever a name is repeated. It went wrong in both directions. A "COPY --from" naming a repeated stage read the wrong one, so a Dockerfile that copies a tool from an unpinned "AS tools" declared second was reported clean; a "build.target" or a FROM base naming one reported the image of a stage the build replaces. A differential run against BuildKit disagreed on 56 of 235 valid multi-stage Dockerfiles before, and on none after. The two Dockerfile rules also claimed in their examples that every image a build pulls is checked, while an image written with a "$" variable is deliberately left out — a gap that shows up in the "ARG VARIANT" form devcontainer templates use, and that was documented nowhere the reader can see. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Y7ohBnPuPSezRvwrkzaRaA --- rules/dockerfile.go | 14 ++++--- rules/no_dockerfile_image_latest.go | 4 +- rules/no_dockerfile_image_latest_test.go | 50 +++++++++++++++++++++++- rules/pin_dockerfile_image_digest.go | 6 ++- 4 files changed, 63 insertions(+), 11 deletions(-) diff --git a/rules/dockerfile.go b/rules/dockerfile.go index c4acee5..51c06cf 100644 --- a/rules/dockerfile.go +++ b/rules/dockerfile.go @@ -230,14 +230,16 @@ func stageBase(stages []instructions.Stage, i int) (int, bool) { return stageNamed(stages[:i], stages[i].BaseName) } -// stageNamed returns the index of the stage named ref and reports whether one is. 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. +// 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 stages { + for i := len(stages) - 1; i >= 0; i-- { // A stage left unnamed has no name to be reached by, whatever ref is. - if stage.Name != "" && stage.Name == ref { + if stages[i].Name != "" && stages[i].Name == ref { return i, true } } diff --git a/rules/no_dockerfile_image_latest.go b/rules/no_dockerfile_image_latest.go index a43b7da..768d3f9 100644 --- a/rules/no_dockerfile_image_latest.go +++ b/rules/no_dockerfile_image_latest.go @@ -58,8 +58,10 @@ RUN apt-get update && apt-get install -y --no-install-recommends jq `}, }, }, - Note: "Every image a build of the Dockerfile pulls is checked: the base image of each stage the\n" + + 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.", }, diff --git a/rules/no_dockerfile_image_latest_test.go b/rules/no_dockerfile_image_latest_test.go index 5eef590..2bac41b 100644 --- a/rules/no_dockerfile_image_latest_test.go +++ b/rules/no_dockerfile_image_latest_test.go @@ -41,6 +41,12 @@ func TestNoDockerfileImageLatest(t *testing.T) { "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", @@ -100,6 +106,11 @@ func TestNoDockerfileImageLatest(t *testing.T) { "FROM golang:latest AS builder\n\nFROM ubuntu:24.04\nCOPY --from=builder /app /app\n", issue(`Dockerfile "Dockerfile" builds from image "golang:latest", which uses the "latest" tag; pin a specific version`), }, + { + "a copy reaches the last stage declared under a shared name", + "FROM golang:1.24 AS tools\n\nFROM golang:latest AS tools\n\nFROM ubuntu:24.04\nCOPY --from=tools /go /go\n", + issue(`Dockerfile "Dockerfile" builds from image "golang:latest", which uses the "latest" tag; pin a specific version`), + }, { "a stage the last one mounts from is reported", "FROM golang:latest AS builder\n\nFROM ubuntu:24.04\nRUN --mount=from=builder,target=/app echo hi\n", @@ -228,9 +239,44 @@ COPY --from=tools /go /go } } +// TestNoDockerfileImageLatest_DuplicateStageName checks that a name several stages share reaches the +// last of them, both as a "build.target" and as a "--from" looked up across the whole Dockerfile. +func TestNoDockerfileImageLatest_DuplicateStageName(t *testing.T) { + t.Parallel() + + // Both cases build the "dev" stage, and "build.dockerfile" comes first, so its value starts at + // column 26. + const src = `{"build": {"dockerfile": "Dockerfile", "target": "dev"}}` + + tests := []struct { + name string + dockerfile string + want []linter.Issue + }{ + { + "a target reaches the last stage declared under its name", + "FROM golang:latest AS dev\n\nFROM ubuntu:24.04 AS dev\n", + nil, + }, + { + "a copy reaches the last stage declared under its name", + "FROM golang:1.24 AS tools\n\nFROM ubuntu:24.04 AS dev\nCOPY --from=tools /go /go\n\nFROM golang:latest AS tools\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`}}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + dir := linter.Dir{FS: fstest.MapFS{"Dockerfile": {Data: []byte(tt.dockerfile)}}} + assertIssuesInDir(t, rules.NoDockerfileImageLatest, linter.SeverityError, "devcontainer.json", linter.Devcontainer, src, dir, tt.want) + }) + } +} + // TestNoDockerfileImageLatest_ForwardStageReference checks that a stage copied from before it is -// declared is read as the stage it names rather than as an image: BuildKit resolves a "--from" -// once the whole Dockerfile is parsed, so the order the two stages are written in does not matter. +// declared is read as the stage it names rather than as an image: BuildKit looks a "--from" up +// among every stage of the Dockerfile, wherever it is declared. Running such a build then fails, +// the copy naming a stage not yet built, but what the reference names is a stage all the same. func TestNoDockerfileImageLatest_ForwardStageReference(t *testing.T) { t.Parallel() diff --git a/rules/pin_dockerfile_image_digest.go b/rules/pin_dockerfile_image_digest.go index 58ee5cd..34e2a1d 100644 --- a/rules/pin_dockerfile_image_digest.go +++ b/rules/pin_dockerfile_image_digest.go @@ -59,8 +59,10 @@ RUN apt-get update && apt-get install -y --no-install-recommends jq `}, }, }, - Note: "Every image a build of the Dockerfile pulls is checked: the base image of each stage the\n" + - "build reaches, and the images its `COPY --from` and `RUN --mount=from` instructions name.", + 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`.", }, Check: checkPinDockerfileImageDigest, } From f192b9498a7ab82135d2c7eb9c2e9dee3ffae7f6 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 04:36:51 +0000 Subject: [PATCH 05/13] fix(rules): read a Feature reference only where one is written MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pin-feature-exact-version declares both properties Feature references are written under, and a rule's paths are matched in every file type it applies to — so it also read a "features" member of a devcontainer-feature.json and a "dependsOn" member of a devcontainer.json, neither of which the specification defines. A Feature that happened to carry a "features" object was told to pin versions in it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Y7ohBnPuPSezRvwrkzaRaA --- rules/pin_feature_exact_version.go | 6 +++++- rules/pin_feature_exact_version_test.go | 14 +++++++++++++ rules/util.go | 15 +++++++++++++ rules/util_test.go | 28 +++++++++++++++++++++++++ 4 files changed, 62 insertions(+), 1 deletion(-) diff --git a/rules/pin_feature_exact_version.go b/rules/pin_feature_exact_version.go index 4f8b06f..093e726 100644 --- a/rules/pin_feature_exact_version.go +++ b/rules/pin_feature_exact_version.go @@ -62,7 +62,11 @@ A reference pinned by digest is accepted as it already names exact content.`, Check: checkPinFeatureExactVersion, } -func checkPinFeatureExactVersion(_ *linter.Context, node *linter.Node) []linter.Finding { +func checkPinFeatureExactVersion(ctx *linter.Context, node *linter.Node) []linter.Finding { + if !holdsFeatureRefs(ctx.Type, node.Pointer) { + return nil + } + var findings []linter.Finding for _, f := range ociFeatureRefs(node.Value) { // A digest names the content itself, whatever tag it is written alongside. diff --git a/rules/pin_feature_exact_version_test.go b/rules/pin_feature_exact_version_test.go index aa67724..102d762 100644 --- a/rules/pin_feature_exact_version_test.go +++ b/rules/pin_feature_exact_version_test.go @@ -53,6 +53,13 @@ func TestPinFeatureExactVersion(t *testing.T) { issue(`feature "localhost:5000/features/foo" has no explicit version; pin a full "major.minor.patch" version`), }, {"registry port with a full version", `{"features": {"localhost:5000/features/foo:1.0.0": {}}}`, nil}, + { + // "dependsOn" is a Feature's property; a devcontainer.json asks for Features under + // "features" alone, so a member spelled that way holds no Feature reference. + "a dependsOn member of a devcontainer.json is not a Feature reference", + `{"dependsOn": {"ghcr.io/devcontainers/features/go": {}}}`, + nil, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -81,6 +88,13 @@ func TestPinFeatureExactVersion_DependsOn(t *testing.T) { }, {"full version", `{"dependsOn": {"ghcr.io/devcontainers/features/common-utils:2.6.2": {}}}`, nil}, {"no dependsOn property", `{"id": "my-feature"}`, nil}, + { + // A Feature declares its dependencies under "dependsOn"; the specification gives it no + // "features" property, so a member spelled that way holds no Feature reference. + "a features member of a Feature is not a Feature reference", + `{"features": {"ghcr.io/devcontainers/features/common-utils": {}}}`, + nil, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/rules/util.go b/rules/util.go index d3ad4a2..3750ec4 100644 --- a/rules/util.go +++ b/rules/util.go @@ -264,6 +264,21 @@ func readConfigFile(dir linter.Dir, name string) ([]byte, bool) { 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 { diff --git a/rules/util_test.go b/rules/util_test.go index 32f92e8..9ec7d25 100644 --- a/rules/util_test.go +++ b/rules/util_test.go @@ -8,6 +8,34 @@ import ( "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. From 8d92e079f89f3c21f23c3edc7af8e881fde24ad7 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 04:52:44 +0000 Subject: [PATCH 06/13] fix(rules): read a full Feature version as semver defines one pin-feature-exact-version accepted a component with a leading zero, which semver forbids, so a reference no Feature can be published under passed as pinned to one. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Y7ohBnPuPSezRvwrkzaRaA --- rules/pin_feature_exact_version.go | 7 ++++--- rules/pin_feature_exact_version_test.go | 12 ++++++++++++ 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/rules/pin_feature_exact_version.go b/rules/pin_feature_exact_version.go index 093e726..5fc39b7 100644 --- a/rules/pin_feature_exact_version.go +++ b/rules/pin_feature_exact_version.go @@ -9,9 +9,10 @@ import ( ) // exactFeatureVersion matches a full "major.minor.patch" Feature version, with the optional -// prerelease suffix semver allows and an OCI tag can spell. The build metadata semver also allows -// is not matched: "+" is not a legal character in a tag, so no Feature is published under one. -var exactFeatureVersion = regexp.MustCompile(`^[0-9]+\.[0-9]+\.[0-9]+(?:-[0-9A-Za-z.-]+)?$`) +// prerelease suffix semver allows. A component with a leading zero is not one: semver forbids it, so +// no Feature is published under it. Neither is the build metadata semver allows, "+" being no legal +// character in a tag. +var exactFeatureVersion = regexp.MustCompile(`^(?:0|[1-9][0-9]*)(?:\.(?:0|[1-9][0-9]*)){2}(?:-[0-9A-Za-z.-]+)?$`) // PinFeatureExactVersion reports a Feature reference that names something other than one published // version, in a devcontainer.json's "features" or a Feature's "dependsOn". Unlike diff --git a/rules/pin_feature_exact_version_test.go b/rules/pin_feature_exact_version_test.go index 102d762..e55a6fa 100644 --- a/rules/pin_feature_exact_version_test.go +++ b/rules/pin_feature_exact_version_test.go @@ -42,6 +42,18 @@ func TestPinFeatureExactVersion(t *testing.T) { }, {"full version", `{"features": {"ghcr.io/devcontainers/features/go:1.3.2": {}}}`, nil}, {"full version with a prerelease", `{"features": {"ghcr.io/devcontainers/features/go:1.3.2-beta.1": {}}}`, nil}, + {"a zero component is a version", `{"features": {"ghcr.io/devcontainers/features/go:0.1.0": {}}}`, nil}, + { + // semver forbids a leading zero, so no Feature is published under such a version. + "leading zero", + `{"features": {"ghcr.io/devcontainers/features/go:01.2.3": {}}}`, + issue(`feature "ghcr.io/devcontainers/features/go:01.2.3" uses version "01.2.3"; pin a full "major.minor.patch" version`), + }, + { + "a v prefix is not a version", + `{"features": {"ghcr.io/devcontainers/features/go:v1.3.2": {}}}`, + issue(`feature "ghcr.io/devcontainers/features/go:v1.3.2" uses version "v1.3.2"; pin a full "major.minor.patch" version`), + }, {"digest alone", `{"features": {"ghcr.io/devcontainers/features/go@sha256:abc123": {}}}`, nil}, {"partial version alongside a digest", `{"features": {"ghcr.io/devcontainers/features/go:1@sha256:abc123": {}}}`, nil}, {"local path feature", `{"features": {"./local-feature": {}}}`, nil}, From 5754f4c2ff0726485b5d76fee8867a5cc411f7d9 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 07:53:03 +0000 Subject: [PATCH 07/13] fix(rules): address golangci-lint findings in the Dockerfile reader The stage lookup's backward scan is spelled with slices.Backward, and the dependency walk appends its slice in one call. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Y7ohBnPuPSezRvwrkzaRaA --- rules/dockerfile.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/rules/dockerfile.go b/rules/dockerfile.go index 51c06cf..1259070 100644 --- a/rules/dockerfile.go +++ b/rules/dockerfile.go @@ -2,6 +2,7 @@ package rules import ( "bytes" + "slices" "strconv" "strings" @@ -148,9 +149,7 @@ func builtStages(stages []instructions.Stage, target string) map[int]bool { continue } built[i] = true - for _, dep := range stageDeps(stages, i) { - queue = append(queue, dep) - } + queue = append(queue, stageDeps(stages, i)...) } return built } @@ -237,9 +236,9 @@ func stageBase(stages []instructions.Stage, i int) (int, bool) { // 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 := len(stages) - 1; i >= 0; i-- { + for i, stage := range slices.Backward(stages) { // A stage left unnamed has no name to be reached by, whatever ref is. - if stages[i].Name != "" && stages[i].Name == ref { + if stage.Name != "" && stage.Name == ref { return i, true } } From 5db754da1a1f5bc75ec3df12e2316f1ef49cd242 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 03:45:05 +0000 Subject: [PATCH 08/13] feat(rules): read the Dockerfile a Compose service builds from MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A Compose-based configuration reached no Dockerfile rule: the Compose rule stops at a service that builds its own image, since the "image" such a service carries names what the build produces, and nothing then read the "build" it produces it with. A service building from a "FROM ubuntu:latest" was reported clean. The Dockerfile rules now resolve that build, in either form Compose writes it — the context alone, or the options object, including "dockerfile_inline" — against the directory of the file declaring it, honoring its "target". Compose is resolved before the configuration's own "build.dockerfile", as the reference implementation resolves the base image in that order. A build more than one file declares is left alone, Compose merging those option by option, as is a context naming a repository or written as a variable. Findings name the service rather than the Dockerfile, since the service is what the devcontainer.json says; the message for a Dockerfile the configuration names itself is unchanged. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Y7ohBnPuPSezRvwrkzaRaA --- rules/compose.go | 179 ++++++++++++++++++++ rules/compose_test.go | 188 ++++++++++++++++++++++ rules/dockerfile.go | 46 +++++- rules/no_compose_image_latest.go | 104 +----------- rules/no_dockerfile_image_latest.go | 14 +- rules/no_dockerfile_image_latest_test.go | 106 ++++++++++++ rules/pin_dockerfile_image_digest.go | 12 +- rules/pin_dockerfile_image_digest_test.go | 11 ++ 8 files changed, 547 insertions(+), 113 deletions(-) create mode 100644 rules/compose.go create mode 100644 rules/compose_test.go diff --git a/rules/compose.go b/rules/compose.go new file mode 100644 index 0000000..caf1309 --- /dev/null +++ b/rules/compose.go @@ -0,0 +1,179 @@ +package rules + +import ( + "path" + "strings" + + "github.com/bare-devcontainer/decolint/linter" + "github.com/tailscale/hujson" + "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 +} + +// 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 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 + 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) + default: + return nil + } + + if inline != "" { + return &composeBuild{inline: inline, target: target} + } + // 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} +} diff --git a/rules/compose_test.go b/rules/compose_test.go new file mode 100644 index 0000000..facd1b8 --- /dev/null +++ b/rules/compose_test.go @@ -0,0 +1,188 @@ +package rules + +import ( + "testing" + "testing/fstest" + + "github.com/bare-devcontainer/decolint/linter" +) + +// 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 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 && *got.build != *tt.wantBuild: + t.Errorf("build = %+v, want %+v", *got.build, *tt.wantBuild) + } + }) + } +} diff --git a/rules/dockerfile.go b/rules/dockerfile.go index 1259070..1ee2550 100644 --- a/rules/dockerfile.go +++ b/rules/dockerfile.go @@ -2,6 +2,7 @@ package rules import ( "bytes" + "fmt" "slices" "strconv" "strings" @@ -245,12 +246,20 @@ func stageNamed(stages []instructions.Stage, ref string) (int, bool) { 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) +// dockerfileBuildImages returns the images the build that obj, a devcontainer.json, declares pulls +// (see [dockerfilePulledImages]), along with the subject naming that build in a finding and the +// offset to anchor findings at. +// +// The build is the Dockerfile the configuration names itself, or, for a Compose-based configuration, +// the one the service it runs is built by. Compose is looked at first, as the reference +// implementation resolves the base image in that order. ok is false when the configuration builds +// nothing of its own, or when the Dockerfile cannot be read (see [readConfigFile]). +func dockerfileBuildImages(dir linter.Dir, obj *hujson.Object) (images []dockerfileImage, subject string, offset int, ok bool) { + if paths, composeOffset, declared := composeFilePaths(obj); declared { + return composeBuildImages(dir, obj, paths, composeOffset) + } + + path, offset, ok := dockerfileRef(obj) if !ok { return nil, "", 0, false } @@ -258,5 +267,28 @@ func dockerfileBuildImages(dir linter.Dir, obj *hujson.Object) (images []dockerf if !ok { return nil, "", 0, false } - return dockerfilePulledImages(src, buildTarget(obj)), path, offset, true + return dockerfilePulledImages(src, buildTarget(obj)), fmt.Sprintf("Dockerfile %q", path), offset, true +} + +// composeBuildImages returns the images the build of the Compose service the dev container runs +// pulls, named by that service. ok is false when the service runs an image instead of building one, +// which [NoComposeImageLatest] reports on. +func composeBuildImages(dir linter.Dir, obj *hujson.Object, paths []string, offset int) (images []dockerfileImage, subject string, anchor int, ok bool) { + service, ok := stringMember(obj, "service") + if !ok { + return nil, "", 0, false + } + source, ok := composeServiceSource(dir, paths, service) + if !ok || source.build == nil { + return nil, "", 0, false + } + + src := []byte(source.build.inline) + if source.build.dockerfile != "" { + src, ok = readConfigFile(dir, source.build.dockerfile) + if !ok { + return nil, "", 0, false + } + } + return dockerfilePulledImages(src, source.build.target), fmt.Sprintf("compose service %q", service), offset, true } diff --git a/rules/no_compose_image_latest.go b/rules/no_compose_image_latest.go index 7cfd25e..42d7e1b 100644 --- a/rules/no_compose_image_latest.go +++ b/rules/no_compose_image_latest.go @@ -2,11 +2,9 @@ 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 @@ -61,9 +59,10 @@ or with "latest", pulls whatever the publisher last released — a container tha }, }, 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.", + "published image; a service that builds its own image is covered by\n" + + "[`no-dockerfile-image-latest`](../no-dockerfile-image-latest/), which reads the\n" + + "Dockerfile its `build` names. An image written as a `${...}` variable is not checked,\n" + + "its value not being in the configuration.", }, Check: checkNoComposeImageLatest, } @@ -81,10 +80,11 @@ func checkNoComposeImageLatest(ctx *linter.Context, node *linter.Node) []linter. if !ok { return nil } - image, ok := composeServiceImage(ctx.Dir, paths, service) - if !ok { + source, ok := composeServiceSource(ctx.Dir, paths, service) + if !ok || source.image == "" { return nil } + image := source.image tag, hasTag := refTag(image) switch { @@ -101,93 +101,3 @@ func checkNoComposeImageLatest(ctx *linter.Context, node *linter.Node) []linter. } 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_dockerfile_image_latest.go b/rules/no_dockerfile_image_latest.go index 768d3f9..00487d9 100644 --- a/rules/no_dockerfile_image_latest.go +++ b/rules/no_dockerfile_image_latest.go @@ -19,7 +19,10 @@ to whatever the publisher last released. The container then changes from one reb 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.`, +brings into the container moves under an unpinned reference the same way, so those are named too. + +The Dockerfile is the one the configuration names, or, for a Compose-based configuration, the one the +service it runs is built by.`, References: []string{ `https://containers.dev/implementors/json_reference/#image-specific`, `https://containers.dev/implementors/spec/#dockerfile-based`, @@ -61,7 +64,8 @@ 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" + + "`build.args`. A Compose service that builds its own image is read the same way,\n" + + "through the Dockerfile its `build` names.\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.", }, @@ -73,7 +77,7 @@ func checkNoDockerfileImageLatest(ctx *linter.Context, node *linter.Node) []lint if !ok { return nil } - images, path, offset, ok := dockerfileBuildImages(ctx.Dir, obj) + images, subject, offset, ok := dockerfileBuildImages(ctx.Dir, obj) if !ok { return nil } @@ -84,12 +88,12 @@ func checkNoDockerfileImageLatest(ctx *linter.Context, node *linter.Node) []lint 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), + Message: fmt.Sprintf("%s %s image %q, which has no explicit tag; pin a specific version", subject, 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), + Message: fmt.Sprintf("%s %s image %q, which uses the \"latest\" tag; pin a specific version", subject, image.verb(), image.ref), Offset: offset, }) } diff --git a/rules/no_dockerfile_image_latest_test.go b/rules/no_dockerfile_image_latest_test.go index 2bac41b..47f7ba0 100644 --- a/rules/no_dockerfile_image_latest_test.go +++ b/rules/no_dockerfile_image_latest_test.go @@ -291,6 +291,112 @@ FROM golang:latest AS tools assertIssuesInDir(t, rules.NoDockerfileImageLatest, linter.SeverityError, "devcontainer.json", linter.Devcontainer, src, dir, want) } +// TestNoDockerfileImageLatest_ComposeBuild checks the Dockerfile a Compose service builds from, the +// other place a configuration names one. +func TestNoDockerfileImageLatest_ComposeBuild(t *testing.T) { + t.Parallel() + + 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-dockerfile-image-latest", Message: message}} + } + + tests := []struct { + name string + compose string + dockerfile string + want []linter.Issue + }{ + { + "a build names its Dockerfile", + "services:\n app:\n build:\n context: .\n dockerfile: Dockerfile\n", + "FROM ubuntu:latest\n", + issue(`compose service "app" builds from image "ubuntu:latest", which uses the "latest" tag; pin a specific version`), + }, + { + "a build written as its context alone defaults the Dockerfile", + "services:\n app:\n build: .\n", + "FROM ubuntu:latest\n", + issue(`compose service "app" builds from image "ubuntu:latest", which uses the "latest" tag; pin a specific version`), + }, + { + "an inline Dockerfile is read from the Compose file itself", + "services:\n app:\n build:\n dockerfile_inline: |\n FROM ubuntu:latest\n", + "FROM debian:24.04\n", + issue(`compose service "app" builds from image "ubuntu:latest", which uses the "latest" tag; pin a specific version`), + }, + { + "a build target leaves the stages it does not reach alone", + "services:\n app:\n build:\n context: .\n target: dev\n", + "FROM ubuntu:latest AS test\n\nFROM ubuntu:24.04 AS dev\n", + nil, + }, + { + "an image the Dockerfile pulls is reported too", + "services:\n app:\n build: .\n", + "FROM ubuntu:24.04\nCOPY --from=busybox:latest /bin/busybox /b\n", + issue(`compose service "app" pulls image "busybox:latest", which uses the "latest" tag; pin a specific version`), + }, + { + "a pinned Dockerfile reports nothing", + "services:\n app:\n build: .\n", + "FROM ubuntu:24.04\n", + nil, + }, + { + // The Compose rule reports the image such a service runs; there is no Dockerfile here. + "a service running an image reports nothing", + "services:\n app:\n image: ubuntu:latest\n", + "FROM ubuntu:latest\n", + nil, + }, + { + // The context leaves the directory the configuration is read through. + "a context outside the directory reports nothing", + "services:\n app:\n build:\n context: ..\n", + "FROM ubuntu:latest\n", + nil, + }, + { + "a missing Dockerfile reports nothing", + "services:\n app:\n build:\n context: .\n dockerfile: absent\n", + "FROM ubuntu:latest\n", + 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)}, + "Dockerfile": {Data: []byte(tt.dockerfile)}, + }} + assertIssuesInDir(t, rules.NoDockerfileImageLatest, linter.SeverityError, "devcontainer.json", linter.Devcontainer, src, dir, tt.want) + }) + } + + t.Run("a Compose configuration without a service reports nothing", func(t *testing.T) { + t.Parallel() + dir := linter.Dir{FS: fstest.MapFS{ + "docker-compose.yml": {Data: []byte("services:\n app:\n build: .\n")}, + "Dockerfile": {Data: []byte("FROM ubuntu:latest\n")}, + }} + assertIssuesInDir(t, rules.NoDockerfileImageLatest, linter.SeverityError, "devcontainer.json", linter.Devcontainer, `{"dockerComposeFile": "docker-compose.yml"}`, dir, nil) + }) + + t.Run("a Compose configuration is read instead of a build property beside it", func(t *testing.T) { + t.Parallel() + dir := linter.Dir{FS: fstest.MapFS{ + "docker-compose.yml": {Data: []byte("services:\n app:\n image: ubuntu:24.04\n")}, + "Dockerfile": {Data: []byte("FROM ubuntu:latest\n")}, + }} + // A configuration declaring both is reported by conflicting-container-def; the base image + // resolves through Compose, as the reference implementation resolves it. + src := `{"dockerComposeFile": "docker-compose.yml", "service": "app", "build": {"dockerfile": "Dockerfile"}}` + assertIssuesInDir(t, rules.NoDockerfileImageLatest, linter.SeverityError, "devcontainer.json", linter.Devcontainer, src, dir, nil) + }) +} + func TestNoDockerfileImageLatest_DockerfileLocation(t *testing.T) { t.Parallel() diff --git a/rules/pin_dockerfile_image_digest.go b/rules/pin_dockerfile_image_digest.go index 34e2a1d..a3bc522 100644 --- a/rules/pin_dockerfile_image_digest.go +++ b/rules/pin_dockerfile_image_digest.go @@ -20,7 +20,10 @@ different bits, so two builds of the same Dockerfile can start from different im Keeping the tag alongside the digest leaves the reference readable. An image a "COPY --from" or a "RUN --mount=from" names is pulled through the same mutable pointer, so -it takes a digest too.`, +it takes a digest too. + +The Dockerfile is the one the configuration names, or, for a Compose-based configuration, the one the +service it runs is built by.`, References: []string{ `https://containers.dev/implementors/spec/#dockerfile-based`, `https://github.com/opencontainers/image-spec/blob/main/descriptor.md#digests`, @@ -62,7 +65,8 @@ 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`.", + "`build.args`. A Compose service that builds its own image is read the same way,\n" + + "through the Dockerfile its `build` names.", }, Check: checkPinDockerfileImageDigest, } @@ -72,7 +76,7 @@ func checkPinDockerfileImageDigest(ctx *linter.Context, node *linter.Node) []lin if !ok { return nil } - images, path, offset, ok := dockerfileBuildImages(ctx.Dir, obj) + images, subject, offset, ok := dockerfileBuildImages(ctx.Dir, obj) if !ok { return nil } @@ -83,7 +87,7 @@ func checkPinDockerfileImageDigest(ctx *linter.Context, node *linter.Node) []lin continue } findings = append(findings, linter.Finding{ - Message: fmt.Sprintf("Dockerfile %q %s image %q, which is not pinned by digest; add an \"@sha256:...\" digest", path, image.verb(), image.ref), + Message: fmt.Sprintf("%s %s image %q, which is not pinned by digest; add an \"@sha256:...\" digest", subject, image.verb(), image.ref), Offset: offset, }) } diff --git a/rules/pin_dockerfile_image_digest_test.go b/rules/pin_dockerfile_image_digest_test.go index e1f8290..51c4dca 100644 --- a/rules/pin_dockerfile_image_digest_test.go +++ b/rules/pin_dockerfile_image_digest_test.go @@ -84,6 +84,17 @@ func TestPinDockerfileImageDigest(t *testing.T) { assertIssuesInDir(t, rules.PinDockerfileImageDigest, linter.SeverityError, "devcontainer.json", linter.Devcontainer, src, linter.Dir{FS: fstest.MapFS{}}, nil) }) + t.Run("the Dockerfile a Compose service builds from is read", func(t *testing.T) { + t.Parallel() + dir := linter.Dir{FS: fstest.MapFS{ + "docker-compose.yml": {Data: []byte("services:\n app:\n build: .\n")}, + "Dockerfile": {Data: []byte("FROM ubuntu:24.04\n")}, + }} + src := `{"dockerComposeFile": "docker-compose.yml", "service": "app"}` + want := []linter.Issue{{Path: "devcontainer.json", Line: 1, Col: 23, RuleID: "pin-dockerfile-image-digest", Message: `compose service "app" builds from image "ubuntu:24.04", which is not pinned by digest; add an "@sha256:..." digest`}} + assertIssuesInDir(t, rules.PinDockerfileImageDigest, linter.SeverityError, "devcontainer.json", linter.Devcontainer, src, dir, want) + }) + t.Run("a document that is not an object reports nothing", func(t *testing.T) { t.Parallel() dir := linter.Dir{FS: fstest.MapFS{"Dockerfile": {Data: []byte("FROM ubuntu\n")}}} From d4cdeea1cdc95a4bb541e87b165fa6dd7d48ae27 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 06:11:30 +0000 Subject: [PATCH 09/13] refactor(rules): let each Feature rule word its own finding unpinnedFeatureVersion returned half a sentence, leaving one finding's wording split between it and the rule that spliced the reference in front, and it reported a pinned version as the empty string. Nothing else in the package returns prose: the helpers return a value and whether they found one. The two rules now read the tag through refTag and word their findings themselves, as no-image-latest and no-dockerfile-image-latest already do for the same question. The messages are unchanged. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Y7ohBnPuPSezRvwrkzaRaA --- rules/pin_depends_on_version.go | 19 ++++++++++++------- rules/pin_feature_version.go | 19 ++++++++++++------- rules/util.go | 15 --------------- 3 files changed, 24 insertions(+), 29 deletions(-) diff --git a/rules/pin_depends_on_version.go b/rules/pin_depends_on_version.go index 9db8969..4a48df0 100644 --- a/rules/pin_depends_on_version.go +++ b/rules/pin_depends_on_version.go @@ -61,14 +61,19 @@ without the Feature's version changing.`, func checkPinDependsOnVersion(_ *linter.Context, node *linter.Node) []linter.Finding { var findings []linter.Finding for _, f := range ociFeatureRefs(node.Value) { - problem := unpinnedFeatureVersion(f.ref) - if problem == "" { - continue + tag, hasTag := refTag(f.ref) + switch { + case !hasTag: + findings = append(findings, linter.Finding{ + Message: fmt.Sprintf(`"dependsOn" feature %q has no explicit version; pin a specific version`, f.ref), + Offset: f.offset, + }) + case tag == "latest": + findings = append(findings, linter.Finding{ + Message: fmt.Sprintf(`"dependsOn" feature %q uses the "latest" version; pin a specific version`, f.ref), + Offset: f.offset, + }) } - findings = append(findings, linter.Finding{ - Message: fmt.Sprintf(`"dependsOn" feature %q %s`, f.ref, problem), - Offset: f.offset, - }) } return findings } diff --git a/rules/pin_feature_version.go b/rules/pin_feature_version.go index 4f3200c..e3785a7 100644 --- a/rules/pin_feature_version.go +++ b/rules/pin_feature_version.go @@ -55,14 +55,19 @@ devcontainer.json changing at all. Features are published under their full versi func checkPinFeatureVersion(_ *linter.Context, node *linter.Node) []linter.Finding { var findings []linter.Finding for _, f := range ociFeatureRefs(node.Value) { - problem := unpinnedFeatureVersion(f.ref) - if problem == "" { - continue + tag, hasTag := refTag(f.ref) + switch { + case !hasTag: + findings = append(findings, linter.Finding{ + Message: fmt.Sprintf("feature %q has no explicit version; pin a specific version", f.ref), + Offset: f.offset, + }) + case tag == "latest": + findings = append(findings, linter.Finding{ + Message: fmt.Sprintf("feature %q uses the \"latest\" version; pin a specific version", f.ref), + Offset: f.offset, + }) } - findings = append(findings, linter.Finding{ - Message: fmt.Sprintf("feature %q %s", f.ref, problem), - Offset: f.offset, - }) } return findings } diff --git a/rules/util.go b/rules/util.go index 3750ec4..ba0f24e 100644 --- a/rules/util.go +++ b/rules/util.go @@ -323,21 +323,6 @@ 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. From 3c425b8ebdc87dd9642af675100ff622e417d789 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 12:11:09 +0000 Subject: [PATCH 10/13] fix(rules): tell Feature reference forms apart as the spec defines them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rules classified a Feature reference with two ad-hoc predicates, so "OCI" was whatever was left over once a relative path and an HTTP(S) URI had been taken out. Two of the three forms were read wrongly as a result: the specification's tarball form is an HTTPS URI, and its local form is a relative path, so an absolute path is no Feature reference at all — yet one was reported as a Feature to pin a version on, as were a bare name and an upper-case registry, none of which name a Feature the tooling can resolve. feature.ParseRef already tells the three forms apart the way the specification defines them, and it is what resolves them for the merge. The rules now ask it for the references of the kind they can report on, so the classification is stated once and a reference that parses as none of the forms is left alone. The version a reference names is still read from the reference as written: ParseRef normalizes a missing version to the "latest" tag, which would collapse the two findings the rules distinguish. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Y7ohBnPuPSezRvwrkzaRaA --- rules/pin_depends_on_version.go | 3 +- rules/pin_depends_on_version_test.go | 5 +- rules/pin_feature_exact_version.go | 3 +- rules/pin_feature_exact_version_test.go | 7 ++- rules/pin_feature_version.go | 9 +-- rules/pin_feature_version_test.go | 7 ++- rules/util.go | 32 +++++----- rules/util_test.go | 77 +++++++++++++++++++++++++ 8 files changed, 115 insertions(+), 28 deletions(-) diff --git a/rules/pin_depends_on_version.go b/rules/pin_depends_on_version.go index 4a48df0..e719d8f 100644 --- a/rules/pin_depends_on_version.go +++ b/rules/pin_depends_on_version.go @@ -3,6 +3,7 @@ package rules import ( "fmt" + "github.com/bare-devcontainer/decolint/feature" "github.com/bare-devcontainer/decolint/linter" ) @@ -60,7 +61,7 @@ without the Feature's version changing.`, func checkPinDependsOnVersion(_ *linter.Context, node *linter.Node) []linter.Finding { var findings []linter.Finding - for _, f := range ociFeatureRefs(node.Value) { + for _, f := range featureRefsOfKind(node.Value, feature.KindOCI) { tag, hasTag := refTag(f.ref) switch { case !hasTag: diff --git a/rules/pin_depends_on_version_test.go b/rules/pin_depends_on_version_test.go index 82d6be6..eb0616e 100644 --- a/rules/pin_depends_on_version_test.go +++ b/rules/pin_depends_on_version_test.go @@ -25,10 +25,13 @@ func TestPinDependsOnVersion(t *testing.T) { {Path: path, Line: 1, Col: 16, RuleID: "pin-depends-on-version", Message: `"dependsOn" feature "ghcr.io/devcontainers/features/node:latest" uses the "latest" version; pin a specific version`}, }}, {"pinned version", `{"dependsOn": {"ghcr.io/devcontainers/features/node:1": {}}}`, nil}, - {"pinned digest", `{"dependsOn": {"ghcr.io/devcontainers/features/node@sha256:abc123": {}}}`, nil}, + {"pinned digest", `{"dependsOn": {"ghcr.io/devcontainers/features/node@sha256:0000000000000000000000000000000000000000000000000000000000000000": {}}}`, nil}, {"local path dependency", `{"dependsOn": {"./local-feature": {}}}`, nil}, {"tarball uri dependency", `{"dependsOn": {"https://example.invalid/devcontainer-feature.tgz": {}}}`, nil}, {"non-object dependsOn", `{"dependsOn": "invalid"}`, nil}, + // A reference in none of the three forms the specification defines names no Feature. + {"absolute path dependency", `{"dependsOn": {"/absolute/feature": {}}}`, nil}, + {"dependency with no registry", `{"dependsOn": {"no-slash": {}}}`, nil}, {"installsAfter is not checked", `{"installsAfter": ["ghcr.io/devcontainers/features/node"]}`, nil}, {"multiple dependencies mixed", `{"dependsOn": { "ghcr.io/devcontainers/features/node:1.6.0": {}, diff --git a/rules/pin_feature_exact_version.go b/rules/pin_feature_exact_version.go index 5fc39b7..9cdc310 100644 --- a/rules/pin_feature_exact_version.go +++ b/rules/pin_feature_exact_version.go @@ -5,6 +5,7 @@ import ( "regexp" "strings" + "github.com/bare-devcontainer/decolint/feature" "github.com/bare-devcontainer/decolint/linter" ) @@ -69,7 +70,7 @@ func checkPinFeatureExactVersion(ctx *linter.Context, node *linter.Node) []linte } var findings []linter.Finding - for _, f := range ociFeatureRefs(node.Value) { + for _, f := range featureRefsOfKind(node.Value, feature.KindOCI) { // A digest names the content itself, whatever tag it is written alongside. if strings.Contains(f.ref, "@") { continue diff --git a/rules/pin_feature_exact_version_test.go b/rules/pin_feature_exact_version_test.go index e55a6fa..7fcbbf1 100644 --- a/rules/pin_feature_exact_version_test.go +++ b/rules/pin_feature_exact_version_test.go @@ -54,11 +54,14 @@ func TestPinFeatureExactVersion(t *testing.T) { `{"features": {"ghcr.io/devcontainers/features/go:v1.3.2": {}}}`, issue(`feature "ghcr.io/devcontainers/features/go:v1.3.2" uses version "v1.3.2"; pin a full "major.minor.patch" version`), }, - {"digest alone", `{"features": {"ghcr.io/devcontainers/features/go@sha256:abc123": {}}}`, nil}, - {"partial version alongside a digest", `{"features": {"ghcr.io/devcontainers/features/go:1@sha256:abc123": {}}}`, nil}, + {"digest alone", `{"features": {"ghcr.io/devcontainers/features/go@sha256:0000000000000000000000000000000000000000000000000000000000000000": {}}}`, nil}, + {"partial version alongside a digest", `{"features": {"ghcr.io/devcontainers/features/go:1@sha256:0000000000000000000000000000000000000000000000000000000000000000": {}}}`, nil}, {"local path feature", `{"features": {"./local-feature": {}}}`, nil}, {"tarball uri feature", `{"features": {"https://example.invalid/devcontainer-feature.tgz": {}}}`, nil}, {"non-object features", `{"features": "invalid"}`, nil}, + // A reference in none of the three forms the specification defines names no Feature. + {"absolute path", `{"features": {"/absolute/feature": {}}}`, nil}, + {"no registry", `{"features": {"no-slash": {}}}`, nil}, { "registry port without a version", `{"features": {"localhost:5000/features/foo": {}}}`, diff --git a/rules/pin_feature_version.go b/rules/pin_feature_version.go index e3785a7..462cd80 100644 --- a/rules/pin_feature_version.go +++ b/rules/pin_feature_version.go @@ -3,14 +3,15 @@ package rules import ( "fmt" + "github.com/bare-devcontainer/decolint/feature" "github.com/bare-devcontainer/decolint/linter" ) // PinFeatureVersion reports a "features" entry whose key references an OCI Feature without an // explicit version tag or with the "latest" tag. Such references are not reproducible: the Feature -// they resolve to changes over time. Local path Features (e.g. "./my-feature") and direct tarball -// URIs (e.g. "https://.../devcontainer-feature.tgz") have no version tag to pin and are not -// checked. +// they resolve to changes over time. The other two forms the specification defines — a relative path +// (e.g. "./my-feature") and a direct HTTPS tarball URI — carry no version to pin and are not +// checked; see [featureRefsOfKind]. var PinFeatureVersion = &linter.Rule{ ID: "pin-feature-version", Description: `disallow a Feature reference without an explicit version or with the "latest" version`, @@ -54,7 +55,7 @@ devcontainer.json changing at all. Features are published under their full versi func checkPinFeatureVersion(_ *linter.Context, node *linter.Node) []linter.Finding { var findings []linter.Finding - for _, f := range ociFeatureRefs(node.Value) { + for _, f := range featureRefsOfKind(node.Value, feature.KindOCI) { tag, hasTag := refTag(f.ref) switch { case !hasTag: diff --git a/rules/pin_feature_version_test.go b/rules/pin_feature_version_test.go index 9f96d4b..01e1dcc 100644 --- a/rules/pin_feature_version_test.go +++ b/rules/pin_feature_version_test.go @@ -23,9 +23,14 @@ func TestPinFeatureVersion(t *testing.T) { {Path: "devcontainer.json", Line: 1, Col: 15, RuleID: "pin-feature-version", Message: `feature "ghcr.io/devcontainers/features/node:latest" uses the "latest" version; pin a specific version`}, }}, {"pinned version", `{"features": {"ghcr.io/devcontainers/features/node:1": {}}}`, nil}, - {"pinned digest", `{"features": {"ghcr.io/devcontainers/features/node@sha256:abc123": {}}}`, nil}, + {"pinned digest", `{"features": {"ghcr.io/devcontainers/features/node@sha256:0000000000000000000000000000000000000000000000000000000000000000": {}}}`, nil}, {"local path feature", `{"features": {"./local-feature": {}}}`, nil}, {"tarball uri feature", `{"features": {"https://example.com/devcontainer-feature.tgz": {}}}`, nil}, + // A reference in none of the three forms the specification defines names no Feature, so + // there is no version to pin in it. + {"absolute path", `{"features": {"/absolute/feature": {}}}`, nil}, + {"no registry", `{"features": {"no-slash": {}}}`, nil}, + {"upper-case registry", `{"features": {"GHCR.IO/UPPER/CASE": {}}}`, nil}, {"registry port without tag", `{"features": {"localhost:5000/features/foo": {}}}`, []linter.Issue{ {Path: "devcontainer.json", Line: 1, Col: 15, RuleID: "pin-feature-version", Message: `feature "localhost:5000/features/foo" has no explicit version; pin a specific version`}, }}, diff --git a/rules/util.go b/rules/util.go index ba0f24e..e9a1e53 100644 --- a/rules/util.go +++ b/rules/util.go @@ -9,6 +9,7 @@ import ( "strings" "github.com/bare-devcontainer/decolint/dockerargs" + "github.com/bare-devcontainer/decolint/feature" "github.com/bare-devcontainer/decolint/linter" "github.com/tailscale/hujson" ) @@ -286,12 +287,18 @@ type featureRef struct { 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. +// featureRefsOfKind returns the Feature references of the given kind 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 { +// References are told apart by [feature.ParseRef], which is what resolves them for the merge, so a +// rule reads the three forms the specification defines and the reference implementation accepts. One +// that parses as none of them names no Feature to report on, and is left out along with the kinds +// not asked for. +// +// A caller reading the version a reference names takes it from the reference as written, not from +// the parsed [feature.Ref]: ParseRef normalizes a reference with no version to the "latest" tag, and +// a rule telling those two apart would lose the distinction. +func featureRefsOfKind(v *hujson.Value, kind feature.RefKind) []featureRef { obj, ok := v.Value.(*hujson.Object) if !ok { return nil @@ -303,7 +310,8 @@ func ociFeatureRefs(v *hujson.Value) []featureRef { continue } ref := name.String() - if isLocalFeature(ref) || isTarballFeature(ref) { + parsed, err := feature.ParseRef(ref) + if err != nil || parsed.Kind != kind { continue } refs = append(refs, featureRef{ref: ref, offset: m.Name.StartOffset}) @@ -311,18 +319,6 @@ func ociFeatureRefs(v *hujson.Value) []featureRef { 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://") -} - // 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 index 9ec7d25..016cce0 100644 --- a/rules/util_test.go +++ b/rules/util_test.go @@ -1,13 +1,90 @@ package rules import ( + "slices" "strings" "testing" "testing/fstest" + "github.com/bare-devcontainer/decolint/feature" "github.com/bare-devcontainer/decolint/linter" + "github.com/tailscale/hujson" ) +// TestFeatureRefsOfKind covers each form the specification defines, and the references that are none +// of them: a rule asks for the kind it can report on and must be handed nothing else. +func TestFeatureRefsOfKind(t *testing.T) { + t.Parallel() + + // One object carrying every form, so each case sees the ones it must leave behind. + const src = `{ + "ghcr.io/devcontainers/features/go:1.3.2": {}, + "localhost:5000/features/foo": {}, + "./local-feature": {}, + "../sibling-feature": {}, + "https://example.invalid/devcontainer-feature.tgz": {}, + "http://example.invalid/devcontainer-feature.tgz": {}, + "/absolute/feature": {}, + "no-slash": {}, + "GHCR.IO/UPPER/CASE": {} +}` + + tests := []struct { + name string + kind feature.RefKind + want []string + }{ + { + // An absolute path, a bare name and an upper-case registry are none of the three forms: + // the specification's local form is a relative path, and an OCI reference needs a + // registry it can be fetched from. + name: "OCI", + kind: feature.KindOCI, + want: []string{"ghcr.io/devcontainers/features/go:1.3.2", "localhost:5000/features/foo"}, + }, + { + name: "local", + kind: feature.KindLocal, + want: []string{"./local-feature", "../sibling-feature"}, + }, + { + // The tarball form is an HTTPS URI; the "http://" spelling is not one. + name: "tarball", + kind: feature.KindTarball, + want: []string{"https://example.invalid/devcontainer-feature.tgz"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + value, err := hujson.Parse([]byte(src)) + if err != nil { + t.Fatalf("parse: %v", err) + } + var got []string + for _, ref := range featureRefsOfKind(&value, tt.kind) { + got = append(got, ref.ref) + } + if !slices.Equal(got, tt.want) { + t.Errorf("featureRefsOfKind(%v) = %q, want %q", tt.kind, got, tt.want) + } + }) + } + + t.Run("a value that is not an object", func(t *testing.T) { + t.Parallel() + + value, err := hujson.Parse([]byte(`"not an object"`)) + if err != nil { + t.Fatalf("parse: %v", err) + } + if got := featureRefsOfKind(&value, feature.KindOCI); got != nil { + t.Errorf("featureRefsOfKind = %v, want none", got) + } + }) +} + // 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) { From 23bf029e027dfcad5ec572a6ba782930395ceda0 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 21:56:06 +0000 Subject: [PATCH 11/13] refactor(rules): report every unpinned image from one rule per judgment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The image rules were split by where the image is named — the "image" property, a Dockerfile, a Compose service — but that is not a split anyone configures against: a project that forbids "latest" forbids it wherever it is written. Splitting that way also let a judgment go missing per place without anyone noticing, and one had: a Compose service's image was checked for the "latest" tag and never for a digest, so "image: ubuntu:24.04" passed both rules. no-image-latest and pin-image-digest now read every image a container of the configuration pulls, and the three rules added for the other places are gone. Each finding names where the image is written and is reported at the property that names it, so what was said by the rule ID is now said by the message: image "ubuntu:latest" uses the "latest" tag; ... Dockerfile "Dockerfile": image "ubuntu:latest" uses the "latest" tag; ... compose service "app": image "ubuntu:latest" uses the "latest" tag; ... The two surviving rules keep their existing message for the "image" property, so a configuration that named its image there reports exactly what it did before. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Y7ohBnPuPSezRvwrkzaRaA --- README.md | 2 +- rules/dockerfile.go | 75 +----------- rules/images.go | 97 +++++++++++++++ rules/no_compose_image_latest.go | 103 ---------------- rules/no_dockerfile_image_latest.go | 102 ---------------- rules/no_image_latest.go | 53 ++++---- ...est.go => no_image_latest_compose_test.go} | 28 +++-- ....go => no_image_latest_dockerfile_test.go} | 113 ++++++++++-------- rules/pin_dockerfile_image_digest.go | 95 --------------- rules/pin_image_digest.go | 43 ++++--- ...go => pin_image_digest_dockerfile_test.go} | 31 ++--- rules/rules.go | 3 - 12 files changed, 251 insertions(+), 494 deletions(-) create mode 100644 rules/images.go delete mode 100644 rules/no_compose_image_latest.go delete mode 100644 rules/no_dockerfile_image_latest.go rename rules/{no_compose_image_latest_test.go => no_image_latest_compose_test.go} (78%) rename rules/{no_dockerfile_image_latest_test.go => no_image_latest_dockerfile_test.go} (68%) delete mode 100644 rules/pin_dockerfile_image_digest.go rename rules/{pin_dockerfile_image_digest_test.go => pin_image_digest_dockerfile_test.go} (56%) diff --git a/README.md b/README.md index a378aec..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` | 9 | +| [`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/rules/dockerfile.go b/rules/dockerfile.go index 1ee2550..e42fcd7 100644 --- a/rules/dockerfile.go +++ b/rules/dockerfile.go @@ -2,12 +2,10 @@ package rules import ( "bytes" - "fmt" "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" @@ -54,24 +52,6 @@ func buildTarget(obj *hujson.Object) string { 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 @@ -83,7 +63,7 @@ func (img dockerfileImage) verb() string { // // 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 { +func dockerfilePulledImages(src []byte, target string) []string { result, err := parser.Parse(bytes.NewReader(src)) if err != nil { return nil @@ -97,17 +77,17 @@ func dockerfilePulledImages(src []byte, target string) []dockerfileImage { } built := builtStages(stages, target) - var images []dockerfileImage + var images []string 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}) + images = append(images, stages[i].BaseName) } for _, from := range stageFroms(stages, i) { if from.stage < 0 && isPulledImage(from.ref) { - images = append(images, dockerfileImage{ref: from.ref}) + images = append(images, from.ref) } } } @@ -245,50 +225,3 @@ func stageNamed(stages []instructions.Stage, ref string) (int, bool) { } return 0, false } - -// dockerfileBuildImages returns the images the build that obj, a devcontainer.json, declares pulls -// (see [dockerfilePulledImages]), along with the subject naming that build in a finding and the -// offset to anchor findings at. -// -// The build is the Dockerfile the configuration names itself, or, for a Compose-based configuration, -// the one the service it runs is built by. Compose is looked at first, as the reference -// implementation resolves the base image in that order. ok is false when the configuration builds -// nothing of its own, or when the Dockerfile cannot be read (see [readConfigFile]). -func dockerfileBuildImages(dir linter.Dir, obj *hujson.Object) (images []dockerfileImage, subject string, offset int, ok bool) { - if paths, composeOffset, declared := composeFilePaths(obj); declared { - return composeBuildImages(dir, obj, paths, composeOffset) - } - - 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)), fmt.Sprintf("Dockerfile %q", path), offset, true -} - -// composeBuildImages returns the images the build of the Compose service the dev container runs -// pulls, named by that service. ok is false when the service runs an image instead of building one, -// which [NoComposeImageLatest] reports on. -func composeBuildImages(dir linter.Dir, obj *hujson.Object, paths []string, offset int) (images []dockerfileImage, subject string, anchor int, ok bool) { - service, ok := stringMember(obj, "service") - if !ok { - return nil, "", 0, false - } - source, ok := composeServiceSource(dir, paths, service) - if !ok || source.build == nil { - return nil, "", 0, false - } - - src := []byte(source.build.inline) - if source.build.dockerfile != "" { - src, ok = readConfigFile(dir, source.build.dockerfile) - if !ok { - return nil, "", 0, false - } - } - return dockerfilePulledImages(src, source.build.target), fmt.Sprintf("compose service %q", service), offset, true -} diff --git a/rules/images.go b/rules/images.go new file mode 100644 index 0000000..1249667 --- /dev/null +++ b/rules/images.go @@ -0,0 +1,97 @@ +package rules + +import ( + "fmt" + + "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, in the order the +// configuration reaches them: +// - the one "image" names; +// - for a Compose-based configuration, the image its service runs, or the ones the Dockerfile that +// service builds from pulls; +// - otherwise the ones the Dockerfile the configuration itself names pulls. +// +// Compose is read before the configuration's own "dockerFile"/"build.dockerfile", as the reference +// implementation resolves the base image in that order; a configuration declaring both 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 + if m := memberNamed(obj, "image"); m != nil { + if lit, ok := m.Value.Value.(hujson.Literal); ok && lit.Kind() == '"' { + images = append(images, pulledImage{ref: lit.String(), offset: m.Value.StartOffset}) + } + } + if paths, offset, declared := composeFilePaths(obj); declared { + return append(images, composeImages(dir, obj, paths, offset)...) + } + return append(images, dockerfileImages(dir, obj)...) +} + +// dockerfileImages returns the images the Dockerfile obj names pulls, anchored at the property +// naming it. +func dockerfileImages(dir linter.Dir, obj *hujson.Object) []pulledImage { + path, offset, ok := dockerfileRef(obj) + if !ok { + return nil + } + src, ok := readConfigFile(dir, path) + if !ok { + return nil + } + return locate(dockerfilePulledImages(src, buildTarget(obj)), fmt.Sprintf("Dockerfile %q: ", path), offset) +} + +// 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, obj *hujson.Object, paths []string, offset int) []pulledImage { + service, ok := stringMember(obj, "service") + if !ok { + return nil + } + source, ok := composeServiceSource(dir, paths, 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.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_compose_image_latest.go b/rules/no_compose_image_latest.go deleted file mode 100644 index 42d7e1b..0000000 --- a/rules/no_compose_image_latest.go +++ /dev/null @@ -1,103 +0,0 @@ -package rules - -import ( - "fmt" - - "github.com/bare-devcontainer/decolint/linter" - "github.com/tailscale/hujson" -) - -// 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; a service that builds its own image is covered by\n" + - "[`no-dockerfile-image-latest`](../no-dockerfile-image-latest/), which reads the\n" + - "Dockerfile its `build` names. An image written as a `${...}` variable is not checked,\n" + - "its value not being in the 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 - } - source, ok := composeServiceSource(ctx.Dir, paths, service) - if !ok || source.image == "" { - return nil - } - image := source.image - - 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 -} diff --git a/rules/no_dockerfile_image_latest.go b/rules/no_dockerfile_image_latest.go deleted file mode 100644 index 00487d9..0000000 --- a/rules/no_dockerfile_image_latest.go +++ /dev/null @@ -1,102 +0,0 @@ -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. - -The Dockerfile is the one the configuration names, or, for a Compose-based configuration, the one the -service it runs is built by.`, - 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`. A Compose service that builds its own image is read the same way,\n" + - "through the Dockerfile its `build` names.\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, subject, 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("%s %s image %q, which has no explicit tag; pin a specific version", subject, image.verb(), image.ref), - Offset: offset, - }) - case tag == "latest": - findings = append(findings, linter.Finding{ - Message: fmt.Sprintf("%s %s image %q, which uses the \"latest\" tag; pin a specific version", subject, image.verb(), image.ref), - Offset: offset, - }) - } - } - return findings -} diff --git a/rules/no_image_latest.go b/rules/no_image_latest.go index bf39586..8641070 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,33 @@ the version the project was tested against.`, `}, }, }, + Note: "An image written with a `$` or `${...}` variable is not checked: its value comes from\n" + + "the environment or from `build.args`, not from the configuration.", }, 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_compose_image_latest_test.go b/rules/no_image_latest_compose_test.go similarity index 78% rename from rules/no_compose_image_latest_test.go rename to rules/no_image_latest_compose_test.go index c202263..db51f75 100644 --- a/rules/no_compose_image_latest_test.go +++ b/rules/no_image_latest_compose_test.go @@ -8,14 +8,14 @@ import ( "github.com/bare-devcontainer/decolint/rules" ) -func TestNoComposeImageLatest(t *testing.T) { +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-compose-image-latest", Message: message}} + return []linter.Issue{{Path: "devcontainer.json", Line: 1, Col: 23, RuleID: "no-image-latest", Message: message}} } tests := []struct { @@ -26,12 +26,12 @@ func TestNoComposeImageLatest(t *testing.T) { { "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`), + 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" runs image "ubuntu:latest", which uses the "latest" tag; pin a specific version`), + 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}, @@ -80,12 +80,12 @@ func TestNoComposeImageLatest(t *testing.T) { 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) + assertIssuesInDir(t, rules.NoImageLatest, linter.SeverityError, "devcontainer.json", linter.Devcontainer, src, dir, tt.want) }) } } -func TestNoComposeImageLatest_ComposeFileList(t *testing.T) { +func TestNoImageLatest_Compose_ComposeFileList(t *testing.T) { t.Parallel() dir := linter.Dir{FS: fstest.MapFS{ @@ -108,14 +108,18 @@ func TestNoComposeImageLatest_ComposeFileList(t *testing.T) { { "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`}}, + []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-compose-image-latest", Message: `compose service "app" runs image "ubuntu:latest", which uses the "latest" tag; pin a specific version`}}, + []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 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}, @@ -135,18 +139,18 @@ func TestNoComposeImageLatest_ComposeFileList(t *testing.T) { 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) + 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.NoComposeImageLatest, linter.SeverityError, "devcontainer.json", linter.Devcontainer, src, linter.Dir{FS: errFS{}}, nil) + 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.NoComposeImageLatest, linter.SeverityError, `{"dockerComposeFile": "docker-compose.yml", "service": "app"}`, nil) + assertIssues(t, rules.NoImageLatest, linter.SeverityError, `{"dockerComposeFile": "docker-compose.yml", "service": "app"}`, nil) }) } diff --git a/rules/no_dockerfile_image_latest_test.go b/rules/no_image_latest_dockerfile_test.go similarity index 68% rename from rules/no_dockerfile_image_latest_test.go rename to rules/no_image_latest_dockerfile_test.go index 47f7ba0..0cb1e69 100644 --- a/rules/no_dockerfile_image_latest_test.go +++ b/rules/no_image_latest_dockerfile_test.go @@ -8,14 +8,14 @@ import ( "github.com/bare-devcontainer/decolint/rules" ) -func TestNoDockerfileImageLatest(t *testing.T) { +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-dockerfile-image-latest", Message: message}} + return []linter.Issue{{Path: "devcontainer.json", Line: 1, Col: 26, RuleID: "no-image-latest", Message: message}} } tests := []struct { @@ -26,12 +26,12 @@ func TestNoDockerfileImageLatest(t *testing.T) { { "untagged base image", "FROM ubuntu\n", - issue(`Dockerfile "Dockerfile" builds from image "ubuntu", which has no explicit tag; pin a specific version`), + issue(`Dockerfile "Dockerfile": image "ubuntu" 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`), + 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}, @@ -58,14 +58,14 @@ func TestNoDockerfileImageLatest(t *testing.T) { // 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`), + 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" builds from image "0", which has no explicit tag; pin a specific version`), + issue(`Dockerfile "Dockerfile": image "0" has no explicit tag; pin a specific version`), }, { "an image reached through a variable is not resolved", @@ -76,14 +76,14 @@ func TestNoDockerfileImageLatest(t *testing.T) { "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`}, + {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" builds from image "ubuntu:latest", which uses the "latest" tag; pin a specific version`), + 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 < Date: Fri, 7 Aug 2026 23:24:09 +0000 Subject: [PATCH 12/13] refactor: read a container definition in one place MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reading what a devcontainer.json declares its container is made from was written twice: the merge resolved "dockerFile"/"build.dockerfile", "build.target", "dockerComposeFile" and "service" to fetch what they name, and the rules read the same properties to lint them. The two copies had already drifted — the same "dockerComposeFile" was read as a declaration with no usable path by one and as no declaration at all by the other — and a property this project reads by specification is exactly what should not be stated twice. The new containerdef package reads the declaration and nothing else, returning each value with the offsets of both its key and its value, so the merge can anchor at the key as it did and a rule at the value as it did. Resolving the declaration stays where it was: the merge fetches through compose-go and BuildKit, and the rules read what the linted directory holds and stay silent on what they cannot settle. Those two answer different questions and are not merged here. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Y7ohBnPuPSezRvwrkzaRaA --- containerdef/containerdef.go | 150 ++++++++++++++++++++++++ containerdef/containerdef_test.go | 183 ++++++++++++++++++++++++++++++ feature/compose.go | 48 +------- feature/merge.go | 73 ++---------- rules/compose.go | 29 ----- rules/dockerfile.go | 41 ------- rules/images.go | 18 +-- 7 files changed, 354 insertions(+), 188 deletions(-) create mode 100644 containerdef/containerdef.go create mode 100644 containerdef/containerdef_test.go diff --git a/containerdef/containerdef.go b/containerdef/containerdef.go new file mode 100644 index 0000000..3c79913 --- /dev/null +++ b/containerdef/containerdef.go @@ -0,0 +1,150 @@ +// Package containerdef reads what a devcontainer.json declares its container is made from: an image +// it pulls, a Dockerfile it builds, or a Compose service it attaches to. It reads the declaration +// 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. +// +// Every reader returns the byte offsets of both the key and the value, so a caller can anchor at +// whichever the reader of its output expects to see. +package containerdef + +import "github.com/tailscale/hujson" + +// Decl is a property's value as declared, with where it is written. +type Decl struct { + // KeyOffset is the byte offset of the property name. + KeyOffset int + // ValueOffset is the byte offset of the value. + ValueOffset int +} + +// Image returns the image "image" names. ok is false when the property is absent or is not a string. +func Image(obj *hujson.Object) (ref string, decl Decl, ok bool) { + m := memberNamed(obj, "image") + if m == nil { + return "", Decl{}, false + } + lit, isLit := m.Value.Value.(hujson.Literal) + if !isLit || lit.Kind() != '"' { + return "", Decl{}, false + } + return lit.String(), declOf(m), true +} + +// Dockerfile returns the Dockerfile path the configuration builds from. The specification defines +// two mutually exclusive forms, the top-level "dockerFile" and the nested "build.dockerfile"; the +// top-level one wins, as the reference implementation prefers it (getDockerfile: 'dockerFile' in +// config ? config.dockerFile : config.build.dockerfile). ok is false when neither names one. +func Dockerfile(obj *hujson.Object) (path string, decl Decl, ok bool) { + if m := memberNamed(obj, "dockerFile"); m != nil { + if lit, isLit := m.Value.Value.(hujson.Literal); isLit && lit.Kind() == '"' { + return lit.String(), declOf(m), 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(), declOf(m), true + } + } + } + return "", Decl{}, false +} + +// BuildOptions returns the "build" options that shape what the Dockerfile produces: the arguments +// passed to the build, and the stage it stops at. Both are zero when "build" declares none. +func BuildOptions(obj *hujson.Object) (args map[string]string, target string) { + build := buildObject(obj) + if build == nil { + return nil, "" + } + if m := memberNamed(build, "args"); m != nil { + if argsObj, isObj := m.Value.Value.(*hujson.Object); isObj { + for _, arg := range argsObj.Members { + name, nameOK := arg.Name.Value.(hujson.Literal) + value, valueOK := arg.Value.Value.(hujson.Literal) + if !nameOK || name.Kind() != '"' || !valueOK || value.Kind() != '"' { + continue + } + if args == nil { + args = map[string]string{} + } + args[name.String()] = value.String() + } + } + } + if m := memberNamed(build, "target"); m != nil { + if lit, isLit := m.Value.Value.(hujson.Literal); isLit && lit.Kind() == '"' { + target = lit.String() + } + } + return args, target +} + +// ComposeFiles returns the Compose file paths "dockerComposeFile" names, in the order they are +// declared, later ones overriding earlier ones. The property is a single path or an array of them. +// +// declared reports whether the property is there at all, which is what tells a Compose-based +// configuration from one that builds or pulls an image. A declaration whose value, or whose element, +// is not a string contributes no path, so declared can be true with no paths: the configuration says +// it is Compose-based while naming nothing readable. +func ComposeFiles(obj *hujson.Object) (paths []string, decl Decl, declared bool) { + m := memberNamed(obj, "dockerComposeFile") + if m == nil { + return nil, Decl{}, false + } + switch v := m.Value.Value.(type) { + case hujson.Literal: + if v.Kind() == '"' { + paths = []string{v.String()} + } + case *hujson.Array: + for _, e := range v.Elements { + if lit, isLit := e.Value.(hujson.Literal); isLit && lit.Kind() == '"' { + paths = append(paths, lit.String()) + } + } + } + return paths, declOf(m), true +} + +// ComposeService returns the Compose service "service" names, the one the dev container runs in. ok +// is false when the property is absent or is not a string. +func ComposeService(obj *hujson.Object) (name string, decl Decl, ok bool) { + m := memberNamed(obj, "service") + if m == nil { + return "", Decl{}, false + } + lit, isLit := m.Value.Value.(hujson.Literal) + if !isLit || lit.Kind() != '"' { + return "", Decl{}, false + } + return lit.String(), declOf(m), true +} + +// buildObject returns the "build" object, or nil when the configuration declares none or declares it +// as something other than an object. +func buildObject(obj *hujson.Object) *hujson.Object { + m := memberNamed(obj, "build") + if m == nil { + return nil + } + build, ok := m.Value.Value.(*hujson.Object) + if !ok { + return nil + } + return build +} + +// memberNamed returns obj's member named name, or nil if obj has no such member. +func memberNamed(obj *hujson.Object, name string) *hujson.ObjectMember { + for i := range obj.Members { + if lit, ok := obj.Members[i].Name.Value.(hujson.Literal); ok && lit.String() == name { + return &obj.Members[i] + } + } + return nil +} + +func declOf(m *hujson.ObjectMember) Decl { + return Decl{KeyOffset: m.Name.StartOffset, ValueOffset: m.Value.StartOffset} +} diff --git a/containerdef/containerdef_test.go b/containerdef/containerdef_test.go new file mode 100644 index 0000000..7ae38bd --- /dev/null +++ b/containerdef/containerdef_test.go @@ -0,0 +1,183 @@ +package containerdef_test + +import ( + "testing" + + "github.com/bare-devcontainer/decolint/containerdef" + "github.com/google/go-cmp/cmp" + "github.com/tailscale/hujson" +) + +// object parses src and returns its root object, failing the test if it is not one. +func object(t *testing.T, src string) *hujson.Object { + t.Helper() + + value, err := hujson.Parse([]byte(src)) + if err != nil { + t.Fatalf("parse: %v", err) + } + obj, ok := value.Value.(*hujson.Object) + if !ok { + t.Fatalf("parsed %s, want an object", src) + } + return obj +} + +func TestImage(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + src string + want string + ok bool + }{ + {"a named image", `{"image": "ubuntu:24.04"}`, "ubuntu:24.04", true}, + {"no image", `{"name": "x"}`, "", false}, + {"a non-string image", `{"image": 42}`, "", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got, _, ok := containerdef.Image(object(t, tt.src)) + if got != tt.want || ok != tt.ok { + t.Errorf("Image = (%q, %v), want (%q, %v)", got, ok, tt.want, tt.ok) + } + }) + } +} + +func TestDockerfile(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + src string + want string + ok bool + }{ + {"build.dockerfile", `{"build": {"dockerfile": "Dockerfile"}}`, "Dockerfile", true}, + {"the legacy top-level property", `{"dockerFile": "Dockerfile"}`, "Dockerfile", true}, + { + // The reference implementation reads the top-level property first, so it is the one + // built when a configuration carries both. + "the top-level property wins", + `{"dockerFile": "top", "build": {"dockerfile": "nested"}}`, + "top", true, + }, + {"neither", `{"image": "ubuntu:24.04"}`, "", false}, + {"a non-object build", `{"build": "Dockerfile"}`, "", false}, + {"a non-string dockerfile", `{"build": {"dockerfile": 42}}`, "", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got, _, ok := containerdef.Dockerfile(object(t, tt.src)) + if got != tt.want || ok != tt.ok { + t.Errorf("Dockerfile = (%q, %v), want (%q, %v)", got, ok, tt.want, tt.ok) + } + }) + } +} + +func TestDockerfile_Offsets(t *testing.T) { + t.Parallel() + + // `{"build": {"dockerfile": "Dockerfile"}}` — the key opens at 11 and the value at 25. + _, decl, ok := containerdef.Dockerfile(object(t, `{"build": {"dockerfile": "Dockerfile"}}`)) + if !ok { + t.Fatal("Dockerfile: not found") + } + if decl.KeyOffset != 11 || decl.ValueOffset != 25 { + t.Errorf("decl = %+v, want {KeyOffset:11 ValueOffset:25}", decl) + } +} + +func TestBuildOptions(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + src string + wantArgs map[string]string + wantTgt string + }{ + {"args and target", `{"build": {"args": {"A": "1"}, "target": "dev"}}`, map[string]string{"A": "1"}, "dev"}, + {"neither", `{"build": {"dockerfile": "Dockerfile"}}`, nil, ""}, + {"no build", `{"image": "ubuntu:24.04"}`, nil, ""}, + {"a non-object build", `{"build": 42}`, nil, ""}, + {"a non-string arg is left out", `{"build": {"args": {"A": 1, "B": "2"}}}`, map[string]string{"B": "2"}, ""}, + {"a non-string target is no target", `{"build": {"target": 42}}`, nil, ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + args, target := containerdef.BuildOptions(object(t, tt.src)) + if diff := cmp.Diff(tt.wantArgs, args); diff != "" { + t.Errorf("args mismatch (-want +got):\n%s", diff) + } + if target != tt.wantTgt { + t.Errorf("target = %q, want %q", target, tt.wantTgt) + } + }) + } +} + +func TestComposeFiles(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + src string + want []string + wantDeclared bool + }{ + {"a single path", `{"dockerComposeFile": "docker-compose.yml"}`, []string{"docker-compose.yml"}, true}, + {"a list of paths", `{"dockerComposeFile": ["a.yml", "b.yml"]}`, []string{"a.yml", "b.yml"}, true}, + {"not declared", `{"image": "ubuntu:24.04"}`, nil, false}, + { + // The configuration says it is Compose-based, so a caller must not fall back to another + // form, even though there is no path to read. + "declared but not a path", + `{"dockerComposeFile": 42}`, + nil, true, + }, + {"a non-string element is left out", `{"dockerComposeFile": ["a.yml", 42]}`, []string{"a.yml"}, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got, _, declared := containerdef.ComposeFiles(object(t, tt.src)) + if diff := cmp.Diff(tt.want, got); diff != "" { + t.Errorf("paths mismatch (-want +got):\n%s", diff) + } + if declared != tt.wantDeclared { + t.Errorf("declared = %v, want %v", declared, tt.wantDeclared) + } + }) + } +} + +func TestComposeService(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + src string + want string + ok bool + }{ + {"a named service", `{"service": "app"}`, "app", true}, + {"no service", `{"dockerComposeFile": "docker-compose.yml"}`, "", false}, + {"a non-string service", `{"service": 42}`, "", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got, _, ok := containerdef.ComposeService(object(t, tt.src)) + if got != tt.want || ok != tt.ok { + t.Errorf("ComposeService = (%q, %v), want (%q, %v)", got, ok, tt.want, tt.ok) + } + }) + } +} diff --git a/feature/compose.go b/feature/compose.go index 6a237a7..0b41a69 100644 --- a/feature/compose.go +++ b/feature/compose.go @@ -7,6 +7,7 @@ import ( "path/filepath" "strings" + "github.com/bare-devcontainer/decolint/containerdef" "github.com/compose-spec/compose-go/v2/loader" "github.com/compose-spec/compose-go/v2/template" "github.com/compose-spec/compose-go/v2/types" @@ -31,14 +32,15 @@ func composeContributors(ctx context.Context, f *Fetcher, fsRoot *os.Root, confi if !ok { return nil, false, nil } - paths, anchor, ok := composeFilePaths(obj) - if !ok { + paths, decl, declared := containerdef.ComposeFiles(obj) + if !declared { return nil, false, nil } + anchor := decl.KeyOffset if len(paths) == 0 { return nil, true, nil } - service, ok := composeServiceName(obj) + service, _, ok := containerdef.ComposeService(obj) if !ok { return nil, true, nil } @@ -64,46 +66,6 @@ func composeContributors(ctx context.Context, f *Fetcher, fsRoot *os.Root, confi return contribs, true, nil } -// composeFilePaths returns the Compose file paths root declares, relative to the devcontainer.json -// directory, with the byte offset of the "dockerComposeFile" key. The property is a single path or -// an array of paths, later ones overriding earlier ones. ok reports whether the member exists at -// all; a present but unusable value returns ok=true with no paths, so the caller still treats -// Compose as declared and does not fall back to "build" or "image". -func composeFilePaths(obj *hujson.Object) (paths []string, anchor int, ok bool) { - i := findMember(obj, "dockerComposeFile") - if i < 0 { - return nil, 0, false - } - anchor = obj.Members[i].Name.StartOffset - switch v := obj.Members[i].Value.Value.(type) { - case hujson.Literal: - if v.Kind() == '"' { - paths = []string{v.String()} - } - case *hujson.Array: - for _, e := range v.Elements { - if lit, isLit := e.Value.(hujson.Literal); isLit && lit.Kind() == '"' { - paths = append(paths, lit.String()) - } - } - } - return paths, anchor, true -} - -// composeServiceName returns the "service" property of root; ok is false when it is absent or not -// a string. -func composeServiceName(obj *hujson.Object) (string, bool) { - i := findMember(obj, "service") - if i < 0 { - return "", false - } - lit, ok := obj.Members[i].Value.Value.(hujson.Literal) - if !ok || lit.Kind() != '"' { - return "", false - } - return lit.String(), true -} - // loadComposeService reads each Compose file at baseDir and returns the named service resolved by // compose-go, merged across all of them per the Compose specification, with "extends" and "include" // resolved as "docker compose config" would. "${...}" variables are interpolated from env as diff --git a/feature/merge.go b/feature/merge.go index 731e04e..eefb0ee 100644 --- a/feature/merge.go +++ b/feature/merge.go @@ -7,6 +7,7 @@ import ( "path/filepath" "slices" + "github.com/bare-devcontainer/decolint/containerdef" "github.com/tailscale/hujson" ) @@ -85,11 +86,12 @@ func dockerfileContributors(ctx context.Context, f *Fetcher, fsRoot *os.Root, co if !ok { return nil, false, nil } - path, anchor, ok := dockerfilePath(obj) + path, decl, ok := containerdef.Dockerfile(obj) if !ok { return nil, false, nil } - args, target := buildOptions(obj) + anchor := decl.KeyOffset + args, target := containerdef.BuildOptions(obj) src, err := readBounded(fsRoot, filepath.Join(configDir, path), maxDockerfileBytes) if err != nil { return nil, true, err @@ -105,62 +107,6 @@ func dockerfileContributors(ctx context.Context, f *Fetcher, fsRoot *os.Root, co return contribs, true, nil } -// dockerfilePath returns the Dockerfile path root declares, with the byte offset of the declaring -// key. The Dev Container schema defines two mutually exclusive Dockerfile forms: the top-level -// "dockerFile" property and the nested "build.dockerfile". The reference implementation prefers the -// top-level property (getDockerfile: 'dockerFile' in config ? config.dockerFile : -// config.build.dockerfile), so it is checked first; a valid configuration declares only one. -func dockerfilePath(obj *hujson.Object) (string, int, bool) { - if i := findMember(obj, "dockerFile"); i >= 0 { - if lit, ok := obj.Members[i].Value.Value.(hujson.Literal); ok && lit.Kind() == '"' { - return lit.String(), obj.Members[i].Name.StartOffset, true - } - } - if i := findMember(obj, "build"); i >= 0 { - if buildObj, ok := obj.Members[i].Value.Value.(*hujson.Object); ok { - if j := findMember(buildObj, "dockerfile"); j >= 0 { - if lit, ok := buildObj.Members[j].Value.Value.(hujson.Literal); ok && lit.Kind() == '"' { - return lit.String(), buildObj.Members[j].Name.StartOffset, true - } - } - } - } - return "", 0, false -} - -// buildOptions extracts the "args" and "target" of the "/build" object. -func buildOptions(obj *hujson.Object) (args map[string]string, target string) { - i := findMember(obj, "build") - if i < 0 { - return nil, "" - } - buildObj, isObj := obj.Members[i].Value.Value.(*hujson.Object) - if !isObj { - return nil, "" - } - if j := findMember(buildObj, "args"); j >= 0 { - if argsObj, isObj := buildObj.Members[j].Value.Value.(*hujson.Object); isObj { - for _, m := range argsObj.Members { - name, nameOK := m.Name.Value.(hujson.Literal) - val, valOK := m.Value.Value.(hujson.Literal) - if !nameOK || name.Kind() != '"' || !valOK || val.Kind() != '"' { - continue - } - if args == nil { - args = map[string]string{} - } - args[name.String()] = val.String() - } - } - } - if j := findMember(buildObj, "target"); j >= 0 { - if lit, isStr := buildObj.Members[j].Value.Value.(hujson.Literal); isStr && lit.Kind() == '"' { - target = lit.String() - } - } - return args, target -} - // readBounded reads the file at path through fsRoot, so its resolution cannot escape fsRoot's // boundary, rejecting a file larger than maxBytes before reading it into memory. func readBounded(fsRoot *os.Root, path string, maxBytes int64) ([]byte, error) { @@ -186,20 +132,15 @@ func imageContributors(ctx context.Context, f *Fetcher, root *hujson.Value) ([]* if !ok { return nil, nil } - i := findMember(obj, "image") - if i < 0 { - return nil, nil - } - lit, ok := obj.Members[i].Value.Value.(hujson.Literal) - if !ok || lit.Kind() != '"' { + image, decl, ok := containerdef.Image(obj) + if !ok { return nil, nil } - image := lit.String() entries, err := f.FetchImageMetadata(ctx, image) if err != nil { return nil, err } - anchor := obj.Members[i].Name.StartOffset + anchor := decl.KeyOffset contribs := make([]*contributor, 0, len(entries)) for _, md := range entries { contribs = append(contribs, &contributor{ref: image, anchor: anchor, md: md}) diff --git a/rules/compose.go b/rules/compose.go index caf1309..ce1abea 100644 --- a/rules/compose.go +++ b/rules/compose.go @@ -5,7 +5,6 @@ import ( "strings" "github.com/bare-devcontainer/decolint/linter" - "github.com/tailscale/hujson" "go.yaml.in/yaml/v3" ) @@ -28,34 +27,6 @@ type composeBuild struct { target string } -// 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 what the service runs, or // that the definition is not all in this file. type composeService struct { diff --git a/rules/dockerfile.go b/rules/dockerfile.go index e42fcd7..033e3eb 100644 --- a/rules/dockerfile.go +++ b/rules/dockerfile.go @@ -9,49 +9,8 @@ import ( "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 -} - // 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 diff --git a/rules/images.go b/rules/images.go index 1249667..38b0035 100644 --- a/rules/images.go +++ b/rules/images.go @@ -3,6 +3,7 @@ package rules import ( "fmt" + "github.com/bare-devcontainer/decolint/containerdef" "github.com/bare-devcontainer/decolint/linter" "github.com/tailscale/hujson" ) @@ -33,13 +34,11 @@ type pulledImage struct { // [dockerfilePulledImages] and [composeServiceSource] for what each leaves behind. func configImages(dir linter.Dir, obj *hujson.Object) []pulledImage { var images []pulledImage - if m := memberNamed(obj, "image"); m != nil { - if lit, ok := m.Value.Value.(hujson.Literal); ok && lit.Kind() == '"' { - images = append(images, pulledImage{ref: lit.String(), offset: m.Value.StartOffset}) - } + if ref, decl, ok := containerdef.Image(obj); ok { + images = append(images, pulledImage{ref: ref, offset: decl.ValueOffset}) } - if paths, offset, declared := composeFilePaths(obj); declared { - return append(images, composeImages(dir, obj, paths, offset)...) + if paths, decl, declared := containerdef.ComposeFiles(obj); declared { + return append(images, composeImages(dir, obj, paths, decl.ValueOffset)...) } return append(images, dockerfileImages(dir, obj)...) } @@ -47,7 +46,7 @@ func configImages(dir linter.Dir, obj *hujson.Object) []pulledImage { // dockerfileImages returns the images the Dockerfile obj names pulls, anchored at the property // naming it. func dockerfileImages(dir linter.Dir, obj *hujson.Object) []pulledImage { - path, offset, ok := dockerfileRef(obj) + path, decl, ok := containerdef.Dockerfile(obj) if !ok { return nil } @@ -55,13 +54,14 @@ func dockerfileImages(dir linter.Dir, obj *hujson.Object) []pulledImage { if !ok { return nil } - return locate(dockerfilePulledImages(src, buildTarget(obj)), fmt.Sprintf("Dockerfile %q: ", path), offset) + _, target := containerdef.BuildOptions(obj) + return locate(dockerfilePulledImages(src, target), fmt.Sprintf("Dockerfile %q: ", path), decl.ValueOffset) } // 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, obj *hujson.Object, paths []string, offset int) []pulledImage { - service, ok := stringMember(obj, "service") + service, _, ok := containerdef.ComposeService(obj) if !ok { return nil } From 6ab0ee190db6e0ce1fb1d7ee6187f76e7e0ad80c Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 05:07:59 +0000 Subject: [PATCH 13/13] fix(rules): resolve a FROM written with a variable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reading Dockerfiles brought a blind spot with it: a base image written as "FROM ubuntu:${VARIANT}" was left unchecked, on the grounds that its value comes from the build rather than from the configuration. It comes from both — the Dockerfile's own ARG default, or the "build.args" the devcontainer.json passes — and BuildKit resolves it from exactly those before it pulls anything. "ARG VARIANT" with a "${VARIANT}" base is the shape devcontainer templates are written in, so the blind spot covered a common configuration, and it was one this change introduced: nothing read a Dockerfile before it. The rules now expand a FROM the way dockerfile2llb does, against the global ARGs with the configuration's arguments applied over them, and a Compose service's build args reach it the same way. A reference whose value neither declares stays unchecked: BuildKit expands it to nothing, so there is no image to report on. A "--from" is still left alone, a variable there failing the build outright. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Y7ohBnPuPSezRvwrkzaRaA --- rules/compose.go | 37 +++++++++++- rules/compose_test.go | 25 ++++++++- rules/dockerfile.go | 71 +++++++++++++++++++----- rules/images.go | 6 +- rules/no_image_latest.go | 5 +- rules/no_image_latest_dockerfile_test.go | 67 +++++++++++++++++++++- rules/pin_image_digest.go | 5 +- 7 files changed, 191 insertions(+), 25 deletions(-) diff --git a/rules/compose.go b/rules/compose.go index ce1abea..1f5c58c 100644 --- a/rules/compose.go +++ b/rules/compose.go @@ -25,6 +25,8 @@ type composeBuild struct { 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 @@ -119,6 +121,7 @@ func composeServiceSource(dir linter.Dir, paths []string, service string) (compo // 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. @@ -128,12 +131,13 @@ func composeServiceBuild(value any, baseDir string) *composeBuild { 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} + 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. @@ -146,5 +150,34 @@ func composeServiceBuild(value any, baseDir string) *composeBuild { if strings.Contains(dockerfile, "$") { return nil } - return &composeBuild{dockerfile: path.Join(baseDir, context, dockerfile), target: target} + 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 index facd1b8..15551a3 100644 --- a/rules/compose_test.go +++ b/rules/compose_test.go @@ -5,6 +5,7 @@ import ( "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 @@ -48,6 +49,28 @@ func TestComposeServiceSource(t *testing.T) { 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"}, @@ -180,7 +203,7 @@ func TestComposeServiceSource(t *testing.T) { 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 && *got.build != *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 index 033e3eb..57c90f0 100644 --- a/rules/dockerfile.go +++ b/rules/dockerfile.go @@ -9,12 +9,18 @@ import ( "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: 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. +// 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 @@ -22,7 +28,7 @@ import ( // // 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) []string { +func dockerfilePulledImages(src []byte, args map[string]string, target string) []string { result, err := parser.Parse(bytes.NewReader(src)) if err != nil { return nil @@ -30,22 +36,28 @@ func dockerfilePulledImages(src []byte, target string) []string { // 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{})) + 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 && isPulledImage(stages[i].BaseName) { - images = append(images, stages[i].BaseName) + 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) { - if from.stage < 0 && isPulledImage(from.ref) { + // 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) } } @@ -54,13 +66,44 @@ func dockerfilePulledImages(src []byte, target string) []string { } // 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. +// 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" && !strings.Contains(ref, "$") + 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, diff --git a/rules/images.go b/rules/images.go index 38b0035..1820e41 100644 --- a/rules/images.go +++ b/rules/images.go @@ -54,8 +54,8 @@ func dockerfileImages(dir linter.Dir, obj *hujson.Object) []pulledImage { if !ok { return nil } - _, target := containerdef.BuildOptions(obj) - return locate(dockerfilePulledImages(src, target), fmt.Sprintf("Dockerfile %q: ", path), decl.ValueOffset) + args, target := containerdef.BuildOptions(obj) + return locate(dockerfilePulledImages(src, args, target), fmt.Sprintf("Dockerfile %q: ", path), decl.ValueOffset) } // composeImages returns the images the Compose service the dev container runs pulls: the one it @@ -84,7 +84,7 @@ func composeImages(dir linter.Dir, obj *hujson.Object, paths []string, offset in } where = fmt.Sprintf("Dockerfile %q: ", source.build.dockerfile) } - return locate(dockerfilePulledImages(src, source.build.target), where, offset) + 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. diff --git a/rules/no_image_latest.go b/rules/no_image_latest.go index 8641070..9547c2a 100644 --- a/rules/no_image_latest.go +++ b/rules/no_image_latest.go @@ -45,8 +45,9 @@ Compose-based configuration, the image its service runs or the Dockerfile that s `}, }, }, - Note: "An image written with a `$` or `${...}` variable is not checked: its value comes from\n" + - "the environment or from `build.args`, not from the configuration.", + 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, } diff --git a/rules/no_image_latest_dockerfile_test.go b/rules/no_image_latest_dockerfile_test.go index 0cb1e69..7738887 100644 --- a/rules/no_image_latest_dockerfile_test.go +++ b/rules/no_image_latest_dockerfile_test.go @@ -68,10 +68,42 @@ func TestNoImageLatest_Dockerfile(t *testing.T) { issue(`Dockerfile "Dockerfile": image "0" has no explicit tag; pin a specific version`), }, { - "an image reached through a variable is not resolved", + // 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", @@ -172,6 +204,39 @@ func TestNoImageLatest_Dockerfile(t *testing.T) { } } +// TestNoImageLatest_Dockerfile_BuildArgs checks that the arguments the configuration passes settle a +// FROM written with a variable, as they do for the build itself. +func TestNoImageLatest_Dockerfile_BuildArgs(t *testing.T) { + t.Parallel() + + dir := linter.Dir{FS: fstest.MapFS{"Dockerfile": {Data: []byte("ARG VARIANT=24.04\nFROM ubuntu:${VARIANT}\n")}}} + + tests := []struct { + name string + src string + want []linter.Issue + }{ + { + "an argument overriding the ARG default is the one resolved", + `{"build": {"dockerfile": "Dockerfile", "args": {"VARIANT": "latest"}}}`, + []linter.Issue{{Path: "devcontainer.json", Line: 1, Col: 26, RuleID: "no-image-latest", Message: `Dockerfile "Dockerfile": image "ubuntu:latest" uses the "latest" tag; pin a specific version`}}, + }, + {"the ARG default stands when no argument overrides it", `{"build": {"dockerfile": "Dockerfile"}}`, nil}, + { + // An argument the Dockerfile never declares is out of scope for a FROM. + "an argument no ARG declares settles nothing", + `{"build": {"dockerfile": "Dockerfile", "args": {"OTHER": "latest"}}}`, + 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) + }) + } +} + // TestNoImageLatest_Dockerfile_BuildTarget checks that only the stages a build of "build.target" // reaches are read, since the others are never built. func TestNoImageLatest_Dockerfile_BuildTarget(t *testing.T) { diff --git a/rules/pin_image_digest.go b/rules/pin_image_digest.go index 7428dac..d034f89 100644 --- a/rules/pin_image_digest.go +++ b/rules/pin_image_digest.go @@ -52,8 +52,9 @@ Compose-based configuration, the image its service runs or the Dockerfile that s `}, }, }, - Note: "An image written with a `$` or `${...}` variable is not checked: its value comes from\n" + - "the environment or from `build.args`, not from the configuration.", + 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: checkPinImageDigest, }