e34914699128c937afad167d43ccb5310e48b319
- Author
- Kieran Klukas <kieran@dunkirk.sh>
- Committer
- GitHub <noreply@github.com>
- Date
Message
Merge commit from fork The Git LFS lock endpoints check the caller's permissions against the repository in the request URL, but looked locks up by their global ID alone. Anyone with write access to any repository could therefore walk lock IDs and read locks belonging to repositories they have no access to, disclosing file paths, the name of the user holding the lock, and when it was taken. Lock file paths from a private repository are themselves sensitive, since they reveal what a project is working on. Look locks up scoped to the repository being requested, matching how every other lock query already behaves. Two callers were affected: the lock list endpoint, which returned the lock outright, and the unlock endpoint, which echoed the path and owner back while refusing the delete. Deleting was already scoped correctly, so the unlock route disclosed information rather than destroying it, and also confirmed whether a given lock ID existed elsewhere on the server. Reported-by: sondt99
Diff
This diff is truncated to protect this page.
1diff --git a/pkg/store/database/lfs.go b/pkg/store/database/lfs.go
2index 8179d18031ae352fa1400d4cdc6d592c9a7c7c7a..15031016b41d1e0b7e28754aec321539c832808a 100644
3--- a/pkg/store/database/lfs.go
4+++ b/pkg/store/database/lfs.go
5@@ -112,14 +112,14 @@ func (*lfsStore) GetLFSLockForUserPath(ctx context.Context, tx db.Handler, repoI
6 }
7
8 // GetLFSLockByID implements store.LFSStore.
9-func (*lfsStore) GetLFSLockByID(ctx context.Context, tx db.Handler, id int64) (models.LFSLock, error) {
10+func (*lfsStore) GetLFSLockByID(ctx context.Context, tx db.Handler, repoID int64, id int64) (models.LFSLock, error) {
11 var lock models.LFSLock
12 query := tx.Rebind(`
13 SELECT *
14 FROM lfs_locks
15- WHERE lfs_locks.id = ?;
16+ WHERE id = ? AND repo_id = ?;
17 `)
18- err := tx.GetContext(ctx, &lock, query, id)
19+ err := tx.GetContext(ctx, &lock, query, id, repoID)
20 return lock, db.WrapError(err)
21 }
22
23diff --git a/pkg/store/lfs.go b/pkg/store/lfs.go
24index 31611e9b5a607d376b5f2dadf7c0d3476ce6e035..f96f82c9a0aeae4965514c37ad0c911ec884f0d4 100644
25--- a/pkg/store/lfs.go
26+++ b/pkg/store/lfs.go
27@@ -21,7 +21,7 @@ type LFSStore interface {
28 GetLFSLocksForUser(ctx context.Context, h db.Handler, repoID int64, userID int64) ([]models.LFSLock, error)
29 GetLFSLockForPath(ctx context.Context, h db.Handler, repoID int64, path string) (models.LFSLock, error)
30 GetLFSLockForUserPath(ctx context.Context, h db.Handler, repoID int64, userID int64, path string) (models.LFSLock, error)
31- GetLFSLockByID(ctx context.Context, h db.Handler, id int64) (models.LFSLock, error)
32+ GetLFSLockByID(ctx context.Context, h db.Handler, repoID int64, id int64) (models.LFSLock, error)
33 GetLFSLockForUserByID(ctx context.Context, h db.Handler, repoID int64, userID int64, id int64) (models.LFSLock, error)
34 DeleteLFSLock(ctx context.Context, h db.Handler, repoID int64, id int64) error
35 DeleteLFSLockForUserByID(ctx context.Context, h db.Handler, repoID int64, userID int64, id int64) error
36diff --git a/pkg/web/git_lfs.go b/pkg/web/git_lfs.go
37index 647e932ea392108c2b8090e16bbba5f7d42266ec..a7def2157ebc3068aca783a996bc31be8d6be15d 100644
38--- a/pkg/web/git_lfs.go
39+++ b/pkg/web/git_lfs.go
40@@ -608,7 +608,7 @@ func serviceLfsLocksGet(w http.ResponseWriter, r *http.Request) {
41 }
42
43 if id > 0 {
44- lock, err := datastore.GetLFSLockByID(ctx, dbx, id)
45+ lock, err := datastore.GetLFSLockByID(ctx, dbx, repo.ID(), id)
46 if err != nil {
47 if errors.Is(err, db.ErrRecordNotFound) {
48 renderJSON(w, http.StatusNotFound, lfs.ErrorResponse{
49@@ -874,8 +874,9 @@ func serviceLfsLocksDelete(w http.ResponseWriter, r *http.Request) {
50 return
51 }
52
53- // The lock being deleted
54- lock, err := datastore.GetLFSLockByID(ctx, dbx, lockID)
55+ // The lock being deleted. Looked up scoped to this repository so that a
56+ // lock ID belonging to another repository cannot be read through it.
57+ lock, err := datastore.GetLFSLockByID(ctx, dbx, repo.ID(), lockID)
58 if err != nil {
59 logger.Error("error getting lock", "err", err)
60 renderJSON(w, http.StatusNotFound, lfs.ErrorResponse{
61diff --git a/pkg/web/git_lfs_test.go b/pkg/web/git_lfs_test.go
62new file mode 100644
63index 0000000000000000000000000000000000000000..376c70ea1769f5ffecdc69c896323c07813f2e69
64--- /dev/null
65+++ b/pkg/web/git_lfs_test.go
66@@ -0,0 +1,203 @@
67+package web
68+
69+import (
70+ "context"
71+ "encoding/json"
72+ "net/http"
73+ "net/http/httptest"
74+ "strconv"
75+ "strings"
76+ "testing"
77+
78+ "github.com/charmbracelet/soft-serve/pkg/backend"
79+ "github.com/charmbracelet/soft-serve/pkg/config"
80+ "github.com/charmbracelet/soft-serve/pkg/db"
81+ "github.com/charmbracelet/soft-serve/pkg/db/migrate"
82+ "github.com/charmbracelet/soft-serve/pkg/lfs"
83+ "github.com/charmbracelet/soft-serve/pkg/proto"
84+ "github.com/charmbracelet/soft-serve/pkg/store"
85+ "github.com/charmbracelet/soft-serve/pkg/store/database"
86+ "github.com/gorilla/mux"
87+ "github.com/matryer/is"
88+ _ "modernc.org/sqlite"
89+)
90+
91+// newLFSTestContext returns a context wired with a config and a real migrated
92+// SQLite-backed backend, plus the backend and datastore so tests can create
93+// users, repositories, and locks.
94+func newLFSTestContext(t *testing.T) (context.Context, *backend.Backend, store.Store) {
95+ t.Helper()
96+ is := is.New(t)
97+ ctx := context.Background()
98+
99+ dp := t.TempDir()
100+ cfg := config.DefaultConfig()
101+ cfg.DataPath = dp
102+ cfg.DB.Driver = "sqlite"
103+ cfg.DB.DataSource = dp + "/test.db"
104+
105+ ctx = config.WithContext(ctx, cfg)
106+ dbx, err := db.Open(ctx, cfg.DB.Driver, cfg.DB.DataSource)
107+ is.NoErr(err)
108+ t.Cleanup(func() { dbx.Close() }) //nolint:errcheck
109+
110+ is.NoErr(migrate.Migrate(ctx, dbx))
111+ ctx = db.WithContext(ctx, dbx)
112+ datastore := database.New(ctx, dbx)
113+ ctx = store.WithContext(ctx, datastore)
114+ be := backend.New(ctx, cfg, dbx, datastore)
115+ ctx = backend.WithContext(ctx, be)
116+
117+ return ctx, be, datastore
118+}
119+
120+// TestLFSLockLookupIsScopedToRepository verifies that a lock cannot be read by
121+// global lock ID from a repository it does not belong to.
122+//
123+// The LFS lock routes authorize the caller against the repository in the URL,
124+// but the lock list handler accepts an `id` query parameter and used to fetch
125+// the lock by global ID alone. A user with write access to any repository could
126+// walk lock IDs and read file paths, owner usernames, and timestamps out of
127+// private repositories they have no access to.
128+func TestLFSLockLookupIsScopedToRepository(t *testing.T) {
129+ is := is.New(t)
130+ ctx, be, datastore := newLFSTestContext(t)
131+ dbx := db.FromContext(ctx)
132+
133+ // The victim owns a private repository with a lock on a sensitive path.
134+ victim, err := be.CreateUser(ctx, "victim", proto.UserOptions{})
135+ is.NoErr(err)
136+ victimRepo, err := be.CreateRepository(ctx, "victim-repo", victim, proto.RepositoryOptions{Private: true})
137+ is.NoErr(err)
138+ is.NoErr(datastore.CreateLFSLockForUser(ctx, dbx, victimRepo.ID(), victim.ID(), "secret/plans.bin", "refs/heads/main"))
139+
140+ victimLock, err := datastore.GetLFSLockForPath(ctx, dbx, victimRepo.ID(), "secret/plans.bin")
141+ is.NoErr(err)
142+
143+ // The attacker owns their own repository, so they have write access to
144+ // it, but no access at all to the victim's repository.
145+ attacker, err := be.CreateUser(ctx, "attacker", proto.UserOptions{})
146+ is.NoErr(err)
147+ attackerRepo, err := be.CreateRepository(ctx, "attacker-repo", attacker, proto.RepositoryOptions{})
148+ is.NoErr(err)
149+
150+ // The store must not return a lock that belongs to another repository.
151+ _, err = datastore.GetLFSLockByID(ctx, dbx, attackerRepo.ID(), victimLock.ID)
152+ if err == nil {
153+ t.Fatal("expected error reading another repository's lock by ID")
154+ }
155+
156+ // The owner can still read their own lock by ID.
157+ got, err := datastore.GetLFSLockByID(ctx, dbx, victimRepo.ID(), victimLock.ID)
158+ is.NoErr(err)
159+ is.Equal(got.Path, "secret/plans.bin")
160+}
161+
162+// TestLFSLocksGetDoesNotLeakAcrossRepositories drives the HTTP lock list
163+// handler the way an attacker would: authorized for their own repository,
164+// asking for a lock ID that belongs to somebody else's private repository.
165+func TestLFSLocksGetDoesNotLeakAcrossRepositories(t *testing.T) {