From 23fd33fd0084e90f07d090c5f14459ebf0dc8365 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 14:42:09 +0000 Subject: [PATCH 1/4] feat: pin external images referenced by COPY --from= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolve and pin `COPY --from=` the same way `FROM` is already pinned, so an image copied from at build time is covered by the same supply-chain guarantee as a base image. What is pinned and what is not follows BuildKit's own reading of the flag: - `--from=` is pinned, including a ref that already carries a digest (re-resolved with `--update`). - `--from=` is skipped. Every stage name in the file is collected up front, so a name declared below the COPY is still recognised as a stage rather than mistaken for an image; using one that way is a build error about stage order, not an unpinned image. - `--from=0` is skipped. BuildKit classifies the value with strconv.Atoi before anything else, so a numeric value always selects a stage by position. - `--from=scratch` and an empty `--from=` are skipped. - `--from=` matching no stage is treated as an image, as `FROM ubuntu` is. A named build context is written the same way and cannot be told apart from the Dockerfile alone; `--ignore-images` excludes one. - `--from=image:${TAG}` is skipped: CopyCommand.Expand covers --chown, --chmod and the paths but not --from, so BuildKit reads the value verbatim and the build fails to parse the stage name (moby/buildkit#2374). `FROM` does expand, and still does. - `ADD --from=`, `RUN --mount=...,from=` and `--FROM=` are left alone: none of them is a COPY --from flag, and `docker build` rejects the last two outright. Two fixes fall out of the same code: - ARG defaults are still collected in file order, so an ARG below a FROM does not expand that FROM's ref. Collecting them up front — which a two-pass parse invites — would silently pin a ref docker never expands. - A rewrite now searches every line an instruction covers instead of only its first, so a reference written after a "\" continuation is pinned rather than silently reported as pinned and left unchanged. This also fixes `FROM \` + newline, which had the same problem before COPY existed. A COPY replacement is anchored at the `--from=` flag, so the same text elsewhere on the line (another flag's value, a source path) is never rewritten by mistake. Tests cover parsing, rewriting, the `run`/`check` command paths and the whole pipeline end to end, including the reproduction from the issue, a testdata Dockerfile round trip that asserts a second pass changes nothing, and the `--update` path. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CERP67PPQ72qxDHKqpXkcR --- README.md | 23 +- cmd/check.go | 4 +- cmd/check_test.go | 49 +++++ cmd/pin.go | 7 +- cmd/pin_test.go | 132 ++++++++++++ e2e_test.go | 198 +++++++++++++++++ internal/dockerfile/parse.go | 199 +++++++++++++---- internal/dockerfile/parse_test.go | 321 ++++++++++++++++++++++++++++ internal/dockerfile/rewrite.go | 58 ++++- internal/dockerfile/rewrite_test.go | 205 +++++++++++++++++- testdata/copy_from.Dockerfile | 24 +++ 11 files changed, 1163 insertions(+), 57 deletions(-) create mode 100644 testdata/copy_from.Dockerfile diff --git a/README.md b/README.md index 3428270..4685bb9 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ # dockerfile-pin -A CLI tool that adds `@sha256:` to `FROM` lines in Dockerfiles, `image` fields in docker-compose.yml, and Docker image references in GitHub Actions and GitLab CI files to prevent supply chain attacks. +A CLI tool that adds `@sha256:` to `FROM` and `COPY --from=` lines in Dockerfiles, `image` fields in docker-compose.yml, and Docker image references in GitHub Actions and GitLab CI files to prevent supply chain attacks. ## Install @@ -77,6 +77,8 @@ dockerfile-pin run --write --update --min-age 7 FROM node:20.11.1 FROM python:3.12-slim AS builder FROM scratch +COPY --from=nginx:1.27 /etc/nginx /etc/nginx +COPY --from=builder /app /app ``` **After:** @@ -85,6 +87,8 @@ FROM scratch FROM node:20.11.1@sha256:e06aae17c40c7a6b5296ca6f942a02e6737ae61bbbf3e2158624bb0f887991b5 FROM python:3.12-slim@sha256:3d5ed973e45820f5ba5e46bd065bd88b3a504ff0724d85980dcd05eab361fcf4 AS builder FROM scratch +COPY --from=nginx:1.27@sha256:6784fb08b4b7c3b6bcd3f4a1b4d1b1f3e3b7a7ca42ec3e0d9df8a97a2c9a3b1d /etc/nginx /etc/nginx +COPY --from=builder /app /app ``` #### docker-compose.yml @@ -295,6 +299,23 @@ ignore-images: | `ARG BASE` + `FROM ${BASE}` (no default) | Skipped with warning | | `FROM ghcr.io/org/image:tag` | Yes | | `FROM registry:5000/image:tag` | Yes | +| `COPY --from=image:tag` | Yes | +| `COPY --from=image:tag@sha256:...` (already pinned) | Skipped (use `--update` to refresh) | +| `COPY --from=` (multi-stage ref) | Skipped | +| `COPY --from=0` (stage index) | Skipped | +| `COPY --from=scratch` | Skipped | +| `COPY --from=image:${TAG}` | Skipped (BuildKit does not expand variables here) | +| `COPY /src /dst` (build context) | Nothing to pin | +| `ADD --from=...`, `RUN --mount=...,from=...` | Not supported | + +A `COPY --from=` that matches no build stage is treated as an image, the same +way `FROM ubuntu` is. A [named build context](https://docs.docker.com/reference/cli/docker/buildx/build/#build-context) +(`docker buildx build --build-context name=...`) is written the same way and cannot +be told apart from the Dockerfile alone; use `--ignore-images` to exclude one. + +Variables are not expanded in `COPY --from=`: BuildKit reads the value verbatim and +the build fails with `failed to parse stage name` ([moby/buildkit#2374](https://github.com/moby/buildkit/issues/2374)), +so such a reference is reported as skipped rather than pinned. `FROM` does expand them. ### docker-compose.yml diff --git a/cmd/check.go b/cmd/check.go index 4c9382f..5c1a3ff 100644 --- a/cmd/check.go +++ b/cmd/check.go @@ -20,14 +20,14 @@ import ( var checkCmd = &cobra.Command{ Use: "check", - Short: "Check if FROM images are pinned to digests", + Short: "Check if FROM and COPY --from images are pinned to digests", Long: `Validate that every Docker image reference has a @sha256: and that the digest exists in the registry. Each image is reported as one of: OK digest present and verified in registry FAIL missing digest, or digest not found in registry - SKIP scratch, multi-stage ref, ignored, non-Docker uses, or CI variable + SKIP scratch, multi-stage ref, stage index, ignored, non-Docker uses, or CI variable WARN registry check failed (network error, auth issue, etc.) Exit code is 1 (configurable with --exit-code) when any image has FAIL status. diff --git a/cmd/check_test.go b/cmd/check_test.go index 2970806..b855172 100644 --- a/cmd/check_test.go +++ b/cmd/check_test.go @@ -51,3 +51,52 @@ test: } } } + +// TestParseDockerfileForCheck_CopyFrom covers the status and the line `check` reports +// for each form of COPY --from. +func TestParseDockerfileForCheck_CopyFrom(t *testing.T) { + content := `FROM golang:1.22@sha256:golang111 AS builder +COPY --from=builder /app /app +COPY --from=0 /go/bin/tool /usr/local/bin/tool +COPY --from=nginx:1.27 /etc/nginx /etc/nginx +COPY --from=busybox:1.36@sha256:busybox222 /bin/busybox /bin/busybox +COPY --from=ghcr.io/myorg/tool:v1 /tool /tool +COPY --from=nginx:${NGINX_VERSION} /etc/nginx /etc/nginx.orig +COPY ./config /config +` + dir := t.TempDir() + path := filepath.Join(dir, "Dockerfile") + if err := os.WriteFile(path, []byte(content), 0644); err != nil { + t.Fatal(err) + } + + results, err := parseDockerfileForCheck(path, true, []string{"ghcr.io/myorg/*"}) + if err != nil { + t.Fatalf("parseDockerfileForCheck() error = %v", err) + } + + want := []struct { + image string + status string + line int + original string + }{ + {"golang:1.22", "ok", 1, "FROM golang:1.22@sha256:golang111 AS builder"}, + {"builder", "skip", 2, "COPY --from=builder /app /app"}, + {"0", "skip", 3, "COPY --from=0 /go/bin/tool /usr/local/bin/tool"}, + {"nginx:1.27", "fail", 4, "COPY --from=nginx:1.27 /etc/nginx /etc/nginx"}, + {"busybox:1.36", "ok", 5, "COPY --from=busybox:1.36@sha256:busybox222 /bin/busybox /bin/busybox"}, + {"ghcr.io/myorg/tool:v1", "skip", 6, "COPY --from=ghcr.io/myorg/tool:v1 /tool /tool"}, + {"nginx:${NGINX_VERSION}", "skip", 7, "COPY --from=nginx:${NGINX_VERSION} /etc/nginx /etc/nginx.orig"}, + } + if len(results) != len(want) { + t.Fatalf("got %d results, want %d: %+v", len(results), len(want), results) + } + for i, w := range want { + got := results[i] + if got.Image != w.image || got.Status != w.status || got.Line != w.line || got.Original != w.original { + t.Errorf("[%d] got %+v, want image=%q status=%q line=%d original=%q", + i, got, w.image, w.status, w.line, w.original) + } + } +} diff --git a/cmd/pin.go b/cmd/pin.go index 91cf772..998a029 100644 --- a/cmd/pin.go +++ b/cmd/pin.go @@ -19,7 +19,7 @@ import ( var runCmd = &cobra.Command{ Use: "run", - Short: "Pin FROM images to their digests", + Short: "Pin FROM and COPY --from images to their digests", Long: `Resolve image tags to sha256 digests and add @sha256: to each reference. By default, prints the rewritten file to stdout without modifying it (dry-run). Use --write to apply changes in place. @@ -27,9 +27,12 @@ Use --write to apply changes in place. Supports Dockerfiles, docker-compose.yml/compose.yaml, GitHub Actions workflows, action.yml files, and .gitlab-ci.yml. File type is detected from filename. +Both "FROM image:tag" and "COPY --from=image:tag" are pinned. + Skipped automatically: - "FROM scratch" (no registry image) - - Multi-stage references ("FROM builder") + - Multi-stage references ("FROM builder", "COPY --from=builder", "COPY --from=0") + - Variables in "COPY --from" (BuildKit does not expand them) - ARG-only base images with no default value - Compose services with a "build:" directive - Non-docker "uses:" in GitHub Actions (e.g., actions/checkout@v4) diff --git a/cmd/pin_test.go b/cmd/pin_test.go index 03a3d51..7489d90 100644 --- a/cmd/pin_test.go +++ b/cmd/pin_test.go @@ -510,3 +510,135 @@ func TestResolveParallel_MinAge_CreatedTimeErrorPinsAnyway(t *testing.T) { t.Errorf("expected node:20 to be pinned despite age-check failure, got %q", results["node:20"]) } } + +const copyFromDockerfile = `FROM golang:1.22 AS builder +COPY --from=builder /app /app +COPY --from=0 /go/bin/tool /usr/local/bin/tool +COPY --from=nginx:1.27 /etc/nginx /etc/nginx +COPY --chown=65532:65532 --from=ghcr.io/myorg/tool:v1 /tool /tool +COPY ./config /config +` + +// TestParseFile_DockerfileCollectsCopyFromRefs checks which references `run` sends to +// the registry: external images from COPY --from, but never a stage name or index. +func TestParseFile_DockerfileCollectsCopyFromRefs(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "Dockerfile") + if err := os.WriteFile(path, []byte(copyFromDockerfile), 0644); err != nil { + t.Fatal(err) + } + + pf, err := parseFile(path, false, nil) + if err != nil { + t.Fatalf("parseFile() error = %v", err) + } + + want := []string{"golang:1.22", "nginx:1.27", "ghcr.io/myorg/tool:v1"} + if len(pf.imageRefs) != len(want) { + t.Fatalf("imageRefs = %v, want %v", pf.imageRefs, want) + } + for i, w := range want { + if pf.imageRefs[i] != w { + t.Errorf("imageRefs[%d] = %q, want %q", i, pf.imageRefs[i], w) + } + } +} + +// TestParseFile_DockerfileIgnoresCopyFromImages checks that --ignore-images applies to +// COPY --from as well, which is how a named build context is excluded. +func TestParseFile_DockerfileIgnoresCopyFromImages(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "Dockerfile") + if err := os.WriteFile(path, []byte(copyFromDockerfile), 0644); err != nil { + t.Fatal(err) + } + + pf, err := parseFile(path, false, []string{"ghcr.io/myorg/*"}) + if err != nil { + t.Fatalf("parseFile() error = %v", err) + } + + for _, ref := range pf.imageRefs { + if ref == "ghcr.io/myorg/tool:v1" { + t.Errorf("ignored image %q should not be resolved: %v", ref, pf.imageRefs) + } + } + if len(pf.imageRefs) != 2 { + t.Errorf("imageRefs = %v, want golang:1.22 and nginx:1.27", pf.imageRefs) + } +} + +// TestApplyDockerfile_PinsCopyFrom writes a pinned Dockerfile the way `run --write` +// does and checks the file on disk. +func TestApplyDockerfile_PinsCopyFrom(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "Dockerfile") + if err := os.WriteFile(path, []byte(copyFromDockerfile), 0644); err != nil { + t.Fatal(err) + } + + instructions, err := dockerfile.Parse(strings.NewReader(copyFromDockerfile)) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + pf := parsedFile{ + path: path, + fileType: FileTypeDockerfile, + dockerInsts: instructions, + content: []byte(copyFromDockerfile), + } + digestMap := map[string]string{ + "golang:1.22": "sha256:golang111", + "nginx:1.27": "sha256:nginx222", + "ghcr.io/myorg/tool:v1": "sha256:tool333", + } + + applyDockerfile(pf, digestMap, false, false) + + result, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + got := string(result) + want := `FROM golang:1.22@sha256:golang111 AS builder +COPY --from=builder /app /app +COPY --from=0 /go/bin/tool /usr/local/bin/tool +COPY --from=nginx:1.27@sha256:nginx222 /etc/nginx /etc/nginx +COPY --chown=65532:65532 --from=ghcr.io/myorg/tool:v1@sha256:tool333 /tool /tool +COPY ./config /config +` + if got != want { + t.Errorf("applyDockerfile() wrote:\n%s\nwant:\n%s", got, want) + } +} + +// TestApplyDockerfile_UpdateCopyFromDigest covers `run --write --update` re-resolving +// a COPY --from that is already pinned. +func TestApplyDockerfile_UpdateCopyFromDigest(t *testing.T) { + content := "FROM ubuntu:24.04@sha256:oldubuntu\nCOPY --from=nginx:1.27@sha256:oldnginx /etc/nginx /etc/nginx\n" + dir := t.TempDir() + path := filepath.Join(dir, "Dockerfile") + if err := os.WriteFile(path, []byte(content), 0644); err != nil { + t.Fatal(err) + } + + instructions, err := dockerfile.Parse(strings.NewReader(content)) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + pf := parsedFile{path: path, fileType: FileTypeDockerfile, dockerInsts: instructions, content: []byte(content)} + + applyDockerfile(pf, map[string]string{ + "ubuntu:24.04": "sha256:newubuntu", + "nginx:1.27": "sha256:newnginx", + }, false, true) + + result, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + want := "FROM ubuntu:24.04@sha256:newubuntu\nCOPY --from=nginx:1.27@sha256:newnginx /etc/nginx /etc/nginx\n" + if string(result) != want { + t.Errorf("applyDockerfile() wrote:\n%s\nwant:\n%s", string(result), want) + } +} diff --git a/e2e_test.go b/e2e_test.go index c82b2bb..55e2102 100644 --- a/e2e_test.go +++ b/e2e_test.go @@ -802,3 +802,201 @@ func TestConfigFileEndToEnd(t *testing.T) { t.Error("node:20 should not be ignored") } } + +// pinAll runs the whole pin pipeline over content: parse, resolve every reference +// that is neither skipped nor already pinned, then rewrite. Resolving is strict, so a +// reference that should have been skipped — a stage name, a stage index — fails the +// test instead of silently reaching the registry. +func pinAll(t *testing.T, content string, mock *resolver.MockResolver, update bool) string { + t.Helper() + instructions, err := dockerfile.Parse(strings.NewReader(content)) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + ctx := context.Background() + digests := make(map[int]string) + for i, inst := range instructions { + if inst.Skip || (inst.Digest != "" && !update) { + continue + } + digest, err := mock.Resolve(ctx, inst.ImageRef) + if err != nil { + t.Fatalf("[%d] unexpected resolve of %q: %v", i, inst.ImageRef, err) + } + digests[i] = digest + } + return dockerfile.RewriteFile(content, instructions, digests) +} + +// TestPinCopyFromEndToEnd pins the Dockerfile from the feature request and compares +// the whole file, including the stage index that must be left as it is. +func TestPinCopyFromEndToEnd(t *testing.T) { + input := "# Dockerfile\nFROM ubuntu:24.04\nCOPY --from=nginx:1.27 /etc/nginx /etc/nginx\nCOPY --from=0 /a /b\n" + mock := &resolver.MockResolver{ + Digests: map[string]string{ + "ubuntu:24.04": "sha256:ubuntu111", + "nginx:1.27": "sha256:nginx222", + }, + } + + want := "# Dockerfile\n" + + "FROM ubuntu:24.04@sha256:ubuntu111\n" + + "COPY --from=nginx:1.27@sha256:nginx222 /etc/nginx /etc/nginx\n" + + "COPY --from=0 /a /b\n" + if got := pinAll(t, input, mock, false); got != want { + t.Errorf("pin =\n%s\nwant:\n%s", got, want) + } +} + +// TestPinCopyFromUpdateEndToEnd re-resolves digests that are already written, the +// path taken by `run --update`. +func TestPinCopyFromUpdateEndToEnd(t *testing.T) { + input := "FROM ubuntu:24.04@sha256:oldubuntu\nCOPY --from=nginx:1.27@sha256:oldnginx /etc/nginx /etc/nginx\n" + mock := &resolver.MockResolver{ + Digests: map[string]string{ + "ubuntu:24.04": "sha256:newubuntu", + "nginx:1.27": "sha256:newnginx", + }, + } + + want := "FROM ubuntu:24.04@sha256:newubuntu\nCOPY --from=nginx:1.27@sha256:newnginx /etc/nginx /etc/nginx\n" + if got := pinAll(t, input, mock, true); got != want { + t.Errorf("pin --update =\n%s\nwant:\n%s", got, want) + } +} + +// TestCheckCopyFromEndToEnd walks the statuses `check` reports for each form of +// COPY --from. +func TestCheckCopyFromEndToEnd(t *testing.T) { + input := "FROM golang:1.22 AS builder\n" + + "COPY --from=builder /app /app\n" + + "COPY --from=0 /go/bin/tool /usr/local/bin/tool\n" + + "COPY --from=nginx:1.27 /etc/nginx /etc/nginx\n" + + "COPY --from=busybox:1.36@sha256:validdigest /bin/busybox /bin/busybox\n" + + "COPY --from=alpine:3.19@sha256:missingdigest /etc/alpine-release /etc/alpine-release\n" + + "COPY --from=scratch /a /b\n" + + "COPY ./config /config\n" + + mock := &resolver.MockResolver{ + Digests: map[string]string{ + "busybox:1.36@sha256:validdigest": "sha256:validdigest", + "golang:1.22@sha256:golangdigest": "sha256:golangdigest", + }, + } + + instructions, err := dockerfile.Parse(strings.NewReader(input)) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + + ctx := context.Background() + type checkResult struct { + imageRef string + status string + } + var results []checkResult + for _, inst := range instructions { + switch { + case inst.Skip: + results = append(results, checkResult{inst.ImageRef, "skip:" + inst.SkipReason}) + case inst.Digest == "": + results = append(results, checkResult{inst.ImageRef, "fail-missing"}) + default: + exists, _ := mock.Exists(ctx, inst.ImageRef+"@"+inst.Digest) + if exists { + results = append(results, checkResult{inst.ImageRef, "ok"}) + } else { + results = append(results, checkResult{inst.ImageRef, "fail-notfound"}) + } + } + } + + expected := []checkResult{ + {"golang:1.22", "fail-missing"}, + {"builder", "skip:" + dockerfile.SkipStageRef}, + {"0", "skip:" + dockerfile.SkipStageIndex}, + {"nginx:1.27", "fail-missing"}, + {"busybox:1.36", "ok"}, + {"alpine:3.19", "fail-notfound"}, + {"scratch", "skip:" + dockerfile.SkipScratch}, + } + if len(results) != len(expected) { + t.Fatalf("got %d results, want %d: %+v", len(results), len(expected), results) + } + for i, want := range expected { + if results[i] != want { + t.Errorf("[%d] got %+v, want %+v", i, results[i], want) + } + } +} + +// TestPinCopyFromTestdataRoundTrip pins testdata/copy_from.Dockerfile, writes it out +// and reads it back the way `run --write` does, then checks that a second pass has +// nothing left to change. +func TestPinCopyFromTestdataRoundTrip(t *testing.T) { + content, err := os.ReadFile(filepath.Join("testdata", "copy_from.Dockerfile")) + if err != nil { + t.Fatal(err) + } + mock := &resolver.MockResolver{ + Digests: map[string]string{ + "golang:1.22": "sha256:golang111", + "gcr.io/distroless/base-debian12:nonroot": "sha256:distroless222", + "nginx:1.27": "sha256:nginx333", + "busybox:1.36": "sha256:busybox444", + "registry.example.com:5000/tool:1.0": "sha256:tool555", + }, + } + + dir := t.TempDir() + path := filepath.Join(dir, "Dockerfile") + if err := os.WriteFile(path, []byte(pinAll(t, string(content), mock, false)), 0644); err != nil { + t.Fatal(err) + } + written, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + got := string(written) + + // Every reference that can be pinned now carries a digest, which is what + // `check --syntax-only` asks of the file. + instructions, err := dockerfile.Parse(strings.NewReader(got)) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + for i, inst := range instructions { + if !inst.Skip && inst.Digest == "" { + t.Errorf("[%d] %q left unpinned", i, inst.ImageRef) + } + } + + for _, want := range []string{ + "FROM golang:1.22@sha256:golang111 AS builder", + "FROM gcr.io/distroless/base-debian12:nonroot@sha256:distroless222", + "COPY --from=nginx:1.27@sha256:nginx333 /etc/nginx /etc/nginx", + "COPY --chown=65532:65532 --from=busybox:1.36@sha256:busybox444 /bin/busybox /bin/busybox", + "COPY --from=registry.example.com:5000/tool:1.0@sha256:tool555 /tool /usr/local/bin/tool2", + } { + if !strings.Contains(got, want) { + t.Errorf("expected pinned line %q\nin:\n%s", want, got) + } + } + + for _, want := range []string{ + "RUN go build -o /app", + "COPY --from=builder /app /app", + "COPY --from=0 /go/bin/tool /usr/local/bin/tool", + "COPY --from=alpine:3.19@sha256:aaaa1111 /etc/alpine-release /etc/alpine-release", + "COPY --from=nginx:${NGINX_VERSION} /etc/nginx /etc/nginx.orig", + "COPY ./config /config", + } { + if !strings.Contains(got, want) { + t.Errorf("expected untouched line %q\nin:\n%s", want, got) + } + } + + if again := pinAll(t, got, mock, false); again != got { + t.Errorf("pinning an already-pinned file changed it:\n%s", again) + } +} diff --git a/internal/dockerfile/parse.go b/internal/dockerfile/parse.go index 9cb2c02..ad61c7e 100644 --- a/internal/dockerfile/parse.go +++ b/internal/dockerfile/parse.go @@ -2,44 +2,81 @@ package dockerfile import ( "io" + "strconv" "strings" "github.com/moby/buildkit/frontend/dockerfile/parser" ) -// FromInstruction represents a parsed FROM instruction in a Dockerfile. +// copyFromFlag is the COPY flag naming what to copy from: an external image, an +// earlier build stage, or a named build context. BuildKit matches flag names +// case-sensitively, so "--FROM=" is not this flag; `docker build` rejects it as an +// unknown flag rather than treating it as --from. +const copyFromFlag = "--from=" + +// Reasons reported in FromInstruction.SkipReason when an instruction is not pinnable. +const ( + SkipMissingRef = "missing image reference" + SkipScratch = "scratch image" + SkipStageRef = "stage reference" + SkipStageIndex = "stage index reference" + SkipUnresolvedARG = "unresolved ARG variable" + SkipCopyFromVar = "unexpanded variable in COPY --from" +) + +// FromInstruction represents a pinnable image reference in a Dockerfile: either a +// FROM instruction or the --from= flag of a COPY instruction. type FromInstruction struct { ImageRef string // image ref without digest, after ARG expansion (e.g., "node:20.11.1") RawRef string // as written in Dockerfile (may contain ${VAR}, may include digest) Digest string // existing digest if present (e.g., "sha256:abc...") - Platform string // --platform value - StageName string // AS clause name - StartLine int // 1-based line number - Original string // original FROM line text + Platform string // --platform value (FROM only) + StageName string // AS clause name (FROM only) + StartLine int // 1-based line where the instruction starts + EndLine int // 1-based line where the instruction ends (larger than StartLine when continued with "\") + Original string // original instruction text, with any line continuations joined Skip bool SkipReason string + IsCopyFrom bool // true for COPY --from=, false for FROM } -// Parse reads a Dockerfile from r and returns all FROM instructions. +// Parse reads a Dockerfile from r and returns every pinnable image reference: +// each FROM instruction and each COPY --from=. func Parse(r io.Reader) ([]FromInstruction, error) { result, err := parser.Parse(r) if err != nil { return nil, err } + // Every stage name is collected up front for COPY --from, so that a name declared + // further down the file is still recognised as a stage rather than mistaken for an + // image to pin. Using one that way is a build error ("cannot copy from stage %q, it + // needs to be defined before current stage %q"), but it is an error about stage + // order, not an unpinned image, so there is nothing here to rewrite. + // FROM does not get the same treatment: BuildKit resolves each FROM against the + // stages declared above it only, so a name defined later really is an image there, + // and seenStages below grows as the file is walked. + allStages := collectStageNames(result.AST) + argDefaults := map[string]string{} - stageNames := map[string]bool{} + seenStages := map[string]bool{} var instructions []FromInstruction for _, node := range result.AST.Children { switch strings.ToLower(node.Value) { case "arg": + // Collected in file order: an ARG below a FROM is scoped to the stage it + // opens, so it must not expand a variable in that FROM. parseArgNode(node, argDefaults) case "from": - inst := parseFromNode(node, argDefaults, stageNames) + inst := parseFromNode(node, argDefaults, seenStages) instructions = append(instructions, inst) if inst.StageName != "" { - stageNames[strings.ToLower(inst.StageName)] = true + seenStages[strings.ToLower(inst.StageName)] = true + } + case "copy": + if inst, ok := parseCopyFromNode(node, allStages); ok { + instructions = append(instructions, inst) } } } @@ -47,6 +84,21 @@ func Parse(r io.Reader) ([]FromInstruction, error) { return instructions, nil } +// collectStageNames returns every name introduced by a "FROM ... AS " clause, +// lowercased because BuildKit looks stage names up case-insensitively. +func collectStageNames(root *parser.Node) map[string]bool { + names := map[string]bool{} + for _, node := range root.Children { + if strings.ToLower(node.Value) != "from" || node.Next == nil { + continue + } + if n := node.Next.Next; n != nil && strings.ToLower(n.Value) == "as" && n.Next != nil { + names[strings.ToLower(n.Next.Value)] = true + } + } + return names +} + // parseArgNode extracts ARG defaults from an ARG node and stores them in defaults map. func parseArgNode(node *parser.Node, defaults map[string]string) { if node.Next == nil { @@ -65,6 +117,7 @@ func parseArgNode(node *parser.Node, defaults map[string]string) { func parseFromNode(node *parser.Node, argDefaults map[string]string, stageNames map[string]bool) FromInstruction { inst := FromInstruction{ StartLine: node.StartLine, + EndLine: node.EndLine, Original: node.Original, } @@ -78,12 +131,11 @@ func parseFromNode(node *parser.Node, argDefaults map[string]string, stageNames // The image ref is the first token after FROM (node.Next) if node.Next == nil { inst.Skip = true - inst.SkipReason = "missing image reference" + inst.SkipReason = SkipMissingRef return inst } - rawRef := node.Next.Value - inst.RawRef = rawRef + inst.RawRef = node.Next.Value // Check for AS clause (Next.Next = "as", Next.Next.Next = stage name) n := node.Next.Next @@ -91,49 +143,110 @@ func parseFromNode(node *parser.Node, argDefaults map[string]string, stageNames inst.StageName = n.Next.Value } - // Handle scratch image - if strings.ToLower(rawRef) == "scratch" { - inst.Skip = true - inst.SkipReason = "scratch image" - inst.ImageRef = rawRef - return inst + inst.ImageRef, inst.Digest, inst.SkipReason, inst.Skip = resolveRef(inst.RawRef, argDefaults, stageNames) + return inst +} + +// parseCopyFromNode parses a COPY node, returning a FromInstruction when the +// instruction carries a --from= flag. The second result is false for a plain COPY, +// which copies from the build context and has nothing to pin. +func parseCopyFromNode(node *parser.Node, stageNames map[string]bool) (FromInstruction, bool) { + fromValue, ok := copyFromValue(node.Flags) + if !ok { + return FromInstruction{}, false } - // Expand ARG variables in the ref - expanded, hasUnresolved := expandVars(rawRef, argDefaults) + inst := FromInstruction{ + StartLine: node.StartLine, + EndLine: node.EndLine, + Original: node.Original, + RawRef: fromValue, + ImageRef: fromValue, + IsCopyFrom: true, + } - // Check if the expanded ref is a stage reference - // A stage reference is when the ref (without tag/digest) matches a known stage name - refWithoutDigest := expanded - if atIdx := strings.LastIndex(expanded, "@"); atIdx >= 0 { - refWithoutDigest = expanded[:atIdx] + if fromValue == "" { + inst.Skip = true + inst.SkipReason = SkipMissingRef + return inst, true } - // Stage names don't contain "/" or ":" or "." - refLower := strings.ToLower(refWithoutDigest) - if stageNames[refLower] { + + // BuildKit reads the value as a stage index before anything else, so a numeric + // --from always selects a build stage by position and never names an image. + if isStageIndex(fromValue) { inst.Skip = true - inst.SkipReason = "stage reference" - inst.ImageRef = expanded - return inst + inst.SkipReason = SkipStageIndex + return inst, true } - // If expansion produced unresolved variables, skip - if hasUnresolved { + // Unlike FROM, whose base name is expanded before it is resolved, the --from value + // is read verbatim: CopyCommand.Expand covers --chown, --chmod and the paths, but + // not --from. A variable written here makes the build fail to parse the stage name + // (moby/buildkit#2374), so the value is left alone rather than pinned to whatever + // the variable would have expanded to. + if strings.ContainsRune(fromValue, '$') { inst.Skip = true - inst.SkipReason = "unresolved ARG variable" - inst.ImageRef = expanded - return inst + inst.SkipReason = SkipCopyFromVar + return inst, true } - // Split digest at "@" if present - if atIdx := strings.LastIndex(expanded, "@"); atIdx >= 0 { - inst.ImageRef = expanded[:atIdx] - inst.Digest = expanded[atIdx+1:] - } else { - inst.ImageRef = expanded + inst.ImageRef, inst.Digest, inst.SkipReason, inst.Skip = classifyRef(fromValue, stageNames) + return inst, true +} + +// copyFromValue returns the value of the --from= flag among a COPY node's flags. +func copyFromValue(flags []string) (string, bool) { + for _, flag := range flags { + if value, ok := strings.CutPrefix(flag, copyFromFlag); ok { + return value, true + } } + return "", false +} - return inst +// isStageIndex reports whether a --from value selects a build stage by position. +// It mirrors BuildKit, which classifies the value with strconv.Atoi. +func isStageIndex(s string) bool { + _, err := strconv.Atoi(s) + return err == nil +} + +// resolveRef expands ARG variables in rawRef, as BuildKit does for a FROM base name, +// and classifies the result. It returns the image ref, any digest already written on +// it, and why it should be skipped (if it should). +func resolveRef(rawRef string, argDefaults map[string]string, stageNames map[string]bool) (imageRef, digest, skipReason string, skip bool) { + expanded, hasUnresolved := expandVars(rawRef, argDefaults) + if hasUnresolved { + // Still classified first: a name that is scratch or a stage is recognisable + // even when some other part of the ref did not expand. + if ref, dgst, reason, skip := classifyRef(expanded, stageNames); skip { + return ref, dgst, reason, skip + } + return expanded, "", SkipUnresolvedARG, true + } + return classifyRef(expanded, stageNames) +} + +// classifyRef sorts an image ref that needs no further expansion into a pinnable +// image, a stage reference, or scratch, splitting off any digest it already carries. +func classifyRef(ref string, stageNames map[string]bool) (imageRef, digest, skipReason string, skip bool) { + if strings.EqualFold(ref, "scratch") { + return ref, "", SkipScratch, true + } + + // Strip any digest before comparing against stage names, which never carry one. + refWithoutDigest := ref + if atIdx := strings.LastIndex(ref, "@"); atIdx >= 0 { + refWithoutDigest = ref[:atIdx] + } + if stageNames[strings.ToLower(refWithoutDigest)] { + return ref, "", SkipStageRef, true + } + + if atIdx := strings.LastIndex(ref, "@"); atIdx >= 0 { + return ref[:atIdx], ref[atIdx+1:], "", false + } + return ref, "", "", false } // expandVars expands ${VAR} and $VAR syntax using the provided defaults map. diff --git a/internal/dockerfile/parse_test.go b/internal/dockerfile/parse_test.go index cf73cfe..5b1a48a 100644 --- a/internal/dockerfile/parse_test.go +++ b/internal/dockerfile/parse_test.go @@ -173,3 +173,324 @@ func TestExpandVars(t *testing.T) { } } } + +// wantInst is the expected shape of one parsed instruction. +type wantInst struct { + imageRef string + rawRef string + digest string + isCopyFrom bool + skip bool + skipReason string + startLine int +} + +func checkInstructions(t *testing.T, input string, want []wantInst) { + t.Helper() + got, err := Parse(strings.NewReader(input)) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + if len(got) != len(want) { + t.Fatalf("got %d instructions, want %d: %+v", len(got), len(want), got) + } + for i, w := range want { + inst := got[i] + if inst.ImageRef != w.imageRef { + t.Errorf("[%d] ImageRef = %q, want %q", i, inst.ImageRef, w.imageRef) + } + if w.rawRef != "" && inst.RawRef != w.rawRef { + t.Errorf("[%d] RawRef = %q, want %q", i, inst.RawRef, w.rawRef) + } + if inst.Digest != w.digest { + t.Errorf("[%d] Digest = %q, want %q", i, inst.Digest, w.digest) + } + if inst.IsCopyFrom != w.isCopyFrom { + t.Errorf("[%d] IsCopyFrom = %v, want %v", i, inst.IsCopyFrom, w.isCopyFrom) + } + if inst.Skip != w.skip { + t.Errorf("[%d] Skip = %v, want %v (reason %q)", i, inst.Skip, w.skip, inst.SkipReason) + } + if inst.SkipReason != w.skipReason { + t.Errorf("[%d] SkipReason = %q, want %q", i, inst.SkipReason, w.skipReason) + } + if w.startLine != 0 && inst.StartLine != w.startLine { + t.Errorf("[%d] StartLine = %d, want %d", i, inst.StartLine, w.startLine) + } + } +} + +// TestParse_CopyFrom covers how each form of `COPY --from=` is classified. The +// reference forms follow the Dockerfile reference, which defines --from as naming +// "an image, a build stage, or a named context". +func TestParse_CopyFrom(t *testing.T) { + tests := []struct { + name string + input string + want []wantInst + }{ + { + // The reproduction from the feature request. + name: "external image with tag alongside a stage index", + input: "FROM ubuntu:24.04\nCOPY --from=nginx:1.27 /etc/nginx /etc/nginx\nCOPY --from=0 /a /b\n", + want: []wantInst{ + {imageRef: "ubuntu:24.04", rawRef: "ubuntu:24.04", startLine: 1}, + {imageRef: "nginx:1.27", rawRef: "nginx:1.27", isCopyFrom: true, startLine: 2}, + {imageRef: "0", rawRef: "0", isCopyFrom: true, skip: true, skipReason: SkipStageIndex, startLine: 3}, + }, + }, + { + name: "stage declared above", + input: "FROM golang:1.22 AS builder\nCOPY --from=builder /app /app\n", + want: []wantInst{ + {imageRef: "golang:1.22", startLine: 1}, + {imageRef: "builder", isCopyFrom: true, skip: true, skipReason: SkipStageRef, startLine: 2}, + }, + }, + { + // BuildKit looks stage names up with strings.ToLower, so case does not matter. + name: "stage name match is case-insensitive", + input: "FROM golang:1.22 AS Builder\nCOPY --from=BUILDER /app /app\n", + want: []wantInst{ + {imageRef: "golang:1.22", startLine: 1}, + {imageRef: "BUILDER", isCopyFrom: true, skip: true, skipReason: SkipStageRef, startLine: 2}, + }, + }, + { + // Naming a stage declared below is a build error about stage order + // ("cannot copy from stage ... it needs to be defined before current + // stage"), not an unpinned image: the name is still a stage, so there is + // nothing here to rewrite. + name: "stage declared below", + input: "FROM alpine:3.19 AS base\nCOPY --from=late /bin/tool /usr/local/bin/tool\nFROM ubuntu:24.04 AS late\n", + want: []wantInst{ + {imageRef: "alpine:3.19", startLine: 1}, + {imageRef: "late", isCopyFrom: true, skip: true, skipReason: SkipStageRef, startLine: 2}, + {imageRef: "ubuntu:24.04", startLine: 3}, + }, + }, + { + name: "already pinned", + input: "COPY --from=nginx:1.27@sha256:abc123 /etc/nginx /etc/nginx\n", + want: []wantInst{ + {imageRef: "nginx:1.27", rawRef: "nginx:1.27@sha256:abc123", digest: "sha256:abc123", isCopyFrom: true}, + }, + }, + { + name: "pinned without a tag", + input: "COPY --from=nginx@sha256:abc123 /etc/nginx /etc/nginx\n", + want: []wantInst{ + {imageRef: "nginx", rawRef: "nginx@sha256:abc123", digest: "sha256:abc123", isCopyFrom: true}, + }, + }, + { + // CopyCommand.Expand expands --chown, --chmod and the paths but not --from, + // so BuildKit reads the value verbatim and the build fails to parse the + // stage name. FROM on the same file still expands, which is the asymmetry + // this case pins down. + name: "variable in --from is not expanded, unlike FROM", + input: "ARG NGINX_VERSION=1.27\nFROM nginx:${NGINX_VERSION}\nCOPY --from=nginx:${NGINX_VERSION} /etc/nginx /etc/nginx\n", + want: []wantInst{ + {imageRef: "nginx:1.27", rawRef: "nginx:${NGINX_VERSION}", startLine: 2}, + {imageRef: "nginx:${NGINX_VERSION}", isCopyFrom: true, skip: true, skipReason: SkipCopyFromVar, startLine: 3}, + }, + }, + { + name: "bare $VAR in --from is not expanded either", + input: "ARG NGINX_IMAGE=nginx:1.27\nFROM ubuntu:24.04\nCOPY --from=$NGINX_IMAGE /etc/nginx /etc/nginx\n", + want: []wantInst{ + {imageRef: "ubuntu:24.04", startLine: 2}, + {imageRef: "$NGINX_IMAGE", isCopyFrom: true, skip: true, skipReason: SkipCopyFromVar, startLine: 3}, + }, + }, + { + name: "scratch has nothing to copy from a registry", + input: "COPY --from=scratch /a /b\n", + want: []wantInst{ + {imageRef: "scratch", isCopyFrom: true, skip: true, skipReason: SkipScratch}, + }, + }, + { + name: "registry with a port", + input: "COPY --from=registry.example.com:5000/tool:1.0 /tool /tool\n", + want: []wantInst{ + {imageRef: "registry.example.com:5000/tool:1.0", isCopyFrom: true}, + }, + }, + { + name: "surrounded by other COPY flags", + input: "COPY --chown=65532:65532 --from=gcr.io/distroless/base:nonroot --chmod=755 /etc/passwd /etc/passwd\n", + want: []wantInst{ + {imageRef: "gcr.io/distroless/base:nonroot", isCopyFrom: true}, + }, + }, + { + name: "boolean flags do not hide --from", + input: "COPY --link --parents --from=busybox:1.36 /bin/ /bin/\n", + want: []wantInst{ + {imageRef: "busybox:1.36", isCopyFrom: true}, + }, + }, + { + // A bare name that matches no stage is an image to BuildKit, which is also + // how `FROM ubuntu` is treated. A named build context supplied at build time + // (--build-context name=...) is written the same way and cannot be told + // apart from the Dockerfile alone; --ignore-images excludes those. + name: "bare name that matches no stage is an image", + input: "COPY --from=ubuntu /etc/os-release /etc/os-release\n", + want: []wantInst{ + {imageRef: "ubuntu", isCopyFrom: true}, + }, + }, + { + name: "instruction keyword is case-insensitive", + input: "copy --from=nginx:1.27 /etc/nginx /etc/nginx\n", + want: []wantInst{ + {imageRef: "nginx:1.27", isCopyFrom: true}, + }, + }, + { + name: "several COPY instructions from the same image", + input: "COPY --from=nginx:1.27 /etc/nginx /etc/nginx\nCOPY --from=nginx:1.27 /usr/share/nginx /usr/share/nginx\n", + want: []wantInst{ + {imageRef: "nginx:1.27", isCopyFrom: true, startLine: 1}, + {imageRef: "nginx:1.27", isCopyFrom: true, startLine: 2}, + }, + }, + { + name: "plain COPY reads the build context", + input: "FROM alpine:3.19\nCOPY /src /dst\nCOPY . .\n", + want: []wantInst{ + {imageRef: "alpine:3.19", startLine: 1}, + }, + }, + { + // BuildKit matches flag names case-sensitively, so --FROM is an unknown + // flag and `docker build` rejects the Dockerfile outright. + name: "--FROM is not the --from flag", + input: "COPY --FROM=nginx:1.27 /a /b\n", + want: nil, + }, + { + // ADD accepts --keep-git-dir, --checksum, --chmod, --chown, --link, + // --unpack and --exclude; --from belongs to COPY alone. + name: "ADD has no --from flag", + input: "ADD --from=nginx:1.27 /a /b\n", + want: nil, + }, + { + // The image is named inside the --mount value, not by a --from flag. + name: "RUN --mount=from= is a different flag", + input: "RUN --mount=type=bind,from=nginx:1.27,source=/a,target=/b true\n", + want: nil, + }, + { + name: "empty --from value", + input: "COPY --from= /a /b\n", + want: []wantInst{ + {imageRef: "", isCopyFrom: true, skip: true, skipReason: SkipMissingRef}, + }, + }, + { + // BuildKit classifies the value with strconv.Atoi before anything else, so + // every value it accepts selects a stage by position. + name: "a signed number is still a stage index", + input: "COPY --from=-1 /a /b\n", + want: []wantInst{ + {imageRef: "-1", isCopyFrom: true, skip: true, skipReason: SkipStageIndex}, + }, + }, + { + name: "leading zeros are still a stage index", + input: "COPY --from=007 /a /b\n", + want: []wantInst{ + {imageRef: "007", isCopyFrom: true, skip: true, skipReason: SkipStageIndex}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + checkInstructions(t, tt.input, tt.want) + }) + } +} + +// TestParse_CopyFromLineSpan records the line range of an instruction continued +// with "\", which is what the rewriter searches for the reference. +func TestParse_CopyFromLineSpan(t *testing.T) { + input := "FROM ubuntu:24.04\nCOPY \\\n --from=nginx:1.27 \\\n /etc/nginx /etc/nginx\n" + instructions, err := Parse(strings.NewReader(input)) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + if len(instructions) != 2 { + t.Fatalf("expected 2 instructions, got %d", len(instructions)) + } + copyInst := instructions[1] + if copyInst.ImageRef != "nginx:1.27" { + t.Errorf("ImageRef = %q, want %q", copyInst.ImageRef, "nginx:1.27") + } + if copyInst.StartLine != 2 { + t.Errorf("StartLine = %d, want 2", copyInst.StartLine) + } + if copyInst.EndLine != 4 { + t.Errorf("EndLine = %d, want 4", copyInst.EndLine) + } +} + +// TestParse_ARGScopeIsSequential guards the scoping rule that an ARG below a FROM +// belongs to the stage that FROM opens, so it cannot expand that FROM's own ref. +func TestParse_ARGScopeIsSequential(t *testing.T) { + input := "FROM python:${VERSION}\nARG VERSION=3.12\nCOPY --from=nginx:1.27 /a /b\n" + checkInstructions(t, input, []wantInst{ + {imageRef: "python:${VERSION}", skip: true, skipReason: SkipUnresolvedARG, startLine: 1}, + {imageRef: "nginx:1.27", isCopyFrom: true, startLine: 3}, + }) +} + +// TestParse_FromStageReferenceStaysSequential guards the FROM rule that differs from +// COPY: BuildKit resolves a FROM against the stages declared above it, so a name +// defined later is an image, not a stage reference. +func TestParse_FromStageReferenceStaysSequential(t *testing.T) { + input := "FROM builder\nFROM golang:1.22 AS builder\nCOPY --from=builder /app /app\n" + checkInstructions(t, input, []wantInst{ + {imageRef: "builder", startLine: 1}, + {imageRef: "golang:1.22", startLine: 2}, + {imageRef: "builder", isCopyFrom: true, skip: true, skipReason: SkipStageRef, startLine: 3}, + }) +} + +// TestParse_ScratchThroughARG covers scratch reached by variable expansion. A FROM +// expands to it and is recognised; the COPY value is never expanded, so it is held +// back for that reason instead. +func TestParse_ScratchThroughARG(t *testing.T) { + input := "ARG BASE=scratch\nFROM ${BASE}\nCOPY --from=${BASE} /a /b\n" + checkInstructions(t, input, []wantInst{ + {imageRef: "scratch", rawRef: "${BASE}", skip: true, skipReason: SkipScratch, startLine: 2}, + {imageRef: "${BASE}", rawRef: "${BASE}", isCopyFrom: true, skip: true, skipReason: SkipCopyFromVar, startLine: 3}, + }) +} + +func TestIsStageIndex(t *testing.T) { + tests := []struct { + in string + want bool + }{ + {"0", true}, + {"12", true}, + {"007", true}, + {"-1", true}, + {"+1", true}, + {"", false}, + {"1.0", false}, + {"nginx", false}, + {"nginx:1.27", false}, + {"1nginx", false}, + } + for _, tt := range tests { + if got := isStageIndex(tt.in); got != tt.want { + t.Errorf("isStageIndex(%q) = %v, want %v", tt.in, got, tt.want) + } + } +} diff --git a/internal/dockerfile/rewrite.go b/internal/dockerfile/rewrite.go index a3dbfc2..3c89c2c 100644 --- a/internal/dockerfile/rewrite.go +++ b/internal/dockerfile/rewrite.go @@ -4,13 +4,33 @@ import "strings" // AddDigest inserts or replaces a digest in a FROM line. func AddDigest(original string, rawRef string, digest string) string { + return addDigestAfter(original, "", rawRef, digest) +} + +// AddCopyFromDigest inserts or replaces a digest in the --from= flag of a COPY line. +// The flag is matched together with the reference so that the same text appearing +// elsewhere on the line — in another flag's value, or in a source path — is left alone. +func AddCopyFromDigest(original string, rawRef string, digest string) string { + return addDigestAfter(original, copyFromFlag, rawRef, digest) +} + +// addDigestAfter rewrites the first occurrence of prefix+rawRef in line, replacing any +// digest rawRef already carries. It returns line unchanged when the reference is not +// found there, which is how RewriteFile detects that it must look on a continuation line. +func addDigestAfter(line string, prefix string, rawRef string, digest string) string { + if rawRef == "" { + return line + } + idx := strings.Index(line, prefix+rawRef) + if idx < 0 { + return line + } + baseRef := rawRef if atIdx := strings.Index(rawRef, "@"); atIdx >= 0 { - baseRef := rawRef[:atIdx] - newRef := baseRef + "@" + digest - return strings.Replace(original, rawRef, newRef, 1) + baseRef = rawRef[:atIdx] } - newRef := rawRef + "@" + digest - return strings.Replace(original, rawRef, newRef, 1) + end := idx + len(prefix) + len(rawRef) + return line[:idx] + prefix + baseRef + "@" + digest + line[end:] } // RewriteFile applies digest pins to a Dockerfile's content. @@ -22,10 +42,32 @@ func RewriteFile(content string, instructions []FromInstruction, digests map[int if !ok || inst.Skip { continue } - lineIdx := inst.StartLine - 1 - if lineIdx >= 0 && lineIdx < len(lines) { - lines[lineIdx] = AddDigest(lines[lineIdx], inst.RawRef, digest) + prefix := "" + if inst.IsCopyFrom { + prefix = copyFromFlag + } + // An instruction continued with "\" spans several lines and the reference is + // not always written on the first one, so every line it covers is tried until + // one of them changes. + for lineIdx := inst.StartLine - 1; lineIdx <= lastLineIndex(inst) && lineIdx < len(lines); lineIdx++ { + if lineIdx < 0 { + continue + } + rewritten := addDigestAfter(lines[lineIdx], prefix, inst.RawRef, digest) + if rewritten != lines[lineIdx] { + lines[lineIdx] = rewritten + break + } } } return strings.Join(lines, "\n") } + +// lastLineIndex returns the 0-based index of the last line an instruction covers. +// EndLine is absent on instructions built by hand, so it falls back to the start line. +func lastLineIndex(inst FromInstruction) int { + if inst.EndLine > inst.StartLine { + return inst.EndLine - 1 + } + return inst.StartLine - 1 +} diff --git a/internal/dockerfile/rewrite_test.go b/internal/dockerfile/rewrite_test.go index f3982d0..21f83e5 100644 --- a/internal/dockerfile/rewrite_test.go +++ b/internal/dockerfile/rewrite_test.go @@ -1,6 +1,9 @@ package dockerfile -import "testing" +import ( + "strings" + "testing" +) func TestAddDigest(t *testing.T) { tests := []struct { @@ -81,3 +84,203 @@ func TestRewriteFile(t *testing.T) { t.Errorf("RewriteFile() =\n%s\nwant:\n%s", got, want) } } + +func TestAddCopyFromDigest(t *testing.T) { + tests := []struct { + name string + original string + rawRef string + digest string + want string + }{ + { + name: "simple tag", + original: "COPY --from=nginx:1.27 /etc/nginx /etc/nginx", + rawRef: "nginx:1.27", + digest: "sha256:abc123", + want: "COPY --from=nginx:1.27@sha256:abc123 /etc/nginx /etc/nginx", + }, + { + name: "replaces an existing digest", + original: "COPY --from=nginx:1.27@sha256:olddigest /etc/nginx /etc/nginx", + rawRef: "nginx:1.27@sha256:olddigest", + digest: "sha256:newdigest", + want: "COPY --from=nginx:1.27@sha256:newdigest /etc/nginx /etc/nginx", + }, + { + name: "other flags are left alone", + original: "COPY --chown=65532:65532 --from=gcr.io/distroless/base:nonroot --chmod=755 /etc/passwd /etc/passwd", + rawRef: "gcr.io/distroless/base:nonroot", + digest: "sha256:abc123", + want: "COPY --chown=65532:65532 --from=gcr.io/distroless/base:nonroot@sha256:abc123 --chmod=755 /etc/passwd /etc/passwd", + }, + { + // Anchoring on --from= is what keeps the earlier --chown value intact: + // a plain first-occurrence replacement would rewrite that instead. + name: "the same text in an earlier flag is not the reference", + original: "COPY --chown=node:20 --from=node:20 /app /app", + rawRef: "node:20", + digest: "sha256:abc123", + want: "COPY --chown=node:20 --from=node:20@sha256:abc123 /app /app", + }, + { + name: "registry with a port keeps its colon", + original: "COPY --from=registry.example.com:5000/tool:1.0 /tool /tool", + rawRef: "registry.example.com:5000/tool:1.0", + digest: "sha256:abc123", + want: "COPY --from=registry.example.com:5000/tool:1.0@sha256:abc123 /tool /tool", + }, + { + name: "reference absent from the line", + original: "COPY --from=nginx:1.27 /etc/nginx /etc/nginx", + rawRef: "alpine:3.19", + digest: "sha256:abc123", + want: "COPY --from=nginx:1.27 /etc/nginx /etc/nginx", + }, + { + name: "empty reference", + original: "COPY --from= /a /b", + rawRef: "", + digest: "sha256:abc123", + want: "COPY --from= /a /b", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := AddCopyFromDigest(tt.original, tt.rawRef, tt.digest); got != tt.want { + t.Errorf("AddCopyFromDigest() = %q, want %q", got, tt.want) + } + }) + } +} + +// TestRewriteFile_IssueReproduction pins the Dockerfile from the feature request and +// compares the whole file, including the parts that must not change. +func TestRewriteFile_IssueReproduction(t *testing.T) { + content := "# Dockerfile\nFROM ubuntu:24.04\nCOPY --from=nginx:1.27 /etc/nginx /etc/nginx\nCOPY --from=0 /a /b\n" + instructions, err := Parse(strings.NewReader(content)) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + digests := map[int]string{ + 0: "sha256:33ceb71981b602c1a7443a53469e4dba065f7503eab3078a2d7a57a2ab987517", + 1: "sha256:6784fb08b4b7c3b6bcd3f4a1b4d1b1f3e3b7a7ca42ec3e0d9df8a97a2c9a3b1d", + } + want := "# Dockerfile\n" + + "FROM ubuntu:24.04@sha256:33ceb71981b602c1a7443a53469e4dba065f7503eab3078a2d7a57a2ab987517\n" + + "COPY --from=nginx:1.27@sha256:6784fb08b4b7c3b6bcd3f4a1b4d1b1f3e3b7a7ca42ec3e0d9df8a97a2c9a3b1d /etc/nginx /etc/nginx\n" + + "COPY --from=0 /a /b\n" + if got := RewriteFile(content, instructions, digests); got != want { + t.Errorf("RewriteFile() =\n%s\nwant:\n%s", got, want) + } +} + +// TestRewriteFile_CopyFromMultiStage checks a realistic multi-stage file: external +// images are pinned while stage references, stage indexes and plain COPY are not. +func TestRewriteFile_CopyFromMultiStage(t *testing.T) { + content := "FROM golang:1.22 AS builder\n" + + "RUN go build -o /app\n" + + "FROM gcr.io/distroless/base:nonroot\n" + + "COPY --from=builder /app /app\n" + + "COPY --from=0 /go/bin/tool /usr/local/bin/tool\n" + + "COPY --chown=65532:65532 --from=nginx:1.27 /etc/nginx /etc/nginx\n" + + "COPY ./config /config\n" + instructions, err := Parse(strings.NewReader(content)) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + digests := map[int]string{ + 0: "sha256:golang111", + 1: "sha256:distroless222", + // index 2 is the stage reference, index 3 the stage index: both skipped. + 4: "sha256:nginx333", + } + want := "FROM golang:1.22@sha256:golang111 AS builder\n" + + "RUN go build -o /app\n" + + "FROM gcr.io/distroless/base:nonroot@sha256:distroless222\n" + + "COPY --from=builder /app /app\n" + + "COPY --from=0 /go/bin/tool /usr/local/bin/tool\n" + + "COPY --chown=65532:65532 --from=nginx:1.27@sha256:nginx333 /etc/nginx /etc/nginx\n" + + "COPY ./config /config\n" + if got := RewriteFile(content, instructions, digests); got != want { + t.Errorf("RewriteFile() =\n%s\nwant:\n%s", got, want) + } +} + +// TestRewriteFile_LineContinuation covers instructions continued with "\", where the +// reference sits on a line below the one the instruction starts on. +func TestRewriteFile_LineContinuation(t *testing.T) { + content := "FROM \\\n ubuntu:24.04\n" + + "COPY \\\n --from=nginx:1.27 \\\n /etc/nginx /etc/nginx\n" + instructions, err := Parse(strings.NewReader(content)) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + digests := map[int]string{0: "sha256:ubuntu111", 1: "sha256:nginx222"} + want := "FROM \\\n ubuntu:24.04@sha256:ubuntu111\n" + + "COPY \\\n --from=nginx:1.27@sha256:nginx222 \\\n /etc/nginx /etc/nginx\n" + if got := RewriteFile(content, instructions, digests); got != want { + t.Errorf("RewriteFile() =\n%s\nwant:\n%s", got, want) + } +} + +// TestRewriteFile_CopyFromUpdatesExistingDigest covers the --update path, where an +// already-pinned COPY --from is re-resolved to a new digest. +func TestRewriteFile_CopyFromUpdatesExistingDigest(t *testing.T) { + content := "FROM ubuntu:24.04@sha256:oldubuntu\nCOPY --from=nginx:1.27@sha256:oldnginx /etc/nginx /etc/nginx\n" + instructions, err := Parse(strings.NewReader(content)) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + digests := map[int]string{0: "sha256:newubuntu", 1: "sha256:newnginx"} + want := "FROM ubuntu:24.04@sha256:newubuntu\nCOPY --from=nginx:1.27@sha256:newnginx /etc/nginx /etc/nginx\n" + if got := RewriteFile(content, instructions, digests); got != want { + t.Errorf("RewriteFile() =\n%s\nwant:\n%s", got, want) + } +} + +// TestRewriteFile_RepeatedCopyFrom checks that two COPY instructions using the same +// image are pinned on their own lines rather than one line twice. +func TestRewriteFile_RepeatedCopyFrom(t *testing.T) { + content := "COPY --from=nginx:1.27 /etc/nginx /etc/nginx\nCOPY --from=nginx:1.27 /usr/share/nginx /usr/share/nginx\n" + instructions, err := Parse(strings.NewReader(content)) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + digests := map[int]string{0: "sha256:nginx111", 1: "sha256:nginx111"} + want := "COPY --from=nginx:1.27@sha256:nginx111 /etc/nginx /etc/nginx\nCOPY --from=nginx:1.27@sha256:nginx111 /usr/share/nginx /usr/share/nginx\n" + if got := RewriteFile(content, instructions, digests); got != want { + t.Errorf("RewriteFile() =\n%s\nwant:\n%s", got, want) + } +} + +// TestRewriteFile_SkippedCopyFromUntouched checks that a digest handed in for a +// skipped instruction never reaches the file. +func TestRewriteFile_SkippedCopyFromUntouched(t *testing.T) { + content := "FROM golang:1.22 AS builder\nCOPY --from=builder /app /app\nCOPY --from=scratch /a /b\n" + instructions, err := Parse(strings.NewReader(content)) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + digests := map[int]string{0: "sha256:golang111", 1: "sha256:bogus", 2: "sha256:bogus"} + want := "FROM golang:1.22@sha256:golang111 AS builder\nCOPY --from=builder /app /app\nCOPY --from=scratch /a /b\n" + if got := RewriteFile(content, instructions, digests); got != want { + t.Errorf("RewriteFile() =\n%s\nwant:\n%s", got, want) + } +} + +// TestRewriteFile_CopyFromAnchoredToFlag guards the whole-file path against +// rewriting text that merely looks like the reference: here the image ref is also +// the value of an earlier --chown flag, and only the --from= one may gain a digest. +func TestRewriteFile_CopyFromAnchoredToFlag(t *testing.T) { + content := "FROM node:20 AS builder\nCOPY --chown=node:20 --from=node:20 /app /app\n" + instructions, err := Parse(strings.NewReader(content)) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + digests := map[int]string{0: "sha256:node111", 1: "sha256:node222"} + want := "FROM node:20@sha256:node111 AS builder\nCOPY --chown=node:20 --from=node:20@sha256:node222 /app /app\n" + if got := RewriteFile(content, instructions, digests); got != want { + t.Errorf("RewriteFile() =\n%s\nwant:\n%s", got, want) + } +} diff --git a/testdata/copy_from.Dockerfile b/testdata/copy_from.Dockerfile new file mode 100644 index 0000000..5d56a95 --- /dev/null +++ b/testdata/copy_from.Dockerfile @@ -0,0 +1,24 @@ +# Multi-stage build that also copies from external images. +FROM golang:1.22 AS builder +RUN go build -o /app + +FROM gcr.io/distroless/base-debian12:nonroot + +# from an earlier stage, by name and by index +COPY --from=builder /app /app +COPY --from=0 /go/bin/tool /usr/local/bin/tool + +# from external images +COPY --from=nginx:1.27 /etc/nginx /etc/nginx +COPY --chown=65532:65532 --from=busybox:1.36 /bin/busybox /bin/busybox +COPY --from=registry.example.com:5000/tool:1.0 /tool /usr/local/bin/tool2 + +# already pinned +COPY --from=alpine:3.19@sha256:aaaa1111 /etc/alpine-release /etc/alpine-release + +# BuildKit does not expand variables in --from +ARG NGINX_VERSION=1.27 +COPY --from=nginx:${NGINX_VERSION} /etc/nginx /etc/nginx.orig + +# from the build context +COPY ./config /config From f1177ecac13f632e10874f77c5683b5d16b03e55 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 15:52:06 +0000 Subject: [PATCH 2/4] fix: address Codex review findings on COPY --from pinning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects Codex flagged, each confirmed against the BuildKit sources this project parses with. ONBUILD COPY --from was invisible. The parser hangs the wrapped instruction off the ONBUILD node instead of listing it at the top level, so scanning only top-level nodes missed it entirely: `check` said nothing and `run` never pinned it. The wrapped instruction is now unwrapped and given the ONBUILD node's line span and source text, which the parser leaves unset on the child. A reference split by a "\" continuation was silently not rewritten. Searching each physical line on its own cannot match `COPY --from=\` + newline + `nginx:1.27`, because no line holds the text the parser reports — so the digest resolved, the file did not change, and `run` still counted the image as pinned. The rewrite now runs over the instruction as the parser joins it, mapping each byte back to the line and column it came from, so the digest lands in the right place however the instruction is broken up. That covers a split value, a split flag name, and an existing digest spanning the continuation. Belt and braces for the same class of bug: RewriteFileReport returns the instructions it was handed a digest for but could not rewrite, and `run` warns about them and leaves them out of its "pinned N image(s)" count rather than overstating what it did. A digest no longer turns an image into a stage reference. BuildKit matches the whole value against its stage names, so `COPY --from=nginx@sha256:...` finds no stage named `nginx` and resolves from the registry; stripping the digest before the lookup marked it as a stage, so `check` never verified it and `run --update` could not refresh it. The same rule governs FROM, where the stripping was equally wrong. Each fix is covered by tests that fail when it is reverted, plus an ONBUILD trigger in the testdata round trip. Verified against the real registry: an ONBUILD COPY --from and a split reference both pin and then check clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CERP67PPQ72qxDHKqpXkcR --- README.md | 5 + cmd/pin.go | 9 +- e2e_test.go | 2 + internal/dockerfile/parse.go | 31 +++++-- internal/dockerfile/parse_test.go | 67 ++++++++++++++ internal/dockerfile/rewrite.go | 139 +++++++++++++++++++++++----- internal/dockerfile/rewrite_test.go | 117 +++++++++++++++++++++++ testdata/copy_from.Dockerfile | 3 + 8 files changed, 341 insertions(+), 32 deletions(-) diff --git a/README.md b/README.md index 4685bb9..adc8b62 100644 --- a/README.md +++ b/README.md @@ -306,8 +306,13 @@ ignore-images: | `COPY --from=scratch` | Skipped | | `COPY --from=image:${TAG}` | Skipped (BuildKit does not expand variables here) | | `COPY /src /dst` (build context) | Nothing to pin | +| `ONBUILD COPY --from=image:tag` | Yes | | `ADD --from=...`, `RUN --mount=...,from=...` | Not supported | +A digest makes a name an image even when a build stage shares it: BuildKit matches the +whole value against its stage names, so `COPY --from=nginx@sha256:...` finds no stage +named `nginx` and is resolved from the registry. The same holds for `FROM`. + A `COPY --from=` that matches no build stage is treated as an image, the same way `FROM ubuntu` is. A [named build context](https://docs.docker.com/reference/cli/docker/buildx/build/#build-context) (`docker buildx build --build-context name=...`) is written the same way and cannot diff --git a/cmd/pin.go b/cmd/pin.go index 998a029..373e938 100644 --- a/cmd/pin.go +++ b/cmd/pin.go @@ -303,7 +303,12 @@ func applyDockerfile(pf parsedFile, digestMap map[string]string, dryRun bool, up if len(digests) == 0 { return } - result := dockerfile.RewriteFile(string(pf.content), pf.dockerInsts, digests) + result, unrewritten := dockerfile.RewriteFileReport(string(pf.content), pf.dockerInsts, digests) + for _, i := range unrewritten { + inst := pf.dockerInsts[i] + fmt.Fprintf(os.Stderr, "WARN %s:%d %s reference not found in the source line, left unchanged\n", + pf.path, inst.StartLine, inst.ImageRef) + } if dryRun { fmt.Printf("--- %s\n", pf.path) fmt.Print(result) @@ -313,7 +318,7 @@ func applyDockerfile(pf parsedFile, digestMap map[string]string, dryRun bool, up fmt.Fprintf(os.Stderr, "error writing %s: %v\n", pf.path, err) return } - fmt.Printf("pinned %d image(s) in %s\n", len(digests), pf.path) + fmt.Printf("pinned %d image(s) in %s\n", len(digests)-len(unrewritten), pf.path) } func applyActions(pf parsedFile, digestMap map[string]string, dryRun bool, update bool) { diff --git a/e2e_test.go b/e2e_test.go index 55e2102..c156bf7 100644 --- a/e2e_test.go +++ b/e2e_test.go @@ -945,6 +945,7 @@ func TestPinCopyFromTestdataRoundTrip(t *testing.T) { "nginx:1.27": "sha256:nginx333", "busybox:1.36": "sha256:busybox444", "registry.example.com:5000/tool:1.0": "sha256:tool555", + "curlimages/curl:8.11.1": "sha256:curl666", }, } @@ -977,6 +978,7 @@ func TestPinCopyFromTestdataRoundTrip(t *testing.T) { "COPY --from=nginx:1.27@sha256:nginx333 /etc/nginx /etc/nginx", "COPY --chown=65532:65532 --from=busybox:1.36@sha256:busybox444 /bin/busybox /bin/busybox", "COPY --from=registry.example.com:5000/tool:1.0@sha256:tool555 /tool /usr/local/bin/tool2", + "ONBUILD COPY --from=curlimages/curl:8.11.1@sha256:curl666 /usr/bin/curl /usr/bin/curl", } { if !strings.Contains(got, want) { t.Errorf("expected pinned line %q\nin:\n%s", want, got) diff --git a/internal/dockerfile/parse.go b/internal/dockerfile/parse.go index ad61c7e..c9c3832 100644 --- a/internal/dockerfile/parse.go +++ b/internal/dockerfile/parse.go @@ -78,12 +78,34 @@ func Parse(r io.Reader) ([]FromInstruction, error) { if inst, ok := parseCopyFromNode(node, allStages); ok { instructions = append(instructions, inst) } + case "onbuild": + // The wrapped instruction runs when a later build uses this image as its + // base, and a COPY --from inside one names a real image all the same. + if child := onbuildChild(node); child != nil && strings.ToLower(child.Value) == "copy" { + if inst, ok := parseCopyFromNode(child, allStages); ok { + instructions = append(instructions, inst) + } + } } } return instructions, nil } +// onbuildChild returns the instruction an ONBUILD wraps, or nil if it wraps nothing. +// The parser hangs that instruction off an unnamed node and leaves its position unset, +// so the ONBUILD node supplies the line span and source text the rewriter needs. +func onbuildChild(node *parser.Node) *parser.Node { + if node.Next == nil || len(node.Next.Children) != 1 { + return nil + } + child := *node.Next.Children[0] + child.StartLine = node.StartLine + child.EndLine = node.EndLine + child.Original = node.Original + return &child +} + // collectStageNames returns every name introduced by a "FROM ... AS " clause, // lowercased because BuildKit looks stage names up case-insensitively. func collectStageNames(root *parser.Node) map[string]bool { @@ -234,12 +256,9 @@ func classifyRef(ref string, stageNames map[string]bool) (imageRef, digest, skip return ref, "", SkipScratch, true } - // Strip any digest before comparing against stage names, which never carry one. - refWithoutDigest := ref - if atIdx := strings.LastIndex(ref, "@"); atIdx >= 0 { - refWithoutDigest = ref[:atIdx] - } - if stageNames[strings.ToLower(refWithoutDigest)] { + // BuildKit matches the whole value against its stage names, digest included, so + // "builder@sha256:..." finds no stage named "builder" and is an image reference. + if stageNames[strings.ToLower(ref)] { return ref, "", SkipStageRef, true } diff --git a/internal/dockerfile/parse_test.go b/internal/dockerfile/parse_test.go index 5b1a48a..436222a 100644 --- a/internal/dockerfile/parse_test.go +++ b/internal/dockerfile/parse_test.go @@ -400,6 +400,42 @@ func TestParse_CopyFrom(t *testing.T) { {imageRef: "-1", isCopyFrom: true, skip: true, skipReason: SkipStageIndex}, }, }, + { + // The parser hangs the wrapped instruction off the ONBUILD node instead of + // listing it at the top level, but the image it names is just as real. + name: "ONBUILD wraps a COPY --from", + input: "FROM alpine:3.19\nONBUILD COPY --from=nginx:1.27 /a /b\n", + want: []wantInst{ + {imageRef: "alpine:3.19", startLine: 1}, + {imageRef: "nginx:1.27", isCopyFrom: true, startLine: 2}, + }, + }, + { + name: "ONBUILD wrapping a stage reference", + input: "FROM golang:1.22 AS builder\nONBUILD COPY --from=builder /app /app\n", + want: []wantInst{ + {imageRef: "golang:1.22", startLine: 1}, + {imageRef: "builder", isCopyFrom: true, skip: true, skipReason: SkipStageRef, startLine: 2}, + }, + }, + { + name: "ONBUILD wrapping anything else", + input: "FROM alpine:3.19\nONBUILD RUN echo hi\n", + want: []wantInst{ + {imageRef: "alpine:3.19", startLine: 1}, + }, + }, + { + // BuildKit matches the whole --from value against its stage names, so a name + // carrying a digest finds no stage and is resolved as an image instead. + name: "a digest turns a stage name into an image reference", + input: "FROM nginx:1.27 AS nginx\nCOPY --from=nginx /etc/nginx /etc/nginx\nCOPY --from=nginx@sha256:abc123 /etc/nginx /etc/nginx\n", + want: []wantInst{ + {imageRef: "nginx:1.27", startLine: 1}, + {imageRef: "nginx", isCopyFrom: true, skip: true, skipReason: SkipStageRef, startLine: 2}, + {imageRef: "nginx", rawRef: "nginx@sha256:abc123", digest: "sha256:abc123", isCopyFrom: true, startLine: 3}, + }, + }, { name: "leading zeros are still a stage index", input: "COPY --from=007 /a /b\n", @@ -494,3 +530,34 @@ func TestIsStageIndex(t *testing.T) { } } } + +// TestParse_FromStageNameWithDigest is the FROM side of the same rule: a stage name +// carrying a digest is an image reference, because BuildKit looks the whole string up. +func TestParse_FromStageNameWithDigest(t *testing.T) { + input := "FROM alpine:3.19 AS builder\nFROM builder\nFROM builder@sha256:abc123\n" + checkInstructions(t, input, []wantInst{ + {imageRef: "alpine:3.19", startLine: 1}, + {imageRef: "builder", skip: true, skipReason: SkipStageRef, startLine: 2}, + {imageRef: "builder", rawRef: "builder@sha256:abc123", digest: "sha256:abc123", startLine: 3}, + }) +} + +// TestParse_OnbuildLineSpan checks that the wrapped instruction reports the ONBUILD's +// own position, which the parser leaves unset on the child node. +func TestParse_OnbuildLineSpan(t *testing.T) { + input := "FROM alpine:3.19\nONBUILD COPY \\\n --from=nginx:1.27 \\\n /a /b\n" + instructions, err := Parse(strings.NewReader(input)) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + if len(instructions) != 2 { + t.Fatalf("expected 2 instructions, got %d", len(instructions)) + } + inst := instructions[1] + if inst.StartLine != 2 || inst.EndLine != 4 { + t.Errorf("line span = %d-%d, want 2-4", inst.StartLine, inst.EndLine) + } + if inst.Original != "ONBUILD COPY --from=nginx:1.27 /a /b" { + t.Errorf("Original = %q, want the ONBUILD text", inst.Original) + } +} diff --git a/internal/dockerfile/rewrite.go b/internal/dockerfile/rewrite.go index 3c89c2c..32287cb 100644 --- a/internal/dockerfile/rewrite.go +++ b/internal/dockerfile/rewrite.go @@ -14,29 +14,30 @@ func AddCopyFromDigest(original string, rawRef string, digest string) string { return addDigestAfter(original, copyFromFlag, rawRef, digest) } -// addDigestAfter rewrites the first occurrence of prefix+rawRef in line, replacing any -// digest rawRef already carries. It returns line unchanged when the reference is not -// found there, which is how RewriteFile detects that it must look on a continuation line. +// addDigestAfter rewrites the first occurrence of prefix+rawRef in a single line, +// replacing any digest rawRef already carries. The line is returned unchanged when +// the reference is not found there. func addDigestAfter(line string, prefix string, rawRef string, digest string) string { - if rawRef == "" { - return line - } - idx := strings.Index(line, prefix+rawRef) - if idx < 0 { + lines := []string{line} + if !rewriteSpan(lines, 0, 0, prefix, rawRef, digest) { return line } - baseRef := rawRef - if atIdx := strings.Index(rawRef, "@"); atIdx >= 0 { - baseRef = rawRef[:atIdx] - } - end := idx + len(prefix) + len(rawRef) - return line[:idx] + prefix + baseRef + "@" + digest + line[end:] + return lines[0] } // RewriteFile applies digest pins to a Dockerfile's content. // digests maps instruction index to digest string. func RewriteFile(content string, instructions []FromInstruction, digests map[int]string) string { + result, _ := RewriteFileReport(content, instructions, digests) + return result +} + +// RewriteFileReport is RewriteFile plus the indexes of the instructions it was handed +// a digest for but could not rewrite. Reporting them keeps a caller from announcing an +// image as pinned when the file was in fact left untouched. +func RewriteFileReport(content string, instructions []FromInstruction, digests map[int]string) (string, []int) { lines := strings.Split(content, "\n") + var unrewritten []int for i, inst := range instructions { digest, ok := digests[i] if !ok || inst.Skip { @@ -46,21 +47,111 @@ func RewriteFile(content string, instructions []FromInstruction, digests map[int if inst.IsCopyFrom { prefix = copyFromFlag } - // An instruction continued with "\" spans several lines and the reference is - // not always written on the first one, so every line it covers is tried until - // one of them changes. - for lineIdx := inst.StartLine - 1; lineIdx <= lastLineIndex(inst) && lineIdx < len(lines); lineIdx++ { - if lineIdx < 0 { - continue + first, last := inst.StartLine-1, lastLineIndex(inst) + if first < 0 || first >= len(lines) { + unrewritten = append(unrewritten, i) + continue + } + if last >= len(lines) { + last = len(lines) - 1 + } + if !rewriteSpan(lines, first, last, prefix, inst.RawRef, digest) { + unrewritten = append(unrewritten, i) + } + } + return strings.Join(lines, "\n"), unrewritten +} + +// rewriteSpan pins the reference inside the lines an instruction covers, editing lines +// in place and reporting whether it found anything to change. +// +// The search runs over the instruction as the parser sees it — one logical line — so a +// reference broken up by a "\" continuation is still found, and the digest still lands +// in the right place. Rewriting each physical line on its own would miss those and +// leave the file silently unchanged. +func rewriteSpan(lines []string, first, last int, prefix, rawRef, digest string) bool { + if rawRef == "" { + return false + } + logical, origin := joinContinued(lines, first, last) + idx := strings.Index(logical, prefix+rawRef) + if idx < 0 { + return false + } + + // Any digest the reference already carries is dropped, and the new one is written + // where it ended. + baseRef := rawRef + if atIdx := strings.Index(rawRef, "@"); atIdx >= 0 { + baseRef = rawRef[:atIdx] + } + cutFrom := idx + len(prefix) + len(baseRef) + cutTo := idx + len(prefix) + len(rawRef) + + // Where the replaced range starts, and which columns of which lines it covers. + insertLine, insertCol := last, len(lines[last]) + if cutFrom < len(origin) { + insertLine, insertCol = origin[cutFrom].line, origin[cutFrom].col + } + dropped := make(map[int][2]int, 2) // line -> [from, to) columns to remove + for k := cutFrom; k < cutTo; k++ { + p := origin[k] + if span, ok := dropped[p.line]; ok { + dropped[p.line] = [2]int{span[0], p.col + 1} + } else { + dropped[p.line] = [2]int{p.col, p.col + 1} + } + } + + for i := first; i <= last; i++ { + span, hasDrop := dropped[i] + if !hasDrop && i != insertLine { + continue + } + var sb strings.Builder + for col := 0; col <= len(lines[i]); col++ { + if i == insertLine && col == insertCol { + sb.WriteString("@" + digest) } - rewritten := addDigestAfter(lines[lineIdx], prefix, inst.RawRef, digest) - if rewritten != lines[lineIdx] { - lines[lineIdx] = rewritten + if col == len(lines[i]) { break } + if hasDrop && col >= span[0] && col < span[1] { + continue + } + sb.WriteByte(lines[i][col]) + } + lines[i] = sb.String() + } + return true +} + +// bytePos records where a byte of a joined instruction came from. +type bytePos struct { + line int // index into the file's lines + col int // byte offset within that line +} + +// joinContinued rebuilds the text of an instruction from the lines it spans, the way +// the parser does: a line continued with "\" contributes everything before the +// backslash and the next line follows it directly. The second result maps each byte of +// the joined text back to the line and column it came from. +func joinContinued(lines []string, first, last int) (string, []bytePos) { + var sb strings.Builder + origin := make([]bytePos, 0, 128) + for i := first; i <= last; i++ { + text := lines[i] + if i < last { + if idx := strings.LastIndexByte(text, '\\'); idx >= 0 && strings.TrimSpace(text[idx+1:]) == "" { + text = text[:idx] + } + } + for col := 0; col < len(text); col++ { + sb.WriteByte(text[col]) + origin = append(origin, bytePos{line: i, col: col}) } } - return strings.Join(lines, "\n") + return sb.String(), origin } // lastLineIndex returns the 0-based index of the last line an instruction covers. diff --git a/internal/dockerfile/rewrite_test.go b/internal/dockerfile/rewrite_test.go index 21f83e5..3880660 100644 --- a/internal/dockerfile/rewrite_test.go +++ b/internal/dockerfile/rewrite_test.go @@ -284,3 +284,120 @@ func TestRewriteFile_CopyFromAnchoredToFlag(t *testing.T) { t.Errorf("RewriteFile() =\n%s\nwant:\n%s", got, want) } } + +// TestRewriteFile_ReferenceSplitAcrossLines covers a reference broken up by a "\" +// continuation, where no single line holds the text the parser reports. Searching each +// line on its own would find nothing and leave the file unchanged while the caller +// still counted the image as pinned. +func TestRewriteFile_ReferenceSplitAcrossLines(t *testing.T) { + tests := []struct { + name string + content string + digest string + want string + }{ + { + name: "value continues on the next line", + content: "COPY --from=\\\nnginx:1.27 /src /dst\n", + digest: "sha256:nginx111", + want: "COPY --from=\\\nnginx:1.27@sha256:nginx111 /src /dst\n", + }, + { + name: "the flag name itself is split", + content: "COPY --fr\\\nom=nginx:1.27 /src /dst\n", + digest: "sha256:nginx111", + want: "COPY --fr\\\nom=nginx:1.27@sha256:nginx111 /src /dst\n", + }, + { + name: "FROM split mid-reference", + content: "FROM ubu\\\nntu:24.04\n", + digest: "sha256:ubuntu111", + want: "FROM ubu\\\nntu:24.04@sha256:ubuntu111\n", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + instructions, err := Parse(strings.NewReader(tt.content)) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + if len(instructions) != 1 { + t.Fatalf("expected 1 instruction, got %d", len(instructions)) + } + got, unrewritten := RewriteFileReport(tt.content, instructions, map[int]string{0: tt.digest}) + if len(unrewritten) != 0 { + t.Errorf("instruction reported as unrewritten: %v", unrewritten) + } + if got != tt.want { + t.Errorf("RewriteFile() =\n%q\nwant:\n%q", got, tt.want) + } + // The result must still parse, and now carry the digest. + after, err := Parse(strings.NewReader(got)) + if err != nil { + t.Fatalf("rewritten file no longer parses: %v", err) + } + if len(after) != 1 || after[0].Digest != tt.digest { + t.Errorf("after rewrite: %+v, want digest %q", after, tt.digest) + } + }) + } +} + +// TestRewriteFile_SplitExistingDigestReplaced covers --update where the digest being +// replaced is itself broken across the continuation. +func TestRewriteFile_SplitExistingDigestReplaced(t *testing.T) { + content := "COPY --from=nginx:1.27@sha256:\\\nolddigest /src /dst\n" + instructions, err := Parse(strings.NewReader(content)) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + got := RewriteFile(content, instructions, map[int]string{0: "sha256:newdigest"}) + after, err := Parse(strings.NewReader(got)) + if err != nil { + t.Fatalf("rewritten file no longer parses: %v", err) + } + if len(after) != 1 { + t.Fatalf("expected 1 instruction after rewrite, got %d", len(after)) + } + if after[0].ImageRef != "nginx:1.27" || after[0].Digest != "sha256:newdigest" { + t.Errorf("after rewrite ref=%q digest=%q, want nginx:1.27 / sha256:newdigest", + after[0].ImageRef, after[0].Digest) + } + if strings.Contains(got, "olddigest") { + t.Errorf("old digest still present:\n%q", got) + } +} + +// TestRewriteFile_OnbuildCopyFrom pins the image an ONBUILD COPY --from names. +func TestRewriteFile_OnbuildCopyFrom(t *testing.T) { + content := "FROM alpine:3.19\nONBUILD COPY --from=nginx:1.27 /a /b\n" + instructions, err := Parse(strings.NewReader(content)) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + digests := map[int]string{0: "sha256:alpine111", 1: "sha256:nginx222"} + want := "FROM alpine:3.19@sha256:alpine111\nONBUILD COPY --from=nginx:1.27@sha256:nginx222 /a /b\n" + if got := RewriteFile(content, instructions, digests); got != want { + t.Errorf("RewriteFile() =\n%s\nwant:\n%s", got, want) + } +} + +// TestRewriteFileReport_UnrewrittenReported checks that a reference the rewriter cannot +// find is reported rather than silently counted as pinned. +func TestRewriteFileReport_UnrewrittenReported(t *testing.T) { + content := "FROM alpine:3.19\nRUN echo hi\n" + instructions := []FromInstruction{ + {ImageRef: "alpine:3.19", RawRef: "alpine:3.19", StartLine: 1}, + {ImageRef: "nginx:1.27", RawRef: "nginx:1.27", StartLine: 2, IsCopyFrom: true}, + } + got, unrewritten := RewriteFileReport(content, instructions, map[int]string{ + 0: "sha256:alpine111", + 1: "sha256:nginx222", + }) + if want := "FROM alpine:3.19@sha256:alpine111\nRUN echo hi\n"; got != want { + t.Errorf("RewriteFileReport() =\n%q\nwant:\n%q", got, want) + } + if len(unrewritten) != 1 || unrewritten[0] != 1 { + t.Errorf("unrewritten = %v, want [1]", unrewritten) + } +} diff --git a/testdata/copy_from.Dockerfile b/testdata/copy_from.Dockerfile index 5d56a95..1f4f875 100644 --- a/testdata/copy_from.Dockerfile +++ b/testdata/copy_from.Dockerfile @@ -20,5 +20,8 @@ COPY --from=alpine:3.19@sha256:aaaa1111 /etc/alpine-release /etc/alpine-release ARG NGINX_VERSION=1.27 COPY --from=nginx:${NGINX_VERSION} /etc/nginx /etc/nginx.orig +# an ONBUILD trigger still names a real image +ONBUILD COPY --from=curlimages/curl:8.11.1 /usr/bin/curl /usr/bin/curl + # from the build context COPY ./config /config From 380d593a473784e6cfcf22b30650f15d564c1c48 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 22:46:24 +0000 Subject: [PATCH 3/4] fix: scope ONBUILD sources correctly and honor the escape directive Two more defects Codex flagged, both confirmed against BuildKit. An ONBUILD trigger was resolved against the wrong file's stages. dispatchOnbuild only records the trigger into this image's config; it never runs here. dispatchOnBuildTriggers executes it later, inside whichever build uses this image as its base, resolving it against *that* Dockerfile's dispatch states. The stage names declared in this file are gone by then, so passing them meant `ONBUILD COPY --from=nginx` in a file that also declares `AS nginx` was skipped as a stage reference and left unpinned, when it is an image to resolve. ONBUILD sources are now classified without any stage names. A stage index and scratch do not depend on a stage name and are still not images. The rewrite assumed a backslash continuation. A Dockerfile may change that character with the "# escape=" parser directive, and with "# escape=`" a reference split as "COPY --from=nginx:`" + newline + "1.27" left the backtick in the rebuilt instruction, so the reference was never found and the image went unpinned. The parser's configured escape token now travels on the instruction and the rewrite strips that character instead of a hard-coded one. Under a backtick escape a backslash is an ordinary character, so Windows-style paths in the same instruction are left alone. Both are covered by tests that fail when the fix is reverted. Verified against the real registry: a file combining "# escape=`", a reference split on a backtick, and an ONBUILD COPY --from pins all three images and then checks clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CERP67PPQ72qxDHKqpXkcR --- README.md | 6 +++ internal/dockerfile/parse.go | 16 ++++-- internal/dockerfile/parse_test.go | 18 ++++++- internal/dockerfile/rewrite.go | 32 ++++++++---- internal/dockerfile/rewrite_test.go | 77 +++++++++++++++++++++++++++++ 5 files changed, 135 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index adc8b62..a688db6 100644 --- a/README.md +++ b/README.md @@ -307,8 +307,14 @@ ignore-images: | `COPY --from=image:${TAG}` | Skipped (BuildKit does not expand variables here) | | `COPY /src /dst` (build context) | Nothing to pin | | `ONBUILD COPY --from=image:tag` | Yes | +| `# escape=` directive | Honored | | `ADD --from=...`, `RUN --mount=...,from=...` | Not supported | +An `ONBUILD COPY --from=name` is resolved as an image even when a stage in the same +file shares that name. The trigger does not run in this build: it is recorded into the +image config and executed later, inside whichever build uses this image as its base, +against that Dockerfile's stages — the ones declared here are gone by then. + A digest makes a name an image even when a build stage shares it: BuildKit matches the whole value against its stage names, so `COPY --from=nginx@sha256:...` finds no stage named `nginx` and is resolved from the registry. The same holds for `FROM`. diff --git a/internal/dockerfile/parse.go b/internal/dockerfile/parse.go index c9c3832..971b293 100644 --- a/internal/dockerfile/parse.go +++ b/internal/dockerfile/parse.go @@ -38,6 +38,9 @@ type FromInstruction struct { Skip bool SkipReason string IsCopyFrom bool // true for COPY --from=, false for FROM + // EscapeToken is the character that continues a line, from the "# escape=" + // parser directive. Zero means the Dockerfile default, a backslash. + EscapeToken rune } // Parse reads a Dockerfile from r and returns every pinnable image reference: @@ -79,16 +82,23 @@ func Parse(r io.Reader) ([]FromInstruction, error) { instructions = append(instructions, inst) } case "onbuild": - // The wrapped instruction runs when a later build uses this image as its - // base, and a COPY --from inside one names a real image all the same. + // The wrapped instruction is only recorded into this image's config here; + // it runs later, inside whichever build uses this image as its base, and is + // resolved against *that* Dockerfile's stages. The names declared in this + // file are gone by then, so none of them is passed: a bare name in an + // ONBUILD trigger is an image to resolve, not a stage to skip. if child := onbuildChild(node); child != nil && strings.ToLower(child.Value) == "copy" { - if inst, ok := parseCopyFromNode(child, allStages); ok { + if inst, ok := parseCopyFromNode(child, nil); ok { instructions = append(instructions, inst) } } } } + for i := range instructions { + instructions[i].EscapeToken = result.EscapeToken + } + return instructions, nil } diff --git a/internal/dockerfile/parse_test.go b/internal/dockerfile/parse_test.go index 436222a..341e659 100644 --- a/internal/dockerfile/parse_test.go +++ b/internal/dockerfile/parse_test.go @@ -411,11 +411,25 @@ func TestParse_CopyFrom(t *testing.T) { }, }, { - name: "ONBUILD wrapping a stage reference", + // The trigger runs inside whichever build uses this image as its base, and + // is resolved against that Dockerfile's stages. A stage declared here is + // gone by then, so the name is an image to resolve rather than a stage. + name: "ONBUILD does not see the stages of the file it is written in", input: "FROM golang:1.22 AS builder\nONBUILD COPY --from=builder /app /app\n", want: []wantInst{ {imageRef: "golang:1.22", startLine: 1}, - {imageRef: "builder", isCopyFrom: true, skip: true, skipReason: SkipStageRef, startLine: 2}, + {imageRef: "builder", isCopyFrom: true, startLine: 2}, + }, + }, + { + // A stage index and scratch do not depend on any stage name, so they are + // still not images to resolve. + name: "ONBUILD wrapping a stage index or scratch", + input: "FROM alpine:3.19 AS base\nONBUILD COPY --from=0 /a /b\nONBUILD COPY --from=scratch /a /b\n", + want: []wantInst{ + {imageRef: "alpine:3.19", startLine: 1}, + {imageRef: "0", isCopyFrom: true, skip: true, skipReason: SkipStageIndex, startLine: 2}, + {imageRef: "scratch", isCopyFrom: true, skip: true, skipReason: SkipScratch, startLine: 3}, }, }, { diff --git a/internal/dockerfile/rewrite.go b/internal/dockerfile/rewrite.go index 32287cb..6456d1f 100644 --- a/internal/dockerfile/rewrite.go +++ b/internal/dockerfile/rewrite.go @@ -19,7 +19,7 @@ func AddCopyFromDigest(original string, rawRef string, digest string) string { // the reference is not found there. func addDigestAfter(line string, prefix string, rawRef string, digest string) string { lines := []string{line} - if !rewriteSpan(lines, 0, 0, prefix, rawRef, digest) { + if !rewriteSpan(lines, 0, 0, prefix, rawRef, digest, defaultEscape) { return line } return lines[0] @@ -55,7 +55,7 @@ func RewriteFileReport(content string, instructions []FromInstruction, digests m if last >= len(lines) { last = len(lines) - 1 } - if !rewriteSpan(lines, first, last, prefix, inst.RawRef, digest) { + if !rewriteSpan(lines, first, last, prefix, inst.RawRef, digest, escapeByte(inst.EscapeToken)) { unrewritten = append(unrewritten, i) } } @@ -69,11 +69,11 @@ func RewriteFileReport(content string, instructions []FromInstruction, digests m // reference broken up by a "\" continuation is still found, and the digest still lands // in the right place. Rewriting each physical line on its own would miss those and // leave the file silently unchanged. -func rewriteSpan(lines []string, first, last int, prefix, rawRef, digest string) bool { +func rewriteSpan(lines []string, first, last int, prefix, rawRef, digest string, escape byte) bool { if rawRef == "" { return false } - logical, origin := joinContinued(lines, first, last) + logical, origin := joinContinued(lines, first, last, escape) idx := strings.Index(logical, prefix+rawRef) if idx < 0 { return false @@ -132,17 +132,31 @@ type bytePos struct { col int // byte offset within that line } +// defaultEscape is the character that continues a line when the Dockerfile carries no +// "# escape=" directive. +const defaultEscape = '\\' + +// escapeByte returns the continuation character to strip for an instruction. A zero +// token means none was recorded, so the Dockerfile default applies. The escape +// directive accepts only "\" or "`", both ASCII, so a byte is enough. +func escapeByte(token rune) byte { + if token == 0 { + return defaultEscape + } + return byte(token) +} + // joinContinued rebuilds the text of an instruction from the lines it spans, the way -// the parser does: a line continued with "\" contributes everything before the -// backslash and the next line follows it directly. The second result maps each byte of -// the joined text back to the line and column it came from. -func joinContinued(lines []string, first, last int) (string, []bytePos) { +// the parser does: a continued line contributes everything before the escape character +// and the next line follows it directly. The second result maps each byte of the +// joined text back to the line and column it came from. +func joinContinued(lines []string, first, last int, escape byte) (string, []bytePos) { var sb strings.Builder origin := make([]bytePos, 0, 128) for i := first; i <= last; i++ { text := lines[i] if i < last { - if idx := strings.LastIndexByte(text, '\\'); idx >= 0 && strings.TrimSpace(text[idx+1:]) == "" { + if idx := strings.LastIndexByte(text, escape); idx >= 0 && strings.TrimSpace(text[idx+1:]) == "" { text = text[:idx] } } diff --git a/internal/dockerfile/rewrite_test.go b/internal/dockerfile/rewrite_test.go index 3880660..e73a17b 100644 --- a/internal/dockerfile/rewrite_test.go +++ b/internal/dockerfile/rewrite_test.go @@ -401,3 +401,80 @@ func TestRewriteFileReport_UnrewrittenReported(t *testing.T) { t.Errorf("unrewritten = %v, want [1]", unrewritten) } } + +// TestRewriteFile_EscapeDirective covers a Dockerfile that changes its continuation +// character with "# escape=`". Stripping a hard-coded backslash would leave the +// backtick in the rebuilt instruction, and the reference would never be found. +func TestRewriteFile_EscapeDirective(t *testing.T) { + tests := []struct { + name string + content string + digest string + want string + }{ + { + name: "COPY --from split on a backtick", + content: "# escape=`\nCOPY --from=nginx:`\n1.27 /src /dst\n", + digest: "sha256:nginx111", + want: "# escape=`\nCOPY --from=nginx:`\n1.27@sha256:nginx111 /src /dst\n", + }, + { + name: "FROM split on a backtick", + content: "# escape=`\nFROM ubu`\nntu:24.04\n", + digest: "sha256:ubuntu111", + want: "# escape=`\nFROM ubu`\nntu:24.04@sha256:ubuntu111\n", + }, + { + // With a backtick escape a backslash is an ordinary character, so a + // Windows-style path must survive untouched. + name: "backslashes in a path are not continuations", + content: "# escape=`\nCOPY --from=nginx:1.27 `\n C:\\nginx\\conf C:\\dst\n", + digest: "sha256:nginx222", + want: "# escape=`\nCOPY --from=nginx:1.27@sha256:nginx222 `\n C:\\nginx\\conf C:\\dst\n", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + instructions, err := Parse(strings.NewReader(tt.content)) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + if len(instructions) != 1 { + t.Fatalf("expected 1 instruction, got %d", len(instructions)) + } + got, unrewritten := RewriteFileReport(tt.content, instructions, map[int]string{0: tt.digest}) + if len(unrewritten) != 0 { + t.Errorf("instruction reported as unrewritten: %v", unrewritten) + } + if got != tt.want { + t.Errorf("RewriteFile() =\n%q\nwant:\n%q", got, tt.want) + } + after, err := Parse(strings.NewReader(got)) + if err != nil { + t.Fatalf("rewritten file no longer parses: %v", err) + } + if len(after) != 1 || after[0].Digest != tt.digest { + t.Errorf("after rewrite: %+v, want digest %q", after, tt.digest) + } + }) + } +} + +// TestRewriteFile_OnbuildPinsNameMatchingLocalStage is the rewrite side of ONBUILD +// scoping: the name matches a stage in this file, but the trigger runs elsewhere, so +// it is pinned as an image. +func TestRewriteFile_OnbuildPinsNameMatchingLocalStage(t *testing.T) { + content := "FROM alpine:3.19 AS nginx\nONBUILD COPY --from=nginx /a /b\n" + instructions, err := Parse(strings.NewReader(content)) + if err != nil { + t.Fatalf("Parse() error = %v", err) + } + if len(instructions) != 2 || instructions[1].Skip { + t.Fatalf("expected the ONBUILD ref to be pinnable, got %+v", instructions) + } + digests := map[int]string{0: "sha256:alpine111", 1: "sha256:nginx222"} + want := "FROM alpine:3.19@sha256:alpine111 AS nginx\nONBUILD COPY --from=nginx@sha256:nginx222 /a /b\n" + if got := RewriteFile(content, instructions, digests); got != want { + t.Errorf("RewriteFile() =\n%s\nwant:\n%s", got, want) + } +} From eeb10228ad6ef113f7ff5de7d0c6cc3e58e82e98 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 23:07:43 +0000 Subject: [PATCH 4/4] docs: link the Dockerfile escape directive reference Point at the official documentation for "# escape=" from both places that depend on it: the EscapeToken field that carries the parser's choice, and the default the rewriter falls back to. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CERP67PPQ72qxDHKqpXkcR --- internal/dockerfile/parse.go | 1 + internal/dockerfile/rewrite.go | 1 + 2 files changed, 2 insertions(+) diff --git a/internal/dockerfile/parse.go b/internal/dockerfile/parse.go index 971b293..37e94bf 100644 --- a/internal/dockerfile/parse.go +++ b/internal/dockerfile/parse.go @@ -40,6 +40,7 @@ type FromInstruction struct { IsCopyFrom bool // true for COPY --from=, false for FROM // EscapeToken is the character that continues a line, from the "# escape=" // parser directive. Zero means the Dockerfile default, a backslash. + // https://docs.docker.com/reference/dockerfile/#escape EscapeToken rune } diff --git a/internal/dockerfile/rewrite.go b/internal/dockerfile/rewrite.go index 6456d1f..570b7a1 100644 --- a/internal/dockerfile/rewrite.go +++ b/internal/dockerfile/rewrite.go @@ -134,6 +134,7 @@ type bytePos struct { // defaultEscape is the character that continues a line when the Dockerfile carries no // "# escape=" directive. +// https://docs.docker.com/reference/dockerfile/#escape const defaultEscape = '\\' // escapeByte returns the continuation character to strip for an instruction. A zero