Fix various bugs (#35684) (#35696)

Backport #35684 by wxiaoguang

models/issues/issue_search.go

@@ -476,7 +476,7 @@ func applySubscribedCondition(sess *xorm.Session, subscriberID int64) {
 			),
 			builder.Eq{"issue.poster_id": subscriberID},
 			builder.In("issue.repo_id", builder.
-				Select("id").
+				Select("repo_id").
 				From("watch").
 				Where(builder.And(builder.Eq{"user_id": subscriberID},
 					builder.In("mode", repo_model.WatchModeNormal, repo_model.WatchModeAuto))),

models/issues/issue_test.go

@@ -197,6 +197,12 @@ func TestIssues(t *testing.T) {
 			},
 			[]int64{2},
 		},
+		{
+			issues_model.IssuesOptions{
+				SubscriberID: 11,
+			},
+			[]int64{11, 5, 9, 8, 3, 2, 1},
+		},
 	} {
 		issues, err := issues_model.Issues(t.Context(), &test.Opts)
 		assert.NoError(t, err)

modules/packages/debian/metadata.go

@@ -46,7 +46,7 @@ var (
 	// https://www.debian.org/doc/debian-policy/ch-controlfields.html#source
 	namePattern = regexp.MustCompile(`\A[a-z0-9][a-z0-9+-.]+\z`)
 	// https://www.debian.org/doc/debian-policy/ch-controlfields.html#version
-	versionPattern = regexp.MustCompile(`\A(?:[0-9]:)?[a-zA-Z0-9.+~]+(?:-[a-zA-Z0-9.+-~]+)?\z`)
+	versionPattern = regexp.MustCompile(`\A(?:(0|[1-9][0-9]*):)?[a-zA-Z0-9.+~]+(?:-[a-zA-Z0-9.+-~]+)?\z`)
 )
 
 type Package struct {

modules/packages/debian/metadata_test.go

@@ -176,4 +176,12 @@ func TestParseControlFile(t *testing.T) {
 		assert.Equal(t, []string{"a", "b"}, p.Metadata.Dependencies)
 		assert.Equal(t, full, p.Control)
 	})
+
+	t.Run("ValidVersions", func(t *testing.T) {
+		for _, version := range []string{"1.0", "0:1.2", "9:1.0", "10:1.0", "900:1a.2b-x-y_z~1+2"} {
+			p, err := ParseControlFile(buildContent("testpkg", version, "amd64"))
+			assert.NoError(t, err, "ParseControlFile with version %q", version)
+			assert.NotNil(t, p)
+		}
+	})
 }

modules/web/routing/logger.go

@@ -104,7 +104,8 @@ func logPrinter(logger log.Logger) func(trigger Event, record *requestRecord) {
 		}
 		logf := logInfo
 		// lower the log level for some specific requests, in most cases these logs are not useful
