From 4af1d58c86afefac67b066ac0484f84ed899881d Mon Sep 17 00:00:00 2001 From: Giteabot Date: Sun, 19 Oct 2025 02:26:03 +0800 Subject: [PATCH] Fix various bugs (#35684) (#35696) Backport #35684 by wxiaoguang --- models/issues/issue_search.go | 2 +- models/issues/issue_test.go | 6 +++ modules/packages/debian/metadata.go | 2 +- modules/packages/debian/metadata_test.go | 8 ++++ modules/web/routing/logger.go | 3 +- routers/api/v1/repo/migrate.go | 50 +++++++++++----------- routers/web/repo/pull.go | 2 +- services/actions/variables.go | 34 --------------- services/secrets/secrets.go | 4 -- services/secrets/validation.go | 24 +++++++---- services/secrets/validation_test.go | 29 +++++++++++++ templates/mail/repo/release.tmpl | 4 +- tests/integration/api_repo_secrets_test.go | 4 -- tests/integration/api_user_secrets_test.go | 4 -- tests/integration/issue_test.go | 8 +++- 15 files changed, 98 insertions(+), 86 deletions(-) create mode 100644 services/secrets/validation_test.go diff --git a/models/issues/issue_search.go b/models/issues/issue_search.go index 466e788d6c..049dcc7de8 100644 --- a/models/issues/issue_search.go +++ b/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))), diff --git a/models/issues/issue_test.go b/models/issues/issue_test.go index 09fd492667..55a90f50a1 100644 --- a/models/issues/issue_test.go +++ b/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) diff --git a/modules/packages/debian/metadata.go b/modules/packages/debian/metadata.go index e76db63975..1cae46ecb2 100644 --- a/modules/packages/debian/metadata.go +++ b/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 { diff --git a/modules/packages/debian/metadata_test.go b/modules/packages/debian/metadata_test.go index a56b131416..fedd276614 100644 --- a/modules/packages/debian/metadata_test.go +++ b/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) + } + }) } diff --git a/modules/web/routing/logger.go b/modules/web/routing/logger.go index 3bca9b3420..a6a0e0d517 100644 --- a/modules/web/routing/logger.go +++ b/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 diff --git a/routers/api/v1/repo/migrate.go b/routers/api/v1/repo/migrate.go index c1e0b47d33..17259dc724 100644 --- a/routers/api/v1/repo/migrate.go +++ b/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() { - if e := recover(); e != nil { - var buf bytes.Buffer - fmt.Fprintf(&buf, "Handler crashed with error: %v", log.Stack(2)) - - err = errors.New(buf.String()) - } - - 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) + doLongTimeMigrate := func(ctx gocontext.Context, doer *user_model.User) (migratedRepo *repo_model.Repository, retErr error) { + defer func() { + if e := recover(); e != nil { + log.Error("MigrateRepository panic: %v\n%s", e, log.Stack(2)) + if errDelete := repo_service.DeleteRepositoryDirectly(ctx, createdRepo.ID); errDelete != nil { + 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 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, repo, access_model.Permission{AccessMode: perm.AccessModeAdmin})) + ctx.JSON(http.StatusCreated, convert.ToRepo(ctx, migratedRepo, access_model.Permission{AccessMode: perm.AccessModeAdmin})) } func handleMigrateError(ctx *context.APIContext, repoOwner *user_model.User, err error) { diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 72c3690b0c..2e65099085 100644 --- a/routers/web/repo/pull.go +++ b/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 } diff --git a/services/actions/variables.go b/services/actions/variables.go index 2603f1d461..57e6af1d9b 100644 --- a/services/actions/variables.go +++ b/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 -} diff --git a/services/secrets/secrets.go b/services/secrets/secrets.go index ec6a3cb062..1aec6e146d 100644 --- a/services/secrets/secrets.go +++ b/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, diff --git a/services/secrets/validation.go b/services/secrets/validation.go index 3db5b96452..d3636ded84 100644 --- a/services/secrets/validation.go +++ b/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 ( - namePattern = regexp.MustCompile("(?i)^[A-Z_][A-Z0-9_]*$") - forbiddenPrefixPattern = regexp.MustCompile("(?i)^GIT(EA|HUB)_") - - ErrInvalidName = util.NewInvalidArgumentErrorf("invalid secret name") -) +var globalVars = sync.OnceValue(func() (ret struct { + namePattern, forbiddenPrefixPattern *regexp.Regexp +}, +) { + 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) { - return ErrInvalidName + vars := globalVars() + 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 } diff --git a/services/secrets/validation_test.go b/services/secrets/validation_test.go new file mode 100644 index 0000000000..6ee47513dc --- /dev/null +++ b/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) + } +} diff --git a/templates/mail/repo/release.tmpl b/templates/mail/repo/release.tmpl index 3172e6a19d..b306ddf5b9 100644 --- a/templates/mail/repo/release.tmpl +++ b/templates/mail/repo/release.tmpl @@ -32,10 +32,10 @@