Skip to content

Commit 6ad0940

Browse files
wxiaoguangdelvh
authored and
GiteaBot
committed
Fix various bugs (#35684)
1. Fix incorrect column in `applySubscribedCondition`, add a test 2. Fix debian version parsing, add more tests fix #35695 3. Fix log level for HTTP errors, fix #35651 4. Fix abused "panic" handler in API `Migrate` 5. Fix the redirection from PR to issue, add a test 6. Fix Actions variable & secret name validation, add more tests * envNameCIRegexMatch is unnecessary, removed * validating in "delete" function doesn't make sense, removed 7. Fix incorrect link in release email --------- Signed-off-by: wxiaoguang <[email protected]> Co-authored-by: delvh <[email protected]>
1 parent f71df88 commit 6ad0940

File tree

15 files changed

+97
-85
lines changed

15 files changed

+97
-85
lines changed

‎models/issues/issue_search.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -476,7 +476,7 @@ func applySubscribedCondition(sess *xorm.Session, subscriberID int64) {
476476
),
477477
builder.Eq{"issue.poster_id": subscriberID},
478478
builder.In("issue.repo_id", builder.
479-
Select("id").
479+
Select("repo_id").
480480
From("watch").
481481
Where(builder.And(builder.Eq{"user_id": subscriberID},
482482
builder.In("mode", repo_model.WatchModeNormal, repo_model.WatchModeAuto))),

‎models/issues/issue_test.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -197,6 +197,12 @@ func TestIssues(t *testing.T) {
197197
},
198198
[]int64{2},
199199
},
200+
{
201+
issues_model.IssuesOptions{
202+
SubscriberID: 11,
203+
},
204+
[]int64{11, 5, 9, 8, 3, 2, 1},
205+
},
200206
} {
201207
issues, err := issues_model.Issues(t.Context(), &test.Opts)
202208
assert.NoError(t, err)

‎modules/packages/debian/metadata.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ var (
4646
// https://www.debian.org/doc/debian-policy/ch-controlfields.html#source
4747
namePattern = regexp.MustCompile(`\A[a-z0-9][a-z0-9+-.]+\z`)
4848
// https://www.debian.org/doc/debian-policy/ch-controlfields.html#version
49-
versionPattern = regexp.MustCompile(`\A(?:[0-9]:)?[a-zA-Z0-9.+~]+(?:-[a-zA-Z0-9.+-~]+)?\z`)
49+
versionPattern = regexp.MustCompile(`\A(?:(0|[1-9][0-9]*):)?[a-zA-Z0-9.+~]+(?:-[a-zA-Z0-9.+-~]+)?\z`)
5050
)
5151

5252
type Package struct {

‎modules/packages/debian/metadata_test.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -176,4 +176,12 @@ func TestParseControlFile(t *testing.T) {
176176
assert.Equal(t, []string{"a", "b"}, p.Metadata.Dependencies)
177177
assert.Equal(t, full, p.Control)
178178
})
179+
180+
t.Run("ValidVersions", func(t *testing.T) {
181+
for _, version := range []string{"1.0", "0:1.2", "9:1.0", "10:1.0", "900:1a.2b-x-y_z~1+2"} {
182+
p, err := ParseControlFile(buildContent("testpkg", version, "amd64"))
183+
assert.NoError(t, err, "ParseControlFile with version %q", version)
184+
assert.NotNil(t, p)
185+
}
186+
})
179187
}

