Refactor sha1 and time-limited code (#31023)

Remove "EncodeSha1", it shouldn't be used as a general purpose hasher
(just like we have removed "EncodeMD5" in #28622)

Rewrite the "time-limited code" related code and write better tests, the
old code doesn't seem quite right.

(cherry picked from commit fb1ad920b769799aa1287441289d15477d9878c5)

Conflicts:
	modules/git/utils_test.go
	trivial context conflict because sha256 testing in Forgejo has diverged

models/user/email_address.go

@@ -10,6 +10,7 @@ import (
 	"net/mail"
 	"regexp"
 	"strings"
+	"time"
 
 	"code.gitea.io/gitea/models/db"
 	"code.gitea.io/gitea/modules/base"
@@ -362,14 +363,12 @@ func MakeEmailPrimary(ctx context.Context, email *EmailAddress) error {
 
 // VerifyActiveEmailCode verifies active email code when active account
 func VerifyActiveEmailCode(ctx context.Context, code, email string) *EmailAddress {
-	minutes := setting.Service.ActiveCodeLives
-
 	if user := GetVerifyUser(ctx, code); user != nil {
 		// time limit code
 		prefix := code[:base.TimeLimitCodeLength]
 		data := fmt.Sprintf("%d%s%s%s%s", user.ID, email, user.LowerName, user.Passwd, user.Rands)
 
-		if base.VerifyTimeLimitCode(data, minutes, prefix) {
+		if base.VerifyTimeLimitCode(time.Now(), data, setting.Service.ActiveCodeLives, prefix) {
 			emailAddress := &EmailAddress{UID: user.ID, Email: email}
 			if has, _ := db.GetEngine(ctx).Get(emailAddress); has {
 				return emailAddress

models/user/user.go

@@ -321,7 +321,7 @@ func (u *User) OrganisationLink() string {
 func (u *User) GenerateEmailActivateCode(email string) string {
 	code := base.CreateTimeLimitCode(
 		fmt.Sprintf("%d%s%s%s%s", u.ID, email, u.LowerName, u.Passwd, u.Rands),
-		setting.Service.ActiveCodeLives, nil)
+		setting.Service.ActiveCodeLives, time.Now(), nil)
 
 	// Add tail hex username
 	code += hex.EncodeToString([]byte(u.LowerName))
@@ -818,14 +818,11 @@ func GetVerifyUser(ctx context.Context, code string) (user *User) {
 
 // VerifyUserActiveCode verifies active code when active account
 func VerifyUserActiveCode(ctx context.Context, code string) (user *User) {
-	minutes := setting.Service.ActiveCodeLives
-
 	if user = GetVerifyUser(ctx, code); user != nil {
 		// time limit code
 		prefix := code[:base.TimeLimitCodeLength]
 		data := fmt.Sprintf("%d%s%s%s%s", user.ID, user.Email, user.LowerName, user.Passwd, user.Rands)
-
-		if base.VerifyTimeLimitCode(data, minutes, prefix) {
+		if base.VerifyTimeLimitCode(time.Now(), data, setting.Service.ActiveCodeLives, prefix) {
 			return user
 		}
 	}

modules/base/tool.go

@@ -4,12 +4,15 @@
 package base
 
 import (
+	"crypto/hmac"
 	"crypto/sha1"
 	"crypto/sha256"
+	"crypto/subtle"
 	"encoding/base64"
 	"encoding/hex"
 	"errors"
 	"fmt"
+	"hash"
 	"os"
 	"path/filepath"
 	"runtime"
@@ -25,13 +28,6 @@ import (
 	"github.com/dustin/go-humanize"
 )
 
-// EncodeSha1 string to sha1 hex value.
-func EncodeSha1(str string) string {
-	h := sha1.New()
-	_, _ = h.Write([]byte(str))
-	return hex.EncodeToString(h.Sum(nil))
-}
-
 // EncodeSha256 string to sha256 hex value.
 func EncodeSha256(str string) string {
 	h := sha256.New()
@@ -62,63 +58,62 @@ func BasicAuthDecode(encoded string) (string, string, error) {
 }
 
 // VerifyTimeLimitCode verify time limit code
-func VerifyTimeLimitCode(data string, minutes int, code string) bool {
+func VerifyTimeLimitCode(now time.Time, data string, minutes int, code string) bool {
 	if len(code) <= 18 {
 		return false
 	}
 
-	// split code
-	start := code[:12]
-	lives := code[12:18]
-	if d, err := strconv.ParseInt(lives, 10, 0); err == nil {
-		minutes = int(d)
-	}
-
-	// right active code
-	retCode := CreateTimeLimitCode(data, minutes, start)
-	if retCode == code && minutes > 0 {
-		// check time is expired or not
-		before, _ := time.ParseInLocation("200601021504", start, time.Local)
-		now := time.Now()
-		if before.Add(time.Minute*time.Duration(minutes)).Unix() > now.Unix() {
-			return true
-		}
-	}
+	startTimeStr := code[:12]
+	aliveTimeStr := code[12:18]
+	aliveTime, _ := strconv.Atoi(aliveTimeStr) // no need to check err, if anything wrong, the following code check will fail soon
 
+	// check code
+	retCode := CreateTimeLimitCode(data, aliveTime, startTimeStr, nil)
+	if subtle.ConstantTimeCompare([]byte(retCode), []byte(code)) != 1 {
+		retCode = CreateTimeLimitCode(data, aliveTime, startTimeStr, sha1.New()) // TODO: this is only for the support of legacy codes, remove this in/after 1.23
+		if subtle.ConstantTimeCompare([]byte(retCode), []byte(code)) != 1 {
 			return false
+		}
+	}
+
+	// check time is expired or not: startTime <= now && now < startTime + minutes
+	startTime, _ := time.ParseInLocation("200601021504", startTimeStr, time.Local)
+	return (startTime.Before(now) || startTime.Equal(now)) && now.Before(startTime.Add(time.Minute*time.Duration(minutes)))
 }
 
 // TimeLimitCodeLength default value for time limit code
 const TimeLimitCodeLength = 12 + 6 + 40
 
-// CreateTimeLimitCode create a time limit code
-// code format: 12 length date time string + 6 minutes string + 40 sha1 encoded string
-func CreateTimeLimitCode(data string, minutes int, startInf any) string {
-	format := "200601021504"
+// CreateTimeLimitCode create a time-limited code.
+// Format: 12 length date time string + 6 minutes string (not used) + 40 hash string, some other code depends on this fixed length
+// If h is nil, then use the default hmac hash.
+func CreateTimeLimitCode[T time.Time | string](data string, minutes int, startTimeGeneric T, h hash.Hash) string {
+	const format = "200601021504"
 
-	var start, end time.Time
-	var startStr, endStr string
-
-	if startInf == nil {
-		// Use now time create code
-		start = time.Now()
-		startStr = start.Format(format)
+	var start time.Time
+	var startTimeAny any = startTimeGeneric
+	if t, ok := startTimeAny.(time.Time); ok {
+		start = t
 	} else {
-		// use start string create code
-		startStr = startInf.(string)
-		start, _ = time.ParseInLocation(format, startStr, time.Local)
-		startStr = start.Format(format)
+		var err error
+		start, err = time.ParseInLocation(format, startTimeAny.(string), time.Local)
+		if err != nil {
+			return "" // return an invalid code because the "parse" failed
 		}
+	}
+	startStr := start.Format(format)
+	end := start.Add(time.Minute * time.Duration(minutes))
 
-	end = start.Add(time.Minute * time.Duration(minutes))
-	endStr = end.Format(format)
-
-	// create sha1 encode string
-	sh := sha1.New()
-	_, _ = sh.Write([]byte(fmt.Sprintf("%s%s%s%s%d", data, hex.EncodeToString(setting.GetGeneralTokenSigningSecret()), startStr, endStr, minutes)))
-	encoded := hex.EncodeToString(sh.Sum(nil))
+	if h == nil {
+		h = hmac.New(sha1.New, setting.GetGeneralTokenSigningSecret())
+	}
+	_, _ = fmt.Fprintf(h, "%s%s%s%s%d", data, hex.EncodeToString(setting.GetGeneralTokenSigningSecret()), startStr, end.Format(format), minutes)
+	encoded := hex.EncodeToString(h.Sum(nil))
 
 	code := fmt.Sprintf("%s%06d%s", startStr, minutes, encoded)
+	if len(code) != TimeLimitCodeLength {
+		panic("there is a hard requirement for the length of time-limited code") // it shouldn't happen
+	}
 	return code
 }
 

modules/base/tool_test.go

@@ -4,20 +4,18 @@
 package base
 
 import (
+	"crypto/sha1"
+	"fmt"
 	"os"
 	"testing"
 	"time"
 
+	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/test"
+
 	"github.com/stretchr/testify/assert"
 )
 
-func TestEncodeSha1(t *testing.T) {
-	assert.Equal(t,
-		"8843d7f92416211de9ebb963ff4ce28125932878",
-		EncodeSha1("foobar"),
-	)
-}
-
 func TestEncodeSha256(t *testing.T) {
 	assert.Equal(t,
 		"c3ab8ff13720e8ad9047dd39466b3c8974e592c2fa383d4a3960714caef0c4f2",
@@ -46,43 +44,54 @@ func TestBasicAuthDecode(t *testing.T) {
 }
 
 func TestVerifyTimeLimitCode(t *testing.T) {
-	tc := []struct {
-		data    string
-		minutes int
-		code    string
-		valid   bool
-	}{{
-		data:    "data",
-		minutes: 2,
-		code:    testCreateTimeLimitCode(t, "data", 2),
-		valid:   true,
-	}, {
-		data:    "abc123-ß",
-		minutes: 1,
-		code:    testCreateTimeLimitCode(t, "abc123-ß", 1),
-		valid:   true,
-	}, {
-		data:    "data",
-		minutes: 2,
-		code:    "2021012723240000005928251dac409d2c33a6eb82c63410aaad569bed",
-		valid:   false,
-	}}
-	for _, test := range tc {
-		actualValid := VerifyTimeLimitCode(test.data, test.minutes, test.code)
-		assert.Equal(t, test.valid, actualValid, "data: '%s' code: '%s' should be valid: %t", test.data, test.code, test.valid)
+	defer test.MockVariableValue(&setting.InstallLock, true)()
+	initGeneralSecret := func(secret string) {
+		setting.InstallLock = true
+		setting.CfgProvider, _ = setting.NewConfigProviderFromData(fmt.Sprintf(`
+[oauth2]
+JWT_SECRET = %s
+`, secret))
+		setting.LoadCommonSettings()
 	}
-}
 
-func testCreateTimeLimitCode(t *testing.T, data string, m int) string {
-	result0 := CreateTimeLimitCode(data, m, nil)
-	result1 := CreateTimeLimitCode(data, m, time.Now().Format("200601021504"))
-	result2 := CreateTimeLimitCode(data, m, time.Unix(time.Now().Unix()+int64(time.Minute)*int64(m), 0).Format("200601021504"))
+	initGeneralSecret("KZb_QLUd4fYVyxetjxC4eZkrBgWM2SndOOWDNtgUUko")
+	now := time.Now()
 
-	assert.Equal(t, result0, result1)
-	assert.NotEqual(t, result0, result2)
+	t.Run("TestGenericParameter", func(t *testing.T) {
+		time2000 := time.Date(2000, 1, 2, 3, 4, 5, 0, time.Local)
+		assert.Equal(t, "2000010203040000026fa5221b2731b7cf80b1b506f5e39e38c115fee5", CreateTimeLimitCode("test-sha1", 2, time2000, sha1.New()))
+		assert.Equal(t, "2000010203040000026fa5221b2731b7cf80b1b506f5e39e38c115fee5", CreateTimeLimitCode("test-sha1", 2, "200001020304", sha1.New()))
+		assert.Equal(t, "2000010203040000024842227a2f87041ff82025199c0187410a9297bf", CreateTimeLimitCode("test-hmac", 2, time2000, nil))
+		assert.Equal(t, "2000010203040000024842227a2f87041ff82025199c0187410a9297bf", CreateTimeLimitCode("test-hmac", 2, "200001020304", nil))
+	})
 
-	assert.True(t, len(result0) != 0)
-	return result0
+	t.Run("TestInvalidCode", func(t *testing.T) {
+		assert.False(t, VerifyTimeLimitCode(now, "data", 2, ""))
+		assert.False(t, VerifyTimeLimitCode(now, "data", 2, "invalid code"))
+	})
+
+	t.Run("TestCreateAndVerify", func(t *testing.T) {
+		code := CreateTimeLimitCode("data", 2, now, nil)
+		assert.False(t, VerifyTimeLimitCode(now.Add(-time.Minute), "data", 2, code)) // not started yet
+		assert.True(t, VerifyTimeLimitCode(now, "data", 2, code))
+		assert.True(t, VerifyTimeLimitCode(now.Add(time.Minute), "data", 2, code))
+		assert.False(t, VerifyTimeLimitCode(now.Add(time.Minute), "DATA", 2, code))   // invalid data
+		assert.False(t, VerifyTimeLimitCode(now.Add(2*time.Minute), "data", 2, code)) // expired
+	})
+
+	t.Run("TestDifferentSecret", func(t *testing.T) {
+		// use another secret to ensure the code is invalid for different secret
+		verifyDataCode := func(c string) bool {
+			return VerifyTimeLimitCode(now, "data", 2, c)
+		}
+		code1 := CreateTimeLimitCode("data", 2, now, sha1.New())
+		code2 := CreateTimeLimitCode("data", 2, now, nil)
+		assert.True(t, verifyDataCode(code1))
+		assert.True(t, verifyDataCode(code2))
+		initGeneralSecret("000_QLUd4fYVyxetjxC4eZkrBgWM2SndOOWDNtgUUko")
+		assert.False(t, verifyDataCode(code1))
+		assert.False(t, verifyDataCode(code2))
+	})
 }
 
 func TestFileSize(t *testing.T) {

modules/git/utils.go

@@ -4,6 +4,8 @@
 package git
 
 import (
+	"crypto/sha1"
+	"encoding/hex"
 	"fmt"
 	"io"
 	"os"
@@ -128,3 +130,9 @@ func (l *LimitedReaderCloser) Read(p []byte) (n int, err error) {
 func (l *LimitedReaderCloser) Close() error {
 	return l.C.Close()
 }
+
+func HashFilePathForWebUI(s string) string {
+	h := sha1.New()
+	_, _ = h.Write([]byte(s))
+	return hex.EncodeToString(h.Sum(nil))
+}

modules/git/utils_test.go

@@ -3,7 +3,11 @@
 
 package git
 
-import "testing"
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
 
 // This file contains utility functions that are used across multiple tests,
 // but not in production code.
@@ -13,3 +17,10 @@ func skipIfSHA256NotSupported(t *testing.T) {
 		t.Skip("skipping because installed Git version doesn't support SHA256")
 	}
 }
+
+func TestHashFilePathForWebUI(t *testing.T) {
+	assert.Equal(t,
+		"8843d7f92416211de9ebb963ff4ce28125932878",
+		HashFilePathForWebUI("foobar"),
+	)
+}

routers/web/repo/compare.go

@@ -931,7 +931,7 @@ func ExcerptBlob(ctx *context.Context) {
 		}
 	}
 	ctx.Data["section"] = section
-	ctx.Data["FileNameHash"] = base.EncodeSha1(filePath)
+	ctx.Data["FileNameHash"] = git.HashFilePathForWebUI(filePath)
 	ctx.Data["AfterCommitID"] = commitID
 	ctx.Data["Anchor"] = anchor
 	ctx.HTML(http.StatusOK, tplBlobExcerpt)

services/gitdiff/gitdiff.go

@@ -23,7 +23,6 @@ import (
 	pull_model "code.gitea.io/gitea/models/pull"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/analyze"
-	"code.gitea.io/gitea/modules/base"
 	"code.gitea.io/gitea/modules/charset"
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/highlight"
@@ -742,7 +741,7 @@ parsingLoop:
 	diffLineTypeBuffers[DiffLineAdd] = new(bytes.Buffer)
 	diffLineTypeBuffers[DiffLineDel] = new(bytes.Buffer)
 	for _, f := range diff.Files {
-		f.NameHash = base.EncodeSha1(f.Name)
+		f.NameHash = git.HashFilePathForWebUI(f.Name)
 
 		for _, buffer := range diffLineTypeBuffers {
 			buffer.Reset()