From 1830a02b72ee75b5614f79f3b04456c1a4d48906 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 07:51:19 +0000 Subject: [PATCH] fix(rules): read mounts as docker/cli and moby do no-docker-socket-mount and no-bind-mount read "mounts" entries and the "runArgs" mount flags with hand-rolled parsers that neither matched docker's syntax nor kept the two flag syntaxes apart, so they missed real Docker socket mounts and reported ones that never happen. A "--mount" value is now read the way docker/cli's opts.MountOpt.Set does: the whole value is trimmed and read as one CSV record, keys are matched case-insensitively with "src" as an alias for "source", and the type is lower-cased. Object entries get the same type folding, since devcontainers/cli hands them to docker as a "--mount" value. A "-v"/"--volume" value has no syntax in common with that, so it gets its own reader: colon-separated fields where a comma is an ordinary character, and a single-field value is an anonymous volume that binds nothing from the host. Which syntax applies depends on the flag introducing the value, which an entry does not carry on its own, so the rule now inspects the whole "runArgs" array. Whether a source is the socket is now one predicate, so the rule that reports it and the rule that excuses it cannot disagree. It cleans the path first, as the daemon does before mounting, so "//var/run/docker.sock" and "/var/run/docker.sock/" are recognized too. --- rules/no_bind_mount.go | 2 +- rules/no_bind_mount_test.go | 18 ++++ rules/no_docker_socket_mount.go | 60 +++++++------- rules/no_docker_socket_mount_test.go | 45 ++++++++++ rules/util.go | 119 ++++++++++++++++++++------- 5 files changed, 184 insertions(+), 60 deletions(-) diff --git a/rules/no_bind_mount.go b/rules/no_bind_mount.go index a6864e8..0f08751 100644 --- a/rules/no_bind_mount.go +++ b/rules/no_bind_mount.go @@ -59,7 +59,7 @@ without an error and the container starts missing the data it expects. Volume mo func checkNoBindMount(_ *linter.Context, node *linter.Node) []linter.Finding { mountType, source, ok := parseMount(node.Value) - if !ok || mountType != "bind" || source == dockerSocketPath { + if !ok || mountType != "bind" || isDockerSocketSource(source) { return nil } return []linter.Finding{{ diff --git a/rules/no_bind_mount_test.go b/rules/no_bind_mount_test.go index 47dd0b9..27d14ab 100644 --- a/rules/no_bind_mount_test.go +++ b/rules/no_bind_mount_test.go @@ -26,7 +26,20 @@ func TestNoBindMount(t *testing.T) { {Path: "devcontainer.json", Line: 1, Col: 13, RuleID: "no-bind-mount", Message: `"mounts" entry uses the "bind" type, which GitHub Codespaces silently ignores`}, }}, + {"string bind mount with upper-case keys and type", `{"mounts": ["Type=Bind,Source=/host/d,dst=/x"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 13, RuleID: "no-bind-mount", + Message: `"mounts" entry uses the "bind" type, which GitHub Codespaces silently ignores`}, + }}, + {"string bind mount with a quoted field", `{"mounts": ["type=bind,\"src=/host/d\",dst=/x"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 13, RuleID: "no-bind-mount", + Message: `"mounts" entry uses the "bind" type, which GitHub Codespaces silently ignores`}, + }}, + {"string mount whose fields are not a CSV record", `{"mounts": ["type=bind,src=\"/host/d\",dst=/x"]}`, nil}, {"object volume mount", `{"mounts": [{"source": "vol", "target": "/data", "type": "volume"}]}`, nil}, + {"object bind mount with upper-case type", `{"mounts": [{"source": "/host/d", "target": "/x", "type": "BIND"}]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 13, RuleID: "no-bind-mount", + Message: `"mounts" entry uses the "bind" type, which GitHub Codespaces silently ignores`}, + }}, {"object mount with no type", `{"mounts": [{"source": "vol", "target": "/data"}]}`, nil}, {"object bind mount", `{"mounts": [{"source": "/host", "target": "/data", "type": "bind"}]}`, []linter.Issue{ {Path: "devcontainer.json", Line: 1, Col: 13, RuleID: "no-bind-mount", @@ -39,6 +52,11 @@ func TestNoBindMount(t *testing.T) { {"string docker socket bind mount", `{"mounts": ["source=/var/run/docker.sock,target=/var/run/docker.sock,type=bind"]}`, nil}, {"string docker socket bind mount with docker-host.sock target", `{"mounts": ["source=/var/run/docker.sock,target=/var/run/docker-host.sock,type=bind"]}`, nil}, {"object docker socket bind mount", `{"mounts": [{"source": "/var/run/docker.sock", "target": "/var/run/docker.sock", "type": "bind"}]}`, nil}, + // The socket exemption must recognize every spelling no-docker-socket-mount reports, or the two + // rules report the same mount with contradictory findings. + {"string docker socket bind mount via src alias", `{"mounts": ["type=bind,src=/var/run/docker.sock,dst=/x"]}`, nil}, + {"string docker socket bind mount with a doubled leading slash", `{"mounts": ["type=bind,src=//var/run/docker.sock,dst=/x"]}`, nil}, + {"string docker socket bind mount with a trailing slash", `{"mounts": ["type=bind,src=/var/run/docker.sock/,dst=/x"]}`, nil}, {"bind mount with unrelated source named docker.sock", `{"mounts": ["source=/host/docker.sock,target=/var/run/docker.sock,type=bind"]}`, []linter.Issue{ {Path: "devcontainer.json", Line: 1, Col: 13, RuleID: "no-bind-mount", Message: `"mounts" entry uses the "bind" type, which GitHub Codespaces silently ignores`}, diff --git a/rules/no_docker_socket_mount.go b/rules/no_docker_socket_mount.go index 0ffa679..cdfba49 100644 --- a/rules/no_docker_socket_mount.go +++ b/rules/no_docker_socket_mount.go @@ -1,8 +1,6 @@ package rules import ( - "strings" - "github.com/bare-devcontainer/decolint/linter" "github.com/tailscale/hujson" ) @@ -25,7 +23,7 @@ rootless daemon keeps that access inside the container.`, }, Category: linter.CategorySecurity, FileTypes: []linter.FileType{linter.Devcontainer}, - Paths: []string{"/mounts/*", "/runArgs/*"}, + Paths: []string{"/mounts/*", "/runArgs"}, Example: linter.Example{ Bad: linter.Snippet{ Files: []linter.ExampleFile{ @@ -58,15 +56,15 @@ rootless daemon keeps that access inside the container.`, } func checkNoDockerSocketMount(_ *linter.Context, node *linter.Node) []linter.Finding { - if strings.HasPrefix(node.Pointer, "/mounts/") { - return checkDockerSocketMount(node) + if node.Pointer == "/runArgs" { + return checkDockerSocketRunArgs(node) } - return checkDockerSocketRunArg(node) + return checkDockerSocketMount(node) } func checkDockerSocketMount(node *linter.Node) []linter.Finding { _, source, ok := parseMount(node.Value) - if !ok || source != dockerSocketPath { + if !ok || !isDockerSocketSource(source) { return nil } return []linter.Finding{{ @@ -75,29 +73,35 @@ func checkDockerSocketMount(node *linter.Node) []linter.Finding { }} } -func checkDockerSocketRunArg(node *linter.Node) []linter.Finding { - lit, ok := node.Value.Value.(hujson.Literal) - if !ok || lit.Kind() != '"' || !runArgMountsDockerSocket(lit.String()) { - return nil - } - return []linter.Finding{{ - Message: `"runArgs" bind-mounts the Docker socket, which grants the container root-equivalent control over the host`, - Offset: node.Value.StartOffset, - }} +// dockerSocketRunArgFlags are the "runArgs" flags that can mount a host path, each paired with the +// reader for its own value syntax. The two syntaxes are unrelated, so a value must be read only as +// the flag introducing it, which is why this rule inspects the whole "runArgs" array rather than its +// entries one by one. +var dockerSocketRunArgFlags = []struct { + flag string + source func(string) string +}{ + {"--mount", func(s string) string { _, source := parseMountString(s); return source }}, + {"--volume", volumeSpecSource}, + {"-v", volumeSpecSource}, } -// runArgMountsDockerSocket reports whether s, a single "runArgs" entry, bind-mounts the Docker -// socket. It recognizes a "--mount"-style "key=value,..." entry with a matching "source", and a -// "-v"/"--volume" entry, with or without an "=" before the value, whose host path is the Docker -// socket. -func runArgMountsDockerSocket(s string) bool { - if strings.Contains(s, ",") { - if _, source := parseMountString(strings.TrimPrefix(s, "--mount=")); source == dockerSocketPath { - return true - } +func checkDockerSocketRunArgs(node *linter.Node) []linter.Finding { + arr, ok := node.Value.Value.(*hujson.Array) + if !ok { + return nil } - for _, prefix := range []string{"--volume=", "-v="} { - s = strings.TrimPrefix(s, prefix) + var findings []linter.Finding + for _, f := range dockerSocketRunArgFlags { + for value, s := range runArgsFlagValues(arr, f.flag) { + if !isDockerSocketSource(f.source(s)) { + continue + } + findings = append(findings, linter.Finding{ + Message: `"runArgs" bind-mounts the Docker socket, which grants the container root-equivalent control over the host`, + Offset: value.StartOffset, + }) + } } - return s == dockerSocketPath || strings.HasPrefix(s, dockerSocketPath+":") + return findings } diff --git a/rules/no_docker_socket_mount_test.go b/rules/no_docker_socket_mount_test.go index 2c7dcff..4c9a823 100644 --- a/rules/no_docker_socket_mount_test.go +++ b/rules/no_docker_socket_mount_test.go @@ -31,7 +31,28 @@ func TestNoDockerSocketMount(t *testing.T) { {Path: "devcontainer.json", Line: 1, Col: 13, RuleID: "no-docker-socket-mount", Message: `"mounts" entry bind-mounts the Docker socket, which grants the container root-equivalent control over the host`}, }}, + {"string docker socket mount via src alias", `{"mounts": ["type=bind,src=/var/run/docker.sock,dst=/x"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 13, RuleID: "no-docker-socket-mount", + Message: `"mounts" entry bind-mounts the Docker socket, which grants the container root-equivalent control over the host`}, + }}, + {"string docker socket mount with upper-case keys", `{"mounts": ["Type=Bind,SRC=/var/run/docker.sock,DST=/x"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 13, RuleID: "no-docker-socket-mount", + Message: `"mounts" entry bind-mounts the Docker socket, which grants the container root-equivalent control over the host`}, + }}, + {"string docker socket mount with a quoted field", `{"mounts": ["type=bind,\"src=/var/run/docker.sock\",dst=/x"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 13, RuleID: "no-docker-socket-mount", + Message: `"mounts" entry bind-mounts the Docker socket, which grants the container root-equivalent control over the host`}, + }}, + {"string docker socket mount with leading whitespace", `{"mounts": [" \"src=/var/run/docker.sock\",dst=/x,type=bind"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 13, RuleID: "no-docker-socket-mount", + Message: `"mounts" entry bind-mounts the Docker socket, which grants the container root-equivalent control over the host`}, + }}, + {"object docker socket mount with upper-case type", `{"mounts": [{"source": "/var/run/docker.sock", "target": "/x", "type": "BIND"}]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 13, RuleID: "no-docker-socket-mount", + Message: `"mounts" entry bind-mounts the Docker socket, which grants the container root-equivalent control over the host`}, + }}, {"unrelated source named docker.sock", `{"mounts": ["source=/host/docker.sock,target=/var/run/docker.sock,type=bind"]}`, nil}, + {"string mount whose fields are not a CSV record", `{"mounts": ["type=bind,src=\"/var/run/docker.sock\",dst=/x"]}`, nil}, {"non-string literal mount entry", `{"mounts": [123]}`, nil}, {"non-literal, non-object mount entry", `{"mounts": [["source=/var/run/docker.sock,target=/var/run/docker.sock,type=bind"]]}`, nil}, @@ -61,7 +82,31 @@ func TestNoDockerSocketMount(t *testing.T) { {Path: "devcontainer.json", Line: 1, Col: 14, RuleID: "no-docker-socket-mount", Message: `"runArgs" bind-mounts the Docker socket, which grants the container root-equivalent control over the host`}, }}, + {"runArgs --mount two tokens via src alias", `{"runArgs": ["--mount", "type=bind,src=/var/run/docker.sock,dst=/x"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 25, RuleID: "no-docker-socket-mount", + Message: `"runArgs" bind-mounts the Docker socket, which grants the container root-equivalent control over the host`}, + }}, + {"runArgs -v host path with a doubled leading slash", `{"runArgs": ["-v", "//var/run/docker.sock:/x"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 20, RuleID: "no-docker-socket-mount", + Message: `"runArgs" bind-mounts the Docker socket, which grants the container root-equivalent control over the host`}, + }}, + {"runArgs -v host path with a trailing slash", `{"runArgs": ["-v", "/var/run/docker.sock/:/x"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 20, RuleID: "no-docker-socket-mount", + Message: `"runArgs" bind-mounts the Docker socket, which grants the container root-equivalent control over the host`}, + }}, + {"runArgs every -v is reported", `{"runArgs": ["-v", "/var/run/docker.sock:/a", "-v", "/var/run/docker.sock:/b"]}`, []linter.Issue{ + {Path: "devcontainer.json", Line: 1, Col: 20, RuleID: "no-docker-socket-mount", + Message: `"runArgs" bind-mounts the Docker socket, which grants the container root-equivalent control over the host`}, + {Path: "devcontainer.json", Line: 1, Col: 53, RuleID: "no-docker-socket-mount", + Message: `"runArgs" bind-mounts the Docker socket, which grants the container root-equivalent control over the host`}, + }}, {"runArgs unrelated volume", `{"runArgs": ["-v", "/host/docker.sock:/var/run/docker.sock"]}`, nil}, + // A -v value of a single field is an anonymous volume, and that field is the container path: + // nothing from the host is bound. + {"runArgs -v with only a container path", `{"runArgs": ["-v", "/var/run/docker.sock"]}`, nil}, + // A -v value is colon-separated, so a comma in it is part of a field rather than a separator. + {"runArgs --volume with a comma in the container path", `{"runArgs": ["--volume", "myvol:/data,source=/var/run/docker.sock"]}`, nil}, + {"runArgs not an array", `{"runArgs": "-v /var/run/docker.sock:/x"}`, nil}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/rules/util.go b/rules/util.go index e74c092..ce3e8ea 100644 --- a/rules/util.go +++ b/rules/util.go @@ -1,6 +1,9 @@ package rules import ( + "encoding/csv" + "iter" + "path" "strings" "github.com/bare-devcontainer/decolint/linter" @@ -12,6 +15,16 @@ import ( // container even though other bind mounts are ignored. const dockerSocketPath = "/var/run/docker.sock" +// isDockerSocketSource reports whether source, a mount source, names the host's Docker socket. It is +// the single answer to that question, so that a rule reporting the socket and a rule excusing it +// never disagree about the same mount. +// +// The daemon cleans a bind mount's source before using it, so spellings such as +// "//var/run/docker.sock" and "/var/run/docker.sock/" all reach the same socket. +func isDockerSocketSource(source string) bool { + return path.Clean(source) == dockerSocketPath +} + // hasMember reports whether obj has a member named name. func hasMember(obj *hujson.Object, name string) bool { return memberNamed(obj, name) != nil @@ -65,53 +78,85 @@ func arrayMember(obj *hujson.Object, name string) (arr *hujson.Array, ok bool) { return nil, false } -// runArgsFindFlagValue scans arr, a "runArgs" array, for an entry that sets flag to a value accepted -// by match. Docker accepts such a value either as a single combined "flag=value" entry or as two -// adjacent entries, "flag" followed by "value". It returns the hujson.Value holding the matching -// value, or nil if no entry sets flag to a value match accepts. -func runArgsFindFlagValue(arr *hujson.Array, flag string, match func(string) bool) *hujson.Value { - for i := range arr.Elements { - lit, ok := arr.Elements[i].Value.(hujson.Literal) - if !ok || lit.Kind() != '"' { - continue - } +// runArgsFlagValues yields every value that arr, a "runArgs" array, gives to flag, in order. Docker +// accepts such a value either as a single combined "flag=value" entry or as two adjacent entries, +// "flag" followed by "value". Each yielded pair is the hujson.Value holding the value and the value +// itself. +func runArgsFlagValues(arr *hujson.Array, flag string) iter.Seq2[*hujson.Value, string] { + return func(yield func(*hujson.Value, string) bool) { + for i := range arr.Elements { + lit, ok := arr.Elements[i].Value.(hujson.Literal) + if !ok || lit.Kind() != '"' { + continue + } - if v, ok := strings.CutPrefix(lit.String(), flag+"="); ok { - if match(v) { - return &arr.Elements[i] + if v, ok := strings.CutPrefix(lit.String(), flag+"="); ok { + if !yield(&arr.Elements[i], v) { + return + } + continue } - continue - } - if lit.String() != flag || i+1 >= len(arr.Elements) { - continue + if lit.String() != flag || i+1 >= len(arr.Elements) { + continue + } + next, ok := arr.Elements[i+1].Value.(hujson.Literal) + if ok && next.Kind() == '"' && !yield(&arr.Elements[i+1], next.String()) { + return + } } - next, ok := arr.Elements[i+1].Value.(hujson.Literal) - if ok && next.Kind() == '"' && match(next.String()) { - return &arr.Elements[i+1] + } +} + +// runArgsFindFlagValue returns the hujson.Value holding the first value arr gives to flag that match +// accepts, or nil if it gives flag no such value. See [runArgsFlagValues] for the entry forms it +// recognizes. +func runArgsFindFlagValue(arr *hujson.Array, flag string, match func(string) bool) *hujson.Value { + for v, s := range runArgsFlagValues(arr, flag) { + if match(s) { + return v } } return nil } -// parseMountString extracts the "type" and "source" fields from s, a "key=value,..." mount entry. +// parseMountString extracts the "type" and "source" fields from s, a "--mount" value, as docker/cli +// reads it: +// - the value is trimmed of surrounding whitespace and then read as a single CSV record, so a +// field may be quoted to protect a comma inside it; +// - field keys are matched case-insensitively, and "src" is an alias for "source"; +// - the type is normalized to lower case. +// +// It returns empty strings for a value that is not a well-formed CSV record, which docker rejects +// outright. +// +// Whitespace around a key or a value is the one deliberate deviation: docker rejects it, but reading +// such a field anyway lets the rules name the mount the author meant rather than fall silent on a +// value that already fails to start the container. func parseMountString(s string) (mountType, source string) { - for part := range strings.SplitSeq(s, ",") { - key, value, ok := strings.Cut(part, "=") + fields, err := csv.NewReader(strings.NewReader(strings.TrimSpace(s))).Read() + if err != nil { + return "", "" + } + for _, field := range fields { + key, value, ok := strings.Cut(field, "=") if !ok { continue } - switch strings.TrimSpace(key) { + value = strings.TrimSpace(value) + switch strings.ToLower(strings.TrimSpace(key)) { case "type": - mountType = strings.TrimSpace(value) - case "source": - source = strings.TrimSpace(value) + mountType = strings.ToLower(value) + case "source", "src": + source = value } } return mountType, source } -// parseMountObject extracts the "type" and "source" members from obj, a "mounts" entry. +// parseMountObject extracts the "type" and "source" members from obj, a "mounts" entry. The member +// names are the ones devcontainers/cli reads off the object and so are case-sensitive, but it hands +// their values to docker as a "--mount" value, which lower-cases the type. func parseMountObject(obj *hujson.Object) (mountType, source string) { for _, m := range obj.Members { name, ok := m.Name.Value.(hujson.Literal) @@ -124,7 +169,7 @@ func parseMountObject(obj *hujson.Object) (mountType, source string) { } switch name.String() { case "type": - mountType = value.String() + mountType = strings.ToLower(value.String()) case "source": source = value.String() } @@ -132,9 +177,21 @@ func parseMountObject(obj *hujson.Object) (mountType, source string) { return mountType, source } +// volumeSpecSource returns the host path or volume name that s, a "-v"/"--volume" value, mounts, or +// "" if s names no source. A volume spec shares no syntax with a "--mount" value: its fields are +// separated by colons and a comma is an ordinary character. A spec of a single field is an anonymous +// volume, and that field is the container path, not a host path. +func volumeSpecSource(s string) string { + source, _, ok := strings.Cut(s, ":") + if !ok { + return "" + } + return source +} + // parseMount extracts the "type" and "source" fields from v, a "mounts" entry, which may be either -// the "key=value,..." string shorthand or an object with corresponding members. ok is false if v is -// neither. +// the "--mount" string shorthand (see [parseMountString]) or an object with corresponding members. +// ok is false if v is neither. func parseMount(v *hujson.Value) (mountType, source string, ok bool) { switch val := v.Value.(type) { case hujson.Literal: