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())