fix: resolve symlinked container paths before building tar entries (#1130)

Docker 29.7 extracts copied archives through `os.Root`, which refuses to follow a symlink to an absolute target, so copying into `/var/run/act` fails with `path escapes from parent` on the many images that link `/var/run` to `/run`. The daemon now resolves every path component for us before any tar entry name is built, and the destination is created with one directory entry per missing component, which no daemon version rejects.

Verified against real daemons (29.4.0, 29.5.0, 29.5.1, 29.5.3, 29.6.2, 29.7.0-rc.1) with `debian:bookworm` (absolute symlink) and `alpine:3` (relative symlink), and against `moby/go-archive` v0.2.0 through the pending fix branch.

1. Fixes https://gitea.com/gitea/runner/issues/1128
1. Upstream bug: https://github.com/moby/moby/issues/53258
1. Supersedes the no-op change in https://gitea.com/gitea/runner/pulls/1129, which cannot help since the daemon strips leading slashes itself

Reviewed-on: https://gitea.com/gitea/runner/pulls/1130
Reviewed-by: bircni <bircni@icloud.com>
Co-authored-by: silverwind <me@silverwind.io>

act/container/docker_run.go

@@ -14,6 +14,7 @@ import (
 	"fmt"
 	"io"
 	"os"
+	"path"
 	"path/filepath"
 	"regexp"
 	"runtime"
@@ -864,24 +865,59 @@ func (cr *containerReference) waitForCommand(ctx context.Context, isTerminal boo
 	}
 }
 
+// mkdirInContainer creates containerPath and returns it with the symlinked components
+// replaced by the targets the daemon reports for them. Docker 29.7 rejects tar entries
+// traversing a symlink to an absolute target, like the "/var/run" of most images, with
+// "path escapes from parent", and not every daemon creates the implied parents of a
+// directory entry, so one entry per missing component is extracted at the deepest
+// existing ancestor.
+// WORKAROUND: https://github.com/moby/moby/issues/53258
+func (cr *containerReference) mkdirInContainer(ctx context.Context, containerPath string) (string, error) {
+	parts := strings.Split(strings.Trim(path.Clean(containerPath), "/"), "/")
+	existing := "/"
+	for i, part := range parts {
+		if part == "" {
+			return existing, nil
+		}
+		stat, err := cr.cli.ContainerStatPath(ctx, cr.id, client.ContainerStatPathOptions{Path: path.Join(existing, part)})
+		if err != nil {
+			// nothing below exists either, so create the remaining components
+			return path.Join(existing, path.Join(parts[i:]...)), cr.mkdirEntries(ctx, existing, parts[i:])
+		}
+		existing = path.Join(existing, part)
+		if target := stat.Stat.LinkTarget; target != "" {
+			if !path.IsAbs(target) {
+				target = path.Join(path.Dir(existing), target)
+			}
+			existing = target
+		}
+	}
+	return existing, nil
+}
+
+func (cr *containerReference) mkdirEntries(ctx context.Context, destPath string, missing []string) error {
+	buf := &bytes.Buffer{}
+	tw := tar.NewWriter(buf)
+	for i := range missing {
+		_ = tw.WriteHeader(&tar.Header{
+			Name:     path.Join(missing[:i+1]...),
+			Mode:     0o777,
+			Typeflag: tar.TypeDir,
+		})
+	}
+	tw.Close()
+	_, err := cr.cli.CopyToContainer(ctx, cr.id, client.CopyToContainerOptions{
+		DestinationPath: destPath,
+		Content:         buf,
+	})
+	return err
+}
+
 func (cr *containerReference) CopyTarStream(ctx context.Context, destPath string, tarStream io.Reader) error {
 	if cr.id == "" {
 		return cr.missingContainerError("copy to %s", destPath)
 	}
-	// Mkdir, with a path relative to the DestinationPath ("/") below. Docker 29.5+
-	// rejects absolute tar entry names with "path escapes from parent".
-	buf := &bytes.Buffer{}
-	tw := tar.NewWriter(buf)
-	_ = tw.WriteHeader(&tar.Header{
-		Name:     strings.TrimPrefix(destPath, "/"),
-		Mode:     0o777,
-		Typeflag: tar.TypeDir,
-	})
-	tw.Close()
-	_, err := cr.cli.CopyToContainer(ctx, cr.id, client.CopyToContainerOptions{
-		DestinationPath: "/",
-		Content:         buf,
-	})
+	destPath, err := cr.mkdirInContainer(ctx, destPath)
 	if err != nil {
 		return fmt.Errorf("failed to mkdir to copy content to container: %w", err)
 	}
@@ -906,6 +942,10 @@ func (cr *containerReference) copyDir(dstPath, srcPath string, useGitIgnore bool
 			return cr.missingContainerError("copy directory to %s", dstPath)
 		}
 		logger := common.Logger(ctx)
+		dstPath, err := cr.mkdirInContainer(ctx, dstPath)
+		if err != nil {
+			return fmt.Errorf("failed to mkdir to copy directory to container: %w", err)
+		}
 		tarFile, err := os.CreateTemp("", "act")
 		if err != nil {
 			return err

act/container/docker_run_test.go

@@ -93,6 +93,11 @@ func (m *mockDockerClient) ExecInspect(ctx context.Context, execID string, opts
 	return args.Get(0).(mobyclient.ExecInspectResult), args.Error(1)
 }
 
+func (m *mockDockerClient) ContainerStatPath(ctx context.Context, containerID string, opts mobyclient.ContainerStatPathOptions) (mobyclient.ContainerStatPathResult, error) {
+	args := m.Called(ctx, containerID, opts)
+	return args.Get(0).(mobyclient.ContainerStatPathResult), args.Error(1)
+}
+
 func (m *mockDockerClient) ContainerAttach(ctx context.Context, containerID string, opts mobyclient.ContainerAttachOptions) (mobyclient.ContainerAttachResult, error) {
 	args := m.Called(ctx, containerID, opts)
 	return args.Get(0).(mobyclient.ContainerAttachResult), args.Error(1)
@@ -336,52 +341,37 @@ func TestDockerWaitFailure(t *testing.T) {
 	client.AssertExpectations(t)
 }
 
-func TestDockerCopyTarStream(t *testing.T) {
-	ctx := context.Background()
-
-	client := &mockDockerClient{}
-	client.On("CopyToContainer", ctx, "123", mock.MatchedBy(func(opts mobyclient.CopyToContainerOptions) bool {
-		return opts.DestinationPath == "/" && opts.Content != nil
-	})).Return(mobyclient.CopyToContainerResult{}, nil)
-	client.On("CopyToContainer", ctx, "123", mock.MatchedBy(func(opts mobyclient.CopyToContainerOptions) bool {
-		return opts.DestinationPath == "/var/run/act" && opts.Content != nil
-	})).Return(mobyclient.CopyToContainerResult{}, nil)
-	cr := &containerReference{
-		id:  "123",
-		cli: client,
-		input: &NewContainerInput{
-			Image: "image",
-		},
+// stubStatPath answers path resolution: the given paths exist, mapped to their target
+// when they are a symlink, everything else does not exist.
+func stubStatPath(client *mockDockerClient, existing map[string]string) {
+	for containerPath, target := range existing {
+		client.On("ContainerStatPath", mock.Anything, "123", mobyclient.ContainerStatPathOptions{Path: containerPath}).
+			Return(mobyclient.ContainerStatPathResult{Stat: container.PathStat{LinkTarget: target}}, nil).Maybe()
 	}
-
-	_ = cr.CopyTarStream(ctx, "/var/run/act", &bytes.Buffer{})
-
-	client.AssertExpectations(t)
+	client.On("ContainerStatPath", mock.Anything, "123", mock.Anything).
+		Return(mobyclient.ContainerStatPathResult{}, cerrdefs.ErrNotFound).Maybe()
 }
 
-// Docker 29.5+ rejects absolute names in the mkdir tarball with
-// "path escapes from parent", since it is extracted relative to "/".
-func TestDockerCopyTarStreamMkdirEntryIsRelative(t *testing.T) {
+// The mkdir tarball is extracted at the deepest existing ancestor, with entries relative
+// to it that never traverse the "/var/run" symlink, see moby/moby#53258.
+func TestDockerCopyTarStream(t *testing.T) {
 	ctx := context.Background()
 
 	var mkdirNames []string
 	client := &mockDockerClient{}
+	stubStatPath(client, map[string]string{"/var": "", "/var/run": "/run", "/run": ""})
 	client.On("CopyToContainer", ctx, "123", mock.MatchedBy(func(opts mobyclient.CopyToContainerOptions) bool {
-		if opts.DestinationPath != "/" || opts.Content == nil {
+		if opts.DestinationPath != "/run" || opts.Content == nil {
 			return false
 		}
 		tr := tar.NewReader(opts.Content)
-		for {
-			hdr, err := tr.Next()
-			if err != nil {
-				break
-			}
+		for hdr, err := tr.Next(); err == nil; hdr, err = tr.Next() {
 			mkdirNames = append(mkdirNames, hdr.Name)
 		}
 		return true
 	})).Return(mobyclient.CopyToContainerResult{}, nil)
 	client.On("CopyToContainer", ctx, "123", mock.MatchedBy(func(opts mobyclient.CopyToContainerOptions) bool {
-		return opts.DestinationPath == "/var/run/act" && opts.Content != nil
+		return opts.DestinationPath == "/run/act" && opts.Content != nil
 	})).Return(mobyclient.CopyToContainerResult{}, nil)
 	cr := &containerReference{
 		id:  "123",
@@ -392,58 +382,45 @@ func TestDockerCopyTarStreamMkdirEntryIsRelative(t *testing.T) {
 	}
 
 	require.NoError(t, cr.CopyTarStream(ctx, "/var/run/act", &bytes.Buffer{}))
-	assert.Equal(t, []string{"var/run/act"}, mkdirNames)
+	assert.Equal(t, []string{"act"}, mkdirNames)
 
 	client.AssertExpectations(t)
 }
 
-func TestDockerCopyTarStreamErrorInCopyFiles(t *testing.T) {
-	ctx := context.Background()
-
+func TestDockerCopyTarStreamErrors(t *testing.T) {
 	merr := errors.New("Failure")
+	for _, testCase := range []struct {
+		name     string
+		mkdirErr error
+		copyErr  error
+	}{
+		{"mkdir", merr, nil},
+		{"copy content", nil, merr},
+	} {
+		t.Run(testCase.name, func(t *testing.T) {
+			ctx := context.Background()
 
-	client := &mockDockerClient{}
-	client.On("CopyToContainer", ctx, "123", mock.MatchedBy(func(opts mobyclient.CopyToContainerOptions) bool {
-		return opts.DestinationPath == "/" && opts.Content != nil
-	})).Return(mobyclient.CopyToContainerResult{}, merr)
-	cr := &containerReference{
-		id:  "123",
-		cli: client,
-		input: &NewContainerInput{
-			Image: "image",
-		},
+			client := &mockDockerClient{}
+			stubStatPath(client, map[string]string{"/var": "", "/var/run": ""})
+			client.On("CopyToContainer", ctx, "123", mock.MatchedBy(func(opts mobyclient.CopyToContainerOptions) bool {
+				return opts.DestinationPath == "/var/run" && opts.Content != nil
+			})).Return(mobyclient.CopyToContainerResult{}, testCase.mkdirErr)
+			client.On("CopyToContainer", ctx, "123", mock.MatchedBy(func(opts mobyclient.CopyToContainerOptions) bool {
+				return opts.DestinationPath == "/var/run/act" && opts.Content != nil
+			})).Return(mobyclient.CopyToContainerResult{}, testCase.copyErr).Maybe()
+			cr := &containerReference{
+				id:  "123",
+				cli: client,
+				input: &NewContainerInput{
+					Image: "image",
+				},
+			}
+
+			require.ErrorIs(t, cr.CopyTarStream(ctx, "/var/run/act", &bytes.Buffer{}), merr)
+
+			client.AssertExpectations(t)
+		})
 	}
-
-	err := cr.CopyTarStream(ctx, "/var/run/act", &bytes.Buffer{})
-	assert.ErrorIs(t, err, merr) //nolint:testifylint // pre-existing issue from nektos/act
-
-	client.AssertExpectations(t)
-}
-
-func TestDockerCopyTarStreamErrorInMkdir(t *testing.T) {
-	ctx := context.Background()
-
-	merr := errors.New("Failure")
-
-	client := &mockDockerClient{}
-	client.On("CopyToContainer", ctx, "123", mock.MatchedBy(func(opts mobyclient.CopyToContainerOptions) bool {
-		return opts.DestinationPath == "/" && opts.Content != nil
-	})).Return(mobyclient.CopyToContainerResult{}, nil)
-	client.On("CopyToContainer", ctx, "123", mock.MatchedBy(func(opts mobyclient.CopyToContainerOptions) bool {
-		return opts.DestinationPath == "/var/run/act" && opts.Content != nil
-	})).Return(mobyclient.CopyToContainerResult{}, merr)
-	cr := &containerReference{
-		id:  "123",
-		cli: client,
-		input: &NewContainerInput{
-			Image: "image",
-		},
-	}
-
-	err := cr.CopyTarStream(ctx, "/var/run/act", &bytes.Buffer{})
-	assert.ErrorIs(t, err, merr) //nolint:testifylint // pre-existing issue from nektos/act
-
-	client.AssertExpectations(t)
 }
 
 // A remove that raced the daemon's AutoRemove teardown is not a failure and must not
@@ -625,9 +602,8 @@ func TestDockerCopyToSymlinkPath(t *testing.T) {
 		_ = rc.Close()(ctx)
 	})
 
-	// CopyTarStream first creates the destination directory by extracting a tar at "/",
-	// which makes the daemon mkdir var, then var/run (the symlink), then act — the exact
-	// step that fails on the broken daemon.
+	// CopyTarStream resolves the var/run symlink and creates act below its target, the
+	// exact step that fails on a broken daemon.
 	err := rc.CopyTarStream(ctx, "/var/run/act", &bytes.Buffer{})
 	require.NoError(t, err)
 }