fix: event.pull_request.action == closed can use the cache of the base repository (#1031)

It was tested locally with https://code.forgejo.org/forgejo/end-to-end/pulls/1062/files.

## Before

![image](/attachments/26890572-08ba-4904-b6a5-f01762141a51)

## After

![image](/attachments/56352930-99a3-4011-b921-b12217c5fa54)

---

When the "closed" action of a pull request event was triggered by a merge, it effectively runs in the context of the base repository. It was merged by a user with write access to the base repository. It is authorized to write the base repository cache.

Resolves forgejo/runner#1030

<!--start release-notes-assistant-->
<!--URL:https://code.forgejo.org/forgejo/runner-->
- bug fixes
  - [PR](https://code.forgejo.org/forgejo/runner/pulls/1031): <!--number 1031 --><!--line 0 --><!--description Zml4OiBldmVudC5wdWxsX3JlcXVlc3QuYWN0aW9uID09IGNsb3NlZCBjYW4gdXNlIHRoZSBjYWNoZSBvZiB0aGUgYmFzZSByZXBvc2l0b3J5-->fix: event.pull_request.action == closed can use the cache of the base repository<!--description-->
<!--end release-notes-assistant-->

Reviewed-on: https://code.forgejo.org/forgejo/runner/pulls/1031
Reviewed-by: Michael Kriese <michael.kriese@gmx.de>
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.code.forgejo.org>
Co-authored-by: Earl Warren <contact@earl-warren.org>
Co-committed-by: Earl Warren <contact@earl-warren.org>

internal/app/run/runner.go

@@ -193,6 +193,44 @@ func explainFailedGenerateWorkflow(task *runnerv1.Task, log func(message string,
 	return fmt.Errorf("the workflow file is not usable")
 }
 
+func getWriteIsolationKey(ctx context.Context, eventName, ref string, event map[string]any) (string, error) {
+	if eventName == "pull_request" {
+		// The "closed" action of a pull request event runs in the context of the base repository
+		// and was merged by a user with write access to the base repository. It is authorized to
+		// write the repository cache.
+		if event["action"] == "closed" {
+			pullRequest, ok := event["pull_request"].(map[string]any)
+			if !ok {
+				return "", fmt.Errorf("getWriteIsolationKey: event.pull_request is not a map[string]any but %T", event["pull_request"])
+			}
+			merged, ok := pullRequest["merged"].(bool)
+			if !ok {
+				return "", fmt.Errorf("getWriteIsolationKey: event.pull_request.merged is not a bool but %T", pullRequest["merged"])
+			}
+			if merged {
+				return "", nil
+			}
+			// a pull request that is closed but not merged falls thru and is expected to obey the same
+			// constraints as an opened pull request, it may be closed by a user with no write permissions to the
+			// base repository
+		}
+		// When performing an action on an event from an opened 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.
+		// Ensure that `ref` has the expected format so that we don't end up with a useless write isolation key
+		if !strings.HasPrefix(ref, "refs/pull/") {
+			return "", fmt.Errorf("getWriteIsolationKey: expected ref to be refs/pull/..., but was %q", ref)
+		}
+		return ref, nil
+	}
+
+	// Other events do not allow the trigger user to modify the content of the repository and
+	// are allowed to write the cache without an isolation key
+	return "", nil
+}
+
 func (r *Runner) run(ctx context.Context, task *runnerv1.Task, reporter *report.Reporter) (err error) {
 	defer func() {
 		if r := recover(); r != nil {
@@ -226,15 +264,18 @@ func (r *Runner) run(ctx context.Context, task *runnerv1.Task, reporter *report.
 		defaultActionURL,
 		r.client.Address())
 
+	eventName := taskContext["event_name"].GetStringValue()
+	ref := taskContext["ref"].GetStringValue()
+	event := taskContext["event"].GetStructValue().AsMap()
 	preset := &model.GithubContext{
-		Event:           taskContext["event"].GetStructValue().AsMap(),
+		Event:           event,
 		RunID:           taskContext["run_id"].GetStringValue(),
 		RunNumber:       taskContext["run_number"].GetStringValue(),
 		Actor:           taskContext["actor"].GetStringValue(),
 		Repository:      taskContext["repository"].GetStringValue(),
-		EventName:       taskContext["event_name"].GetStringValue(),
+		EventName:       eventName,
 		Sha:             taskContext["sha"].GetStringValue(),
-		Ref:             taskContext["ref"].GetStringValue(),
+		Ref:             ref,
 		RefName:         taskContext["ref_name"].GetStringValue(),
 		RefType:         taskContext["ref_type"].GetStringValue(),
 		HeadRef:         taskContext["head_ref"].GetStringValue(),
@@ -264,19 +305,9 @@ 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 := ""
+		writeIsolationKey, err := getWriteIsolationKey(ctx, eventName, ref, event)
-
+		if err != nil {
-		// When performing an action on an event from a PR, provide a "write isolation key" to the cache. The generated
+			return err
-		// 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" {
-			// 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)

internal/app/run/runner_test.go

@@ -141,6 +141,83 @@ func (m *forgejoClientMock) UpdateLog(ctx context.Context, request *connect.Requ
 	}), nil
 }
 
+func TestRunner_getWriteIsolationKey(t *testing.T) {
+	t.Run("push", func(t *testing.T) {
+		key, err := getWriteIsolationKey(t.Context(), "push", "whatever", nil)
+		require.NoError(t, err)
+		assert.Empty(t, key)
+	})
+
+	t.Run("pull_request synchronized key is ref", func(t *testing.T) {
+		expectedKey := "refs/pull/1/head"
+		actualKey, err := getWriteIsolationKey(t.Context(), "pull_request", expectedKey, map[string]any{
+			"action": "synchronized",
+		})
+		require.NoError(t, err)
+		assert.Equal(t, expectedKey, actualKey)
+	})
+
+	t.Run("pull_request synchronized ref is invalid", func(t *testing.T) {
+		invalidKey := "refs/is/invalid"
+		key, err := getWriteIsolationKey(t.Context(), "pull_request", invalidKey, map[string]any{
+			"action": "synchronized",
+		})
+		require.Empty(t, key)
+		assert.ErrorContains(t, err, invalidKey)
+	})
+
+	t.Run("pull_request closed and not merged key is ref", func(t *testing.T) {
+		expectedKey := "refs/pull/1/head"
+		actualKey, err := getWriteIsolationKey(t.Context(), "pull_request", expectedKey, map[string]any{
+			"action": "closed",
+			"pull_request": map[string]any{
+				"merged": false,
+			},
+		})
+		require.NoError(t, err)
+		assert.Equal(t, expectedKey, actualKey)
+	})
+
+	t.Run("pull_request closed and merged key is empty", func(t *testing.T) {
+		key, err := getWriteIsolationKey(t.Context(), "pull_request", "whatever", map[string]any{
+			"action": "closed",
+			"pull_request": map[string]any{
+				"merged": true,
+			},
+		})
+		require.NoError(t, err)
+		assert.Empty(t, key)
+	})
+
+	t.Run("pull_request missing event.pull_request", func(t *testing.T) {
+		key, err := getWriteIsolationKey(t.Context(), "pull_request", "whatever", map[string]any{
+			"action": "closed",
+		})
+		require.Empty(t, key)
+		assert.ErrorContains(t, err, "event.pull_request is not a map")
+	})
+
+	t.Run("pull_request missing event.pull_request.merge", func(t *testing.T) {
+		key, err := getWriteIsolationKey(t.Context(), "pull_request", "whatever", map[string]any{
+			"action":       "closed",
+			"pull_request": map[string]any{},
+		})
+		require.Empty(t, key)
+		assert.ErrorContains(t, err, "event.pull_request.merged is not a bool")
+	})
+
+	t.Run("pull_request with event.pull_request.merge of an unexpected type", func(t *testing.T) {
+		key, err := getWriteIsolationKey(t.Context(), "pull_request", "whatever", map[string]any{
+			"action": "closed",
+			"pull_request": map[string]any{
+				"merged": "string instead of bool",
+			},
+		})
+		require.Empty(t, key)
+		assert.ErrorContains(t, err, "not a bool but string")
+	})
+}
+
 func TestRunnerCacheConfiguration(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skipping integration test")