‎modules/web/routing/logger.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,8 @@ func logPrinter(logger log.Logger) func(trigger Event, record *requestRecord) {
104104
}
105105
logf := logInfo
106106
// lower the log level for some specific requests, in most cases these logs are not useful
107-
if strings.HasPrefix(req.RequestURI, "/assets/") /* static assets */ ||
107+
if status > 0 && status < 400 &&
108+
strings.HasPrefix(req.RequestURI, "/assets/") /* static assets */ ||
108109
req.RequestURI == "/user/events" /* Server-Sent Events (SSE) handler */ ||
109110
req.RequestURI == "/api/actions/runner.v1.RunnerService/FetchTask" /* Actions Runner polling */ {
110111
logf = logTrace

‎routers/api/v1/repo/migrate.go‎

Lines changed: 25 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
package repo
55

66
import (
7-
"bytes"
7+
gocontext "context"
88
"errors"
99
"fmt"
1010
"net/http"
@@ -173,7 +173,7 @@ func Migrate(ctx *context.APIContext) {
173173
opts.AWSSecretAccessKey = form.AWSSecretAccessKey
174174
}
175175

176-
repo, err := repo_service.CreateRepositoryDirectly(ctx, ctx.Doer, repoOwner, repo_service.CreateRepoOptions{
176+
createdRepo, err := repo_service.CreateRepositoryDirectly(ctx, ctx.Doer, repoOwner, repo_service.CreateRepoOptions{
177177
Name: opts.RepoName,
178178
Description: opts.Description,
179179
OriginalURL: form.CloneAddr,
@@ -187,35 +187,37 @@ func Migrate(ctx *context.APIContext) {
187187
return
188188
}
189189

190-
opts.MigrateToRepoID = repo.ID
190+
opts.MigrateToRepoID = createdRepo.ID
191191

192-
defer func() {
193-
if e := recover(); e != nil {
194-
var buf bytes.Buffer
195-
fmt.Fprintf(&buf, "Handler crashed with error: %v", log.Stack(2))
196-
197-
err = errors.New(buf.String())
198-
}
199-
200-
if err == nil {
201-
notify_service.MigrateRepository(ctx, ctx.Doer, repoOwner, repo)
202-
return
203-
}
204-
205-
if repo != nil {
206-
if errDelete := repo_service.DeleteRepositoryDirectly(ctx, repo.ID); errDelete != nil {
207-
log.Error("DeleteRepository: %v", errDelete)
192+
doLongTimeMigrate := func(ctx gocontext.Context, doer *user_model.User) (migratedRepo *repo_model.Repository, retErr error) {
193+
defer func() {
194+
if e := recover(); e != nil {
195+
log.Error("MigrateRepository panic: %v\n%s", e, log.Stack(2))
196+
if errDelete := repo_service.DeleteRepositoryDirectly(ctx, createdRepo.ID); errDelete != nil {
197+
log.Error("Unable to delete repo after MigrateRepository panic: %v", errDelete)
198+
}
199+
retErr = errors.New("MigrateRepository panic") // no idea why it would happen, just legacy code
208200
}
201+
}()
202+
203+
migratedRepo, err := migrations.MigrateRepository(ctx, doer, repoOwner.Name, opts, nil)
204+
if err != nil {
205+
return nil, err
209206
}
210-
}()
207+
notify_service.MigrateRepository(ctx, doer, repoOwner, migratedRepo)
208+
return migratedRepo, nil
209+
}
211210

212-
if repo, err = migrations.MigrateRepository(graceful.GetManager().HammerContext(), ctx.Doer, repoOwner.Name, opts, nil); err != nil {
211+
// use a background context, don't cancel the migration even if the client goes away
212+
// HammerContext doesn't seem right (from https://github.com/go-gitea/gitea/pull/9335/files)
213+
// There are other abuses, maybe most HammerContext abuses should be fixed together in the future.
214+
migratedRepo, err := doLongTimeMigrate(graceful.GetManager().HammerContext(), ctx.Doer)
215+
if err != nil {
213216
handleMigrateError(ctx, repoOwner, err)
214217
return
215218
}
216219

217-
log.Trace("Repository migrated: %s/%s", repoOwner.Name, form.RepoName)
218-
ctx.JSON(http.StatusCreated, convert.ToRepo(ctx, repo, access_model.Permission{AccessMode: perm.AccessModeAdmin}))
220+
ctx.JSON(http.StatusCreated, convert.ToRepo(ctx, migratedRepo, access_model.Permission{AccessMode: perm.AccessModeAdmin}))
219221
}
220222

221223
func handleMigrateError(ctx *context.APIContext, repoOwner *user_model.User, err error) {

‎routers/web/repo/pull.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,7 @@ func getPullInfo(ctx *context.Context) (issue *issues_model.Issue, ok bool) {
131131
ctx.Data["Issue"] = issue
132132

133133
if !issue.IsPull {
134-
ctx.NotFound(nil)
134+
ctx.Redirect(issue.Link())
135135
return nil, false
136136
}
137137

‎services/actions/variables.go‎

Lines changed: 0 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,8 @@ package actions
55

66
import (
77
"context"
8-
"regexp"
98

109
actions_model "code.gitea.io/gitea/models/actions"
11-
"code.gitea.io/gitea/modules/log"
1210
"code.gitea.io/gitea/modules/util"
1311
secret_service "code.gitea.io/gitea/services/secrets"
1412
)
@@ -18,10 +16,6 @@ func CreateVariable(ctx context.Context, ownerID, repoID int64, name, data, desc
1816
return nil, err
1917
}
2018

21-
if err := envNameCIRegexMatch(name); err != nil {
22-
return nil, err
23-
}
24-
2519
v, err := actions_model.InsertVariable(ctx, ownerID, repoID, name, util.ReserveLineBreakForTextarea(data), description)
2620
if err != nil {
2721
return nil, err
@@ -35,10 +29,6 @@ func UpdateVariableNameData(ctx context.Context, variable *actions_model.ActionV
3529
return false, err
3630
}
3731

38-
if err := envNameCIRegexMatch(variable.Name); err != nil {
39-
return false, err
40-
}
41-
4232
variable.Data = util.ReserveLineBreakForTextarea(variable.Data)
4333

4434
return actions_model.UpdateVariableCols(ctx, variable, "name", "data", "description")
@@ -49,14 +39,6 @@ func DeleteVariableByID(ctx context.Context, variableID int64) error {
4939
}
5040

5141
func DeleteVariableByName(ctx context.Context, ownerID, repoID int64, name string) error {
52-
if err := secret_service.ValidateName(name); err != nil {
53-
return err
54-
}
55-
56-
if err := envNameCIRegexMatch(name); err != nil {
57-
return err
58-
}
59-
6042
v, err := GetVariable(ctx, actions_model.FindVariablesOpts{
6143
OwnerID: ownerID,
6244
RepoID: repoID,
@@ -79,19 +61,3 @@ func GetVariable(ctx context.Context, opts actions_model.FindVariablesOpts) (*ac
7961
}
8062
return vars[0], nil
8163
}
82-
83-
// some regular expression of `variables` and `secrets`
84-
// reference to:
85-
// https://docs.github.com/en/actions/learn-github-actions/variables#naming-conventions-for-configuration-variables
86-
// https://docs.github.com/en/actions/security-guides/encrypted-secrets#naming-your-secrets
87-
var (
88-
forbiddenEnvNameCIRx = regexp.MustCompile("(?i)^CI")
89-
)
90-
91-
func envNameCIRegexMatch(name string) error {
92-
if forbiddenEnvNameCIRx.MatchString(name) {
93-
log.Error("Env Name cannot be ci")
94-
return util.NewInvalidArgumentErrorf("env name cannot be ci")
95-
}
96-
return nil
97-
}

‎services/secrets/secrets.go‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -56,10 +56,6 @@ func DeleteSecretByID(ctx context.Context, ownerID, repoID, secretID int64) erro
5656
}
5757

5858
func DeleteSecretByName(ctx context.Context, ownerID, repoID int64, name string) error {
59-
if err := ValidateName(name); err != nil {
60-
return err
61-
}
62-
6359
s, err := db.Find[secret_model.Secret](ctx, secret_model.FindSecretsOptions{
6460
OwnerID: ownerID,
6561
RepoID: repoID,

‎services/secrets/validation.go‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,21 +5,29 @@ package secrets
55

66
import (
77
"regexp"
8+
"strings"
9+
"sync"
810

911
"code.gitea.io/gitea/modules/util"
1012
)
1113

14+
// https://docs.github.com/en/actions/learn-github-actions/variables#naming-conventions-for-configuration-variables
1215
// https://docs.github.com/en/actions/security-guides/encrypted-secrets#naming-your-secrets
13-
var (
14-
namePattern = regexp.MustCompile("(?i)^[A-Z_][A-Z0-9_]*$")
15-
forbiddenPrefixPattern = regexp.MustCompile("(?i)^GIT(EA|HUB)_")
16-
17-
ErrInvalidName = util.NewInvalidArgumentErrorf("invalid secret name")
18-
)
16+
var globalVars = sync.OnceValue(func() (ret struct {
17+
namePattern, forbiddenPrefixPattern *regexp.Regexp
18+
},
19+
) {
20+
ret.namePattern = regexp.MustCompile("(?i)^[A-Z_][A-Z0-9_]*$")
21+
ret.forbiddenPrefixPattern = regexp.MustCompile("(?i)^GIT(EA|HUB)_")
22+
return ret
23+
})
1924

2025
func ValidateName(name string) error {
21-
if !namePattern.MatchString(name) || forbiddenPrefixPattern.MatchString(name) {
22-
return ErrInvalidName
26+
vars := globalVars()
27+
if !vars.namePattern.MatchString(name) ||
28+
vars.forbiddenPrefixPattern.MatchString(name) ||
29+
strings.EqualFold(name, "CI") /* CI is always set to true in GitHub Actions*/ {
30+
return util.NewInvalidArgumentErrorf("invalid variable or secret name")
2331
}
2432
return nil
2533
}

0 commit comments

Comments
 (0)