From da7ef7c2a19cb4e94a9323eefe54c7a723a0aaaa Mon Sep 17 00:00:00 2001 From: Mathieu Fenniak Date: Fri, 15 Aug 2025 20:26:35 -0600 Subject: [PATCH] fix: PRs cache artifacts separate from other runs --- act/artifactcache/handler.go | 29 +++++++++++++++++++++++++++-- act/artifactcache/model.go | 17 +++++++++-------- act/cacheproxy/handler.go | 7 ++++++- internal/app/run/runner.go | 17 ++++++++++++++++- 4 files changed, 58 insertions(+), 12 deletions(-) diff --git a/act/artifactcache/handler.go b/act/artifactcache/handler.go index 0f78be85..8b8572ee 100644 --- a/act/artifactcache/handler.go +++ b/act/artifactcache/handler.go @@ -181,11 +181,19 @@ func (h *Handler) find(w http.ResponseWriter, r *http.Request, params httprouter } defer db.Close() - cache, err := findCache(db, repo, keys, version) + cache, err := findCache(db, repo, keys, version, rundata.WriteIsolationKey) if err != nil { h.responseJSON(w, r, 500, err) return } + // If read was scoped to WriteIsolationKey and didn't find anything, we can fallback to the non-isolated cache read + if cache == nil && rundata.WriteIsolationKey != "" { + cache, err = findCache(db, repo, keys, version, "") + if err != nil { + h.responseJSON(w, r, 500, err) + return + } + } if cache == nil { h.responseJSON(w, r, 204) return @@ -236,6 +244,7 @@ func (h *Handler) reserve(w http.ResponseWriter, r *http.Request, params httprou cache.CreatedAt = now cache.UsedAt = now cache.Repo = repo + cache.WriteIsolationKey = rundata.WriteIsolationKey if err := insertCache(db, cache); err != nil { h.responseJSON(w, r, 500, err) return @@ -275,6 +284,10 @@ func (h *Handler) upload(w http.ResponseWriter, r *http.Request, params httprout h.responseJSON(w, r, 500, fmt.Errorf("cache repo is not valid")) return } + if cache.WriteIsolationKey != rundata.WriteIsolationKey { + h.responseJSON(w, r, 403, fmt.Errorf("cache authorized for write isolation %q, but attempting to operate on %q", rundata.WriteIsolationKey, cache.WriteIsolationKey)) + return + } if cache.Complete { h.responseJSON(w, r, 400, fmt.Errorf("cache %v %q: already complete", cache.ID, cache.Key)) @@ -326,6 +339,10 @@ func (h *Handler) commit(w http.ResponseWriter, r *http.Request, params httprout h.responseJSON(w, r, 500, fmt.Errorf("cache repo is not valid")) return } + if cache.WriteIsolationKey != rundata.WriteIsolationKey { + h.responseJSON(w, r, 403, fmt.Errorf("cache authorized for write isolation %q, but attempting to operate on %q", rundata.WriteIsolationKey, cache.WriteIsolationKey)) + return + } if cache.Complete { h.responseJSON(w, r, 400, fmt.Errorf("cache %v %q: already complete", cache.ID, cache.Key)) @@ -386,6 +403,11 @@ func (h *Handler) get(w http.ResponseWriter, r *http.Request, params httprouter. h.responseJSON(w, r, 500, fmt.Errorf("cache repo is not valid")) return } + // reads permitted against caches w/ the same isolation key, or no isolation key + if cache.WriteIsolationKey != rundata.WriteIsolationKey && cache.WriteIsolationKey != "" { + h.responseJSON(w, r, 403, fmt.Errorf("cache authorized for write isolation %q, but attempting to operate on %q", rundata.WriteIsolationKey, cache.WriteIsolationKey)) + return + } if err := h.useCache(id); err != nil { h.responseJSON(w, r, 500, fmt.Errorf("cache useCache: %w", err)) @@ -417,7 +439,7 @@ func (h *Handler) middleware(handler httprouter.Handle) httprouter.Handle { } // if not found, return (nil, nil) instead of an error. -func findCache(db *bolthold.Store, repo string, keys []string, version string) (*Cache, error) { +func findCache(db *bolthold.Store, repo string, keys []string, version, writeIsolationKey string) (*Cache, error) { cache := &Cache{} for _, prefix := range keys { // if a key in the list matches exactly, don't return partial matches @@ -425,6 +447,7 @@ func findCache(db *bolthold.Store, repo string, keys []string, version string) ( bolthold.Where("Repo").Eq(repo).Index("Repo"). And("Key").Eq(prefix). And("Version").Eq(version). + And("WriteIsolationKey").Eq(writeIsolationKey). And("Complete").Eq(true). SortBy("CreatedAt").Reverse()); err == nil || !errors.Is(err, bolthold.ErrNotFound) { if err != nil { @@ -441,6 +464,7 @@ func findCache(db *bolthold.Store, repo string, keys []string, version string) ( bolthold.Where("Repo").Eq(repo).Index("Repo"). And("Key").RegExp(re). And("Version").Eq(version). + And("WriteIsolationKey").Eq(writeIsolationKey). And("Complete").Eq(true). SortBy("CreatedAt").Reverse()); err != nil { if errors.Is(err, bolthold.ErrNotFound) { @@ -644,5 +668,6 @@ func runDataFromHeaders(r *http.Request) cacheproxy.RunData { RunNumber: r.Header.Get("Forgejo-Cache-RunNumber"), Timestamp: r.Header.Get("Forgejo-Cache-Timestamp"), RepositoryMAC: r.Header.Get("Forgejo-Cache-MAC"), + WriteIsolationKey: r.Header.Get("Forgejo-Cache-WriteIsolationKey"), } } diff --git a/act/artifactcache/model.go b/act/artifactcache/model.go index b27fd8ed..cb1af860 100644 --- a/act/artifactcache/model.go +++ b/act/artifactcache/model.go @@ -24,12 +24,13 @@ func (c *Request) ToCache() *Cache { } type Cache struct { - ID uint64 `json:"id" boltholdKey:"ID"` - Repo string `json:"repo" boltholdIndex:"Repo"` - Key string `json:"key"` - Version string `json:"version"` - Size int64 `json:"cacheSize"` - Complete bool `json:"complete"` - UsedAt int64 `json:"usedAt"` - CreatedAt int64 `json:"createdAt"` + ID uint64 `json:"id" boltholdKey:"ID"` + Repo string `json:"repo" boltholdIndex:"Repo"` + Key string `json:"key"` + Version string `json:"version"` + Size int64 `json:"cacheSize"` + Complete bool `json:"complete"` + UsedAt int64 `json:"usedAt"` + CreatedAt int64 `json:"createdAt"` + WriteIsolationKey string `json:"writeIsolationKey"` } diff --git a/act/cacheproxy/handler.go b/act/cacheproxy/handler.go index 56b083a6..a2d76527 100644 --- a/act/cacheproxy/handler.go +++ b/act/cacheproxy/handler.go @@ -51,15 +51,17 @@ type RunData struct { RunNumber string Timestamp string RepositoryMAC string + WriteIsolationKey string } -func (h *Handler) CreateRunData(fullName, runNumber, timestamp string) RunData { +func (h *Handler) CreateRunData(fullName, runNumber, timestamp, writeIsolationKey string) RunData { mac := computeMac(h.cacheSecret, fullName, runNumber, timestamp) return RunData{ RepositoryFullName: fullName, RunNumber: runNumber, Timestamp: timestamp, RepositoryMAC: mac, + WriteIsolationKey: writeIsolationKey, } } @@ -152,6 +154,9 @@ func (h *Handler) newReverseProxy(targetHost string) (*httputil.ReverseProxy, er r.Out.Header.Set("Forgejo-Cache-Timestamp", runData.Timestamp) r.Out.Header.Set("Forgejo-Cache-MAC", runData.RepositoryMAC) r.Out.Header.Set("Forgejo-Cache-Host", h.ExternalURL()) + if runData.WriteIsolationKey != "" { + r.Out.Header.Set("Forgejo-Cache-WriteIsolationKey", runData.WriteIsolationKey) + } }, } return proxy, nil diff --git a/internal/app/run/runner.go b/internal/app/run/runner.go index b2fcf4fc..86cddd1a 100644 --- a/internal/app/run/runner.go +++ b/internal/app/run/runner.go @@ -265,8 +265,23 @@ func (r *Runner) run(ctx context.Context, task *runnerv1.Task, reporter *report. // Register the run with the cacheproxy and modify the CACHE_URL if r.cacheProxy != nil { + writeIsolationKey := "" + + // When performing an action on an event from a PR, provide a "write isolation key" to the cache. The generated + // ACTIONS_CACHE_URL will be able to read the cache, and write to a cache, but its writes will be isolated to + // future runs of the PR's workflows and won't be shared with other pull requests or actions. This is a security + // measure to prevent a malicious pull request from poisoning the cache with secret-stealing code which would + // later be executed on another action. + if taskContext["event_name"].GetStringValue() == "pull_request" || taskContext["event_name"].GetStringValue() == "pull_request_target" { + // Ensure that `Ref` has the expected format so that we don't end up with a useless write isolation key + if !strings.HasPrefix(preset.Ref, "refs/pull/") { + return fmt.Errorf("write isolation key: expected preset.Ref to be refs/pull/..., but was %q", preset.Ref) + } + writeIsolationKey = preset.Ref + } + timestamp := strconv.FormatInt(time.Now().Unix(), 10) - cacheRunData := r.cacheProxy.CreateRunData(preset.Repository, preset.RunID, timestamp) + cacheRunData := r.cacheProxy.CreateRunData(preset.Repository, preset.RunID, timestamp, writeIsolationKey) cacheRunID, err := r.cacheProxy.AddRun(cacheRunData) if err == nil { defer func() { _ = r.cacheProxy.RemoveRun(cacheRunID) }()