fix(lfs): validate OID before building filesystem object path (#39762) (#39766)

Backport #39762

Co-authored-by: Basavaraj S m <basavaraj@digiscrypt.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

modules/lfs/client_test.go

@@ -8,12 +8,16 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestNewClient(t *testing.T) {
 	u, _ := url.Parse("file:///test")
 	c := newClient(u, nil)
-	assert.IsType(t, &FilesystemClient{}, c)
+	require.IsType(t, &FilesystemClient{}, c)
+	localPath, err := c.(*FilesystemClient).objectPath("invalid") //nolint:forcetypeassert // type is expected
+	assert.Empty(t, localPath)
+	assert.ErrorIs(t, err, ErrInvalidOIDFormat)
 
 	u, _ = url.Parse("https://test.com/lfs")
 	c = newClient(u, nil)

modules/lfs/filesystem_client.go

@@ -18,7 +18,7 @@ type FilesystemClient struct {
 	lfsDir string
 }
 
-// BatchSize returns the preferred size of batchs to process
+// BatchSize returns the preferred size of batches to process
 func (c *FilesystemClient) BatchSize() int {
 	return 1
 }
@@ -29,8 +29,12 @@ func newFilesystemClient(endpoint *url.URL) *FilesystemClient {
 	return &FilesystemClient{lfsDir}
 }
 
-func (c *FilesystemClient) objectPath(oid string) string {
+func (c *FilesystemClient) objectPath(oid string) (string, error) {
-	return filepath.Join(c.lfsDir, oid[0:2], oid[2:4], oid)
+	if !isValidOid(oid) {
+		return "", ErrInvalidOIDFormat
+	}
+	// HINT: LFS-INTERNAL-PATH-PATTERN
+	return filepath.Join(c.lfsDir, oid[0:2], oid[2:4], oid), nil
 }
 
 // Download reads the specific LFS object from the target path
@@ -38,8 +42,10 @@ func (c *FilesystemClient) Download(ctx context.Context, objects []Pointer, call
 	for _, object := range objects {
 		p := Pointer{object.Oid, object.Size}
 
-		objectPath := c.objectPath(p.Oid)
+		objectPath, err := c.objectPath(p.Oid)
-
+		if err != nil {
+			return err
+		}
 		f, err := os.Open(objectPath)
 		if err != nil {
 			return err
@@ -57,8 +63,10 @@ func (c *FilesystemClient) Upload(ctx context.Context, objects []Pointer, callba
 	for _, object := range objects {
 		p := Pointer{object.Oid, object.Size}
 
-		objectPath := c.objectPath(p.Oid)
+		objectPath, err := c.objectPath(p.Oid)
-
+		if err != nil {
+			return err
+		}
 		if err := os.MkdirAll(filepath.Dir(objectPath), os.ModePerm); err != nil {
 			return err
 		}

modules/lfs/pointer.go

@@ -10,9 +10,11 @@ import (
 	"fmt"
 	"io"
 	"path"
-	"regexp"
 	"strconv"
 	"strings"
+
+	"gitea.dev/modules/setting"
+	"gitea.dev/modules/util"
 )
 
 // spec: https://github.com/git-lfs/git-lfs/blob/master/docs/spec.md
@@ -47,8 +49,6 @@ func ReadPointer(reader io.Reader) (Pointer, error) {
 	return ReadPointerFromBuffer(buf)
 }
 
-var oidPattern = regexp.MustCompile(`^[a-f\d]{64}$`)
-
 // ReadPointerFromBuffer will return a pointer if the provided byte slice is a pointer file or an error otherwise.
 func ReadPointerFromBuffer(buf []byte) (Pointer, error) {
 	var p Pointer
@@ -65,7 +65,7 @@ func ReadPointerFromBuffer(buf []byte) (Pointer, error) {
 
 	// spec says "key/value pairs MUST be sorted alphabetically in ascending order (version is exception and must be the first)"
 	oid := strings.TrimPrefix(splitLines[1], MetaFileOidPrefix)
-	if len(oid) != 64 || !oidPattern.MatchString(oid) {
+	if !isValidOid(oid) {
 		return p, ErrInvalidOIDFormat
 	}
 	size, err := strconv.ParseInt(strings.TrimPrefix(splitLines[2], "size "), 10, 64)
@@ -79,13 +79,24 @@ func ReadPointerFromBuffer(buf []byte) (Pointer, error) {
 	return p, nil
 }
 
+func isValidOid(oid string) bool {
+	if len(oid) != 64 {
+		return false
+	}
+	for i := range len(oid) {
+		c := oid[i]
+		valid := (c >= '0' && c <= '9') || (c >= 'a' && c <= 'f')
+		if !valid {
+			return false
+		}
+	}
+	return true
+}
+
 // IsValid checks if the pointer has a valid structure.
 // It doesn't check if the pointed-to-content exists.
 func (p Pointer) IsValid() bool {
-	if len(p.Oid) != 64 {
+	if !isValidOid(p.Oid) {
-		return false
-	}
-	if !oidPattern.MatchString(p.Oid) {
 		return false
 	}
 	if p.Size < 0 {
@@ -102,10 +113,13 @@ func (p Pointer) StringContent() string {
 
 // RelativePath returns the relative storage path of the pointer
 func (p Pointer) RelativePath() string {
-	if len(p.Oid) < 5 {
+	if !isValidOid(p.Oid) {
-		return p.Oid
+		setting.PanicInDevOrTesting("invalid LFS OID: %s", p.Oid)
+		return "invalid-lfs-oid-" + util.PathJoinRelX(p.Oid)
 	}
-
+	// HINT: LFS-INTERNAL-PATH-PATTERN: this path pattern is only used by Gitea internally,
+	// It isn't the same as the path pattern used by Git LFS, which is: <oid[0:2]>/<oid[2:4]>/<oid>
+	// However, there are other parts of Gitea that rely on such path pattern.
 	return path.Join(p.Oid[0:2], p.Oid[2:4], p.Oid[4:])
 }
 

modules/lfs/pointer_test.go

@@ -8,6 +8,9 @@ import (
 	"strings"
 	"testing"
 
+	"gitea.dev/modules/setting"
+	"gitea.dev/modules/test"
+
 	"github.com/stretchr/testify/assert"
 )
 
@@ -22,8 +25,10 @@ func TestRelativePath(t *testing.T) {
 	expected := path.Join("4d", "7a", "214614ab2935c943f9e0ff69d22eadbb8f32b1258daaa5e2ca24d17e2393")
 	assert.Equal(t, expected, p.RelativePath())
 
+	defer test.MockVariableValue(&setting.IsProd, true)()
+	defer test.MockVariableValue(&setting.IsInTesting, false)()
 	p2 := Pointer{Oid: "4d7a"}
-	assert.Equal(t, "4d7a", p2.RelativePath())
+	assert.Equal(t, "invalid-lfs-oid-4d7a", p2.RelativePath())
 }
 
 func TestIsValid(t *testing.T) {

services/doctor/storage.go

@@ -119,6 +119,7 @@ func checkStorage(opts *checkStorageOptions) func(ctx context.Context, logger lo
 				&commonStorageCheckOptions{
 					storer: storage.LFS,
 					isOrphaned: func(path string, obj storage.Object, stat fs.FileInfo) (bool, error) {
+						// HINT: LFS-INTERNAL-PATH-PATTERN
 						// The oid of an LFS stored object is the name but with all the path.Separators removed
 						oid := strings.ReplaceAll(strings.ReplaceAll(path, "\\", ""), "/", "")
 						exists, err := git.ExistsLFSObject(ctx, oid)