diff --git a/act/container/docker_run.go b/act/container/docker_run.go index f3a279c8..73237af9 100644 --- a/act/container/docker_run.go +++ b/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 diff --git a/act/container/docker_run_test.go b/act/container/docker_run_test.go index 43ea19fb..e3c5949f 100644 --- a/act/container/docker_run_test.go +++ b/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) }