From 7e1e32b42ed99efc31dd530aedff8c10bf0f99bc Mon Sep 17 00:00:00 2001 From: Giteabot Date: Tue, 13 Oct 2026 17:19:59 -0700 Subject: [PATCH] fix(lfs): validate OID before building filesystem object path (#39762) (#39766) Backport #39762 Co-authored-by: Basavaraj S m Co-authored-by: wxiaoguang --- modules/lfs/client_test.go | 6 +++++- modules/lfs/filesystem_client.go | 22 ++++++++++++------- modules/lfs/pointer.go | 36 ++++++++++++++++++++++---------- modules/lfs/pointer_test.go | 7 ++++++- services/doctor/storage.go | 1 + 5 files changed, 52 insertions(+), 20 deletions(-) diff --git a/modules/lfs/client_test.go b/modules/lfs/client_test.go index db6403e07ab..046fa8a7348 100644 --- a/modules/lfs/client_test.go +++ b/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) diff --git a/modules/lfs/filesystem_client.go b/modules/lfs/filesystem_client.go index bf95e3ec6db..b10ca817aef 100644 --- a/modules/lfs/filesystem_client.go +++ b/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 { - return filepath.Join(c.lfsDir, oid[0:2], oid[2:4], oid) +func (c *FilesystemClient) objectPath(oid string) (string, error) { + 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 } diff --git a/modules/lfs/pointer.go b/modules/lfs/pointer.go index 9c95613057b..af6db2d3c24 100644 --- a/modules/lfs/pointer.go +++ b/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 { - return false - } - if !oidPattern.MatchString(p.Oid) { + if !isValidOid(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 { - return p.Oid + if !isValidOid(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: // + // 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:]) } diff --git a/modules/lfs/pointer_test.go b/modules/lfs/pointer_test.go index 41b5459fef6..3d274211837 100644 --- a/modules/lfs/pointer_test.go +++ b/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) { diff --git a/services/doctor/storage.go b/services/doctor/storage.go index bf48857e577..21b160ae0f2 100644 --- a/services/doctor/storage.go +++ b/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)