From 2ab5e0cbecc1c477009439a9b95a7fa01209ba4b Mon Sep 17 00:00:00 2001 From: bircni Date: Fri, 3 Jul 2026 19:21:19 +0200 Subject: [PATCH] fix(lfs): require proof of possession for cross-repo objects 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 any other repo. Because deploy-key tokens carry the repo owner's identity, a single-repo write deploy key could link and then download objects from any repo the owner can see. Stop using the cross-repo access check: the batch handler now requires the client to 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. --- services/lfs/server.go | 37 +++++++++++--------------- tests/integration/api_repo_lfs_test.go | 16 ++++++++--- 2 files changed, 28 insertions(+), 25 deletions(-) diff --git a/services/lfs/server.go b/services/lfs/server.go index 6340b06252141..cf6cb4e3f246f 100644 --- a/services/lfs/server.go +++ b/services/lfs/server.go @@ -263,22 +263,13 @@ 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 - } + // 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. + exists = false } responseObject = buildObjectResponse(rc, p, false, !exists, err) @@ -337,13 +328,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 5dc8671e2aebd..0f65606017f61 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())