fix(executor): normalize container paths before translating and read matcher files only below shared directories (GO-006)

commit: 63e200617332e3cbfc38cf2851cd2e6e6d3c5bba
author: brice · 8d ago · 2026-10-14 06:19 UTC
parent: 6c6f5f6
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

3 files · +76 -1

M internal/executor/containers_spec.go +25 -1


@@ -5,6 +5,7 @@ package executor
5	5		import (
6	6		"context"
7	7		"fmt"
	8	+	"path"
8	9		"regexp"
9	10		"strings"
10	11

@@ -96,11 +97,20 @@ func (c *container) translateToContainer(p string) string {
96	97		return p
97	98		}
98	99
99		-	// translateToHost is TranslateToHostPath (ContainerInfo.cs:230-268).
	100	+	// translateToHost is TranslateToHostPath (ContainerInfo.cs:230-268) for a path
	101	+	// the container printed. An absolute path is normalised first, so that
	102	+	// "/__w/../outside/x" is "/outside/x" and is not found below /__w: the runner
	103	+	// joins the text as it is, which would read <work>/../outside/x on the host.
	104	+	// What comes out of a mapping is the host directory and a cleaned relative
	105	+	// part, so it is always inside the host directory of that mapping. Paths that
	106	+	// match no mapping are returned as given (relative ones included).
100	107		func (c *container) translateToHost(p string) string {
101	108		if p == "" {
102	109		return p
103	110		}
	111	+	if strings.HasPrefix(p, "/") {
	112	+	p = path.Clean(p)
	113	+	}
104	114		for _, m := range c.maps {
105	115		if p == m.container {
106	116		return m.host

@@ -112,6 +122,20 @@ func (c *container) translateToHost(p string) string {
112	122		return p
113	123		}
114	124
	125	+	// insideMappings reports whether a host path (cleaned) is a mapped host
	126	+	// directory or below one: the only host files a path printed by the container
	127	+	// can stand for. A path of the container that no mount covers (/etc/passwd of
	128	+	// the image) is not a file of the host.
	129	+	func (c *container) insideMappings(host string) bool {
	130	+	host = path.Clean(host)
	131	+	for _, m := range c.maps {
	132	+	if host == m.host \|\| strings.HasPrefix(host, m.host+"/") {
	133	+	return true
	134	+	}
	135	+	}
	136	+	return false
	137	+	}
	138	+
115	139		// resolvePathForStepHost is ContainerStepHost.ResolvePathForStepHost
116	140		// (StepHost.cs:127-152): the path is translated when it starts with the host
117	141		// path of one of the mounts (a plain string prefix, as in the runner).


@@ -5,6 +5,7 @@ package executor
5		import (	5		import (
6		"context"	6		"context"
7		"fmt"	7		"fmt"
			8	+	"path"
8		"regexp"	9		"regexp"
9		"strings"	10		"strings"
10			11

@@ -96,11 +97,20 @@ func (c *container) translateToContainer(p string) string {
96		return p	97		return p
97		}	98		}
98			99
99	-	// translateToHost is TranslateToHostPath (ContainerInfo.cs:230-268).	100	+	// translateToHost is TranslateToHostPath (ContainerInfo.cs:230-268) for a path
			101	+	// the container printed. An absolute path is normalised first, so that
			102	+	// "/__w/../outside/x" is "/outside/x" and is not found below /__w: the runner
			103	+	// joins the text as it is, which would read <work>/../outside/x on the host.
			104	+	// What comes out of a mapping is the host directory and a cleaned relative
			105	+	// part, so it is always inside the host directory of that mapping. Paths that
			106	+	// match no mapping are returned as given (relative ones included).
100		func (c *container) translateToHost(p string) string {	107		func (c *container) translateToHost(p string) string {
101		if p == "" {	108		if p == "" {
102		return p	109		return p
103		}	110		}
			111	+	if strings.HasPrefix(p, "/") {
			112	+	p = path.Clean(p)
			113	+	}
104		for _, m := range c.maps {	114		for _, m := range c.maps {
105		if p == m.container {	115		if p == m.container {
106		return m.host	116		return m.host

@@ -112,6 +122,20 @@ func (c *container) translateToHost(p string) string {
112		return p	122		return p
113		}	123		}
114			124
			125	+	// insideMappings reports whether a host path (cleaned) is a mapped host
			126	+	// directory or below one: the only host files a path printed by the container
			127	+	// can stand for. A path of the container that no mount covers (/etc/passwd of
			128	+	// the image) is not a file of the host.
			129	+	func (c *container) insideMappings(host string) bool {
			130	+	host = path.Clean(host)
			131	+	for _, m := range c.maps {
			132	+	if host == m.host \|\| strings.HasPrefix(host, m.host+"/") {
			133	+	return true
			134	+	}
			135	+	}
			136	+	return false
			137	+	}
			138	+
115		// resolvePathForStepHost is ContainerStepHost.ResolvePathForStepHost	139		// resolvePathForStepHost is ContainerStepHost.ResolvePathForStepHost
116		// (StepHost.cs:127-152): the path is translated when it starts with the host	140		// (StepHost.cs:127-152): the path is translated when it starts with the host
117		// path of one of the mounts (a plain string prefix, as in the runner).	141		// path of one of the mounts (a plain string prefix, as in the runner).

M internal/executor/containers_spec_test.go +44 -0


@@ -3,6 +3,8 @@
3	3		package executor
4	4
5	5		import (
	6	+	"path"
	7	+	"path/filepath"
6	8		"strings"
7	9		"testing"
8	10

@@ -141,3 +143,45 @@ func TestServiceNamesAndContainerIDs(t *testing.T) {
141	143		}
142	144		}
143	145		}
	146	+
	147	+	func TestTranslateToHostDoesNotEscapeTheMapping(t *testing.T) {
	148	+	c := &container{maps: []pathMap{{"/h/work", "/__w"}, {"/opt/tc", "/__t"}}}
	149	+	for in, want := range map[string]string{
	150	+	"/__w/a/b": "/h/work/a/b",
	151	+	"/__w/a/../b": "/h/work/b",
	152	+	"/__w/../outside/x": "/outside/x",
	153	+	"/__w/a/../../../etc": "/etc",
	154	+	"/__w//a///b/": "/h/work/a/b",
	155	+	"/__w": "/h/work",
	156	+	"/__t/../__w/x": "/h/work/x",
	157	+	"/__wx/y": "/__wx/y",
	158	+	"rel/../x": "rel/../x",
	159	+	} {
	160	+	got := c.translateToHost(in)
	161	+	if got != want {
	162	+	t.Errorf("%q -> %q, want %q", in, got, want)
	163	+	}
	164	+	if strings.HasPrefix(in, "/__w") && !strings.HasPrefix(got, "/h/work") && got != path.Clean(in) {
	165	+	t.Errorf("%q escaped to %q", in, got)
	166	+	}
	167	+	}
	168	+	}
	169	+
	170	+	// A matcher file named by a command of the container is read only when it is
	171	+	// in a directory shared with the host: ".." cannot reach another host file.
	172	+	func TestMatcherPathFromTheContainerStaysInside(t *testing.T) {
	173	+	f := newFakeCtr(t)
	174	+	var outside string
	175	+	rec, res := ctrRun(t, f, ctrJob, " - name: s\n run: echo\n", func(c *Config) {
	176	+	outside = filepath.Join(filepath.Dir(c.Layout.WorkRoot), "outside")
	177	+	writeFiles(t, filepath.Dir(c.Layout.WorkRoot), files{"outside/m.json": `{"problemMatcher":[{"owner":"leak","pattern":[{"regexp":"x"}]}]}`})
	178	+	f.set(t, "exec_out", "::add-matcher::/__w/../outside/m.json\n::add-matcher::"+outside+"/m.json\n::add-matcher::/etc/hostname\n")
	179	+	})
	180	+	if res.Conclusion != ConclusionFailure {
	181	+	t.Fatalf("%+v", res)
	182	+	}
	183	+	log := logOf(rec.step(t, "s"))
	184	+	if strings.Contains(log, "leak") \|\| strings.Count(log, "##[error]") < 2 \|\| !strings.Contains(log, "Could not find file") && !strings.Contains(log, "ould not find") {
	185	+	t.Errorf("log:\n%s", log)
	186	+	}
	187	+	}


@@ -3,6 +3,8 @@
3	package executor	3		package executor
4		4
5	import (	5		import (
		6	+	"path"
		7	+	"path/filepath"
6	"strings"	8		"strings"
7	"testing"	9		"testing"
8		10

@@ -141,3 +143,45 @@ func TestServiceNamesAndContainerIDs(t *testing.T) {
141	}	143		}
142	}	144		}
143	}	145		}
		146	+
		147	+	func TestTranslateToHostDoesNotEscapeTheMapping(t *testing.T) {
		148	+	c := &container{maps: []pathMap{{"/h/work", "/__w"}, {"/opt/tc", "/__t"}}}
		149	+	for in, want := range map[string]string{
		150	+	"/__w/a/b": "/h/work/a/b",
		151	+	"/__w/a/../b": "/h/work/b",
		152	+	"/__w/../outside/x": "/outside/x",
		153	+	"/__w/a/../../../etc": "/etc",
		154	+	"/__w//a///b/": "/h/work/a/b",
		155	+	"/__w": "/h/work",
		156	+	"/__t/../__w/x": "/h/work/x",
		157	+	"/__wx/y": "/__wx/y",
		158	+	"rel/../x": "rel/../x",
		159	+	} {
		160	+	got := c.translateToHost(in)
		161	+	if got != want {
		162	+	t.Errorf("%q -> %q, want %q", in, got, want)
		163	+	}
		164	+	if strings.HasPrefix(in, "/__w") && !strings.HasPrefix(got, "/h/work") && got != path.Clean(in) {
		165	+	t.Errorf("%q escaped to %q", in, got)
		166	+	}
		167	+	}
		168	+	}
		169	+
		170	+	// A matcher file named by a command of the container is read only when it is
		171	+	// in a directory shared with the host: ".." cannot reach another host file.
		172	+	func TestMatcherPathFromTheContainerStaysInside(t *testing.T) {
		173	+	f := newFakeCtr(t)
		174	+	var outside string
		175	+	rec, res := ctrRun(t, f, ctrJob, " - name: s\n run: echo\n", func(c *Config) {
		176	+	outside = filepath.Join(filepath.Dir(c.Layout.WorkRoot), "outside")
		177	+	writeFiles(t, filepath.Dir(c.Layout.WorkRoot), files{"outside/m.json": `{"problemMatcher":[{"owner":"leak","pattern":[{"regexp":"x"}]}]}`})
		178	+	f.set(t, "exec_out", "::add-matcher::/__w/../outside/m.json\n::add-matcher::"+outside+"/m.json\n::add-matcher::/etc/hostname\n")
		179	+	})
		180	+	if res.Conclusion != ConclusionFailure {
		181	+	t.Fatalf("%+v", res)
		182	+	}
		183	+	log := logOf(rec.step(t, "s"))
		184	+	if strings.Contains(log, "leak") \|\| strings.Count(log, "##[error]") < 2 \|\| !strings.Contains(log, "Could not find file") && !strings.Contains(log, "ould not find") {
		185	+	t.Errorf("log:\n%s", log)
		186	+	}
		187	+	}

M internal/executor/results.go +7 -0


@@ -6,6 +6,7 @@ import (
6	6		"context"
7	7		"fmt"
8	8		"io"
	9	+	"io/fs"
9	10		"os"
10	11		"strings"
11	12		"sync/atomic"

@@ -79,6 +80,12 @@ const maxMatcherFile = 1 << 20
79	80
80	81		// ReadMatcherFile reads a problem matcher file.
81	82		func (s *stepRun) ReadMatcherFile(path string) ([]byte, error) {
	83	+	// With a job container the path came from a command of the container: it
	84	+	// is read only when it is a file of a directory the container shares with
	85	+	// the host, never the host's own file of the same name.
	86	+	if c := s.j.stepContainer(); c != nil && !c.insideMappings(path) {
	87	+	return nil, &fs.PathError{Op: "open", Path: path, Err: fs.ErrNotExist}
	88	+	}
82	89		f, err := os.Open(path)
83	90		if err != nil {
84	91		return nil, err


@@ -6,6 +6,7 @@ import (
6	"context"	6		"context"
7	"fmt"	7		"fmt"
8	"io"	8		"io"
		9	+	"io/fs"
9	"os"	10		"os"
10	"strings"	11		"strings"
11	"sync/atomic"	12		"sync/atomic"

@@ -79,6 +80,12 @@ const maxMatcherFile = 1 << 20
79		80
80	// ReadMatcherFile reads a problem matcher file.	81		// ReadMatcherFile reads a problem matcher file.
81	func (s *stepRun) ReadMatcherFile(path string) ([]byte, error) {	82		func (s *stepRun) ReadMatcherFile(path string) ([]byte, error) {
		83	+	// With a job container the path came from a command of the container: it
		84	+	// is read only when it is a file of a directory the container shares with
		85	+	// the host, never the host's own file of the same name.
		86	+	if c := s.j.stepContainer(); c != nil && !c.insideMappings(path) {
		87	+	return nil, &fs.PathError{Op: "open", Path: path, Err: fs.ErrNotExist}
		88	+	}
82	f, err := os.Open(path)	89		f, err := os.Open(path)
83	if err != nil {	90		if err != nil {
84	return nil, err	91		return nil, err