From 2aa466f65f994f902ab4f9a1134a58d9e44f4586 Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Sun, 25 Oct 2026 19:29:18 -0300 Subject: [PATCH 01/22] test(doctor): hold every check result without a docker daemon With Docker unreachable no unit test pinned the summaries, remediations, boundaries and builder actions of the doctor checks, so mutate left 41 edits standing. The new tests assert the exact Result and Action each probe yields through fake runners, temp dirs and HOME. The probe-context setup moves into writeProbeContext so its write failure needs no unobservable branch, keeping both messages. Writes of StatusOK, Result's zero value, are exempt. Co-Authored-By: Claude Opus 5.5 --- internal/doctor/checks.go | 39 +++-- internal/doctor/checks_held_test.go | 253 ++++++++++++++++++++++++++++ internal/doctor/doctor_held_test.go | 28 +++ internal/doctor/setup_held_test.go | 81 +++++++++ 4 files changed, 386 insertions(+), 15 deletions(-) create mode 100644 internal/doctor/checks_held_test.go create mode 100644 internal/doctor/doctor_held_test.go create mode 100644 internal/doctor/setup_held_test.go diff --git a/internal/doctor/checks.go b/internal/doctor/checks.go index 8a7d04b..f85d742 100644 --- a/internal/doctor/checks.go +++ b/internal/doctor/checks.go @@ -35,7 +35,7 @@ func checkDockerDaemon(ctx context.Context, env *Env) Result { r.Remediation = "Start Docker (e.g. `sudo systemctl start docker`) and ensure your user can access it." return r } - r.Status = StatusOK + r.Status = StatusOK // mutate-exempt: StatusOK is Result's zero value, so dropping this write leaves the same Result r.Summary = fmt.Sprintf("Docker daemon reachable (server %s)", strings.TrimSpace(string(stdout))) return r } @@ -51,7 +51,7 @@ func checkBuildx(ctx context.Context, env *Env) Result { r.Remediation = "Install the Docker buildx plugin: https://github.com/docker/buildx#installing" return r } - r.Status = StatusOK + r.Status = StatusOK // mutate-exempt: StatusOK is Result's zero value, so dropping this write leaves the same Result r.Summary = firstLine(string(stdout)) return r } @@ -64,25 +64,19 @@ func checkBuildx(ctx context.Context, env *Env) Result { func checkCacheExport(ctx context.Context, env *Env) Result { r := Result{Name: "build-cache-export", Fixable: true} - dir, err := os.MkdirTemp(env.ProbeDir, "devcontainer-doctor-*") + dir, err := writeProbeContext(env.ProbeDir) if err != nil { r.Status = StatusWarn - r.Summary = "could not create probe context: " + err.Error() + r.Summary = err.Error() return r } defer os.RemoveAll(dir) - if err := os.WriteFile(filepath.Join(dir, "Dockerfile"), []byte(probeDockerfile), 0o644); err != nil { - r.Status = StatusWarn - r.Summary = "could not write probe context: " + err.Error() - return r - } - dest := filepath.Join(dir, "cache") _, stderr, code, err := env.runner().Run(ctx, nil, env.dockerPath(), "buildx", "build", "--cache-to", "type=local,dest="+dest, dir) if err == nil && code == 0 { - r.Status = StatusOK + r.Status = StatusOK // mutate-exempt: StatusOK is Result's zero value, so dropping this write leaves the same Result r.Summary = "builder can export build cache" return r } @@ -98,6 +92,21 @@ func checkCacheExport(ctx context.Context, env *Env) Result { return r } +// writeProbeContext creates the cache-export probe's build context under base: +// a fresh temp directory holding probeDockerfile. On failure nothing is left +// behind and the error says which step failed. +func writeProbeContext(base string) (string, error) { + dir, err := os.MkdirTemp(base, "devcontainer-doctor-*") + if err != nil { + return "", fmt.Errorf("could not create probe context: %w", err) + } + if err := os.WriteFile(filepath.Join(dir, "Dockerfile"), []byte(probeDockerfile), 0o644); err != nil { + os.RemoveAll(dir) + return "", fmt.Errorf("could not write probe context: %w", err) + } + return dir, nil +} + // checkComposeV2 verifies the Compose v2 plugin. Only Compose-based dev // containers need it, so its absence is a warn. func checkComposeV2(ctx context.Context, env *Env) Result { @@ -109,7 +118,7 @@ func checkComposeV2(ctx context.Context, env *Env) Result { r.Remediation = "Install the Docker Compose v2 plugin (only needed for Compose-based dev containers)." return r } - r.Status = StatusOK + r.Status = StatusOK // mutate-exempt: StatusOK is Result's zero value, so dropping this write leaves the same Result r.Summary = "docker compose v" + firstLine(string(stdout)) return r } @@ -138,7 +147,7 @@ func checkDiskSpace(ctx context.Context, env *Env) Result { r.Remediation = "Free up space or run `docker system prune` to reclaim unused images/containers." return r } - r.Status = StatusOK + r.Status = StatusOK // mutate-exempt: StatusOK is Result's zero value, so dropping this write leaves the same Result r.Summary = fmt.Sprintf("%s free on %s", humanBytes(avail), path) return r } @@ -157,12 +166,12 @@ func checkSELinux(_ context.Context, env *Env) Result { data, err := os.ReadFile(path) if err != nil { // No selinuxfs → SELinux not enabled on this host. - r.Status = StatusOK + r.Status = StatusOK // mutate-exempt: StatusOK is Result's zero value, so dropping this write leaves the same Result r.Summary = "SELinux not enabled" return r } if strings.TrimSpace(string(data)) != "1" { - r.Status = StatusOK + r.Status = StatusOK // mutate-exempt: StatusOK is Result's zero value, so dropping this write leaves the same Result r.Summary = "SELinux not enforcing" return r } diff --git a/internal/doctor/checks_held_test.go b/internal/doctor/checks_held_test.go new file mode 100644 index 0000000..1f44f05 --- /dev/null +++ b/internal/doctor/checks_held_test.go @@ -0,0 +1,253 @@ +package doctor + +import ( + "errors" + "os" + "path/filepath" + "strings" + "testing" +) + +// respond answers every docker invocation with the same outcome. +func respond(stdout, stderr string, code int, err error) *scriptRunner { + return &scriptRunner{fn: func([]string) ([]byte, []byte, int, error) { + return []byte(stdout), []byte(stderr), code, err + }} +} + +func TestChecksReportTheirExactResults(t *testing.T) { + notFound := errors.New("exec: \"docker\": executable file not found in $PATH") + tests := []struct { + name string + check func(t *testing.T, env *Env) Result + run *scriptRunner + want Result + }{ + { + name: "daemon reachable", run: respond("27.0.3\n", "", 0, nil), + check: func(t *testing.T, env *Env) Result { return checkDockerDaemon(t.Context(), env) }, + want: Result{Name: "docker-daemon", Status: StatusOK, Summary: "Docker daemon reachable (server 27.0.3)"}, + }, + { + name: "docker CLI missing", run: respond("", "", -1, notFound), + check: func(t *testing.T, env *Env) Result { return checkDockerDaemon(t.Context(), env) }, + want: Result{Name: "docker-daemon", Status: StatusFail, Summary: "Docker CLI not found (docker)", + Remediation: "Install Docker Engine: https://docs.docker.com/engine/install/"}, + }, + { + name: "daemon unreachable", run: respond("", "Cannot connect to the Docker daemon", 1, nil), + check: func(t *testing.T, env *Env) Result { return checkDockerDaemon(t.Context(), env) }, + want: Result{Name: "docker-daemon", Status: StatusFail, Summary: "Docker daemon is not reachable", + Remediation: "Start Docker (e.g. `sudo systemctl start docker`) and ensure your user can access it."}, + }, + { + name: "buildx missing", run: respond("", "unknown command", 1, nil), + check: func(t *testing.T, env *Env) Result { return checkBuildx(t.Context(), env) }, + want: Result{Name: "buildx", Status: StatusWarn, Summary: "docker buildx plugin is not available", + Remediation: "Install the Docker buildx plugin: https://github.com/docker/buildx#installing"}, + }, + { + name: "buildx binary missing", run: respond("", "", 0, notFound), + check: func(t *testing.T, env *Env) Result { return checkBuildx(t.Context(), env) }, + want: Result{Name: "buildx", Status: StatusWarn, Summary: "docker buildx plugin is not available", + Remediation: "Install the Docker buildx plugin: https://github.com/docker/buildx#installing"}, + }, + { + name: "buildx present", run: respond("\ngithub.com/docker/buildx v0.16.0\nmore\n", "", 0, nil), + check: func(t *testing.T, env *Env) Result { return checkBuildx(t.Context(), env) }, + want: Result{Name: "buildx", Status: StatusOK, Summary: "github.com/docker/buildx v0.16.0"}, + }, + { + name: "compose missing", run: respond("", "unknown command", 1, nil), + check: func(t *testing.T, env *Env) Result { return checkComposeV2(t.Context(), env) }, + want: Result{Name: "compose-v2", Status: StatusWarn, Summary: "docker compose (v2) plugin is not available", + Remediation: "Install the Docker Compose v2 plugin (only needed for Compose-based dev containers)."}, + }, + { + name: "compose binary missing", run: respond("2.29.0\n", "", 0, notFound), + check: func(t *testing.T, env *Env) Result { return checkComposeV2(t.Context(), env) }, + want: Result{Name: "compose-v2", Status: StatusWarn, Summary: "docker compose (v2) plugin is not available", + Remediation: "Install the Docker Compose v2 plugin (only needed for Compose-based dev containers)."}, + }, + { + name: "compose present", run: respond("2.29.0\n", "", 0, nil), + check: func(t *testing.T, env *Env) Result { return checkComposeV2(t.Context(), env) }, + want: Result{Name: "compose-v2", Status: StatusOK, Summary: "docker compose v2.29.0"}, + }, + { + name: "cache export works", run: respond("", "", 0, nil), + check: func(t *testing.T, env *Env) Result { return checkCacheExport(t.Context(), env) }, + want: Result{Name: "build-cache-export", Status: StatusOK, Fixable: true, Summary: "builder can export build cache"}, + }, + { + name: "cache export fails without stderr", run: respond("", "", 0, notFound), + check: func(t *testing.T, env *Env) Result { return checkCacheExport(t.Context(), env) }, + want: Result{Name: "build-cache-export", Status: StatusWarn, Fixable: true, + Summary: "active builder cannot export build cache (--cache-to/--output/--platform)", + Remediation: "Run `devcontainer setup` to create a cache-capable builder, or enable the " + + "containerd image store (add {\"features\":{\"containerd-snapshotter\":true}} to " + + "/etc/docker/daemon.json and restart Docker)."}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + env := &Env{Runner: tt.run, ProbeDir: t.TempDir()} + if got := tt.check(t, env); got != tt.want { + t.Fatalf("got %+v\nwant %+v", got, tt.want) + } + }) + } +} + +func TestCacheExportBuildsTheProbeContextAndCleansUp(t *testing.T) { + probeDir := t.TempDir() + var dockerfile string + sr := &scriptRunner{fn: func(args []string) ([]byte, []byte, int, error) { + ctxDir := args[len(args)-1] + b, err := os.ReadFile(filepath.Join(ctxDir, "Dockerfile")) + if err != nil { + t.Errorf("probe context has no Dockerfile: %v", err) + } + dockerfile = string(b) + return nil, nil, 0, nil + }} + got := checkCacheExport(t.Context(), &Env{Runner: sr, ProbeDir: probeDir}) + if got.Status != StatusOK { + t.Fatalf("status = %s (%s), want ok", got.Status, got.Summary) + } + if dockerfile != probeDockerfile { + t.Errorf("Dockerfile = %q, want %q", dockerfile, probeDockerfile) + } + if left, _ := os.ReadDir(probeDir); len(left) != 0 { + t.Errorf("probe context left behind: %v", left) + } +} + +func TestCacheExportWarnsWhenTheProbeContextCannotBeCreated(t *testing.T) { + sr := respond("", "", 0, nil) + missing := filepath.Join(t.TempDir(), "missing") + got := checkCacheExport(t.Context(), &Env{Runner: sr, ProbeDir: missing}) + if got.Name != "build-cache-export" || got.Status != StatusWarn || !got.Fixable || got.Remediation != "" { + t.Fatalf("got %+v, want a fixable warn without remediation", got) + } + if !strings.HasPrefix(got.Summary, "could not create probe context: ") || + !strings.Contains(got.Summary, missing) || !strings.HasSuffix(got.Summary, "no such file or directory") { + t.Errorf("summary = %q, want the probe-context creation error", got.Summary) + } + if len(sr.calls) != 0 { + t.Errorf("docker must not run without a probe context; calls=%v", sr.calls) + } +} + +func TestDiskSpaceResults(t *testing.T) { + root := t.TempDir() + run := respond(root+"\n", "", 0, nil) + + t.Run("enough space", func(t *testing.T) { + env := &Env{Runner: run, DiskFree: func(string) (uint64, error) { return 100 << 30, nil }} + want := Result{Name: "disk-space", Status: StatusOK, Summary: "100.0 GiB free on " + root} + if got := checkDiskSpace(t.Context(), env); got != want { + t.Fatalf("got %+v, want %+v", got, want) + } + }) + t.Run("free space unknown", func(t *testing.T) { + env := &Env{Runner: run, DiskFree: func(string) (uint64, error) { return 0, errors.New("statfs boom") }} + want := Result{Name: "disk-space", Status: StatusWarn, Summary: "could not determine free space on " + root + ": statfs boom"} + if got := checkDiskSpace(t.Context(), env); got != want { + t.Fatalf("got %+v, want %+v", got, want) + } + }) + t.Run("low space", func(t *testing.T) { + env := &Env{Runner: run, DiskFree: func(string) (uint64, error) { return 3 << 20, nil }} + want := Result{Name: "disk-space", Status: StatusWarn, + Summary: "low free disk space on " + root + ": 3 MiB (recommended ≥ 5.0 GiB)", + Remediation: "Free up space or run `docker system prune` to reclaim unused images/containers."} + if got := checkDiskSpace(t.Context(), env); got != want { + t.Fatalf("got %+v, want %+v", got, want) + } + }) +} + +func TestDockerDataRootPrefersAReadableDaemonRoot(t *testing.T) { + root := t.TempDir() + home := t.TempDir() + tests := []struct { + name string + run *scriptRunner + home string + want string + }{ + {"daemon root exists locally", respond(" "+root+"\n", "", 0, nil), home, root}, + {"info exits non-zero", respond(root, "", 1, nil), home, home}, + {"docker cannot run", respond(root, "", 0, errors.New("not found")), home, home}, + {"empty root", respond(" \n", "", 0, nil), home, home}, + {"root missing locally", respond(filepath.Join(root, "absent"), "", 0, nil), home, home}, + {"no home either", respond("", "", 1, nil), "", "/"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("HOME", tt.home) + if got := dockerDataRoot(t.Context(), &Env{Runner: tt.run}); got != tt.want { + t.Fatalf("dockerDataRoot = %q, want %q", got, tt.want) + } + }) + } +} + +func TestSELinuxSummaries(t *testing.T) { + write := func(t *testing.T, content string) string { + p := filepath.Join(t.TempDir(), "enforce") + if err := os.WriteFile(p, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + return p + } + tests := []struct { + name string + path string + want Result + }{ + {"absent", filepath.Join(t.TempDir(), "enforce"), Result{Name: "selinux", Status: StatusOK, Summary: "SELinux not enabled"}}, + {"permissive", write(t, "0\n"), Result{Name: "selinux", Status: StatusOK, Summary: "SELinux not enforcing"}}, + {"enforcing", write(t, " 1\n"), Result{Name: "selinux", Status: StatusWarn, + Summary: "SELinux is enforcing; workspace/bind mounts can fail with 'Permission denied' in the container", + Remediation: `Relabel bind mounts for the container: add "runArgs": ["--security-opt", "label=disable"] ` + + "to devcontainer.json, or append :z/:Z to bind-mount sources."}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := checkSELinux(t.Context(), &Env{SELinuxEnforcePath: tt.path}); got != tt.want { + t.Fatalf("got %+v, want %+v", got, tt.want) + } + }) + } +} + +func TestHumanBytesBoundaries(t *testing.T) { + tests := []struct { + in uint64 + want string + }{ + {0, "0 B"}, + {1<<20 - 1, "1048575 B"}, + {1 << 20, "1 MiB"}, + {3 << 20, "3 MiB"}, + {1<<30 - 1<<20, "1023 MiB"}, + {1 << 30, "1.0 GiB"}, + {3 << 30, "3.0 GiB"}, + } + for _, tt := range tests { + if got := humanBytes(tt.in); got != tt.want { + t.Errorf("humanBytes(%d) = %q, want %q", tt.in, got, tt.want) + } + } +} + +func TestStatfsFree(t *testing.T) { + if free, err := statfsFree(t.TempDir()); err != nil || free == 0 { + t.Errorf("statfsFree(tempdir) = %d, %v; want free bytes", free, err) + } + if free, err := statfsFree(filepath.Join(t.TempDir(), "missing")); !errors.Is(err, os.ErrNotExist) || free != 0 { + t.Errorf("statfsFree(missing) = %d, %v; want 0, ErrNotExist", free, err) + } +} diff --git a/internal/doctor/doctor_held_test.go b/internal/doctor/doctor_held_test.go new file mode 100644 index 0000000..4487ebc --- /dev/null +++ b/internal/doctor/doctor_held_test.go @@ -0,0 +1,28 @@ +package doctor + +import ( + "context" + "io" + "testing" +) + +// nameRunner records the executable each docker call runs. +type nameRunner struct{ names []string } + +func (n *nameRunner) Run(_ context.Context, _ io.Writer, name string, _ ...string) ([]byte, []byte, int, error) { + n.names = append(n.names, name) + return []byte("27.0.3\n"), nil, 0, nil +} + +func TestChecksRunTheConfiguredDockerBinary(t *testing.T) { + for _, tt := range []struct{ configured, want string }{ + {"", "docker"}, + {"/opt/bin/podman-docker", "/opt/bin/podman-docker"}, + } { + nr := &nameRunner{} + checkDockerDaemon(t.Context(), &Env{Runner: nr, DockerPath: tt.configured}) + if len(nr.names) != 1 || nr.names[0] != tt.want { + t.Errorf("DockerPath %q ran %v, want [%s]", tt.configured, nr.names, tt.want) + } + } +} diff --git a/internal/doctor/setup_held_test.go b/internal/doctor/setup_held_test.go new file mode 100644 index 0000000..14c092c --- /dev/null +++ b/internal/doctor/setup_held_test.go @@ -0,0 +1,81 @@ +package doctor + +import ( + "errors" + "slices" + "testing" +) + +// builderRunner answers `buildx inspect` with inspect and every other docker +// call with rest. +func builderRunner(inspectCode int, rest func() ([]byte, []byte, int, error)) *scriptRunner { + return &scriptRunner{fn: func(args []string) ([]byte, []byte, int, error) { + if has(args, "inspect") { + return nil, nil, inspectCode, nil + } + return rest() + }} +} + +func TestEnsureCacheBuilderActions(t *testing.T) { + ok := func() ([]byte, []byte, int, error) { return nil, nil, 0, nil } + tests := []struct { + name string + run *scriptRunner + dryRun bool + want Action + }{ + {"existing builder selected", builderRunner(0, ok), false, Action{Name: "build-cache-export", Applied: true, + Message: `buildx builder "devcontainer" already exists; selecting it as the default`}}, + {"existing builder dry run", builderRunner(0, ok), true, Action{Name: "build-cache-export", + Message: `buildx builder "devcontainer" already exists; selecting it as the default`}}, + {"existing builder use fails with stderr", builderRunner(0, func() ([]byte, []byte, int, error) { + return nil, []byte("\nERROR: no such builder\nmore\n"), 1, nil + }), false, Action{Name: "build-cache-export", Err: "ERROR: no such builder", + Message: `buildx builder "devcontainer" already exists; selecting it as the default`}}, + {"existing builder use cannot run", builderRunner(0, func() ([]byte, []byte, int, error) { + return nil, nil, 0, errors.New("exec: docker: not found") + }), false, Action{Name: "build-cache-export", Err: "exec: docker: not found", + Message: `buildx builder "devcontainer" already exists; selecting it as the default`}}, + {"existing builder use fails silently", builderRunner(0, func() ([]byte, []byte, int, error) { + return nil, nil, 1, nil + }), false, Action{Name: "build-cache-export", Err: "command failed", + Message: `buildx builder "devcontainer" already exists; selecting it as the default`}}, + {"absent builder dry run", builderRunner(1, ok), true, Action{Name: "build-cache-export", + Message: `create docker-container buildx builder "devcontainer" and select it as the default`}}, + {"absent builder created", builderRunner(1, ok), false, Action{Name: "build-cache-export", Applied: true, + Message: `created and selected docker-container buildx builder "devcontainer"`}}, + {"absent builder create cannot run", builderRunner(1, func() ([]byte, []byte, int, error) { + return nil, nil, 0, errors.New("exec: docker: not found") + }), false, Action{Name: "build-cache-export", Err: "exec: docker: not found", + Message: `create docker-container buildx builder "devcontainer" and select it as the default`}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := ensureCacheBuilder(t.Context(), &Env{Runner: tt.run}, tt.dryRun); got != tt.want { + t.Fatalf("got %+v\nwant %+v", got, tt.want) + } + }) + } +} + +// Only a fixable cache-export result is remediated automatically; anything else +// is reported for manual action, whatever its name or Fixable flag. +func TestSetupRemediatesOnlyAFixableCacheExport(t *testing.T) { + sr := builderRunner(1, func() ([]byte, []byte, int, error) { return nil, nil, 0, nil }) + report := Report{Results: []Result{ + {Name: "build-cache-export", Status: StatusWarn, Remediation: "enable containerd"}, + {Name: "disk-space", Status: StatusWarn, Fixable: true, Remediation: "free space"}, + }} + got := Setup(t.Context(), &Env{Runner: sr}, report, false) + want := []Action{ + {Name: "build-cache-export", Message: "manual action required: enable containerd"}, + {Name: "disk-space", Message: "manual action required: free space"}, + } + if !slices.Equal(got, want) { + t.Fatalf("got %+v\nwant %+v", got, want) + } + if len(sr.calls) != 0 { + t.Errorf("no builder may be touched; calls=%v", sr.calls) + } +} From 43b40cfa02c4ef501f0797c838314bf1bbb29807 Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Sun, 25 Oct 2026 19:29:18 -0300 Subject: [PATCH 02/22] test(pfs): hold the modes and root entry ExtractTarGz writes No unit test checked the 0755 directory mode, that a header without a mode keeps os.Create's default, or that a regular file naming destDir itself fails, so mutate's edits to those lines survived. Co-Authored-By: Claude Opus 5.5 --- internal/pfs/pfs_held_test.go | 91 +++++++++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) create mode 100644 internal/pfs/pfs_held_test.go diff --git a/internal/pfs/pfs_held_test.go b/internal/pfs/pfs_held_test.go new file mode 100644 index 0000000..9647f01 --- /dev/null +++ b/internal/pfs/pfs_held_test.go @@ -0,0 +1,91 @@ +//go:build unix + +package pfs + +import ( + "archive/tar" + "os" + "path/filepath" + "syscall" + "testing" +) + +// writeEntries writes a plain tar holding headers in order, each regular file +// with body as content. +func writeEntries(t *testing.T, body string, headers ...tar.Header) string { + t.Helper() + path := filepath.Join(t.TempDir(), "a.tar") + f, err := os.Create(path) + if err != nil { + t.Fatal(err) + } + tw := tar.NewWriter(f) + for _, h := range headers { + if h.Typeflag == tar.TypeReg { + h.Size = int64(len(body)) + } + if err := tw.WriteHeader(&h); err != nil { + t.Fatal(err) + } + if h.Typeflag == tar.TypeReg { + if _, err := tw.Write([]byte(body)); err != nil { + t.Fatal(err) + } + } + } + if err := tw.Close(); err != nil { + t.Fatal(err) + } + if err := f.Close(); err != nil { + t.Fatal(err) + } + return path +} + +func perm(t *testing.T, path string) os.FileMode { + t.Helper() + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + return info.Mode().Perm() +} + +func TestExtractTarGzModes(t *testing.T) { + old := syscall.Umask(0o022) + t.Cleanup(func() { syscall.Umask(old) }) + + src := writeEntries(t, "x", + tar.Header{Name: "dir/", Typeflag: tar.TypeDir, Mode: 0o700}, + tar.Header{Name: "nested/deep/file", Typeflag: tar.TypeReg, Mode: 0o600}, + tar.Header{Name: "modeless", Typeflag: tar.TypeReg, Mode: 0}, + tar.Header{Name: "script.sh", Typeflag: tar.TypeReg, Mode: 0o751}, + ) + dest := t.TempDir() + if err := ExtractTarGz(src, dest); err != nil { + t.Fatal(err) + } + for rel, want := range map[string]os.FileMode{ + "dir": 0o755, // directories are created 0755, not with the entry's mode + "nested/deep": 0o755, + "nested/deep/file": 0o600, + "modeless": 0o644, // no mode in the header: os.Create's default stays + "script.sh": 0o751, + } { + if got := perm(t, filepath.Join(dest, rel)); got != want { + t.Errorf("%s mode = %v, want %v", rel, got, want) + } + } +} + +func TestExtractTarGzRefusesARegularFileAtTheRoot(t *testing.T) { + // "." names destDir itself; a regular file cannot replace the directory, + // and the extraction must say so instead of dropping the entry. + src := writeEntries(t, "x", tar.Header{Name: ".", Typeflag: tar.TypeReg, Mode: 0o644}) + if err := ExtractTarGz(src, t.TempDir()); err == nil { + t.Fatal("ExtractTarGz succeeded writing a regular file over destDir") + } + if _, err := os.Stat(src); err != nil { + t.Errorf("a failed extraction must keep the archive: %v", err) + } +} From 016bb5d55f2ddf56a99c6aa36859d7321859d4da Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Sun, 25 Oct 2026 19:29:18 -0300 Subject: [PATCH 03/22] test(httpx): hold the extra CA on top of the system roots Nothing checked that NODE_EXTRA_CA_CERTS is added to the system roots rather than replacing them, nor that HTTP/2 negotiation survives. Tests now install generated system roots in TestMain and probe the GODEBUG=http2client=0 case, where the transport has no TLS config, in a child process. The TLS config is now cloned and assigned in one place so the nil case leaves no field write only that child can observe. Co-Authored-By: Claude Opus 5.5 --- internal/httpx/httpx.go | 10 +- internal/httpx/httpx_held_test.go | 220 ++++++++++++++++++++++++++++++ 2 files changed, 227 insertions(+), 3 deletions(-) create mode 100644 internal/httpx/httpx_held_test.go diff --git a/internal/httpx/httpx.go b/internal/httpx/httpx.go index 3dbfd2a..c1b499d 100644 --- a/internal/httpx/httpx.go +++ b/internal/httpx/httpx.go @@ -48,10 +48,14 @@ func NewTransport() *http.Transport { // looking like "the proxy is ignored". Surface it instead of swallowing. fmt.Fprintf(os.Stderr, "warning: could not load CA certs from %s: %v\n", caFile, err) } else if pool != nil { - if transport.TLSClientConfig == nil { - transport.TLSClientConfig = &tls.Config{} + // The clone carries the HTTP/2 ALPN settings unless HTTP/2 is + // disabled (GODEBUG=http2client=0), when it has no TLS config at all. + tlsConfig := &tls.Config{} + if transport.TLSClientConfig != nil { + tlsConfig = transport.TLSClientConfig.Clone() } - transport.TLSClientConfig.RootCAs = pool + tlsConfig.RootCAs = pool + transport.TLSClientConfig = tlsConfig } } diff --git a/internal/httpx/httpx_held_test.go b/internal/httpx/httpx_held_test.go new file mode 100644 index 0000000..a71a8fd --- /dev/null +++ b/internal/httpx/httpx_held_test.go @@ -0,0 +1,220 @@ +package httpx + +import ( + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/x509" + "crypto/x509/pkix" + "encoding/json" + "encoding/pem" + "errors" + "fmt" + "io/fs" + "math/big" + "net/http" + "os" + "os/exec" + "path/filepath" + "slices" + "testing" + "time" +) + +// childEnv makes the test binary act as a probe process: it builds a transport +// in a process whose system roots and GODEBUG the parent chose, and prints what +// the transport trusts. Both are read once per process, so only a fresh process +// can set them. +const childEnv = "HTTPX_TRANSPORT_PROBE" + +type probe struct { + NextProtos []string `json:"nextProtos"` + // TrustsSystemAndExtra is true when RootCAs holds exactly the system roots + // (SSL_CERT_FILE) plus the NODE_EXTRA_CA_CERTS bundle. + TrustsSystemAndExtra bool `json:"trustsSystemAndExtra"` +} + +// TestMain installs a generated CA as this process's system roots +// (SSL_CERT_FILE, with an empty SSL_CERT_DIR), so tests that compare against +// the system pool do not depend on the host's trust store. +func TestMain(m *testing.M) { + if os.Getenv(childEnv) != "" { + os.Exit(probeTransport()) + } + dir, err := os.MkdirTemp("", "httpx-roots-") + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(2) + } + code, err := runWithSystemRoots(m, dir) + os.RemoveAll(dir) + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(2) + } + os.Exit(code) +} + +func runWithSystemRoots(m *testing.M, dir string) (int, error) { + certDir := filepath.Join(dir, "certs") + if err := os.Mkdir(certDir, 0o700); err != nil { + return 0, err + } + roots, err := writeCA(dir, "system-root") + if err != nil { + return 0, err + } + os.Setenv("SSL_CERT_FILE", roots) + os.Setenv("SSL_CERT_DIR", certDir) + // Load the system roots now, while SSL_CERT_FILE names them: the pool is + // read once per process, and tests later clear SSL_CERT_FILE. + if _, err := x509.SystemCertPool(); err != nil { + return 0, err + } + return m.Run(), nil +} + +// probeTransport is the probe process: it reports what NewTransport trusts. +func probeTransport() int { + tr := NewTransport() + var p probe + if cfg := tr.TLSClientConfig; cfg != nil { + p.NextProtos = cfg.NextProtos + want, err := poolOf(os.Getenv("SSL_CERT_FILE"), os.Getenv("NODE_EXTRA_CA_CERTS")) + if err != nil { + fmt.Fprintln(os.Stderr, "probe:", err) + return 3 + } + p.TrustsSystemAndExtra = cfg.RootCAs != nil && cfg.RootCAs.Equal(want) + } + if err := json.NewEncoder(os.Stdout).Encode(p); err != nil { + return 3 + } + return 0 +} + +func poolOf(files ...string) (*x509.CertPool, error) { + pool := x509.NewCertPool() + for _, f := range files { + data, err := os.ReadFile(f) + if err != nil { + return nil, err + } + if !pool.AppendCertsFromPEM(data) { + return nil, fmt.Errorf("no certificate in %s", f) + } + } + return pool, nil +} + +func writeCA(dir, name string) (string, error) { + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + return "", err + } + tmpl := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: name}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + IsCA: true, + BasicConstraintsValid: true, + KeyUsage: x509.KeyUsageCertSign, + } + der, err := x509.CreateCertificate(rand.Reader, tmpl, tmpl, &key.PublicKey, key) + if err != nil { + return "", err + } + path := filepath.Join(dir, name+".pem") + return path, os.WriteFile(path, pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der}), 0o600) +} + +func testCA(t *testing.T, name string) string { + t.Helper() + path, err := writeCA(t.TempDir(), name) + if err != nil { + t.Fatal(err) + } + return path +} + +func runProbe(t *testing.T, godebug string) probe { + t.Helper() + cmd := exec.Command(os.Args[0]) + // SSL_CERT_FILE and SSL_CERT_DIR come from TestMain through os.Environ. + cmd.Env = append(os.Environ(), + childEnv+"=1", + "GODEBUG="+godebug, + "NODE_EXTRA_CA_CERTS="+testCA(t, "corporate-proxy"), + ) + out, err := cmd.Output() + if err != nil { + t.Fatalf("probe process failed: %v\n%s", err, out) + } + var p probe + if err := json.Unmarshal(out, &p); err != nil { + t.Fatalf("probe output %q: %v", out, err) + } + return p +} + +// An extra CA is added to the system roots, not substituted for them (behind a +// TLS-intercepting proxy some hosts are still reached directly), and the rest +// of the default TLS config, HTTP/2 negotiation included, is kept. +func TestNewTransportKeepsSystemRootsAndHTTP2WithExtraCA(t *testing.T) { + extra := testCA(t, "corporate-proxy") + t.Setenv("NODE_EXTRA_CA_CERTS", extra) + base := http.DefaultTransport.(*http.Transport).Clone().TLSClientConfig + if base == nil { + t.Fatal("the default transport has no TLS config; is GODEBUG=http2client=0 set?") + } + + cfg := NewTransport().TLSClientConfig + want, err := poolOf(os.Getenv("SSL_CERT_FILE"), extra) + if err != nil { + t.Fatal(err) + } + if cfg == nil || !cfg.RootCAs.Equal(want) { + t.Fatal("RootCAs must hold the system roots plus NODE_EXTRA_CA_CERTS") + } + if !slices.Equal(cfg.NextProtos, base.NextProtos) { + t.Errorf("NextProtos = %v, want the default %v", cfg.NextProtos, base.NextProtos) + } + if http.DefaultTransport.(*http.Transport).TLSClientConfig.RootCAs != nil { + t.Error("NewTransport must not change the default transport's TLS config") + } +} + +// With HTTP/2 disabled the cloned transport has no TLS config; the extra CA must +// still be installed on a fresh one. +func TestNewTransportInstallsExtraCAWithoutATLSConfig(t *testing.T) { + p := runProbe(t, "http2client=0") + if !p.TrustsSystemAndExtra { + t.Error("RootCAs must hold the system roots plus NODE_EXTRA_CA_CERTS") + } + if len(p.NextProtos) != 0 { + t.Errorf("NextProtos = %v, want none with http2client=0", p.NextProtos) + } +} + +func TestLoadCACertsAddsTheBundleToTheSystemRoots(t *testing.T) { + extra := testCA(t, "corporate-proxy") + got, err := loadCACerts(extra) + if err != nil { + t.Fatal(err) + } + want, err := poolOf(os.Getenv("SSL_CERT_FILE"), extra) + if err != nil { + t.Fatal(err) + } + if !got.Equal(want) { + t.Fatal("loadCACerts must return the system roots plus the bundle") + } +} + +func TestLoadCACertsReportsAnUnreadableBundle(t *testing.T) { + pool, err := loadCACerts(filepath.Join(t.TempDir(), "missing.pem")) + if !errors.Is(err, fs.ErrNotExist) || pool != nil { + t.Fatalf("loadCACerts(missing) = %v, %v; want nil, ErrNotExist", pool, err) + } +} From f79d5efdb67295c1b59f9866033264f11c3b2a3e Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Sun, 25 Oct 2026 19:31:34 -0300 Subject: [PATCH 04/22] fix(oci): stop falling back to credsStore after a registry's helper The reference CLI asks credsStore only when the docker config names no credential helper for the registry (getCredentialFromDockerConfigOr- CredentialHelper in httpOCIRegistry.ts): a registry's own helper that finds nothing goes straight to the auths entry, and an empty helper name counts as no helper. Go asked credsStore after a failed registry helper and ran "docker-credential-" for an empty name. The new tests also hold, without Docker, the platform default helper choice (pass on PATH or secretservice), the configContainsAuth rule that skips it, and what each helper outcome logs: until now only Docker-backed e2e tests refused those mutate edits. Co-Authored-By: Claude Opus 5.5 --- internal/oci/auth.go | 12 +-- internal/oci/auth_held_test.go | 169 +++++++++++++++++++++++++++++++++ 2 files changed, 175 insertions(+), 6 deletions(-) create mode 100644 internal/oci/auth_held_test.go diff --git a/internal/oci/auth.go b/internal/oci/auth.go index 7988ef4..f26c9bd 100644 --- a/internal/oci/auth.go +++ b/internal/oci/auth.go @@ -93,16 +93,16 @@ func getCredentialFromDockerConfig(env map[string]string, registry string, logge hasAuth := len(config.CredHelpers) > 0 || config.CredsStore != "" || len(config.Auths) > 0 - // credHelpers for specific registry - if helper, ok := config.CredHelpers[registry]; ok { + // credHelpers for the registry, else credsStore (the global helper). As in + // TS getCredentialFromDockerConfigOrCredentialHelper (httpOCIRegistry.ts), + // a registry's own helper that finds nothing does not fall back to + // credsStore, and an empty helper name counts as no helper. + if helper := config.CredHelpers[registry]; helper != "" { logger.Write(fmt.Sprintf("[httpOci] Found credential helper %q for registry %q", helper, registry), log.LevelTrace) if cred := getCredentialFromHelper(registry, helper, logger); cred != nil { return cred } - } - - // credsStore (global helper) - if config.CredsStore != "" { + } else if config.CredsStore != "" { logger.Write(fmt.Sprintf("[httpOci] Invoking credsStore credential helper %q", config.CredsStore), log.LevelTrace) if cred := getCredentialFromHelper(registry, config.CredsStore, logger); cred != nil { return cred diff --git a/internal/oci/auth_held_test.go b/internal/oci/auth_held_test.go new file mode 100644 index 0000000..ba46c86 --- /dev/null +++ b/internal/oci/auth_held_test.go @@ -0,0 +1,169 @@ +package oci + +import ( + "bytes" + "encoding/base64" + "os" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/devcontainers/cli/internal/log" +) + +// pathOnly makes dir the whole PATH and writes each named executable into it as +// a /bin/sh script with the given body. The scripts use shell builtins only, so +// they run although nothing else is on PATH. +func pathOnly(t *testing.T, scripts map[string]string) { + t.Helper() + if runtime.GOOS != "linux" { + t.Skip("the platform default helper is chosen on Linux; the fake helpers are POSIX shell") + } + dir := t.TempDir() + for name, body := range scripts { + if err := os.WriteFile(filepath.Join(dir, name), []byte("#!/bin/sh\n"+body+"\n"), 0o755); err != nil { + t.Fatal(err) + } + } + t.Setenv("PATH", dir) +} + +// helperReturning is a docker-credential-* body answering `get` with user:secret. +func helperReturning(user string) string { + return `read reg; printf '{"Username":"` + user + `","Secret":"s"}\n'` +} + +func dockerConfigDir(t *testing.T, config string) string { + t.Helper() + dir := t.TempDir() + if config != "" { + if err := os.WriteFile(filepath.Join(dir, "config.json"), []byte(config), 0o600); err != nil { + t.Fatal(err) + } + } + return dir +} + +func basicUser(t *testing.T, cred *credential) string { + t.Helper() + if cred == nil { + t.Fatal("credential is nil") + } + raw, err := base64.StdEncoding.DecodeString(cred.base64Encoded) + if err != nil { + t.Fatal(err) + } + user, _, _ := strings.Cut(string(raw), ":") + return user +} + +// TS getCredentialFromDockerConfigOrCredentialHelper (httpOCIRegistry.ts): with +// no docker config holding auth, Linux asks docker-credential-pass when `pass` +// is on PATH and the libsecret helper otherwise (Go names it secretservice, the +// binary docker ships, where TS says `secret`). +func TestPlatformDefaultHelperFollowsPass(t *testing.T) { + for _, tt := range []struct { + name string + withPass bool + want string + }{ + {"pass on PATH", true, "pass"}, + {"no pass on PATH", false, "secretservice"}, + } { + t.Run(tt.name, func(t *testing.T) { + scripts := map[string]string{ + "docker-credential-pass": helperReturning("pass"), + "docker-credential-secretservice": helperReturning("secretservice"), + } + if tt.withPass { + scripts["pass"] = "exit 0" + } + pathOnly(t, scripts) + env := map[string]string{"DOCKER_CONFIG": dockerConfigDir(t, "")} + if got := basicUser(t, getCredentialFromDockerConfig(env, "reg.io", log.Null)); got != tt.want { + t.Fatalf("credential from helper %q, want %q", got, tt.want) + } + }) + } +} + +// TS computes configContainsAuth as any credHelpers key, a credsStore, or any +// auths key, and asks the platform default helper only when it is false. +func TestDockerConfigWithAuthSkipsPlatformDefault(t *testing.T) { + for _, tt := range []struct { + name, config string + wantDefault bool + }{ + {"empty config", `{}`, true}, + {"a helper for another registry", `{"credHelpers":{"other.io":"x"}}`, false}, + {"a credsStore that finds nothing", `{"credsStore":"missing"}`, false}, + {"auths for another registry", `{"auths":{"other.io":{}}}`, false}, + } { + t.Run(tt.name, func(t *testing.T) { + pathOnly(t, map[string]string{"docker-credential-secretservice": helperReturning("default")}) + env := map[string]string{"DOCKER_CONFIG": dockerConfigDir(t, tt.config)} + cred := getCredentialFromDockerConfig(env, "reg.io", log.Null) + if !tt.wantDefault { + if cred != nil { + t.Fatalf("credential = %#v, want none", cred) + } + return + } + if got := basicUser(t, cred); got != "default" { + t.Fatalf("credential from %q, want the platform default helper", got) + } + }) + } +} + +// TS asks credsStore only in the else branch of credHelpers[registry]: a +// registry's own helper that finds nothing does not fall back to credsStore, +// and an empty helper name is no helper at all. +func TestRegistryHelperDoesNotFallBackToCredsStore(t *testing.T) { + pathOnly(t, map[string]string{ + "docker-credential-store": helperReturning("store"), + "docker-credential-fails": "exit 1", + }) + failing := map[string]string{"DOCKER_CONFIG": dockerConfigDir(t, `{"credHelpers":{"reg.io":"fails"},"credsStore":"store"}`)} + if cred := getCredentialFromDockerConfig(failing, "reg.io", log.Null); cred != nil { + t.Fatalf("credential = %#v, want none once the registry's helper failed", cred) + } + empty := map[string]string{"DOCKER_CONFIG": dockerConfigDir(t, `{"credHelpers":{"reg.io":""},"credsStore":"store"}`)} + if got := basicUser(t, getCredentialFromDockerConfig(empty, "reg.io", log.Null)); got != "store" { + t.Fatalf("credential from %q, want credsStore", got) + } +} + +// TS getCredentialFromHelper: a failing helper is a trace line, an empty answer +// is no credential without a word, and anything that is not JSON is a warning. +// Go also warns when a helper times out, which a helper that just fails must +// not claim. +func TestCredentialHelperOutcomesLog(t *testing.T) { + pathOnly(t, map[string]string{ + "docker-credential-fails": "exit 1", + "docker-credential-silent": "exit 0", + "docker-credential-junk": "printf x", + }) + for _, tt := range []struct { + helper, want, notWant string + }{ + {"fails", `Credential helper "fails" failed for "reg.io"`, "timed out"}, + {"silent", "", "non-JSON"}, + {"junk", `Credential helper "junk" returned non-JSON for "reg.io"`, ""}, + } { + t.Run(tt.helper, func(t *testing.T) { + var buf bytes.Buffer + logger := log.New(log.Options{Writer: &buf, Level: log.LevelTrace}) + if cred := getCredentialFromHelper("reg.io", tt.helper, logger); cred != nil { + t.Fatalf("credential = %#v, want none", cred) + } + if tt.want != "" && !strings.Contains(buf.String(), tt.want) { + t.Errorf("log %q lacks %q", buf.String(), tt.want) + } + if tt.notWant != "" && strings.Contains(buf.String(), tt.notWant) { + t.Errorf("log %q has %q", buf.String(), tt.notWant) + } + }) + } +} From d99ffa402bca92636b30d81664094d210549967a Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Sun, 25 Oct 2026 19:31:34 -0300 Subject: [PATCH 05/22] test(oci): hold the auth policy, ref and client edits without docker A mutate run with Docker unreachable, as on CI, left edits in the OCI auth policy, ref parsing, the client's response-header timeout and the CLI's policy context that only Docker-backed e2e tests refused. Unit tests now hold them against the reference: diagnostics written to the policy's logger, URL.origin port and Docker Hub rules, authority validation, the 300-399 token redirect range, 403 challenges, relative realms, digest rejections, fetch by digest, and a context holding the policy keeping its cancellation. Edits nobody can observe are simplified away instead: the userinfo/path/ query/fragment checks the Host comparison already covers, the unreachable ::1 host, the resp == nil a conforming transport never returns, and index arithmetic strings.Cut replaces. ParseRef now splits a digest on every colon as TS does, so sha256:a:b is a format error. Co-Authored-By: Claude Opus 5.5 --- internal/cli/build_auth.go | 5 +- internal/cli/ociauth_held_test.go | 44 ++++++ internal/oci/authpolicy.go | 13 +- internal/oci/authpolicy_held_test.go | 200 +++++++++++++++++++++++++++ internal/oci/client_held_test.go | 68 +++++++++ internal/oci/orasclient.go | 7 +- internal/oci/ref.go | 14 +- internal/oci/ref_held_test.go | 21 +++ 8 files changed, 351 insertions(+), 21 deletions(-) create mode 100644 internal/cli/ociauth_held_test.go create mode 100644 internal/oci/authpolicy_held_test.go create mode 100644 internal/oci/client_held_test.go create mode 100644 internal/oci/ref_held_test.go diff --git a/internal/cli/build_auth.go b/internal/cli/build_auth.go index f5ffa32..e9fa873 100644 --- a/internal/cli/build_auth.go +++ b/internal/cli/build_auth.go @@ -54,11 +54,10 @@ func registryHostFromImageRef(ref string) string { if ref == "" { return "" } - i := strings.IndexByte(ref, '/') - if i < 0 { + first, _, found := strings.Cut(ref, "/") + if !found { return "" } - first := ref[:i] if first == "localhost" || strings.ContainsAny(first, ".:") { return first } diff --git a/internal/cli/ociauth_held_test.go b/internal/cli/ociauth_held_test.go new file mode 100644 index 0000000..ade3562 --- /dev/null +++ b/internal/cli/ociauth_held_test.go @@ -0,0 +1,44 @@ +package cli + +import ( + "context" + "testing" + + "github.com/devcontainers/cli/internal/log" + "github.com/devcontainers/cli/internal/oci" +) + +// Storing the policy keeps the command context it is stored on: a cancelled +// command (Ctrl-C) must still cancel the registry traffic that reads the +// policy from the same context. A caller without a context gets one. +func TestWithOCIAuthPolicyKeepsTheContext(t *testing.T) { + policy, err := oci.NewAuthPolicy(true, nil, log.Null) + if err != nil { + t.Fatal(err) + } + parent, cancel := context.WithCancel(context.Background()) + ctx := withOCIAuthPolicy(parent, policy) + cancel() + if ctx.Err() == nil { + t.Error("cancelling the command context did not reach the context holding the policy") + } + if got := ociAuthPolicy(ctx); got != policy { + t.Errorf("ociAuthPolicy = %p, want the stored policy %p", got, policy) + } + //nolint:staticcheck // SA1012: a nil context is the case under test + if got := ociAuthPolicy(withOCIAuthPolicy(nil, policy)); got != policy { + t.Errorf("ociAuthPolicy without a parent context = %p, want %p", got, policy) + } +} + +// A context holding a nil policy reads as the default policy (hardening off, +// as the reference CLI defaults --oci-auth-hardening), never as nil. +func TestOCIAuthPolicyIgnoresAStoredNil(t *testing.T) { + policy := ociAuthPolicy(withOCIAuthPolicy(context.Background(), nil)) + if policy == nil { + t.Fatal("ociAuthPolicy returned nil") + } + if policy.Hardening() { + t.Error("the default policy must not harden") + } +} diff --git a/internal/oci/authpolicy.go b/internal/oci/authpolicy.go index 623a622..3372e47 100644 --- a/internal/oci/authpolicy.go +++ b/internal/oci/authpolicy.go @@ -150,10 +150,11 @@ func normalizeHTTPSAuthority(authority string) (string, error) { if err != nil || parsed.Host == "" { return "", invalid } - if parsed.User != nil || (parsed.Path != "" && parsed.Path != "/") || parsed.RawQuery != "" || parsed.Fragment != "" { - return "", invalid - } - if parsed.Host != authority && !strings.EqualFold(parsed.Host, authority) { + // url.Parse keeps the host as written, so userinfo, a path, a query or a + // fragment leaves Host short of the authority: TS normalizeHttpsAuthority's + // username/pathname/search/hash checks (httpOCIRegistry.ts) all reject + // through this one comparison. + if parsed.Host != authority { return "", invalid } return strings.ToLower(parsed.Host), nil @@ -256,8 +257,8 @@ func (t *authPolicyTransport) RoundTrip(req *http.Request) (*http.Response, erro tokenRequest := t.isTokenEndpoint(req.URL) resp, err := t.base.RoundTrip(req) - if err != nil || resp == nil { - return resp, err + if err != nil { + return nil, err } if tokenRequest { diff --git a/internal/oci/authpolicy_held_test.go b/internal/oci/authpolicy_held_test.go new file mode 100644 index 0000000..00c3922 --- /dev/null +++ b/internal/oci/authpolicy_held_test.go @@ -0,0 +1,200 @@ +package oci + +import ( + "bytes" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/devcontainers/cli/internal/log" +) + +func untrustedChallenge(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("WWW-Authenticate", `Bearer realm="https://evil.example/token",service="registry"`) + w.WriteHeader(http.StatusUnauthorized) +} + +const blockedLine = "[httpOci] OCI auth diagnostics: Authentication lookup from registry" + +// TS recordOCIAuthDiagnostic (httpOCIRegistry.ts) writes +// `[httpOci] OCI auth diagnostics: ` to the command's output; Go +// writes it to the logger the policy was built with, or the one SetLogger +// handed it later, and a nil logger silences it. +func TestAuthPolicyLogsDiagnosticsToItsLogger(t *testing.T) { + t.Run("the logger the policy was built with", func(t *testing.T) { + var buf bytes.Buffer + policy, err := NewAuthPolicy(false, nil, log.New(log.Options{Writer: &buf, Level: log.LevelInfo})) + if err != nil { + t.Fatal(err) + } + if _, err := roundTrip(t, policy, "http", "", untrustedChallenge, "/v2/x/manifests/1"); err != nil { + t.Fatal(err) + } + if !strings.Contains(buf.String(), blockedLine) || !strings.Contains(buf.String(), "to realm origin 'https://evil.example' would be blocked by OCI auth hardening.") { + t.Fatalf("log = %q", buf.String()) + } + }) + + t.Run("no logger", func(t *testing.T) { + policy, err := NewAuthPolicy(false, nil, nil) + if err != nil { + t.Fatal(err) + } + if _, err := roundTrip(t, policy, "http", "", untrustedChallenge, "/v2/x/manifests/1"); err != nil { + t.Fatal(err) + } + if !policy.Diagnostics().AuthLookupWouldBeBlocked { + t.Fatal("diagnostic not recorded") + } + }) + + t.Run("the logger SetLogger hands it", func(t *testing.T) { + var first, second bytes.Buffer + policy, err := NewAuthPolicy(false, nil, log.New(log.Options{Writer: &first, Level: log.LevelInfo})) + if err != nil { + t.Fatal(err) + } + policy.SetLogger(log.New(log.Options{Writer: &second, Level: log.LevelInfo})) + policy.SetLogger(nil) // keeps the logger it had + var none *AuthPolicy + none.SetLogger(log.Null) // a nil policy ignores it + if _, err := roundTrip(t, policy, "http", "", untrustedChallenge, "/v2/x/manifests/1"); err != nil { + t.Fatal(err) + } + if first.Len() != 0 || !strings.Contains(second.String(), blockedLine) { + t.Fatalf("first logger %q, second logger %q", first.String(), second.String()) + } + }) +} + +// TS isOCIRegistryOrigin compares URL.origin, which keeps a non-default port, +// and treats the Docker Hub authorities as one origin only when both URLs are +// HTTPS and both hosts are Docker Hub's. +func TestIsRegistryOriginConditions(t *testing.T) { + for _, tt := range []struct { + u, registry string + want bool + }{ + {"https://ghcr.io:5000", "https://ghcr.io", false}, + {"http://localhost:80", "http://localhost", true}, + {"http://docker.io", "https://index.docker.io", false}, + {"https://docker.io", "http://index.docker.io", false}, + {"https://ghcr.io", "https://docker.io", false}, + {"https://docker.io", "https://ghcr.io", false}, + {"https://docker.io", "https://index.docker.io", true}, + } { + if got := isRegistryOrigin(mustURL(t, tt.u), mustURL(t, tt.registry)); got != tt.want { + t.Errorf("isRegistryOrigin(%q, %q) = %v, want %v", tt.u, tt.registry, got, tt.want) + } + } +} + +// TS normalizeHttpsAuthority lower-cases a bare host[:port] and rejects +// anything else with "Invalid authority ''."; an empty authority +// has no host. +func TestNormalizeHTTPSAuthority(t *testing.T) { + if got, err := normalizeHTTPSAuthority("Reg.Example:5000"); err != nil || got != "reg.example:5000" { + t.Errorf("normalizeHTTPSAuthority(Reg.Example:5000) = %q, %v", got, err) + } + for _, authority := range []string{"", "a?q", "a#f", "a b"} { + if _, err := normalizeHTTPSAuthority(authority); err == nil || err.Error() != "Invalid authority '"+authority+"'." { + t.Errorf("normalizeHTTPSAuthority(%q) error = %v", authority, err) + } + } +} + +// Only token requests are held to the no-redirect rule (TS +// requestResolvingRedirects guards the token fetch alone): a blob on the +// registry that redirects to storage, on the host of a known token endpoint, +// passes under hardening. +func TestRegistryRedirectBesideTokenEndpointPasses(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, "https://storage.example/blob", http.StatusTemporaryRedirect) + })) + defer srv.Close() + policy, err := NewAuthPolicy(true, nil, log.Null) + if err != nil { + t.Fatal(err) + } + transport := newAuthPolicyTransport(http.DefaultTransport, policy, "http", strings.TrimPrefix(srv.URL, "http://")) + transport.addTokenEndpoint(mustURL(t, srv.URL+"/token")) + req, _ := http.NewRequest(http.MethodGet, srv.URL+"/v2/x/blobs/sha256:abc", nil) + resp, err := transport.RoundTrip(req) + if err != nil { + t.Fatalf("blob redirect refused: %v", err) + } + resp.Body.Close() + if resp.StatusCode != http.StatusTemporaryRedirect { + t.Fatalf("status = %d, want 307", resp.StatusCode) + } +} + +// TS records authServerRedirect when follow-redirects followed the token +// request, which it does for a 3xx carrying a Location: 300 counts, a 201 or +// a 400 with a Location header does not. +func TestTokenRedirectStatusRange(t *testing.T) { + for _, tt := range []struct { + status int + want bool + }{ + {http.StatusMultipleChoices, true}, + {http.StatusCreated, false}, + {http.StatusBadRequest, false}, + } { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Location", "https://elsewhere.example/token") + w.WriteHeader(tt.status) + })) + policy, err := NewAuthPolicy(false, nil, log.Null) + if err != nil { + t.Fatal(err) + } + transport := newAuthPolicyTransport(http.DefaultTransport, policy, "http", strings.TrimPrefix(srv.URL, "http://")) + transport.addTokenEndpoint(mustURL(t, srv.URL+"/token")) + req, _ := http.NewRequest(http.MethodGet, srv.URL+"/token", nil) + resp, err := transport.RoundTrip(req) + if err != nil { + t.Fatalf("status %d: %v", tt.status, err) + } + resp.Body.Close() + srv.Close() + if got := policy.Diagnostics().AuthServerRedirect; got != tt.want { + t.Errorf("status %d: authServerRedirect = %v, want %v", tt.status, got, tt.want) + } + } +} + +// TS requestEnsureAuthenticated reads the challenge of a 403 (insufficient +// scope) as it does a 401's. +func TestForbiddenChallengeIsInspected(t *testing.T) { + policy, err := NewAuthPolicy(false, nil, log.Null) + if err != nil { + t.Fatal(err) + } + if _, err := roundTrip(t, policy, "http", "", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("WWW-Authenticate", `Bearer realm="https://evil.example/token",service="registry"`) + w.WriteHeader(http.StatusForbidden) + }, "/v2/x/manifests/1"); err != nil { + t.Fatal(err) + } + if !policy.Diagnostics().AuthLookupWouldBeBlocked { + t.Fatal("the 403 challenge's realm was not checked") + } +} + +// TS fails the authentication when `new URL(realm)` throws, as it does for a +// relative realm; Go refuses such a realm under hardening as unparsable. +func TestRelativeRealmIsUnparsableUnderHardening(t *testing.T) { + policy, err := NewAuthPolicy(true, nil, log.Null) + if err != nil { + t.Fatal(err) + } + _, err = roundTrip(t, policy, "http", "", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("WWW-Authenticate", `Bearer realm="/token",service="registry"`) + w.WriteHeader(http.StatusUnauthorized) + }, "/v2/x/manifests/1") + if err == nil || !strings.Contains(err.Error(), "requested authentication from an unparsable realm '/token'") { + t.Fatalf("error = %v, want the unparsable-realm error", err) + } +} diff --git a/internal/oci/client_held_test.go b/internal/oci/client_held_test.go new file mode 100644 index 0000000..464d0f2 --- /dev/null +++ b/internal/oci/client_held_test.go @@ -0,0 +1,68 @@ +package oci + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + godigest "github.com/opencontainers/go-digest" + + "github.com/devcontainers/cli/internal/log" +) + +// A registry that accepts the request and never answers must not hold a +// caller that brought no deadline of its own: the client's transport gives up +// waiting for response headers. +func TestClientGivesUpOnRegistryThatNeverAnswers(t *testing.T) { + blocked := make(chan struct{}) + srv := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { <-blocked })) + defer srv.Close() + defer close(blocked) + + orig := responseHeaderTimeout + responseHeaderTimeout = 100 * time.Millisecond + defer func() { responseHeaderTimeout = orig }() + + c := NewClientWithAuthPolicy(log.Null, map[string]string{"DOCKER_CONFIG": t.TempDir()}, DefaultAuthPolicy()) + ref := &Ref{Registry: strings.TrimPrefix(srv.URL, "http://"), Path: "me/feat", Resource: "me/feat"} + done := make(chan error, 1) + go func() { + _, err := c.FetchBlobContext(context.Background(), ref, "sha256:"+strings.Repeat("a", 64)) + done <- err + }() + select { + case err := <-done: + if err == nil { + t.Fatal("fetch from a registry that never answers succeeded") + } + case <-time.After(20 * time.Second): + t.Fatal("fetch waited on a registry that never answers") + } +} + +// TS getManifest (containerCollectionsOCI.ts) fails only when the manifest's +// content digest differs from the expected one: fetching a manifest by the +// digest it has succeeds and reports that digest. +func TestFetchManifestByItsOwnDigest(t *testing.T) { + reg := newFakeOCIRegistry() + host := reg.start(t) + body := []byte(`{"schemaVersion":2,"mediaType":"` + OCIManifestMediaType + `","config":{"mediaType":"` + ManifestMediaType + `","digest":"sha256:` + strings.Repeat("0", 64) + `","size":0},"layers":[]}`) + digest := godigest.FromBytes(body).String() + reg.manifests[digest] = body + + ref, err := ParseRef(host + "/me/features/feat:1") + if err != nil { + t.Fatal(err) + } + c := NewClientWithAuthPolicy(log.Null, map[string]string{"DOCKER_CONFIG": t.TempDir()}, DefaultAuthPolicy()) + got, err := c.FetchManifest(ref, digest) + if err != nil { + t.Fatalf("FetchManifest by digest: %v", err) + } + if got.ContentDigest != digest || got.CanonicalID != ref.Resource+"@"+digest { + t.Fatalf("content digest %q, canonical id %q", got.ContentDigest, got.CanonicalID) + } +} diff --git a/internal/oci/orasclient.go b/internal/oci/orasclient.go index 6f7efe9..5b0911f 100644 --- a/internal/oci/orasclient.go +++ b/internal/oci/orasclient.go @@ -14,11 +14,8 @@ import ( // isLocalRegistry reports whether the registry is a local/insecure one that // serves plain HTTP (localhost / loopback), as opposed to a real HTTPS registry. func isLocalRegistry(registry string) bool { - host := registry - if i := strings.IndexByte(host, ':'); i >= 0 { - host = host[:i] - } - return host == "localhost" || host == "127.0.0.1" || host == "::1" + host, _, _ := strings.Cut(registry, ":") + return host == "localhost" || host == "127.0.0.1" } // repository builds an oras-go remote.Repository for the given ref, wired to the diff --git a/internal/oci/ref.go b/internal/oci/ref.go index a1d38ef..2340f56 100644 --- a/internal/oci/ref.go +++ b/internal/oci/ref.go @@ -38,10 +38,7 @@ type Ref struct { // registry, HTTPS otherwise. It is part of the `read-configuration` output // (featuresConfiguration.featureSets[].sourceInformation.featureRef.scheme). func (r *Ref) Scheme() string { - host := r.Registry - if i := strings.IndexByte(host, ':'); i >= 0 { - host = host[:i] - } + host, _, _ := strings.Cut(r.Registry, ":") if strings.EqualFold(host, "localhost") { return "http" } @@ -77,7 +74,8 @@ func ParseRef(input string) (*Ref, error) { // Digest reference: registry/path@sha256:hex resource = input[:indexOfLastAt] digestWithAlgo := input[indexOfLastAt+1:] - parts := strings.SplitN(digestWithAlgo, ":", 2) + // TS splits on every colon and wants exactly two parts. + parts := strings.Split(digestWithAlgo, ":") if len(parts) != 2 { return nil, fmt.Errorf("invalid digest format %q, expected 'sha256:hex'", digestWithAlgo) } @@ -88,8 +86,10 @@ func ParseRef(input string) (*Ref, error) { return nil, fmt.Errorf("digest %q does not match expected pattern", parts[1]) } digest = digestWithAlgo - } else if indexOfLastColon != -1 && indexOfLastColon > indexOfLastSlash { - // Tag reference: registry/path:tag + } else if indexOfLastColon > indexOfLastSlash { + // Tag reference: registry/path:tag. TS also checks + // indexOfLastColon !== -1, which this implies: the last slash is never + // before index -1. resource = input[:indexOfLastColon] tag = input[indexOfLastColon+1:] } else { diff --git a/internal/oci/ref_held_test.go b/internal/oci/ref_held_test.go new file mode 100644 index 0000000..9effe06 --- /dev/null +++ b/internal/oci/ref_held_test.go @@ -0,0 +1,21 @@ +package oci + +import ( + "strings" + "testing" +) + +// TS getRef (containerCollectionsOCI.ts) splits a digest on every colon and +// wants exactly two parts, then checks the algorithm is sha256, then checks the +// hex against regexForVersionOrDigest: each rejection names what failed. +func TestParseRefDigestRejections(t *testing.T) { + for _, tt := range []struct{ input, want string }{ + {"ghcr.io/ns/id/feat@sha256:a:b", `invalid digest format "sha256:a:b"`}, + {"ghcr.io/ns/id/feat@md5:abc", `unsupported digest algorithm "md5"`}, + {"ghcr.io/ns/id/feat@sha256:-bad", `digest "-bad" does not match expected pattern`}, + } { + if _, err := ParseRef(tt.input); err == nil || !strings.Contains(err.Error(), tt.want) { + t.Errorf("ParseRef(%q) error = %v, want %q", tt.input, err, tt.want) + } + } +} From 297ca342daa33c0fb4980b7a8b7be039077d19ca Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Sun, 25 Oct 2026 19:30:54 -0300 Subject: [PATCH 06/22] fix(docker): gate compose features on the whole version like the reference TS useNewProjectName and the additional build contexts check compare the parsed Compose version with isEarlierVersion, a missing component counting as 0. The Go gates treated a one-component version as unknown, so "1" allowed new project names and "3" refused additional contexts. Both now use isEarlierVersion, and parseComposeVersion keeps every component like TS parseVersion (the cap at three could not change any comparison). Down's error paths and the ToProjectName drop value are now held or exempted, so mutate refuses every edit in compose.go without Docker. Co-Authored-By: Claude Opus 5.5 --- internal/docker/compose.go | 41 ++++++---------- internal/docker/compose_held_test.go | 70 ++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 28 deletions(-) create mode 100644 internal/docker/compose_held_test.go diff --git a/internal/docker/compose.go b/internal/docker/compose.go index 6fd7008..e045fb7 100644 --- a/internal/docker/compose.go +++ b/internal/docker/compose.go @@ -194,34 +194,19 @@ func (c *ComposeClient) Down(ctx context.Context, composeFiles []string, project return nil } -// SupportsAdditionalContexts returns true if compose version >= 2.17.0. +// SupportsAdditionalContexts returns true if compose version >= 2.17.0, which +// added additional_contexts (TS buildAndExtendDockerCompose: `version && +// !isEarlierVersion(version, [2, 17, 0])`). An unknown (empty) version is +// earlier than 2.17.0, so it reports false like TS. func (c *ComposeClient) SupportsAdditionalContexts() bool { - if len(c.Version) < 2 { - return false - } - // 2.17.0 added additional_contexts - if c.Version[0] > 2 { - return true - } - if c.Version[0] == 2 && c.Version[1] >= 17 { - return true - } - return false + return !isEarlierVersion(c.Version, []int{2, 17, 0}) } -// UsesNewProjectNames returns true if compose version >= 1.21.0 -// (allowed hyphens and underscores in project names). +// UsesNewProjectNames returns true if compose version >= 1.21.0 (allowed +// hyphens and underscores in project names), or when the version is unknown +// (TS useNewProjectName: optimistic default). func (c *ComposeClient) UsesNewProjectNames() bool { - if len(c.Version) < 2 { - return true // optimistic default - } - if c.Version[0] > 1 { - return true - } - if c.Version[0] == 1 && c.Version[1] >= 21 { - return true - } - return false + return len(c.Version) == 0 || !isEarlierVersion(c.Version, []int{1, 21, 0}) } func (c *ComposeClient) buildGlobalArgs(composeFiles []string, envFile string) []string { @@ -235,15 +220,15 @@ func (c *ComposeClient) buildGlobalArgs(composeFiles []string, envFile string) [ return args } -// parseComposeVersion extracts the first N[.N[.N]] version found in s (e.g. from -// "Docker Compose version v2.15.1"), up to three numeric components. +// parseComposeVersion extracts the first N[.N]* version found in s (e.g. from +// "Docker Compose version v2.15.1"), every numeric component like TS parseVersion. func parseComposeVersion(s string) []int { i := strings.IndexFunc(s, func(r rune) bool { return r >= '0' && r <= '9' }) if i < 0 { return nil } var version []int - for len(version) < 3 { + for { j := i for j < len(s) && s[j] >= '0' && s[j] <= '9' { j++ @@ -269,7 +254,7 @@ func ToProjectName(basename string, newProjectName bool) string { case newProjectName && (r == '_' || r == '-'): return r } - return -1 + return -1 // mutate-exempt: strings.Map drops the rune for any negative value }, strings.ToLower(basename)) } diff --git a/internal/docker/compose_held_test.go b/internal/docker/compose_held_test.go new file mode 100644 index 0000000..e8a5c3a --- /dev/null +++ b/internal/docker/compose_held_test.go @@ -0,0 +1,70 @@ +package docker + +import ( + "errors" + "slices" + "testing" + + "github.com/devcontainers/cli/internal/log" +) + +// Down reports a compose process that could not run and one that exited +// non-zero, and succeeds only on exit 0. +func TestDownFailsUnlessComposeExitsZero(t *testing.T) { + newClient := func(r *fakeRunner) *ComposeClient { + return &ComposeClient{Command: []string{"docker", "compose"}, Log: log.Null, Runner: r} + } + + ok := &fakeRunner{} + if err := newClient(ok).Down(t.Context(), []string{"a.yml"}, "proj", true); err != nil { + t.Fatalf("Down = %v, want nil", err) + } + if want := []string{"compose", "-f", "a.yml", "--project-name", "proj", "down", "--volumes"}; !slices.Equal(ok.gotArgs, want) { + t.Errorf("args = %q, want %q", ok.gotArgs, want) + } + + sentinel := errors.New("exec: docker: not found") + if err := newClient(&fakeRunner{err: sentinel}).Down(t.Context(), nil, "", false); !errors.Is(err, sentinel) { + t.Errorf("Down with a failed run = %v, want it to wrap %v", err, sentinel) + } + if err := newClient(&fakeRunner{code: 1, stderr: []byte("boom")}).Down(t.Context(), nil, "", false); err == nil { + t.Error("Down with exit 1 = nil, want an error") + } +} + +// TS parseVersion (spec-common/commonUtils.ts) keeps every numeric component. +func TestParseComposeVersionKeepsEveryComponent(t *testing.T) { + if got, want := parseComposeVersion("2.24.5.1"), []int{2, 24, 5, 1}; !slices.Equal(got, want) { + t.Errorf("parseComposeVersion = %v, want %v", got, want) + } +} + +// TS compares the whole version with isEarlierVersion, a missing component +// counting as 0: useNewProjectName (dockerCompose.ts) is +// !isEarlierVersion(version, [1, 21, 0]), optimistic when the version is +// unknown, and additional build contexts need !isEarlierVersion(version, +// [2, 17, 0]) (buildAndExtendDockerCompose). A one-component version is +// therefore 1.0.0, 2.0.0 or 3.0.0, not "unknown". +func TestComposeVersionGatesCountAMissingComponentAsZero(t *testing.T) { + for _, tc := range []struct { + version []int + newProjectNames, addlContexts bool + }{ + {nil, true, false}, + {[]int{1}, false, false}, + {[]int{1, 20}, false, false}, + {[]int{1, 21}, true, false}, + {[]int{2}, true, false}, + {[]int{2, 16, 9}, true, false}, + {[]int{2, 17}, true, true}, + {[]int{3}, true, true}, + } { + c := &ComposeClient{Version: tc.version} + if got := c.UsesNewProjectNames(); got != tc.newProjectNames { + t.Errorf("UsesNewProjectNames(%v) = %v, want %v", tc.version, got, tc.newProjectNames) + } + if got := c.SupportsAdditionalContexts(); got != tc.addlContexts { + t.Errorf("SupportsAdditionalContexts(%v) = %v, want %v", tc.version, got, tc.addlContexts) + } + } +} From 21a9676f83b485a8ae3f2413caaeb664167debad Mon Sep 17 00:00:00 2001 From: Manuel de Brito Fontes Date: Sun, 25 Oct 2026 19:30:54 -0300 Subject: [PATCH 07/22] test(docker): hold the engine, dockerfile and context edits without docker With Docker unreachable, as on CI, only the e2e tests refused these edits. Unit tests now hold them through the mock API, the HTTP transport fake, temporary context stores and the BuildKit parser: the endpoint NewEngineClient dials, the seven removal tries (under synctest), image labels without a Config, the pull platform, the continued-FROM and label-insertion paths of EnsureFinalStageName, the escape directive and named-stage labels. Edits nobody can observe are simplified (the Stages preallocation, the Keys loop, the redundant parse-error guard) or exempted (exit codes and offsets returned beside a failure). Co-Authored-By: Claude Opus 5.5 --- internal/docker/client_held_test.go | 16 ++ internal/docker/context_held_test.go | 47 ++++++ internal/docker/dockerfile.go | 25 +-- internal/docker/dockerfile_held_test.go | 94 +++++++++++ internal/docker/engine_held_test.go | 200 ++++++++++++++++++++++++ internal/docker/exec.go | 9 +- 6 files changed, 375 insertions(+), 16 deletions(-) create mode 100644 internal/docker/client_held_test.go create mode 100644 internal/docker/context_held_test.go create mode 100644 internal/docker/dockerfile_held_test.go create mode 100644 internal/docker/engine_held_test.go diff --git a/internal/docker/client_held_test.go b/internal/docker/client_held_test.go new file mode 100644 index 0000000..b4e2d31 --- /dev/null +++ b/internal/docker/client_held_test.go @@ -0,0 +1,16 @@ +package docker + +import ( + "slices" + "testing" +) + +// Every KEY=VALUE build secret becomes `--secret id=KEY,env=KEY` (the contract +// on BuildOptions.Secrets), a one-letter key included. +func TestBuildArgsPassesAOneLetterSecret(t *testing.T) { + args := (&Client{}).buildArgs(BuildOptions{UseBuildx: true, ContextPath: ".", Secrets: []string{"A=1"}}) + i := slices.Index(args, "--secret") + if i < 0 || i+1 >= len(args) || args[i+1] != "id=A,env=A" { + t.Errorf("args = %q, want --secret id=A,env=A", args) + } +} diff --git a/internal/docker/context_held_test.go b/internal/docker/context_held_test.go new file mode 100644 index 0000000..5961371 --- /dev/null +++ b/internal/docker/context_held_test.go @@ -0,0 +1,47 @@ +package docker + +import ( + "os" + "path/filepath" + "testing" +) + +// "default" is the Docker CLI's built-in context: its endpoint comes from the +// environment, never from the context store, even if a meta.json exists for it. +func TestResolveDockerContextHostIgnoresAStoredDefault(t *testing.T) { + dir := t.TempDir() + t.Setenv("DOCKER_CONFIG", dir) + t.Setenv("DOCKER_CONTEXT", "default") + writeContext(t, dir, "default", "tcp://10.0.0.9:2376") + if got := resolveDockerContextHost(); got != "" { + t.Errorf("host = %q, want empty for the default context", got) + } +} + +// A config.json without currentContext selects "default". +func TestActiveContextNameDefaultsWithoutCurrentContext(t *testing.T) { + dir := t.TempDir() + t.Setenv("DOCKER_CONTEXT", "") + if err := os.WriteFile(filepath.Join(dir, "config.json"), []byte(`{"auths":{}}`), 0o644); err != nil { + t.Fatal(err) + } + if got := activeContextName(dir); got != "default" { + t.Errorf("activeContextName = %q, want default", got) + } +} + +// Without DOCKER_CONFIG the store is ~/.docker, as for the Docker CLI. +func TestResolveDockerContextHostReadsTheHomeStore(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("DOCKER_CONFIG", "") + t.Setenv("DOCKER_CONTEXT", "") + dir := filepath.Join(home, ".docker") + writeContext(t, dir, "remote", "tcp://10.0.0.7:2376") + if err := os.WriteFile(filepath.Join(dir, "config.json"), []byte(`{"currentContext":"remote"}`), 0o644); err != nil { + t.Fatal(err) + } + if got := resolveDockerContextHost(); got != "tcp://10.0.0.7:2376" { + t.Errorf("host = %q, want tcp://10.0.0.7:2376 from ~/.docker", got) + } +} diff --git a/internal/docker/dockerfile.go b/internal/docker/dockerfile.go index ae7a840..779b76c 100644 --- a/internal/docker/dockerfile.go +++ b/internal/docker/dockerfile.go @@ -1,6 +1,8 @@ package docker import ( + "maps" + "slices" "strings" "github.com/moby/buildkit/frontend/dockerfile/instructions" @@ -78,7 +80,6 @@ func ExtractDockerfile(content string) *Dockerfile { } } - df.Stages = make([]Stage, 0, len(stages)) for i := range stages { s := &stages[i] st := Stage{From: From{Platform: s.Platform, Image: trimQuotes(s.BaseName), Label: s.Name}} @@ -163,13 +164,7 @@ func derefTrim(p *string) string { type mapEnv map[string]string func (m mapEnv) Get(k string) (string, bool) { v, ok := m[k]; return v, ok } -func (m mapEnv) Keys() []string { - ks := make([]string, 0, len(m)) - for k := range m { - ks = append(ks, k) - } - return ks -} +func (m mapEnv) Keys() []string { return slices.Collect(maps.Keys(m)) } // FindBaseImage resolves the base image for a target stage (or the last stage), // following FROM references to the root image. ${VAR} / ${VAR:+x} / @@ -225,7 +220,9 @@ func EnsureFinalStageName(content, defaultName string) (stageName string, modifi // on a parse error (e.g. a trailing inline comment the strict parser rejects // but the TS tolerates) or no name, fall through to the tolerant scan below. if res, err := parser.Parse(strings.NewReader(content)); err == nil { - if stages, _, perr := instructions.Parse(res.AST, nil); perr == nil && len(stages) > 0 { + // instructions.Parse returns no stages with its error, so an error lands in + // the tolerant scan like "no stages" does. + if stages, _, _ := instructions.Parse(res.AST, nil); len(stages) > 0 { if name := stages[len(stages)-1].Name; name != "" { return name, "" } @@ -236,7 +233,11 @@ func EnsureFinalStageName(content, defaultName string) (stageName string, modifi // it by hand (no regex). If it already has an "AS