From d9534e7bfaaa914f5320515ad789e6e470c92e4a Mon Sep 17 00:00:00 2001 From: Giteabot Date: Thu, 9 Jul 2026 15:18:07 -0700 Subject: [PATCH] fix(lfs): require proof of possession for cross-repo objects (#38322) (#38389) Backport #38322 by @bircni The LFS batch and upload handlers linked an object that already existed in the content store but was not linked to the current repo whenever the token's user could access it in another repo. Deploy-key tokens carry the repo owner's identity, so a single-repo write deploy key could link and then download objects from any repo the owner can see. This drops the cross-repo access check: the batch handler now makes the client upload (hash-verified) any object not yet linked to the repo, and the upload handler skips proof of possession only when the object is already linked to the current repo. Co-authored-by: bircni --- services/lfs/server.go | 44 ++++++++++++-------------- tests/integration/api_repo_lfs_test.go | 16 +++++++--- 2 files changed, 32 insertions(+), 28 deletions(-) diff --git a/services/lfs/server.go b/services/lfs/server.go index 6340b062521..8c7de2ccb65 100644 --- a/services/lfs/server.go +++ b/services/lfs/server.go @@ -254,6 +254,17 @@ func BatchHandler(ctx *context.Context) { var responseObject *lfs_module.ObjectResponse if isUpload { + if exists && meta == nil { + // The object exists in the content store but is not linked to this + // repo. Do not auto-link it based on cross-repo access: the token + // only authorizes this repo, and for deploy keys ctx.Doer is the + // repo owner, so trusting it here would let a single-repo key pull + // objects from any repo the owner can see. Require proof of + // possession by making the client upload the (hash-verified) bytes, + // and treat it as a new upload for size-limit enforcement below. + exists = false + } + var err *lfs_module.ObjectError if !exists && setting.LFS.MaxFileSize > 0 && p.Size > setting.LFS.MaxFileSize { err = &lfs_module.ObjectError{ @@ -262,25 +273,6 @@ func BatchHandler(ctx *context.Context) { } } - if exists && meta == nil { - accessible, err := git_model.LFSObjectAccessible(ctx, ctx.Doer, p.Oid) - if err != nil { - log.Error("Unable to check if LFS MetaObject [%s] is accessible. Error: %v", p.Oid, err) - writeStatus(ctx, http.StatusInternalServerError) - return - } - if accessible { - _, err := git_model.NewLFSMetaObject(ctx, repository.ID, p) - if err != nil { - log.Error("Unable to create LFS MetaObject [%s] for %s/%s. Error: %v", p.Oid, rc.User, rc.Repo, err) - writeStatus(ctx, http.StatusInternalServerError) - return - } - } else { - exists = false - } - } - responseObject = buildObjectResponse(rc, p, false, !exists, err) } else { var err *lfs_module.ObjectError @@ -337,13 +329,17 @@ func UploadHandler(ctx *context.Context) { uploadOrVerify := func() error { if exists { - accessible, err := git_model.LFSObjectAccessible(ctx, ctx.Doer, p.Oid) - if err != nil { - log.Error("Unable to check if LFS MetaObject [%s] is accessible. Error: %v", p.Oid, err) + // The bytes already exist in the content store. Only skip proof of + // possession when the object is already linked to *this* repo; never + // trust cross-repo access (ctx.Doer is the repo owner for deploy keys), + // which would let a caller link an object it cannot produce. + meta, err := git_model.GetLFSMetaObjectByOid(ctx, repository.ID, p.Oid) + if err != nil && err != git_model.ErrLFSObjectNotExist { + log.Error("Unable to get LFS MetaObject [%s]. Error: %v", p.Oid, err) return err } - if !accessible { - // The file exists but the user has no access to it. + if meta == nil { + // The file exists but is not linked to this repo. // The upload gets verified by hashing and size comparison to prove access to it. hash := sha256.New() written, err := io.Copy(hash, ctx.Req.Body) diff --git a/tests/integration/api_repo_lfs_test.go b/tests/integration/api_repo_lfs_test.go index 5dc8671e2ae..0f65606017f 100644 --- a/tests/integration/api_repo_lfs_test.go +++ b/tests/integration/api_repo_lfs_test.go @@ -240,9 +240,14 @@ func TestAPILFSBatch(t *testing.T) { assert.Equal(t, "Size must be less than or equal to 2", br.Objects[0].Error.Message) }) - t.Run("AddMeta", func(t *testing.T) { + t.Run("CrossRepoObjectRequiresUpload", func(t *testing.T) { defer tests.PrintCurrentTest(t)() + // An object whose bytes already exist in the store but which is not + // linked to this repo must not be silently linked, even when the + // caller can access it in another repo. Auto-linking let a deploy key + // (whose token carries the repo owner's identity) exfiltrate objects + // across repos without proving possession. The client must upload. p := lfs.Pointer{Oid: "05eeb4eb5be71f2dd291ca39157d6d9effd7d1ea19cbdc8a99411fe2a8f26a00", Size: 6} contentStore := lfs.NewContentStore() @@ -250,6 +255,7 @@ func TestAPILFSBatch(t *testing.T) { assert.NoError(t, err) assert.True(t, exist) + // The object is linked to another repo owned by the same user. repo2 := createLFSTestRepository(t, "lfs-batch2-repo") storeObjectInRepo(t, repo2.ID, "dummy0") @@ -266,11 +272,13 @@ func TestAPILFSBatch(t *testing.T) { br := decodeResponse(t, resp.Body) assert.Len(t, br.Objects, 1) assert.Nil(t, br.Objects[0].Error) - assert.Empty(t, br.Objects[0].Actions) + // The client is told to upload instead of the object being linked. + assert.Contains(t, br.Objects[0].Actions, "upload") + // No meta object may have been created for this repo. meta, err = git_model.GetLFSMetaObjectByOid(t.Context(), repo.ID, p.Oid) - assert.NoError(t, err) - assert.NotNil(t, meta) + assert.Nil(t, meta) + assert.Equal(t, git_model.ErrLFSObjectNotExist, err) // Cleanup err = contentStore.Delete(p.RelativePath())