Skip to content

Commit 71360a9

Browse files
authored
Address some CodeQL security concerns (#35572)
Although there is no real security problem
1 parent c453210 commit 71360a9

File tree

35 files changed

+118
-78
lines changed

35 files changed

+118
-78
lines changed

cmd/admin_user_create.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,7 @@ func runCreateUser(ctx context.Context, c *cli.Command) error {
151151
if err != nil {
152152
return err
153153
}
154+
// codeql[disable-next-line=go/clear-text-logging]
154155
fmt.Printf("generated random password is '%s'\n", password)
155156
} else if userType == user_model.UserTypeIndividual {
156157
return errors.New("must set either password or random-password flag")

cmd/admin_user_must_change_password.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ func runMustChangePassword(ctx context.Context, c *cli.Command) error {
5858
return err
5959
}
6060

61+
// codeql[disable-next-line=go/clear-text-logging]
6162
fmt.Printf("Updated %d users setting MustChangePassword to %t\n", n, mustChangePassword)
6263
return nil
6364
}

cmd/generate.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,7 @@ func runGenerateSecretKey(_ context.Context, c *cli.Command) error {
9191
return err
9292
}
9393

94+
// codeql[disable-next-line=go/clear-text-logging]
9495
fmt.Printf("%s", secretKey)
9596

9697
if isatty.IsTerminal(os.Stdout.Fd()) {

cmd/hook.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,7 @@ Gitea or set your environment appropriately.`, "")
186186
userID, _ := strconv.ParseInt(os.Getenv(repo_module.EnvPusherID), 10, 64)
187187
prID, _ := strconv.ParseInt(os.Getenv(repo_module.EnvPRID), 10, 64)
188188
deployKeyID, _ := strconv.ParseInt(os.Getenv(repo_module.EnvDeployKeyID), 10, 64)
189-
actionPerm, _ := strconv.ParseInt(os.Getenv(repo_module.EnvActionPerm), 10, 64)
189+
actionPerm, _ := strconv.Atoi(os.Getenv(repo_module.EnvActionPerm))
190190

191191
hookOptions := private.HookOptions{
192192
UserID: userID,
@@ -196,7 +196,7 @@ Gitea or set your environment appropriately.`, "")
196196
GitPushOptions: pushOptions(),
197197
PullRequestID: prID,
198198
DeployKeyID: deployKeyID,
199-
ActionPerm: int(actionPerm),
199+
ActionPerm: actionPerm,
200200
}
201201

202202
scanner := bufio.NewScanner(os.Stdin)

