diff --git a/act/artifactcache/handler.go b/act/artifactcache/handler.go index c5bb0376..6efff636 100644 --- a/act/artifactcache/handler.go +++ b/act/artifactcache/handler.go @@ -431,6 +431,7 @@ func (h *Handler) upload(w http.ResponseWriter, r *http.Request, params httprout } if err := h.storage.Write(cache.ID, start, r.Body); err != nil { h.responseJSON(w, r, 500, err) + return } h.useCache(id) h.responseJSON(w, r, 200) diff --git a/act/artifactcache/handler_test.go b/act/artifactcache/handler_test.go index 7cca691c..71e84e64 100644 --- a/act/artifactcache/handler_test.go +++ b/act/artifactcache/handler_test.go @@ -11,6 +11,7 @@ import ( "fmt" "io" "net/http" + "os" "path/filepath" "strings" "testing" @@ -338,6 +339,54 @@ func TestHandler(t *testing.T) { } }) + t.Run("upload write failure returns only error", func(t *testing.T) { + key := strings.ToLower(t.Name()) + version := "c19da02a2bd7e77277f1ac29ab45c09b7d46a4ee758284e26bb3045ad11d9d20" + var id uint64 + { + body, err := json.Marshal(&Request{ + Key: key, + Version: version, + Size: 100, + }) + require.NoError(t, err) + resp, err := testClient.Post(base+"/caches", "application/json", bytes.NewReader(body)) + require.NoError(t, err) + defer resp.Body.Close() + require.Equal(t, 200, resp.StatusCode) + + got := struct { + CacheID uint64 `json:"cacheId"` + }{} + require.NoError(t, json.NewDecoder(resp.Body).Decode(&got)) + id = got.CacheID + } + + storageFile := filepath.Join(dir, "not-a-directory") + require.NoError(t, os.WriteFile(storageFile, []byte("blocked"), 0o600)) + originalStorage := handler.storage + handler.storage = &Storage{rootDir: storageFile} + defer func() { + handler.storage = originalStorage + }() + + req, err := http.NewRequest(http.MethodPatch, + fmt.Sprintf("%s/caches/%d", base, id), bytes.NewReader(make([]byte, 100))) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/octet-stream") + req.Header.Set("Content-Range", "bytes 0-99/*") + resp, err := testClient.Do(req) + require.NoError(t, err) + defer resp.Body.Close() + require.Equal(t, 500, resp.StatusCode) + + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + var got map[string]string + require.NoError(t, json.Unmarshal(body, &got)) + assert.NotEmpty(t, got["error"]) + }) + t.Run("commit early", func(t *testing.T) { key := strings.ToLower(t.Name()) version := "c19da02a2bd7e77277f1ac29ab45c09b7d46a4ee758284e26bb3045ad11d9d20" diff --git a/act/container/docker_run.go b/act/container/docker_run.go index 15251721..b141bd62 100644 --- a/act/container/docker_run.go +++ b/act/container/docker_run.go @@ -17,6 +17,7 @@ import ( "path/filepath" "regexp" "runtime" + "slices" "strconv" "strings" @@ -968,22 +969,7 @@ func (cr *containerReference) sanitizeConfig(ctx context.Context, config *contai logger := common.Logger(ctx) if len(cr.input.ValidVolumes) > 0 { - globs := make([]glob.Glob, 0, len(cr.input.ValidVolumes)) - for _, v := range cr.input.ValidVolumes { - if g, err := glob.Compile(v); err != nil { - logger.Errorf("create glob from %s error: %v", v, err) - } else { - globs = append(globs, g) - } - } - isValid := func(v string) bool { - for _, g := range globs { - if g.Match(v) { - return true - } - } - return false - } + matcher := newValidVolumeMatcher(ctx, cr.input.ValidVolumes) // sanitize binds sanitizedBinds := make([]string, 0, len(hostConfig.Binds)) for _, bind := range hostConfig.Binds { @@ -997,7 +983,7 @@ func (cr *containerReference) sanitizeConfig(ctx context.Context, config *contai sanitizedBinds = append(sanitizedBinds, bind) continue } - if isValid(parsed.Source) { + if matcher.isValid(parsed.Source, mount.Type(parsed.Type)) { sanitizedBinds = append(sanitizedBinds, bind) } else { logger.Warnf("[%s] is not a valid volume, will be ignored", parsed.Source) @@ -1007,7 +993,7 @@ func (cr *containerReference) sanitizeConfig(ctx context.Context, config *contai // sanitize mounts sanitizedMounts := make([]mount.Mount, 0, len(hostConfig.Mounts)) for _, mt := range hostConfig.Mounts { - if isValid(mt.Source) { + if matcher.isValid(mt.Source, mt.Type) { sanitizedMounts = append(sanitizedMounts, mt) } else { logger.Warnf("[%s] is not a valid volume, will be ignored", mt.Source) @@ -1021,3 +1007,129 @@ func (cr *containerReference) sanitizeConfig(ctx context.Context, config *contai return config, hostConfig } + +type validVolumeMatcher struct { + allowAll bool + named []glob.Glob + host []glob.Glob +} + +func newValidVolumeMatcher(ctx context.Context, validVolumes []string) validVolumeMatcher { + logger := common.Logger(ctx) + ret := validVolumeMatcher{ + named: make([]glob.Glob, 0, len(validVolumes)), + host: make([]glob.Glob, 0, len(validVolumes)), + } + + for _, v := range validVolumes { + if v == "**" { + ret.allowAll = true + continue + } + if !isHostVolumePattern(v) { + if g, err := glob.Compile(v); err != nil { + logger.Errorf("create glob from %s error: %v", v, err) + } else { + ret.named = append(ret.named, g) + } + continue + } + normalized, err := normalizeHostVolumePath(v) + if err != nil { + logger.Errorf("normalize volume pattern %s error: %v", v, err) + continue + } + if g, err := glob.Compile(normalized); err != nil { + logger.Errorf("create glob from %s error: %v", normalized, err) + } else { + ret.host = append(ret.host, g) + } + } + + return ret +} + +func (m validVolumeMatcher) isValid(source string, sourceType mount.Type) bool { + if m.allowAll { + return true + } + if isHostVolumeSource(source, sourceType) { + normalized, err := normalizeHostVolumePath(source) + if err != nil { + return false + } + for _, g := range m.host { + if g.Match(normalized) { + return true + } + } + return false + } + for _, g := range m.named { + if g.Match(source) { + return true + } + } + return false +} + +func isHostVolumePattern(pattern string) bool { + return filepath.IsAbs(pattern) || + strings.HasPrefix(pattern, "."+string(filepath.Separator)) || + strings.HasPrefix(pattern, ".."+string(filepath.Separator)) || + strings.Contains(pattern, "/") || + strings.Contains(pattern, `\`) +} + +func isHostVolumeSource(source string, sourceType mount.Type) bool { + if sourceType == mount.TypeBind { + return true + } + if sourceType == mount.TypeVolume { + return false + } + return isHostVolumePattern(source) +} + +func normalizeHostVolumePath(path string) (string, error) { + abs, err := filepath.Abs(path) + if err != nil { + return "", err + } + return evalSymlinksExistingPrefix(abs) +} + +func evalSymlinksExistingPrefix(path string) (string, error) { + resolved, err := filepath.EvalSymlinks(path) + if err == nil { + return filepath.Clean(resolved), nil + } + if !errors.Is(err, os.ErrNotExist) { + return "", err + } + + current := path + var missing []string + for { + _, err := os.Lstat(current) + if err == nil { + resolved, err := filepath.EvalSymlinks(current) + if err != nil { + return "", err + } + for _, name := range slices.Backward(missing) { + resolved = filepath.Join(resolved, name) + } + return filepath.Clean(resolved), nil + } + if !errors.Is(err, os.ErrNotExist) { + return "", err + } + parent := filepath.Dir(current) + if parent == current { + return filepath.Clean(path), nil + } + missing = append(missing, filepath.Base(current)) + current = parent + } +} diff --git a/act/container/docker_run_test.go b/act/container/docker_run_test.go index d8a23fc5..903ad67b 100644 --- a/act/container/docker_run_test.go +++ b/act/container/docker_run_test.go @@ -11,6 +11,8 @@ import ( "errors" "io" "net" + "os" + "path/filepath" "strings" "testing" "time" @@ -375,3 +377,40 @@ func TestCheckVolumes(t *testing.T) { }) } } + +func TestCheckVolumesRejectsEscapingHostPaths(t *testing.T) { + logger, _ := test.NewNullLogger() + ctx := common.WithLogger(context.Background(), logger) + + base := t.TempDir() + allowed := filepath.Join(base, "allowed") + denied := filepath.Join(base, "denied") + require.NoError(t, os.MkdirAll(allowed, 0o700)) + require.NoError(t, os.MkdirAll(denied, 0o700)) + + cr := &containerReference{ + input: &NewContainerInput{ + ValidVolumes: []string{filepath.Join(allowed, "**")}, + }, + } + + escapingPath := allowed + string(filepath.Separator) + ".." + string(filepath.Separator) + "denied" + _, hostConf := cr.sanitizeConfig(ctx, &container.Config{}, &container.HostConfig{ + Binds: []string{escapingPath + ":/mnt"}, + }) + assert.Empty(t, hostConf.Binds) + + linkPath := filepath.Join(allowed, "link") + if err := os.Symlink(denied, linkPath); err != nil { + t.Skipf("cannot create symlink: %v", err) + } + _, hostConf = cr.sanitizeConfig(ctx, &container.Config{}, &container.HostConfig{ + Binds: []string{linkPath + ":/mnt"}, + }) + assert.Empty(t, hostConf.Binds) + + _, hostConf = cr.sanitizeConfig(ctx, &container.Config{}, &container.HostConfig{ + Binds: []string{filepath.Join(linkPath, "missing") + ":/mnt"}, + }) + assert.Empty(t, hostConf.Binds) +} diff --git a/act/container/host_environment.go b/act/container/host_environment.go index 497039bf..ee456a24 100644 --- a/act/container/host_environment.go +++ b/act/container/host_environment.go @@ -37,13 +37,13 @@ type HostEnvironment struct { TmpDir string ToolCache string Workdir string - // BindWorkdir is true when the app runner mounts the workspace on the host and - // deletes the task directory after the job; host teardown must not remove Workdir. - BindWorkdir bool - ActPath string - CleanUp func() - StdOut io.Writer - AllocatePTY bool // allocate a pseudo-TTY for each step's process + // CleanWorkdir means teardown owns Workdir and may delete it. Leave false + // when Workdir points at a caller-owned checkout (e.g. `act` local mode). + CleanWorkdir bool + ActPath string + CleanUp func() + StdOut io.Writer + AllocatePTY bool // allocate a pseudo-TTY for each step's process mu sync.Mutex runningPIDs map[int]struct{} @@ -483,7 +483,7 @@ func (e *HostEnvironment) Remove() common.Executor { logger.Warnf("failed to remove host misc state %s: %v", e.Path, err) errs = append(errs, err) } - if !e.BindWorkdir && e.Workdir != "" { + if e.CleanWorkdir { if err := removePathWithRetry(ctx, e.Workdir); err != nil { logger.Warnf("failed to remove host workspace %s: %v", e.Workdir, err) errs = append(errs, err) diff --git a/act/container/host_environment_test.go b/act/container/host_environment_test.go index a9911d19..945685c9 100644 --- a/act/container/host_environment_test.go +++ b/act/container/host_environment_test.go @@ -141,7 +141,7 @@ func TestHostEnvironmentAllocatePTY(t *testing.T) { } } -func TestHostEnvironmentRemoveCleansWorkdir(t *testing.T) { +func TestHostEnvironmentRemovePreservesWorkdirByDefault(t *testing.T) { logger := logrus.New() ctx := common.WithLogger(context.Background(), logrus.NewEntry(logger)) base := t.TempDir() @@ -152,9 +152,8 @@ func TestHostEnvironmentRemoveCleansWorkdir(t *testing.T) { require.NoError(t, os.MkdirAll(workdir, 0o700)) e := &HostEnvironment{ - Path: path, - Workdir: workdir, - BindWorkdir: false, + Path: path, + Workdir: workdir, CleanUp: func() { _ = os.RemoveAll(miscRoot) }, @@ -162,10 +161,10 @@ func TestHostEnvironmentRemoveCleansWorkdir(t *testing.T) { } require.NoError(t, e.Remove()(ctx)) _, err := os.Stat(workdir) - assert.ErrorIs(t, err, os.ErrNotExist) + require.NoError(t, err) } -func TestHostEnvironmentRemoveSkipsWorkdirWhenBindWorkdir(t *testing.T) { +func TestHostEnvironmentRemoveCleansWorkdirWhenOwned(t *testing.T) { logger := logrus.New() ctx := common.WithLogger(context.Background(), logrus.NewEntry(logger)) base := t.TempDir() @@ -176,9 +175,9 @@ func TestHostEnvironmentRemoveSkipsWorkdirWhenBindWorkdir(t *testing.T) { require.NoError(t, os.MkdirAll(workdir, 0o700)) e := &HostEnvironment{ - Path: path, - Workdir: workdir, - BindWorkdir: true, + Path: path, + Workdir: workdir, + CleanWorkdir: true, CleanUp: func() { _ = os.RemoveAll(miscRoot) }, @@ -186,5 +185,5 @@ func TestHostEnvironmentRemoveSkipsWorkdirWhenBindWorkdir(t *testing.T) { } require.NoError(t, e.Remove()(ctx)) _, err := os.Stat(workdir) - require.NoError(t, err) + assert.ErrorIs(t, err, os.ErrNotExist) } diff --git a/act/runner/run_context.go b/act/runner/run_context.go index 23a0d67e..70919361 100644 --- a/act/runner/run_context.go +++ b/act/runner/run_context.go @@ -220,12 +220,12 @@ func (rc *RunContext) startHostEnvironment() common.Executor { } toolCache := filepath.Join(cacheDir, "tool_cache") rc.JobContainer = &container.HostEnvironment{ - Path: path, - TmpDir: runnerTmp, - ToolCache: toolCache, - Workdir: rc.Config.Workdir, - BindWorkdir: rc.Config.BindWorkdir, - ActPath: actPath, + Path: path, + TmpDir: runnerTmp, + ToolCache: toolCache, + Workdir: rc.Config.Workdir, + CleanWorkdir: rc.Config.CleanWorkdir, + ActPath: actPath, CleanUp: func() { os.RemoveAll(miscpath) }, diff --git a/act/runner/runner.go b/act/runner/runner.go index 0da97e42..cb389ecd 100644 --- a/act/runner/runner.go +++ b/act/runner/runner.go @@ -73,6 +73,7 @@ type Config struct { EventJSON string // the content of JSON file to use for event.json in containers, overrides EventPath ContainerNamePrefix string // the prefix of container name ContainerMaxLifetime time.Duration // the max lifetime of job containers + CleanWorkdir bool // remove host executor workdir on teardown DefaultActionInstance string // the default actions web site PlatformPicker func(labels []string) string // platform picker, it will take precedence over Platforms if isn't nil JobLoggerLevel *log.Level // the level of job logger @@ -91,6 +92,17 @@ func (c Config) GetToken() string { return token } +// DefaultActionURL returns the host used for implicit remote actions. +func (c Config) DefaultActionURL() string { + if c.DefaultActionInstance != "" { + return c.DefaultActionInstance + } + if c.GitHubInstance != "" { + return c.GitHubInstance + } + return "github.com" +} + type caller struct { runContext *RunContext diff --git a/act/runner/step_action_remote.go b/act/runner/step_action_remote.go index e842ca97..fdddebad 100644 --- a/act/runner/step_action_remote.go +++ b/act/runner/step_action_remote.go @@ -113,9 +113,10 @@ func (sar *stepActionRemote) prepareActionExecutor() common.Executor { } actionDir := fmt.Sprintf("%s/%s", sar.RunContext.ActionCacheDir(), sar.Step.UsesHash()) - token := getGitCloneToken(sar.getRunContext().Config, sar.remoteAction.CloneURL(sar.RunContext.Config.DefaultActionInstance)) + defaultActionURL := sar.RunContext.Config.DefaultActionURL() + token := getGitCloneToken(sar.getRunContext().Config, sar.remoteAction.CloneURL(defaultActionURL)) gitClone := stepActionRemoteNewCloneExecutor(git.NewGitCloneExecutorInput{ - URL: sar.remoteAction.CloneURL(sar.RunContext.Config.DefaultActionInstance), + URL: sar.remoteAction.CloneURL(defaultActionURL), Ref: sar.remoteAction.Ref, Dir: actionDir, Token: token, @@ -274,7 +275,7 @@ func (sar *stepActionRemote) cloneSkipTLS() bool { if sar.remoteAction.URL == "" { // Empty URL means the default action instance should be used // Return true if the URL of the Gitea instance is the same as the URL of the default action instance - return sar.RunContext.Config.DefaultActionInstance == sar.RunContext.Config.GitHubInstance + return sar.RunContext.Config.DefaultActionURL() == sar.RunContext.Config.GitHubInstance } // Return true if the URL of the remote action is the same as the URL of the Gitea instance return sar.remoteAction.URL == sar.RunContext.Config.GitHubInstance diff --git a/act/runner/step_action_remote_test.go b/act/runner/step_action_remote_test.go index 0f0c9db5..0531d0ea 100644 --- a/act/runner/step_action_remote_test.go +++ b/act/runner/step_action_remote_test.go @@ -20,6 +20,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" "go.yaml.in/yaml/v4" ) @@ -434,6 +435,57 @@ func TestStepActionRemotePreThroughActionToken(t *testing.T) { } } +func TestStepActionRemoteUsesGitHubInstanceWhenDefaultActionInstanceEmpty(t *testing.T) { + ctx := context.Background() + + var actualURL string + sarm := &stepActionRemoteMocks{} + + origStepAtionRemoteNewCloneExecutor := stepActionRemoteNewCloneExecutor + stepActionRemoteNewCloneExecutor = func(input git.NewGitCloneExecutorInput) common.Executor { + return func(ctx context.Context) error { + actualURL = input.URL + return nil + } + } + defer func() { + stepActionRemoteNewCloneExecutor = origStepAtionRemoteNewCloneExecutor + }() + + sar := &stepActionRemote{ + Step: &model.Step{ + Uses: "actions/setup-go@v4", + }, + RunContext: &RunContext{ + Config: &Config{ + GitHubInstance: "gitea.example", + DefaultActionInstance: "", + ActionCacheDir: t.TempDir(), + }, + Run: &model.Run{ + JobID: "1", + Workflow: &model.Workflow{ + Jobs: map[string]*model.Job{ + "1": {}, + }, + }, + }, + }, + readAction: sarm.readAction, + } + + suffixMatcher := func(suffix string) any { + return mock.MatchedBy(func(actionDir string) bool { + return strings.HasSuffix(actionDir, suffix) + }) + } + sarm.On("readAction", sar.Step, suffixMatcher(sar.Step.UsesHash()), "", mock.Anything, mock.Anything).Return(&model.Action{}, nil) + + require.NoError(t, sar.prepareActionExecutor()(ctx)) + assert.Equal(t, "https://gitea.example/actions/setup-go", actualURL) + sarm.AssertExpectations(t) +} + func TestStepActionRemotePost(t *testing.T) { table := []struct { name string diff --git a/internal/app/run/runner.go b/internal/app/run/runner.go index 6f83e560..6d72415f 100644 --- a/internal/app/run/runner.go +++ b/internal/app/run/runner.go @@ -363,6 +363,7 @@ func (r *Runner) run(ctx context.Context, task *runnerv1.Task, reporter *report. EventJSON: string(eventJSON), ContainerNamePrefix: fmt.Sprintf("GITEA-ACTIONS-TASK-%d", task.Id), ContainerMaxLifetime: maxLifetime, + CleanWorkdir: true, ContainerNetworkMode: container.NetworkMode(r.cfg.Container.Network), ContainerOptions: r.cfg.Container.Options, ContainerDaemonSocket: r.cfg.Container.DockerHost,