-		if strings.HasPrefix(req.RequestURI, "/assets/") /* static assets */ ||
+		if status > 0 && status < 400 &&
+			strings.HasPrefix(req.RequestURI, "/assets/") /* static assets */ ||
 			req.RequestURI == "/user/events" /* Server-Sent Events (SSE) handler */ ||
 			req.RequestURI == "/api/actions/runner.v1.RunnerService/FetchTask" /* Actions Runner polling */ {
 			logf = logTrace

routers/api/v1/repo/migrate.go

@@ -4,7 +4,7 @@
 package repo
 
 import (
-	"bytes"
+	gocontext "context"
 	"errors"
 	"fmt"
 	"net/http"
@@ -173,7 +173,7 @@ func Migrate(ctx *context.APIContext) {
 		opts.AWSSecretAccessKey = form.AWSSecretAccessKey
 	}
 
-	repo, err := repo_service.CreateRepositoryDirectly(ctx, ctx.Doer, repoOwner, repo_service.CreateRepoOptions{
+	createdRepo, err := repo_service.CreateRepositoryDirectly(ctx, ctx.Doer, repoOwner, repo_service.CreateRepoOptions{
 		Name:           opts.RepoName,
 		Description:    opts.Description,
 		OriginalURL:    form.CloneAddr,
@@ -187,35 +187,37 @@ func Migrate(ctx *context.APIContext) {
 		return
 	}
 
-	opts.MigrateToRepoID = repo.ID
+	opts.MigrateToRepoID = createdRepo.ID
 
-	defer func() {
+	doLongTimeMigrate := func(ctx gocontext.Context, doer *user_model.User) (migratedRepo *repo_model.Repository, retErr error) {
-		if e := recover(); e != nil {
+		defer func() {
-			var buf bytes.Buffer
+			if e := recover(); e != nil {
-			fmt.Fprintf(&buf, "Handler crashed with error: %v", log.Stack(2))
+				log.Error("MigrateRepository panic: %v\n%s", e, log.Stack(2))
-
+				if errDelete := repo_service.DeleteRepositoryDirectly(ctx, createdRepo.ID); errDelete != nil {
-			err = errors.New(buf.String())
+					log.Error("Unable to delete repo after MigrateRepository panic: %v", errDelete)
-		}
+				}
-
+				retErr = errors.New("MigrateRepository panic") // no idea why it would happen, just legacy code
-		if err == nil {
-			notify_service.MigrateRepository(ctx, ctx.Doer, repoOwner, repo)
-			return
-		}
-
-		if repo != nil {
-			if errDelete := repo_service.DeleteRepositoryDirectly(ctx, repo.ID); errDelete != nil {
-				log.Error("DeleteRepository: %v", errDelete)
 			}
-		}
+		}()
-	}()
 
-	if repo, err = migrations.MigrateRepository(graceful.GetManager().HammerContext(), ctx.Doer, repoOwner.Name, opts, nil); err != nil {
+		migratedRepo, err := migrations.MigrateRepository(ctx, doer, repoOwner.Name, opts, nil)
+		if err != nil {
+			return nil, err
+		}
+		notify_service.MigrateRepository(ctx, doer, repoOwner, migratedRepo)
+		return migratedRepo, nil
+	}
+
+	// use a background context, don't cancel the migration even if the client goes away
+	// HammerContext doesn't seem right (from https://github.com/go-gitea/gitea/pull/9335/files)
+	// There are other abuses, maybe most HammerContext abuses should be fixed together in the future.
+	migratedRepo, err := doLongTimeMigrate(graceful.GetManager().HammerContext(), ctx.Doer)
+	if err != nil {
 		handleMigrateError(ctx, repoOwner, err)
 		return
 	}
 
-	log.Trace("Repository migrated: %s/%s", repoOwner.Name, form.RepoName)
+	ctx.JSON(http.StatusCreated, convert.ToRepo(ctx, migratedRepo, access_model.Permission{AccessMode: perm.AccessModeAdmin}))
-	ctx.JSON(http.StatusCreated, convert.ToRepo(ctx, repo, access_model.Permission{AccessMode: perm.AccessModeAdmin}))
 }
 
 func handleMigrateError(ctx *context.APIContext, repoOwner *user_model.User, err error) {

routers/web/repo/pull.go

@@ -131,7 +131,7 @@ func getPullInfo(ctx *context.Context) (issue *issues_model.Issue, ok bool) {
 	ctx.Data["Issue"] = issue
 
 	if !issue.IsPull {
-		ctx.NotFound(nil)
+		ctx.Redirect(issue.Link())
 		return nil, false
 	}
 

services/actions/variables.go

@@ -5,10 +5,8 @@ package actions
 
 import (
 	"context"
-	"regexp"
 
 	actions_model "code.gitea.io/gitea/models/actions"
-	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/util"
 	secret_service "code.gitea.io/gitea/services/secrets"
 )
@@ -18,10 +16,6 @@ func CreateVariable(ctx context.Context, ownerID, repoID int64, name, data, desc
 		return nil, err
 	}
 
-	if err := envNameCIRegexMatch(name); err != nil {
-		return nil, err
-	}
-
 	v, err := actions_model.InsertVariable(ctx, ownerID, repoID, name, util.ReserveLineBreakForTextarea(data), description)
 	if err != nil {
 		return nil, err
@@ -35,10 +29,6 @@ func UpdateVariableNameData(ctx context.Context, variable *actions_model.ActionV
 		return false, err
 	}
 
-	if err := envNameCIRegexMatch(variable.Name); err != nil {
-		return false, err
-	}
-
 	variable.Data = util.ReserveLineBreakForTextarea(variable.Data)
 
 	return actions_model.UpdateVariableCols(ctx, variable, "name", "data", "description")
@@ -49,14 +39,6 @@ func DeleteVariableByID(ctx context.Context, variableID int64) error {
 }
 
 func DeleteVariableByName(ctx context.Context, ownerID, repoID int64, name string) error {
-	if err := secret_service.ValidateName(name); err != nil {
-		return err
-	}
-
-	if err := envNameCIRegexMatch(name); err != nil {
-		return err
-	}
-
 	v, err := GetVariable(ctx, actions_model.FindVariablesOpts{
 		OwnerID: ownerID,
 		RepoID:  repoID,
@@ -79,19 +61,3 @@ func GetVariable(ctx context.Context, opts actions_model.FindVariablesOpts) (*ac
 	}
 	return vars[0], nil
 }
-
-// some regular expression of `variables` and `secrets`
-// reference to:
-// https://docs.github.com/en/actions/learn-github-actions/variables#naming-conventions-for-configuration-variables
-// https://docs.github.com/en/actions/security-guides/encrypted-secrets#naming-your-secrets
-var (
-	forbiddenEnvNameCIRx = regexp.MustCompile("(?i)^CI")
-)
-
-func envNameCIRegexMatch(name string) error {
-	if forbiddenEnvNameCIRx.MatchString(name) {
-		log.Error("Env Name cannot be ci")
-		return util.NewInvalidArgumentErrorf("env name cannot be ci")
-	}
-	return nil
-}

services/secrets/secrets.go

@@ -56,10 +56,6 @@ func DeleteSecretByID(ctx context.Context, ownerID, repoID, secretID int64) erro
 }
 
 func DeleteSecretByName(ctx context.Context, ownerID, repoID int64, name string) error {
-	if err := ValidateName(name); err != nil {
-		return err
-	}
-
 	s, err := db.Find[secret_model.Secret](ctx, secret_model.FindSecretsOptions{
 		OwnerID: ownerID,
 		RepoID:  repoID,

services/secrets/validation.go

@@ -5,21 +5,29 @@ package secrets
 
 import (
 	"regexp"
+	"strings"
+	"sync"
 
 	"code.gitea.io/gitea/modules/util"
 )
 
+// https://docs.github.com/en/actions/learn-github-actions/variables#naming-conventions-for-configuration-variables
 // https://docs.github.com/en/actions/security-guides/encrypted-secrets#naming-your-secrets
-var (
+var globalVars = sync.OnceValue(func() (ret struct {
-	namePattern            = regexp.MustCompile("(?i)^[A-Z_][A-Z0-9_]*$")
+	namePattern, forbiddenPrefixPattern *regexp.Regexp
-	forbiddenPrefixPattern = regexp.MustCompile("(?i)^GIT(EA|HUB)_")
+},
-
+) {
-	ErrInvalidName = util.NewInvalidArgumentErrorf("invalid secret name")
+	ret.namePattern = regexp.MustCompile("(?i)^[A-Z_][A-Z0-9_]*$")
-)
+	ret.forbiddenPrefixPattern = regexp.MustCompile("(?i)^GIT(EA|HUB)_")
+	return ret
+})
 
 func ValidateName(name string) error {
-	if !namePattern.MatchString(name) || forbiddenPrefixPattern.MatchString(name) {
+	vars := globalVars()
-		return ErrInvalidName
+	if !vars.namePattern.MatchString(name) ||
+		vars.forbiddenPrefixPattern.MatchString(name) ||
+		strings.EqualFold(name, "CI") /* CI is always set to true in GitHub Actions*/ {
+		return util.NewInvalidArgumentErrorf("invalid variable or secret name")
 	}
 	return nil
 }

services/secrets/validation_test.go

@@ -0,0 +1,29 @@
+// Copyright 2025 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package secrets
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestValidateName(t *testing.T) {
+	cases := []struct {
+		name  string
+		valid bool
+	}{
+		{"FOO", true},
+		{"FOO1_BAR2", true},
+		{"_FOO", true}, // really? why support this
+		{"1FOO", false},
+		{"giteA_xx", false},
+		{"githuB_xx", false},
+		{"cI", false},
+	}
+	for _, c := range cases {
+		err := ValidateName(c.name)
+		assert.Equal(t, c.valid, err == nil, "ValidateName(%q)", c.name)
+	}
+}

templates/mail/repo/release.tmpl

@@ -32,10 +32,10 @@
 		<ul>
 			{{if not .DisableDownloadSourceArchives}}
 			<li>
-				<a href="{{.Release.Repo.Link}}/archive/{{.Release.TagName | PathEscapeSegments}}.zip" rel="nofollow"><strong>{{.locale.Tr "mail.release.download.zip"}}</strong></a>
+				<a href="{{.Release.Repo.HTMLURL}}/archive/{{.Release.TagName | PathEscapeSegments}}.zip" rel="nofollow"><strong>{{.locale.Tr "mail.release.download.zip"}}</strong></a>
 			</li>
 			<li>
-				<a href="{{.Release.Repo.Link}}/archive/{{.Release.TagName | PathEscapeSegments}}.tar.gz" rel="nofollow"><strong>{{.locale.Tr "mail.release.download.targz"}}</strong></a>
+				<a href="{{.Release.Repo.HTMLURL}}/archive/{{.Release.TagName | PathEscapeSegments}}.tar.gz" rel="nofollow"><strong>{{.locale.Tr "mail.release.download.targz"}}</strong></a>
 			</li>
 			{{end}}
 			{{if .Release.Attachments}}

tests/integration/api_repo_secrets_test.go

@@ -131,9 +131,5 @@ func TestAPIRepoSecrets(t *testing.T) {
 		req = NewRequest(t, "DELETE", url).
 			AddTokenAuth(token)
 		MakeRequest(t, req, http.StatusNotFound)
-
-		req = NewRequest(t, "DELETE", fmt.Sprintf("/api/v1/repos/%s/actions/secrets/000", repo.FullName())).
-			AddTokenAuth(token)
-		MakeRequest(t, req, http.StatusBadRequest)
 	})
 }

tests/integration/api_user_secrets_test.go

@@ -92,9 +92,5 @@ func TestAPIUserSecrets(t *testing.T) {
 		req = NewRequest(t, "DELETE", url).
 			AddTokenAuth(token)
 		MakeRequest(t, req, http.StatusNotFound)
-
-		req = NewRequest(t, "DELETE", "/api/v1/user/actions/secrets/000").
-			AddTokenAuth(token)
-		MakeRequest(t, req, http.StatusBadRequest)
 	})
 }

tests/integration/issue_test.go

@@ -497,9 +497,13 @@ func TestIssueRedirect(t *testing.T) {
 	resp = session.MakeRequest(t, req, http.StatusSeeOther)
 	assert.Equal(t, "/user2/repo1/pulls/2", test.RedirectURL(resp))
 
-	repoUnit := unittest.AssertExistsAndLoadBean(t, &repo_model.RepoUnit{RepoID: 1, Type: unit.TypeIssues})
+	// by the way, test PR redirection
+	req = NewRequest(t, "GET", "/user2/repo1/pulls/1")
+	resp = session.MakeRequest(t, req, http.StatusSeeOther)
+	assert.Equal(t, "/user2/repo1/issues/1", test.RedirectURL(resp))
 
-	// disable issue unit, it will be reset
+	// disable issue unit
+	repoUnit := unittest.AssertExistsAndLoadBean(t, &repo_model.RepoUnit{RepoID: 1, Type: unit.TypeIssues})
 	_, err := db.DeleteByID[repo_model.RepoUnit](t.Context(), repoUnit.ID)
 	assert.NoError(t, err)