models/repo/repo.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -605,7 +605,7 @@ func (repo *Repository) IsGenerated() bool {
605605

606606
// RepoPath returns repository path by given user and repository name.
607607
func RepoPath(userName, repoName string) string { //revive:disable-line:exported
608-
return filepath.Join(user_model.UserPath(userName), strings.ToLower(repoName)+".git")
608+
return filepath.Join(setting.RepoRootPath, filepath.Clean(strings.ToLower(userName)), filepath.Clean(strings.ToLower(repoName)+".git"))
609609
}
610610

611611
// RepoPath returns the repository path

models/user/user.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -980,7 +980,7 @@ func GetInactiveUsers(ctx context.Context, olderThan time.Duration) ([]*User, er
980980

981981
// UserPath returns the path absolute path of user repositories.
982982
func UserPath(userName string) string { //revive:disable-line:exported
983-
return filepath.Join(setting.RepoRootPath, strings.ToLower(userName))
983+
return filepath.Join(setting.RepoRootPath, filepath.Clean(strings.ToLower(userName)))
984984
}
985985

986986
// GetUserByID returns the user object by given ID if exists.

modules/auth/password/hash/argon2.go

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -61,17 +61,11 @@ func NewArgon2Hasher(config string) *Argon2Hasher {
6161
return nil
6262
}
6363

64-
parsed, err := parseUIntParam(vals[0], "time", "argon2", config, nil)
65-
hasher.time = uint32(parsed)
66-
67-
parsed, err = parseUIntParam(vals[1], "memory", "argon2", config, err)
68-
hasher.memory = uint32(parsed)
69-
70-
parsed, err = parseUIntParam(vals[2], "threads", "argon2", config, err)
71-
hasher.threads = uint8(parsed)
72-
73-
parsed, err = parseUIntParam(vals[3], "keyLen", "argon2", config, err)
74-
hasher.keyLen = uint32(parsed)
64+
var err error
65+
hasher.time, err = parseUintParam[uint32](vals[0], "time", "argon2", config, nil)
66+
hasher.memory, err = parseUintParam[uint32](vals[1], "memory", "argon2", config, err)
67+
hasher.threads, err = parseUintParam[uint8](vals[2], "threads", "argon2", config, err)
68+
hasher.keyLen, err = parseUintParam[uint32](vals[3], "keyLen", "argon2", config, err)
7569
if err != nil {
7670
return nil
7771
}

modules/auth/password/hash/common.go

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"strconv"
88

99
"code.gitea.io/gitea/modules/log"
10+
"code.gitea.io/gitea/modules/util"
1011
)
1112

1213
func parseIntParam(value, param, algorithmName, config string, previousErr error) (int, error) {
@@ -18,11 +19,12 @@ func parseIntParam(value, param, algorithmName, config string, previousErr error
1819
return parsed, previousErr // <- Keep the previous error as this function should still return an error once everything has been checked if any call failed
1920
}
2021

21-
func parseUIntParam(value, param, algorithmName, config string, previousErr error) (uint64, error) { //nolint:unparam // algorithmName is always argon2
22-
parsed, err := strconv.ParseUint(value, 10, 64)
22+
func parseUintParam[T uint32 | uint8](value, param, algorithmName, config string, previousErr error) (ret T, _ error) {
23+
_, isUint32 := any(ret).(uint32)
24+
parsed, err := strconv.ParseUint(value, 10, util.Iif(isUint32, 32, 8))
2325
if err != nil {
2426
log.Error("invalid integer for %s representation in %s hash spec %s", param, algorithmName, config)
2527
return 0, err
2628
}
27-
return parsed, previousErr // <- Keep the previous error as this function should still return an error once everything has been checked if any call failed
29+
return T(parsed), previousErr // <- Keep the previous error as this function should still return an error once everything has been checked if any call failed
2830
}

modules/auth/password/pwn/pwn.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ func newRequest(ctx context.Context, method, url string, body io.ReadCloser) (*h
7272
// Adding padding will make requests more secure, however is also slower
7373
// because artificial responses will be added to the response
7474
// For more information, see https://www.troyhunt.com/enhancing-pwned-passwords-privacy-with-padding/
75-
func (c *Client) CheckPassword(pw string, padding bool) (int, error) {
75+
func (c *Client) CheckPassword(pw string, padding bool) (int64, error) {
7676
if pw == "" {
7777
return -1, ErrEmptyPassword
7878
}
@@ -111,7 +111,7 @@ func (c *Client) CheckPassword(pw string, padding bool) (int, error) {
111111
if err != nil {
112112
return -1, err
113113
}
114-
return int(count), nil
114+
return count, nil
115115
}
116116
}
117117
return 0, nil

modules/auth/password/pwn/pwn_test.go

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -37,25 +37,25 @@ func TestPassword(t *testing.T) {
3737

3838
count, err := client.CheckPassword("", false)
3939
assert.ErrorIs(t, err, ErrEmptyPassword, "blank input should return ErrEmptyPassword")
40-
assert.Equal(t, -1, count)
40+
assert.EqualValues(t, -1, count)
4141

4242
count, err = client.CheckPassword("pwned", false)
4343
assert.NoError(t, err)
44-
assert.Equal(t, 1, count)
44+
assert.EqualValues(t, 1, count)
4545

4646
count, err = client.CheckPassword("notpwned", false)
4747
assert.NoError(t, err)
48-
assert.Equal(t, 0, count)
48+
assert.EqualValues(t, 0, count)
4949

5050
count, err = client.CheckPassword("paddedpwned", true)
5151
assert.NoError(t, err)
52-
assert.Equal(t, 1, count)
52+
assert.EqualValues(t, 1, count)
5353

5454
count, err = client.CheckPassword("paddednotpwned", true)
5555
assert.NoError(t, err)
56-
assert.Equal(t, 0, count)
56+
assert.EqualValues(t, 0, count)
5757

5858
count, err = client.CheckPassword("paddednotpwnedzero", true)
5959
assert.NoError(t, err)
60-
assert.Equal(t, 0, count)
60+
assert.EqualValues(t, 0, count)
6161
}

0 commit comments

Comments
 (0)