From fb8ea906d430e821e5cc5bc1fd51deb29e18e570 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 12 Jul 2026 08:02:48 +0000 Subject: [PATCH 1/4] feat(cli): add -merge-features to lint the merged configuration Dev Container Features contribute properties (containerEnv, mounts, capAdd, securityOpt, privileged, init, customizations, and lifecycle hooks) that the tooling merges into the effective configuration per the specification's merge logic, so issues a Feature introduces are invisible when only the raw devcontainer.json is linted. Add an opt-in -merge-features flag (and a "mergeFeatures" config member) that fetches every referenced Feature - OCI artifacts with anonymous pull, HTTP(S) tarballs, and local paths - resolves dependsOn recursively, merges the contributed properties into the parsed syntax tree, and lints the result. Merged-in nodes carry the byte offset of the referencing "features" key, so findings on them point at the Feature reference and the usual ignore comments apply. A Feature that cannot be fetched is a runtime error (exit code 2). Fetched metadata is cached in memory for the duration of the run. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01UrZdqcow5W3yq4uGMMPS8D --- README.md | 45 +- cmd/decolint/config.go | 16 +- cmd/decolint/config_test.go | 25 + cmd/decolint/init.go | 3 + cmd/decolint/main.go | 15 + cmd/decolint/main_test.go | 72 +++ cmd/decolint/opts.go | 5 + cmd/decolint/opts_test.go | 26 + cmd/decolint/testdata/e2e/merge-on.jsonc | 9 + cmd/decolint/testdata/e2e/merge.jsonc | 8 + .../e2e/merge/.devcontainer/devcontainer.json | 7 + .../devcontainer-feature.json | 9 + feature/apply.go | 444 ++++++++++++++++++ feature/fetch.go | 105 +++++ feature/fetch_test.go | 293 ++++++++++++ feature/merge.go | 219 +++++++++ feature/merge_test.go | 313 ++++++++++++ feature/metadata.go | 68 +++ feature/oci.go | 240 ++++++++++ feature/ref.go | 77 +++ feature/ref_test.go | 82 ++++ feature/tarball.go | 76 +++ linter/linter.go | 19 + linter/linter_test.go | 78 +++ 24 files changed, 2251 insertions(+), 3 deletions(-) create mode 100644 cmd/decolint/testdata/e2e/merge-on.jsonc create mode 100644 cmd/decolint/testdata/e2e/merge.jsonc create mode 100644 cmd/decolint/testdata/e2e/merge/.devcontainer/devcontainer.json create mode 100644 cmd/decolint/testdata/e2e/merge/.devcontainer/privileged-feature/devcontainer-feature.json create mode 100644 feature/apply.go create mode 100644 feature/fetch.go create mode 100644 feature/fetch_test.go create mode 100644 feature/merge.go create mode 100644 feature/merge_test.go create mode 100644 feature/metadata.go create mode 100644 feature/oci.go create mode 100644 feature/ref.go create mode 100644 feature/ref_test.go create mode 100644 feature/tarball.go diff --git a/README.md b/README.md index 3ef0697..f1a59fa 100644 --- a/README.md +++ b/README.md @@ -72,6 +72,7 @@ list. | `-format` | output format: `text` (default), `json`, or `github` (see [Output formats](#output-formats)) | | `-deny-warnings` | also exit non-zero on `warn`-severity findings (see [Exit codes](#exit-codes)) | | `-config` | path to a config file overriding rule and category severities (see [Config file](#config-file)) | +| `-merge-features` | fetch the Features referenced in `features` and lint the merged configuration (see [Merging Features](#merging-features)) | | `-rules` | print the available rules | | `-init` | write a new `.decolint.jsonc` listing every rule at its default severity (see [Config file](#config-file)) | @@ -91,6 +92,46 @@ Target platforms can also be declared in the [config file](#config-file) with the `platforms` member; the `-platform` flag, when given, takes precedence. +### Merging Features + +The [Features](https://containers.dev/implementors/features/) a +`devcontainer.json` references contribute configuration of their own — +`containerEnv`, `mounts`, `capAdd`, `securityOpt`, `privileged`, +`init`, `customizations`, and lifecycle hooks — which the Dev +Container tooling merges into the effective configuration following +the specification's [merge +logic](https://containers.dev/implementors/spec/#merge-logic). By +default decolint lints only the raw file, so an issue introduced by a +Feature (say, one that sets `privileged: true` or bind-mounts the +Docker socket) goes unnoticed. + +Pass `-merge-features` (or set `"mergeFeatures": true` in the [config +file](#config-file)) to fetch every referenced Feature, merge the +properties it contributes, and lint the merged configuration instead: + +```console +decolint -merge-features -config .decolint.jsonc +``` + +- OCI references (e.g. `ghcr.io/devcontainers/features/node:1`) are + pulled from the registry with anonymous access, direct HTTP(S) + tarball URIs are downloaded, and relative paths (e.g. + `./my-feature`) are read from disk. +- Features referenced by a Feature's `dependsOn` are resolved + recursively and contribute their properties as well; installation + order follows `dependsOn`, `installsAfter`, and + `overrideFeatureInstallOrder`. +- Findings on merged-in properties are reported at the referencing + entry in `features`, so the usual [suppression + comments](#suppressing-findings) on that line apply to them. +- A Feature that cannot be fetched (network failure, unknown + reference, private registry) is an error (exit code 2). + +Fetched metadata is cached in memory for the duration of the run, but +not on disk. Note that Feature `options` never affect the merged +configuration, and variable substitutions (e.g. `${devcontainerId}`) +in contributed values are merged literally. + ### Output formats By default, findings are printed one per line, with the rule's @@ -152,7 +193,9 @@ rule in a [category](#rule-categories) at once; `rules` sets an individual rule's severity and takes precedence over its category. `platforms` lists the [target platforms](#target-platforms) whose rules run in addition to platform-agnostic ones; the `-platform` flag, -when given, takes precedence over it. +when given, takes precedence over it. `mergeFeatures` set to `true` +enables [merging Features](#merging-features), same as the +`-merge-features` flag. For the strictest configuration, enable every category: diff --git a/cmd/decolint/config.go b/cmd/decolint/config.go index 0c27020..1b2f2e9 100644 --- a/cmd/decolint/config.go +++ b/cmd/decolint/config.go @@ -17,6 +17,9 @@ type Config struct { // Platforms lists target platforms whose rules are linted in addition to platform-agnostic // ones. The -platform flag, when given, takes precedence. Platforms []linter.Platform `json:"platforms"` + // MergeFeatures, when true, fetches the Features referenced in each devcontainer.json and + // lints the merged (effective) configuration. The -merge-features flag can enable it as well. + MergeFeatures bool `json:"mergeFeatures"` // Categories maps a category name to the severity every rule in that category should be // overridden to. Per-rule entries in Rules take precedence. Categories map[string]linter.Severity `json:"categories"` @@ -26,8 +29,9 @@ type Config struct { // MarshalJSONTo encodes cfg with its categories and rules written in sorted key order, for use // with encoding/json/v2. Map iteration order is otherwise unspecified, so without this, marshaling -// the same Config twice could produce differently ordered output. The "platforms" and "categories" -// members are omitted when empty, so generated configs (see initConfigFile) stay minimal. +// the same Config twice could produce differently ordered output. The "platforms", +// "mergeFeatures", and "categories" members are omitted when empty or false, so generated configs +// (see initConfigFile) stay minimal. func (cfg Config) MarshalJSONTo(enc *jsontext.Encoder) error { if err := enc.WriteToken(jsontext.BeginObject); err != nil { return err @@ -40,6 +44,14 @@ func (cfg Config) MarshalJSONTo(enc *jsontext.Encoder) error { return err } } + if cfg.MergeFeatures { + if err := enc.WriteToken(jsontext.String("mergeFeatures")); err != nil { + return err + } + if err := enc.WriteToken(jsontext.Bool(true)); err != nil { + return err + } + } if len(cfg.Categories) > 0 { if err := enc.WriteToken(jsontext.String("categories")); err != nil { return err diff --git a/cmd/decolint/config_test.go b/cmd/decolint/config_test.go index 1f5ad27..d7c9daa 100644 --- a/cmd/decolint/config_test.go +++ b/cmd/decolint/config_test.go @@ -79,6 +79,25 @@ func TestConfigMarshalJSONToWithPlatforms(t *testing.T) { } } +func TestConfigMarshalJSONToWithMergeFeatures(t *testing.T) { + t.Parallel() + + cfg := Config{ + MergeFeatures: true, + Rules: map[string]linter.Severity{"no-image-latest": linter.SeverityError}, + } + + want := `{"mergeFeatures":true,"rules":{"no-image-latest":"error"}}` + + got, err := json.Marshal(cfg) + if err != nil { + t.Fatalf("json.Marshal: %v", err) + } + if string(got) != want { + t.Errorf("json.Marshal(cfg) = %s, want %s", got, want) + } +} + func TestParseConfig(t *testing.T) { t.Parallel() @@ -136,6 +155,12 @@ func TestParseConfig(t *testing.T) { }, false, }, + { + "mergeFeatures", + `{"mergeFeatures": true}`, + Config{MergeFeatures: true}, + false, + }, {"invalid severity", `{"rules": {"no-image-latest": "critical"}}`, Config{}, true}, {"invalid category severity", `{"categories": {"security": "critical"}}`, Config{}, true}, {"unknown platform", `{"platforms": ["intellij"]}`, Config{}, true}, diff --git a/cmd/decolint/init.go b/cmd/decolint/init.go index fe688d9..3f462c7 100644 --- a/cmd/decolint/init.go +++ b/cmd/decolint/init.go @@ -37,6 +37,9 @@ func initConfigFile(output io.Writer) error { // "platforms" lists target platforms whose rules run in addition to // platform-agnostic ones (the -platform flag takes precedence), e.g.: // "platforms": ["vscode", "codespaces"] +// "mergeFeatures", when true, fetches the Features referenced in each +// devcontainer.json and lints the merged (effective) configuration, e.g.: +// "mergeFeatures": true `) if err := json.MarshalWrite(&buf, cfg, jsontext.Multiline(true)); err != nil { return fmt.Errorf("marshal %s: %w", name, err) diff --git a/cmd/decolint/main.go b/cmd/decolint/main.go index 81149fc..9654194 100644 --- a/cmd/decolint/main.go +++ b/cmd/decolint/main.go @@ -9,9 +9,11 @@ import ( "io" "os" "os/signal" + "path/filepath" "strings" "syscall" + "github.com/bare-devcontainer/decolint/feature" "github.com/bare-devcontainer/decolint/linter" "github.com/bare-devcontainer/decolint/rules" ) @@ -83,6 +85,9 @@ func run(ctx context.Context, args []string, stdout, stderr io.Writer) int { if len(opts.Platforms) == 0 { opts.Platforms = cfg.Platforms } + // The -merge-features flag and the config file's "mergeFeatures" member can each enable + // merging; the flag cannot turn it back off. + opts.MergeFeatures = opts.MergeFeatures || cfg.MergeFeatures if opts.ListRules { if err := listRules(stdout, opts.Config); err != nil { @@ -236,6 +241,16 @@ func runLint(ctx context.Context, stdout io.Writer, opts Options) (bool, error) if err := rules.RegisterRules(l, opts.Platforms, overrides); err != nil { return false, fmt.Errorf("register rules: %w", err) } + if opts.MergeFeatures { + // One Fetcher per run, so a Feature shared by several files is fetched at most once. + fetcher := feature.NewFetcher() + l.SetTransform(func(ctx context.Context, fctx *linter.Context) error { + if fctx.Type != linter.Devcontainer { + return nil + } + return feature.Merge(ctx, fetcher, filepath.Dir(fctx.Path), fctx.Root) + }) + } var allIssues []linter.Issue var worstSeverity linter.Severity diff --git a/cmd/decolint/main_test.go b/cmd/decolint/main_test.go index ca3e489..8bdc733 100644 --- a/cmd/decolint/main_test.go +++ b/cmd/decolint/main_test.go @@ -27,6 +27,8 @@ func TestRun(t *testing.T) { // violationsFile is where every firing in the violations fixture is reported. const violationsFile = "testdata/e2e/violations/.devcontainer/devcontainer.json" + // mergeFile is where every firing in the merge fixture is reported. + const mergeFile = "testdata/e2e/merge/.devcontainer/devcontainer.json" tests := []struct { name string @@ -146,6 +148,35 @@ func TestRun(t *testing.T) { want: nil, wantExitCode: 0, }, + { + // With -merge-features, the local Feature's contributions (privileged mode and a Docker + // socket mount) become part of the effective configuration and trip the security rules + // merge.jsonc enables. + name: "merge features", + args: []string{"-merge-features", "-config=testdata/e2e/merge.jsonc", "testdata/e2e/merge"}, + want: []firing{ + {mergeFile, "no-docker-socket-mount", linter.SeverityError}, + {mergeFile, "no-privileged-container", linter.SeverityError}, + }, + wantExitCode: 1, + }, + { + // Without the flag only the raw file is linted, and the raw file is clean. + name: "merge features disabled", + args: []string{"-config=testdata/e2e/merge.jsonc", "testdata/e2e/merge"}, + want: nil, + wantExitCode: 0, + }, + { + // merge-on.jsonc enables merging via the config file's "mergeFeatures" member. + name: "merge features enabled by config", + args: []string{"-config=testdata/e2e/merge-on.jsonc", "testdata/e2e/merge"}, + want: []firing{ + {mergeFile, "no-docker-socket-mount", linter.SeverityError}, + {mergeFile, "no-privileged-container", linter.SeverityError}, + }, + wantExitCode: 1, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -515,6 +546,47 @@ func TestRunLint(t *testing.T) { }) } +func TestRunMergeFeatures(t *testing.T) { + t.Parallel() + + t.Run("findings point at the feature reference", func(t *testing.T) { + t.Parallel() + + var stdout, stderr bytes.Buffer + args := []string{"-format=json", "-merge-features", "-config=testdata/e2e/merge.jsonc", "testdata/e2e/merge"} + exitCode := run(t.Context(), args, &stdout, &stderr) + if exitCode != 1 { + t.Fatalf("exit code = %d, want 1; stderr: %s", exitCode, stderr.String()) + } + + var issues []linter.Issue + if err := json.Unmarshal(stdout.Bytes(), &issues); err != nil { + t.Fatalf("output is not a JSON issue array: %v\noutput: %s", err, stdout.String()) + } + // The fixture references "./privileged-feature" on line 5; every merged-in property must be + // reported there. + for _, issue := range issues { + if issue.Line != 5 { + t.Errorf("issue %s reported at line %d, want 5 (the feature reference)", issue.RuleID, issue.Line) + } + } + }) + + t.Run("unresolvable feature is a runtime error", func(t *testing.T) { + t.Parallel() + dir := writeDevcontainer(t, `{"image": "ubuntu:24.04", "features": {"./missing": {}}}`) + + var stdout, stderr bytes.Buffer + exitCode := run(t.Context(), []string{"-merge-features", dir}, &stdout, &stderr) + if exitCode != 2 { + t.Errorf("exit code = %d, want 2", exitCode) + } + if !strings.Contains(stderr.String(), "./missing") { + t.Errorf("stderr = %q, want it to mention the unresolvable feature", stderr.String()) + } + }) +} + // mdTableRow finds the row of a Markdown table in out whose first cell, after trimming the padding // listRules adds for alignment, equals key, and returns its cells trimmed of that padding. It fails // the test if no such row exists. diff --git a/cmd/decolint/opts.go b/cmd/decolint/opts.go index 4d63ec4..fac3980 100644 --- a/cmd/decolint/opts.go +++ b/cmd/decolint/opts.go @@ -21,6 +21,10 @@ type Options struct { // Platforms restricts registered rules to those targeting one of these platforms, plus any rule // with no target platform. If empty, only rules with no target platform are registered. Platforms []linter.Platform + // MergeFeatures, when set, fetches the Features referenced in each devcontainer.json and lints + // the merged (effective) configuration instead of the raw file. The config file's + // "mergeFeatures" member can enable it as well (see run). + MergeFeatures bool // Format selects how lint issues are written to stdout. Format Format // Version, when set, causes the program to print its version and exit. @@ -47,6 +51,7 @@ func parseOptions(args []string, output io.Writer) (Options, string, error) { fs.StringVar(&configPathFlag, "config", "", "path to a config file (default: auto-discover .decolint.jsonc or .decolint.json in the current directory)") fs.StringVar(&platformFlag, "platform", "", "comma-separated target platforms to include in addition to \"all\" (vscode, codespaces); overrides the config file's \"platforms\" member") fs.StringVar(&formatFlag, "format", "text", "output format: text, json, or github") + fs.BoolVar(&opts.MergeFeatures, "merge-features", false, "fetch the Features referenced in \"features\" and lint the merged (effective) configuration") fs.BoolVar(&opts.Version, "version", false, "print version information and exit") fs.BoolVar(&opts.ListRules, "rules", false, "print the built-in rules as a Markdown table (category, target platforms, current severity), then exit") fs.BoolVar(&opts.Init, "init", false, "write a new .decolint.jsonc config file listing every rule at its default severity, then exit") diff --git a/cmd/decolint/opts_test.go b/cmd/decolint/opts_test.go index 4ca8f78..452a963 100644 --- a/cmd/decolint/opts_test.go +++ b/cmd/decolint/opts_test.go @@ -157,6 +157,32 @@ func TestParseOptionsInit(t *testing.T) { } } +func TestParseOptionsMergeFeatures(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + args []string + want bool + }{ + {"no flag", nil, false}, + {"single dash", []string{"-merge-features"}, true}, + {"double dash", []string{"--merge-features"}, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + opts, _, err := parseOptions(tt.args, io.Discard) + if err != nil { + t.Fatalf("parseOptions(%v): %v", tt.args, err) + } + if opts.MergeFeatures != tt.want { + t.Errorf("MergeFeatures = %v, want %v", opts.MergeFeatures, tt.want) + } + }) + } +} + func TestParseOptionsConfig(t *testing.T) { t.Parallel() diff --git a/cmd/decolint/testdata/e2e/merge-on.jsonc b/cmd/decolint/testdata/e2e/merge-on.jsonc new file mode 100644 index 0000000..19afdf0 --- /dev/null +++ b/cmd/decolint/testdata/e2e/merge-on.jsonc @@ -0,0 +1,9 @@ +// Same as merge.jsonc, but enables Feature merging via the config file instead of the +// -merge-features flag. +{ + "mergeFeatures": true, + "rules": { + "no-privileged-container": "error", + "no-docker-socket-mount": "error" + } +} diff --git a/cmd/decolint/testdata/e2e/merge.jsonc b/cmd/decolint/testdata/e2e/merge.jsonc new file mode 100644 index 0000000..d9ebd43 --- /dev/null +++ b/cmd/decolint/testdata/e2e/merge.jsonc @@ -0,0 +1,8 @@ +// Enables the two security rules the merge fixture's Feature trips, so the e2e tests can assert +// they fire only when -merge-features is given. +{ + "rules": { + "no-privileged-container": "error", + "no-docker-socket-mount": "error" + } +} diff --git a/cmd/decolint/testdata/e2e/merge/.devcontainer/devcontainer.json b/cmd/decolint/testdata/e2e/merge/.devcontainer/devcontainer.json new file mode 100644 index 0000000..8770253 --- /dev/null +++ b/cmd/decolint/testdata/e2e/merge/.devcontainer/devcontainer.json @@ -0,0 +1,7 @@ +{ + "name": "e2e-merge", + "image": "ubuntu:24.04", + "features": { + "./privileged-feature": {} + } +} diff --git a/cmd/decolint/testdata/e2e/merge/.devcontainer/privileged-feature/devcontainer-feature.json b/cmd/decolint/testdata/e2e/merge/.devcontainer/privileged-feature/devcontainer-feature.json new file mode 100644 index 0000000..28abfaa --- /dev/null +++ b/cmd/decolint/testdata/e2e/merge/.devcontainer/privileged-feature/devcontainer-feature.json @@ -0,0 +1,9 @@ +{ + "id": "privileged-feature", + "version": "1.0.0", + "name": "Privileged Feature", + "privileged": true, + "mounts": [ + "source=/var/run/docker.sock,target=/var/run/docker.sock,type=bind" + ] +} diff --git a/feature/apply.go b/feature/apply.go new file mode 100644 index 0000000..3ae6416 --- /dev/null +++ b/feature/apply.go @@ -0,0 +1,444 @@ +package feature + +import ( + "strings" + + "github.com/tailscale/hujson" +) + +// lifecycleHooks are the lifecycle script properties a Feature can contribute. +var lifecycleHooks = []string{ + "onCreateCommand", + "updateContentCommand", + "postCreateCommand", + "postStartCommand", + "postAttachCommand", +} + +// mergeState applies contributors to the root object of a devcontainer.json one by one, in +// installation order, and tracks which values originate from the user's file so that those always +// win, as the user's configuration is applied last per the specification's merge logic. +type mergeState struct { + root *hujson.Object + // userEnvKeys are the containerEnv keys the user's file defines. + userEnvKeys map[string]bool + // userMountTargets are the mount targets the user's file defines. + userMountTargets map[string]bool + // customizations accumulates the contributors' customizations, detached from the tree; finish + // merges the user's own customizations on top and grafts the result. + customizations *hujson.Value + customizationsAnchor int + // lifecycle accumulates contributed lifecycle hook commands, keyed by hook name. + lifecycle map[string][]lifecycleEntry +} + +type lifecycleEntry struct { + id string + anchor int + value hujson.Value +} + +func newMergeState(root *hujson.Object) *mergeState { + s := &mergeState{ + root: root, + userEnvKeys: map[string]bool{}, + userMountTargets: map[string]bool{}, + lifecycle: map[string][]lifecycleEntry{}, + } + if i := findMember(root, "containerEnv"); i >= 0 { + if obj, ok := root.Members[i].Value.Value.(*hujson.Object); ok { + for _, m := range obj.Members { + if lit, ok := m.Name.Value.(hujson.Literal); ok && lit.Kind() == '"' { + s.userEnvKeys[lit.String()] = true + } + } + } + } + if i := findMember(root, "mounts"); i >= 0 { + if arr, ok := root.Members[i].Value.Value.(*hujson.Array); ok { + for j := range arr.Elements { + if target := mountTarget(&arr.Elements[j]); target != "" { + s.userMountTargets[target] = true + } + } + } + } + return s +} + +// apply merges the properties contributed by c into the tree. +func (s *mergeState) apply(c *contributor) { + s.mergeBool(c, "init") + s.mergeBool(c, "privileged") + s.mergeUnion(c, "capAdd") + s.mergeUnion(c, "securityOpt") + s.mergeEnv(c) + s.mergeMounts(c) + s.collectCustomizations(c) + s.collectLifecycle(c) +} + +// finish grafts the accumulated customizations and lifecycle hooks into the tree. +func (s *mergeState) finish() { + s.finishCustomizations() + s.finishLifecycle() +} + +// mergeBool applies a boolean-OR property (init, privileged): the effective value is true if any +// contributor or the user sets it to true. +func (s *mergeState) mergeBool(c *contributor, name string) { + v := c.md.Root.Find("/" + name) + if v == nil { + return + } + lit, ok := v.Value.(hujson.Literal) + if !ok || !lit.Bool() { + return + } + i := findMember(s.root, name) + if i < 0 { + appendMember(s.root, name, anchoredValue(hujson.Bool(true), c.anchor), c.anchor) + return + } + // Only overwrite an explicit false or null; leave a malformed value for correctness rules to + // report. + if existing, ok := s.root.Members[i].Value.Value.(hujson.Literal); ok && (existing.Kind() == 'f' || existing.Kind() == 'n') { + s.root.Members[i].Value = anchoredValue(hujson.Bool(true), c.anchor) + } +} + +// mergeUnion applies a union-of-arrays property (capAdd, securityOpt): the effective array is the +// deduplicated union of every contributor's and the user's entries. +func (s *mergeState) mergeUnion(c *contributor, name string) { + v := c.md.Root.Find("/" + name) + if v == nil { + return + } + src, ok := v.Value.(*hujson.Array) + if !ok { + return + } + i := findMember(s.root, name) + if i < 0 { + i = appendMember(s.root, name, anchoredValue(&hujson.Array{}, c.anchor), c.anchor) + } + dst, ok := s.root.Members[i].Value.Value.(*hujson.Array) + if !ok { + return + } + for j := range src.Elements { + lit, ok := src.Elements[j].Value.(hujson.Literal) + if !ok || lit.Kind() != '"' { + continue + } + if arrayContainsString(dst, lit.String()) { + continue + } + dst.Elements = append(dst.Elements, anchored(src.Elements[j], c.anchor)) + } +} + +// mergeEnv applies containerEnv: an object merge where a later contributor overrides an earlier +// one and the user's own entries always win. +func (s *mergeState) mergeEnv(c *contributor) { + v := c.md.Root.Find("/containerEnv") + if v == nil { + return + } + src, ok := v.Value.(*hujson.Object) + if !ok { + return + } + i := findMember(s.root, "containerEnv") + if i < 0 { + i = appendMember(s.root, "containerEnv", anchoredValue(&hujson.Object{}, c.anchor), c.anchor) + } + dst, ok := s.root.Members[i].Value.Value.(*hujson.Object) + if !ok { + return + } + for _, m := range src.Members { + lit, ok := m.Name.Value.(hujson.Literal) + if !ok || lit.Kind() != '"' { + continue + } + key := lit.String() + if s.userEnvKeys[key] { + continue + } + if j := findMember(dst, key); j >= 0 { + dst.Members[j].Value = anchored(m.Value, c.anchor) + } else { + appendMember(dst, key, anchored(m.Value, c.anchor), c.anchor) + } + } +} + +// mergeMounts applies mounts: entries are collected across contributors, deduplicated by mount +// target with the last contributor winning; a target the user's file mounts always wins. +func (s *mergeState) mergeMounts(c *contributor) { + v := c.md.Root.Find("/mounts") + if v == nil { + return + } + src, ok := v.Value.(*hujson.Array) + if !ok { + return + } + i := findMember(s.root, "mounts") + if i < 0 { + i = appendMember(s.root, "mounts", anchoredValue(&hujson.Array{}, c.anchor), c.anchor) + } + dst, ok := s.root.Members[i].Value.Value.(*hujson.Array) + if !ok { + return + } + for j := range src.Elements { + target := mountTarget(&src.Elements[j]) + if target != "" && s.userMountTargets[target] { + continue + } + merged := anchored(src.Elements[j], c.anchor) + replaced := false + if target != "" { + for k := range dst.Elements { + if mountTarget(&dst.Elements[k]) == target { + dst.Elements[k] = merged + replaced = true + break + } + } + } + if !replaced { + dst.Elements = append(dst.Elements, merged) + } + } +} + +// collectCustomizations deep-merges c's customizations into the detached accumulator: objects +// merge member-wise, arrays concatenate, and a later contributor wins scalar conflicts. +func (s *mergeState) collectCustomizations(c *contributor) { + v := c.md.Root.Find("/customizations") + if v == nil { + return + } + if _, ok := v.Value.(*hujson.Object); !ok { + return + } + av := anchored(*v, c.anchor) + if s.customizations == nil { + s.customizations = &av + s.customizationsAnchor = c.anchor + return + } + deepMerge(s.customizations, av) +} + +// finishCustomizations merges the user's own customizations on top of the accumulated ones, +// reusing the user's original nodes so their positions survive, and grafts the result. +func (s *mergeState) finishCustomizations() { + if s.customizations == nil { + return + } + if i := findMember(s.root, "customizations"); i >= 0 { + merged := *s.customizations + deepMerge(&merged, s.root.Members[i].Value) + s.root.Members[i].Value = merged + } else { + appendMember(s.root, "customizations", *s.customizations, s.customizationsAnchor) + } +} + +// deepMerge merges src into dst: objects merge member-wise recursively, arrays append the src +// elements missing from dst, and src wins any other conflict. src subtrees are attached as-is, so +// they must already carry the offsets they should report. +func deepMerge(dst *hujson.Value, src hujson.Value) { + if dstObj, ok := dst.Value.(*hujson.Object); ok { + if srcObj, ok := src.Value.(*hujson.Object); ok { + for _, m := range srcObj.Members { + lit, ok := m.Name.Value.(hujson.Literal) + if !ok || lit.Kind() != '"' { + continue + } + if i := findMember(dstObj, lit.String()); i >= 0 { + deepMerge(&dstObj.Members[i].Value, m.Value) + } else { + dstObj.Members = append(dstObj.Members, m) + } + } + return + } + } + if dstArr, ok := dst.Value.(*hujson.Array); ok { + if srcArr, ok := src.Value.(*hujson.Array); ok { + for _, e := range srcArr.Elements { + if lit, ok := e.Value.(hujson.Literal); ok && lit.Kind() == '"' && arrayContainsString(dstArr, lit.String()) { + continue + } + dstArr.Elements = append(dstArr.Elements, e) + } + return + } + } + *dst = src +} + +// collectLifecycle records the lifecycle hook commands c contributes. +func (s *mergeState) collectLifecycle(c *contributor) { + for _, hook := range lifecycleHooks { + v := c.md.Root.Find("/" + hook) + if v == nil { + continue + } + s.lifecycle[hook] = append(s.lifecycle[hook], lifecycleEntry{ + id: c.displayID(), + anchor: c.anchor, + value: anchored(*v, c.anchor), + }) + } +} + +// finishLifecycle rewrites each lifecycle hook at least one Feature contributes to into the +// specification's object form, with one member per contributed command keyed by the Feature's ID. +// The user's own command is preserved: an object-form value contributes its members verbatim (they +// win name conflicts), any other form is kept under the "devcontainer.json" key. +func (s *mergeState) finishLifecycle() { + for _, hook := range lifecycleHooks { + entries := s.lifecycle[hook] + if len(entries) == 0 { + continue + } + merged := &hujson.Object{} + for _, e := range entries { + if i := findMember(merged, e.id); i >= 0 { + merged.Members[i].Value = e.value + } else { + appendMember(merged, e.id, e.value, e.anchor) + } + } + + i := findMember(s.root, hook) + if i < 0 { + appendMember(s.root, hook, hujson.Value{ + Value: merged, + StartOffset: entries[0].anchor, + EndOffset: entries[0].anchor, + }, entries[0].anchor) + continue + } + user := s.root.Members[i].Value + if userObj, ok := user.Value.(*hujson.Object); ok { + for _, m := range userObj.Members { + lit, ok := m.Name.Value.(hujson.Literal) + if !ok || lit.Kind() != '"' { + continue + } + if j := findMember(merged, lit.String()); j >= 0 { + merged.Members[j] = m + } else { + merged.Members = append(merged.Members, m) + } + } + } else { + merged.Members = append(merged.Members, hujson.ObjectMember{ + Name: anchoredValue(hujson.String("devcontainer.json"), user.StartOffset), + Value: user, + }) + } + s.root.Members[i].Value = hujson.Value{ + Value: merged, + StartOffset: user.StartOffset, + EndOffset: user.EndOffset, + } + } +} + +// findMember returns the index of obj's member named name, or -1. +func findMember(obj *hujson.Object, name string) int { + for i := range obj.Members { + if lit, ok := obj.Members[i].Name.Value.(hujson.Literal); ok && lit.Kind() == '"' && lit.String() == name { + return i + } + } + return -1 +} + +// appendMember appends a member to obj with a synthesized name carrying the anchor offset, and +// returns its index. +func appendMember(obj *hujson.Object, name string, value hujson.Value, anchor int) int { + obj.Members = append(obj.Members, hujson.ObjectMember{ + Name: anchoredValue(hujson.String(name), anchor), + Value: value, + }) + return len(obj.Members) - 1 +} + +// anchoredValue wraps a constructed value with offsets pointing at anchor. +func anchoredValue(v hujson.ValueTrimmed, anchor int) hujson.Value { + return hujson.Value{Value: v, StartOffset: anchor, EndOffset: anchor} +} + +// anchored returns a deep copy of v with every offset in the subtree set to anchor and all +// comments stripped, so findings on the copy resolve to the anchor position in the original file. +func anchored(v hujson.Value, anchor int) hujson.Value { + c := v.Clone() + for n := range c.All() { + n.StartOffset, n.EndOffset = anchor, anchor + n.BeforeExtra, n.AfterExtra = nil, nil + switch t := n.Value.(type) { + case *hujson.Object: + t.AfterExtra = nil + case *hujson.Array: + t.AfterExtra = nil + } + } + return c +} + +// arrayContainsString reports whether arr has a string element equal to s. +func arrayContainsString(arr *hujson.Array, s string) bool { + for i := range arr.Elements { + if lit, ok := arr.Elements[i].Value.(hujson.Literal); ok && lit.Kind() == '"' && lit.String() == s { + return true + } + } + return false +} + +// mountTarget extracts the mount target path from a "mounts" entry, which is either a +// "key=value,..." string or an object with "target"/"dst" members. It returns "" if v is neither +// or declares no target. +func mountTarget(v *hujson.Value) string { + switch val := v.Value.(type) { + case hujson.Literal: + if val.Kind() != '"' { + return "" + } + for _, part := range strings.Split(val.String(), ",") { + key, value, ok := strings.Cut(part, "=") + if !ok { + continue + } + switch strings.TrimSpace(key) { + case "target", "dst", "destination": + return strings.TrimSpace(value) + } + } + case *hujson.Object: + for _, m := range val.Members { + name, ok := m.Name.Value.(hujson.Literal) + if !ok { + continue + } + value, ok := m.Value.Value.(hujson.Literal) + if !ok || value.Kind() != '"' { + continue + } + switch name.String() { + case "target", "dst", "destination": + return value.String() + } + } + } + return "" +} diff --git a/feature/fetch.go b/feature/fetch.go new file mode 100644 index 0000000..acc869e --- /dev/null +++ b/feature/fetch.go @@ -0,0 +1,105 @@ +package feature + +import ( + "context" + "fmt" + "net/http" + "os" + "path/filepath" + "sync" + "time" +) + +// Size limits for downloaded content, so a misbehaving registry or tarball cannot exhaust memory. +const ( + // maxArchiveBytes caps a downloaded Feature archive (an OCI layer blob or a tarball). + maxArchiveBytes = 64 << 20 + // maxMetadataBytes caps a devcontainer-feature.json, whether read from disk or from an archive. + maxMetadataBytes = 4 << 20 +) + +// metadataFileName is the file declaring a Feature, located at the root of the Feature's directory +// or archive. +const metadataFileName = "devcontainer-feature.json" + +// Fetcher retrieves Feature metadata for the references found in devcontainer.json files. It +// caches every result in memory for the lifetime of the Fetcher, including failures, so a +// reference shared by several files is fetched at most once per run. +type Fetcher struct { + client *http.Client + + mu sync.Mutex + cache map[string]fetchResult +} + +type fetchResult struct { + md *Metadata + err error +} + +// NewFetcher returns a Fetcher with a default HTTP client. +func NewFetcher() *Fetcher { + return &Fetcher{ + client: &http.Client{Timeout: 30 * time.Second}, + cache: map[string]fetchResult{}, + } +} + +// Fetch retrieves the metadata of the Feature referenced by raw. baseDir is the directory +// containing the devcontainer.json that references the Feature; local references are resolved +// relative to it. +func (f *Fetcher) Fetch(ctx context.Context, raw string, baseDir string) (*Metadata, error) { + ref, err := ParseRef(raw) + if err != nil { + return nil, err + } + + key := raw + if ref.Kind == KindLocal { + // The same relative reference can name different directories depending on the referencing + // file's location. + key = "local:" + filepath.Join(baseDir, raw) + } + + f.mu.Lock() + res, ok := f.cache[key] + f.mu.Unlock() + if !ok { + res.md, res.err = f.fetch(ctx, ref, baseDir) + if res.err != nil { + res.err = fmt.Errorf("fetch feature %q: %w", raw, res.err) + } + f.mu.Lock() + f.cache[key] = res + f.mu.Unlock() + } + return res.md, res.err +} + +func (f *Fetcher) fetch(ctx context.Context, ref Ref, baseDir string) (*Metadata, error) { + switch ref.Kind { + case KindLocal: + return fetchLocal(filepath.Join(baseDir, ref.Raw)) + case KindTarball: + return f.fetchTarball(ctx, ref.Raw) + default: + return f.fetchOCI(ctx, ref) + } +} + +// fetchLocal reads the metadata of the Feature in directory dir. +func fetchLocal(dir string) (*Metadata, error) { + path := filepath.Join(dir, metadataFileName) + info, err := os.Stat(path) + if err != nil { + return nil, err + } + if info.Size() > maxMetadataBytes { + return nil, fmt.Errorf("%s exceeds %d bytes", path, maxMetadataBytes) + } + src, err := os.ReadFile(path) + if err != nil { + return nil, err + } + return parseMetadata(src) +} diff --git a/feature/fetch_test.go b/feature/fetch_test.go new file mode 100644 index 0000000..6430442 --- /dev/null +++ b/feature/fetch_test.go @@ -0,0 +1,293 @@ +package feature + +import ( + "archive/tar" + "bytes" + "compress/gzip" + "fmt" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" +) + +// writeLocalFeature creates dir//devcontainer-feature.json with the given content and +// returns the base directory. +func writeLocalFeature(t *testing.T, dir, name, src string) { + t.Helper() + featureDir := filepath.Join(dir, name) + if err := os.MkdirAll(featureDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(featureDir, metadataFileName), []byte(src), 0o644); err != nil { + t.Fatal(err) + } +} + +func TestFetchLocal(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + writeLocalFeature(t, dir, "my-feature", `{"id": "my-feature", "version": "1.0.0"}`) + + f := NewFetcher() + md, err := f.Fetch(t.Context(), "./my-feature", dir) + if err != nil { + t.Fatalf("Fetch: %v", err) + } + if md.ID != "my-feature" || md.Version != "1.0.0" { + t.Errorf("got ID %q version %q, want my-feature 1.0.0", md.ID, md.Version) + } +} + +func TestFetchLocalMissing(t *testing.T) { + t.Parallel() + + f := NewFetcher() + if _, err := f.Fetch(t.Context(), "./nope", t.TempDir()); err == nil { + t.Error("Fetch of a missing local feature: got nil error") + } +} + +func TestFetchCachesResults(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + writeLocalFeature(t, dir, "f", `{"id": "before"}`) + + f := NewFetcher() + md, err := f.Fetch(t.Context(), "./f", dir) + if err != nil { + t.Fatalf("Fetch: %v", err) + } + if md.ID != "before" { + t.Fatalf("ID = %q, want before", md.ID) + } + + // A second fetch must hit the cache and not observe the changed file. + writeLocalFeature(t, dir, "f", `{"id": "after"}`) + md, err = f.Fetch(t.Context(), "./f", dir) + if err != nil { + t.Fatalf("Fetch (cached): %v", err) + } + if md.ID != "before" { + t.Errorf("cached ID = %q, want before", md.ID) + } +} + +func TestFetchMetadataParse(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + writeLocalFeature(t, dir, "f", `{ + // JSONC comments are allowed in feature metadata. + "id": "f", + "dependsOn": {"./dep1": {}, "./dep2": {"opt": true}}, + "installsAfter": ["ghcr.io/devcontainers/features/common-utils"], + }`) + + md, err := NewFetcher().Fetch(t.Context(), "./f", dir) + if err != nil { + t.Fatalf("Fetch: %v", err) + } + if got, want := strings.Join(md.DependsOn, ","), "./dep1,./dep2"; got != want { + t.Errorf("DependsOn = %q, want %q", got, want) + } + if got, want := strings.Join(md.InstallsAfter, ","), "ghcr.io/devcontainers/features/common-utils"; got != want { + t.Errorf("InstallsAfter = %q, want %q", got, want) + } +} + +// archiveWithMetadata builds an in-memory tar archive containing a devcontainer-feature.json, +// optionally gzip-compressed. +func archiveWithMetadata(t *testing.T, src string, compress bool) []byte { + t.Helper() + var buf bytes.Buffer + tw := tar.NewWriter(&buf) + files := []struct{ name, content string }{ + {"install.sh", "#!/bin/sh\n"}, + {metadataFileName, src}, + } + for _, f := range files { + if err := tw.WriteHeader(&tar.Header{Name: "./" + f.name, Mode: 0o644, Size: int64(len(f.content))}); err != nil { + t.Fatal(err) + } + if _, err := tw.Write([]byte(f.content)); err != nil { + t.Fatal(err) + } + } + if err := tw.Close(); err != nil { + t.Fatal(err) + } + if !compress { + return buf.Bytes() + } + var gzBuf bytes.Buffer + gz := gzip.NewWriter(&gzBuf) + if _, err := gz.Write(buf.Bytes()); err != nil { + t.Fatal(err) + } + if err := gz.Close(); err != nil { + t.Fatal(err) + } + return gzBuf.Bytes() +} + +func TestFetchTarball(t *testing.T) { + t.Parallel() + + archive := archiveWithMetadata(t, `{"id": "tarred", "version": "2.0.0"}`, true) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/feature.tgz" { + http.NotFound(w, r) + return + } + _, _ = w.Write(archive) + })) + defer srv.Close() + + md, err := NewFetcher().Fetch(t.Context(), srv.URL+"/feature.tgz", "") + if err != nil { + t.Fatalf("Fetch: %v", err) + } + if md.ID != "tarred" { + t.Errorf("ID = %q, want tarred", md.ID) + } +} + +func TestFetchTarballNotFound(t *testing.T) { + t.Parallel() + + srv := httptest.NewServer(http.NotFoundHandler()) + defer srv.Close() + + if _, err := NewFetcher().Fetch(t.Context(), srv.URL+"/feature.tgz", ""); err == nil { + t.Error("Fetch of a missing tarball: got nil error") + } +} + +// fakeRegistry serves a minimal OCI distribution API for a single feature artifact, requiring the +// anonymous bearer token flow. +type fakeRegistry struct { + repository string + blob []byte + blobDigest string + // useIndex makes the tag reference resolve to an image index pointing at the manifest. + useIndex bool + // manifestDigest is the digest the index points at. + manifestDigest string + token string +} + +func (fr *fakeRegistry) handler(registryHost func() string) http.Handler { + mux := http.NewServeMux() + mux.HandleFunc("/token", func(w http.ResponseWriter, r *http.Request) { + if r.URL.Query().Get("scope") != "repository:"+fr.repository+":pull" { + http.Error(w, "bad scope", http.StatusBadRequest) + return + } + _, _ = fmt.Fprintf(w, `{"token": %q}`, fr.token) + }) + authorized := func(w http.ResponseWriter, r *http.Request) bool { + if r.Header.Get("Authorization") == "Bearer "+fr.token { + return true + } + w.Header().Set("Www-Authenticate", fmt.Sprintf( + `Bearer realm="http://%s/token",service="registry",scope="repository:%s:pull"`, + registryHost(), fr.repository)) + w.WriteHeader(http.StatusUnauthorized) + return false + } + mux.HandleFunc("/v2/"+fr.repository+"/manifests/", func(w http.ResponseWriter, r *http.Request) { + if !authorized(w, r) { + return + } + reference := strings.TrimPrefix(r.URL.Path, "/v2/"+fr.repository+"/manifests/") + if fr.useIndex && reference != fr.manifestDigest { + w.Header().Set("Content-Type", ociIndexMediaType) + _, _ = fmt.Fprintf(w, `{"mediaType": %q, "manifests": [{"mediaType": %q, "digest": %q}]}`, + ociIndexMediaType, ociManifestMediaType, fr.manifestDigest) + return + } + w.Header().Set("Content-Type", ociManifestMediaType) + _, _ = fmt.Fprintf(w, `{"mediaType": %q, "layers": [{"mediaType": %q, "digest": %q}]}`, + ociManifestMediaType, featureLayerMediaType, fr.blobDigest) + }) + mux.HandleFunc("/v2/"+fr.repository+"/blobs/"+fr.blobDigest, func(w http.ResponseWriter, r *http.Request) { + if !authorized(w, r) { + return + } + _, _ = w.Write(fr.blob) + }) + return mux +} + +// startRegistry serves fr on a loopback address and returns the registry host (host:port). +func startRegistry(t *testing.T, fr *fakeRegistry) string { + t.Helper() + var host string + srv := httptest.NewServer(fr.handler(func() string { return host })) + t.Cleanup(srv.Close) + host = strings.TrimPrefix(srv.URL, "http://") + return host +} + +func TestFetchOCI(t *testing.T) { + t.Parallel() + + fr := &fakeRegistry{ + repository: "devcontainers/features/node", + blob: archiveWithMetadata(t, `{"id": "node", "version": "1.2.3"}`, false), + blobDigest: "sha256:feedface", + token: "anonymous-token", + } + host := startRegistry(t, fr) + + md, err := NewFetcher().Fetch(t.Context(), host+"/devcontainers/features/node:1", "") + if err != nil { + t.Fatalf("Fetch: %v", err) + } + if md.ID != "node" || md.Version != "1.2.3" { + t.Errorf("got ID %q version %q, want node 1.2.3", md.ID, md.Version) + } +} + +func TestFetchOCIThroughIndex(t *testing.T) { + t.Parallel() + + fr := &fakeRegistry{ + repository: "devcontainers/features/go", + blob: archiveWithMetadata(t, `{"id": "go"}`, false), + blobDigest: "sha256:cafebabe", + useIndex: true, + manifestDigest: "sha256:deadbeef", + token: "anonymous-token", + } + host := startRegistry(t, fr) + + md, err := NewFetcher().Fetch(t.Context(), host+"/devcontainers/features/go:1", "") + if err != nil { + t.Fatalf("Fetch: %v", err) + } + if md.ID != "go" { + t.Errorf("ID = %q, want go", md.ID) + } +} + +func TestFetchOCIUnknownRepository(t *testing.T) { + t.Parallel() + + fr := &fakeRegistry{ + repository: "devcontainers/features/node", + blob: archiveWithMetadata(t, `{"id": "node"}`, false), + blobDigest: "sha256:feedface", + token: "anonymous-token", + } + host := startRegistry(t, fr) + + if _, err := NewFetcher().Fetch(t.Context(), host+"/devcontainers/features/nope:1", ""); err == nil { + t.Error("Fetch of an unknown repository: got nil error") + } +} diff --git a/feature/merge.go b/feature/merge.go new file mode 100644 index 0000000..1d92cce --- /dev/null +++ b/feature/merge.go @@ -0,0 +1,219 @@ +package feature + +import ( + "context" + "fmt" + "math" + "strings" + + "github.com/tailscale/hujson" +) + +// Merge fetches the Features referenced under "/features" of root, a devcontainer.json parsed from +// a file in directory dir, and merges the properties they contribute into root in place, following +// the merge logic of the Dev Container specification. Features named by "dependsOn" are resolved +// recursively and contribute properties as well. +// +// Every node Merge adds to the tree carries the byte offset of the referencing Feature key in the +// original file, so findings on merged-in properties point at the Feature reference. Any fetch or +// parse failure is returned as an error. +func Merge(ctx context.Context, f *Fetcher, dir string, root *hujson.Value) error { + features := root.Find("/features") + if features == nil { + return nil + } + obj, ok := features.Value.(*hujson.Object) + if !ok { + return nil + } + + var declared []*contributor + for _, m := range obj.Members { + name, ok := m.Name.Value.(hujson.Literal) + if !ok || name.Kind() != '"' { + continue + } + declared = append(declared, &contributor{ref: name.String(), anchor: m.Name.StartOffset}) + } + + contribs, err := resolveAll(ctx, f, dir, declared) + if err != nil { + return err + } + ordered := installOrder(root, contribs) + + state := newMergeState(root.Value.(*hujson.Object)) + for _, c := range ordered { + state.apply(c) + } + state.finish() + return nil +} + +// contributor is one resolved Feature that contributes properties to the effective configuration. +type contributor struct { + // ref is the reference the Feature was fetched by. + ref string + // anchor is the byte offset, in the original file, of the "features" key that (directly or via + // dependencies) pulled this Feature in. + anchor int + // declIdx is the declaration index of that key, used as an ordering tiebreak. + declIdx int + // deps are the refs of the Features named by this Feature's dependsOn. + deps []string + md *Metadata +} + +// id returns the identifier the Feature is matched by in "installsAfter" and +// "overrideFeatureInstallOrder": its reference without a version. The declared metadata ID is +// accepted as well. +func (c *contributor) matches(id string) bool { + return id == refWithoutVersion(c.ref) || (c.md != nil && id == c.md.ID) +} + +// displayID returns the identifier used for members synthesized on behalf of this Feature. +func (c *contributor) displayID() string { + if c.md != nil && c.md.ID != "" { + return c.md.ID + } + return c.ref +} + +// refWithoutVersion strips the ":tag" and "@digest" suffixes off an OCI reference. +func refWithoutVersion(ref string) string { + if at := strings.LastIndex(ref, "@"); at >= 0 { + ref = ref[:at] + } + if colon := strings.LastIndex(ref, ":"); colon > strings.LastIndex(ref, "/") { + ref = ref[:colon] + } + return ref +} + +// resolveAll fetches every declared Feature and, recursively, the Features they depend on. The +// result is in discovery order (dependencies before their dependents), deduplicated by reference. +func resolveAll(ctx context.Context, f *Fetcher, dir string, declared []*contributor) ([]*contributor, error) { + seen := map[string]*contributor{} + var out []*contributor + + var visit func(c *contributor, stack []string) error + visit = func(c *contributor, stack []string) error { + for _, s := range stack { + if s == c.ref { + return fmt.Errorf("feature dependency cycle: %s", strings.Join(append(stack, c.ref), " -> ")) + } + } + if _, ok := seen[c.ref]; ok { + return nil + } + md, err := f.Fetch(ctx, c.ref, dir) + if err != nil { + return err + } + c.md = md + seen[c.ref] = c + stack = append(stack, c.ref) + for _, dep := range md.DependsOn { + c.deps = append(c.deps, dep) + if err := visit(&contributor{ref: dep, anchor: c.anchor, declIdx: c.declIdx}, stack); err != nil { + return err + } + } + out = append(out, c) + return nil + } + + for i, c := range declared { + c.declIdx = i + if err := visit(c, nil); err != nil { + return nil, err + } + } + return out, nil +} + +// installOrder sorts contribs into the order the Features would be installed in: dependencies +// always precede their dependents, "overrideFeatureInstallOrder" entries come first in the listed +// order, "installsAfter" preferences are honored when possible, and declaration order breaks the +// remaining ties. Later Features win merge conflicts, mirroring how a later installation +// overrides an earlier one. +func installOrder(root *hujson.Value, contribs []*contributor) []*contributor { + override := map[string]int{} + if v := root.Find("/overrideFeatureInstallOrder"); v != nil { + if arr, ok := v.Value.(*hujson.Array); ok { + for i, e := range arr.Elements { + if lit, ok := e.Value.(hujson.Literal); ok && lit.Kind() == '"' { + override[lit.String()] = i + } + } + } + } + overrideIdx := func(c *contributor) int { + for id, i := range override { + if c.matches(id) { + return i + } + } + return math.MaxInt + } + + emitted := map[string]bool{} + byRef := map[string]*contributor{} + for _, c := range contribs { + byRef[c.ref] = c + } + // ready reports whether every dependency of c is already emitted. + ready := func(c *contributor) bool { + for _, dep := range c.deps { + if !emitted[dep] { + return false + } + } + return true + } + // softSatisfied reports whether every Feature named by c's installsAfter that is part of this + // merge is already emitted. + softSatisfied := func(c *contributor) bool { + for _, id := range c.md.InstallsAfter { + for _, other := range contribs { + if other != c && other.matches(id) && !emitted[other.ref] { + return false + } + } + } + return true + } + + out := make([]*contributor, 0, len(contribs)) + for len(out) < len(contribs) { + var best *contributor + bestSoft := false + for _, c := range contribs { + if emitted[c.ref] || !ready(c) { + continue + } + soft := softSatisfied(c) + if best == nil || (soft && !bestSoft) { + best, bestSoft = c, soft + continue + } + if soft != bestSoft { + continue + } + if oi, boi := overrideIdx(c), overrideIdx(best); oi != boi { + if oi < boi { + best = c + } + continue + } + if c.declIdx < best.declIdx { + best = c + } + } + // resolveAll rejects dependency cycles, so a ready contributor always exists (installsAfter + // is only a preference and never blocks). + emitted[best.ref] = true + out = append(out, best) + } + return out +} diff --git a/feature/merge_test.go b/feature/merge_test.go new file mode 100644 index 0000000..e89fe87 --- /dev/null +++ b/feature/merge_test.go @@ -0,0 +1,313 @@ +package feature + +import ( + "encoding/json/v2" + "strings" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/tailscale/hujson" +) + +// mergeSrc parses src as a devcontainer.json, writes each named feature under a temporary +// directory (referenced as "./"), and merges. It returns the merged tree and the source, so +// callers can locate anchors in it. +func mergeSrc(t *testing.T, src string, features map[string]string) *hujson.Value { + t.Helper() + dir := t.TempDir() + for name, content := range features { + writeLocalFeature(t, dir, name, content) + } + root, err := hujson.Parse([]byte(src)) + if err != nil { + t.Fatalf("parse devcontainer.json: %v", err) + } + if err := Merge(t.Context(), NewFetcher(), dir, &root); err != nil { + t.Fatalf("Merge: %v", err) + } + return &root +} + +// assertJSON compares the merged tree, reduced to standard JSON, against want. +func assertJSON(t *testing.T, root *hujson.Value, want string) { + t.Helper() + clone := root.Clone() + clone.Standardize() + var got, wantVal any + if err := json.Unmarshal(clone.Pack(), &got); err != nil { + t.Fatalf("unmarshal merged tree: %v", err) + } + if err := json.Unmarshal([]byte(want), &wantVal); err != nil { + t.Fatalf("unmarshal want: %v", err) + } + if diff := cmp.Diff(wantVal, got); diff != "" { + t.Errorf("merged configuration mismatch (-want +got):\n%s\n%s", diff, clone.Pack()) + } +} + +func TestMergeNoFeatures(t *testing.T) { + t.Parallel() + + src := `{"image": "ubuntu:24.04"}` + root := mergeSrc(t, src, nil) + assertJSON(t, root, src) +} + +func TestMergeBooleanOr(t *testing.T) { + t.Parallel() + + feature := `{"id": "f", "privileged": true, "init": true}` + + t.Run("absent member is added", func(t *testing.T) { + t.Parallel() + root := mergeSrc(t, `{"features": {"./f": {}}}`, map[string]string{"f": feature}) + assertJSON(t, root, `{"features": {"./f": {}}, "privileged": true, "init": true}`) + }) + + t.Run("explicit false is overridden", func(t *testing.T) { + t.Parallel() + root := mergeSrc(t, `{"privileged": false, "features": {"./f": {}}}`, map[string]string{"f": feature}) + assertJSON(t, root, `{"privileged": true, "features": {"./f": {}}, "init": true}`) + }) + + t.Run("feature false does not override", func(t *testing.T) { + t.Parallel() + root := mergeSrc(t, `{"privileged": false, "features": {"./f": {}}}`, + map[string]string{"f": `{"id": "f", "privileged": false}`}) + assertJSON(t, root, `{"privileged": false, "features": {"./f": {}}}`) + }) +} + +func TestMergeUnionArrays(t *testing.T) { + t.Parallel() + + root := mergeSrc(t, + `{"capAdd": ["SYS_PTRACE"], "features": {"./a": {}, "./b": {}}}`, + map[string]string{ + "a": `{"id": "a", "capAdd": ["SYS_PTRACE", "NET_ADMIN"], "securityOpt": ["seccomp=unconfined"]}`, + "b": `{"id": "b", "capAdd": ["NET_ADMIN", "SYS_ADMIN"]}`, + }) + assertJSON(t, root, `{ + "capAdd": ["SYS_PTRACE", "NET_ADMIN", "SYS_ADMIN"], + "features": {"./a": {}, "./b": {}}, + "securityOpt": ["seccomp=unconfined"] + }`) +} + +func TestMergeContainerEnv(t *testing.T) { + t.Parallel() + + root := mergeSrc(t, + `{"containerEnv": {"USER_VAR": "user"}, "features": {"./a": {}, "./b": {}}}`, + map[string]string{ + "a": `{"id": "a", "containerEnv": {"USER_VAR": "a", "SHARED": "a", "A_ONLY": "a"}}`, + "b": `{"id": "b", "containerEnv": {"SHARED": "b"}}`, + }) + // The user's own key always wins; for keys only features set, the later feature wins. + assertJSON(t, root, `{ + "containerEnv": {"USER_VAR": "user", "SHARED": "b", "A_ONLY": "a"}, + "features": {"./a": {}, "./b": {}} + }`) +} + +func TestMergeMounts(t *testing.T) { + t.Parallel() + + root := mergeSrc(t, + `{ + "mounts": [{"type": "volume", "source": "user-vol", "target": "/data"}], + "features": {"./a": {}, "./b": {}} + }`, + map[string]string{ + "a": `{"id": "a", "mounts": [ + {"type": "volume", "source": "a-vol", "target": "/data"}, + "source=a-cache,target=/cache,type=volume" + ]}`, + "b": `{"id": "b", "mounts": ["source=b-cache,target=/cache,type=volume"]}`, + }) + // /data is mounted by the user (user wins); /cache is contributed by both features (the later + // one wins). + assertJSON(t, root, `{ + "mounts": [ + {"type": "volume", "source": "user-vol", "target": "/data"}, + "source=b-cache,target=/cache,type=volume" + ], + "features": {"./a": {}, "./b": {}} + }`) +} + +func TestMergeCustomizations(t *testing.T) { + t.Parallel() + + root := mergeSrc(t, + `{ + "customizations": {"vscode": {"extensions": ["user.ext"], "settings": {"a": "user"}}}, + "features": {"./f": {}} + }`, + map[string]string{ + "f": `{"id": "f", "customizations": {"vscode": { + "extensions": ["feature.ext", "user.ext"], + "settings": {"a": "feature", "b": "feature"} + }}}`, + }) + // Extension lists are concatenated (without duplicates); on a scalar conflict the user wins. + assertJSON(t, root, `{ + "customizations": {"vscode": { + "extensions": ["feature.ext", "user.ext"], + "settings": {"a": "user", "b": "feature"} + }}, + "features": {"./f": {}} + }`) +} + +func TestMergeLifecycleHooks(t *testing.T) { + t.Parallel() + + t.Run("user command in string form", func(t *testing.T) { + t.Parallel() + root := mergeSrc(t, + `{"postCreateCommand": "make setup", "features": {"./f": {}}}`, + map[string]string{"f": `{"id": "f", "postCreateCommand": "f-setup.sh"}`}) + assertJSON(t, root, `{ + "postCreateCommand": {"f": "f-setup.sh", "devcontainer.json": "make setup"}, + "features": {"./f": {}} + }`) + }) + + t.Run("user command in object form", func(t *testing.T) { + t.Parallel() + root := mergeSrc(t, + `{"onCreateCommand": {"mine": "make setup"}, "features": {"./f": {}}}`, + map[string]string{"f": `{"id": "f", "onCreateCommand": ["echo", "hi"]}`}) + assertJSON(t, root, `{ + "onCreateCommand": {"f": ["echo", "hi"], "mine": "make setup"}, + "features": {"./f": {}} + }`) + }) + + t.Run("no user command", func(t *testing.T) { + t.Parallel() + root := mergeSrc(t, + `{"features": {"./f": {}}}`, + map[string]string{"f": `{"id": "f", "postStartCommand": "f-start.sh"}`}) + assertJSON(t, root, `{ + "postStartCommand": {"f": "f-start.sh"}, + "features": {"./f": {}} + }`) + }) +} + +func TestMergeDependsOn(t *testing.T) { + t.Parallel() + + root := mergeSrc(t, + `{"features": {"./a": {}}}`, + map[string]string{ + "a": `{"id": "a", "dependsOn": {"./dep": {}}, "containerEnv": {"SHARED": "a"}}`, + "dep": `{"id": "dep", "containerEnv": {"SHARED": "dep", "DEP_ONLY": "dep"}, "privileged": true}`, + }) + // The dependency installs first, so the dependent feature wins the SHARED conflict; the + // dependency's other contributions are merged as well. + assertJSON(t, root, `{ + "features": {"./a": {}}, + "containerEnv": {"SHARED": "a", "DEP_ONLY": "dep"}, + "privileged": true + }`) +} + +func TestMergeDependsOnCycle(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + writeLocalFeature(t, dir, "a", `{"id": "a", "dependsOn": {"./b": {}}}`) + writeLocalFeature(t, dir, "b", `{"id": "b", "dependsOn": {"./a": {}}}`) + root, err := hujson.Parse([]byte(`{"features": {"./a": {}}}`)) + if err != nil { + t.Fatal(err) + } + if err := Merge(t.Context(), NewFetcher(), dir, &root); err == nil || !strings.Contains(err.Error(), "cycle") { + t.Errorf("Merge with a dependency cycle: err = %v, want a cycle error", err) + } +} + +func TestMergeFetchFailure(t *testing.T) { + t.Parallel() + + root, err := hujson.Parse([]byte(`{"features": {"./missing": {}}}`)) + if err != nil { + t.Fatal(err) + } + if err := Merge(t.Context(), NewFetcher(), t.TempDir(), &root); err == nil { + t.Error("Merge with an unresolvable feature: got nil error") + } +} + +func TestMergeInstallsAfter(t *testing.T) { + t.Parallel() + + // b declares installsAfter a, so a installs first and b wins the conflict even though b is + // declared first. + root := mergeSrc(t, + `{"features": {"./b": {}, "./a": {}}}`, + map[string]string{ + "a": `{"id": "a", "containerEnv": {"SHARED": "a"}}`, + "b": `{"id": "b", "installsAfter": ["a"], "containerEnv": {"SHARED": "b"}}`, + }) + assertJSON(t, root, `{ + "features": {"./b": {}, "./a": {}}, + "containerEnv": {"SHARED": "b"} + }`) +} + +func TestMergeOverrideFeatureInstallOrder(t *testing.T) { + t.Parallel() + + // The override moves b to the front, so a installs later and wins the conflict. + root := mergeSrc(t, + `{ + "overrideFeatureInstallOrder": ["b"], + "features": {"./a": {}, "./b": {}} + }`, + map[string]string{ + "a": `{"id": "a", "containerEnv": {"SHARED": "a"}}`, + "b": `{"id": "b", "containerEnv": {"SHARED": "b"}}`, + }) + assertJSON(t, root, `{ + "overrideFeatureInstallOrder": ["b"], + "features": {"./a": {}, "./b": {}}, + "containerEnv": {"SHARED": "a"} + }`) +} + +func TestMergeAnchorsPointAtFeatureKey(t *testing.T) { + t.Parallel() + + src := `{ + "name": "test", + "features": { + "./f": {} + } +}` + root := mergeSrc(t, src, map[string]string{ + "f": `{"id": "f", "privileged": true, "containerEnv": {"KEY": "value"}}`, + }) + anchor := strings.Index(src, `"./f"`) + if anchor < 0 { + t.Fatal("anchor not found in source") + } + for _, ptr := range []string{"/privileged", "/containerEnv", "/containerEnv/KEY"} { + v := root.Find(ptr) + if v == nil { + t.Errorf("merged tree lacks %s", ptr) + continue + } + if v.StartOffset != anchor { + t.Errorf("%s StartOffset = %d, want %d (the feature key)", ptr, v.StartOffset, anchor) + } + } + + // The user's own nodes keep their original positions. + if v := root.Find("/name"); v == nil || v.StartOffset != strings.Index(src, `"test"`) { + t.Errorf("/name StartOffset changed: %+v", v) + } +} diff --git a/feature/metadata.go b/feature/metadata.go new file mode 100644 index 0000000..57b2836 --- /dev/null +++ b/feature/metadata.go @@ -0,0 +1,68 @@ +package feature + +import ( + "fmt" + + "github.com/tailscale/hujson" +) + +// Metadata is the declaration of one fetched Feature: the content of its +// devcontainer-feature.json. +type Metadata struct { + // ID is the Feature's declared identifier. + ID string + // Version is the Feature's declared version. + Version string + // DependsOn lists the references of the Features this Feature depends on, in declaration + // order. Dependencies are installed before the Feature and contribute properties of their own. + DependsOn []string + // InstallsAfter lists Feature IDs this Feature prefers to be installed after. Unlike DependsOn + // it does not pull in new Features; it only influences installation order. + InstallsAfter []string + // Root is the parsed devcontainer-feature.json with comments stripped. It is the source of + // truth for the properties the Feature contributes (e.g. Root.Find("/containerEnv")). + Root hujson.Value +} + +// parseMetadata parses src, the content of a devcontainer-feature.json. +func parseMetadata(src []byte) (*Metadata, error) { + root, err := hujson.Parse(src) + if err != nil { + return nil, fmt.Errorf("parse devcontainer-feature.json: %w", err) + } + root.Minimize() + if _, ok := root.Value.(*hujson.Object); !ok { + return nil, fmt.Errorf("parse devcontainer-feature.json: root is not an object") + } + + md := &Metadata{Root: root} + if v := root.Find("/id"); v != nil { + if lit, ok := v.Value.(hujson.Literal); ok && lit.Kind() == '"' { + md.ID = lit.String() + } + } + if v := root.Find("/version"); v != nil { + if lit, ok := v.Value.(hujson.Literal); ok && lit.Kind() == '"' { + md.Version = lit.String() + } + } + if v := root.Find("/dependsOn"); v != nil { + if obj, ok := v.Value.(*hujson.Object); ok { + for _, m := range obj.Members { + if lit, ok := m.Name.Value.(hujson.Literal); ok && lit.Kind() == '"' { + md.DependsOn = append(md.DependsOn, lit.String()) + } + } + } + } + if v := root.Find("/installsAfter"); v != nil { + if arr, ok := v.Value.(*hujson.Array); ok { + for _, e := range arr.Elements { + if lit, ok := e.Value.(hujson.Literal); ok && lit.Kind() == '"' { + md.InstallsAfter = append(md.InstallsAfter, lit.String()) + } + } + } + } + return md, nil +} diff --git a/feature/oci.go b/feature/oci.go new file mode 100644 index 0000000..f2b114b --- /dev/null +++ b/feature/oci.go @@ -0,0 +1,240 @@ +package feature + +import ( + "context" + "encoding/json/v2" + "fmt" + "io" + "net/http" + "net/url" + "strings" +) + +// Media types of the OCI artifacts a published Feature consists of. Docker registries may rewrite +// the manifest media types, so both the OCI and Docker variants are accepted. +const ( + ociManifestMediaType = "application/vnd.oci.image.manifest.v1+json" + ociIndexMediaType = "application/vnd.oci.image.index.v1+json" + dockerManifestMediaType = "application/vnd.docker.distribution.manifest.v2+json" + dockerListMediaType = "application/vnd.docker.distribution.manifest.list.v2+json" + // featureLayerMediaType is the media type of the single tar layer a Feature is packaged as, per + // the Features distribution specification. + featureLayerMediaType = "application/vnd.devcontainers.layer.v1+tar" +) + +// manifestAccept is the Accept header for manifest requests. +var manifestAccept = strings.Join([]string{ + ociManifestMediaType, + ociIndexMediaType, + dockerManifestMediaType, + dockerListMediaType, +}, ", ") + +// ociDescriptor is a content descriptor within a manifest or index. +type ociDescriptor struct { + MediaType string `json:"mediaType"` + Digest string `json:"digest"` +} + +// ociManifest is the subset of an OCI image manifest or index needed to locate the Feature layer. +// A manifest carries Layers; an index carries Manifests. +type ociManifest struct { + Layers []ociDescriptor `json:"layers"` + Manifests []ociDescriptor `json:"manifests"` +} + +// fetchOCI retrieves a Feature distributed as an OCI artifact, using anonymous pull access. +func (f *Fetcher) fetchOCI(ctx context.Context, ref Ref) (*Metadata, error) { + reference := ref.Tag + if ref.Digest != "" { + reference = ref.Digest + } + + // The token, obtained lazily on the first 401 challenge, is shared by the subsequent requests + // against the same repository. + var token string + manifest, err := f.fetchManifest(ctx, ref, reference, &token) + if err != nil { + return nil, err + } + if len(manifest.Manifests) > 0 { + // The reference resolved to an index; Features are single-platform, so follow its first + // entry. + manifest, err = f.fetchManifest(ctx, ref, manifest.Manifests[0].Digest, &token) + if err != nil { + return nil, err + } + } + + layer, err := featureLayer(manifest) + if err != nil { + return nil, err + } + blobURL := fmt.Sprintf("%s://%s/v2/%s/blobs/%s", registryScheme(ref.Registry), ref.Registry, ref.Repository, layer.Digest) + resp, err := f.registryGet(ctx, ref, blobURL, "", &token) + if err != nil { + return nil, err + } + defer func() { _ = resp.Body.Close() }() + src, err := metadataFromArchive(io.LimitReader(resp.Body, maxArchiveBytes)) + if err != nil { + return nil, fmt.Errorf("read layer %s: %w", layer.Digest, err) + } + return parseMetadata(src) +} + +// featureLayer picks the layer carrying the Feature archive out of manifest: the layer with the +// Features distribution media type, or the sole layer if none declares it. +func featureLayer(manifest *ociManifest) (ociDescriptor, error) { + for _, layer := range manifest.Layers { + if layer.MediaType == featureLayerMediaType { + return layer, nil + } + } + if len(manifest.Layers) == 1 { + return manifest.Layers[0], nil + } + return ociDescriptor{}, fmt.Errorf("manifest has no %s layer", featureLayerMediaType) +} + +// fetchManifest retrieves and decodes the manifest (or index) at reference, a tag or digest. +func (f *Fetcher) fetchManifest(ctx context.Context, ref Ref, reference string, token *string) (*ociManifest, error) { + manifestURL := fmt.Sprintf("%s://%s/v2/%s/manifests/%s", registryScheme(ref.Registry), ref.Registry, ref.Repository, reference) + resp, err := f.registryGet(ctx, ref, manifestURL, manifestAccept, token) + if err != nil { + return nil, err + } + defer func() { _ = resp.Body.Close() }() + data, err := io.ReadAll(io.LimitReader(resp.Body, maxMetadataBytes)) + if err != nil { + return nil, err + } + var manifest ociManifest + if err := json.Unmarshal(data, &manifest); err != nil { + return nil, fmt.Errorf("decode manifest %s: %w", reference, err) + } + return &manifest, nil +} + +// registryGet performs an authenticated GET against a registry endpoint. On a 401 challenge it +// obtains an anonymous bearer token per the OCI distribution auth flow, stores it in *token for +// reuse, and retries once. +func (f *Fetcher) registryGet(ctx context.Context, ref Ref, url, accept string, token *string) (*http.Response, error) { + do := func() (*http.Response, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + if err != nil { + return nil, err + } + if accept != "" { + req.Header.Set("Accept", accept) + } + if *token != "" { + req.Header.Set("Authorization", "Bearer "+*token) + } + return f.client.Do(req) + } + + resp, err := do() + if err != nil { + return nil, err + } + if resp.StatusCode == http.StatusUnauthorized && *token == "" { + challenge := resp.Header.Get("Www-Authenticate") + _ = resp.Body.Close() + *token, err = f.fetchToken(ctx, ref, challenge) + if err != nil { + return nil, err + } + resp, err = do() + if err != nil { + return nil, err + } + } + if resp.StatusCode != http.StatusOK { + status := resp.Status + _ = resp.Body.Close() + return nil, fmt.Errorf("GET %s: %s", url, status) + } + return resp, nil +} + +// fetchToken obtains an anonymous pull token from the token endpoint named by a Bearer challenge +// (RFC 6750): `Bearer realm="...",service="...",scope="..."`. +func (f *Fetcher) fetchToken(ctx context.Context, ref Ref, challenge string) (string, error) { + scheme, rest, ok := strings.Cut(strings.TrimSpace(challenge), " ") + if !ok || !strings.EqualFold(scheme, "Bearer") { + return "", fmt.Errorf("registry %s requires unsupported authentication %q", ref.Registry, challenge) + } + params := parseChallengeParams(rest) + realm := params["realm"] + if realm == "" { + return "", fmt.Errorf("registry %s sent a Bearer challenge without a realm", ref.Registry) + } + + query := url.Values{} + if s := params["service"]; s != "" { + query.Set("service", s) + } + scope := params["scope"] + if scope == "" { + scope = fmt.Sprintf("repository:%s:pull", ref.Repository) + } + query.Set("scope", scope) + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, realm+"?"+query.Encode(), nil) + if err != nil { + return "", err + } + resp, err := f.client.Do(req) + if err != nil { + return "", err + } + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode != http.StatusOK { + return "", fmt.Errorf("GET %s: %s", realm, resp.Status) + } + data, err := io.ReadAll(io.LimitReader(resp.Body, maxMetadataBytes)) + if err != nil { + return "", err + } + var body struct { + Token string `json:"token"` + AccessToken string `json:"access_token"` + } + if err := json.Unmarshal(data, &body); err != nil { + return "", fmt.Errorf("decode token response from %s: %w", realm, err) + } + if body.Token != "" { + return body.Token, nil + } + if body.AccessToken != "" { + return body.AccessToken, nil + } + return "", fmt.Errorf("token response from %s contains no token", realm) +} + +// parseChallengeParams parses the comma-separated key="value" parameters of an auth challenge. +func parseChallengeParams(s string) map[string]string { + params := map[string]string{} + for _, part := range strings.Split(s, ",") { + key, value, ok := strings.Cut(strings.TrimSpace(part), "=") + if !ok { + continue + } + params[strings.ToLower(key)] = strings.Trim(value, `"`) + } + return params +} + +// registryScheme returns the URL scheme for reaching a registry host: plain HTTP for loopback +// hosts (local test registries), HTTPS otherwise. +func registryScheme(host string) string { + h := host + if colon := strings.LastIndex(h, ":"); colon >= 0 { + h = h[:colon] + } + if h == "localhost" || h == "127.0.0.1" || h == "::1" || h == "[::1]" { + return "http" + } + return "https" +} diff --git a/feature/ref.go b/feature/ref.go new file mode 100644 index 0000000..e5bed72 --- /dev/null +++ b/feature/ref.go @@ -0,0 +1,77 @@ +// Package feature fetches Dev Container Features referenced by a devcontainer.json and merges the +// properties they contribute into the parsed configuration, producing the effective configuration +// defined by the Dev Container specification's merge logic (see +// https://containers.dev/implementors/spec/#merge-logic). +package feature + +import ( + "fmt" + "strings" +) + +// RefKind identifies how a Feature reference locates the Feature. +type RefKind int + +const ( + // KindOCI is a reference to a Feature distributed as an OCI artifact, e.g. + // "ghcr.io/devcontainers/features/node:1". + KindOCI RefKind = iota + // KindTarball is a direct HTTP(S) URI to a Feature tarball. + KindTarball + // KindLocal is a relative path to a Feature directory next to the devcontainer.json. + KindLocal +) + +// Ref is a parsed Feature reference, as used for the keys of the "features" object in a +// devcontainer.json. +type Ref struct { + // Raw is the reference exactly as written. + Raw string + // Kind identifies how the reference locates the Feature. + Kind RefKind + // Registry, Repository, Tag, and Digest are the components of an OCI reference; they are empty + // for other kinds. Tag defaults to "latest" when the reference names neither a tag nor a digest. + Registry string + Repository string + Tag string + Digest string +} + +// ParseRef parses a Feature reference. Relative paths ("./..." or "../...") are local Features, +// HTTP(S) URIs are tarball Features, and everything else is parsed as an OCI reference of the form +// "registry/repository[:tag][@digest]". +func ParseRef(raw string) (Ref, error) { + if strings.HasPrefix(raw, "./") || strings.HasPrefix(raw, "../") { + return Ref{Raw: raw, Kind: KindLocal}, nil + } + if strings.HasPrefix(raw, "http://") || strings.HasPrefix(raw, "https://") { + return Ref{Raw: raw, Kind: KindTarball}, nil + } + + ref := Ref{Raw: raw, Kind: KindOCI} + rest := raw + if at := strings.LastIndex(rest, "@"); at >= 0 { + ref.Digest = rest[at+1:] + rest = rest[:at] + if !strings.HasPrefix(ref.Digest, "sha256:") { + return Ref{}, fmt.Errorf("invalid feature reference %q: digest must start with \"sha256:\"", raw) + } + } + // A colon after the last slash separates the tag; a colon before it belongs to a registry host + // with a port (e.g. "localhost:5000/f"). + if colon := strings.LastIndex(rest, ":"); colon > strings.LastIndex(rest, "/") { + ref.Tag = rest[colon+1:] + rest = rest[:colon] + } + if ref.Tag == "" && ref.Digest == "" { + ref.Tag = "latest" + } + + registry, repository, ok := strings.Cut(rest, "/") + if !ok || registry == "" || repository == "" { + return Ref{}, fmt.Errorf("invalid feature reference %q: want \"registry/repository[:tag]\"", raw) + } + ref.Registry = registry + ref.Repository = repository + return ref, nil +} diff --git a/feature/ref_test.go b/feature/ref_test.go new file mode 100644 index 0000000..85ff5f3 --- /dev/null +++ b/feature/ref_test.go @@ -0,0 +1,82 @@ +package feature + +import "testing" + +func TestParseRef(t *testing.T) { + t.Parallel() + + tests := []struct { + raw string + want Ref + wantErr bool + }{ + { + raw: "ghcr.io/devcontainers/features/node:1", + want: Ref{Raw: "ghcr.io/devcontainers/features/node:1", Kind: KindOCI, Registry: "ghcr.io", Repository: "devcontainers/features/node", Tag: "1"}, + }, + { + raw: "ghcr.io/devcontainers/features/node", + want: Ref{Raw: "ghcr.io/devcontainers/features/node", Kind: KindOCI, Registry: "ghcr.io", Repository: "devcontainers/features/node", Tag: "latest"}, + }, + { + raw: "ghcr.io/devcontainers/features/node@sha256:0123456789abcdef", + want: Ref{ + Raw: "ghcr.io/devcontainers/features/node@sha256:0123456789abcdef", Kind: KindOCI, + Registry: "ghcr.io", Repository: "devcontainers/features/node", Digest: "sha256:0123456789abcdef", + }, + }, + { + raw: "localhost:5000/features/go:2", + want: Ref{Raw: "localhost:5000/features/go:2", Kind: KindOCI, Registry: "localhost:5000", Repository: "features/go", Tag: "2"}, + }, + { + raw: "./local-feature", + want: Ref{Raw: "./local-feature", Kind: KindLocal}, + }, + { + raw: "../sibling-feature", + want: Ref{Raw: "../sibling-feature", Kind: KindLocal}, + }, + { + raw: "https://example.com/features/foo.tgz", + want: Ref{Raw: "https://example.com/features/foo.tgz", Kind: KindTarball}, + }, + {raw: "no-slash", wantErr: true}, + {raw: "ghcr.io/features/node@md5:abc", wantErr: true}, + } + for _, tt := range tests { + t.Run(tt.raw, func(t *testing.T) { + t.Parallel() + got, err := ParseRef(tt.raw) + if tt.wantErr { + if err == nil { + t.Fatalf("ParseRef(%q) = %+v, want error", tt.raw, got) + } + return + } + if err != nil { + t.Fatalf("ParseRef(%q): %v", tt.raw, err) + } + if got != tt.want { + t.Errorf("ParseRef(%q) = %+v, want %+v", tt.raw, got, tt.want) + } + }) + } +} + +func TestRefWithoutVersion(t *testing.T) { + t.Parallel() + + tests := []struct{ ref, want string }{ + {"ghcr.io/devcontainers/features/node:1", "ghcr.io/devcontainers/features/node"}, + {"ghcr.io/devcontainers/features/node", "ghcr.io/devcontainers/features/node"}, + {"ghcr.io/devcontainers/features/node@sha256:abc", "ghcr.io/devcontainers/features/node"}, + {"localhost:5000/features/go:2", "localhost:5000/features/go"}, + {"localhost:5000/features/go", "localhost:5000/features/go"}, + } + for _, tt := range tests { + if got := refWithoutVersion(tt.ref); got != tt.want { + t.Errorf("refWithoutVersion(%q) = %q, want %q", tt.ref, got, tt.want) + } + } +} diff --git a/feature/tarball.go b/feature/tarball.go new file mode 100644 index 0000000..f5b6139 --- /dev/null +++ b/feature/tarball.go @@ -0,0 +1,76 @@ +package feature + +import ( + "archive/tar" + "bufio" + "compress/gzip" + "context" + "errors" + "fmt" + "io" + "net/http" + "path" +) + +// fetchTarball retrieves a Feature distributed as a direct HTTP(S) URI to its archive. +func (f *Fetcher) fetchTarball(ctx context.Context, url string) (*Metadata, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + if err != nil { + return nil, err + } + resp, err := f.client.Do(req) + if err != nil { + return nil, err + } + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("GET %s: %s", url, resp.Status) + } + src, err := metadataFromArchive(io.LimitReader(resp.Body, maxArchiveBytes)) + if err != nil { + return nil, fmt.Errorf("read archive %s: %w", url, err) + } + return parseMetadata(src) +} + +// metadataFromArchive extracts the devcontainer-feature.json at the root of the tar archive read +// from r. The archive may be gzip-compressed (Features are published as .tgz tarballs) or a plain +// tar (the OCI layer format). +func metadataFromArchive(r io.Reader) ([]byte, error) { + br := bufio.NewReader(r) + if magic, err := br.Peek(2); err == nil && magic[0] == 0x1f && magic[1] == 0x8b { + gz, err := gzip.NewReader(br) + if err != nil { + return nil, err + } + defer func() { _ = gz.Close() }() + return metadataFromTar(gz) + } + return metadataFromTar(br) +} + +// metadataFromTar scans the tar stream read from r for the devcontainer-feature.json entry at the +// archive root and returns its content. +func metadataFromTar(r io.Reader) ([]byte, error) { + tr := tar.NewReader(r) + for { + hdr, err := tr.Next() + if errors.Is(err, io.EOF) { + return nil, fmt.Errorf("no %s in archive", metadataFileName) + } + if err != nil { + return nil, err + } + if path.Clean(hdr.Name) != metadataFileName { + continue + } + src, err := io.ReadAll(io.LimitReader(tr, maxMetadataBytes+1)) + if err != nil { + return nil, err + } + if len(src) > maxMetadataBytes { + return nil, fmt.Errorf("%s exceeds %d bytes", metadataFileName, maxMetadataBytes) + } + return src, nil + } +} diff --git a/linter/linter.go b/linter/linter.go index 2b10e93..36b2157 100644 --- a/linter/linter.go +++ b/linter/linter.go @@ -35,12 +35,20 @@ type ConfigFile struct { Type FileType } +// Transform mutates a parsed configuration file before rules run, e.g. to merge Feature-contributed +// properties into the effective configuration. It may modify ctx.Root in place; ctx.Src is left +// untouched, so any node a Transform adds must carry offsets pointing into the original source. An +// error aborts the lint of that file. +type Transform func(ctx context.Context, fctx *Context) error + // Linter runs a set of rules against devcontainer configuration files. type Linter struct { patterns map[FileType][]pattern // severities holds the effective severity of each rule, keyed by rule ID, as specified when the // rule was registered via RegisterRule. severities map[string]Severity + // transform, if set, mutates each parsed file before rules run. See SetTransform. + transform Transform } // New returns an empty Linter. Use RegisterRule to add rules to it. @@ -48,6 +56,12 @@ func New() *Linter { return &Linter{patterns: map[FileType][]pattern{}, severities: map[string]Severity{}} } +// SetTransform installs t to run on each parsed file before rules are applied. Only one transform +// can be installed; a later call replaces the previous one. +func (l *Linter) SetTransform(t Transform) { + l.transform = t +} + // RegisterRule adds r to the linter, to run at the given severity. func (l *Linter) RegisterRule(r *Rule, severity Severity) { l.severities[r.ID] = severity @@ -113,6 +127,11 @@ func (l *Linter) Lint(ctx context.Context, path string, src []byte, fileType Fil return nil, nil } rctx := &Context{Path: path, Type: fileType, Src: src, Root: &root} + if l.transform != nil { + if err := l.transform(ctx, rctx); err != nil { + return nil, fmt.Errorf("transform %s: %w", path, err) + } + } pos := newPositions(src) ignores := buildIgnoreIndex(&root, pos) diff --git a/linter/linter_test.go b/linter/linter_test.go index 277552b..1f3c2bb 100644 --- a/linter/linter_test.go +++ b/linter/linter_test.go @@ -1,12 +1,15 @@ package linter_test import ( + "context" + "errors" "path/filepath" "slices" "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 { @@ -218,6 +221,81 @@ func TestLintParseError(t *testing.T) { } } +// flagRule is a stub Rule that reports the value at "/flag" when it is true, used to observe +// mutations a Transform makes to the syntax tree. +var flagRule = &linter.Rule{ + ID: "flag-rule", + Description: "reports a true /flag value", + FileTypes: []linter.FileType{linter.Devcontainer}, + Paths: []string{"/flag"}, + Check: func(_ *linter.Context, node *linter.Node) []linter.Finding { + if lit, ok := node.Value.Value.(hujson.Literal); ok && lit.Bool() { + return []linter.Finding{{Message: "flag is true", Offset: node.Value.StartOffset}} + } + return nil + }, +} + +func TestLintTransform(t *testing.T) { + t.Parallel() + + // The transform adds a synthetic "flag": true member whose offsets point at the "name" member of + // the original source, so the finding must resolve to that position. + src := "{\n \"name\": \"test\"\n}" + + t.Run("rules see the transformed tree at anchored positions", func(t *testing.T) { + t.Parallel() + l := linter.New() + l.RegisterRule(flagRule, linter.SeverityWarn) + l.SetTransform(func(_ context.Context, fctx *linter.Context) error { + obj := fctx.Root.Value.(*hujson.Object) + anchor := obj.Members[0].Name.StartOffset + obj.Members = append(obj.Members, hujson.ObjectMember{ + Name: hujson.Value{Value: hujson.String("flag"), StartOffset: anchor, EndOffset: anchor}, + Value: hujson.Value{Value: hujson.Bool(true), StartOffset: anchor, EndOffset: anchor}, + }) + return nil + }) + issues, err := l.Lint(t.Context(), "devcontainer.json", []byte(src), linter.Devcontainer) + if err != nil { + t.Fatalf("Lint: %v", err) + } + if len(issues) != 1 { + t.Fatalf("got %d issues %v, want 1", len(issues), issues) + } + if issues[0].Line != 2 || issues[0].Col != 3 { + t.Errorf("position = %d:%d, want 2:3", issues[0].Line, issues[0].Col) + } + }) + + t.Run("transform error aborts the lint", func(t *testing.T) { + t.Parallel() + l := linter.New() + l.RegisterRule(flagRule, linter.SeverityWarn) + wantErr := errors.New("fetch failed") + l.SetTransform(func(context.Context, *linter.Context) error { return wantErr }) + if _, err := l.Lint(t.Context(), "devcontainer.json", []byte(src), linter.Devcontainer); !errors.Is(err, wantErr) { + t.Errorf("Lint error = %v, want %v", err, wantErr) + } + }) + + t.Run("transform is skipped when no rule applies", func(t *testing.T) { + t.Parallel() + l := linter.New() + called := false + l.SetTransform(func(context.Context, *linter.Context) error { + called = true + return nil + }) + if _, err := l.Lint(t.Context(), "devcontainer.json", []byte(src), linter.Devcontainer); err != nil { + t.Fatalf("Lint: %v", err) + } + if called { + t.Error("transform ran although no rule is registered") + } + }) +} + // 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{ From 7bf26b6297ba676fc778df80162fc1828dbb90e6 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 12 Jul 2026 23:56:44 +0000 Subject: [PATCH 2/4] fix(cli): let -merge-features explicitly disable config file mergeFeatures Previously -merge-features and the config file's "mergeFeatures" member merged as a boolean OR, so the CLI flag could only enable merging, never disable it. Track whether -merge-features was explicitly passed (via flag.Visit) so mergeConfig can distinguish "not given" (defer to the config file) from "given as false" (override "mergeFeatures": true), matching how -platform already overrides the config file's "platforms" member. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01UrZdqcow5W3yq4uGMMPS8D --- README.md | 9 ++++++++- cmd/decolint/config.go | 12 +++++++----- cmd/decolint/config_test.go | 10 +++++----- cmd/decolint/main_test.go | 8 ++++++++ cmd/decolint/opts.go | 14 ++++++++++++-- cmd/decolint/opts_test.go | 18 ++++++++++++------ 6 files changed, 52 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index f1a59fa..a389bda 100644 --- a/README.md +++ b/README.md @@ -113,6 +113,12 @@ properties it contributes, and lint the merged configuration instead: decolint -merge-features -config .decolint.jsonc ``` +When given explicitly, `-merge-features` (in either form, +`-merge-features` or `-merge-features=false`) overrides the config +file's `mergeFeatures` member; for example, `-merge-features=false` +disables merging even if the config file sets `"mergeFeatures": +true`. Leaving the flag unset defers to the config file. + - OCI references (e.g. `ghcr.io/devcontainers/features/node:1`) are pulled from the registry with anonymous access, direct HTTP(S) tarball URIs are downloaded, and relative paths (e.g. @@ -195,7 +201,8 @@ individual rule's severity and takes precedence over its category. rules run in addition to platform-agnostic ones; the `-platform` flag, when given, takes precedence over it. `mergeFeatures` set to `true` enables [merging Features](#merging-features), same as the -`-merge-features` flag. +`-merge-features` flag; when given explicitly, `-merge-features` +takes precedence over `mergeFeatures` in either direction. For the strictest configuration, enable every category: diff --git a/cmd/decolint/config.go b/cmd/decolint/config.go index 921e82b..df4c525 100644 --- a/cmd/decolint/config.go +++ b/cmd/decolint/config.go @@ -85,15 +85,17 @@ func writeSeverityMap(enc *jsontext.Encoder, m map[string]linter.Severity) error return enc.WriteToken(jsontext.EndObject) } -// mergeConfig returns cfg with any CLI-provided opts fields applied. Platforms (-platform), when -// given, overrides the config file's value; Categories and Rules are config-file only. MergeFeatures -// (-merge-features) can be enabled by either source, so it merges as a boolean OR rather than an -// override. +// mergeConfig returns cfg with any CLI-provided opts fields applied as overrides. Platforms +// (-platform) and MergeFeatures (-merge-features), when explicitly given, override the config +// file's value in either direction (e.g. "-merge-features=false" disables merging even if the +// config file sets "mergeFeatures": true); Categories and Rules are config-file only. func mergeConfig(opts Options, cfg Config) Config { if len(opts.Platforms) > 0 { cfg.Platforms = opts.Platforms } - cfg.MergeFeatures = cfg.MergeFeatures || opts.MergeFeatures + if opts.mergeFeaturesSet { + cfg.MergeFeatures = opts.MergeFeatures + } return cfg } diff --git a/cmd/decolint/config_test.go b/cmd/decolint/config_test.go index 3caa097..2660b72 100644 --- a/cmd/decolint/config_test.go +++ b/cmd/decolint/config_test.go @@ -279,21 +279,21 @@ func TestMergeConfig(t *testing.T) { }, { "CLI merge-features flag enables it", - Options{MergeFeatures: true}, + Options{MergeFeatures: true, mergeFeaturesSet: true}, Config{}, Config{MergeFeatures: true}, }, { - "config file mergeFeatures member enables it", + "CLI merge-features flag not given falls back to config file mergeFeatures", Options{}, Config{MergeFeatures: true}, Config{MergeFeatures: true}, }, { - "CLI flag does not disable config file mergeFeatures", - Options{MergeFeatures: false}, - Config{MergeFeatures: true}, + "CLI merge-features=false overrides config file mergeFeatures: true", + Options{MergeFeatures: false, mergeFeaturesSet: true}, Config{MergeFeatures: true}, + Config{MergeFeatures: false}, }, } for _, tt := range tests { diff --git a/cmd/decolint/main_test.go b/cmd/decolint/main_test.go index eb55bf5..e5b2a82 100644 --- a/cmd/decolint/main_test.go +++ b/cmd/decolint/main_test.go @@ -177,6 +177,14 @@ func TestRun(t *testing.T) { }, wantExitCode: 1, }, + { + // -merge-features=false, given explicitly, overrides merge-on.jsonc's "mergeFeatures": + // true and disables merging. + name: "merge features disabled by CLI flag overrides config", + args: []string{"-merge-features=false", "-config=testdata/e2e/merge-on.jsonc", "testdata/e2e/merge"}, + want: nil, + wantExitCode: 0, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/cmd/decolint/opts.go b/cmd/decolint/opts.go index e599676..68db835 100644 --- a/cmd/decolint/opts.go +++ b/cmd/decolint/opts.go @@ -25,8 +25,13 @@ type Options struct { Platforms []linter.Platform // MergeFeatures, when set, fetches the Features referenced in each devcontainer.json and lints // the merged (effective) configuration instead of the raw file. The config file's - // "mergeFeatures" member can enable it as well (see mergeConfig). + // "mergeFeatures" member sets it as well, but -merge-features takes precedence over it, in + // either direction, when explicitly given (see mergeFeaturesSet and mergeConfig). MergeFeatures bool + // mergeFeaturesSet records whether -merge-features was explicitly passed, distinguishing that + // from its default false value so mergeConfig can tell "not given" (defer to the config file) + // apart from "explicitly given as false" (override the config file's "mergeFeatures": true). + mergeFeaturesSet bool // Format selects how lint issues are written to stdout. Format Format // Version, when set, causes the program to print its version and exit. @@ -50,7 +55,7 @@ func parseOptions(args []string, output io.Writer) (Options, error) { fs.StringVar(&opts.ConfigPath, "config", "", "path to a config file (default: auto-discover .decolint.jsonc or .decolint.json in the current directory)") fs.StringVar(&platformFlag, "platform", "", "comma-separated target platforms to include in addition to \"all\" (vscode, codespaces); overrides the config file's \"platforms\" member") fs.StringVar(&formatFlag, "format", "text", "output format: text, json, or github") - fs.BoolVar(&opts.MergeFeatures, "merge-features", false, "fetch the Features referenced in \"features\" and lint the merged (effective) configuration") + fs.BoolVar(&opts.MergeFeatures, "merge-features", false, "fetch the Features referenced in \"features\" and lint the merged (effective) configuration; overrides the config file's \"mergeFeatures\" member") fs.BoolVar(&opts.Version, "version", false, "print version information and exit") fs.BoolVar(&opts.ListRules, "rules", false, "print the built-in rules as a Markdown table (category, target platforms, current severity), then exit") fs.BoolVar(&opts.Init, "init", false, "write a new .decolint.jsonc config file listing every rule at its default severity, then exit") @@ -58,6 +63,11 @@ func parseOptions(args []string, output io.Writer) (Options, error) { if err := fs.Parse(args); err != nil { return Options{}, err } + fs.Visit(func(f *flag.Flag) { + if f.Name == "merge-features" { + opts.mergeFeaturesSet = true + } + }) platforms, err := parsePlatforms(platformFlag) if err != nil { diff --git a/cmd/decolint/opts_test.go b/cmd/decolint/opts_test.go index f7f3c82..2f21569 100644 --- a/cmd/decolint/opts_test.go +++ b/cmd/decolint/opts_test.go @@ -161,13 +161,16 @@ func TestParseOptionsMergeFeatures(t *testing.T) { t.Parallel() tests := []struct { - name string - args []string - want bool + name string + args []string + want bool + wantSet bool }{ - {"no flag", nil, false}, - {"single dash", []string{"-merge-features"}, true}, - {"double dash", []string{"--merge-features"}, true}, + {"no flag", nil, false, false}, + {"single dash", []string{"-merge-features"}, true, true}, + {"double dash", []string{"--merge-features"}, true, true}, + {"explicit true", []string{"-merge-features=true"}, true, true}, + {"explicit false", []string{"-merge-features=false"}, false, true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -179,6 +182,9 @@ func TestParseOptionsMergeFeatures(t *testing.T) { if opts.MergeFeatures != tt.want { t.Errorf("MergeFeatures = %v, want %v", opts.MergeFeatures, tt.want) } + if opts.mergeFeaturesSet != tt.wantSet { + t.Errorf("mergeFeaturesSet = %v, want %v", opts.mergeFeaturesSet, tt.wantSet) + } }) } } From 1d7ae744375a4614b9552751d888b8433b636369 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 13 Jul 2026 00:13:16 +0000 Subject: [PATCH 3/4] docs: consolidate CLI-over-config precedence note in README Move the "-merge-features overrides mergeFeatures" and "-platform overrides platforms" precedence notes out of their per-flag sections and into a single general statement in the Config file section, instead of repeating the rule for each flag. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01UrZdqcow5W3yq4uGMMPS8D --- README.md | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index a389bda..9bed4fd 100644 --- a/README.md +++ b/README.md @@ -89,8 +89,7 @@ decolint -platform=vscode,codespaces ``` Target platforms can also be declared in the [config -file](#config-file) with the `platforms` member; the `-platform` flag, -when given, takes precedence. +file](#config-file) with the `platforms` member. ### Merging Features @@ -113,12 +112,6 @@ properties it contributes, and lint the merged configuration instead: decolint -merge-features -config .decolint.jsonc ``` -When given explicitly, `-merge-features` (in either form, -`-merge-features` or `-merge-features=false`) overrides the config -file's `mergeFeatures` member; for example, `-merge-features=false` -disables merging even if the config file sets `"mergeFeatures": -true`. Leaving the flag unset defers to the config file. - - OCI references (e.g. `ghcr.io/devcontainers/features/node:1`) are pulled from the registry with anonymous access, direct HTTP(S) tarball URIs are downloaded, and relative paths (e.g. @@ -198,11 +191,15 @@ to edit: rule in a [category](#rule-categories) at once; `rules` sets an individual rule's severity and takes precedence over its category. `platforms` lists the [target platforms](#target-platforms) whose -rules run in addition to platform-agnostic ones; the `-platform` flag, -when given, takes precedence over it. `mergeFeatures` set to `true` -enables [merging Features](#merging-features), same as the -`-merge-features` flag; when given explicitly, `-merge-features` -takes precedence over `mergeFeatures` in either direction. +rules run in addition to platform-agnostic ones. `mergeFeatures` set +to `true` enables [merging Features](#merging-features), same as the +`-merge-features` flag. + +For a config file member with a corresponding flag (`platforms` / +`-platform`, `mergeFeatures` / `-merge-features`), the flag, when +given explicitly, takes precedence over the config file in either +direction — e.g. `-merge-features=false` disables merging even if the +config file sets `"mergeFeatures": true`. For the strictest configuration, enable every category: From f05ded7ff60d82b2cadf87dcf604ddbfb87a1421 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 14 Jul 2026 22:58:14 +0000 Subject: [PATCH 4/4] feat(feature): confine local Feature resolution through os.Root #16 routed the main devcontainer.json/Feature/Template file discovery through os.Root to prevent path traversal, but local Feature references resolved during -merge-features (a relative path under "features") still went through plain os.ReadFile/os.Stat, bypassing that protection. Thread the same os.Root boundary through: linter.Context gains Dir (the os.Root LintDir already confines the file to) and FileDir (the file's own directory relative to it), populated by lintConfig and left zero when Lint is called directly on in-memory content (as existing tests do). feature.Fetch/Merge now take that (dir, fileDir) pair instead of a bare baseDir string, and resolve local references by joining fileDir with the reference and reading it through dir, so a reference cannot escape the same boundary the referencing devcontainer.json was itself read through. Note this changes local Feature semantics slightly: a Feature stored outside the confining directory (e.g. a project-root sibling of .devcontainer, referenced from .devcontainer/devcontainer.json via "../") no longer resolves, even though it did before #16. This mirrors the boundary #16 already established for the file discovery its own comment anticipated extending to Feature resolution. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01UrZdqcow5W3yq4uGMMPS8D --- README.md | 14 ++++++++++- cmd/decolint/main.go | 3 +-- cmd/decolint/main_test.go | 23 ++++++++++++++++++ feature/fetch.go | 31 ++++++++++++++----------- feature/fetch_test.go | 49 +++++++++++++++++++++++++++++---------- feature/merge.go | 19 +++++++++------ feature/merge_test.go | 6 ++--- linter/linter.go | 14 ++++++++--- linter/rule.go | 9 +++++++ 9 files changed, 126 insertions(+), 42 deletions(-) diff --git a/README.md b/README.md index 9bed4fd..2aed0d4 100644 --- a/README.md +++ b/README.md @@ -63,6 +63,17 @@ layout, and the configuration files it contains are linted: With no arguments, the current directory is linted. +### Path handling + +All file access for a linted directory is confined to it: a +`devcontainer.json` under `.devcontainer` is only read from within +that directory, and a symbolic link resolving outside the boundary +its config file was read through (see above) is treated as +nonexistent rather than followed. The same boundary applies to local +Feature references resolved while [merging +Features](#merging-features) — a reference that would resolve outside +it is an error, even if the target exists elsewhere on disk. + decolint supports the following flags; run `decolint -help` for the full list. @@ -115,7 +126,8 @@ decolint -merge-features -config .decolint.jsonc - OCI references (e.g. `ghcr.io/devcontainers/features/node:1`) are pulled from the registry with anonymous access, direct HTTP(S) tarball URIs are downloaded, and relative paths (e.g. - `./my-feature`) are read from disk. + `./my-feature`) are read from disk (see [Path + handling](#path-handling)). - Features referenced by a Feature's `dependsOn` are resolved recursively and contribute their properties as well; installation order follows `dependsOn`, `installsAfter`, and diff --git a/cmd/decolint/main.go b/cmd/decolint/main.go index 9509fe1..951976b 100644 --- a/cmd/decolint/main.go +++ b/cmd/decolint/main.go @@ -9,7 +9,6 @@ import ( "io" "os" "os/signal" - "path/filepath" "strings" "syscall" @@ -241,7 +240,7 @@ func runLint(ctx context.Context, stdout io.Writer, opts Options, cfg Config) (b if fctx.Type != linter.Devcontainer { return nil } - return feature.Merge(ctx, fetcher, filepath.Dir(fctx.Path), fctx.Root) + return feature.Merge(ctx, fetcher, fctx.Dir, fctx.FileDir, fctx.Root) }) } diff --git a/cmd/decolint/main_test.go b/cmd/decolint/main_test.go index 8a8ee27..6afc16d 100644 --- a/cmd/decolint/main_test.go +++ b/cmd/decolint/main_test.go @@ -605,6 +605,29 @@ func TestRunMergeFeatures(t *testing.T) { t.Errorf("stderr = %q, want it to mention the unresolvable feature", stderr.String()) } }) + + t.Run("local feature escaping .devcontainer is a runtime error", func(t *testing.T) { + t.Parallel() + dir := writeDevcontainer(t, `{"image": "ubuntu:24.04", "features": {"../sibling-feature": {}}}`) + // sibling-feature exists on disk, but as a project-root sibling of .devcontainer, not inside + // it, so it is outside the boundary local Feature references are confined to. + if err := os.MkdirAll(filepath.Join(dir, "sibling-feature"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "sibling-feature", "devcontainer-feature.json"), + []byte(`{"id": "sibling-feature"}`), 0o644); err != nil { + t.Fatal(err) + } + + var stdout, stderr bytes.Buffer + exitCode := run(t.Context(), []string{"-merge-features", dir}, &stdout, &stderr) + if exitCode != 2 { + t.Errorf("exit code = %d, want 2; stdout: %s", exitCode, stdout.String()) + } + if !strings.Contains(stderr.String(), "../sibling-feature") { + t.Errorf("stderr = %q, want it to mention the unresolvable feature", stderr.String()) + } + }) } // mdTableRow finds the row of a Markdown table in out whose first cell, after trimming the padding diff --git a/feature/fetch.go b/feature/fetch.go index acc869e..fa80410 100644 --- a/feature/fetch.go +++ b/feature/fetch.go @@ -45,10 +45,12 @@ func NewFetcher() *Fetcher { } } -// Fetch retrieves the metadata of the Feature referenced by raw. baseDir is the directory -// containing the devcontainer.json that references the Feature; local references are resolved -// relative to it. -func (f *Fetcher) Fetch(ctx context.Context, raw string, baseDir string) (*Metadata, error) { +// Fetch retrieves the metadata of the Feature referenced by raw. dir and fileDir together locate +// the devcontainer.json that references the Feature (see linter.Context.Dir and +// linter.Context.FileDir): a local reference is resolved by joining fileDir with it and reading the +// result through dir, so the resolution cannot escape dir's boundary. dir and fileDir are unused +// for an OCI or tarball reference. +func (f *Fetcher) Fetch(ctx context.Context, raw string, dir *os.Root, fileDir string) (*Metadata, error) { ref, err := ParseRef(raw) if err != nil { return nil, err @@ -57,15 +59,15 @@ func (f *Fetcher) Fetch(ctx context.Context, raw string, baseDir string) (*Metad key := raw if ref.Kind == KindLocal { // The same relative reference can name different directories depending on the referencing - // file's location. - key = "local:" + filepath.Join(baseDir, raw) + // file's location and the root it is confined to. + key = fmt.Sprintf("local:%p:%s", dir, filepath.Join(fileDir, raw)) } f.mu.Lock() res, ok := f.cache[key] f.mu.Unlock() if !ok { - res.md, res.err = f.fetch(ctx, ref, baseDir) + res.md, res.err = f.fetch(ctx, ref, dir, fileDir) if res.err != nil { res.err = fmt.Errorf("fetch feature %q: %w", raw, res.err) } @@ -76,10 +78,10 @@ func (f *Fetcher) Fetch(ctx context.Context, raw string, baseDir string) (*Metad return res.md, res.err } -func (f *Fetcher) fetch(ctx context.Context, ref Ref, baseDir string) (*Metadata, error) { +func (f *Fetcher) fetch(ctx context.Context, ref Ref, dir *os.Root, fileDir string) (*Metadata, error) { switch ref.Kind { case KindLocal: - return fetchLocal(filepath.Join(baseDir, ref.Raw)) + return fetchLocal(dir, filepath.Join(fileDir, ref.Raw)) case KindTarball: return f.fetchTarball(ctx, ref.Raw) default: @@ -87,17 +89,18 @@ func (f *Fetcher) fetch(ctx context.Context, ref Ref, baseDir string) (*Metadata } } -// fetchLocal reads the metadata of the Feature in directory dir. -func fetchLocal(dir string) (*Metadata, error) { - path := filepath.Join(dir, metadataFileName) - info, err := os.Stat(path) +// fetchLocal reads the metadata of the Feature at featureDir, read through dir so its resolution +// cannot escape dir's boundary. +func fetchLocal(dir *os.Root, featureDir string) (*Metadata, error) { + path := filepath.Join(featureDir, metadataFileName) + info, err := dir.Stat(path) if err != nil { return nil, err } if info.Size() > maxMetadataBytes { return nil, fmt.Errorf("%s exceeds %d bytes", path, maxMetadataBytes) } - src, err := os.ReadFile(path) + src, err := dir.ReadFile(path) if err != nil { return nil, err } diff --git a/feature/fetch_test.go b/feature/fetch_test.go index 6430442..b850693 100644 --- a/feature/fetch_test.go +++ b/feature/fetch_test.go @@ -13,8 +13,7 @@ import ( "testing" ) -// writeLocalFeature creates dir//devcontainer-feature.json with the given content and -// returns the base directory. +// writeLocalFeature creates dir//devcontainer-feature.json with the given content. func writeLocalFeature(t *testing.T, dir, name, src string) { t.Helper() featureDir := filepath.Join(dir, name) @@ -26,6 +25,17 @@ func writeLocalFeature(t *testing.T, dir, name, src string) { } } +// openRoot opens dir as an os.Root, closed when the test ends, to fetch local Features 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 TestFetchLocal(t *testing.T) { t.Parallel() @@ -33,7 +43,7 @@ func TestFetchLocal(t *testing.T) { writeLocalFeature(t, dir, "my-feature", `{"id": "my-feature", "version": "1.0.0"}`) f := NewFetcher() - md, err := f.Fetch(t.Context(), "./my-feature", dir) + md, err := f.Fetch(t.Context(), "./my-feature", openRoot(t, dir), ".") if err != nil { t.Fatalf("Fetch: %v", err) } @@ -42,11 +52,25 @@ func TestFetchLocal(t *testing.T) { } } +func TestFetchLocalEscapingRootIsRejected(t *testing.T) { + t.Parallel() + + // A Feature outside the confining root (here, dir's parent) must not be reachable via "..", + // even though it exists on disk. + dir := t.TempDir() + writeLocalFeature(t, filepath.Dir(dir), "escaped-feature", `{"id": "escaped-feature"}`) + + f := NewFetcher() + if _, err := f.Fetch(t.Context(), "../escaped-feature", openRoot(t, dir), "."); err == nil { + t.Error("Fetch of a feature escaping the root: got nil error") + } +} + func TestFetchLocalMissing(t *testing.T) { t.Parallel() f := NewFetcher() - if _, err := f.Fetch(t.Context(), "./nope", t.TempDir()); err == nil { + if _, err := f.Fetch(t.Context(), "./nope", openRoot(t, t.TempDir()), "."); err == nil { t.Error("Fetch of a missing local feature: got nil error") } } @@ -56,9 +80,10 @@ func TestFetchCachesResults(t *testing.T) { dir := t.TempDir() writeLocalFeature(t, dir, "f", `{"id": "before"}`) + root := openRoot(t, dir) f := NewFetcher() - md, err := f.Fetch(t.Context(), "./f", dir) + md, err := f.Fetch(t.Context(), "./f", root, ".") if err != nil { t.Fatalf("Fetch: %v", err) } @@ -68,7 +93,7 @@ func TestFetchCachesResults(t *testing.T) { // A second fetch must hit the cache and not observe the changed file. writeLocalFeature(t, dir, "f", `{"id": "after"}`) - md, err = f.Fetch(t.Context(), "./f", dir) + md, err = f.Fetch(t.Context(), "./f", root, ".") if err != nil { t.Fatalf("Fetch (cached): %v", err) } @@ -88,7 +113,7 @@ func TestFetchMetadataParse(t *testing.T) { "installsAfter": ["ghcr.io/devcontainers/features/common-utils"], }`) - md, err := NewFetcher().Fetch(t.Context(), "./f", dir) + md, err := NewFetcher().Fetch(t.Context(), "./f", openRoot(t, dir), ".") if err != nil { t.Fatalf("Fetch: %v", err) } @@ -148,7 +173,7 @@ func TestFetchTarball(t *testing.T) { })) defer srv.Close() - md, err := NewFetcher().Fetch(t.Context(), srv.URL+"/feature.tgz", "") + md, err := NewFetcher().Fetch(t.Context(), srv.URL+"/feature.tgz", nil, "") if err != nil { t.Fatalf("Fetch: %v", err) } @@ -163,7 +188,7 @@ func TestFetchTarballNotFound(t *testing.T) { srv := httptest.NewServer(http.NotFoundHandler()) defer srv.Close() - if _, err := NewFetcher().Fetch(t.Context(), srv.URL+"/feature.tgz", ""); err == nil { + if _, err := NewFetcher().Fetch(t.Context(), srv.URL+"/feature.tgz", nil, ""); err == nil { t.Error("Fetch of a missing tarball: got nil error") } } @@ -245,7 +270,7 @@ func TestFetchOCI(t *testing.T) { } host := startRegistry(t, fr) - md, err := NewFetcher().Fetch(t.Context(), host+"/devcontainers/features/node:1", "") + md, err := NewFetcher().Fetch(t.Context(), host+"/devcontainers/features/node:1", nil, "") if err != nil { t.Fatalf("Fetch: %v", err) } @@ -267,7 +292,7 @@ func TestFetchOCIThroughIndex(t *testing.T) { } host := startRegistry(t, fr) - md, err := NewFetcher().Fetch(t.Context(), host+"/devcontainers/features/go:1", "") + md, err := NewFetcher().Fetch(t.Context(), host+"/devcontainers/features/go:1", nil, "") if err != nil { t.Fatalf("Fetch: %v", err) } @@ -287,7 +312,7 @@ func TestFetchOCIUnknownRepository(t *testing.T) { } host := startRegistry(t, fr) - if _, err := NewFetcher().Fetch(t.Context(), host+"/devcontainers/features/nope:1", ""); err == nil { + if _, err := NewFetcher().Fetch(t.Context(), host+"/devcontainers/features/nope:1", nil, ""); err == nil { t.Error("Fetch of an unknown repository: got nil error") } } diff --git a/feature/merge.go b/feature/merge.go index 1d92cce..c85aab5 100644 --- a/feature/merge.go +++ b/feature/merge.go @@ -4,20 +4,25 @@ import ( "context" "fmt" "math" + "os" "strings" "github.com/tailscale/hujson" ) // Merge fetches the Features referenced under "/features" of root, a devcontainer.json parsed from -// a file in directory dir, and merges the properties they contribute into root in place, following -// the merge logic of the Dev Container specification. Features named by "dependsOn" are resolved -// recursively and contribute properties as well. +// a file at fileDir within dir, and merges the properties they contribute into root in place, +// following the merge logic of the Dev Container specification. Features named by "dependsOn" are +// resolved recursively and contribute properties as well. +// +// dir and fileDir together locate the referencing devcontainer.json (see linter.Context.Dir and +// linter.Context.FileDir): a local Feature reference is resolved relative to fileDir and read +// through dir, so it cannot escape dir's boundary. // // Every node Merge adds to the tree carries the byte offset of the referencing Feature key in the // original file, so findings on merged-in properties point at the Feature reference. Any fetch or // parse failure is returned as an error. -func Merge(ctx context.Context, f *Fetcher, dir string, root *hujson.Value) error { +func Merge(ctx context.Context, f *Fetcher, dir *os.Root, fileDir string, root *hujson.Value) error { features := root.Find("/features") if features == nil { return nil @@ -36,7 +41,7 @@ func Merge(ctx context.Context, f *Fetcher, dir string, root *hujson.Value) erro declared = append(declared, &contributor{ref: name.String(), anchor: m.Name.StartOffset}) } - contribs, err := resolveAll(ctx, f, dir, declared) + contribs, err := resolveAll(ctx, f, dir, fileDir, declared) if err != nil { return err } @@ -92,7 +97,7 @@ func refWithoutVersion(ref string) string { // resolveAll fetches every declared Feature and, recursively, the Features they depend on. The // result is in discovery order (dependencies before their dependents), deduplicated by reference. -func resolveAll(ctx context.Context, f *Fetcher, dir string, declared []*contributor) ([]*contributor, error) { +func resolveAll(ctx context.Context, f *Fetcher, dir *os.Root, fileDir string, declared []*contributor) ([]*contributor, error) { seen := map[string]*contributor{} var out []*contributor @@ -106,7 +111,7 @@ func resolveAll(ctx context.Context, f *Fetcher, dir string, declared []*contrib if _, ok := seen[c.ref]; ok { return nil } - md, err := f.Fetch(ctx, c.ref, dir) + md, err := f.Fetch(ctx, c.ref, dir, fileDir) if err != nil { return err } diff --git a/feature/merge_test.go b/feature/merge_test.go index e89fe87..09620b9 100644 --- a/feature/merge_test.go +++ b/feature/merge_test.go @@ -22,7 +22,7 @@ func mergeSrc(t *testing.T, src string, features map[string]string) *hujson.Valu if err != nil { t.Fatalf("parse devcontainer.json: %v", err) } - if err := Merge(t.Context(), NewFetcher(), dir, &root); err != nil { + if err := Merge(t.Context(), NewFetcher(), openRoot(t, dir), ".", &root); err != nil { t.Fatalf("Merge: %v", err) } return &root @@ -225,7 +225,7 @@ func TestMergeDependsOnCycle(t *testing.T) { if err != nil { t.Fatal(err) } - if err := Merge(t.Context(), NewFetcher(), dir, &root); err == nil || !strings.Contains(err.Error(), "cycle") { + if err := Merge(t.Context(), NewFetcher(), openRoot(t, dir), ".", &root); err == nil || !strings.Contains(err.Error(), "cycle") { t.Errorf("Merge with a dependency cycle: err = %v, want a cycle error", err) } } @@ -237,7 +237,7 @@ func TestMergeFetchFailure(t *testing.T) { if err != nil { t.Fatal(err) } - if err := Merge(t.Context(), NewFetcher(), t.TempDir(), &root); err == nil { + if err := Merge(t.Context(), NewFetcher(), openRoot(t, t.TempDir()), ".", &root); err == nil { t.Error("Merge with an unresolvable feature: got nil error") } } diff --git a/linter/linter.go b/linter/linter.go index 29f04fb..b3762c4 100644 --- a/linter/linter.go +++ b/linter/linter.go @@ -119,19 +119,27 @@ func (l *Linter) LintDir(ctx context.Context, root *os.Root) ([]Issue, error) { // 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. +// boundary; the same f.root, and f's own directory relative to it, are passed through as the +// resulting Context's Dir and FileDir, so a Transform resolving a path relative to the file cannot +// escape that boundary either. 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) + return l.lintWithDir(ctx, display, src, f.typ, f.root, filepath.Dir(f.path)) } // 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) { + return l.lintWithDir(ctx, path, src, fileType, nil, "") +} + +// lintWithDir is Lint, additionally attaching dir and fileDir to the Context passed to the +// transform and rules; see Context.Dir and Context.FileDir. +func (l *Linter) lintWithDir(ctx context.Context, path string, src []byte, fileType FileType, dir *os.Root, fileDir string) ([]Issue, error) { if err := ctx.Err(); err != nil { return nil, fmt.Errorf("aborted %s: %w", path, err) } @@ -143,7 +151,7 @@ func (l *Linter) Lint(ctx context.Context, path string, src []byte, fileType Fil if len(patterns) == 0 { return nil, nil } - rctx := &Context{Path: path, Type: fileType, Src: src, Root: &root} + rctx := &Context{Path: path, Type: fileType, Src: src, Root: &root, Dir: dir, FileDir: fileDir} if l.transform != nil { if err := l.transform(ctx, rctx); err != nil { return nil, fmt.Errorf("transform %s: %w", path, err) diff --git a/linter/rule.go b/linter/rule.go index b21aa79..fcbaf5b 100644 --- a/linter/rule.go +++ b/linter/rule.go @@ -3,6 +3,7 @@ package linter import ( "encoding/json/jsontext" "fmt" + "os" "strings" "github.com/tailscale/hujson" @@ -142,6 +143,14 @@ type Context struct { Src []byte // Root is the HuJSON syntax tree parsed from Src. It preserves comments and byte offsets into Src. Root *hujson.Value + // Dir is an os.Root confined to the same boundary LintDir enforces for this file (the lint root, + // or its .devcontainer sub-root). FileDir is this file's own directory, relative to Dir. A + // Transform that resolves a path relative to the file (e.g. a local Feature reference) should + // join FileDir with that relative path and access the result through Dir, so the resolution + // cannot escape the same boundary the file itself was read through. Both are the zero value when + // Lint is called directly on in-memory content with no real directory backing it (e.g. in tests). + Dir *os.Root + FileDir string } // Severity indicates how a finding should be treated: whether it's reported as an error or a