fix(actions): verify raw artifact signatures first (#39049)

Validate raw-artifact signatures before resolving the requested
artifact.

---------

Co-authored-by: silverwind <me@silverwind.io>

routers/api/v1/repo/action.go

@@ -2077,8 +2077,8 @@ func buildSignature(endp string, expires, artifactID int64) []byte {
 	return actions.BuildSignature("api", endp, strconv.FormatInt(expires, 10), strconv.FormatInt(artifactID, 10))
 }
 
-func buildDownloadRawEndpoint(repo *repo_model.Repository, artifactID int64) string {
-	return fmt.Sprintf("api/v1/repos/%s/%s/actions/artifacts/%d/zip/raw", url.PathEscape(repo.OwnerName), url.PathEscape(repo.Name), artifactID)
+func buildDownloadRawEndpoint(ownerName, repoName string, artifactID int64) string {
+	return fmt.Sprintf("api/v1/repos/%s/%s/actions/artifacts/%d/zip/raw", url.PathEscape(ownerName), url.PathEscape(repoName), artifactID)
 }
 
 func buildSigURL(ctx go_context.Context, endPoint string, artifactID int64) string {
@@ -2139,7 +2139,7 @@ func DownloadArtifact(ctx *context.APIContext) {
 
 		// @actions/toolkit asserts a 302 for the artifact download, so we have to build a signed URL and redirect to it
 		// TODO: a perma link to the code for reference
-		redirectURL := buildSigURL(ctx, buildDownloadRawEndpoint(ctx.Repo.Repository, art.ID), art.ID)
+		redirectURL := buildSigURL(ctx, buildDownloadRawEndpoint(ctx.Repo.Repository.OwnerName, ctx.Repo.Repository.Name, art.ID), art.ID)
 		ctx.Redirect(redirectURL, http.StatusFound)
 		return
 	}
@@ -2150,7 +2150,22 @@ func DownloadArtifact(ctx *context.APIContext) {
 // DownloadArtifactRaw Downloads a specific artifact for a workflow run directly.
 func DownloadArtifactRaw(ctx *context.APIContext) {
 	// it doesn't use repoAssignment middleware, so it needs to prepare the repo and check permission (sig) by itself
-	repo, err := repo_model.GetRepositoryByOwnerAndName(ctx, ctx.PathParam("username"), ctx.PathParam("reponame"))
+	ownerName, repoName := ctx.PathParam("username"), ctx.PathParam("reponame")
+	query := ctx.Req.URL.Query()
+	sigBytes, _ := base64.RawURLEncoding.DecodeString(query.Get("sig"))
+	expires, _ := strconv.ParseInt(query.Get("expires"), 10, 64)
+	artifactID := ctx.PathParamInt64("artifact_id")
+
+	if !hmac.Equal(sigBytes, buildSignature(buildDownloadRawEndpoint(ownerName, repoName, artifactID), expires, artifactID)) {
+		ctx.APIErrorNotFound()
+		return
+	}
+	if time.Unix(expires, 0).Before(time.Now()) {
+		ctx.APIError(http.StatusUnauthorized, "Error link expired")
+		return
+	}
+
+	repo, err := repo_model.GetRepositoryByOwnerAndName(ctx, ownerName, repoName)
 	if err != nil {
 		if errors.Is(err, util.ErrNotExist) {
 			ctx.APIErrorNotFound()
@@ -2164,22 +2179,6 @@ func DownloadArtifactRaw(ctx *context.APIContext) {
 		return
 	}
 
-	sigStr := ctx.Req.URL.Query().Get("sig")
-	expiresStr := ctx.Req.URL.Query().Get("expires")
-	sigBytes, _ := base64.RawURLEncoding.DecodeString(sigStr)
-	expires, _ := strconv.ParseInt(expiresStr, 10, 64)
-
-	expectedSig := buildSignature(buildDownloadRawEndpoint(repo, art.ID), expires, art.ID)
-	if !hmac.Equal(sigBytes, expectedSig) {
-		ctx.APIError(http.StatusUnauthorized, "Error unauthorized")
-		return
-	}
-	t := time.Unix(expires, 0)
-	if t.Before(time.Now()) {
-		ctx.APIError(http.StatusUnauthorized, "Error link expired")
-		return
-	}
-
 	// if artifacts status is not uploaded-confirmed, treat it as not found
 	if art.Status == actions_model.ArtifactStatusExpired {
 		ctx.APIError(http.StatusNotFound, "Artifact has expired")

tests/integration/api_actions_artifact_v4_test.go

@@ -662,10 +662,25 @@ func TestActionsArtifactV4DownloadSinglePublicApi(t *testing.T) {
 	body := strings.Repeat("D", 1024)
 	assert.Equal(t, body, resp.Body.String())
 
-	// confirm artifact can not be downloaded without query
-	req = NewRequestWithBody(t, "GET", blobLocation, nil)
+	req = NewRequestWithBody(t, "GET", blobLocation, nil).AddTokenAuth(token)
 	req.URL.RawQuery = ""
-	_ = MakeRequest(t, req, http.StatusUnauthorized)
+	notFoundResp := MakeRequest(t, req, http.StatusNotFound)
+
+	otherRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
+	require.NotEqual(t, repo.ID, otherRepo.ID)
+	for _, testCase := range []struct {
+		name string
+		path string
+	}{
+		{"other repository", fmt.Sprintf("/api/v1/repos/%s/actions/artifacts/%d/zip/raw", otherRepo.FullName(), listResp.Entries[0].ID)},
+		{"missing artifact", fmt.Sprintf("/api/v1/repos/%s/actions/artifacts/0/zip/raw", repo.FullName())},
+		{"missing repository", fmt.Sprintf("/api/v1/repos/%s/does-not-exist/actions/artifacts/%d/zip/raw", repo.OwnerName, listResp.Entries[0].ID)},
+	} {
+		t.Run(testCase.name, func(t *testing.T) {
+			invalidResp := MakeRequest(t, NewRequestWithBody(t, "GET", testCase.path, nil), http.StatusNotFound)
+			assert.Equal(t, notFoundResp.Body.String(), invalidResp.Body.String())
+		})
+	}
 }
 
 func TestActionsArtifactV4DownloadSinglePublicApiPrivateRepo(t *testing.T) {
@@ -703,7 +718,7 @@ func TestActionsArtifactV4DownloadSinglePublicApiPrivateRepo(t *testing.T) {
 	// confirm artifact can not be downloaded without query
 	req = NewRequestWithBody(t, "GET", blobLocation, nil)
 	req.URL.RawQuery = ""
-	_ = MakeRequest(t, req, http.StatusUnauthorized)
+	_ = MakeRequest(t, req, http.StatusNotFound)
 }
 
 func TestActionsArtifactV4ListAndGetPublicApi(t *testing.T) {
@@ -783,20 +798,6 @@ func TestActionsArtifactV4DownloadArtifactCorrectRepoFound(t *testing.T) {
 	MakeRequest(t, req, http.StatusFound)
 }
 
-func TestActionsArtifactV4DownloadRawArtifactCorrectRepoMissingSignatureUnauthorized(t *testing.T) {
-	defer prepareTestEnvActionsArtifacts(t)()
-
-	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 4})
-	user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID})
-	session := loginUser(t, user.Name)
-	token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeWriteRepository)
-
-	// confirm cannot use the raw artifact endpoint even with a correct access token
-	req := NewRequestWithBody(t, "GET", fmt.Sprintf("/api/v1/repos/%s/actions/artifacts/%d/zip/raw", repo.FullName(), 22), nil).
-		AddTokenAuth(token)
-	MakeRequest(t, req, http.StatusUnauthorized)
-}
-
 func TestActionsArtifactV4Delete(t *testing.T) {
 	defer prepareTestEnvActionsArtifacts(t)()