From c59544f3ad58cca229c4f9248b4474d68434ddf3 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 14 Jul 2026 00:32:09 +0000 Subject: [PATCH 1/6] feat(linter): route file access through os.Root to prevent path traversal Upcoming Feature/dependsOn resolution will read files at paths derived from untrusted configuration content (e.g. local Feature references like "./features/foo"). As a proactive defense, LintDir and FindConfigs now perform all filesystem access through a single os.Root opened on the directory being linted, so any path derived from configuration content cannot escape it. Behavior change: a configuration file reached via a symbolic link that escapes the lint directory (including any link with an absolute target) is now treated as nonexistent instead of being followed. Links that resolve within the directory keep working. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012ujRvJhfjXKaXwNVGwgxv4 --- linter/linter.go | 74 ++++++++++++++++++++++++++------------ linter/linter_test.go | 82 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 22 deletions(-) diff --git a/linter/linter.go b/linter/linter.go index 2b10e93..dfce5f5 100644 --- a/linter/linter.go +++ b/linter/linter.go @@ -8,6 +8,7 @@ import ( "context" "errors" "fmt" + "io/fs" "os" "path/filepath" "sort" @@ -61,7 +62,9 @@ func (l *Linter) RegisterRule(r *Rule, severity Severity) { // LintDir determines the kind of devcontainer directory dir is (a dev container definition, a // Feature, or a Template), and lints every configuration file it contains. It is an error if dir is -// not a directory or contains no configuration. +// not a directory or contains no configuration. Files are only accessed within dir: symbolic links +// are followed only while they resolve inside dir, and a link escaping dir is treated as +// nonexistent. func (l *Linter) LintDir(ctx context.Context, dir string) ([]Issue, error) { if err := ctx.Err(); err != nil { return nil, fmt.Errorf("aborted %s: %w", dir, err) @@ -73,22 +76,29 @@ func (l *Linter) LintDir(ctx context.Context, dir string) ([]Issue, error) { if !info.IsDir() { return nil, fmt.Errorf("not a directory: %s", dir) } - files := FindConfigs(dir) + root, err := os.OpenRoot(dir) + if err != nil { + return nil, fmt.Errorf("resolve directory: %w", err) + } + // The root is only read from, so a close error is inconsequential. + defer func() { _ = root.Close() }() + files := findConfigs(root) if len(files) == 0 { return nil, fmt.Errorf("no devcontainer configuration found in %s", dir) } var issues []Issue var errs []error for _, f := range files { + display := filepath.Join(dir, f.Path) if err := ctx.Err(); err != nil { - return issues, errors.Join(append(errs, fmt.Errorf("aborted %s: %w", f.Path, err))...) + return issues, errors.Join(append(errs, fmt.Errorf("aborted %s: %w", display, err))...) } - src, err := os.ReadFile(f.Path) + src, err := root.ReadFile(f.Path) if err != nil { - errs = append(errs, fmt.Errorf("read config: %w", err)) + errs = append(errs, fmt.Errorf("read config %s: %w", display, err)) continue } - fileIssues, err := l.Lint(ctx, f.Path, src, f.Type) + fileIssues, err := l.Lint(ctx, display, src, f.Type) if err != nil { errs = append(errs, err) continue @@ -173,38 +183,58 @@ func safeCheck(r *Rule, rctx *Context, node *Node) (findings []Finding) { // the devcontainer specification: .devcontainer/devcontainer.json, .devcontainer.json, and // .devcontainer//devcontainer.json (one level deep). // -// An empty result means dir contains no devcontainer configuration. +// An empty result means dir contains no devcontainer configuration. Files are only accessed within +// dir: symbolic links are followed only while they resolve inside dir, and a link escaping dir is +// treated as nonexistent. func FindConfigs(dir string) []ConfigFile { - if p := filepath.Join(dir, "devcontainer-feature.json"); isFile(p) { + root, err := os.OpenRoot(dir) + if err != nil { + return nil + } + // The root is only read from, so a close error is inconsequential. + defer func() { _ = root.Close() }() + files := findConfigs(root) + for i := range files { + files[i].Path = filepath.Join(dir, files[i].Path) + } + return files +} + +// findConfigs implements FindConfigs against root, returning root-relative paths. All filesystem +// access during a lint run goes through a single *os.Root so that paths derived from configuration +// content (e.g. local Feature references, once dependsOn resolution is implemented) cannot escape +// the directory being linted. +func findConfigs(root *os.Root) []ConfigFile { + if p := "devcontainer-feature.json"; isFile(root, p) { return []ConfigFile{{Path: p, Type: Feature}} } - if p := filepath.Join(dir, "devcontainer-template.json"); isFile(p) { + if p := "devcontainer-template.json"; isFile(root, p) { files := []ConfigFile{{Path: p, Type: Template}} - return append(files, devcontainerConfigs(dir)...) + return append(files, devcontainerConfigs(root)...) } - return devcontainerConfigs(dir) + return devcontainerConfigs(root) } -// devcontainerConfigs returns the devcontainer.json files under dir at the locations defined by the -// devcontainer specification. -func devcontainerConfigs(dir string) []ConfigFile { +// devcontainerConfigs returns the devcontainer.json files under root at the locations defined by +// the devcontainer specification, as root-relative paths. +func devcontainerConfigs(root *os.Root) []ConfigFile { var paths []string for _, p := range []string{ - filepath.Join(dir, ".devcontainer", "devcontainer.json"), - filepath.Join(dir, ".devcontainer.json"), + filepath.Join(".devcontainer", "devcontainer.json"), + ".devcontainer.json", } { - if isFile(p) { + if isFile(root, p) { paths = append(paths, p) } } - entries, err := os.ReadDir(filepath.Join(dir, ".devcontainer")) + entries, err := fs.ReadDir(root.FS(), ".devcontainer") if err == nil { for _, e := range entries { if !e.IsDir() { continue } - p := filepath.Join(dir, ".devcontainer", e.Name(), "devcontainer.json") - if isFile(p) { + p := filepath.Join(".devcontainer", e.Name(), "devcontainer.json") + if isFile(root, p) { paths = append(paths, p) } } @@ -217,7 +247,7 @@ func devcontainerConfigs(dir string) []ConfigFile { return files } -func isFile(path string) bool { - info, err := os.Stat(path) +func isFile(root *os.Root, path string) bool { + info, err := root.Stat(path) return err == nil && !info.IsDir() } diff --git a/linter/linter_test.go b/linter/linter_test.go index 277552b..3470fb2 100644 --- a/linter/linter_test.go +++ b/linter/linter_test.go @@ -1,6 +1,7 @@ package linter_test import ( + "os" "path/filepath" "slices" "testing" @@ -91,6 +92,87 @@ func TestFindConfigs(t *testing.T) { } } +// symlink creates a symbolic link, skipping the test on platforms where symlink creation is not +// permitted (e.g. Windows without the required privilege). +func symlink(t *testing.T, target, link string) { + t.Helper() + if err := os.Symlink(target, link); err != nil { + t.Skipf("symlink unsupported: %v", err) + } +} + +func TestFindConfigsSymlink(t *testing.T) { + t.Parallel() + + t.Run("link escaping the directory is treated as nonexistent", func(t *testing.T) { + t.Parallel() + tmp := t.TempDir() + outside := filepath.Join(tmp, "outside") + proj := filepath.Join(tmp, "proj") + if err := os.MkdirAll(filepath.Join(proj, ".devcontainer"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(outside, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(outside, "devcontainer.json"), []byte(`{}`), 0o644); err != nil { + t.Fatal(err) + } + symlink(t, filepath.Join("..", "..", "outside", "devcontainer.json"), + filepath.Join(proj, ".devcontainer", "devcontainer.json")) + + if got := linter.FindConfigs(proj); len(got) != 0 { + t.Errorf("linter.FindConfigs(%q) = %v, want none", proj, got) + } + l := linter.New() + if _, err := l.LintDir(t.Context(), proj); err == nil { + t.Error("LintDir: got nil error, want 'no devcontainer configuration found'") + } + }) + + t.Run("link with an absolute target is treated as nonexistent", func(t *testing.T) { + t.Parallel() + proj := t.TempDir() + if err := os.MkdirAll(filepath.Join(proj, ".devcontainer"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(proj, "real.json"), []byte(`{}`), 0o644); err != nil { + t.Fatal(err) + } + // The target is inside the directory, but os.Root rejects absolute symlink targets. + symlink(t, filepath.Join(proj, "real.json"), + filepath.Join(proj, ".devcontainer", "devcontainer.json")) + + if got := linter.FindConfigs(proj); len(got) != 0 { + t.Errorf("linter.FindConfigs(%q) = %v, want none", proj, got) + } + }) + + t.Run("link resolving within the directory is followed", func(t *testing.T) { + t.Parallel() + proj := t.TempDir() + if err := os.MkdirAll(filepath.Join(proj, ".devcontainer"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(proj, "real.json"), []byte(`{"image": "ubuntu:24.04"}`), 0o644); err != nil { + t.Fatal(err) + } + symlink(t, filepath.Join("..", "real.json"), + filepath.Join(proj, ".devcontainer", "devcontainer.json")) + + want := []linter.ConfigFile{ + {filepath.Join(proj, ".devcontainer", "devcontainer.json"), linter.Devcontainer}, + } + if got := linter.FindConfigs(proj); !slices.Equal(got, want) { + t.Errorf("linter.FindConfigs(%q) = %v, want %v", proj, got, want) + } + l := linter.New() + if _, err := l.LintDir(t.Context(), proj); err != nil { + t.Errorf("LintDir: %v", err) + } + }) +} + func TestLintDir(t *testing.T) { t.Parallel() From 7cf630e9f86e9fee8d6a0c83750822faed6d055b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 14 Jul 2026 01:25:56 +0000 Subject: [PATCH 2/6] fix(linter): confine .devcontainer file access to the .devcontainer directory The os.Root opened by LintDir covers the lint-target directory, which is the parent of .devcontainer, so a symbolic link such as .devcontainer/devcontainer.json -> ../secret.json could still reach unrelated files in the project root. Configuration files under .devcontainer are now discovered and read through a sub-root confined to that directory. The boundary is .devcontainer itself rather than each config's own subfolder, because Features stored inside the active .devcontainer directory can become dependsOn targets once Feature resolution is implemented. Also remove the exported FindConfigs, which lost its last production caller in the os.Root refactoring; discovery is tested directly against the unexported findConfigs instead. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012ujRvJhfjXKaXwNVGwgxv4 --- linter/linter.go | 93 +++++++++++---------- linter/linter_internal_test.go | 79 ++++++++++++++++++ linter/linter_test.go | 148 ++++++++++++--------------------- 3 files changed, 183 insertions(+), 137 deletions(-) create mode 100644 linter/linter_internal_test.go diff --git a/linter/linter.go b/linter/linter.go index dfce5f5..19db6a9 100644 --- a/linter/linter.go +++ b/linter/linter.go @@ -12,6 +12,7 @@ import ( "os" "path/filepath" "sort" + "strings" "github.com/tailscale/hujson" ) @@ -62,9 +63,10 @@ func (l *Linter) RegisterRule(r *Rule, severity Severity) { // LintDir determines the kind of devcontainer directory dir is (a dev container definition, a // Feature, or a Template), and lints every configuration file it contains. It is an error if dir is -// not a directory or contains no configuration. Files are only accessed within dir: symbolic links -// are followed only while they resolve inside dir, and a link escaping dir is treated as -// nonexistent. +// not a directory or contains no configuration. Configuration files under dir's .devcontainer +// directory are only accessed within that directory, and other configuration files only within dir: +// symbolic links are followed only while they resolve inside that boundary, and a link escaping it +// is treated as nonexistent. func (l *Linter) LintDir(ctx context.Context, dir string) ([]Issue, error) { if err := ctx.Err(); err != nil { return nil, fmt.Errorf("aborted %s: %w", dir, err) @@ -93,7 +95,7 @@ func (l *Linter) LintDir(ctx context.Context, dir string) ([]Issue, error) { if err := ctx.Err(); err != nil { return issues, errors.Join(append(errs, fmt.Errorf("aborted %s: %w", display, err))...) } - src, err := root.ReadFile(f.Path) + src, err := readConfig(root, f.Path) if err != nil { errs = append(errs, fmt.Errorf("read config %s: %w", display, err)) continue @@ -173,8 +175,8 @@ func safeCheck(r *Rule, rctx *Context, node *Node) (findings []Finding) { return r.Check(rctx, node) } -// FindConfigs determines the kind of devcontainer directory dir is and returns the configuration -// files it contains: +// findConfigs determines the kind of devcontainer directory root is opened on and returns the +// configuration files it contains, as root-relative paths: // // - a Feature (dir contains devcontainer-feature.json): that file; // - a Template (dir contains devcontainer-template.json): that file, plus the dev container @@ -183,27 +185,10 @@ func safeCheck(r *Rule, rctx *Context, node *Node) (findings []Finding) { // the devcontainer specification: .devcontainer/devcontainer.json, .devcontainer.json, and // .devcontainer//devcontainer.json (one level deep). // -// An empty result means dir contains no devcontainer configuration. Files are only accessed within -// dir: symbolic links are followed only while they resolve inside dir, and a link escaping dir is -// treated as nonexistent. -func FindConfigs(dir string) []ConfigFile { - root, err := os.OpenRoot(dir) - if err != nil { - return nil - } - // The root is only read from, so a close error is inconsequential. - defer func() { _ = root.Close() }() - files := findConfigs(root) - for i := range files { - files[i].Path = filepath.Join(dir, files[i].Path) - } - return files -} - -// findConfigs implements FindConfigs against root, returning root-relative paths. All filesystem -// access during a lint run goes through a single *os.Root so that paths derived from configuration -// content (e.g. local Feature references, once dependsOn resolution is implemented) cannot escape -// the directory being linted. +// An empty result means the directory contains no devcontainer configuration. Everything under the +// .devcontainer directory is accessed through a root confined to that directory: the future +// Feature/dependsOn resolver receives the same boundary, so local Feature references — including +// Features stored inside the active .devcontainer directory — resolve within it. func findConfigs(root *os.Root) []ConfigFile { if p := "devcontainer-feature.json"; isFile(root, p) { return []ConfigFile{{Path: p, Type: Feature}} @@ -215,27 +200,32 @@ func findConfigs(root *os.Root) []ConfigFile { return devcontainerConfigs(root) } +// devcontainerDir is the directory that holds a dev container definition's configuration, and the +// boundary that access to that configuration is confined to. +const devcontainerDir = ".devcontainer" + // devcontainerConfigs returns the devcontainer.json files under root at the locations defined by // the devcontainer specification, as root-relative paths. func devcontainerConfigs(root *os.Root) []ConfigFile { var paths []string - for _, p := range []string{ - filepath.Join(".devcontainer", "devcontainer.json"), - ".devcontainer.json", - } { - if isFile(root, p) { - paths = append(paths, p) - } + if isFile(root, ".devcontainer.json") { + paths = append(paths, ".devcontainer.json") } - entries, err := fs.ReadDir(root.FS(), ".devcontainer") - if err == nil { - for _, e := range entries { - if !e.IsDir() { - continue - } - p := filepath.Join(".devcontainer", e.Name(), "devcontainer.json") - if isFile(root, p) { - paths = append(paths, p) + if sub, err := root.OpenRoot(devcontainerDir); err == nil { + // The root is only read from, so a close error is inconsequential. + defer func() { _ = sub.Close() }() + if isFile(sub, "devcontainer.json") { + paths = append(paths, filepath.Join(devcontainerDir, "devcontainer.json")) + } + entries, err := fs.ReadDir(sub.FS(), ".") + if err == nil { + for _, e := range entries { + if !e.IsDir() { + continue + } + if isFile(sub, filepath.Join(e.Name(), "devcontainer.json")) { + paths = append(paths, filepath.Join(devcontainerDir, e.Name(), "devcontainer.json")) + } } } } @@ -247,6 +237,23 @@ func devcontainerConfigs(root *os.Root) []ConfigFile { return files } +// readConfig reads the configuration file at name, which is relative to root. A file under the +// .devcontainer directory is accessed through a root confined to that directory, so its resolution +// cannot reach files outside of it. +func readConfig(root *os.Root, name string) ([]byte, error) { + rest, ok := strings.CutPrefix(name, devcontainerDir+string(filepath.Separator)) + if !ok { + return root.ReadFile(name) + } + sub, err := root.OpenRoot(devcontainerDir) + if err != nil { + return nil, err + } + // The root is only read from, so a close error is inconsequential. + defer func() { _ = sub.Close() }() + return sub.ReadFile(rest) +} + func isFile(root *os.Root, path string) bool { info, err := root.Stat(path) return err == nil && !info.IsDir() diff --git a/linter/linter_internal_test.go b/linter/linter_internal_test.go new file mode 100644 index 0000000..d432852 --- /dev/null +++ b/linter/linter_internal_test.go @@ -0,0 +1,79 @@ +package linter + +import ( + "os" + "path/filepath" + "slices" + "testing" +) + +func TestFindConfigs(t *testing.T) { + t.Parallel() + + tests := []struct { + dir string + want []ConfigFile + }{ + { + "testdata/project", + []ConfigFile{ + {filepath.Join(".devcontainer", "devcontainer.json"), Devcontainer}, + {filepath.Join(".devcontainer", "go", "devcontainer.json"), Devcontainer}, + }, + }, + { + "testdata/rootfile", + []ConfigFile{ + {".devcontainer.json", Devcontainer}, + }, + }, + { + "testdata/feature", + []ConfigFile{ + {"devcontainer-feature.json", Feature}, + }, + }, + { + "testdata/template", + []ConfigFile{ + {"devcontainer-template.json", Template}, + {filepath.Join(".devcontainer", "devcontainer.json"), Devcontainer}, + }, + }, + { + "testdata/template-rootfile", + []ConfigFile{ + {"devcontainer-template.json", Template}, + {".devcontainer.json", Devcontainer}, + }, + }, + { + "testdata/template-subfolder", + []ConfigFile{ + {"devcontainer-template.json", Template}, + {filepath.Join(".devcontainer", "go", "devcontainer.json"), Devcontainer}, + }, + }, + { + "testdata/template-no-devcontainer", + []ConfigFile{ + {"devcontainer-template.json", Template}, + }, + }, + {"testdata", nil}, + } + for _, tt := range tests { + t.Run(tt.dir, func(t *testing.T) { + t.Parallel() + root, err := os.OpenRoot(tt.dir) + if err != nil { + t.Fatalf("os.OpenRoot(%q): %v", tt.dir, err) + } + t.Cleanup(func() { _ = root.Close() }) + got := findConfigs(root) + if !slices.Equal(got, tt.want) { + t.Errorf("findConfigs(%q) = %v, want %v", tt.dir, got, tt.want) + } + }) + } +} diff --git a/linter/linter_test.go b/linter/linter_test.go index 3470fb2..67f4d46 100644 --- a/linter/linter_test.go +++ b/linter/linter_test.go @@ -3,7 +3,6 @@ package linter_test import ( "os" "path/filepath" - "slices" "testing" "github.com/bare-devcontainer/decolint/linter" @@ -26,72 +25,6 @@ func lintSrc(t *testing.T, src string) []linter.Issue { return issues } -func TestFindConfigs(t *testing.T) { - t.Parallel() - - tests := []struct { - dir string - want []linter.ConfigFile - }{ - { - "testdata/project", - []linter.ConfigFile{ - {filepath.Join("testdata", "project", ".devcontainer", "devcontainer.json"), linter.Devcontainer}, - {filepath.Join("testdata", "project", ".devcontainer", "go", "devcontainer.json"), linter.Devcontainer}, - }, - }, - { - "testdata/rootfile", - []linter.ConfigFile{ - {filepath.Join("testdata", "rootfile", ".devcontainer.json"), linter.Devcontainer}, - }, - }, - { - "testdata/feature", - []linter.ConfigFile{ - {filepath.Join("testdata", "feature", "devcontainer-feature.json"), linter.Feature}, - }, - }, - { - "testdata/template", - []linter.ConfigFile{ - {filepath.Join("testdata", "template", "devcontainer-template.json"), linter.Template}, - {filepath.Join("testdata", "template", ".devcontainer", "devcontainer.json"), linter.Devcontainer}, - }, - }, - { - "testdata/template-rootfile", - []linter.ConfigFile{ - {filepath.Join("testdata", "template-rootfile", "devcontainer-template.json"), linter.Template}, - {filepath.Join("testdata", "template-rootfile", ".devcontainer.json"), linter.Devcontainer}, - }, - }, - { - "testdata/template-subfolder", - []linter.ConfigFile{ - {filepath.Join("testdata", "template-subfolder", "devcontainer-template.json"), linter.Template}, - {filepath.Join("testdata", "template-subfolder", ".devcontainer", "go", "devcontainer.json"), linter.Devcontainer}, - }, - }, - { - "testdata/template-no-devcontainer", - []linter.ConfigFile{ - {filepath.Join("testdata", "template-no-devcontainer", "devcontainer-template.json"), linter.Template}, - }, - }, - {"testdata", nil}, - } - for _, tt := range tests { - t.Run(tt.dir, func(t *testing.T) { - t.Parallel() - got := linter.FindConfigs(tt.dir) - if !slices.Equal(got, tt.want) { - t.Errorf("linter.FindConfigs(%q) = %v, want %v", tt.dir, got, tt.want) - } - }) - } -} - // symlink creates a symbolic link, skipping the test on platforms where symlink creation is not // permitted (e.g. Windows without the required privilege). func symlink(t *testing.T, target, link string) { @@ -101,29 +34,48 @@ func symlink(t *testing.T, target, link string) { } } -func TestFindConfigsSymlink(t *testing.T) { +func TestLintDirSymlink(t *testing.T) { t.Parallel() - t.Run("link escaping the directory is treated as nonexistent", func(t *testing.T) { - t.Parallel() - tmp := t.TempDir() - outside := filepath.Join(tmp, "outside") - proj := filepath.Join(tmp, "proj") + // setup creates a dev container definition directory whose .devcontainer/devcontainer.json is a + // symbolic link to target, and returns the directory. + setup := func(t *testing.T, target string) string { + t.Helper() + proj := t.TempDir() if err := os.MkdirAll(filepath.Join(proj, ".devcontainer"), 0o755); err != nil { t.Fatal(err) } - if err := os.MkdirAll(outside, 0o755); err != nil { + symlink(t, target, filepath.Join(proj, ".devcontainer", "devcontainer.json")) + return proj + } + + t.Run("link escaping the lint directory is treated as nonexistent", func(t *testing.T) { + t.Parallel() + tmp := t.TempDir() + if err := os.WriteFile(filepath.Join(tmp, "devcontainer.json"), []byte(`{}`), 0o644); err != nil { t.Fatal(err) } - if err := os.WriteFile(filepath.Join(outside, "devcontainer.json"), []byte(`{}`), 0o644); err != nil { + proj := filepath.Join(tmp, "proj") + if err := os.MkdirAll(filepath.Join(proj, ".devcontainer"), 0o755); err != nil { t.Fatal(err) } - symlink(t, filepath.Join("..", "..", "outside", "devcontainer.json"), + symlink(t, filepath.Join("..", "..", "devcontainer.json"), filepath.Join(proj, ".devcontainer", "devcontainer.json")) - if got := linter.FindConfigs(proj); len(got) != 0 { - t.Errorf("linter.FindConfigs(%q) = %v, want none", proj, got) + l := linter.New() + if _, err := l.LintDir(t.Context(), proj); err == nil { + t.Error("LintDir: got nil error, want 'no devcontainer configuration found'") + } + }) + + t.Run("link leaving .devcontainer is treated as nonexistent", func(t *testing.T) { + t.Parallel() + proj := setup(t, filepath.Join("..", "real.json")) + // The target is inside the lint directory, but outside .devcontainer. + if err := os.WriteFile(filepath.Join(proj, "real.json"), []byte(`{}`), 0o644); err != nil { + t.Fatal(err) } + l := linter.New() if _, err := l.LintDir(t.Context(), proj); err == nil { t.Error("LintDir: got nil error, want 'no devcontainer configuration found'") @@ -132,40 +84,48 @@ func TestFindConfigsSymlink(t *testing.T) { t.Run("link with an absolute target is treated as nonexistent", func(t *testing.T) { t.Parallel() + // The target is inside .devcontainer, but os.Root rejects absolute symlink targets. proj := t.TempDir() if err := os.MkdirAll(filepath.Join(proj, ".devcontainer"), 0o755); err != nil { t.Fatal(err) } - if err := os.WriteFile(filepath.Join(proj, "real.json"), []byte(`{}`), 0o644); err != nil { + if err := os.WriteFile(filepath.Join(proj, ".devcontainer", "real.json"), []byte(`{}`), 0o644); err != nil { t.Fatal(err) } - // The target is inside the directory, but os.Root rejects absolute symlink targets. - symlink(t, filepath.Join(proj, "real.json"), + symlink(t, filepath.Join(proj, ".devcontainer", "real.json"), filepath.Join(proj, ".devcontainer", "devcontainer.json")) - if got := linter.FindConfigs(proj); len(got) != 0 { - t.Errorf("linter.FindConfigs(%q) = %v, want none", proj, got) + l := linter.New() + if _, err := l.LintDir(t.Context(), proj); err == nil { + t.Error("LintDir: got nil error, want 'no devcontainer configuration found'") } }) - t.Run("link resolving within the directory is followed", func(t *testing.T) { + t.Run("link resolving within .devcontainer is followed", func(t *testing.T) { t.Parallel() - proj := t.TempDir() - if err := os.MkdirAll(filepath.Join(proj, ".devcontainer"), 0o755); err != nil { + proj := setup(t, "main.json") + if err := os.WriteFile(filepath.Join(proj, ".devcontainer", "main.json"), []byte(`{}`), 0o644); err != nil { t.Fatal(err) } - if err := os.WriteFile(filepath.Join(proj, "real.json"), []byte(`{"image": "ubuntu:24.04"}`), 0o644); err != nil { - t.Fatal(err) + + l := linter.New() + if _, err := l.LintDir(t.Context(), proj); err != nil { + t.Errorf("LintDir: %v", err) } - symlink(t, filepath.Join("..", "real.json"), - filepath.Join(proj, ".devcontainer", "devcontainer.json")) + }) - want := []linter.ConfigFile{ - {filepath.Join(proj, ".devcontainer", "devcontainer.json"), linter.Devcontainer}, + t.Run("link from a subfolder resolving within .devcontainer is followed", func(t *testing.T) { + t.Parallel() + proj := t.TempDir() + if err := os.MkdirAll(filepath.Join(proj, ".devcontainer", "go"), 0o755); err != nil { + t.Fatal(err) } - if got := linter.FindConfigs(proj); !slices.Equal(got, want) { - t.Errorf("linter.FindConfigs(%q) = %v, want %v", proj, got, want) + if err := os.WriteFile(filepath.Join(proj, ".devcontainer", "shared.json"), []byte(`{}`), 0o644); err != nil { + t.Fatal(err) } + symlink(t, filepath.Join("..", "shared.json"), + filepath.Join(proj, ".devcontainer", "go", "devcontainer.json")) + l := linter.New() if _, err := l.LintDir(t.Context(), proj); err != nil { t.Errorf("LintDir: %v", err) From 3371504fd11e1e94bc04dd2c9a2361b6bba726b0 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 14 Jul 2026 10:26:08 +0000 Subject: [PATCH 3/6] chore(linter): accept an opened os.Root in LintDir Move the os.Root creation out of LintDir and into the CLI: main.go opens each lint path as an os.Root and LintDir only reads files and directories through the root it receives. The not-a-directory check now falls out of os.OpenRoot itself. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012ujRvJhfjXKaXwNVGwgxv4 --- cmd/decolint/main.go | 14 ++++++++++++- cmd/decolint/main_test.go | 12 ++++++++++++ linter/linter.go | 29 +++++++++------------------ linter/linter_test.go | 41 +++++++++++++++++++++------------------ 4 files changed, 56 insertions(+), 40 deletions(-) diff --git a/cmd/decolint/main.go b/cmd/decolint/main.go index a69684b..372be40 100644 --- a/cmd/decolint/main.go +++ b/cmd/decolint/main.go @@ -237,7 +237,7 @@ func runLint(ctx context.Context, stdout io.Writer, opts Options, cfg Config) (b var worstSeverity linter.Severity var lintErr error for _, path := range opts.Paths { - issues, err := l.LintDir(ctx, path) + issues, err := lintPath(ctx, l, path) for _, issue := range issues { if issue.Severity > worstSeverity { worstSeverity = issue.Severity @@ -258,3 +258,15 @@ func runLint(ctx context.Context, stdout io.Writer, opts Options, cfg Config) (b return worstSeverity >= threshold, nil } + +// lintPath lints the devcontainer directory at path. The directory is opened as an os.Root, so +// every file the lint reads is confined to it. It is an error if path is not a directory. +func lintPath(ctx context.Context, l *linter.Linter, path string) ([]linter.Issue, error) { + root, err := os.OpenRoot(path) + if err != nil { + return nil, fmt.Errorf("resolve directory: %w", err) + } + // The root is only read from, so a close error is inconsequential. + defer func() { _ = root.Close() }() + return l.LintDir(ctx, root) +} diff --git a/cmd/decolint/main_test.go b/cmd/decolint/main_test.go index f0e2591..5060d83 100644 --- a/cmd/decolint/main_test.go +++ b/cmd/decolint/main_test.go @@ -498,6 +498,18 @@ func TestRunLint(t *testing.T) { } }) + t.Run("file path is rejected", func(t *testing.T) { + t.Parallel() + dir := writeDevcontainer(t, `{"image": "ubuntu:24.04"}`) + file := filepath.Join(dir, ".devcontainer", "devcontainer.json") + + var stdout bytes.Buffer + hasIssue, runErr := runLint(t.Context(), &stdout, Options{Paths: []string{file}, Format: format.TextFormat{}}, Config{}) + if runErr == nil || hasIssue { + t.Errorf("hasIssue = %v, err = %v, want false, 'not a directory'", hasIssue, runErr) + } + }) + t.Run("override for unselected platform-scoped rule is not an error", func(t *testing.T) { t.Parallel() dir := writeDevcontainer(t, `{"image": "ubuntu:latest"}`) diff --git a/linter/linter.go b/linter/linter.go index 19db6a9..32b577c 100644 --- a/linter/linter.go +++ b/linter/linter.go @@ -61,29 +61,18 @@ func (l *Linter) RegisterRule(r *Rule, severity Severity) { } } -// LintDir determines the kind of devcontainer directory dir is (a dev container definition, a -// Feature, or a Template), and lints every configuration file it contains. It is an error if dir is -// not a directory or contains no configuration. Configuration files under dir's .devcontainer -// directory are only accessed within that directory, and other configuration files only within dir: -// symbolic links are followed only while they resolve inside that boundary, and a link escaping it -// is treated as nonexistent. -func (l *Linter) LintDir(ctx context.Context, dir string) ([]Issue, error) { +// LintDir determines the kind of devcontainer directory root is opened on (a dev container +// definition, a Feature, or a Template), and lints every configuration file it contains. It is an +// error if the directory contains no configuration. All file access happens through root, so it is +// confined to that directory; configuration files under its .devcontainer directory are only +// accessed within that directory. Symbolic links are followed only while they resolve inside that +// boundary, and a link escaping it is treated as nonexistent. Issue paths are the files' locations +// joined onto root's name. +func (l *Linter) LintDir(ctx context.Context, root *os.Root) ([]Issue, error) { + dir := root.Name() if err := ctx.Err(); err != nil { return nil, fmt.Errorf("aborted %s: %w", dir, err) } - info, err := os.Stat(dir) - if err != nil { - return nil, fmt.Errorf("resolve directory: %w", err) - } - if !info.IsDir() { - return nil, fmt.Errorf("not a directory: %s", dir) - } - root, err := os.OpenRoot(dir) - if err != nil { - return nil, fmt.Errorf("resolve directory: %w", err) - } - // The root is only read from, so a close error is inconsequential. - defer func() { _ = root.Close() }() files := findConfigs(root) if len(files) == 0 { return nil, fmt.Errorf("no devcontainer configuration found in %s", dir) diff --git a/linter/linter_test.go b/linter/linter_test.go index 67f4d46..e667f0f 100644 --- a/linter/linter_test.go +++ b/linter/linter_test.go @@ -34,6 +34,17 @@ func symlink(t *testing.T, target, link string) { } } +// openRoot opens dir as an os.Root, closed when the test ends, to lint through. +func openRoot(t *testing.T, dir string) *os.Root { + t.Helper() + root, err := os.OpenRoot(dir) + if err != nil { + t.Fatalf("os.OpenRoot(%q): %v", dir, err) + } + t.Cleanup(func() { _ = root.Close() }) + return root +} + func TestLintDirSymlink(t *testing.T) { t.Parallel() @@ -63,7 +74,7 @@ func TestLintDirSymlink(t *testing.T) { filepath.Join(proj, ".devcontainer", "devcontainer.json")) l := linter.New() - if _, err := l.LintDir(t.Context(), proj); err == nil { + if _, err := l.LintDir(t.Context(), openRoot(t, proj)); err == nil { t.Error("LintDir: got nil error, want 'no devcontainer configuration found'") } }) @@ -77,7 +88,7 @@ func TestLintDirSymlink(t *testing.T) { } l := linter.New() - if _, err := l.LintDir(t.Context(), proj); err == nil { + if _, err := l.LintDir(t.Context(), openRoot(t, proj)); err == nil { t.Error("LintDir: got nil error, want 'no devcontainer configuration found'") } }) @@ -96,7 +107,7 @@ func TestLintDirSymlink(t *testing.T) { filepath.Join(proj, ".devcontainer", "devcontainer.json")) l := linter.New() - if _, err := l.LintDir(t.Context(), proj); err == nil { + if _, err := l.LintDir(t.Context(), openRoot(t, proj)); err == nil { t.Error("LintDir: got nil error, want 'no devcontainer configuration found'") } }) @@ -109,7 +120,7 @@ func TestLintDirSymlink(t *testing.T) { } l := linter.New() - if _, err := l.LintDir(t.Context(), proj); err != nil { + if _, err := l.LintDir(t.Context(), openRoot(t, proj)); err != nil { t.Errorf("LintDir: %v", err) } }) @@ -127,7 +138,7 @@ func TestLintDirSymlink(t *testing.T) { filepath.Join(proj, ".devcontainer", "go", "devcontainer.json")) l := linter.New() - if _, err := l.LintDir(t.Context(), proj); err != nil { + if _, err := l.LintDir(t.Context(), openRoot(t, proj)); err != nil { t.Errorf("LintDir: %v", err) } }) @@ -146,7 +157,7 @@ func TestLintDir(t *testing.T) { t.Run("definition with multiple configs", func(t *testing.T) { t.Parallel() - issues, err := l.LintDir(t.Context(), "testdata/project") + issues, err := l.LintDir(t.Context(), openRoot(t, "testdata/project")) if err != nil { t.Fatalf("LintDir: %v", err) } @@ -166,7 +177,7 @@ func TestLintDir(t *testing.T) { t.Run("clean definition", func(t *testing.T) { t.Parallel() - issues, err := l.LintDir(t.Context(), "testdata/rootfile") + issues, err := l.LintDir(t.Context(), openRoot(t, "testdata/rootfile")) if err != nil { t.Fatalf("LintDir: %v", err) } @@ -177,7 +188,7 @@ func TestLintDir(t *testing.T) { t.Run("feature is not checked by devcontainer rules", func(t *testing.T) { t.Parallel() - issues, err := l.LintDir(t.Context(), "testdata/feature") + issues, err := l.LintDir(t.Context(), openRoot(t, "testdata/feature")) if err != nil { t.Fatalf("LintDir: %v", err) } @@ -188,7 +199,7 @@ func TestLintDir(t *testing.T) { t.Run("template lints the shipped devcontainer config", func(t *testing.T) { t.Parallel() - issues, err := l.LintDir(t.Context(), "testdata/template") + issues, err := l.LintDir(t.Context(), openRoot(t, "testdata/template")) if err != nil { t.Fatalf("LintDir: %v", err) } @@ -203,7 +214,7 @@ func TestLintDir(t *testing.T) { t.Run("a broken file does not stop other files in the same directory from being linted", func(t *testing.T) { t.Parallel() - issues, err := l.LintDir(t.Context(), "testdata/broken") + issues, err := l.LintDir(t.Context(), openRoot(t, "testdata/broken")) if err == nil { t.Fatal("got nil error, want a parse error for the broken config") } @@ -216,17 +227,9 @@ func TestLintDir(t *testing.T) { } }) - t.Run("file path is rejected", func(t *testing.T) { - t.Parallel() - file := filepath.Join("testdata", "project", ".devcontainer", "devcontainer.json") - if _, err := l.LintDir(t.Context(), file); err == nil { - t.Error("got nil error, want 'not a directory'") - } - }) - t.Run("directory without config", func(t *testing.T) { t.Parallel() - if _, err := l.LintDir(t.Context(), t.TempDir()); err == nil { + if _, err := l.LintDir(t.Context(), openRoot(t, t.TempDir())); err == nil { t.Error("got nil error, want 'no devcontainer configuration found'") } }) From 317ab373de5f51e474f5bbb34a679c5a28e2d7df Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 14 Jul 2026 11:58:35 +0000 Subject: [PATCH 4/6] chore(linter): visit configs via a callback and open the .devcontainer root once Replace findConfigs/readConfig with visitConfigs, which walks the configuration files and hands each one to a callback together with the os.Root it must be read through. The whole walk completes within one call, so the .devcontainer sub-root is opened once and closed by a defer, instead of being reopened by readConfig for every file. The per-file work is extracted into lintConfig, giving each file its own function scope for deferred cleanup. A broken file still does not stop the remaining files from being linted; only a callback error (context cancellation) aborts the visit. The now-unused exported ConfigFile type is removed. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012ujRvJhfjXKaXwNVGwgxv4 --- linter/linter.go | 155 +++++++++++++++++---------------- linter/linter_internal_test.go | 70 ++++++++++++--- 2 files changed, 138 insertions(+), 87 deletions(-) diff --git a/linter/linter.go b/linter/linter.go index 32b577c..19c28de 100644 --- a/linter/linter.go +++ b/linter/linter.go @@ -12,7 +12,6 @@ import ( "os" "path/filepath" "sort" - "strings" "github.com/tailscale/hujson" ) @@ -31,10 +30,14 @@ func (i Issue) String() string { return fmt.Sprintf("%s:%d:%d: %s: %s (%s)", i.Path, i.Line, i.Col, i.Severity, i.Message, i.RuleID) } -// ConfigFile is a configuration file detected in a directory. -type ConfigFile struct { - Path string - Type FileType +// configEntry is a discovered configuration file and the boundary it must be read through: root is +// the os.Root confining access (the lint root, or its .devcontainer sub-root), and path is relative +// to that root. rel is the path relative to the lint directory, for display. +type configEntry struct { + root *os.Root + path string + rel string + typ FileType } // Linter runs a set of rules against devcontainer configuration files. @@ -73,32 +76,45 @@ func (l *Linter) LintDir(ctx context.Context, root *os.Root) ([]Issue, error) { if err := ctx.Err(); err != nil { return nil, fmt.Errorf("aborted %s: %w", dir, err) } - files := findConfigs(root) - if len(files) == 0 { - return nil, fmt.Errorf("no devcontainer configuration found in %s", dir) - } var issues []Issue var errs []error - for _, f := range files { - display := filepath.Join(dir, f.Path) + found := false + err := visitConfigs(root, func(f configEntry) error { + found = true if err := ctx.Err(); err != nil { - return issues, errors.Join(append(errs, fmt.Errorf("aborted %s: %w", display, err))...) - } - src, err := readConfig(root, f.Path) - if err != nil { - errs = append(errs, fmt.Errorf("read config %s: %w", display, err)) - continue + return fmt.Errorf("aborted %s: %w", filepath.Join(dir, f.rel), err) } - fileIssues, err := l.Lint(ctx, display, src, f.Type) + fileIssues, err := l.lintConfig(ctx, dir, f) if err != nil { + // A broken file must not stop the remaining files from being linted, so record the + // error and keep visiting. errs = append(errs, err) - continue + return nil } issues = append(issues, fileIssues...) + return nil + }) + if err != nil { + return issues, errors.Join(append(errs, err)...) + } + if !found { + return nil, fmt.Errorf("no devcontainer configuration found in %s", dir) } return issues, errors.Join(errs...) } +// lintConfig reads and lints the single configuration file f, reporting issues under +// filepath.Join(dir, f.rel). The file is read through f.root, so its resolution cannot escape that +// boundary. +func (l *Linter) lintConfig(ctx context.Context, dir string, f configEntry) ([]Issue, error) { + display := filepath.Join(dir, f.rel) + src, err := f.root.ReadFile(f.path) + if err != nil { + return nil, fmt.Errorf("read config %s: %w", display, err) + } + return l.Lint(ctx, display, src, f.typ) +} + // Lint lints src, which is the content of a configuration file of the given type. path is used only // for reporting. func (l *Linter) Lint(ctx context.Context, path string, src []byte, fileType FileType) ([]Issue, error) { @@ -164,83 +180,76 @@ func safeCheck(r *Rule, rctx *Context, node *Node) (findings []Finding) { return r.Check(rctx, node) } -// findConfigs determines the kind of devcontainer directory root is opened on and returns the -// configuration files it contains, as root-relative paths: +// visitConfigs determines the kind of devcontainer directory root is opened on and calls fn once +// for each configuration file it contains: // // - a Feature (dir contains devcontainer-feature.json): that file; // - a Template (dir contains devcontainer-template.json): that file, plus the dev container // configuration the template ships; // - otherwise, a dev container definition: the configuration files at the locations defined by -// the devcontainer specification: .devcontainer/devcontainer.json, .devcontainer.json, and -// .devcontainer//devcontainer.json (one level deep). +// the devcontainer specification: .devcontainer.json, .devcontainer/devcontainer.json, and +// .devcontainer//devcontainer.json (one level deep), in that order. // -// An empty result means the directory contains no devcontainer configuration. Everything under the -// .devcontainer directory is accessed through a root confined to that directory: the future -// Feature/dependsOn resolver receives the same boundary, so local Feature references — including -// Features stored inside the active .devcontainer directory — resolve within it. -func findConfigs(root *os.Root) []ConfigFile { +// fn never being called means the directory contains no devcontainer configuration. A non-nil +// error from fn aborts the visit and is returned as is; a per-file problem that should not stop +// the remaining files from being visited must be handled inside fn. The entry's root is only +// valid during the fn call. Everything under the .devcontainer directory is accessed through a +// root confined to that directory: the future Feature/dependsOn resolver receives the same +// boundary, so local Feature references — including Features stored inside the active +// .devcontainer directory — resolve within it. +func visitConfigs(root *os.Root, fn func(configEntry) error) error { if p := "devcontainer-feature.json"; isFile(root, p) { - return []ConfigFile{{Path: p, Type: Feature}} + return fn(configEntry{root, p, p, Feature}) } if p := "devcontainer-template.json"; isFile(root, p) { - files := []ConfigFile{{Path: p, Type: Template}} - return append(files, devcontainerConfigs(root)...) + if err := fn(configEntry{root, p, p, Template}); err != nil { + return err + } } - return devcontainerConfigs(root) + return visitDevcontainerConfigs(root, fn) } // devcontainerDir is the directory that holds a dev container definition's configuration, and the // boundary that access to that configuration is confined to. const devcontainerDir = ".devcontainer" -// devcontainerConfigs returns the devcontainer.json files under root at the locations defined by -// the devcontainer specification, as root-relative paths. -func devcontainerConfigs(root *os.Root) []ConfigFile { - var paths []string - if isFile(root, ".devcontainer.json") { - paths = append(paths, ".devcontainer.json") - } - if sub, err := root.OpenRoot(devcontainerDir); err == nil { - // The root is only read from, so a close error is inconsequential. - defer func() { _ = sub.Close() }() - if isFile(sub, "devcontainer.json") { - paths = append(paths, filepath.Join(devcontainerDir, "devcontainer.json")) +// visitDevcontainerConfigs calls fn for each devcontainer.json under root at the locations defined +// by the devcontainer specification. Files inside the .devcontainer directory are visited with a +// root confined to that directory, opened once for the whole visit. +func visitDevcontainerConfigs(root *os.Root, fn func(configEntry) error) error { + if p := ".devcontainer.json"; isFile(root, p) { + if err := fn(configEntry{root, p, p, Devcontainer}); err != nil { + return err } - entries, err := fs.ReadDir(sub.FS(), ".") - if err == nil { - for _, e := range entries { - if !e.IsDir() { - continue - } - if isFile(sub, filepath.Join(e.Name(), "devcontainer.json")) { - paths = append(paths, filepath.Join(devcontainerDir, e.Name(), "devcontainer.json")) - } - } - } - } - sort.Strings(paths) - files := make([]ConfigFile, 0, len(paths)) - for _, p := range paths { - files = append(files, ConfigFile{Path: p, Type: Devcontainer}) - } - return files -} - -// readConfig reads the configuration file at name, which is relative to root. A file under the -// .devcontainer directory is accessed through a root confined to that directory, so its resolution -// cannot reach files outside of it. -func readConfig(root *os.Root, name string) ([]byte, error) { - rest, ok := strings.CutPrefix(name, devcontainerDir+string(filepath.Separator)) - if !ok { - return root.ReadFile(name) } sub, err := root.OpenRoot(devcontainerDir) if err != nil { - return nil, err + return nil } // The root is only read from, so a close error is inconsequential. defer func() { _ = sub.Close() }() - return sub.ReadFile(rest) + if p := "devcontainer.json"; isFile(sub, p) { + if err := fn(configEntry{sub, p, filepath.Join(devcontainerDir, p), Devcontainer}); err != nil { + return err + } + } + entries, err := fs.ReadDir(sub.FS(), ".") + if err != nil { + return nil + } + for _, e := range entries { + if !e.IsDir() { + continue + } + p := filepath.Join(e.Name(), "devcontainer.json") + if !isFile(sub, p) { + continue + } + if err := fn(configEntry{sub, p, filepath.Join(devcontainerDir, p), Devcontainer}); err != nil { + return err + } + } + return nil } func isFile(root *os.Root, path string) bool { diff --git a/linter/linter_internal_test.go b/linter/linter_internal_test.go index d432852..d6ef3b0 100644 --- a/linter/linter_internal_test.go +++ b/linter/linter_internal_test.go @@ -1,62 +1,81 @@ package linter import ( + "errors" "os" "path/filepath" "slices" "testing" ) -func TestFindConfigs(t *testing.T) { +// configRef identifies a visited configuration file independently of the root it is read through: +// its path relative to the lint directory, and its type. +type configRef struct { + rel string + typ FileType +} + +// openRoot opens dir as an os.Root, closed when the test ends, to visit configs through. +func openRoot(t *testing.T, dir string) *os.Root { + t.Helper() + root, err := os.OpenRoot(dir) + if err != nil { + t.Fatalf("os.OpenRoot(%q): %v", dir, err) + } + t.Cleanup(func() { _ = root.Close() }) + return root +} + +func TestVisitConfigs(t *testing.T) { t.Parallel() tests := []struct { dir string - want []ConfigFile + want []configRef }{ { "testdata/project", - []ConfigFile{ + []configRef{ {filepath.Join(".devcontainer", "devcontainer.json"), Devcontainer}, {filepath.Join(".devcontainer", "go", "devcontainer.json"), Devcontainer}, }, }, { "testdata/rootfile", - []ConfigFile{ + []configRef{ {".devcontainer.json", Devcontainer}, }, }, { "testdata/feature", - []ConfigFile{ + []configRef{ {"devcontainer-feature.json", Feature}, }, }, { "testdata/template", - []ConfigFile{ + []configRef{ {"devcontainer-template.json", Template}, {filepath.Join(".devcontainer", "devcontainer.json"), Devcontainer}, }, }, { "testdata/template-rootfile", - []ConfigFile{ + []configRef{ {"devcontainer-template.json", Template}, {".devcontainer.json", Devcontainer}, }, }, { "testdata/template-subfolder", - []ConfigFile{ + []configRef{ {"devcontainer-template.json", Template}, {filepath.Join(".devcontainer", "go", "devcontainer.json"), Devcontainer}, }, }, { "testdata/template-no-devcontainer", - []ConfigFile{ + []configRef{ {"devcontainer-template.json", Template}, }, }, @@ -65,15 +84,38 @@ func TestFindConfigs(t *testing.T) { for _, tt := range tests { t.Run(tt.dir, func(t *testing.T) { t.Parallel() - root, err := os.OpenRoot(tt.dir) + var got []configRef + err := visitConfigs(openRoot(t, tt.dir), func(f configEntry) error { + if _, err := f.root.Stat(f.path); err != nil { + t.Errorf("entry %q is not accessible through its root: %v", f.rel, err) + } + got = append(got, configRef{f.rel, f.typ}) + return nil + }) if err != nil { - t.Fatalf("os.OpenRoot(%q): %v", tt.dir, err) + t.Fatalf("visitConfigs(%q): %v", tt.dir, err) } - t.Cleanup(func() { _ = root.Close() }) - got := findConfigs(root) if !slices.Equal(got, tt.want) { - t.Errorf("findConfigs(%q) = %v, want %v", tt.dir, got, tt.want) + t.Errorf("visitConfigs(%q) visited %v, want %v", tt.dir, got, tt.want) } }) } } + +func TestVisitConfigsStopsOnError(t *testing.T) { + t.Parallel() + + wantErr := errors.New("stop") + calls := 0 + // testdata/project contains two configs, so a first-call error must prevent a second call. + err := visitConfigs(openRoot(t, "testdata/project"), func(configEntry) error { + calls++ + return wantErr + }) + if !errors.Is(err, wantErr) { + t.Errorf("visitConfigs returned %v, want %v", err, wantErr) + } + if calls != 1 { + t.Errorf("fn called %d times, want 1", calls) + } +} From b3e33312c1aaf6989e4464ce43857a660d004db2 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 14 Jul 2026 12:13:29 +0000 Subject: [PATCH 5/6] refactor(linter): unify error handling style in visitConfigs The feature branch returned fn's error directly, while every other branch used an explicit if-err check because it has work to do afterward. Match the feature branch to that style for consistency. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_012ujRvJhfjXKaXwNVGwgxv4 --- linter/linter.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/linter/linter.go b/linter/linter.go index 19c28de..6fc66b9 100644 --- a/linter/linter.go +++ b/linter/linter.go @@ -199,7 +199,10 @@ func safeCheck(r *Rule, rctx *Context, node *Node) (findings []Finding) { // .devcontainer directory — resolve within it. func visitConfigs(root *os.Root, fn func(configEntry) error) error { if p := "devcontainer-feature.json"; isFile(root, p) { - return fn(configEntry{root, p, p, Feature}) + if err := fn(configEntry{root, p, p, Feature}); err != nil { + return err + } + return nil } if p := "devcontainer-template.json"; isFile(root, p) { if err := fn(configEntry{root, p, p, Template}); err != nil { From e05fe6ef3fe9cc1600449732f99c2cdc1be76a29 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 14 Jul 2026 22:24:04 +0000 Subject: [PATCH 6/6] test(linter): decouple linter tests from the rules package and merge test files linter_test.go relied on the production rules package's no-image-latest rule just to exercise LintDir's file discovery and error handling, which are independent of which rules are registered. Replace it with a package-local test double (like the existing panicRule), removing linter's only dependency on rules from its test binary. Also fold linter_internal_test.go's white-box visitConfigs tests into this file (both now package linter), so the duplicated openRoot helper collapses to one copy. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_012ujRvJhfjXKaXwNVGwgxv4 --- linter/linter_internal_test.go | 121 -------------------- linter/linter_test.go | 194 ++++++++++++++++++++++++++------- 2 files changed, 156 insertions(+), 159 deletions(-) delete mode 100644 linter/linter_internal_test.go diff --git a/linter/linter_internal_test.go b/linter/linter_internal_test.go deleted file mode 100644 index d6ef3b0..0000000 --- a/linter/linter_internal_test.go +++ /dev/null @@ -1,121 +0,0 @@ -package linter - -import ( - "errors" - "os" - "path/filepath" - "slices" - "testing" -) - -// configRef identifies a visited configuration file independently of the root it is read through: -// its path relative to the lint directory, and its type. -type configRef struct { - rel string - typ FileType -} - -// openRoot opens dir as an os.Root, closed when the test ends, to visit configs through. -func openRoot(t *testing.T, dir string) *os.Root { - t.Helper() - root, err := os.OpenRoot(dir) - if err != nil { - t.Fatalf("os.OpenRoot(%q): %v", dir, err) - } - t.Cleanup(func() { _ = root.Close() }) - return root -} - -func TestVisitConfigs(t *testing.T) { - t.Parallel() - - tests := []struct { - dir string - want []configRef - }{ - { - "testdata/project", - []configRef{ - {filepath.Join(".devcontainer", "devcontainer.json"), Devcontainer}, - {filepath.Join(".devcontainer", "go", "devcontainer.json"), Devcontainer}, - }, - }, - { - "testdata/rootfile", - []configRef{ - {".devcontainer.json", Devcontainer}, - }, - }, - { - "testdata/feature", - []configRef{ - {"devcontainer-feature.json", Feature}, - }, - }, - { - "testdata/template", - []configRef{ - {"devcontainer-template.json", Template}, - {filepath.Join(".devcontainer", "devcontainer.json"), Devcontainer}, - }, - }, - { - "testdata/template-rootfile", - []configRef{ - {"devcontainer-template.json", Template}, - {".devcontainer.json", Devcontainer}, - }, - }, - { - "testdata/template-subfolder", - []configRef{ - {"devcontainer-template.json", Template}, - {filepath.Join(".devcontainer", "go", "devcontainer.json"), Devcontainer}, - }, - }, - { - "testdata/template-no-devcontainer", - []configRef{ - {"devcontainer-template.json", Template}, - }, - }, - {"testdata", nil}, - } - for _, tt := range tests { - t.Run(tt.dir, func(t *testing.T) { - t.Parallel() - var got []configRef - err := visitConfigs(openRoot(t, tt.dir), func(f configEntry) error { - if _, err := f.root.Stat(f.path); err != nil { - t.Errorf("entry %q is not accessible through its root: %v", f.rel, err) - } - got = append(got, configRef{f.rel, f.typ}) - return nil - }) - if err != nil { - t.Fatalf("visitConfigs(%q): %v", tt.dir, err) - } - if !slices.Equal(got, tt.want) { - t.Errorf("visitConfigs(%q) visited %v, want %v", tt.dir, got, tt.want) - } - }) - } -} - -func TestVisitConfigsStopsOnError(t *testing.T) { - t.Parallel() - - wantErr := errors.New("stop") - calls := 0 - // testdata/project contains two configs, so a first-call error must prevent a second call. - err := visitConfigs(openRoot(t, "testdata/project"), func(configEntry) error { - calls++ - return wantErr - }) - if !errors.Is(err, wantErr) { - t.Errorf("visitConfigs returned %v, want %v", err, wantErr) - } - if calls != 1 { - t.Errorf("fn called %d times, want 1", calls) - } -} diff --git a/linter/linter_test.go b/linter/linter_test.go index e667f0f..fed68dc 100644 --- a/linter/linter_test.go +++ b/linter/linter_test.go @@ -1,24 +1,49 @@ -package linter_test +package linter import ( + "errors" + "fmt" "os" "path/filepath" + "slices" + "strings" "testing" - "github.com/bare-devcontainer/decolint/linter" - "github.com/bare-devcontainer/decolint/rules" + "github.com/tailscale/hujson" ) -func lintSrc(t *testing.T, src string) []linter.Issue { +// noImageLatestRule is a test double for the rules package's no-image-latest rule: it flags an +// "image" property with no tag or the "latest" tag. Reusing its ID keeps testdata's +// decolint-ignore-next-line comments meaningful without this package depending on rules. +var noImageLatestRule = &Rule{ + ID: "no-image-latest", + FileTypes: []FileType{Devcontainer}, + Paths: []string{"/image"}, + Check: func(_ *Context, node *Node) []Finding { + lit, ok := node.Value.Value.(hujson.Literal) + if !ok || lit.Kind() != '"' { + return nil + } + image := lit.String() + tag, hasTag := "", false + if i := strings.LastIndex(image, ":"); i >= 0 { + tag, hasTag = image[i+1:], true + } + switch { + case !hasTag: + return []Finding{{Message: fmt.Sprintf("image %q has no explicit tag", image), Offset: node.Value.StartOffset}} + case tag == "latest": + return []Finding{{Message: fmt.Sprintf("image %q uses the \"latest\" tag", image), Offset: node.Value.StartOffset}} + } + return nil + }, +} + +func lintSrc(t *testing.T, src string) []Issue { t.Helper() - l := linter.New() - // Only the correctness category is enabled by default; enable no-image-latest specifically, - // since tests using this helper rely on it firing. - overrides := rules.Overrides{Rules: map[string]linter.Severity{"no-image-latest": linter.SeverityWarn}} - if err := rules.RegisterRules(l, nil, overrides); err != nil { - t.Fatalf("RegisterRules: %v", err) - } - issues, err := l.Lint(t.Context(), "devcontainer.json", []byte(src), linter.Devcontainer) + l := New() + l.RegisterRule(noImageLatestRule, SeverityWarn) + issues, err := l.Lint(t.Context(), "devcontainer.json", []byte(src), Devcontainer) if err != nil { t.Fatalf("Lint: %v", err) } @@ -34,7 +59,7 @@ func symlink(t *testing.T, target, link string) { } } -// openRoot opens dir as an os.Root, closed when the test ends, to lint through. +// openRoot opens dir as an os.Root, closed when the test ends, to lint or visit configs through. func openRoot(t *testing.T, dir string) *os.Root { t.Helper() root, err := os.OpenRoot(dir) @@ -73,7 +98,7 @@ func TestLintDirSymlink(t *testing.T) { symlink(t, filepath.Join("..", "..", "devcontainer.json"), filepath.Join(proj, ".devcontainer", "devcontainer.json")) - l := linter.New() + l := New() if _, err := l.LintDir(t.Context(), openRoot(t, proj)); err == nil { t.Error("LintDir: got nil error, want 'no devcontainer configuration found'") } @@ -87,7 +112,7 @@ func TestLintDirSymlink(t *testing.T) { t.Fatal(err) } - l := linter.New() + l := New() if _, err := l.LintDir(t.Context(), openRoot(t, proj)); err == nil { t.Error("LintDir: got nil error, want 'no devcontainer configuration found'") } @@ -106,7 +131,7 @@ func TestLintDirSymlink(t *testing.T) { symlink(t, filepath.Join(proj, ".devcontainer", "real.json"), filepath.Join(proj, ".devcontainer", "devcontainer.json")) - l := linter.New() + l := New() if _, err := l.LintDir(t.Context(), openRoot(t, proj)); err == nil { t.Error("LintDir: got nil error, want 'no devcontainer configuration found'") } @@ -119,7 +144,7 @@ func TestLintDirSymlink(t *testing.T) { t.Fatal(err) } - l := linter.New() + l := New() if _, err := l.LintDir(t.Context(), openRoot(t, proj)); err != nil { t.Errorf("LintDir: %v", err) } @@ -137,7 +162,7 @@ func TestLintDirSymlink(t *testing.T) { symlink(t, filepath.Join("..", "shared.json"), filepath.Join(proj, ".devcontainer", "go", "devcontainer.json")) - l := linter.New() + l := New() if _, err := l.LintDir(t.Context(), openRoot(t, proj)); err != nil { t.Errorf("LintDir: %v", err) } @@ -147,13 +172,8 @@ func TestLintDirSymlink(t *testing.T) { func TestLintDir(t *testing.T) { t.Parallel() - l := linter.New() - // Only the correctness category is enabled by default; enable no-image-latest specifically, - // since the fixtures below rely on it firing. - overrides := rules.Overrides{Rules: map[string]linter.Severity{"no-image-latest": linter.SeverityWarn}} - if err := rules.RegisterRules(l, nil, overrides); err != nil { - t.Fatalf("RegisterRules: %v", err) - } + l := New() + l.RegisterRule(noImageLatestRule, SeverityWarn) t.Run("definition with multiple configs", func(t *testing.T) { t.Parallel() @@ -254,23 +274,20 @@ func TestIssuePosition(t *testing.T) { func TestLintParseError(t *testing.T) { t.Parallel() - l := linter.New() - if err := rules.RegisterRules(l, nil, rules.Overrides{}); err != nil { - t.Fatalf("RegisterRules: %v", err) - } - if _, err := l.Lint(t.Context(), "bad.json", []byte(`{`), linter.Devcontainer); err == nil { + l := New() + if _, err := l.Lint(t.Context(), "bad.json", []byte(`{`), Devcontainer); err == nil { t.Error("Lint on malformed input: got nil error, want parse error") } } // panicRule is a stub Rule whose Check always panics, used to verify that the engine survives a // defective rule instead of letting it abort the whole run. -var panicRule = &linter.Rule{ +var panicRule = &Rule{ ID: "panic-rule", Description: "always panics", - FileTypes: []linter.FileType{linter.Devcontainer}, + FileTypes: []FileType{Devcontainer}, Paths: []string{""}, - Check: func(*linter.Context, *linter.Node) []linter.Finding { + Check: func(*Context, *Node) []Finding { panic("boom") }, } @@ -278,9 +295,9 @@ var panicRule = &linter.Rule{ func TestLintRulePanicIsRecovered(t *testing.T) { t.Parallel() - l := linter.New() - l.RegisterRule(panicRule, linter.SeverityError) - issues, err := l.Lint(t.Context(), "devcontainer.json", []byte(`{}`), linter.Devcontainer) + l := New() + l.RegisterRule(panicRule, SeverityError) + issues, err := l.Lint(t.Context(), "devcontainer.json", []byte(`{}`), Devcontainer) if err != nil { t.Fatalf("Lint: %v", err) } @@ -290,7 +307,108 @@ func TestLintRulePanicIsRecovered(t *testing.T) { if issues[0].RuleID != "panic-rule" { t.Errorf("RuleID = %q, want %q", issues[0].RuleID, "panic-rule") } - if issues[0].Severity != linter.SeverityError { - t.Errorf("Severity = %v, want %v", issues[0].Severity, linter.SeverityError) + if issues[0].Severity != SeverityError { + t.Errorf("Severity = %v, want %v", issues[0].Severity, SeverityError) + } +} + +// configRef identifies a visited configuration file independently of the root it is read through: +// its path relative to the lint directory, and its type. +type configRef struct { + rel string + typ FileType +} + +func TestVisitConfigs(t *testing.T) { + t.Parallel() + + tests := []struct { + dir string + want []configRef + }{ + { + "testdata/project", + []configRef{ + {filepath.Join(".devcontainer", "devcontainer.json"), Devcontainer}, + {filepath.Join(".devcontainer", "go", "devcontainer.json"), Devcontainer}, + }, + }, + { + "testdata/rootfile", + []configRef{ + {".devcontainer.json", Devcontainer}, + }, + }, + { + "testdata/feature", + []configRef{ + {"devcontainer-feature.json", Feature}, + }, + }, + { + "testdata/template", + []configRef{ + {"devcontainer-template.json", Template}, + {filepath.Join(".devcontainer", "devcontainer.json"), Devcontainer}, + }, + }, + { + "testdata/template-rootfile", + []configRef{ + {"devcontainer-template.json", Template}, + {".devcontainer.json", Devcontainer}, + }, + }, + { + "testdata/template-subfolder", + []configRef{ + {"devcontainer-template.json", Template}, + {filepath.Join(".devcontainer", "go", "devcontainer.json"), Devcontainer}, + }, + }, + { + "testdata/template-no-devcontainer", + []configRef{ + {"devcontainer-template.json", Template}, + }, + }, + {"testdata", nil}, + } + for _, tt := range tests { + t.Run(tt.dir, func(t *testing.T) { + t.Parallel() + var got []configRef + err := visitConfigs(openRoot(t, tt.dir), func(f configEntry) error { + if _, err := f.root.Stat(f.path); err != nil { + t.Errorf("entry %q is not accessible through its root: %v", f.rel, err) + } + got = append(got, configRef{f.rel, f.typ}) + return nil + }) + if err != nil { + t.Fatalf("visitConfigs(%q): %v", tt.dir, err) + } + if !slices.Equal(got, tt.want) { + t.Errorf("visitConfigs(%q) visited %v, want %v", tt.dir, got, tt.want) + } + }) + } +} + +func TestVisitConfigsStopsOnError(t *testing.T) { + t.Parallel() + + wantErr := errors.New("stop") + calls := 0 + // testdata/project contains two configs, so a first-call error must prevent a second call. + err := visitConfigs(openRoot(t, "testdata/project"), func(configEntry) error { + calls++ + return wantErr + }) + if !errors.Is(err, wantErr) { + t.Errorf("visitConfigs returned %v, want %v", err, wantErr) + } + if calls != 1 { + t.Errorf("fn called %d times, want 1", calls) } }