From fe2a66dc25403d9a204c3300858e07d581433f6a Mon Sep 17 00:00:00 2001 From: silverwind Date: Fri, 23 Oct 2026 09:18:24 +0000 Subject: [PATCH] fix: align runner behaviours (#1338) Contains a number of small bug fixes where jobs behaved differently from GitHub or failed silently. 1. Hooks fail on a refused `::set-env::` or `::add-path::` and no longer inherit the last step's identity 1. Docker build, exec and wait errors are returned instead of swallowed, token-only registry credentials work, logs of `--tty` services decode 1. Env files split lines like GitHub on each OS, so a carriage return is kept on Linux and CRLF ends a line on Windows, empty heredoc names or delimiters are rejected 1. Post steps run after their pre step even when main was skipped, composites see undeclared inputs, `runs.args` wins without `with.args` being parsed 1. Docker actions skip evaluating defaults of inputs given in `with` and read `entryPoint` and `args` in any case, a lowercase `dockerfile` is found, undeclared inputs get GitHub's warning 1. A user-set `CI` is kept, `GITHUB_REPOSITORY_ID` and the other missing `GITHUB_*` variables are exported 1. `timeout-minutes` can read the step's own `env`, a step-level `ACTIONS_ALLOW_UNSECURE_COMMANDS: false` beats a job-level `true` while the runner's own opt-in still wins, `pwsh` quotes the script path 1. A step `env:` given as one expression such as `${{ fromJSON(matrix.env) }}` is applied, `hashFiles` fails with GitHub's messages on script errors and after 120 seconds, a job image without node still gets an empty hash 1. A step whose command files cannot be reset fails instead of importing stale ones, a timeout between steps fails the job instead of reporting success 1. One named volume can be mounted at several paths, host-mode PTY steps keep their last output when they fail, host scratch directories are removed when job setup fails 1. Cancelling a step right after it starts kills its whole process tree 1. Cache prefix restores pick the newest entry created within the same second, also on S3, cache commits with a wrong size are rejected 1. Artifact chunks are written at their offset, download URLs are escaped, listing a run without artifacts no longer crashes 1. Poll backoff reaches `fetch_interval_max`, proxy passwords are masked in more forms, secrets are also masked in step names and in their quoted form in the runner log, a failed final report is logged 1. On Windows hosts a differently cased unsecure-commands opt-in or proxy variable counts, and an early cancel no longer leaks a job object handle 1. Interactive `register` prompts for a missing token and stops on SIGTERM, `exec --list --detect-event` no longer panics without events, `config set` works on `cache.s3` keys Reviewed-on: https://gitea.com/gitea/runner/pulls/1338 Reviewed-by: bircni Co-authored-by: silverwind --- act/artifactcache/handler.go | 39 ++++-- act/artifactcache/handler_test.go | 46 +++++++ act/artifactcache/handler_v2.go | 17 +-- act/artifactcache/model.go | 25 ++-- act/artifactcache/s3_storage_test.go | 20 +++ act/artifacts/server.go | 27 ++-- act/artifacts/server_test.go | 32 +++-- act/common/executor.go | 5 +- act/common/executor_test.go | 13 +- act/container/docker_build.go | 6 +- act/container/docker_build_test.go | 22 ++++ act/container/docker_logger.go | 3 +- act/container/docker_pull.go | 2 +- act/container/docker_pull_test.go | 24 ++++ act/container/docker_run.go | 44 +++++-- act/container/docker_run_test.go | 136 +++++++++++++++------ act/container/host_environment.go | 16 +-- act/container/host_environment_test.go | 36 ++++-- act/container/kubernetes.go | 2 +- act/container/parse_env_file.go | 26 +++- act/container/parse_env_file_test.go | 45 +++++-- act/ghcontext/github_context.go | 21 +++- act/ghcontext/github_context_test.go | 42 +++++++ act/runner/action.go | 81 ++++++++++-- act/runner/action_composite.go | 5 + act/runner/action_composite_test.go | 24 ++-- act/runner/action_test.go | 109 +++++++++++++++-- act/runner/cancellation_test.go | 4 + act/runner/command.go | 27 +++- act/runner/command_test.go | 20 +-- act/runner/expression.go | 40 ++++-- act/runner/expression_test.go | 97 +++++++++++++++ act/runner/job_executor.go | 1 + act/runner/job_hooks.go | 15 ++- act/runner/job_hooks_test.go | 53 ++++++++ act/runner/logger.go | 9 +- act/runner/logger_test.go | 14 ++- act/runner/run_context.go | 25 +++- act/runner/run_context_test.go | 58 ++++++++- act/runner/runner_test.go | 1 + act/runner/step.go | 64 +++++----- act/runner/step_action_remote.go | 6 +- act/runner/step_action_remote_test.go | 54 ++++++++ act/runner/step_docker.go | 4 +- act/runner/step_run.go | 3 + act/runner/step_run_print_test.go | 4 +- act/runner/step_run_test.go | 21 +++- act/runner/step_test.go | 42 ++++++- act/runner/testdata/github-parity/push.yml | 20 +++ docs/job-hooks.md | 2 +- internal/app/cmd/exec.go | 9 +- internal/app/cmd/exec_test.go | 3 + internal/app/cmd/register.go | 29 ++--- internal/app/cmd/register_test.go | 50 +++++++- internal/app/poll/poller.go | 12 +- internal/app/poll/poller_test.go | 3 + internal/app/run/proxy.go | 17 ++- internal/app/run/proxy_test.go | 13 +- internal/app/run/runner.go | 8 +- internal/app/run/runner_test.go | 24 ++++ internal/pkg/config/edit.go | 3 + internal/pkg/config/edit_test.go | 5 + internal/pkg/process/killer_unix_test.go | 29 +++++ internal/pkg/process/treekill.go | 4 + internal/pkg/report/globalmask.go | 10 +- internal/pkg/report/globalmask_test.go | 7 ++ internal/pkg/report/reporter.go | 4 + internal/pkg/report/reporter_test.go | 12 ++ 68 files changed, 1413 insertions(+), 281 deletions(-) create mode 100644 act/runner/testdata/github-parity/push.yml diff --git a/act/artifactcache/handler.go b/act/artifactcache/handler.go index 04345b33..5dbef3fe 100644 --- a/act/artifactcache/handler.go +++ b/act/artifactcache/handler.go @@ -15,6 +15,7 @@ import ( "encoding/json/v2" "errors" "fmt" + "io" "net" "net/http" "net/http/httputil" @@ -710,9 +711,10 @@ func (h *Handler) reserve(w http.ResponseWriter, r *http.Request) { } defer db.Close() - now := time.Now().Unix() - cache.CreatedAt = now - cache.UsedAt = now + now := time.Now() + cache.CreatedAt = now.Unix() + cache.CreatedAtNanos = now.Nanosecond() + cache.UsedAt = now.Unix() if err := h.insertCache(db, cache); err != nil { h.responseJSON(w, r, 500, err) return @@ -778,6 +780,17 @@ func (h *Handler) commit(w http.ResponseWriter, r *http.Request) { h.responseJSON(w, r, 400, err) return } + var request struct { + Size *int64 `json:"size"` + } + body, err := io.ReadAll(io.LimitReader(r.Body, 1<<20)) + if err == nil && len(body) > 0 { + err = json.Unmarshal(body, &request) + } + if err != nil { + h.responseJSON(w, r, http.StatusBadRequest, err) + return + } db, err := h.openDB() if err != nil { @@ -805,6 +818,14 @@ func (h *Handler) commit(w http.ResponseWriter, r *http.Request) { return } + if request.Size != nil { + if *request.Size < 0 || cache.Size >= 0 && cache.Size != *request.Size { + h.responseJSON(w, r, http.StatusBadRequest, errors.New("invalid cache size")) + return + } + cache.Size = *request.Size + } + db.Close() if err := h.commitCache(cache); err != nil { @@ -1067,7 +1088,7 @@ func findCache(db *cacheDB, repo string, keys []string, version string) (*Cache, if err != nil { return nil, fmt.Errorf("find cache: %w", err) } - if cache := latest(caches, func(cache *Cache) int64 { return cache.CreatedAt }); cache != nil { + if cache := latest(caches, (*Cache).createdAtNano); cache != nil { return cache, nil } } @@ -1078,7 +1099,9 @@ func latest(caches []*Cache, stamp func(*Cache) int64) *Cache { if len(caches) == 0 { return nil } - return slices.MaxFunc(caches, func(left, right *Cache) int { return cmp.Compare(stamp(left), stamp(right)) }) + return slices.MaxFunc(caches, func(left, right *Cache) int { + return cmp.Or(cmp.Compare(stamp(left), stamp(right)), cmp.Compare(left.ID, right.ID)) + }) } // findExactCache returns the entry for exactly this key and version, or (nil, nil) if there is @@ -1097,7 +1120,7 @@ func findExactCache(db *cacheDB, repo, key, version string, complete bool) (*Cac } stamp := func(cache *Cache) int64 { return cache.UsedAt } if complete { - stamp = func(cache *Cache) int64 { return cache.CreatedAt } + stamp = (*Cache).createdAtNano } return latest(caches, stamp), nil } @@ -1267,7 +1290,9 @@ func (h *Handler) evictSuperseded(db *cacheDB) { groups[group] = append(groups[group], cache) } for _, caches := range groups { - slices.SortFunc(caches, func(left, right *Cache) int { return cmp.Compare(left.CreatedAt, right.CreatedAt) }) + slices.SortFunc(caches, func(left, right *Cache) int { + return cmp.Or(cmp.Compare(left.createdAtNano(), right.createdAtNano()), cmp.Compare(left.ID, right.ID)) + }) for _, cache := range caches[:len(caches)-1] { if inUse(cache) { continue diff --git a/act/artifactcache/handler_test.go b/act/artifactcache/handler_test.go index 6f7fdd46..ff0477cf 100644 --- a/act/artifactcache/handler_test.go +++ b/act/artifactcache/handler_test.go @@ -15,6 +15,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "strconv" "strings" "testing" "testing/synctest" @@ -302,6 +303,37 @@ func TestHandler(t *testing.T) { } }) + t.Run("chunked commit validates size omitted at reservation", func(t *testing.T) { + for _, size := range []int64{5, 10} { + t.Run(strconv.FormatInt(size, 10), func(t *testing.T) { + id := reserveCache(t, testClient, base, strings.ToLower(t.Name()), version, 0) + req, err := http.NewRequest(http.MethodPatch, fmt.Sprintf("%s/caches/%d", base, id), strings.NewReader("cache")) + require.NoError(t, err) + req.Header.Set("Content-Range", "bytes 0-4/*") + resp, err := testClient.Do(req) + require.NoError(t, err) + resp.Body.Close() + require.Equal(t, http.StatusOK, resp.StatusCode) + + resp, err = testClient.Post(fmt.Sprintf("%s/caches/%d", base, id), "application/json", io.MultiReader(strings.NewReader(fmt.Sprintf(`{"size":%d}`, size)))) + require.NoError(t, err) + resp.Body.Close() + if size == 5 { + require.Equal(t, http.StatusOK, resp.StatusCode) + } else { + require.Equal(t, http.StatusInternalServerError, resp.StatusCode) + } + + db, err := handler.openDB() + require.NoError(t, err) + defer db.Close() + cache, err := db.get(id) + require.NoError(t, err) + require.Equal(t, size == 5, cache.Complete) + }) + } + }) + t.Run("get with bad id", func(t *testing.T) { resp, err := testClient.Get(base + "/artifacts/invalid_id") require.NoError(t, err) @@ -709,6 +741,20 @@ func TestHandler_FindProtectsFromEviction(t *testing.T) { assert.Equal(t, []string{wanted.Key}, keptKeys(t, handler, []*Cache{wanted, other}), "the entry just promised to a job survives, the next least recently used goes instead") } +func TestHandler_PrefixUsesNewestReservationWithinSecond(t *testing.T) { + created := time.Now().Add(-time.Hour).Unix() + older := &Cache{Repo: testRepo, Key: "deps-old", Version: "v", Complete: true, CreatedAt: created} + newer := &Cache{Repo: testRepo, Key: "deps-new", Version: "v", Complete: true, CreatedAt: created} + handler := newTestHandler(t, Policy{}, older, newer) + + db, err := handler.openDB() + require.NoError(t, err) + defer db.Close() + cache, err := findCache(db, testRepo, []string{"deps-"}, "v") + require.NoError(t, err) + require.Equal(t, newer, cache) +} + func TestHandler_evictOnCommit(t *testing.T) { full := &Cache{Repo: testRepo, Key: "full", Version: "v", Complete: true, Size: 4 * miB, UsedAt: time.Now().Add(-time.Hour).Unix()} handler := newTestHandler(t, Policy{RepoSizeLimit: 4 * miB}, full) diff --git a/act/artifactcache/handler_v2.go b/act/artifactcache/handler_v2.go index 49574707..bb188f1c 100644 --- a/act/artifactcache/handler_v2.go +++ b/act/artifactcache/handler_v2.go @@ -100,15 +100,16 @@ func (h *Handler) v2CreateCacheEntry(w http.ResponseWriter, r *http.Request) { return } - now := time.Now().Unix() + now := time.Now() cache := &Cache{ - Repo: cred.Repo, - Key: req.Key, - Version: req.Version, - Owner: owner, - Size: -1, // the size is only known at finalize time - CreatedAt: now, - UsedAt: now, + Repo: cred.Repo, + Key: req.Key, + Version: req.Version, + Owner: owner, + Size: -1, // the size is only known at finalize time + CreatedAt: now.Unix(), + CreatedAtNanos: now.Nanosecond(), + UsedAt: now.Unix(), } if err := h.insertCache(db, cache); err != nil { h.twirpError(w, r, twirpInternal, err) diff --git a/act/artifactcache/model.go b/act/artifactcache/model.go index def1d441..9dfd3201 100644 --- a/act/artifactcache/model.go +++ b/act/artifactcache/model.go @@ -4,6 +4,8 @@ package artifactcache +import "time" + type Request struct { Key string `json:"key" ` Version string `json:"version"` @@ -11,13 +13,18 @@ type Request struct { } type Cache struct { - ID uint64 `json:"id"` - Repo string `json:"repo"` - Key string `json:"key"` - Version string `json:"version"` - Size int64 `json:"cacheSize"` - Owner string `json:"owner"` // hashed bearer, so a job's own retry is not another job's reservation - Complete bool `json:"complete"` - UsedAt int64 `json:"usedAt"` - CreatedAt int64 `json:"createdAt"` + ID uint64 `json:"id"` + Repo string `json:"repo"` + Key string `json:"key"` + Version string `json:"version"` + Size int64 `json:"cacheSize"` + Owner string `json:"owner"` // hashed bearer, so a job's own retry is not another job's reservation + Complete bool `json:"complete"` + UsedAt int64 `json:"usedAt"` + CreatedAt int64 `json:"createdAt"` + CreatedAtNanos int `json:"createdAtNanos,omitzero"` +} + +func (cache *Cache) createdAtNano() int64 { + return cache.CreatedAt*int64(time.Second) + int64(cache.CreatedAtNanos) } diff --git a/act/artifactcache/s3_storage_test.go b/act/artifactcache/s3_storage_test.go index 90c23748..c449a045 100644 --- a/act/artifactcache/s3_storage_test.go +++ b/act/artifactcache/s3_storage_test.go @@ -8,6 +8,7 @@ import ( "io" "net/http" "net/http/httptest" + "strconv" "strings" "sync" "sync/atomic" @@ -51,14 +52,33 @@ func TestHandler_S3SharesEntriesAndDropsExpiredOnes(t *testing.T) { saver, restorer := startTestHandler(t, opts), startTestHandler(t, opts) entry := map[string]any{"key": "deps", "version": "v1"} + started := time.Now() finalized, _ := saveV2(t, saver, "deps", "v1", []byte("archive")) require.Equal(t, true, finalized["ok"]) assert.EqualValues(t, 1, uploads.Load()) + id, err := strconv.ParseUint(finalized["entry_id"].(string), 10, 64) + require.NoError(t, err) + cache, err := saver.s3.fetch(t.Context(), id) + require.NoError(t, err) + assert.WithinRange(t, time.Unix(cache.CreatedAt, int64(cache.CreatedAtNanos)), started, time.Now()) require.NoError(t, restorer.importMetadata(t.Context())) found := v2Call(t, restorer, testClient, "GetCacheEntryDownloadURL", entry) require.Equal(t, true, found["ok"]) assert.Equal(t, []byte("archive"), getURL(t, found["signed_download_url"].(string))) + for _, cache := range []*Cache{ + {ID: 2, Repo: testRepo, Key: "deps-old", Version: "v1", Complete: true, CreatedAt: cache.CreatedAt}, + {ID: 1, Repo: testRepo, Key: "deps-new", Version: "v1", Complete: true, CreatedAt: cache.CreatedAt, CreatedAtNanos: 1}, + } { + require.NoError(t, saver.s3.publish(cache)) + objects.Store(saver.s3.blobKey(cache.ID), []byte(cache.Key)) + } + require.NoError(t, restorer.importMetadata(t.Context())) + found = v2Call(t, restorer, testClient, "GetCacheEntryDownloadURL", map[string]any{"key": "deps-", "version": "v1"}) + require.Equal(t, true, found["ok"]) + assert.Equal(t, "deps-new", found["matched_key"]) + assert.Equal(t, []byte("deps-new"), getURL(t, found["signed_download_url"].(string))) + objects.Clear() require.NoError(t, restorer.importMetadata(t.Context())) assert.Equal(t, true, v2Call(t, restorer, testClient, "CreateCacheEntry", entry)["ok"]) diff --git a/act/artifacts/server.go b/act/artifacts/server.go index e66af8d9..7df8e3c5 100644 --- a/act/artifacts/server.go +++ b/act/artifacts/server.go @@ -13,6 +13,7 @@ import ( "io/fs" "net" "net/http" + "net/url" "os" "path/filepath" "strings" @@ -84,16 +85,25 @@ func uploads(router *http.ServeMux, baseDir string) { if err := os.MkdirAll(filepath.Dir(safePath), os.ModePerm); err != nil { panic(err) } - flags := os.O_CREATE | os.O_WRONLY | os.O_TRUNC - appendUpload := req.Header.Get("Content-Range") - if appendUpload != "" && !strings.HasPrefix(appendUpload, "bytes 0-") { - flags = os.O_CREATE | os.O_WRONLY | os.O_APPEND + var start int64 + if contentRange := req.Header.Get("Content-Range"); contentRange != "" { + if _, err := fmt.Sscanf(contentRange, "bytes %d-", &start); err != nil || start < 0 { + http.Error(w, "invalid content range", http.StatusBadRequest) + return + } + } + flags := os.O_CREATE | os.O_WRONLY + if start == 0 { + flags |= os.O_TRUNC } file, err := os.OpenFile(safePath, flags, 0o644) if err != nil { panic(err) } defer file.Close() + if _, err := file.Seek(start, io.SeekStart); err != nil { + panic(err) + } if req.Body == nil { panic(errors.New("no body given")) } @@ -120,7 +130,7 @@ func downloads(router *http.ServeMux, baseDir string) { runID := req.PathValue("runId") entries, err := os.ReadDir(safeResolve(baseDir, runID)) - if err != nil { + if err != nil && !os.IsNotExist(err) { panic(err) } @@ -145,6 +155,9 @@ func downloads(router *http.ServeMux, baseDir string) { var files []ContainerItem err := filepath.WalkDir(safePath, func(path string, entry fs.DirEntry, err error) error { + if err != nil { + return err + } if !entry.IsDir() { rel, err := filepath.Rel(safePath, path) if err != nil { @@ -157,12 +170,12 @@ func downloads(router *http.ServeMux, baseDir string) { files = append(files, ContainerItem{ Path: filepath.ToSlash(filepath.Join(itemPath, rel)), ItemType: "file", - ContentLocation: fmt.Sprintf("http://%s/artifact/%s/%s/%s", req.Host, container, itemPath, filepath.ToSlash(rel)), + ContentLocation: (&url.URL{Scheme: "http", Host: req.Host, Path: "/artifact/" + container + "/" + itemPath + "/" + filepath.ToSlash(rel)}).String(), }) } return nil }) - if err != nil { + if err != nil && !os.IsNotExist(err) { panic(err) } diff --git a/act/artifacts/server_test.go b/act/artifacts/server_test.go index 3c2dad23..4be02f23 100644 --- a/act/artifacts/server_test.go +++ b/act/artifacts/server_test.go @@ -48,8 +48,17 @@ func TestArtifactFlow(t *testing.T) { return resp.StatusCode, data } + t.Run("no-artifacts", func(t *testing.T) { + status, data := request(t, http.MethodGet, baseURL+"/_apis/pipelines/workflows/0/artifacts", nil, nil) + require.Equal(t, http.StatusOK, status, string(data)) + var list NamedFileContainerResourceURLResponse + require.NoError(t, json.Unmarshal(data, &list)) + require.Zero(t, list.Count) + require.Empty(t, list.Value) + }) + t.Run("upload-and-download", func(t *testing.T) { - const runID, item, content = "1", "my-artifact/data.txt", "hello artifact\n" + const runID, item, content = "1", "my-artifact/data#1%25.txt", "hello artifact\n" status, data := request(t, http.MethodPost, baseURL+"/_apis/pipelines/workflows/"+runID+"/artifacts", nil, nil) require.Equal(t, http.StatusOK, status, string(data)) @@ -81,13 +90,13 @@ func TestArtifactFlow(t *testing.T) { require.NoError(t, json.Unmarshal(data, &items)) require.Len(t, items.Value, 1) require.Equal(t, "file", items.Value[0].ItemType) - require.Equal(t, "my-artifact/data.txt", items.Value[0].Path) + require.Equal(t, item, items.Value[0].Path) status, data = request(t, http.MethodGet, items.Value[0].ContentLocation, nil, nil) require.Equal(t, http.StatusOK, status) require.Equal(t, content, string(data)) - stored, err := os.ReadFile(filepath.Join(artifactPath, runID, "my-artifact", "data.txt")) + stored, err := os.ReadFile(filepath.Join(artifactPath, runID, filepath.FromSlash(item))) require.NoError(t, err) require.Equal(t, content, string(stored)) }) @@ -95,16 +104,25 @@ func TestArtifactFlow(t *testing.T) { t.Run("content-range", func(t *testing.T) { const rawURL = "/upload/4?itemPath=chunks.txt" status, data := request(t, http.MethodPut, baseURL+rawURL, strings.NewReader("first"), - http.Header{"Content-Range": []string{"bytes 0-4/11"}}) + http.Header{"Content-Range": []string{"bytes 0-4/12"}}) require.Equal(t, http.StatusOK, status, string(data)) - status, data = request(t, http.MethodPut, baseURL+rawURL, strings.NewReader("-second"), - http.Header{"Content-Range": []string{"bytes 5-11/11"}}) - require.Equal(t, http.StatusOK, status, string(data)) + for range 2 { + status, data = request(t, http.MethodPut, baseURL+rawURL, strings.NewReader("-second"), + http.Header{"Content-Range": []string{"bytes 5-11/12"}}) + require.Equal(t, http.StatusOK, status, string(data)) + } stored, err := os.ReadFile(filepath.Join(artifactPath, "4", "chunks.txt")) require.NoError(t, err) require.Equal(t, "first-second", string(stored)) + + status, data = request(t, http.MethodPut, baseURL+rawURL, strings.NewReader(""), + http.Header{"Content-Range": []string{"bytes 0--1/0"}}) + require.Equal(t, http.StatusOK, status, string(data)) + stored, err = os.ReadFile(filepath.Join(artifactPath, "4", "chunks.txt")) + require.NoError(t, err) + require.Empty(t, stored) }) t.Run("gzip-roundtrip", func(t *testing.T) { diff --git a/act/common/executor.go b/act/common/executor.go index 255cc250..2d46a9bd 100644 --- a/act/common/executor.go +++ b/act/common/executor.go @@ -164,7 +164,10 @@ func (e Executor) Finally(finally Executor) Executor { err := e(ctx) err2 := finally(ctx) if err2 != nil { - return fmt.Errorf("error occurred running finally: %v (original error: %v)", err2, err) + if err != nil { + return fmt.Errorf("error occurred running finally: %w (original error: %w)", err2, err) + } + return fmt.Errorf("error occurred running finally: %w (original error: %v)", err2, err) } return err } diff --git a/act/common/executor_test.go b/act/common/executor_test.go index 04153187..ec8ae115 100644 --- a/act/common/executor_test.go +++ b/act/common/executor_test.go @@ -7,6 +7,7 @@ package common import ( "context" "errors" + "fmt" "sync" "sync/atomic" "testing" @@ -134,9 +135,13 @@ func TestExecutorConditionalsAndFinally(t *testing.T) { } func TestExecutorFinallyReturnsFinallyErrorWithOriginal(t *testing.T) { - mainErr := errors.New("main failed") finalErr := errors.New("cleanup failed") - - err := NewErrorExecutor(mainErr).Finally(NewErrorExecutor(finalErr))(t.Context()) - require.EqualError(t, err, "error occurred running finally: cleanup failed (original error: main failed)") + for _, mainErr := range []error{context.Canceled, nil} { + err := NewErrorExecutor(mainErr).Finally(NewErrorExecutor(finalErr))(t.Context()) + require.EqualError(t, err, fmt.Sprintf("error occurred running finally: cleanup failed (original error: %v)", mainErr)) + require.ErrorIs(t, err, finalErr) + if mainErr != nil { + require.ErrorIs(t, err, mainErr) + } + } } diff --git a/act/container/docker_build.go b/act/container/docker_build.go index 06cd3697..8dcb3137 100644 --- a/act/container/docker_build.go +++ b/act/container/docker_build.go @@ -74,8 +74,10 @@ func NewDockerBuildExecutor(input NewDockerBuildExecutorInput) common.Executor { logger.Debugf("Creating image from context dir '%s' with tag '%s' and platform '%s'", input.ContextDir, input.ImageTag, input.Platform) resp, err := cli.ImageBuild(ctx, buildContext, options) - - return logDockerResponse(logger, resp.Body, err != nil) + if err != nil { + return err + } + return logDockerResponse(logger, resp.Body, false) } } diff --git a/act/container/docker_build_test.go b/act/container/docker_build_test.go index a1843fd4..2546880d 100644 --- a/act/container/docker_build_test.go +++ b/act/container/docker_build_test.go @@ -8,13 +8,35 @@ package container import ( "archive/tar" "io" + "net/http" + "net/http/httptest" "os" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/require" ) +func TestDockerBuildReportsDaemonError(t *testing.T) { + daemon := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) { + writer.Header().Set("API-Version", "1.47") + if strings.HasSuffix(request.URL.Path, "/build") { + writer.Header().Set("Content-Type", "application/json") + writer.WriteHeader(http.StatusInternalServerError) + _, _ = io.WriteString(writer, `{"message":"build rejected"}`) + } + })) + t.Cleanup(daemon.Close) + t.Setenv("DOCKER_HOST", daemon.URL) + t.Setenv("DOCKER_CONFIG", t.TempDir()) + t.Setenv("DOCKER_AUTH_CONFIG", "") + + require.ErrorContains(t, NewDockerBuildExecutor(NewDockerBuildExecutorInput{ + ImageTag: "action:test", Dockerfile: "Dockerfile", BuildContext: strings.NewReader(""), + })(t.Context()), "build rejected") +} + func TestCreateBuildContextFollowsDockerignoreAndSymlinkedRootButKeepsDockerfile(t *testing.T) { dir := t.TempDir() for name, content := range map[string]string{ diff --git a/act/container/docker_logger.go b/act/container/docker_logger.go index 5baeb08c..cd22828e 100644 --- a/act/container/docker_logger.go +++ b/act/container/docker_logger.go @@ -33,6 +33,7 @@ func logDockerResponse(logger *logging.Logger, dockerResponse io.ReadCloser, isE defer dockerResponse.Close() scanner := bufio.NewScanner(dockerResponse) + scanner.Buffer(nil, 64*1024*1024) // a build echoes long instructions in one record for scanner.Scan() { line := scanner.Bytes() var msg dockerMessage @@ -65,7 +66,7 @@ func logDockerResponse(logger *logging.Logger, dockerResponse io.ReadCloser, isE } } - return nil + return scanner.Err() } func writeLog(logger *logging.Logger, isError bool, format string, args ...any) { diff --git a/act/container/docker_pull.go b/act/container/docker_pull.go index 3204dcc1..9b585461 100644 --- a/act/container/docker_pull.go +++ b/act/container/docker_pull.go @@ -114,7 +114,7 @@ func getImagePullOptions(ctx context.Context, input NewDockerPullExecutorInput) if err != nil { return imagePullOptions, err } - if authConfig.Username == "" && authConfig.Password == "" { + if authConfig.Username == "" && authConfig.Password == "" && authConfig.IdentityToken == "" && authConfig.RegistryToken == "" { return imagePullOptions, nil } logger.Info("using DockerAuthConfig authentication for docker pull") diff --git a/act/container/docker_pull_test.go b/act/container/docker_pull_test.go index 0b3d5f24..55e73f52 100644 --- a/act/container/docker_pull_test.go +++ b/act/container/docker_pull_test.go @@ -6,12 +6,17 @@ package container import ( "io" + "os" + "path/filepath" "strings" "testing" + "testing/iotest" "gitea.com/gitea/runner/act/common/logging" "gitea.com/gitea/runner/act/common/logging/logtest" + "github.com/moby/moby/api/pkg/authconfig" + "github.com/moby/moby/api/types/registry" assert "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -64,6 +69,21 @@ func TestGetImagePullOptions(t *testing.T) { }) assert.NoError(t, err, "Failed to create ImagePullOptions") //nolint:testifylint // pre-existing issue from nektos/act assert.Equal(t, "eyJ1c2VybmFtZSI6InVzZXJuYW1lIiwicGFzc3dvcmQiOiJwYXNzd29yZFxuIiwic2VydmVyYWRkcmVzcyI6Imh0dHBzOi8vaW5kZXguZG9ja2VyLmlvL3YxLyJ9", options.RegistryAuth, "RegistryAuth should be taken from local docker config") + + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "config.json"), []byte(`{"auths":{"identity.example.com":{"identitytoken":"identity-token"},"registry.example.com":{"registrytoken":"registry-token"}}}`), 0o600)) + t.Setenv("DOCKER_CONFIG", dir) + for host, want := range map[string]registry.AuthConfig{ + "identity.example.com": {IdentityToken: "identity-token", ServerAddress: "identity.example.com"}, + "registry.example.com": {RegistryToken: "registry-token", ServerAddress: "registry.example.com"}, + } { + options, err := getImagePullOptions(ctx, NewDockerPullExecutorInput{Image: host + "/app"}) + require.NoError(t, err) + require.NotEmpty(t, options.RegistryAuth, host) + got, err := authconfig.Decode(options.RegistryAuth) + require.NoError(t, err) + assert.Equal(t, want, *got) + } } // A digest-pinned image is immutable, so its local copy is always current. @@ -82,4 +102,8 @@ func TestLogDockerResponseError(t *testing.T) { require.EqualError(t, streamErr(`{"error":"toomanyrequests: rate limit exceeded"}`), "toomanyrequests: rate limit exceeded") require.EqualError(t, streamErr(`{"errorDetail":{"message":"unexpected EOF"}}`), "unexpected EOF") require.NoError(t, streamErr(`{"status":"Downloading"}`)) + require.NoError(t, streamErr(`{"stream":"`+strings.Repeat("x", 70000)+`"}`)) + require.ErrorIs(t, logDockerResponse(logger, io.NopCloser(io.MultiReader( + strings.NewReader(`{"status":"Downloading"}`+"\n"), iotest.ErrReader(io.ErrUnexpectedEOF), + )), false), io.ErrUnexpectedEOF) } diff --git a/act/container/docker_run.go b/act/container/docker_run.go index a4c3f248..29b4c2c3 100644 --- a/act/container/docker_run.go +++ b/act/container/docker_run.go @@ -205,18 +205,30 @@ func (cr *containerReference) DumpLogs(ctx context.Context) error { if cr.id == "" { return cr.missingContainerError("read its logs") } + tty, err := cr.containerTTY(ctx) + if err != nil { + return err + } logs, err := cr.cli.ContainerLogs(ctx, cr.id, client.ContainerLogsOptions{ShowStdout: true, ShowStderr: true}) if err != nil { return err } defer logs.Close() - return cr.copyOutput(logs) + return cr.copyOutput(logs, tty) +} + +func (cr *containerReference) containerTTY(ctx context.Context) (bool, error) { + inspect, err := cr.cli.ContainerInspect(ctx, cr.id, client.ContainerInspectOptions{}) + if err != nil { + return false, fmt.Errorf("failed to inspect container: %w", err) + } + return inspect.Container.Config != nil && inspect.Container.Config.Tty, nil } // copyOutput writes a container stream to the writers the container was created with, // demultiplexing it unless the container has a TTY, which sends a single raw stream. -func (cr *containerReference) copyOutput(stream io.Reader) error { +func (cr *containerReference) copyOutput(stream io.Reader, tty bool) error { outWriter := cr.input.Stdout if outWriter == nil { outWriter = os.Stdout @@ -227,7 +239,7 @@ func (cr *containerReference) copyOutput(stream io.Reader) error { } var err error - if !cr.input.AllocatePTY || os.Getenv("NORAW") != "" { + if !tty || os.Getenv("NORAW") != "" { _, err = stdcopy.StdCopy(outWriter, errWriter, stream) } else { _, err = io.Copy(outWriter, stream) @@ -296,7 +308,7 @@ func containerInfoFromInspect(inspect container.InspectResponse) *Info { } func (cr *containerReference) UpdateFromEnv(srcPath string, env *map[string]string) common.Executor { - return parseEnvFile(cr, srcPath, env).IfNot(common.Dryrun) + return parseEnvFile(cr, srcPath, env, false).IfNot(common.Dryrun) } func (cr *containerReference) UpdateFromImageEnv(env *map[string]string) common.Executor { @@ -499,7 +511,7 @@ func (cr *containerReference) remove() common.Executor { if idOrName == "" && cr.input != nil { idOrName = cr.input.Name } - if idOrName == "" { + if idOrName == "" || cr.cli == nil { return nil } @@ -851,7 +863,7 @@ func (cr *containerReference) waitForCommand(ctx context.Context, resp client.Hi cmdResponse := make(chan error, 1) go func() { - cmdResponse <- cr.copyOutput(resp.Reader) + cmdResponse <- cr.copyOutput(resp.Reader, cr.input.AllocatePTY) }() select { @@ -876,11 +888,7 @@ func (cr *containerReference) waitForCommand(ctx context.Context, resp client.Hi // from executing return ctx.Err() case err := <-cmdResponse: - if err != nil { - logger.Error(err) - } - - return nil + return err } } @@ -897,7 +905,7 @@ func (cr *containerReference) copyDir(dstPath, srcPath string, useGitIgnore, ski } logger.Debugf("Writing tarball %s from %s", tarFile.Name(), srcPath) defer func() { - if err := tarFile.Close(); !errors.Is(err, os.ErrClosed) { + if err := tarFile.Close(); err != nil && !errors.Is(err, os.ErrClosed) { logger.Error(err) } if err := os.Remove(tarFile.Name()); err != nil { @@ -944,6 +952,10 @@ func (cr *containerReference) upload(ctx context.Context, dstPath string, conten func (cr *containerReference) attach() common.Executor { return func(ctx context.Context) error { + tty, err := cr.containerTTY(ctx) + if err != nil { + return err + } out, err := cr.cli.ContainerAttach(ctx, cr.id, client.ContainerAttachOptions{ Stream: true, Stdout: true, @@ -954,9 +966,12 @@ func (cr *containerReference) attach() common.Executor { } done := make(chan struct{}) cr.attachDone = done + stopClose := context.AfterFunc(ctx, out.Close) go func() { defer close(done) - if copyErr := cr.copyOutput(out.Reader); copyErr != nil { + defer out.Close() + defer stopClose() + if copyErr := cr.copyOutput(out.Reader, tty); copyErr != nil && ctx.Err() == nil { common.Logger(ctx).Error(copyErr) } }() @@ -991,6 +1006,9 @@ func (cr *containerReference) wait() common.Executor { return fmt.Errorf("failed to wait for container: %w", err) } case status := <-waitResult.Result: + if status.Error != nil { + return fmt.Errorf("failed to wait for container: %s", status.Error.Message) + } statusCode = status.StatusCode } diff --git a/act/container/docker_run_test.go b/act/container/docker_run_test.go index 8f499751..fc1a4081 100644 --- a/act/container/docker_run_test.go +++ b/act/container/docker_run_test.go @@ -92,6 +92,16 @@ func (m *mockDockerClient) ContainerAttach(ctx context.Context, containerID stri return args.Get(0).(mobyclient.ContainerAttachResult), args.Error(1) } +func (m *mockDockerClient) ContainerLogs(ctx context.Context, containerID string, opts mobyclient.ContainerLogsOptions) (mobyclient.ContainerLogsResult, error) { + args := m.Called(ctx, containerID, opts) + return args.Get(0).(mobyclient.ContainerLogsResult), args.Error(1) +} + +func (m *mockDockerClient) ContainerStart(ctx context.Context, containerID string, opts mobyclient.ContainerStartOptions) (mobyclient.ContainerStartResult, error) { + args := m.Called(ctx, containerID, opts) + return args.Get(0).(mobyclient.ContainerStartResult), args.Error(1) +} + func (m *mockDockerClient) ContainerWait(ctx context.Context, containerID string, opts mobyclient.ContainerWaitOptions) mobyclient.ContainerWaitResult { args := m.Called(ctx, containerID, opts) return args.Get(0).(mobyclient.ContainerWaitResult) @@ -158,6 +168,7 @@ func (r *interruptReader) Read(_ []byte) (int, error) { type mockConn struct { net.Conn mock.Mock + closed atomic.Bool } func (m *mockConn) Write(b []byte) (n int, err error) { @@ -166,6 +177,7 @@ func (m *mockConn) Write(b []byte) (n int, err error) { } func (m *mockConn) Close() (err error) { + m.closed.Store(true) return nil } @@ -236,6 +248,21 @@ func TestDockerExecFailure(t *testing.T) { client.AssertExpectations(t) } +func TestDockerExecStreamFailure(t *testing.T) { + ctx := t.Context() + client := &mockDockerClient{} + client.On("ExecCreate", ctx, "123", mock.AnythingOfType("client.ExecCreateOptions")). + Return(mobyclient.ExecCreateResult{ID: "id"}, nil) + client.On("ExecAttach", ctx, "id", mock.AnythingOfType("client.ExecAttachOptions")). + Return(mobyclient.ExecAttachResult{ + Conn: &mockConn{}, + Reader: bufio.NewReader(bytes.NewReader(stdcopyFrame(stdcopy.Systemerr, "unable to start exec"))), + }, nil) + require.ErrorContains(t, (&containerReference{id: "123", cli: client, input: &NewContainerInput{}}). + exec([]string{"sh"}, nil, "", "")(ctx), "unable to start exec") + client.AssertExpectations(t) +} + // stdcopyFrame wraps payload in a single Docker multiplexed-stream frame, the // format StdCopy expects: an 8-byte header (stream type + 4-byte big-endian // length) followed by the payload. @@ -248,60 +275,88 @@ func stdcopyFrame(stream stdcopy.StdType, payload string) []byte { } func TestDockerAttachFlushesTrailingLine(t *testing.T) { - ctx := t.Context() + for _, testcase := range []struct { + name string + tty bool + }{ + {name: "multiplexed"}, + {name: "tty from container options", tty: true}, + } { + t.Run(testcase.name, func(t *testing.T) { + ctx := t.Context() + output := []byte("line one\nlast line without newline") + if !testcase.tty { + output = stdcopyFrame(stdcopy.Stdout, string(output)) + } + var lines []string + logWriter := common.NewLineWriter(func(line string) bool { + lines = append(lines, line) + return true + }) + client := &mockDockerClient{} + client.On("ContainerInspect", ctx, "123", mobyclient.ContainerInspectOptions{}). + Return(mobyclient.ContainerInspectResult{Container: container.InspectResponse{Config: &container.Config{Tty: testcase.tty}}}, nil) + conn := &mockConn{} + client.On("ContainerAttach", ctx, "123", mock.AnythingOfType("client.ContainerAttachOptions")). + Return(mobyclient.ContainerAttachResult{ + Conn: conn, + Reader: bufio.NewReader(bytes.NewReader(output)), + }, nil) + client.On("ContainerLogs", ctx, "123", mobyclient.ContainerLogsOptions{ShowStdout: true, ShowStderr: true}). + Return(io.NopCloser(bytes.NewReader(output)), nil) + statusCh := make(chan container.WaitResponse, 1) + statusCh <- container.WaitResponse{StatusCode: 0} + client.On("ContainerWait", ctx, "123", mobyclient.ContainerWaitOptions{Condition: container.WaitConditionNotRunning}). + Return(mobyclient.ContainerWaitResult{Result: statusCh}) + cr := &containerReference{id: "123", cli: client, input: &NewContainerInput{Stdout: logWriter, Stderr: logWriter}} - framed := bytes.NewBuffer(stdcopyFrame(stdcopy.Stdout, "line one\nlast line without newline")) - - var lines []string - logWriter := common.NewLineWriter(func(s string) bool { - lines = append(lines, s) - return true - }) - - client := &mockDockerClient{} - client.On("ContainerAttach", ctx, "123", mock.AnythingOfType("client.ContainerAttachOptions")). - Return(mobyclient.ContainerAttachResult{ - Conn: &mockConn{}, - Reader: bufio.NewReader(framed), - }, nil) - - statusCh := make(chan container.WaitResponse, 1) - statusCh <- container.WaitResponse{StatusCode: 0} - client.On("ContainerWait", ctx, "123", mobyclient.ContainerWaitOptions{Condition: container.WaitConditionNotRunning}). - Return(mobyclient.ContainerWaitResult{Result: statusCh}) - - cr := &containerReference{ - id: "123", - cli: client, - input: &NewContainerInput{ - Stdout: logWriter, - Stderr: logWriter, - }, + require.NoError(t, cr.attach()(ctx)) + require.NoError(t, cr.wait()(ctx)) + assert.Equal(t, []string{"line one\n", "last line without newline"}, lines) + assert.True(t, conn.closed.Load()) + require.NoError(t, cr.DumpLogs(ctx)) + assert.Equal(t, []string{"line one\n", "last line without newline", "line one\n", "last line without newline"}, lines) + client.AssertExpectations(t) + }) } +} - require.NoError(t, cr.attach()(ctx)) - require.NoError(t, cr.wait()(ctx)) - - assert.Equal(t, []string{"line one\n", "last line without newline"}, lines) +func TestDockerAttachStreamClosesWithItsContext(t *testing.T) { + conn, peer := net.Pipe() + t.Cleanup(func() { _ = conn.Close() }) + t.Cleanup(func() { _ = peer.Close() }) + client := &mockDockerClient{} + client.On("ContainerInspect", mock.Anything, "123", mobyclient.ContainerInspectOptions{}). + Return(mobyclient.ContainerInspectResult{Container: container.InspectResponse{Config: &container.Config{}}}, nil) + client.On("ContainerAttach", mock.Anything, "123", mock.AnythingOfType("client.ContainerAttachOptions")). + Return(mobyclient.ContainerAttachResult{HijackedResponse: mobyclient.NewHijackedResponse(conn, "")}, nil) + client.On("ContainerStart", mock.Anything, "123", mobyclient.ContainerStartOptions{}). + Return(mobyclient.ContainerStartResult{}, errors.New("container start failed")) + cr := &containerReference{id: "123", cli: client, input: &NewContainerInput{}} + ctx, cancel := context.WithCancel(t.Context()) + require.ErrorContains(t, cr.Start(true)(ctx), "container start failed") + cancel() + <-cr.attachDone client.AssertExpectations(t) } func TestDockerWaitFailure(t *testing.T) { ctx := t.Context() - statusCh := make(chan container.WaitResponse, 1) statusCh <- container.WaitResponse{StatusCode: 2} client := &mockDockerClient{} client.On("ContainerWait", ctx, "123", mobyclient.ContainerWaitOptions{Condition: container.WaitConditionNotRunning}). Return(mobyclient.ContainerWaitResult{Result: statusCh}) + cr := &containerReference{id: "123", cli: client} - err := (&containerReference{id: "123", cli: client}).wait()(ctx) + err := cr.wait()(ctx) var exitErr ExitCodeError require.ErrorAs(t, err, &exitErr) assert.Equal(t, ExitCodeError(2), exitErr) assert.Equal(t, "Process completed with exit code 2.", err.Error()) - + statusCh <- container.WaitResponse{Error: &container.WaitExitError{Message: "container wait failed"}} + require.EqualError(t, cr.wait()(ctx), "failed to wait for container: container wait failed") client.AssertExpectations(t) } @@ -370,6 +425,13 @@ func TestRemoveWithoutIDUsesName(t *testing.T) { client.AssertExpectations(t) } +func TestRemovePreservesClientCreationError(t *testing.T) { + t.Setenv("DOCKER_HOST", "://") + + require.ErrorContains(t, (&containerReference{input: &NewContainerInput{Name: "job-1"}}). + Remove()(t.Context()), "failed to connect to docker daemon") +} + func TestFindRevalidatesStaleID(t *testing.T) { ctx := t.Context() inspectOpts := mobyclient.ContainerInspectOptions{} @@ -477,7 +539,8 @@ func TestPublicCopyPipelineHandlesStaleID(t *testing.T) { {name: "sibling prefix", directory: true, destination: "/var/run/actor/action", extractionRoot: "/", entry: "var/run/actor/action/x"}, } { t.Run(testcase.name, func(t *testing.T) { - ctx := t.Context() + hook := logtest.NewWithLevel(logging.LevelError) + ctx := common.WithLogger(t.Context(), logging.New(hook)) client := &mockDockerClient{} client.On("ContainerInspect", ctx, "stale", mobyclient.ContainerInspectOptions{}). Return(mobyclient.ContainerInspectResult{}, cerrdefs.ErrNotFound.WithMessage("gone")) @@ -501,6 +564,7 @@ func TestPublicCopyPipelineHandlesStaleID(t *testing.T) { require.NoError(t, cr.Copy(testcase.destination, &FileEntry{Name: "x", Mode: 0o644})(ctx)) } assert.Equal(t, "fresh", cr.id) + assert.Empty(t, hook.AllRecords()) client.AssertExpectations(t) }) } diff --git a/act/container/host_environment.go b/act/container/host_environment.go index 842daa54..e35ede3f 100644 --- a/act/container/host_environment.go +++ b/act/container/host_environment.go @@ -318,21 +318,21 @@ func (e *HostEnvironment) exec(ctx context.Context, command []string, cmdline st defer k.Close() } err = treeKill.Wait(ctx, cmd) - if err != nil { - if exitErr, ok := errors.AsType[*exec.ExitError](err); ok { - return ExitCodeError(exitErr.ExitCode()) - } - return err - } if tty != nil { writer.AutoStop.Store(true) if _, err := tty.WriteString("\x04"); err != nil { common.Logger(ctx).Debug("Failed to write EOT") } - <-logctx.Done() + select { + case <-logctx.Done(): + case <-ctx.Done(): // a cancelled step does not wait on output nobody consumes + } ppty.Close() ppty = nil } + if exitErr, ok := errors.AsType[*exec.ExitError](err); ok { + return ExitCodeError(exitErr.ExitCode()) + } return err } @@ -355,7 +355,7 @@ func (e *HostEnvironment) ExecWithCmdLine(command []string, cmdline string, env } func (e *HostEnvironment) UpdateFromEnv(srcPath string, env *map[string]string) common.Executor { - return parseEnvFile(e, srcPath, env) + return parseEnvFile(e, srcPath, env, runtime.GOOS == "windows") } // removeAll is a var so tests can substitute a blocking stub. diff --git a/act/container/host_environment_test.go b/act/container/host_environment_test.go index 97497b79..be525b32 100644 --- a/act/container/host_environment_test.go +++ b/act/container/host_environment_test.go @@ -82,14 +82,36 @@ func TestHostEnvironmentExecExitCode(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("uses POSIX shell") } - ctx := t.Context() - e := &HostEnvironment{Path: t.TempDir(), StdOut: io.Discard} + for name, allocatePTY := range map[string]bool{"without PTY": false, "with PTY": true} { + t.Run(name, func(t *testing.T) { + reader, writer := io.Pipe() + defer reader.Close() + e := &HostEnvironment{Path: t.TempDir(), StdOut: writer, AllocatePTY: allocatePTY} + t.Cleanup(func() { assert.NoError(t, e.Remove()(t.Context())) }) - err := e.Exec([]string{"sh", "-c", "exit 3"}, map[string]string{"PATH": os.Getenv("PATH")}, "", "")(ctx) - var exitErr ExitCodeError - require.ErrorAs(t, err, &exitErr) - assert.Equal(t, ExitCodeError(3), exitErr) - assert.Equal(t, "Process completed with exit code 3.", err.Error()) + result := make(chan error, 1) + go func() { + defer writer.Close() + result <- e.Exec([]string{"sh", "-c", "printf 'failed\\n'; exit 3"}, map[string]string{"PATH": os.Getenv("PATH")}, "", "")(t.Context()) + }() + first := make([]byte, 1) + _, err := io.ReadFull(reader, first) + require.NoError(t, err) + select { + case err := <-result: + t.Fatalf("Exec returned before its output was consumed: %v", err) + case <-time.After(100 * time.Millisecond): + } + remaining, err := io.ReadAll(reader) + require.NoError(t, err) + err = <-result + var exitErr ExitCodeError + require.ErrorAs(t, err, &exitErr) + assert.Equal(t, ExitCodeError(3), exitErr) + assert.Equal(t, "Process completed with exit code 3.", err.Error()) + assert.Equal(t, "failed\n", string(first)+string(remaining)) + }) + } } func TestHostEnvironmentAllocatePTY(t *testing.T) { diff --git a/act/container/kubernetes.go b/act/container/kubernetes.go index d3f0bb8a..5c5bd186 100644 --- a/act/container/kubernetes.go +++ b/act/container/kubernetes.go @@ -500,7 +500,7 @@ func (c *kubernetesContainer) GetContainerArchive(ctx context.Context, srcPath s } func (c *kubernetesContainer) UpdateFromEnv(srcPath string, env *map[string]string) common.Executor { - return parseEnvFile(c, srcPath, env) + return parseEnvFile(c, srcPath, env, false) } func (c *kubernetesContainer) UpdateFromImageEnv(env *map[string]string) common.Executor { diff --git a/act/container/parse_env_file.go b/act/container/parse_env_file.go index 01064580..d7c4cf6b 100644 --- a/act/container/parse_env_file.go +++ b/act/container/parse_env_file.go @@ -18,7 +18,8 @@ import ( "golang.org/x/text/transform" ) -func parseEnvFile(e Container, srcPath string, env *map[string]string) common.Executor { +// parseEnvFile reads a GITHUB_ENV style file, splitting lines like GitHub's runner on Windows or elsewhere. +func parseEnvFile(e Container, srcPath string, env *map[string]string, windows bool) common.Executor { localEnv := *env return func(ctx context.Context) error { envTar, err := e.GetContainerArchive(ctx, srcPath) @@ -38,8 +39,15 @@ func parseEnvFile(e Container, srcPath string, env *map[string]string) common.Ex s := bufio.NewScanner(decoded) // Default 64 KiB max token size is too small for realistic env-file lines; allow up to 16 MiB. s.Buffer(make([]byte, 0, 64*1024), 16*1024*1024) + s.Split(func(data []byte, atEOF bool) (int, []byte, error) { + advance, token, err := bufio.ScanLines(data, atEOF) + if token != nil { + return advance, data[:advance], err + } + return advance, token, err + }) for s.Scan() { - line := s.Text() + line := trimEnvLineEnding(s.Text(), windows) // GitHub's runner ignores blank lines if strings.TrimSpace(line) == "" { continue @@ -52,10 +60,13 @@ func parseEnvFile(e Container, srcPath string, env *map[string]string) common.Ex case multiLineEnv != -1: var multiLineEnvContent []string multiLineEnvDelimiter := line[multiLineEnv+2:] + if multiLineEnv == 0 || multiLineEnvDelimiter == "" { + return fmt.Errorf("invalid format '%v', name and delimiter must not be empty", line) + } delimiterFound := false for s.Scan() { content := s.Text() - if content == multiLineEnvDelimiter { + if trimEnvLineEnding(content, windows) == multiLineEnvDelimiter { delimiterFound = true break } @@ -67,7 +78,7 @@ func parseEnvFile(e Container, srcPath string, env *map[string]string) common.Ex if !delimiterFound { return fmt.Errorf("invalid format delimiter '%v' not found before end of file", multiLineEnvDelimiter) } - localEnv[line[:multiLineEnv]] = strings.Join(multiLineEnvContent, "\n") + localEnv[line[:multiLineEnv]] = trimEnvLineEnding(strings.Join(multiLineEnvContent, ""), windows) default: return fmt.Errorf("invalid format '%v', expected a line with '=' or '<<'", line) } @@ -78,3 +89,10 @@ func parseEnvFile(e Container, srcPath string, env *map[string]string) common.Ex return nil } } + +func trimEnvLineEnding(line string, windows bool) string { + if windows && strings.HasSuffix(line, "\r\n") { + return line[:len(line)-2] + } + return strings.TrimSuffix(line, "\n") +} diff --git a/act/container/parse_env_file_test.go b/act/container/parse_env_file_test.go index 4f49375a..700b5e99 100644 --- a/act/container/parse_env_file_test.go +++ b/act/container/parse_env_file_test.go @@ -33,23 +33,36 @@ func TestParseEnvFile(t *testing.T) { want map[string]string }{ {"single line", "FOO=bar\nBAZ=qux\n", map[string]string{"FOO": "bar", "BAZ": "qux"}}, + {"single line CRLF and final bare CR", "FOO=bar\r\nBAZ=qux\r", map[string]string{"FOO": "bar\r", "BAZ": "qux\r"}}, + {"single line without final newline", "FOO=bar", map[string]string{"FOO": "bar"}}, {"blank lines", "\nFOO=bar\n\n \nBAZ=qux\n\n", map[string]string{"FOO": "bar", "BAZ": "qux"}}, {"multiline", "FOO< 0 { + if fields := strings.Fields(stepInput(step.getStepModel(), "entrypoint")); len(fields) > 0 { return fields, nil } } @@ -413,9 +424,15 @@ func evalDockerEnv(ctx context.Context, step step, action *model.Action) (*expre var err error inputs := make(map[string]string) + providedInputs := make(map[string]bool, len(step.getStepModel().With)) + for key := range step.getStepModel().With { + providedInputs[strings.ToLower(key)] = true + } eval := rc.NewExpressionEvaluator(ctx) - // Set Defaults for k, input := range action.Inputs { + if providedInputs[strings.ToLower(k)] { + continue + } if inputs[k], err = eval.Interpolate(ctx, input.Default); err != nil { return nil, fmt.Errorf("unable to interpolate the default of input %s: %w", k, err) } @@ -493,6 +510,47 @@ func populateEnvsFromInput(ctx context.Context, env *map[string]string, action * return nil } +// lookupFold finds name in values, ignoring case where the environment does, as GitHub's inputs always do. +func lookupFold[V any](values map[string]V, name string, caseInsensitive bool) (V, bool) { + if value, ok := values[name]; ok || !caseInsensitive { + return value, ok + } + for key, value := range values { + if strings.EqualFold(key, name) { + return value, true + } + } + var zero V + return zero, false +} + +func stepInput(step *model.Step, name string) string { + value, _ := lookupFold(step.With, name, true) + return value +} + +func warnUnexpectedInputs(ctx context.Context, action *model.Action, inputs map[string]string) { + docker := action.Runs.Using.IsDocker() + var unexpectedInputs []string + for input := range inputs { + _, declared := lookupFold(action.Inputs, input, true) + containerInput := docker && (strings.EqualFold(input, "entryPoint") || strings.EqualFold(input, "args")) + if !declared && !containerInput { + unexpectedInputs = append(unexpectedInputs, input) + } + } + if len(unexpectedInputs) == 0 { + return + } + validInputs := slices.Sorted(maps.Keys(action.Inputs)) + if docker { + validInputs = append([]string{"entryPoint", "args"}, validInputs...) + } + slices.Sort(unexpectedInputs) + common.Logger(ctx).With(rawOutputField, true).Warnf("##[warning]%s", EscapeCommandData(fmt.Sprintf( + "Unexpected input(s) '%s', valid inputs are ['%s']", strings.Join(unexpectedInputs, "', '"), strings.Join(validInputs, "', '")))) +} + func getContainerActionPaths(step actionStep) (string, string) { rc := step.getRunContext() if sar, ok := step.(*stepActionRemote); ok { // a copy per action, as its repository's directory is shared @@ -633,6 +691,9 @@ func runPreStep(step actionStep) common.Executor { func shouldRunPostStep(step actionStep) common.Conditional { return func(ctx context.Context) bool { + if remote, ok := step.(*stepActionRemote); ok && remote.preStepStarted && !remote.action.Runs.Using.IsComposite() { + return true + } log := common.Logger(ctx) stepResults := step.getRunContext().StepResults stepResult := stepResults[step.getStepModel().ID] diff --git a/act/runner/action_composite.go b/act/runner/action_composite.go index 8f4f1d48..a36b3a7c 100644 --- a/act/runner/action_composite.go +++ b/act/runner/action_composite.go @@ -52,6 +52,11 @@ func evaluateCompositeInputAndEnv(ctx context.Context, parent *RunContext, step } } } + for envKey, value := range (&model.Step{With: step.getStepModel().With}).GetEnv() { + if _, ok := env[envKey]; !ok { + env[envKey] = value + } + } gh := step.getGithubContext(ctx) env["GITHUB_ACTION_REPOSITORY"] = gh.ActionRepository env["GITHUB_ACTION_REF"] = gh.ActionRef diff --git a/act/runner/action_composite_test.go b/act/runner/action_composite_test.go index 6d12af1b..5fd7df75 100644 --- a/act/runner/action_composite_test.go +++ b/act/runner/action_composite_test.go @@ -7,7 +7,9 @@ import ( "context" "testing" + "gitea.com/gitea/runner/act/common" "gitea.com/gitea/runner/act/common/git" + "gitea.com/gitea/runner/act/common/logging/logtest" "gitea.dev/actionslib/pkg/model" "github.com/stretchr/testify/assert" @@ -17,10 +19,11 @@ import ( func TestCompositeActionParity(t *testing.T) { t.Run("inherits contexts without leaking inputs", func(t *testing.T) { - ctx := t.Context() + logger, hook := logtest.NewLogger() + ctx := common.WithLogger(t.Context(), logger) strategy := &model.Strategy{MaxParallelString: "3", RawMatrix: yaml.Node{Kind: yaml.MappingNode}} parent := &RunContext{ - Config: &Config{}, + Config: &Config{PresetGitHubContext: &model.GithubContext{}}, Matrix: map[string]any{"os": "linux"}, Run: &model.Run{JobID: "job", Workflow: &model.Workflow{Name: "workflow", Jobs: map[string]*model.Job{"job": {Strategy: strategy}}}}, JobContainer: &jobContainerMock{}, @@ -28,21 +31,26 @@ func TestCompositeActionParity(t *testing.T) { jobIndex: 1, jobTotal: 4, } - composite, err := newCompositeRunContext(ctx, parent, &stepActionRemote{ - Step: &model.Step{With: map[string]string{"SHARED": "outer"}}, + step := &stepActionLocal{ + Step: &model.Step{Uses: "./action", With: map[string]string{"SHARED": "outer", "extra": "literal${{ github.job }}"}}, RunContext: parent, - action: &model.Action{Inputs: map[string]model.Input{"shared": {Default: "outer-default"}}}, + action: &model.Action{Inputs: map[string]model.Input{"shared": {Default: "outer-default"}}, Runs: model.ActionRuns{Using: "composite"}}, env: map[string]string{"INPUT_SHARED": "outer"}, - }, "/action") + } + require.NoError(t, runActionImpl(step, "/action", nil)(ctx)) + require.Len(t, hook.AllRecords(), 1) + assert.Equal(t, "##[warning]Unexpected input(s) 'extra', valid inputs are ['shared']", hook.LastRecord().Message) + composite, err := step.getCompositeRunContext(ctx) require.NoError(t, err) assert.Same(t, strategy, composite.Run.Job().Strategy) assert.True(t, composite.IsHostEnv()) interpolated, err := composite.NewExpressionEvaluator(ctx).Interpolate(ctx, - "${{ matrix.os }}|${{ strategy.max-parallel }}|${{ strategy.job-index }}|${{ inputs.shared }}") + "${{ matrix.os }}|${{ strategy.max-parallel }}|${{ strategy.job-index }}|${{ inputs.shared }}|${{ inputs.extra }}") require.NoError(t, err) - assert.Equal(t, "linux|3|1|outer", interpolated) + assert.Equal(t, "linux|3|1|outer|literal${{ github.job }}", interpolated) assert.NotContains(t, composite.Env, "INPUT_SHARED") + assert.NotContains(t, composite.Env, "INPUT_EXTRA") nestedEnv := composite.GetEnv() require.NoError(t, populateEnvsFromInput(ctx, &nestedEnv, &model.Action{Inputs: map[string]model.Input{"shared": {Default: "inner-default"}}}, composite)) diff --git a/act/runner/action_test.go b/act/runner/action_test.go index ddccb2d5..ea2c3013 100644 --- a/act/runner/action_test.go +++ b/act/runner/action_test.go @@ -19,6 +19,7 @@ import ( "gitea.com/gitea/runner/act/common" "gitea.com/gitea/runner/act/common/git" + "gitea.com/gitea/runner/act/common/logging/logtest" "gitea.com/gitea/runner/act/container" "gitea.dev/actionslib/pkg/model" @@ -85,6 +86,19 @@ runs: }, }, }, + { + name: "readLowercaseDockerfile", + step: &model.Step{}, + filename: "dockerfile", + fileContent: "FROM ubuntu:20.04", + expected: &model.Action{ + Name: "(Synthetic)", + Runs: model.ActionRuns{ + Using: "docker", + Image: "dockerfile", + }, + }, + }, { name: "readWithArgs", step: &model.Step{ @@ -95,6 +109,7 @@ runs: expected: &model.Action{ Name: "(Synthetic)", Inputs: map[string]model.Input{ + "args": {}, "cwd": { Description: "(Actual working directory)", Required: false, @@ -147,14 +162,33 @@ func TestActionRunner(t *testing.T) { for _, tt := range []struct { name string inputs map[string]model.Input + with map[string]string state map[string]string expectedEnv map[string]string + warning string }{ { name: "with-input", inputs: map[string]model.Input{"key": {Default: "default value"}}, expectedEnv: map[string]string{"INPUT_KEY": "default value"}, }, + { + name: "declared input ignores case", + inputs: map[string]model.Input{"key": {Default: "default value"}}, + with: map[string]string{"KEY": "provided value"}, + expectedEnv: map[string]string{"INPUT_KEY": "provided value"}, + }, + { + name: "unexpected inputs warn once across stages", + inputs: map[string]model.Input{"key": {}}, + with: map[string]string{"KEY": "provided value", "UNKNOWN": "value", "other%0A\r\n": "value"}, + warning: "##[warning]Unexpected input(s) 'UNKNOWN', 'other%250A%0D%0A', valid inputs are ['key']", + }, + { + name: "no declared inputs", + with: map[string]string{"unknown": "value"}, + warning: "##[warning]Unexpected input(s) 'unknown', valid inputs are ['']", + }, { name: "restore-saved-state", state: map[string]string{"name": "state value"}, @@ -162,6 +196,8 @@ func TestActionRunner(t *testing.T) { }, } { t.Run(tt.name, func(t *testing.T) { + logger, hook := logtest.NewLogger() + ctx := common.WithLogger(t.Context(), logger) cm := &containerMock{} containerActionDir := "/var/run/act/actions/" + model.UsesHash("/org/repo/path@ref") + "/" cm.On("CopyDir", containerActionDir, "dir/", false, true).Return(func(_ctx context.Context) error { return nil }) @@ -175,21 +211,36 @@ func TestActionRunner(t *testing.T) { return true }) cm.On("Exec", []string{"node", "--preserve-symlinks-main", containerActionDir + "path"}, envMatcher, "", "").Return(func(_ctx context.Context) error { return nil }) + cm.On("Exec", nodeActionCommand(containerActionDir+"path/pre.js"), mock.Anything, "", "").Return(func(_ctx context.Context) error { return nil }) + cm.On("Exec", nodeActionCommand(containerActionDir+"path/post.js"), envMatcher, "", "").Return(func(_ctx context.Context) error { return nil }) remoteAction := mustNewRemoteAction(t, "org/repo/path@ref") step := &stepActionRemote{ - Step: &model.Step{ID: "step", Uses: "org/repo/path@ref"}, + Step: &model.Step{ID: "step", Uses: "org/repo/path@ref", With: tt.with}, RunContext: &RunContext{ - Config: &Config{}, + Config: &Config{PresetGitHubContext: &model.GithubContext{}}, Run: &model.Run{JobID: "job", Workflow: &model.Workflow{Jobs: map[string]*model.Job{"job": {}}}}, IntraActionState: map[string]map[string]string{"step": tt.state}, JobContainer: cm, }, - action: &model.Action{Inputs: tt.inputs, Runs: model.ActionRuns{Using: "node16"}}, - env: map[string]string{}, + action: &model.Action{Inputs: tt.inputs, Runs: model.ActionRuns{Using: "node16", Pre: "pre.js", Post: "post.js"}}, + actionDir: "dir", + env: (&model.Step{With: tt.with}).GetEnv(), remoteAction: remoteAction, } - require.NoError(t, runActionImpl(step, "dir", remoteAction)(t.Context())) + require.NoError(t, runPreStep(step)(ctx)) + assert.Empty(t, hook.AllRecords()) + require.NoError(t, runActionImpl(step, "dir", remoteAction)(ctx)) + require.NoError(t, runPostStep(step)(ctx)) + if tt.warning == "" { + assert.Empty(t, hook.AllRecords()) + } else { + require.Len(t, hook.AllRecords(), 1) + assert.Equal(t, tt.warning, hook.LastRecord().Message) + rawOutput, ok := hook.LastRecord().Attr(rawOutputField) + require.True(t, ok) + assert.Equal(t, true, rawOutput) + } cm.AssertExpectations(t) }) } @@ -473,10 +524,12 @@ func TestExecAsDockerStageEntrypoint(t *testing.T) { name string stage stepStage with map[string]string + inputs map[string]model.Input runs model.ActionRuns env map[string]string wantCmd []string wantEntrypoint []string + warning string }{ { name: "main stage prefers manifest values", @@ -491,17 +544,36 @@ func TestExecAsDockerStageEntrypoint(t *testing.T) { wantEntrypoint: []string{"main.sh"}, }, { - name: "main stage uses caller fallbacks", + name: "main stage uses caller fallbacks in any case and warns about undeclared inputs", stage: stepStageMain, - with: map[string]string{"args": "caller --flag", "entrypoint": "input.sh --verbose"}, + with: map[string]string{"ARGS": "caller --flag", "entryPoint": "input.sh --verbose", "unknown": "value"}, + inputs: map[string]model.Input{"message": {}}, runs: model.ActionRuns{Env: map[string]string{"ACTION_ONLY": "manifest"}}, wantCmd: []string{"caller", "--flag"}, wantEntrypoint: []string{"input.sh", "--verbose"}, + warning: "##[warning]Unexpected input(s) 'unknown', valid inputs are ['entryPoint', 'args', 'message']", + }, + { + name: "manifest args keep caller input quotes literal", + stage: stepStageMain, + with: map[string]string{"args": "it's literal"}, + runs: model.ActionRuns{Args: []string{"${{ inputs.args }}"}}, + env: map[string]string{"INPUT_ARGS": "it's literal"}, + wantCmd: []string{"it's literal"}, + }, + { + name: "caller input skips its default expression", + stage: stepStageMain, + with: map[string]string{"MESSAGE": "provided"}, + inputs: map[string]model.Input{"message": {Default: "${{ fromJSON('') }}"}}, + runs: model.ActionRuns{Args: []string{"${{ inputs.message }}"}}, + env: map[string]string{"INPUT_MESSAGE": "provided"}, + wantCmd: []string{"provided"}, }, { name: "explicit empty manifest args suppress caller args", stage: stepStageMain, - with: map[string]string{"args": "caller"}, + with: map[string]string{"args": "it's ignored"}, runs: model.ActionRuns{Args: []string{}}, wantCmd: []string{}, }, @@ -527,6 +599,8 @@ func TestExecAsDockerStageEntrypoint(t *testing.T) { }, } { t.Run(tc.name, func(t *testing.T) { + logger, hook := logtest.NewLogger() + ctx := common.WithLogger(t.Context(), logger) tc.runs.Using, tc.runs.Image = "docker", "docker://node:14" cm := &containerMock{} var input *container.NewContainerInput @@ -538,12 +612,13 @@ func TestExecAsDockerStageEntrypoint(t *testing.T) { step := &stepActionRemote{ Step: &model.Step{ID: "1", Uses: "org/action@v1", With: tc.with}, RunContext: &RunContext{ - Config: &Config{}, + Config: &Config{PresetGitHubContext: &model.GithubContext{}}, Run: &model.Run{JobID: "1", Workflow: &model.Workflow{Jobs: map[string]*model.Job{"1": {}}}}, JobContainer: cm, }, - action: &model.Action{Runs: tc.runs}, - env: mergeMaps(tc.env), + action: &model.Action{Inputs: tc.inputs, Runs: tc.runs}, + env: mergeMaps(tc.env), + remoteAction: mustNewRemoteAction(t, "org/action@v1"), } cm.On("Pull", false).Return(func(context.Context) error { return nil }) @@ -552,7 +627,17 @@ func TestExecAsDockerStageEntrypoint(t *testing.T) { cm.On("Start", true).Return(func(context.Context) error { return nil }) cm.On("Close").Return(func(context.Context) error { return nil }) - require.NoError(t, execAsDocker(t.Context(), step, "action", t.TempDir(), t.TempDir(), false, tc.stage)) + if tc.stage == stepStageMain { + require.NoError(t, runActionImpl(step, t.TempDir(), step.remoteAction)(ctx)) + } else { + require.NoError(t, execAsDocker(ctx, step, "action", t.TempDir(), t.TempDir(), false, tc.stage)) + } + if tc.warning == "" { + assert.Empty(t, hook.AllRecords()) + } else { + require.Len(t, hook.AllRecords(), 1) + assert.Equal(t, tc.warning, hook.LastRecord().Message) + } require.NotNil(t, input) assert.Equal(t, tc.wantCmd, input.Cmd) assert.Equal(t, tc.wantEntrypoint, input.Entrypoint) diff --git a/act/runner/cancellation_test.go b/act/runner/cancellation_test.go index 1c593e48..ea2aa80d 100644 --- a/act/runner/cancellation_test.go +++ b/act/runner/cancellation_test.go @@ -101,9 +101,13 @@ func TestMainStepsExecutorRunsAlwaysStepsAfterInterruption(t *testing.T) { assert.Equal(t, []string{"step1", "always-step"}, ran) require.NoError(t, laterStepCtxErr) assert.Equal(t, wantError == context.Canceled, rc.jobCancelled) + setJobResult(t.Context(), rc, rc, true) if wantError == context.DeadlineExceeded { assert.True(t, rc.jobFailed) assert.Equal(t, "failure", rc.getJobContext().Status) + assert.Equal(t, "failure", rc.Run.Job().Result) + } else { + assert.Equal(t, "cancelled", rc.Run.Job().Result) } }) } diff --git a/act/runner/command.go b/act/runner/command.go index 91112f75..3cc9de7f 100644 --- a/act/runner/command.go +++ b/act/runner/command.go @@ -8,6 +8,8 @@ import ( "cmp" "context" "fmt" + "maps" + "os" "strings" "unicode" @@ -158,11 +160,18 @@ const allowUnsecureCommandsVar = "ACTIONS_ALLOW_UNSECURE_COMMANDS" // not run, recording the error that fails the step. GitHub disabled both because a step that // echoes untrusted content can use them to set NODE_OPTIONS or PATH for every later step. func (rc *RunContext) refuseUnsecureCommand(ctx context.Context, command string) bool { - if isTruthyEnv(rc.currentStepEnv()[allowUnsecureCommandsVar]) || - isTruthyEnv(rc.Env[allowUnsecureCommandsVar]) || - isTruthyEnv(rc.GlobalEnv[allowUnsecureCommandsVar]) { + if isTruthyEnv(os.Getenv(allowUnsecureCommandsVar)) { // the runner's own opt-in wins, as on GitHub return false } + caseInsensitive := rc.JobContainer != nil && rc.JobContainer.IsEnvironmentCaseInsensitive() + for _, env := range []map[string]string{rc.currentStepEnv(), rc.Env, rc.GlobalEnv} { + if value, ok := lookupFold(env, allowUnsecureCommandsVar, caseInsensitive); ok { + if isTruthyEnv(value) { + return false + } + break + } + } // The step executor logs the failure itself, so keep this line's wording distinct. common.Logger(ctx).With(rawOutputField, true).Errorf("##[error]%s", EscapeCommandData(fmt.Sprintf( @@ -192,6 +201,18 @@ func (rc *RunContext) takeUnsecureCommandError() error { return err } +// runWithCommands runs executor judging its workflow commands by env, failing it on a refused one. +func (rc *RunContext) runWithCommands(ctx context.Context, env map[string]string, executor common.Executor) error { + rc.setCurrentStepEnv(maps.Clone(env)) // the executor keeps writing to env while the command handler reads it + defer rc.setCurrentStepEnv(nil) + _ = rc.takeUnsecureCommandError() // a refusal from before belongs to no step + err := executor(ctx) + if refusal := rc.takeUnsecureCommandError(); err == nil { + return refusal + } + return err +} + func (rc *RunContext) setEnv(ctx context.Context, kvPairs map[string]string, arg string, fromCommand bool) { name := kvPairs["name"] if strings.EqualFold(name, "NODE_OPTIONS") { diff --git a/act/runner/command_test.go b/act/runner/command_test.go index 46ad8f52..16eed13b 100644 --- a/act/runner/command_test.go +++ b/act/runner/command_test.go @@ -136,21 +136,28 @@ func TestEscapeCommandData(t *testing.T) { func TestUnsecureCommands(t *testing.T) { tests := []struct { - name string - jobEnv map[string]string - stepEnv map[string]string - optedIn bool + name string + jobEnv map[string]string + globalEnv map[string]string + stepEnv map[string]string + processEnv string + caseInsensitive bool + optedIn bool }{ {name: "refused with no opt-in"}, - // GitHub reads the opt-in with bool.TryParse, so "1" is not one. {name: "refused for a value bool.TryParse rejects", jobEnv: map[string]string{allowUnsecureCommandsVar: "1"}}, {name: "opted in through the step environment", stepEnv: map[string]string{allowUnsecureCommandsVar: "true"}, optedIn: true}, {name: "opted in through the job environment", jobEnv: map[string]string{allowUnsecureCommandsVar: "TRUE"}, optedIn: true}, + {name: "step opt-out overrides the job and global environment", jobEnv: map[string]string{allowUnsecureCommandsVar: "true"}, globalEnv: map[string]string{allowUnsecureCommandsVar: "true"}, stepEnv: map[string]string{allowUnsecureCommandsVar: "false"}}, + {name: "the runner's own opt-in overrides a step opt-out", stepEnv: map[string]string{allowUnsecureCommandsVar: "false"}, processEnv: "true", optedIn: true}, + {name: "a differently cased opt-in counts where env names are case-insensitive", jobEnv: map[string]string{"Actions_Allow_Unsecure_Commands": "true"}, caseInsensitive: true, optedIn: true}, + {name: "a differently cased opt-in is ignored where env names are case-sensitive", jobEnv: map[string]string{"Actions_Allow_Unsecure_Commands": "true"}}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Setenv(allowUnsecureCommandsVar, tt.processEnv) a := assert.New(t) - rc := &RunContext{Config: &Config{}, Env: tt.jobEnv} + rc := &RunContext{Config: &Config{}, Env: tt.jobEnv, GlobalEnv: tt.globalEnv, JobContainer: fakeContainer{caseInsensitive: tt.caseInsensitive}} rc.setCurrentStepEnv(tt.stepEnv) handler := rc.commandHandler(t.Context()) @@ -160,7 +167,6 @@ func TestUnsecureCommands(t *testing.T) { if !tt.optedIn { a.Empty(rc.Env["x"]) a.Empty(rc.ExtraPath) - // The refusal fails the step that produced it, once. require.ErrorContains(t, rc.takeUnsecureCommandError(), "set-env") a.NoError(rc.takeUnsecureCommandError()) return diff --git a/act/runner/expression.go b/act/runner/expression.go index c872d80e..7e55edd3 100644 --- a/act/runner/expression.go +++ b/act/runner/expression.go @@ -7,6 +7,7 @@ package runner import ( "bytes" "context" + "errors" "fmt" "maps" "path" @@ -127,7 +128,7 @@ func (rc *RunContext) newStepExpressionEvaluator(ctx context.Context, step step, func getHashFilesFunction(ctx context.Context, rc *RunContext) func(v []reflect.Value) (any, error) { return func(v []reflect.Value) (any, error) { if rc.JobContainer != nil { - timeed, cancel := context.WithTimeout(ctx, time.Minute) + timeoutCtx, cancel := context.WithTimeout(ctx, 120*time.Second) defer cancel() name := "workflow/hashfiles/index.js" hout := &bytes.Buffer{} @@ -154,20 +155,30 @@ func getHashFilesFunction(ctx context.Context, rc *RunContext) func(v []reflect. } stdout, stderr := rc.JobContainer.ReplaceLogWriter(hout, herr) - _ = rc.JobContainer.Copy(rc.JobContainer.GetActPath(), &container.FileEntry{ + defer rc.JobContainer.ReplaceLogWriter(stdout, stderr) + err := rc.JobContainer.Copy(rc.JobContainer.GetActPath(), &container.FileEntry{ Name: name, Mode: 0o644, Body: hashfiles, - }). - Then(rc.JobContainer.Exec([]string{"node", path.Join(rc.JobContainer.GetActPath(), name)}, - env, "", "")). - Finally(func(context.Context) error { - rc.JobContainer.ReplaceLogWriter(stdout, stderr) - return nil - })(timeed) - if _, output, found := strings.Cut(hout.String()+"\n"+herr.String(), "__OUTPUT__"); found { - if hash, _, found := strings.Cut(output, "__OUTPUT__"); found { - return hash, nil + }).Then(rc.JobContainer.Exec([]string{"node", path.Join(rc.JobContainer.GetActPath(), name)}, env, "", ""))(timeoutCtx) + quotedPatterns := expressionStringEscape(strings.Join(patterns, ", ")) + switch { + case err == nil: + case ctx.Err() != nil: + return "", ctx.Err() + case timeoutCtx.Err() != nil || errors.Is(err, context.DeadlineExceeded): + return "", fmt.Errorf("hashFiles('%s') couldn't finish within 120 seconds", quotedPatterns) + case hashFilesNodeMissing(err): + common.Logger(ctx).With(rawOutputField, true).Warnf("##[warning]hashFiles could not run because node is unavailable in the job environment. Returning an empty string.") + return "", nil + default: + return "", fmt.Errorf("hashFiles('%s') failed. Fail to hash files under directory '%s'", quotedPatterns, rc.githubWorkspace()) + } + for line := range strings.Lines(hout.String() + "\n" + herr.String()) { + if hash, ok := strings.CutPrefix(strings.TrimRight(line, "\r\n"), "__OUTPUT__"); ok { + if hash, ok = strings.CutSuffix(hash, "__OUTPUT__"); ok { + return hash, nil + } } } } @@ -175,6 +186,11 @@ func getHashFilesFunction(ctx context.Context, rc *RunContext) func(v []reflect. } } +func hashFilesNodeMissing(err error) bool { + exitErr, ok := errors.AsType[container.ExitCodeError](err) + return ok && (exitErr == 126 || exitErr == 127) || err.Error() == "Cannot find: node in PATH" +} + type expressionEvaluator struct { interpreter exprparser.Interpreter } diff --git a/act/runner/expression_test.go b/act/runner/expression_test.go index 3afdfb88..ff4d2ba4 100644 --- a/act/runner/expression_test.go +++ b/act/runner/expression_test.go @@ -5,9 +5,20 @@ package runner import ( + "bytes" + "context" + "errors" + "fmt" + "io" + "reflect" "strings" "sync" "testing" + "time" + + "gitea.com/gitea/runner/act/common" + "gitea.com/gitea/runner/act/common/logging/logtest" + "gitea.com/gitea/runner/act/container" "gitea.dev/actionslib/pkg/exprparser" "gitea.dev/actionslib/pkg/model" @@ -144,6 +155,92 @@ func TestEvaluateRunContext(t *testing.T) { } } +type hashFilesContainer struct { + fakeContainer + stdout, stderr io.Writer + copyErr error + execute func([]string, map[string]string, string, string) common.Executor +} + +func (c *hashFilesContainer) Copy(_destPath string, _files ...*container.FileEntry) common.Executor { + return common.NewErrorExecutor(c.copyErr) +} + +func (c *hashFilesContainer) Exec(command []string, env map[string]string, user, workdir string) common.Executor { + return c.execute(command, env, user, workdir) +} + +func (c *hashFilesContainer) ReplaceLogWriter(stdout, stderr io.Writer) (io.Writer, io.Writer) { + previousOut, previousErr := c.stdout, c.stderr + c.stdout, c.stderr = stdout, stderr + return previousOut, previousErr +} + +func TestGetHashFilesFunction(t *testing.T) { + const failureMessage = "hashFiles('dir''file, *.sum') failed. Fail to hash files under directory '/workspace'" + for _, test := range []struct { + name string + output string + execErr error + copyErr error + hash string + err string + warning bool + expired bool + }{ + {name: "success", output: "patterns __OUTPUT__wrong__OUTPUT__ echoed\n__OUTPUT__digest__OUTPUT__", hash: "digest"}, + {name: "no matches", output: "__OUTPUT____OUTPUT__"}, + {name: "script failure", output: "Cannot find: node in PATH\n__OUTPUT__digest__OUTPUT__", execErr: container.ExitCodeError(1), err: failureMessage}, + {name: "timeout", execErr: context.DeadlineExceeded, err: "hashFiles('dir''file, *.sum') couldn't finish within 120 seconds"}, + {name: "job deadline", execErr: context.DeadlineExceeded, expired: true, err: context.DeadlineExceeded.Error()}, + {name: "node missing", execErr: fmt.Errorf("wrapped: %w", container.ExitCodeError(127)), warning: true}, + {name: "node not executable", execErr: container.ExitCodeError(126), warning: true}, + {name: "host node missing", execErr: errors.New("Cannot find: node in PATH"), warning: true}, + {name: "copy failure", copyErr: errors.New("open /runner/cache: permission denied"), err: failureMessage}, + } { + t.Run(test.name, func(t *testing.T) { + logger, hook := logtest.NewLogger() + stdout, stderr := &bytes.Buffer{}, &bytes.Buffer{} + jobContainer := &hashFilesContainer{stdout: stdout, stderr: stderr, copyErr: test.copyErr} + jobContainer.execute = func(command []string, env map[string]string, user, workdir string) common.Executor { + assert.Equal(t, []string{"node", "/var/run/act/workflow/hashfiles/index.js"}, command) + assert.Equal(t, map[string]string{"patterns": "dir'file\n*.sum", "followSymbolicLinks": "true"}, env) + assert.Empty(t, user) + assert.Empty(t, workdir) + return func(_ctx context.Context) error { + _, err := io.WriteString(jobContainer.stderr, test.output) + require.NoError(t, err) + return test.execErr + } + } + ctx := common.WithLogger(t.Context(), logger) + if test.expired { + var cancel context.CancelFunc + ctx, cancel = context.WithDeadline(ctx, time.Now()) + defer cancel() + } + result, err := getHashFilesFunction(ctx, &RunContext{ + Config: &Config{Workdir: "/workspace"}, + JobContainer: jobContainer, + })([]reflect.Value{reflect.ValueOf("--follow-symbolic-links"), reflect.ValueOf("dir'file"), reflect.ValueOf("*.sum")}) + if test.err == "" { + require.NoError(t, err) + } else { + require.EqualError(t, err, test.err) + } + assert.Equal(t, test.hash, result) + assert.Same(t, stdout, jobContainer.stdout) + assert.Same(t, stderr, jobContainer.stderr) + if test.warning { + require.Len(t, hook.AllRecords(), 1) + assert.Equal(t, "##[warning]hashFiles could not run because node is unavailable in the job environment. Returning an empty string.", hook.LastRecord().Message) + } else { + assert.Empty(t, hook.AllRecords()) + } + }) + } +} + func TestEvaluateStep(t *testing.T) { rc := createRunContext(t) rc.Env["INPUT_FORGED"] = "leaked" diff --git a/act/runner/job_executor.go b/act/runner/job_executor.go index bb65d01b..affab193 100644 --- a/act/runner/job_executor.go +++ b/act/runner/job_executor.go @@ -400,6 +400,7 @@ func runMainStepsAfterInterrupt(ctx context.Context, rc *RunContext, steps []com func setJobResult(ctx context.Context, info jobInfo, rc *RunContext, success bool) { logger := common.Logger(ctx) + success = success && !rc.jobFailed // Matrix combinations share one *model.Job and run in parallel; serialize the // read-modify-write of the job result so a failing combination is not lost-updated by a diff --git a/act/runner/job_hooks.go b/act/runner/job_hooks.go index b074898a..26ae6387 100644 --- a/act/runner/job_hooks.go +++ b/act/runner/job_hooks.go @@ -40,6 +40,9 @@ func (rc *RunContext) runJobHook(ctx context.Context, hookPath, name string) err if hookPath == "" { return nil } + previousStep := rc.CurrentStep + rc.CurrentStep = "" + defer func() { rc.CurrentStep = previousStep }() cmd, shell := hookCommand(hookPath) rawLogger := common.Logger(ctx).With(rawOutputField, true) @@ -56,12 +59,12 @@ func (rc *RunContext) runJobHook(ctx context.Context, hookPath, name string) err err := rc.setupHookFileCommands(ctx, env) if err == nil { - err = rc.withCommandLogWriter(ctx, rc.JobContainer.Exec(cmd, env, "", "")) - } - // Processed even on failure, so a hook that exports what it managed to set up before - // failing still hands it to the job. - if processErr := rc.processHookFileCommands(ctx); err == nil { - err = processErr + err = rc.runWithCommands(ctx, env, func(ctx context.Context) error { + return rc.withCommandLogWriter(ctx, rc.JobContainer.Exec(cmd, env, "", "")) + }) + if processErr := rc.processHookFileCommands(ctx); err == nil { // a failed hook still exports what it set up + err = processErr + } } if err == nil { return nil diff --git a/act/runner/job_hooks_test.go b/act/runner/job_hooks_test.go index 84b82e93..421cf394 100644 --- a/act/runner/job_hooks_test.go +++ b/act/runner/job_hooks_test.go @@ -14,6 +14,7 @@ import ( "gitea.com/gitea/runner/act/common" "gitea.com/gitea/runner/act/common/logging" "gitea.com/gitea/runner/act/common/logging/logtest" + "gitea.com/gitea/runner/act/container" "gitea.dev/actionslib/pkg/model" "github.com/stretchr/testify/assert" @@ -27,6 +28,7 @@ type hookContainer struct { cmd []string env map[string]string err error + copyErr error envFile map[string]string pathTar []byte output string @@ -58,6 +60,10 @@ func (c *hookContainer) ReplaceLogWriter(stdout, _ io.Writer) (io.Writer, io.Wri return old, nil } +func (c *hookContainer) Copy(_destPath string, _files ...*container.FileEntry) common.Executor { + return common.NewErrorExecutor(c.copyErr) +} + func (c *hookContainer) UpdateFromEnv(_ string, env *map[string]string) common.Executor { return func(context.Context) error { maps.Copy(*env, c.envFile) @@ -116,6 +122,44 @@ func TestRunJobHook(t *testing.T) { assert.Same(t, jobWriter, jobContainer.stdout) }) + t.Run("fails on refused commands and honors the container opt-in", func(t *testing.T) { + for _, allow := range []bool{false, true} { + jobContainer := &hookContainer{output: "::set-env name=FROM_HOOK::1\n::add-path::/opt/hook/bin"} + rc, ctx := newHookRunContext(jobContainer, &Config{JobStartedHook: "/started.sh"}) + if allow { + rc.containerSpec.Env = map[string]string{allowUnsecureCommandsVar: "true"} + } + + err := rc.runJobStartedHook(ctx) + if allow { + require.NoError(t, err) + assert.Equal(t, "1", rc.Env["FROM_HOOK"]) + assert.Equal(t, []string{"/opt/hook/bin"}, rc.ExtraPath) + } else { + require.ErrorContains(t, err, "workflow command is disabled") + assert.Equal(t, "failure", rc.getJobContext().Status) + require.ErrorContains(t, common.JobError(ctx), "workflow command is disabled") + assert.NotContains(t, rc.Env, "FROM_HOOK") + assert.Empty(t, rc.ExtraPath) + } + assert.Nil(t, rc.currentStepEnv()) + require.NoError(t, rc.takeUnsecureCommandError()) + } + }) + + t.Run("the completed hook has no preceding step identity", func(t *testing.T) { + jobContainer := &hookContainer{output: "::set-output name=value::after\n::save-state name=value::after"} + rc, ctx := newHookRunContext(jobContainer, &Config{JobCompletedHook: "/completed.sh"}) + rc.CurrentStep = "last-step" + rc.StepResults = map[string]*model.StepResult{"last-step": {Outputs: map[string]string{"value": "before"}}} + + require.NoError(t, rc.runJobCompletedHook(ctx)) + assert.Empty(t, jobContainer.env["GITHUB_ACTION"]) + assert.Equal(t, "before", rc.StepResults["last-step"].Outputs["value"]) + assert.Empty(t, rc.IntraActionState) + assert.Equal(t, "last-step", rc.CurrentStep) + }) + // Each hook reads its own variable, so a swapped constant cannot pass. t.Run("falls back to the GitHub environment variables", func(t *testing.T) { for name, hook := range map[string]struct { @@ -161,6 +205,15 @@ func TestRunJobHook(t *testing.T) { require.ErrorContains(t, common.JobError(ctx), "boom") }) + t.Run("a hook whose file commands cannot be reset neither runs nor exports stale ones", func(t *testing.T) { + jobContainer := &hookContainer{copyErr: errors.New("boom"), envFile: map[string]string{"STALE": "1"}} + rc, ctx := newHookRunContext(jobContainer, &Config{JobStartedHook: "/hooks/started.sh"}) + + require.ErrorContains(t, rc.runJobStartedHook(ctx), "boom") + assert.Nil(t, jobContainer.cmd) + assert.NotContains(t, rc.Env, "STALE") + }) + t.Run("is a no-op without a hook", func(t *testing.T) { jobContainer := &hookContainer{} rc, ctx := newHookRunContext(jobContainer, &Config{}) diff --git a/act/runner/logger.go b/act/runner/logger.go index febe70de..3c673c1a 100644 --- a/act/runner/logger.go +++ b/act/runner/logger.go @@ -60,6 +60,8 @@ type masksContextKey string const masksContextKeyVal = masksContextKey("logrus.FieldLogger") +type valueMaskerContextKey struct{} + // Logger returns the appropriate logger for current context func Masks(ctx context.Context) *[]string { if masks, ok := ctx.Value(masksContextKeyVal).(*[]string); ok { @@ -88,6 +90,8 @@ func WithJobLoggerFactory(ctx context.Context, factory JobLoggerFactory) context // WithJobLogger attaches a new logger to context that is aware of steps func WithJobLogger(ctx context.Context, jobID, jobName string, config *Config, masks *[]string, matrix map[string]any) context.Context { + masker := valueMasker(config.InsecureSecrets, config.maskers()) + ctx = context.WithValue(ctx, valueMaskerContextKey{}, masker) ctx = WithMasks(ctx, masks) var output logging.Handler @@ -109,7 +113,7 @@ func WithJobLogger(ctx context.Context, jobID, jobName string, config *Config, m } handlers = append(handlers, &maskedHandler{ Handler: output, - masker: valueMasker(config.InsecureSecrets, config.maskers()), + masker: masker, }) level := logging.LevelTrace @@ -140,6 +144,9 @@ func WithCompositeStepLogger(ctx context.Context, stepID string) context.Context } func withStepLogger(ctx context.Context, stepNumber int, stepID, stepName, stageName string) context.Context { + if masker, ok := ctx.Value(valueMaskerContextKey{}).(entryProcessor); ok { + stepName = masker(ctx, logging.Record{Message: stepName}).Message + } stepIDs := []string{stepID} ctx = common.WithStepIDs(ctx, stepIDs) diff --git a/act/runner/logger_test.go b/act/runner/logger_test.go index d58a504f..52b7f46a 100644 --- a/act/runner/logger_test.go +++ b/act/runner/logger_test.go @@ -255,7 +255,11 @@ func TestWithJobLoggerFansOutAndMasksOnlyOutput(t *testing.T) { func TestJobLogJSONHandler(t *testing.T) { buf := &bytes.Buffer{} - logger := logging.New(newJobLogJSONHandler(buf)).With("job", "build", "msg", "clash").WithError(errors.New("boom")) + rc := &RunContext{Config: &Config{Secrets: map[string]string{"TOKEN": "s3cr3t-value"}}} + ctx := WithJobLoggerFactory(t.Context(), factoryFunc(func() logging.Handler { return newJobLogJSONHandler(buf) })) + ctx = WithJobLogger(ctx, "j1", "build", rc.Config, &rc.Masks, nil) + logger := common.Logger(withStepLogger(ctx, 1, "s1", "deploy s3cr3t-value", StageMain)). + With("msg", "clash").WithError(errors.New("boom")) logger.Warnf("careful %d", 1) var got map[string]any @@ -263,7 +267,15 @@ func TestJobLogJSONHandler(t *testing.T) { assert.Equal(t, "careful 1", got["msg"]) assert.Equal(t, "warning", got["level"]) assert.Equal(t, "build", got["job"]) + assert.Equal(t, "deploy ***", got["step"]) assert.Equal(t, "boom", got["error"]) assert.Equal(t, "clash", got["fields.msg"]) assert.NotEmpty(t, got["time"]) + + rc.AddMask(ctx, "runtime-value") + buf.Reset() + common.Logger(withStepLogger(ctx, 2, "s2", "deploy s3cr3t-value runtime-value", StageMain)).Info("done") + + require.NoError(t, json.Unmarshal(buf.Bytes(), &got)) + assert.Equal(t, "deploy *** ***", got["step"]) } diff --git a/act/runner/run_context.go b/act/runner/run_context.go index a9f8179b..a0883f1f 100644 --- a/act/runner/run_context.go +++ b/act/runner/run_context.go @@ -333,9 +333,12 @@ func splitVolumes(specs []string) ([]string, map[string]string, map[string]bool) continue } targets[parsed.Target] = true - if parsed.Type == mount.TypeVolume && parsed.Source != "" && !parsed.ReadOnly { + switch { + case parsed.Type != mount.TypeVolume || parsed.Source == "" || parsed.ReadOnly: + binds = append(binds, spec) + case mounts[parsed.Source] == "": mounts[parsed.Source] = parsed.Target - } else { + case mounts[parsed.Source] != parsed.Target: // a repeat of the same mount is dropped binds = append(binds, spec) } } @@ -393,6 +396,7 @@ func (rc *RunContext) startHostEnvironment() common.Executor { randBytes := make([]byte, 8) _, _ = rand.Read(randBytes) miscpath := filepath.Join(cacheDir, hex.EncodeToString(randBytes)) + rc.cleanUpJobContainer = func(_ctx context.Context) error { return os.RemoveAll(miscpath) } actPath := filepath.Join(miscpath, "act") if err := os.MkdirAll(actPath, 0o777); err != nil { return err @@ -1593,7 +1597,9 @@ func isLocalCheckout(ghc *model.GithubContext, step *model.Step) bool { } func (rc *RunContext) withGithubEnv(ctx context.Context, github *model.GithubContext, env map[string]string) { - env["CI"] = "true" + if _, ok := env["CI"]; !ok { + env["CI"] = "true" + } env["GITHUB_WORKFLOW"] = github.Workflow env["GITHUB_RUN_ID"] = github.RunID env["GITHUB_RUN_NUMBER"] = github.RunNumber @@ -1611,6 +1617,7 @@ func (rc *RunContext) withGithubEnv(ctx context.Context, github *model.GithubCon env["GITHUB_REF"] = github.Ref env["GITHUB_REF_NAME"] = github.RefName env["GITHUB_REF_TYPE"] = github.RefType + env["GITHUB_REF_PROTECTED"] = strconv.FormatBool(github.RefProtected) env["GITHUB_JOB"] = github.Job env["GITHUB_REPOSITORY_OWNER"] = github.RepositoryOwner env["GITHUB_RETENTION_DAYS"] = github.RetentionDays @@ -1620,6 +1627,18 @@ func (rc *RunContext) withGithubEnv(ctx context.Context, github *model.GithubCon env["GITHUB_HEAD_REF"] = github.HeadRef env["GITHUB_SERVER_URL"] = github.ServerURL env["GITHUB_API_URL"] = github.APIURL + for name, value := range map[string]string{ + "GITHUB_WORKFLOW_REF": github.WorkflowRef, + "GITHUB_WORKFLOW_SHA": github.WorkflowSha, + "GITHUB_ACTOR_ID": github.ActorID, + "GITHUB_TRIGGERING_ACTOR": github.TriggeringActor, + "GITHUB_REPOSITORY_ID": github.RepositoryID, + "GITHUB_REPOSITORY_OWNER_ID": github.RepositoryOwnerID, + } { + if value != "" { // a context without them keeps what the environment set + env[name] = value + } + } for _, name := range []string{"GITHUB_SERVER_URL", "GITHUB_API_URL"} { if env[name] == "" { diff --git a/act/runner/run_context_test.go b/act/runner/run_context_test.go index 7a293bc1..66232bc1 100644 --- a/act/runner/run_context_test.go +++ b/act/runner/run_context_test.go @@ -143,8 +143,11 @@ func TestRunContext_EvalBool(t *testing.T) { type fakeContainer struct { container.ExecutionsEnvironment + caseInsensitive bool } +func (c fakeContainer) IsEnvironmentCaseInsensitive() bool { return c.caseInsensitive } + func (fakeContainer) Pull(bool) common.Executor { return func(context.Context) error { return nil } } func (fakeContainer) Start(bool) common.Executor { return func(context.Context) error { return nil } } @@ -281,12 +284,13 @@ jobs: image: redis:latest volumes: - data:/data + - data:/backup `, &Config{ValidVolumes: []string{"data"}})[0] require.Equal(t, "redis:latest", redis.Image) require.Equal(t, []string{"data"}, redis.ValidVolumes) require.Equal(t, map[string]string{"data": "/data"}, redis.Mounts) - require.Empty(t, redis.Binds) + require.Equal(t, []string{"data:/backup"}, redis.Binds) require.Empty(t, redis.WorkingDir) require.Nil(t, redis.Entrypoint) require.Nil(t, redis.Cmd) @@ -461,6 +465,7 @@ func TestRunContext_GetBindsAndMounts(t *testing.T) { {"BindAnonymousVolume", []string{"/volume"}, "/volume", map[string]string{}}, {"BindHostFile", []string{"/path/to/file/on/host:/volume"}, "/path/to/file/on/host:/volume", map[string]string{}}, {"MountExistingVolume", []string{"volume-id:/volume"}, "", map[string]string{"volume-id": "/volume"}}, + {"MountExistingVolumeTwice", []string{"volume-id:/volume", "volume-id:/other"}, "volume-id:/other", map[string]string{"volume-id": "/volume"}}, {"MountExistingVolumeReadOnly", []string{"volume-id:/volume:ro"}, "volume-id:/volume:ro", map[string]string{}}, {"BindRelativeHostPath", []string{"./relative:/volume"}, "./relative:/volume", map[string]string{}}, {"OverridesToolCache", []string{"/host/tools:/opt/hostedtoolcache"}, "/host/tools:/opt/hostedtoolcache", map[string]string{}}, @@ -862,11 +867,37 @@ func TestGetGitHubContext(t *testing.T) { assert.Equal(t, rc.Config.Secrets["GITHUB_TOKEN"], ghc.Token) assert.Equal(t, "job1", ghc.Job) - rc.Config.PresetGitHubContext = &model.GithubContext{Actor: "preset-actor", TriggeringActor: "triggerer", Job: "preset-job"} + rc.Config.PresetGitHubContext = &model.GithubContext{ + Actor: "preset-actor", + ActorID: "12", + TriggeringActor: "triggerer", + Job: "preset-job", + RepositoryID: "34", + RepositoryOwnerID: "56", + WorkflowRef: "owner/repo/.gitea/workflows/test.yml@refs/heads/main", + WorkflowSha: "workflow-sha", + RefProtected: true, + } ghc = rc.getGithubContext(t.Context()) assert.Equal(t, "preset-actor", ghc.Actor) assert.Equal(t, "triggerer", ghc.TriggeringActor) assert.Equal(t, "job1", ghc.Job) + env := map[string]string{} + rc.withGithubEnv(t.Context(), ghc, env) + for name, value := range map[string]string{ + "GITHUB_ACTOR_ID": "12", + "GITHUB_TRIGGERING_ACTOR": "triggerer", + "GITHUB_REPOSITORY_ID": "34", + "GITHUB_REPOSITORY_OWNER_ID": "56", + "GITHUB_WORKFLOW_REF": "owner/repo/.gitea/workflows/test.yml@refs/heads/main", + "GITHUB_WORKFLOW_SHA": "workflow-sha", + "GITHUB_REF_PROTECTED": "true", + } { + assert.Equal(t, value, env[name], name) + } + ghc.RefProtected = false + rc.withGithubEnv(t.Context(), ghc, env) + assert.Equal(t, "false", env["GITHUB_REF_PROTECTED"]) preset := &model.GithubContext{ServerURL: "https://preset.test", APIURL: "https://preset.test/api/v1"} for _, testcase := range []struct { @@ -1092,6 +1123,22 @@ func TestRunContext_cleanupFailedStart(t *testing.T) { type ctxKey string const sentinel = ctxKey("sentinel") + t.Run("removes host directories when tool cache creation fails", func(t *testing.T) { + cacheDir := t.TempDir() + toolCache := filepath.Join(t.TempDir(), "file") + require.NoError(t, os.WriteFile(toolCache, nil, 0o600)) + rc := &RunContext{ + Config: &Config{ActionCacheDir: cacheDir}, + Env: map[string]string{"RUNNER_TOOL_CACHE": toolCache}, + platformImage: "-self-hosted", + } + require.Error(t, rc.startContainer()(t.Context())) + entries, err := os.ReadDir(cacheDir) + require.NoError(t, err) + assert.Empty(t, entries) + assert.FileExists(t, toolCache) + }) + for name, canceled := range map[string]bool{"cancellation during cleanup": false, "already canceled": true} { t.Run(name, func(t *testing.T) { ctx, cancel := context.WithCancel(context.WithValue(t.Context(), sentinel, "v")) @@ -1573,4 +1620,11 @@ func TestRunContextWithGithubEnvRunnerValues(t *testing.T) { assert.Equal(t, "/workspace/owner", env["RUNNER_WORKSPACE"]) assert.Equal(t, "1", env["RUNNER_DEBUG"]) assert.Equal(t, "http://[2001:db8::1]:8080/", env["ACTIONS_RUNTIME_URL"]) + assert.Equal(t, "true", env["CI"]) + + for _, ci := range []string{"false", ""} { + env["CI"] = ci + rc.withGithubEnv(ctx, &model.GithubContext{}, env) + assert.Equal(t, ci, env["CI"]) + } } diff --git a/act/runner/runner_test.go b/act/runner/runner_test.go index 1d102d48..c59b77d5 100644 --- a/act/runner/runner_test.go +++ b/act/runner/runner_test.go @@ -456,6 +456,7 @@ func TestRunEventHostEnvironment(t *testing.T) { {workdir, "do-not-leak-step-env-in-composite", "push", "", platforms, secrets}, {workdir, "set-env-step-env-override", "push", "", platforms, secrets}, {workdir, "set-env-new-env-file-per-step", "push", "", platforms, secrets}, + {workdir, "github-parity", "push", "", platforms, secrets}, {workdir, "no-panic-on-invalid-composite-action", "push", "jobs failed due to invalid action", platforms, secrets}, }...) } diff --git a/act/runner/step.go b/act/runner/step.go index 42984557..31f4a9ba 100644 --- a/act/runner/step.go +++ b/act/runner/step.go @@ -170,25 +170,16 @@ func runStepExecutor(step step, stage stepStage, executor common.Executor) commo initialFiles = append([]*container.FileEntry{{Name: summaryFileCommand, Mode: 0o666}}, files...) topRC.summaryFileInitialized[stepSummaryIndex] = true } - if err := rc.JobContainer.Copy(actPath, initialFiles...)(ctx); err != nil && len(initialFiles) > len(files) { - _ = rc.JobContainer.Copy(actPath, files...)(ctx) // the summary goes first and the resets retry alone, so neither failure blocks the other + fileCommandInitErr := rc.JobContainer.Copy(actPath, initialFiles...)(ctx) + if fileCommandInitErr != nil && len(initialFiles) > len(files) { + fileCommandInitErr = rc.JobContainer.Copy(actPath, files...)(ctx) // a broken summary must not block the resets } - // The command handler needs the step's env to judge ACTIONS_ALLOW_UNSECURE_COMMANDS. - // Cloned: the step executor keeps writing to its own env map after this point, on a - // different goroutine from the command handler that reads it. - rc.setCurrentStepEnv(maps0.Clone(*step.getEnv())) - defer rc.setCurrentStepEnv(nil) - _ = rc.takeUnsecureCommandError() // a refusal from before any step belongs to no step - - timeoutctx, cancelTimeOut := evaluateStepTimeout(ctx, rc.ExprEval, stepModel) + timeoutctx, cancelTimeOut := evaluateStepTimeout(ctx, step) defer cancelTimeOut() - err = executor(timeoutctx) - // Always take it, so the job-scoped error cannot leak onto a later step. A refusal - // fails the step as it does on GitHub, but the executor's own error wins. - insecureErr := rc.takeUnsecureCommandError() + err = fileCommandInitErr if err == nil { - err = insecureErr + err = rc.runWithCommands(timeoutctx, *step.getEnv(), executor) } fileCommandCtx := ctx if ctx.Err() != nil { // an interrupted step still records its file commands, a live deadline stays in force @@ -196,17 +187,19 @@ func runStepExecutor(step step, stage stepStage, executor common.Executor) commo fileCommandCtx, cancelFileCommands = context.WithTimeout(context.WithoutCancel(ctx), jobCleanupTimeout) defer cancelFileCommands() } - if fileErr := processRunnerEnvFileCommand(fileCommandCtx, envFileCommand, rc, rc.setEnvFile); fileErr != nil && err == nil { - err = fileErr - } - if fileErr := processRunnerEnvFileCommand(fileCommandCtx, stateFileCommand, rc, rc.saveState); fileErr != nil && err == nil { - err = fileErr - } - if fileErr := processRunnerEnvFileCommand(fileCommandCtx, outputFileCommand, rc, rc.setOutput); fileErr != nil && err == nil { - err = fileErr - } - if fileErr := rc.UpdateExtraPath(fileCommandCtx, path.Join(actPath, pathFileCommand)); fileErr != nil && err == nil { - err = fileErr + if fileCommandInitErr == nil { + if fileErr := processRunnerEnvFileCommand(fileCommandCtx, envFileCommand, rc, rc.setEnvFile); fileErr != nil && err == nil { + err = fileErr + } + if fileErr := processRunnerEnvFileCommand(fileCommandCtx, stateFileCommand, rc, rc.saveState); fileErr != nil && err == nil { + err = fileErr + } + if fileErr := processRunnerEnvFileCommand(fileCommandCtx, outputFileCommand, rc, rc.setOutput); fileErr != nil && err == nil { + err = fileErr + } + if fileErr := rc.UpdateExtraPath(fileCommandCtx, path.Join(actPath, pathFileCommand)); fileErr != nil && err == nil { + err = fileErr + } } _ = rc.JobContainer.Copy(actPath, files...)(fileCommandCtx) @@ -239,8 +232,11 @@ func runStepExecutor(step step, stage stepStage, executor common.Executor) commo } } -func evaluateStepTimeout(ctx context.Context, exprEval *expressionEvaluator, stepModel *model.Step) (context.Context, context.CancelFunc) { - timeout, err := exprEval.Interpolate(ctx, stepModel.TimeoutMinutes) +func evaluateStepTimeout(ctx context.Context, step step) (context.Context, context.CancelFunc) { + if step.getStepModel().TimeoutMinutes == "" { + return ctx, func() {} + } + timeout, err := step.getRunContext().NewStepExpressionEvaluator(ctx, step).Interpolate(ctx, step.getStepModel().TimeoutMinutes) if err != nil { common.Logger(ctx).Errorf("An error occurred when attempting to determine the step timeout: %s", err) } else if timeout != "" { @@ -262,9 +258,15 @@ func setupEnv(ctx context.Context, step step) error { var err error exprEval := rc.NewExpressionEvaluator(ctx) stepEnv := step.getStepModel().Environment() - for k, v := range stepEnv { - if stepEnv[k], err = exprEval.Interpolate(ctx, v); err != nil { - return fmt.Errorf("unable to interpolate env %s: %w", k, err) + if envNode := step.getStepModel().Env; strings.Contains(envNode.Value, "${{") { // only a whole-map expression has a value + if err := model.DecodeEvaluated("step env", envNode, exprEval.shared(ctx).EvaluateYamlNode, &stepEnv); err != nil { + return err + } + } else { + for k, v := range stepEnv { + if stepEnv[k], err = exprEval.Interpolate(ctx, v); err != nil { + return fmt.Errorf("unable to interpolate env %s: %w", k, err) + } } } // merge step env last, since it should not be overwritten diff --git a/act/runner/step_action_remote.go b/act/runner/step_action_remote.go index 28401f92..4a504fb2 100644 --- a/act/runner/step_action_remote.go +++ b/act/runner/step_action_remote.go @@ -32,6 +32,7 @@ type stepActionRemote struct { remoteAction *remoteAction actionDir string // the repository at the resolved commit, shared by the job's actions in it resolvedSha string // set on the first step preparing the repository + preStepStarted bool } type actionDownload struct { @@ -142,7 +143,10 @@ func (sar *stepActionRemote) pre() common.Executor { return common.NewPipelineExecutor( sar.prepareActionExecutor(), - runStepExecutor(sar, stepStagePre, runPreStep(sar)).If(hasPreStep(sar)).If(shouldRunPreStep(sar))) + runStepExecutor(sar, stepStagePre, func(ctx context.Context) error { + sar.preStepStarted = true + return runPreStep(sar)(ctx) + }).If(hasPreStep(sar)).If(shouldRunPreStep(sar))) } func (sar *stepActionRemote) main() common.Executor { diff --git a/act/runner/step_action_remote_test.go b/act/runner/step_action_remote_test.go index 917a9b6a..3326e802 100644 --- a/act/runner/step_action_remote_test.go +++ b/act/runner/step_action_remote_test.go @@ -207,6 +207,60 @@ func TestStepActionRemotePrepare(t *testing.T) { assert.Equal(t, "restore", path.Base(restoreContainerDir)) } +func TestStepActionRemotePreCleanup(t *testing.T) { + for _, test := range []struct { + name string + preIf string + preError error + }{ + {name: "pre ran and main skipped", preIf: "always()"}, + {name: "pre failed before main", preIf: "always()", preError: assert.AnError}, + {name: "pre skipped and main skipped", preIf: "false"}, + } { + t.Run(test.name, func(t *testing.T) { + cm := &containerMock{} + sar := &stepActionRemote{ + Step: &model.Step{ID: "step", Uses: "remote/action@v1", If: yaml.Node{Value: "false"}}, + RunContext: &RunContext{ + Config: &Config{}, + Run: &model.Run{JobID: "job", Workflow: &model.Workflow{Jobs: map[string]*model.Job{"job": {}}}}, + StepResults: map[string]*model.StepResult{}, + JobContainer: cm, + }, + action: &model.Action{Runs: model.ActionRuns{ + Using: "node24", Pre: "pre.js", PreIf: test.preIf, Post: "post.js", PostIf: "always()", + }}, + remoteAction: mustNewRemoteAction(t, "remote/action@v1"), + actionDir: t.TempDir(), + } + sar.RunContext.ExprEval = sar.RunContext.NewExpressionEvaluator(t.Context()) + if test.preIf != "false" { + _, actionDir := getContainerActionPaths(sar) + expectStepFileCommands(t, cm) + cm.On("CopyDir", actionDir+"/", sar.actionDir+"/", false, true).Return(noopExecutor) + cm.On("Exec", nodeActionCommand(path.Join(actionDir, "pre.js")), mock.Anything, "", "").Return(func(_ctx context.Context) error { return test.preError }).Once() + cm.On("Exec", nodeActionCommand(path.Join(actionDir, "post.js")), mock.Anything, "", "").Return(noopExecutor).Once() + } + + assert.Equal(t, test.preError, sar.pre()(t.Context())) + if test.preError == nil { + require.NoError(t, sar.main()(t.Context())) + assert.Equal(t, model.StepStatusSkipped, sar.RunContext.StepResults["step"].Conclusion) + } + require.NoError(t, sar.post()(t.Context())) + cm.AssertExpectations(t) + }) + } + + skippedComposite := &stepActionRemote{ + Step: &model.Step{ID: "step"}, + RunContext: &RunContext{StepResults: map[string]*model.StepResult{"step": {Conclusion: model.StepStatusSkipped}}}, + action: &model.Action{Runs: model.ActionRuns{Using: "composite"}}, + preStepStarted: true, + } + assert.False(t, shouldRunPostStep(skippedComposite)(t.Context())) +} + func Test_newRemoteAction(t *testing.T) { check := func(action string, want *remoteAction, wantCloneURL string) { t.Run(action, func(t *testing.T) { diff --git a/act/runner/step_docker.go b/act/runner/step_docker.go index e43a4802..4b290bff 100644 --- a/act/runner/step_docker.go +++ b/act/runner/step_docker.go @@ -60,13 +60,13 @@ func (sd *stepDocker) runUsesContainer() common.Executor { return errKubernetesDocker } image := strings.TrimPrefix(step.Uses, "docker://") - cmd, err := shellquote.Split(step.With["args"]) + cmd, err := shellquote.Split(stepInput(step, "args")) if err != nil { return err } var entrypoint []string - if entry := step.With["entrypoint"]; entry != "" { + if entry := stepInput(step, "entrypoint"); entry != "" { entrypoint = []string{entry} } diff --git a/act/runner/step_run.go b/act/runner/step_run.go index cb062156..f8f174d4 100644 --- a/act/runner/step_run.go +++ b/act/runner/step_run.go @@ -259,6 +259,9 @@ func (sr *stepRun) setupShellCommand(ctx context.Context) (name, script string, } scCmd := step.ShellCommand() + if step.Shell == "pwsh" || step.Shell == "powershell" { + scCmd = step.Shell + ` -command ". '{0}'"` + } if implicitShell && (step.Shell == "bash" || step.Shell == "sh") { scCmd = step.Shell + " -e {0}" } diff --git a/act/runner/step_run_print_test.go b/act/runner/step_run_print_test.go index 1225efcc..e0b2ec54 100644 --- a/act/runner/step_run_print_test.go +++ b/act/runner/step_run_print_test.go @@ -91,7 +91,7 @@ env: sr := &stepRun{ Step: &step, RunContext: &RunContext{}, - shellCommand: "pwsh -command . '{0}'", + shellCommand: `pwsh -command ". '{0}'"`, interpolatedScript: "echo one\necho two\n", env: map[string]string{ "PATH_PREFIX": "/custom/bin", @@ -106,7 +106,7 @@ env: "[j1] | ::group::Run echo one", "[j1] | echo one", "[j1] | echo two", - "[j1] | shell: pwsh -command . '{0}'", + `[j1] | shell: pwsh -command ". '{0}'"`, "[j1] | env:", "[j1] | PATH_PREFIX: /custom/bin", "[j1] | GREETING: hello", diff --git a/act/runner/step_run_test.go b/act/runner/step_run_test.go index 28c2d35d..90e3c20a 100644 --- a/act/runner/step_run_test.go +++ b/act/runner/step_run_test.go @@ -68,6 +68,7 @@ func TestStepRun(t *testing.T) { func TestStepRunShellParity(t *testing.T) { tests := []struct { name, run, shell, workingDir string + actPath string env map[string]string host bool probeErr error @@ -89,6 +90,19 @@ func TestStepRunShellParity(t *testing.T) { probeErr: assert.AnError, wantCmd: []string{"sh", "-e", "/var/run/act/workflow/1.sh"}, }, + { + name: "builtin pwsh", + shell: "pwsh", + actPath: "/var/run/act with spaces", + wantExt: ".ps1", + wantCmd: []string{"pwsh", "-command", ". '/var/run/act with spaces/workflow/1.ps1'"}, + }, + { + name: "builtin powershell", + shell: "powershell", + wantExt: ".ps1", + wantCmd: []string{"powershell", "-command", ". '/var/run/act/workflow/1.ps1'"}, + }, { name: "custom pwsh template", shell: "pwsh -NoProfile -File {0}", @@ -134,14 +148,17 @@ func TestStepRunShellParity(t *testing.T) { t.Run(test.name, func(t *testing.T) { cm := &containerMock{} var jobContainer container.ExecutionsEnvironment = &shellContainerMock{cm} - if test.host { + switch { + case test.actPath != "": + jobContainer = &container.HostEnvironment{ActPath: test.actPath} + case test.host: if runtime.GOOS == "windows" { t.Skip("Linux host shell selection") } test.env = map[string]string{"PATH": t.TempDir()} require.NoError(t, os.WriteFile(filepath.Join(test.env["PATH"], "bash"), nil, 0o755)) jobContainer = &container.HostEnvironment{ActPath: "/var/run/act"} - } else if test.shell == "" { + case test.shell == "": cm.On("Exec", []string{"sh", "-c", "command -v bash >/dev/null 2>&1"}, mock.AnythingOfType("map[string]string"), "", "").Return(func(context.Context) error { return test.probeErr diff --git a/act/runner/step_test.go b/act/runner/step_test.go index 53623b1d..79086153 100644 --- a/act/runner/step_test.go +++ b/act/runner/step_test.go @@ -183,6 +183,7 @@ func TestSetupEnv(t *testing.T) { "GITHUB_GRAPHQL_URL": "", "GITHUB_HEAD_REF": "", "GITHUB_JOB": "1", + "GITHUB_REF_PROTECTED": "false", "GITHUB_RETENTION_DAYS": "0", "GITHUB_RUN_ATTEMPT": "", "GITHUB_RUN_ID": "runId", @@ -256,6 +257,17 @@ jobs: rc.workflowCallInputs = map[string]any{"args": "${{ inputs.unresolved }}"} require.ErrorContains(t, setupInputs(t.Context(), &stepRun{RunContext: rc, Step: workflow.Jobs["test"].Steps[0].Clone(), env: map[string]string{}}), "with:") }) + + t.Run("whole step environment expression is evaluated once", func(t *testing.T) { + rc := createRunContext(t) + rc.Matrix["environment"] = `{"NAME":"dynamic${{ github.job }}","COUNT":12}` + step := &stepRun{RunContext: rc, Step: &model.Step{}, env: map[string]string{}} + require.NoError(t, step.Step.Env.Encode("${{ fromJSON(matrix.environment) }}")) + + require.NoError(t, setupEnv(t.Context(), step)) + assert.Equal(t, "dynamic${{ github.job }}", step.env["NAME"]) + assert.Equal(t, "12", step.env["COUNT"]) + }) } func TestIsStepEnabled(t *testing.T) { @@ -478,6 +490,23 @@ func TestRunStepExecutorParity(t *testing.T) { require.NoError(t, runStepExecutor(step, stepStageMain, noopExecutor)(t.Context())) }) + t.Run("command file initialization failure prevents execution and stale commands", func(t *testing.T) { + step := newStep(t, &model.Step{ID: "commands"}) + workflowDir := filepath.Join(step.RunContext.JobContainer.GetActPath(), "workflow") + require.NoError(t, os.MkdirAll(filepath.Join(workflowDir, "outputcmd.txt"), 0o700)) + require.NoError(t, os.WriteFile(filepath.Join(workflowDir, "envs.txt"), []byte("STALE=1\n"), 0o600)) + executed := false + err := runStepExecutor(step, stepStageMain, func(context.Context) error { + executed = true + return nil + })(t.Context()) + + require.Error(t, err) + assert.False(t, executed) + assert.Empty(t, step.RunContext.Env["STALE"]) + assert.Equal(t, model.StepStatusFailure, step.RunContext.StepResults[step.Step.ID].Conclusion) + }) + t.Run("file commands survive a job timeout", func(t *testing.T) { synctest.Test(t, func(t *testing.T) { step := newStep(t, &model.Step{ID: "interrupted"}) @@ -552,14 +581,23 @@ func TestRunStepExecutorParity(t *testing.T) { }) t.Run("timeout must be positive", func(t *testing.T) { - exprEval := createRunContext(t).NewExpressionEvaluator(t.Context()) for timeout, wantDeadline := range map[string]bool{"-1": false, "0": false, "1": true} { - ctx, cancel := evaluateStepTimeout(t.Context(), exprEval, &model.Step{TimeoutMinutes: timeout}) + ctx, cancel := evaluateStepTimeout(t.Context(), newStep(t, &model.Step{TimeoutMinutes: timeout})) _, hasDeadline := ctx.Deadline() cancel() assert.Equal(t, wantDeadline, hasDeadline, timeout) } }) + + t.Run("timeout reads step environment", func(t *testing.T) { + step := newStep(t, &model.Step{ID: "timeout", TimeoutMinutes: "${{ fromJSON(env.STEP_TIMEOUT) }}"}) + require.NoError(t, step.Step.Env.Encode(map[string]string{"STEP_TIMEOUT": "1"})) + require.NoError(t, runStepExecutor(step, stepStageMain, func(ctx context.Context) error { + _, hasDeadline := ctx.Deadline() + assert.True(t, hasDeadline) + return nil + })(t.Context())) + }) } func lastRecordAttr(h *logtest.Handler, key string) any { diff --git a/act/runner/testdata/github-parity/push.yml b/act/runner/testdata/github-parity/push.yml new file mode 100644 index 00000000..79d446d9 --- /dev/null +++ b/act/runner/testdata/github-parity/push.yml @@ -0,0 +1,20 @@ +on: push +jobs: + _: + runs-on: ubuntu-latest + env: + CI: 'false' + steps: + - run: test "$CI" = false + - run: printf 'MULTI<> "$GITHUB_ENV" + - run: test "$MULTI" = "$(printf 'a\r\nb\r')" + - run: | + runs: + using: composite + steps: + - run: test "${{ '${{ inputs.undeclared }}' }}" = given + shell: bash + shell: cp {0} action.yml + - uses: ./ + with: + undeclared: given diff --git a/docs/job-hooks.md b/docs/job-hooks.md index c3f95501..f7a4080a 100644 --- a/docs/job-hooks.md +++ b/docs/job-hooks.md @@ -47,7 +47,7 @@ A hook path that does not exist inside the job environment fails the job with `N A hook sees the job's environment: the workflow, job and `container:` `env:`, the runner's `envs`, and the `GITHUB_*` context variables, with the same masking applied to its output as to a step's. The step-specific ones (`GITHUB_ACTION`, `GITHUB_OUTPUT`, `GITHUB_STATE`) are not set — a hook is not a step, so `::save-state::` and `::set-output::` have nowhere to go. -Its stdout is part of the job log, inside a collapsible group, and is scanned for workflow commands. `::add-mask::` registers a value to be masked for the rest of the job, `::set-env::` and `::add-path::` apply to the steps that follow. +Its stdout is part of the job log, inside a collapsible group, and is scanned for workflow commands. `::add-mask::` registers a value to be masked for the rest of the job. `::set-env::` and `::add-path::` apply to the steps that follow, but as in a step they need `ACTIONS_ALLOW_UNSECURE_COMMANDS: true`, and a refused one fails the hook. `$GITHUB_ENV` and `$GITHUB_PATH` point at files that are read back after the hook exits, so the file-command form works too: diff --git a/internal/app/cmd/exec.go b/internal/app/cmd/exec.go index 52989534..8e6a7f78 100644 --- a/internal/app/cmd/exec.go +++ b/internal/app/cmd/exec.go @@ -326,11 +326,10 @@ func runExecList(planner model.WorkflowPlanner, execArgs *executeArgs) error { // collect all events from loaded workflows events := planner.GetEvents() - // set default event type to first event from many available - // this way user dont have to specify the event. - logging.Infof("Using first detected workflow event for filtering: %s", events[0]) - - filterEventName = events[0] + if len(events) > 0 { + logging.Infof("Using first detected workflow event for filtering: %s", events[0]) + filterEventName = events[0] + } } var err error diff --git a/internal/app/cmd/exec_test.go b/internal/app/cmd/exec_test.go index f0a872bb..a92849cd 100644 --- a/internal/app/cmd/exec_test.go +++ b/internal/app/cmd/exec_test.go @@ -187,6 +187,9 @@ func TestRunExecListUsesJobEventAndAllPlans(t *testing.T) { require.NoError(t, runExecList(planner, &executeArgs{event: "push"})) require.NoError(t, runExecList(planner, &executeArgs{autodetectEvent: true})) require.NoError(t, runExecList(planner, &executeArgs{})) + emptyPlanner, err := model.NewWorkflowPlanner(t.TempDir(), false) + require.NoError(t, err) + require.NoError(t, runExecList(emptyPlanner, &executeArgs{autodetectEvent: true})) }) require.Contains(t, out, "Build") diff --git a/internal/app/cmd/register.go b/internal/app/cmd/register.go index 3a6c3961..6c1ad10d 100644 --- a/internal/app/cmd/register.go +++ b/internal/app/cmd/register.go @@ -9,7 +9,6 @@ import ( "errors" "fmt" "os" - "os/signal" goruntime "runtime" "strings" @@ -48,25 +47,17 @@ func runRegister(ctx context.Context, regArgs *registerArgs, configFile *string) } if regArgs.NoInteractive { - if err := registerNoInteractive(ctx, *configFile, regArgs); err != nil { - return err - } - } else { - go func() { - if err := registerInteractive(ctx, *configFile, regArgs); err != nil { - logging.Error(err) - os.Exit(1) - return - } - os.Exit(0) - }() - - c := make(chan os.Signal, 1) - signal.Notify(c, os.Interrupt) - <-c + return registerNoInteractive(ctx, *configFile, regArgs) } - return nil + result := make(chan error, 1) + go func() { result <- registerInteractive(ctx, *configFile, regArgs) }() + select { + case err := <-result: + return err + case <-ctx.Done(): + return nil + } } } @@ -214,7 +205,7 @@ func initInputs(regArgs *registerArgs) (*registerInputs, error) { token = regArgs.Token default: token = os.Getenv(registerTokenEnvVar) - if token == "" { + if token == "" && regArgs.NoInteractive { return nil, fmt.Errorf("missing token, token-file argument, or %s environment variable", registerTokenEnvVar) } } diff --git a/internal/app/cmd/register_test.go b/internal/app/cmd/register_test.go index fc940cd2..5f1293ea 100644 --- a/internal/app/cmd/register_test.go +++ b/internal/app/cmd/register_test.go @@ -4,6 +4,8 @@ package cmd import ( + "bytes" + "context" "net/http" "net/http/httptest" "os" @@ -11,11 +13,43 @@ import ( "sync/atomic" "testing" + "gitea.com/gitea/runner/act/common/logging" "gitea.com/gitea/runner/internal/pkg/config" "github.com/stretchr/testify/require" ) +type cancelRegistrationPrompt struct { + cancel context.CancelFunc +} + +func (w cancelRegistrationPrompt) Write(data []byte) (int, error) { + if bytes.Contains(data, []byte("Enter the Gitea instance URL")) { + w.cancel() + } + return len(data), nil +} + +func TestRunRegisterCancelsWhilePrompting(t *testing.T) { + t.Setenv(registerTokenEnvVar, "") + t.Chdir(t.TempDir()) + stdin, input, err := os.Pipe() + require.NoError(t, err) + previousStdin := os.Stdin + os.Stdin = stdin + t.Cleanup(func() { + _ = input.Close() + _ = stdin.Close() + os.Stdin = previousStdin + }) + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + previousOutput := logging.Output() + logging.SetOutput(cancelRegistrationPrompt{cancel}) + t.Cleanup(func() { logging.SetOutput(previousOutput) }) + require.NoError(t, runRegister(ctx, ®isterArgs{}, new(string))(nil, nil)) +} + func TestRegisterNonInteractiveReturnsLabelValidationError(t *testing.T) { err := registerNoInteractive(t.Context(), "", ®isterArgs{ Labels: "ubuntu:host,,broken", @@ -148,6 +182,7 @@ func TestRegisterInputsAssignToNext(t *testing.T) { } func TestInitInputs(t *testing.T) { + t.Setenv(registerTokenEnvVar, "") for _, tc := range []struct { name, tokenFile, wantErr string env *string @@ -164,11 +199,12 @@ func TestInitInputs(t *testing.T) { t.Setenv(registerTokenEnvVar, *tc.env) } _, err := initInputs(®isterArgs{ - InstanceAddr: "http://localhost:3000", - TokenFile: tc.tokenFile, - RunnerName: "runner", - Ephemeral: true, - Labels: " ubuntu:host , ubuntu:docker://node:18 ", + NoInteractive: true, + InstanceAddr: "http://localhost:3000", + TokenFile: tc.tokenFile, + RunnerName: "runner", + Ephemeral: true, + Labels: " ubuntu:host , ubuntu:docker://node:18 ", }) require.EqualError(t, err, tc.wantErr) }) @@ -178,12 +214,16 @@ func TestInitInputs(t *testing.T) { name, address, token, runnerName, wantToken string file bool }{ + {name: "interactive token prompt", address: "http://localhost:3000", runnerName: "runner"}, {name: "valid token", address: " http://localhost:3000 ", token: "from-plain-arg\n", runnerName: "runner\n", wantToken: "from-plain-arg"}, {name: "valid token file", address: "http://localhost:3000", runnerName: "runner", file: true, wantToken: "from-file"}, {name: "token from environment variable", address: "http://localhost:3000", runnerName: "runner", wantToken: "from-env"}, } { t.Run(tc.name, func(t *testing.T) { t.Setenv(registerTokenEnvVar, "from-env") + if tc.wantToken == "" { + t.Setenv(registerTokenEnvVar, "") + } var tokenFile string if tc.file { tokenFile = filepath.Join(t.TempDir(), "from-file") diff --git a/internal/app/poll/poller.go b/internal/app/poll/poller.go index 1c321e31..6d87ae96 100644 --- a/internal/app/poll/poller.go +++ b/internal/app/poll/poller.go @@ -313,10 +313,14 @@ func (p *Poller) calculateInterval(s *workerState) time.Duration { return base } - // Capped exponential backoff: base * 2^(n-1), max shift=5 so multiplier <= 32 - shift := min(n-1, 5) - interval := base * time.Duration(int64(1)<= maxInterval-interval { + return maxInterval + } + interval *= 2 + } + return interval } // addJitter adds +/- 20% random jitter to the given duration to avoid thundering herd. diff --git a/internal/app/poll/poller_test.go b/internal/app/poll/poller_test.go index 83ba0fdf..88deafb1 100644 --- a/internal/app/poll/poller_test.go +++ b/internal/app/poll/poller_test.go @@ -140,6 +140,9 @@ func TestPoller_CalculateInterval(t *testing.T) { assert.Equal(t, tc.wantInterval, p.calculateInterval(s)) }) } + + cfg.Runner.FetchIntervalMax = 2 * time.Minute + assert.Equal(t, 2*time.Minute, p.calculateInterval(&workerState{consecutiveEmpty: 7})) } func atomicMax(target *atomic.Int64, val int64) { diff --git a/internal/app/run/proxy.go b/internal/app/run/proxy.go index c010624c..e7b83aa2 100644 --- a/internal/app/run/proxy.go +++ b/internal/app/run/proxy.go @@ -111,15 +111,20 @@ func WarnIfDaemonHasNoProxy(ctx context.Context) { // proxyPasswords returns the passwords embedded in the runner's proxy URLs, to mask before // a job echoes its environment. -func proxyPasswords() []string { - cfg := proxyFromEnv() - if cfg == nil { - return nil +func proxyPasswords(envs map[string]string) []string { + cfg := httpproxy.FromEnvironment() + raws := []string{cfg.HTTPProxy, cfg.HTTPSProxy} + for name, value := range envs { + if strings.EqualFold(name, "http_proxy") || strings.EqualFold(name, "https_proxy") { // Windows reads env names in any case + raws = append(raws, value) + } } - var passwords []string - for _, raw := range []string{cfg.HTTPProxy, cfg.HTTPSProxy} { + for _, raw := range raws { parsed, err := url.Parse(raw) + if err != nil || parsed.Scheme == "" || parsed.Host == "" { + parsed, err = url.Parse("http://" + raw) + } if err != nil { continue } diff --git a/internal/app/run/proxy_test.go b/internal/app/run/proxy_test.go index 33e1e750..d0dcde78 100644 --- a/internal/app/run/proxy_test.go +++ b/internal/app/run/proxy_test.go @@ -169,10 +169,19 @@ func TestProxyPasswords(t *testing.T) { clearProxyEnv(t) t.Setenv("http_proxy", "http://user:hunter2@proxy:3128") t.Setenv("https_proxy", "http://user:s3cret@proxy:3128") - assert.Equal(t, []string{"hunter2", "s3cret"}, proxyPasswords()) + assert.Equal(t, []string{"hunter2", "s3cret"}, proxyPasswords(nil)) + + t.Setenv("http_proxy", "user:hunter2@proxy:3128") + t.Setenv("https_proxy", "user:s3cret@proxy:3128") + assert.Equal(t, []string{"hunter2", "s3cret"}, proxyPasswords(nil)) t.Setenv("https_proxy", "http://proxy:3128") - assert.Equal(t, []string{"hunter2"}, proxyPasswords()) + assert.Equal(t, []string{"hunter2"}, proxyPasswords(nil)) + + clearProxyEnv(t) + assert.Equal(t, []string{"configured"}, proxyPasswords(map[string]string{ + "Http_Proxy": "http://user:configured@proxy:3128", + })) } func TestAppendNoProxy(t *testing.T) { diff --git a/internal/app/run/runner.go b/internal/app/run/runner.go index 6258f102..8fd09d3a 100644 --- a/internal/app/run/runner.go +++ b/internal/app/run/runner.go @@ -287,7 +287,7 @@ func (r *Runner) cleanupStaleDirs(ctx context.Context, root string, match func(n } } -func (r *Runner) Run(ctx context.Context, task *runnerv1.Task) error { +func (r *Runner) Run(ctx context.Context, task *runnerv1.Task) (err error) { if _, ok := r.runningTasks.LoadOrStore(task.Id, struct{}{}); ok { return fmt.Errorf("task %d is already running", task.Id) } @@ -302,7 +302,7 @@ func (r *Runner) Run(ctx context.Context, task *runnerv1.Task) error { defer cancel() ctx, endJob := telemetry.StartJob(ctx, task) // A proxy URL may carry credentials, and every job is given it; keep them out of the log. - reporter := report.NewReporter(ctx, cancel, r.client, task, r.cfg, proxyPasswords()...) + reporter := report.NewReporter(ctx, cancel, r.client, task, r.cfg, proxyPasswords(r.envs)...) var volumeCleanup []common.Executor var volumeCleanupMu sync.Mutex if r.cfg.Runner.PostTaskScript == "" { @@ -318,7 +318,7 @@ func (r *Runner) Run(ctx context.Context, task *runnerv1.Task) error { if runErr != nil { lastWords = runErr.Error() } - _ = reporter.Close(lastWords) + err = reporter.Close(lastWords) if err := cleanupJobVolumes(ctx, volumeCleanup); err != nil { logging.Warnf("task %d volume cleanup after reporting: %v", task.Id, err) } @@ -584,7 +584,7 @@ func (r *Runner) run(ctx context.Context, task *runnerv1.Task, reporter *report. Env: envs, ProxyEnv: proxyEnv, Secrets: task.Secrets, - ExtraMasks: append(proxyPasswords(), preset.Token), + ExtraMasks: append(proxyPasswords(r.envs), preset.Token), GitHubInstance: strings.TrimSuffix(r.client.Address(), "/"), NoSkipCheckout: true, DisableActEnv: r.cfg.Runner.SetActEnv != nil && !*r.cfg.Runner.SetActEnv, diff --git a/internal/app/run/runner_test.go b/internal/app/run/runner_test.go index bec97f49..eb1b3b6f 100644 --- a/internal/app/run/runner_test.go +++ b/internal/app/run/runner_test.go @@ -6,6 +6,7 @@ package run import ( "context" "encoding/base64" + "errors" "io" "net/http" "net/http/httptest" @@ -187,6 +188,29 @@ func TestCleanupJobVolumesJoinsErrorsAfterCancellation(t *testing.T) { require.ErrorIs(t, err, io.ErrClosedPipe) } +func TestRunnerReturnsMaskedFinalReportingError(t *testing.T) { + cfg, err := config.LoadDefault("") + require.NoError(t, err) + cfg.Cache.Enabled = new(false) + cfg.Runner.ReportCloseTimeout = time.Millisecond + cli := clientmocks.NewClient(t) + cli.AddressValue = "https://gitea.example/" + reportErr := errors.New("reporting unavailable for hunter2") + cli.On("UpdateLog", mock.Anything, mock.Anything).Return(nil, reportErr) + cli.On("UpdateTask", mock.Anything, mock.Anything).Return(nil, reportErr) + r := NewRunner(cfg, &config.Registration{}, cli) + + err = r.Run(t.Context(), &runnerv1.Task{ + Context: &structpb.Struct{}, + WorkflowPayload: []byte("["), + Secrets: map[string]string{"PASSWORD": "hunter2"}, + }) + + require.ErrorContains(t, err, "reporting unavailable for ***") + assert.NotContains(t, err.Error(), "hunter2") + assert.Zero(t, r.RunningCount()) +} + func TestNewRunnerInitializesLabelsAndEnvironment(t *testing.T) { cacheEnabled := false cfg := &config.Config{} diff --git a/internal/pkg/config/edit.go b/internal/pkg/config/edit.go index ed91e3c5..537325b3 100644 --- a/internal/pkg/config/edit.go +++ b/internal/pkg/config/edit.go @@ -460,6 +460,9 @@ func resolvePath(segments []string) (*fieldInfo, error) { strings.Join(segments[:i+1], "."), strings.Join(yamlNames(typ), ", ")) } typ = field.Type + if typ.Kind() == reflect.Pointer { + typ = typ.Elem() + } case reflect.Map: // The segment names a user-defined entry, so the walk ends here. if i != len(segments)-1 { diff --git a/internal/pkg/config/edit_test.go b/internal/pkg/config/edit_test.go index c7366010..943c51e0 100644 --- a/internal/pkg/config/edit_test.go +++ b/internal/pkg/config/edit_test.go @@ -219,6 +219,11 @@ func TestGetValue(t *testing.T) { _, err = GetValue(file, "metrics.addr") require.Error(t, err) assert.Contains(t, err.Error(), "is not set") + + require.NoError(t, SetValue(file, "cache.s3.endpoint", "https://cache.example")) + value, err = GetValue(file, "cache.s3.endpoint") + require.NoError(t, err) + assert.Equal(t, "https://cache.example", value) } func TestEditValuesFileHandling(t *testing.T) { diff --git a/internal/pkg/process/killer_unix_test.go b/internal/pkg/process/killer_unix_test.go index 15d37674..9fa859ad 100644 --- a/internal/pkg/process/killer_unix_test.go +++ b/internal/pkg/process/killer_unix_test.go @@ -98,3 +98,32 @@ func TestKillerKillsTree(t *testing.T) { return !processAlive(childPID) }, 20*time.Second, 100*time.Millisecond, "backgrounded child should be terminated") } + +func TestTreeKillCancelBeforeCaptureKillsTree(t *testing.T) { + stdin, input, err := os.Pipe() + require.NoError(t, err) + t.Cleanup(func() { _ = stdin.Close() }) + t.Cleanup(func() { _ = input.Close() }) + cmd := exec.CommandContext(t.Context(), "/bin/sh", "-c", "cat <&0 & echo $!; read value") + cmd.Stdin = stdin + cmd.SysProcAttr = SysProcAttr("", false) + output, err := cmd.StdoutPipe() + require.NoError(t, err) + treeKill := NewTreeKill(cmd) + require.NoError(t, cmd.Start()) + t.Cleanup(func() { + _ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) + _ = cmd.Wait() + }) + var childPID int + _, err = fmt.Fscan(output, &childPID) + require.NoError(t, err) + require.True(t, processAlive(childPID)) + + require.NoError(t, cmd.Cancel()) + killer, err := treeKill.Capture(cmd.Process) + require.NoError(t, err) + defer killer.Close() + require.Error(t, cmd.Wait()) + require.Eventually(t, func() bool { return !processAlive(childPID) }, 5*time.Second, 10*time.Millisecond) +} diff --git a/internal/pkg/process/treekill.go b/internal/pkg/process/treekill.go index 2073253e..ca2c029a 100644 --- a/internal/pkg/process/treekill.go +++ b/internal/pkg/process/treekill.go @@ -44,6 +44,10 @@ func NewTreeKill(cmd *exec.Cmd) *TreeKill { return k.Kill() } if cmd.Process != nil { + if killer, err := NewKiller(cmd.Process); err == nil { + defer killer.Close() + return killer.Kill() + } return cmd.Process.Kill() } return nil diff --git a/internal/pkg/report/globalmask.go b/internal/pkg/report/globalmask.go index 6edf59ef..83db0f5a 100644 --- a/internal/pkg/report/globalmask.go +++ b/internal/pkg/report/globalmask.go @@ -4,6 +4,8 @@ package report import ( + "slices" + "strconv" "strings" "sync" "sync/atomic" @@ -22,7 +24,13 @@ var ( func registerGlobalMasks(r *Reporter) { globalMu.Lock() defer globalMu.Unlock() - globalMasks[r] = r.oldnew + masks := slices.Clip(r.oldnew) + for i := 0; i < len(r.oldnew); i += 2 { // the console handler Go-quotes values with special characters + if quoted := strconv.Quote(r.oldnew[i]); quoted[1:len(quoted)-1] != r.oldnew[i] { + masks = append(masks, quoted[1:len(quoted)-1], r.oldnew[i+1]) + } + } + globalMasks[r] = masks rebuildGlobalReplacer() } diff --git a/internal/pkg/report/globalmask_test.go b/internal/pkg/report/globalmask_test.go index 76708375..16f99870 100644 --- a/internal/pkg/report/globalmask_test.go +++ b/internal/pkg/report/globalmask_test.go @@ -9,6 +9,7 @@ import ( "testing" "gitea.com/gitea/runner/act/common/logging" + "gitea.com/gitea/runner/act/runner" "github.com/stretchr/testify/assert" ) @@ -61,4 +62,10 @@ func TestGlobalMasks(t *testing.T) { writers.Wait() assert.Contains(t, format("saw runtime-secret"), "runtime-secret") assert.Equal(t, "saw *** ***", third.mask("saw runtime-secret another-secret")) + + secret := "pass\a\v\x1b\u00a0word" + register(runner.AppendSecretMasker(nil, secret)...) + out.Reset() + logging.With("token", secret).Info(secret) + assert.Equal(t, "level=info msg=\"***\" token=\"***\"\n", out.String()) } diff --git a/internal/pkg/report/reporter.go b/internal/pkg/report/reporter.go index 45bbb47e..7817a059 100644 --- a/internal/pkg/report/reporter.go +++ b/internal/pkg/report/reporter.go @@ -279,6 +279,9 @@ func (r *Reporter) enterReportedStep(step *runnerv1.StepState, timestamp time.Ti // cleanup is left running unless closing, as it still logs after the job result. Caller holds stateMu. func (r *Reporter) finishReportedSteps(jobResult runnerv1.Result, timestamp time.Time, closing bool) { for _, v := range r.state.ReportedSteps { + if closing && v.StartedAt != nil && v.StoppedAt == nil { + v.StoppedAt = timestamppb.New(timestamp) + } if v.Result != runnerv1.Result_RESULT_UNSPECIFIED || !closing && v.StartedAt != nil && v.Stage == runnerv1.StepStage_STEP_STAGE_CLEANUP { continue } @@ -716,6 +719,7 @@ func (r *Reporter) Close(lastWords string) error { trailer := fmt.Sprintf("task %d finished: %s", r.state.Id, metrics.ResultToStatusLabel(r.state.Result)) if err != nil { trailer += fmt.Sprintf(", the final flush to Gitea failed: %v", err) + err = errors.New(r.mask(err.Error())) // the caller logs it after this task's masks leave the global masker } r.jobLog.close(r.mask(trailer)) r.stateMu.Unlock() diff --git a/internal/pkg/report/reporter_test.go b/internal/pkg/report/reporter_test.go index 219e8b9b..0367a546 100644 --- a/internal/pkg/report/reporter_test.go +++ b/internal/pkg/report/reporter_test.go @@ -1197,6 +1197,18 @@ func TestReporter_ReportedSteps(t *testing.T) { assert.Equal(t, []runnerv1.Result{failure, skipped, success}, []runnerv1.Result{got[0].result, got[1].result, got[2].result}) }) + t.Run("cleanup failure gets a stop timestamp", func(t *testing.T) { + reporter, _ := newReporter(t, newTestSteps(1)) + send(t, reporter, logging.LevelError, "##[error]job completed hook failed", map[string]any{"stage": runner.StageComplete}) + send(t, reporter, logging.LevelInfo, "Job failed", map[string]any{"stage": runner.StageComplete, "jobResult": model.StepStatusFailure}) + require.NoError(t, reporter.Close("")) + + step := reporter.state.ReportedSteps[len(reporter.state.ReportedSteps)-1] + assert.Equal(t, failure, step.Result) + require.NotNil(t, step.StartedAt) + assert.NotNil(t, step.StoppedAt) + }) + t.Run("step not reached in a succeeding job", func(t *testing.T) { reporter, _ := newReporter(t, newTestSteps(1)) send(t, reporter, logging.LevelInfo, "Job succeeded", map[string]any{"stage": runner.StageComplete, "jobResult": model.StepStatusSuccess})