fix!: use run ID instead of run Index in artifacts download web views

- the run ID used to download artifacts is absolute (ID) instead of being
  relative to the repository (Index) for compatibility with the url built
  and returned as `artifact-url` by the the upload-artifact@v4 action.
- this is a breaking change because URLs to download artifacts
  previous saved/bookmarked and not yet expired expired are no
  longer working, they need to be looked up again by visiting the job
  web page.

- add unit tests for getRunByID().
- RepoActionView.test.js verifies the download URL is built using the
  run ID.
- lAdd integration tests to verify the RunID is set as expected in
  the template used by RepoActionView.vue.

Refs https://code.forgejo.org/forgejo/runner/issues/187

models/actions/run.go

@@ -309,15 +309,26 @@ func GetLatestRunForBranchAndWorkflow(ctx context.Context, repoID int64, branch,
 }
 
 func GetRunByID(ctx context.Context, id int64) (*ActionRun, error) {
-	var run ActionRun
+	run, has, err := GetRunByIDWithHas(ctx, id)
-	has, err := db.GetEngine(ctx).Where("id=?", id).Get(&run)
 	if err != nil {
 		return nil, err
 	} else if !has {
 		return nil, fmt.Errorf("run with id %d: %w", id, util.ErrNotExist)
 	}
 
-	return &run, nil
+	return run, nil
+}
+
+func GetRunByIDWithHas(ctx context.Context, id int64) (*ActionRun, bool, error) {
+	var run ActionRun
+	has, err := db.GetEngine(ctx).Where("id=?", id).Get(&run)
+	if err != nil {
+		return nil, false, err
+	} else if !has {
+		return nil, false, nil
+	}
+
+	return &run, true, nil
 }
 
 func GetRunByIndex(ctx context.Context, repoID, index int64) (*ActionRun, error) {

release-notes/9023.md

@@ -0,0 +1 @@
+The `artifact-url` ouput [returned by the upload-artifact@v4 action](https://code.forgejo.org/actions/upload-artifact#outputs) can be used to download the artifact. It was previously 404. To implement this compatibility fix, the web UI URL to download artifacts (i.e. `/{owner}/{repo}/actions/runs/{run_id}/artifacts/{artifact_name}`) now relies on an identifier that is unique accross the instance. URLs to download artifacts that were bookmarked or copied prior to this change use an id relative to the repository and will no longer work. It previously was `/{owner}/{repo}/actions/runs/{run_index}/artifacts/{artifact_name}`, note the difference between `{run_id}` and `{run_index}`. The new URL can be obtained again by visiting the parent page, which still uses the relative id (`/{owner}/{repo}/actions/runs/{run_index}`).

routers/web/repo/actions/view.go

@@ -53,6 +53,7 @@ func View(ctx *context_module.Context) {
 	workflowName := job.Run.WorkflowID
 
 	ctx.Data["RunIndex"] = runIndex
+	ctx.Data["RunID"] = job.Run.ID
 	ctx.Data["JobIndex"] = jobIndex
 	ctx.Data["ActionsURL"] = ctx.Repo.RepoLink + "/actions"
 	ctx.Data["WorkflowName"] = workflowName
@@ -668,6 +669,31 @@ func ArtifactsDeleteView(ctx *context_module.Context) {
 	ctx.JSON(http.StatusOK, struct{}{})
 }
 
+func getRunByID(ctx *context_module.Context, runID int64) *actions_model.ActionRun {
+	if runID == 0 {
+		log.Debug("Requested runID is zero.")
+		ctx.Error(http.StatusNotFound, "zero is not a valid run ID")
+		return nil
+	}
+
+	run, has, err := actions_model.GetRunByIDWithHas(ctx, runID)
+	if err != nil {
+		ctx.Error(http.StatusInternalServerError, err.Error())
+		return nil
+	}
+	if !has {
+		log.Debug("Requested runID[%d] not found.", runID)
+		ctx.Error(http.StatusNotFound, fmt.Sprintf("no such run %d", runID))
+		return nil
+	}
+	if run.RepoID != ctx.Repo.Repository.ID {
+		log.Debug("Requested runID[%d] does not belong to repo[%-v].", runID, ctx.Repo.Repository)
+		ctx.Error(http.StatusNotFound, "no such run")
+		return nil
+	}
+	return run
+}
+
 func artifactsFind(ctx *context_module.Context, opts actions_model.FindArtifactsOptions) []*actions_model.ActionArtifact {
 	artifacts, err := db.Find[actions_model.ActionArtifact](ctx, opts)
 	if err != nil {
@@ -711,19 +737,12 @@ func artifactsFindByNameOrID(ctx *context_module.Context, runID int64, nameOrID
 }
 
 func ArtifactsDownloadView(ctx *context_module.Context) {
-	runIndex := ctx.ParamsInt64("run")
+	run := getRunByID(ctx, ctx.ParamsInt64("run"))
+	if ctx.Written() {
+		return
+	}
 	artifactNameOrID := ctx.Params("artifact_name_or_id")
 
-	run, err := actions_model.GetRunByIndex(ctx, ctx.Repo.Repository.ID, runIndex)
-	if err != nil {
-		if errors.Is(err, util.ErrNotExist) {
-			ctx.Error(http.StatusNotFound, err.Error())
-			return
-		}
-		ctx.Error(http.StatusInternalServerError, err.Error())
-		return
-	}
-
 	artifacts := artifactsFindByNameOrID(ctx, run.ID, artifactNameOrID)
 	if ctx.Written() {
 		return

routers/web/repo/actions/view_test.go

@@ -8,12 +8,54 @@ import (
 	"testing"
 
 	actions_model "forgejo.org/models/actions"
+	repo_model "forgejo.org/models/repo"
 	unittest "forgejo.org/models/unittest"
 	"forgejo.org/services/contexttest"
 
 	"github.com/stretchr/testify/assert"
 )
 
+func Test_getRunByID(t *testing.T) {
+	unittest.PrepareTestEnv(t)
+
+	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{OwnerID: 5, ID: 4})
+
+	for _, testCase := range []struct {
+		name  string
+		runID int64
+		err   string
+	}{
+		{
+			name:  "Found",
+			runID: 792,
+		},
+		{
+			name:  "NotFound",
+			runID: 24344,
+			err:   "no such run",
+		},
+		{
+			name:  "ZeroNotFound",
+			runID: 0,
+			err:   "zero is not a valid run ID",
+		},
+	} {
+		t.Run(testCase.name, func(t *testing.T) {
+			ctx, resp := contexttest.MockContext(t, fmt.Sprintf("user5/repo4/actions/runs/%v/artifacts/some-name", testCase.runID))
+			ctx.Repo.Repository = repo
+			run := getRunByID(ctx, testCase.runID)
+			if testCase.err == "" {
+				assert.NotNil(t, run)
+				assert.False(t, ctx.Written(), resp.Body.String())
+			} else {
+				assert.Nil(t, run)
+				assert.True(t, ctx.Written())
+				assert.Contains(t, resp.Body.String(), testCase.err)
+			}
+		})
+	}
+}
+
 func Test_artifactsFind(t *testing.T) {
 	unittest.PrepareTestEnv(t)
 

templates/repo/actions/view.tmpl

@@ -4,6 +4,7 @@
 	{{template "repo/header" .}}
 	<div id="repo-action-view"
 		data-run-index="{{.RunIndex}}"
+		data-run-id="{{.RunID}}"
 		data-job-index="{{.JobIndex}}"
 		data-actions-url="{{.ActionsURL}}"
 		data-workflow-name="{{.WorkflowName}}"

tests/integration/actions_view_test.go

@@ -4,6 +4,8 @@
 package integration
 
 import (
+	"bytes"
+	"fmt"
 	"net/http"
 	"net/url"
 	"strings"
@@ -58,29 +60,53 @@ func TestActionsViewArtifactDeletion(t *testing.T) {
 func TestActionViewsArtifactDownload(t *testing.T) {
 	defer prepareTestEnvActionsArtifacts(t)()
 
+	assertDataAttrs := func(t *testing.T, body *bytes.Buffer, runID int64) {
+		t.Helper()
+		run := unittest.AssertExistsAndLoadBean(t, &actions_model.ActionRun{ID: runID})
+		htmlDoc := NewHTMLParser(t, body)
+		selector := "#repo-action-view"
+		htmlDoc.AssertAttrEqual(t, selector, "data-run-id", fmt.Sprintf("%d", run.ID))
+		htmlDoc.AssertAttrEqual(t, selector, "data-run-index", fmt.Sprintf("%d", run.Index))
+	}
+
 	t.Run("V3", func(t *testing.T) {
-		req := NewRequest(t, "GET", "/user5/repo4/actions/runs/187/artifacts")
+		runIndex := 187
+		runID := int64(791)
+
+		req := NewRequest(t, "GET", fmt.Sprintf("/user5/repo4/actions/runs/%d/artifacts", runIndex))
 		resp := MakeRequest(t, req, http.StatusOK)
 		assert.JSONEq(t, `{"artifacts":[{"name":"multi-file-download","size":2048,"status":"completed"}]}`, strings.TrimSuffix(resp.Body.String(), "\n"))
 
-		req = NewRequest(t, "GET", "/user5/repo4/actions/runs/187/artifacts/multi-file-download")
+		req = NewRequest(t, "GET", fmt.Sprintf("/user5/repo4/actions/runs/%d", runIndex))
+		resp = MakeRequest(t, req, http.StatusOK)
+		assertDataAttrs(t, resp.Body, runID)
+
+		req = NewRequest(t, "GET", fmt.Sprintf("/user5/repo4/actions/runs/%d/artifacts/multi-file-download", runID))
 		resp = MakeRequest(t, req, http.StatusOK)
 		assert.Contains(t, resp.Header().Get("content-disposition"), "multi-file-download.zip")
 	})
 
 	t.Run("V4", func(t *testing.T) {
-		req := NewRequest(t, "GET", "/user5/repo4/actions/runs/188/artifacts")
+		runIndex := 188
+		runID := int64(792)
+
+		req := NewRequest(t, "GET", fmt.Sprintf("/user5/repo4/actions/runs/%d/artifacts", runIndex))
 		resp := MakeRequest(t, req, http.StatusOK)
 		assert.JSONEq(t, `{"artifacts":[{"name":"artifact-v4-download","size":1024,"status":"completed"}]}`, strings.TrimSuffix(resp.Body.String(), "\n"))
 
-		req = NewRequest(t, "GET", "/user5/repo4/actions/runs/188/artifacts/artifact-v4-download")
+		req = NewRequest(t, "GET", fmt.Sprintf("/user5/repo4/actions/runs/%d", runIndex))
+		resp = MakeRequest(t, req, http.StatusOK)
+		assertDataAttrs(t, resp.Body, runID)
+
+		download := fmt.Sprintf("/user5/repo4/actions/runs/%d/artifacts/artifact-v4-download", runID)
+		req = NewRequest(t, "GET", download)
 		resp = MakeRequest(t, req, http.StatusOK)
 		assert.Equal(t, "bytes", resp.Header().Get("accept-ranges"))
 		assert.Contains(t, resp.Header().Get("content-disposition"), "artifact-v4-download.zip")
 		assert.Equal(t, strings.Repeat("D", 1024), resp.Body.String())
 
 		// Partial artifact download
-		req = NewRequest(t, "GET", "/user5/repo4/actions/runs/188/artifacts/artifact-v4-download").SetHeader("range", "bytes=0-99")
+		req = NewRequest(t, "GET", download).SetHeader("range", "bytes=0-99")
 		resp = MakeRequest(t, req, http.StatusPartialContent)
 		assert.Equal(t, "bytes 0-99/1024", resp.Header().Get("content-range"))
 		assert.Equal(t, strings.Repeat("D", 100), resp.Body.String())

tests/integration/html_helper.go

@@ -26,6 +26,17 @@ func NewHTMLParser(t testing.TB, body *bytes.Buffer) *HTMLDoc {
 	return &HTMLDoc{doc: doc}
 }
 
+func (doc *HTMLDoc) AssertAttrEqual(t testing.TB, selector, attr, expected string) bool {
+	t.Helper()
+	selection := doc.doc.Find(selector)
+	require.NotEmpty(t, selection, selector)
+
+	actual, exists := selection.Attr(attr)
+	require.True(t, exists, "%s not found in %s", attr, selection.Text())
+
+	return assert.Equal(t, expected, actual)
+}
+
 // GetInputValueByID for get input value by id
 func (doc *HTMLDoc) GetInputValueByID(id string) string {
 	text, _ := doc.doc.Find("#" + id).Attr("value")

web_src/js/components/RepoActionView.test.js

@@ -215,3 +215,104 @@ test('load multiple steps on a finished action', async () => {
   expect(wrapper.get('.job-step-section:nth-of-type(2) .job-log-line:nth-of-type(2) .log-msg').text()).toEqual('Step #2 Log #2');
   expect(wrapper.get('.job-step-section:nth-of-type(2) .job-log-line:nth-of-type(3) .log-msg').text()).toEqual('Step #2 Log #3');
 });
+
+test('artifacts download links', async () => {
+  Object.defineProperty(document.documentElement, 'lang', {value: 'en'});
+  vi.spyOn(global, 'fetch').mockImplementation((url, opts) => {
+    if (url.endsWith('/artifacts')) {
+      return Promise.resolve({
+        ok: true,
+        json: vi.fn().mockResolvedValue(
+          {
+            artifacts: [
+              {name: 'artifactname1', size: 111, status: 'completed'},
+              {name: 'artifactname2', size: 222, status: 'expired'},
+            ],
+          },
+        ),
+      });
+    }
+
+    const postBody = JSON.parse(opts.body);
+    const stepsLog_value = [];
+    for (const cursor of postBody.logCursors) {
+      if (cursor.expanded) {
+        stepsLog_value.push(
+          {
+            step: cursor.step,
+            cursor: 0,
+            lines: [
+              {index: 1, message: `Step #${cursor.step + 1} Log #1`, timestamp: 0},
+            ],
+          },
+        );
+      }
+    }
+    const jobs_value = {
+      state: {
+        run: {
+          status: 'success',
+          commit: {
+            pusher: {},
+          },
+        },
+        currentJob: {
+          steps: [
+            {
+              summary: 'Test Step #1',
+              duration: '1s',
+              status: 'success',
+            },
+          ],
+        },
+      },
+      logs: {
+        stepsLog: opts.body?.includes('"cursor":null') ? stepsLog_value : [],
+      },
+    };
+
+    return Promise.resolve({
+      ok: true,
+      json: vi.fn().mockResolvedValue(
+        jobs_value,
+      ),
+    });
+  });
+
+  const wrapper = mount(RepoActionView, {
+    props: {
+      actionsURL: 'https://example.com/example-org/example-repo/actions',
+      runIndex: '10',
+      runID: '1001',
+      jobIndex: '2',
+      locale: {
+        approve: '',
+        cancel: '',
+        rerun: '',
+        artifactsTitle: 'artifactTitleHere',
+        areYouSure: '',
+        confirmDeleteArtifact: '',
+        rerun_all: '',
+        showTimeStamps: '',
+        showLogSeconds: '',
+        showFullScreen: '',
+        downloadLogs: '',
+        status: {
+          unknown: '',
+          waiting: '',
+          running: '',
+          success: '',
+          failure: '',
+          cancelled: '',
+          skipped: '',
+          blocked: '',
+        },
+      },
+    },
+  });
+  await flushPromises();
+
+  expect(wrapper.get('.job-artifacts .job-artifacts-title').text()).toEqual('artifactTitleHere');
+  expect(wrapper.get('.job-artifacts .job-artifacts-item:nth-of-type(1) .job-artifacts-link').attributes('href')).toEqual('https://example.com/example-org/example-repo/actions/runs/1001/artifacts/artifactname1');
+  expect(wrapper.get('.job-artifacts .job-artifacts-item:nth-of-type(2) .job-artifacts-link').attributes('href')).toEqual('https://example.com/example-org/example-repo/actions/runs/1001/artifacts/artifactname2');
+});

web_src/js/components/RepoActionView.vue

@@ -15,6 +15,7 @@ const sfc = {
   },
   props: {
     runIndex: String,
+    runID: String,
     jobIndex: String,
     actionsURL: String,
     workflowName: String,
@@ -386,6 +387,7 @@ export function initRepositoryActionView() {
 
   const view = createApp(sfc, {
     runIndex: el.getAttribute('data-run-index'),
+    runID: el.getAttribute('data-run-id'),
     jobIndex: el.getAttribute('data-job-index'),
     actionsURL: el.getAttribute('data-actions-url'),
     workflowName: el.getAttribute('data-workflow-name'),
@@ -473,7 +475,7 @@ export function initRepositoryActionView() {
           </div>
           <ul class="job-artifacts-list">
             <li class="job-artifacts-item" v-for="artifact in artifacts" :key="artifact.name">
-              <a class="job-artifacts-link" target="_blank" :href="run.link+'/artifacts/'+artifact.name">
+              <a class="job-artifacts-link" target="_blank" :href="actionsURL+'/runs/'+runID+'/artifacts/'+artifact.name">
                 <SvgIcon name="octicon-file" class="ui text black job-artifacts-icon"/>{{ artifact.name }}
               </a>
               <a v-if="run.canDeleteArtifact" @click="deleteArtifact(artifact.name)" class="job-artifacts-delete">