27f524593a281662874408dd31f270d73c72f48c

Message

fix: authorize LFS requests by route rather than claimed service

The HTTP middleware picked which permission checks to apply from the Git
service being requested, which on an LFS route came from a caller-supplied
query parameter. Appending ?service=git-upload-pack to an LFS upload or
lock request matched the earlier upload-pack branch and left the switch
before reaching the LFS write checks, so under the shipped defaults an
unauthenticated caller could write objects into any public repository. The
skipped branch also held the check that LFS is enabled at all.

Match the LFS path prefix before the service cases; that prefix comes from
the request path, not from anything the caller can restate. Basic upload
and lock create now check write access themselves too, as the batch
handler already did.

The upload endpoint still does not verify that stored content hashes to
the object ID in the URL. That is a distinct weakness, not fixed here.

Reported-by: xmsama (Little_Ming) <36942744+xmsama@users.noreply.github.com>

Diff

This diff is truncated to protect this page.

  1diff --git a/pkg/web/git.go b/pkg/web/git.go
  2index 5a15ed2eda6e2f29ff156971ba0204fce3c504cb..16c967ffab9ff0fe83d93e1346f3c8abf8a11072 100644
  3--- a/pkg/web/git.go
  4+++ b/pkg/web/git.go
  5@@ -252,43 +252,15 @@ func withAccess(next http.Handler) http.HandlerFunc {
  6 		// - git-upload-pack
  7 		// - git-receive-pack
  8 		// - git-lfs
  9+		//
 10+		// The LFS case must stay ahead of the service cases. "file" is derived
 11+		// from the request path by withParams, but "service" can come from a
 12+		// query parameter the caller controls, and withParams only fills it in
 13+		// for paths ending in git-upload-pack or git-receive-pack, so it is
 14+		// always caller-supplied on an LFS route. Matching the path first means
 15+		// an LFS request is authorized as LFS no matter what service it claims
 16+		// to be.
 17 		switch {
 18-		case service == git.ReceivePackService:
 19-			if accessLevel < access.ReadWriteAccess {
 20-				askCredentials(w, r)
 21-				renderUnauthorized(w, r)
 22-				return
 23-			}
 24-
 25-			// Create the repo if it doesn't exist.
 26-			if repo == nil {
 27-				repo, err = be.CreateRepository(ctx, repoName, user, proto.RepositoryOptions{})
 28-				if err != nil {
 29-					logger.Error("failed to create repository", "repo", repoName, "err", err)
 30-					renderInternalServerError(w, r)
 31-					return
 32-				}
 33-
 34-				ctx = proto.WithRepositoryContext(ctx, repo)
 35-				r = r.WithContext(ctx)
 36-			}
 37-
 38-			fallthrough
 39-		case service == git.UploadPackService || service == git.UploadArchiveService:
 40-			if repo == nil {
 41-				// If the repo doesn't exist, return 404
 42-				renderNotFound(w, r)
 43-				return
 44-			} else if errors.Is(err, ErrInvalidToken) || errors.Is(err, ErrInvalidPassword) {
 45-				// return 403 when bad credentials are provided
 46-				renderForbidden(w, r)
 47-				return
 48-			} else if accessLevel < access.ReadOnlyAccess {
 49-				askCredentials(w, r)
 50-				renderUnauthorized(w, r)
 51-				return
 52-			}
 53-
 54 		case strings.HasPrefix(file, "info/lfs"):
 55 			if !cfg.LFS.Enabled {
 56 				logger.Debug("LFS is not enabled, skipping")
 57@@ -346,6 +318,42 @@ func withAccess(next http.Handler) http.HandlerFunc {
 58 				}
 59 				return
 60 			}
 61+
 62+		case service == git.ReceivePackService:
 63+			if accessLevel < access.ReadWriteAccess {
 64+				askCredentials(w, r)
 65+				renderUnauthorized(w, r)
 66+				return
 67+			}
 68+
 69+			// Create the repo if it doesn't exist.
 70+			if repo == nil {
 71+				repo, err = be.CreateRepository(ctx, repoName, user, proto.RepositoryOptions{})
 72+				if err != nil {
 73+					logger.Error("failed to create repository", "repo", repoName, "err", err)
 74+					renderInternalServerError(w, r)
 75+					return
 76+				}
 77+
 78+				ctx = proto.WithRepositoryContext(ctx, repo)
 79+				r = r.WithContext(ctx)
 80+			}
 81+
 82+			fallthrough
 83+		case service == git.UploadPackService || service == git.UploadArchiveService:
 84+			if repo == nil {
 85+				// If the repo doesn't exist, return 404
 86+				renderNotFound(w, r)
 87+				return
 88+			} else if errors.Is(err, ErrInvalidToken) || errors.Is(err, ErrInvalidPassword) {
 89+				// return 403 when bad credentials are provided
 90+				renderForbidden(w, r)
 91+				return
 92+			} else if accessLevel < access.ReadOnlyAccess {
 93+				askCredentials(w, r)
 94+				renderUnauthorized(w, r)
 95+				return
 96+			}
 97 		}
 98 
 99 		switch {
100diff --git a/pkg/web/git_lfs.go b/pkg/web/git_lfs.go
101index a7def2157ebc3068aca783a996bc31be8d6be15d..12d3cbd61307fdb9fddfd3650230626edb594e49 100644
102--- a/pkg/web/git_lfs.go
103+++ b/pkg/web/git_lfs.go
104@@ -302,6 +302,17 @@ func serviceLfsBasicUpload(w http.ResponseWriter, r *http.Request) {
105 	}
106 
107 	ctx := r.Context()
108+
109+	// Checked in withAccess too. Repeated here so a routing or middleware
110+	// mistake cannot turn into an unauthenticated write.
111+	if access.FromContext(ctx) < access.ReadWriteAccess {
112+		askCredentials(w, r)
113+		renderJSON(w, http.StatusForbidden, lfs.ErrorResponse{
114+			Message: "write access required",
115+		})
116+		return
117+	}
118+
119 	oid := mux.Vars(r)["oid"]
120 	cfg := config.FromContext(ctx)
121 	be := backend.FromContext(ctx)
122@@ -464,6 +475,16 @@ func serviceLfsLocksCreate(w http.ResponseWriter, r *http.Request) {
123 	ctx := r.Context()
124 	logger := log.FromContext(ctx).WithPrefix("http.lfs-locks")
125 
126+	// Checked in withAccess too. Repeated here so a routing or middleware
127+	// mistake cannot turn into an unauthorized lock.
128+	if access.FromContext(ctx) < access.ReadWriteAccess {
129+		askCredentials(w, r)
130+		renderJSON(w, http.StatusForbidden, lfs.ErrorResponse{
131+			Message: "write access required",
132+		})
133+		return
134+	}
135+
136 	var req lfs.LockCreateRequest
137 	if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
138 		logger.Error("error decoding json", "err", err)
139diff --git a/pkg/web/git_lfs_test.go b/pkg/web/git_lfs_test.go
140index 376c70ea1769f5ffecdc69c896323c07813f2e69..bb158935c62b0a62fc11c37abb7a1a237f54d53e 100644
141--- a/pkg/web/git_lfs_test.go
142+++ b/pkg/web/git_lfs_test.go
143@@ -5,9 +5,12 @@ import (
144 	"encoding/json"
145 	"net/http"
146 	"net/http/httptest"
147+	"os"
148+	"path/filepath"
149 	"strconv"
150 	"strings"
151 	"testing"
152+	"time"
153 
154 	"github.com/charmbracelet/soft-serve/pkg/backend"
155 	"github.com/charmbracelet/soft-serve/pkg/config"
156@@ -201,3 +204,194 @@ func TestLFSLocksDeleteDoesNotLeakAcrossRepositories(t *testing.T) {
157 	is.NoErr(err)
158 	is.Equal(still.Path, "secret/plans.bin")
159 }
160+
161+// forgeableServices are the service names that select a non-LFS branch in
162+// withAccess. None of them may change how an LFS route is authorized.
163+var forgeableServices = []string{"git-upload-pack", "git-receive-pack", "git-upload-archive"}
164+
165+// lfsTestRouter builds the real git router so requests pass through withParams
166+// and withAccess rather than reaching a handler directly. The middleware is
167+// where LFS authorization lives, so a test that calls the handler itself cannot
168+// see a bypass.
169+func lfsTestRouter(ctx context.Context) *mux.Router {
170+	router := mux.NewRouter()
171+	GitController(ctx, router)
172+	return router
173+}
174+
175+// TestLFSUploadRejectsForgedServiceParam covers an authorization bypass in
176+// withAccess: the LFS branch was selected by a `service` value that, on an LFS
177+// route, always came from a caller-supplied query parameter. Because the
178+// git-upload-pack branch sat earlier in the same switch, appending
179+// `?service=git-upload-pack` matched that branch instead and skipped every LFS
180+// write check. Under the shipped defaults (anon-access read-only, LFS enabled)
181+// an unauthenticated caller could write objects into any public repository.
182+func TestLFSUploadRejectsForgedServiceParam(t *testing.T) {
183+	is := is.New(t)
184+	ctx, be, datastore := newLFSTestContext(t)
185+	cfg := config.FromContext(ctx)
186+	dbx := db.FromContext(ctx)
187+
188+	owner, err := be.CreateUser(ctx, "owner", proto.UserOptions{})
189+	is.NoErr(err)
190+	repo, err := be.CreateRepository(ctx, "victim-repo", owner, proto.RepositoryOptions{})
191+	is.NoErr(err)
192+
193+	router := lfsTestRouter(ctx)
194+	oid := strings.Repeat("a", 64)
195+	path := "/victim-repo.git/info/lfs/objects/basic/" + oid
196+
197+	upload := func(url string) *httptest.ResponseRecorder {
198+		req := httptest.NewRequestWithContext(ctx, http.MethodPut, url, strings.NewReader("payload"))
199+		req.Header.Set("Content-Type", "application/octet-stream")
200+		w := httptest.NewRecorder()
201+		router.ServeHTTP(w, req)
202+		return w
203+	}
204+
205+	// Baseline: anonymous read-only access cannot upload.
206+	is.Equal(upload(path).Code, http.StatusForbidden)
207+
208+	for _, service := range forgeableServices {
209+		w := upload(path + "?service=" + service)
210+		if w.Code != http.StatusForbidden {
211+			t.Errorf("service=%s: got status %d, want 403: %s", service, w.Code, w.Body.String())
212+		}
213+	}
214+
215+	// The object must not exist on disk or in the database. The handler writes
216+	// the body before it parses Content-Length, so an error status alone does
217+	// not prove nothing was stored.
218+	objPath := filepath.Join(cfg.DataPath, "lfs", strconv.FormatInt(repo.ID(), 10),
219+		"objects", oid[0:2], oid[2:4], oid)
220+	if _, err := os.Stat(objPath); !os.IsNotExist(err) {
221+		t.Errorf("object written to disk at %s despite rejection", objPath)
222+	}
223+	if _, err := datastore.GetLFSObjectByOid(ctx, dbx, repo.ID(), oid); err == nil {
224+		t.Error("object registered in database despite rejection")
225+	}
226+}
227+
228+// TestLFSLockCreateRejectsForgedServiceParam is the lock-create half of the
229+// same bypass: a read-only collaborator could take locks on arbitrary paths.
230+func TestLFSLockCreateRejectsForgedServiceParam(t *testing.T) {
231+	is := is.New(t)
232+	ctx, be, datastore := newLFSTestContext(t)
233+	dbx := db.FromContext(ctx)
234+
235+	owner, err := be.CreateUser(ctx, "owner", proto.UserOptions{})
236+	is.NoErr(err)
237+	repo, err := be.CreateRepository(ctx, "victim-repo", owner, proto.RepositoryOptions{})
238+	is.NoErr(err)
239+
240+	// An authenticated user with no more than read access to the repository.
241+	attacker, err := be.CreateUser(ctx, "attacker", proto.UserOptions{})
242+	is.NoErr(err)