fix: return bad request on malformed packages upload input (#10954)

I noticed that the wrong content type in an `/upload` request can trigger a 500, and I'm guessing it is more appropriate to return 400 instead.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/10954
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: Roberto Vidal <roberto.vidal@ikumene.com>
Co-committed-by: Roberto Vidal <roberto.vidal@ikumene.com>

routers/api/packages/alpine/alpine.go

@@ -146,7 +146,11 @@ func UploadPackageFile(ctx *context.Context) {
 
 	upload, needToClose, err := ctx.UploadStream()
 	if err != nil {
-		apiError(ctx, http.StatusInternalServerError, err)
+		if context.IsFormError(err) {
+			apiError(ctx, http.StatusBadRequest, err)
+		} else {
+			apiError(ctx, http.StatusInternalServerError, err)
+		}
 		return
 	}
 	if needToClose {

routers/api/packages/alt/alt.go

@@ -78,7 +78,11 @@ func GetRepositoryFile(ctx *context.Context, arch string) {
 func UploadPackageFile(ctx *context.Context) {
 	upload, needToClose, err := ctx.UploadStream()
 	if err != nil {
-		apiError(ctx, http.StatusInternalServerError, err)
+		if context.IsFormError(err) {
+			apiError(ctx, http.StatusBadRequest, err)
+		} else {
+			apiError(ctx, http.StatusInternalServerError, err)
+		}
 		return
 	}
 	if needToClose {

routers/api/packages/arch/arch.go

@@ -64,7 +64,11 @@ func PushPackage(ctx *context.Context) {
 	defer releaser()
 	upload, needToClose, err := ctx.UploadStream()
 	if err != nil {
-		apiError(ctx, http.StatusInternalServerError, err)
+		if context.IsFormError(err) {
+			apiError(ctx, http.StatusBadRequest, err)
+		} else {
+			apiError(ctx, http.StatusInternalServerError, err)
+		}
 		return
 	}
 	if needToClose {

routers/api/packages/conan/conan.go

@@ -316,7 +316,11 @@ func uploadFile(ctx *context.Context, fileFilter container.Set[string], fileKey
 
 	upload, needToClose, err := ctx.UploadStream()
 	if err != nil {
-		apiError(ctx, http.StatusBadRequest, err)
+		if context.IsFormError(err) {
+			apiError(ctx, http.StatusBadRequest, err)
+		} else {
+			apiError(ctx, http.StatusInternalServerError, err)
+		}
 		return
 	}
 	if needToClose {

routers/api/packages/conda/conda.go

@@ -176,7 +176,11 @@ func EnumeratePackages(ctx *context.Context) {
 func UploadPackageFile(ctx *context.Context) {
 	upload, needToClose, err := ctx.UploadStream()
 	if err != nil {
-		apiError(ctx, http.StatusInternalServerError, err)
+		if context.IsFormError(err) {
+			apiError(ctx, http.StatusBadRequest, err)
+		} else {
+			apiError(ctx, http.StatusInternalServerError, err)
+		}
 		return
 	}
 	if needToClose {

routers/api/packages/cran/cran.go

@@ -153,7 +153,11 @@ func UploadBinaryPackageFile(ctx *context.Context) {
 func uploadPackageFile(ctx *context.Context, compositeKey string, properties map[string]string) {
 	upload, needToClose, err := ctx.UploadStream()
 	if err != nil {
-		apiError(ctx, http.StatusBadRequest, err)
+		if context.IsFormError(err) {
+			apiError(ctx, http.StatusBadRequest, err)
+		} else {
+			apiError(ctx, http.StatusInternalServerError, err)
+		}
 		return
 	}
 	if needToClose {

routers/api/packages/debian/debian.go

@@ -129,7 +129,11 @@ func UploadPackageFile(ctx *context.Context) {
 
 	upload, needToClose, err := ctx.UploadStream()
 	if err != nil {
-		apiError(ctx, http.StatusInternalServerError, err)
+		if context.IsFormError(err) {
+			apiError(ctx, http.StatusBadRequest, err)
+		} else {
+			apiError(ctx, http.StatusInternalServerError, err)
+		}
 		return
 	}
 	if needToClose {

routers/api/packages/generic/generic.go

@@ -92,7 +92,11 @@ func UploadPackage(ctx *context.Context) {
 
 	upload, needToClose, err := ctx.UploadStream()
 	if err != nil {
-		apiError(ctx, http.StatusInternalServerError, err)
+		if context.IsFormError(err) {
+			apiError(ctx, http.StatusBadRequest, err)
+		} else {
+			apiError(ctx, http.StatusInternalServerError, err)
+		}
 		return
 	}
 	if needToClose {

routers/api/packages/goproxy/goproxy.go

@@ -156,7 +156,11 @@ func resolvePackage(ctx *context.Context, ownerID int64, name, version string) (
 func UploadPackage(ctx *context.Context) {
 	upload, needToClose, err := ctx.UploadStream()
 	if err != nil {
-		apiError(ctx, http.StatusInternalServerError, err)
+		if context.IsFormError(err) {
+			apiError(ctx, http.StatusBadRequest, err)
+		} else {
+			apiError(ctx, http.StatusInternalServerError, err)
+		}
 		return
 	}
 	if needToClose {

routers/api/packages/helm/helm.go

@@ -145,7 +145,11 @@ func DownloadPackageFile(ctx *context.Context) {
 func UploadPackage(ctx *context.Context) {
 	upload, needToClose, err := ctx.UploadStream()
 	if err != nil {
-		apiError(ctx, http.StatusInternalServerError, err)
+		if context.IsFormError(err) {
+			apiError(ctx, http.StatusBadRequest, err)
+		} else {
+			apiError(ctx, http.StatusInternalServerError, err)
+		}
 		return
 	}
 	if needToClose {

routers/api/packages/nuget/nuget.go

@@ -606,7 +606,11 @@ func processUploadedFile(ctx *context.Context, expectedType nuget_module.Package
 
 	upload, needToClose, err := ctx.UploadStream()
 	if err != nil {
-		apiError(ctx, http.StatusBadRequest, err)
+		if context.IsFormError(err) {
+			apiError(ctx, http.StatusBadRequest, err)
+		} else {
+			apiError(ctx, http.StatusInternalServerError, err)
+		}
 		return nil, nil, closables
 	}
 

routers/api/packages/rpm/rpm.go

@@ -119,7 +119,11 @@ func GetRepositoryFile(ctx *context.Context) {
 func UploadPackageFile(ctx *context.Context) {
 	upload, needToClose, err := ctx.UploadStream()
 	if err != nil {
-		apiError(ctx, http.StatusInternalServerError, err)
+		if context.IsFormError(err) {
+			apiError(ctx, http.StatusBadRequest, err)
+		} else {
+			apiError(ctx, http.StatusInternalServerError, err)
+		}
 		return
 	}
 	if needToClose {

routers/api/packages/rubygems/rubygems.go

@@ -285,7 +285,11 @@ func DownloadPackageFile(ctx *context.Context) {
 func UploadPackageFile(ctx *context.Context) {
 	upload, needToClose, err := ctx.UploadStream()
 	if err != nil {
-		apiError(ctx, http.StatusBadRequest, err)
+		if context.IsFormError(err) {
+			apiError(ctx, http.StatusBadRequest, err)
+		} else {
+			apiError(ctx, http.StatusInternalServerError, err)
+		}
 		return
 	}
 	if needToClose {

routers/api/packages/vagrant/vagrant.go

@@ -151,7 +151,11 @@ func UploadPackageFile(ctx *context.Context) {
 
 	upload, needsClose, err := ctx.UploadStream()
 	if err != nil {
-		apiError(ctx, http.StatusInternalServerError, err)
+		if context.IsFormError(err) {
+			apiError(ctx, http.StatusBadRequest, err)
+		} else {
+			apiError(ctx, http.StatusInternalServerError, err)
+		}
 		return
 	}
 	if needsClose {

services/context/context_request.go

@@ -30,3 +30,7 @@ func (ctx *Context) UploadStream() (rd io.ReadCloser, needToClose bool, err erro
 	}
 	return ctx.Req.Body, false, nil
 }
+
+func IsFormError(err error) bool {
+	return err == http.ErrMissingFile || err == http.ErrMissingBoundary
+}

tests/integration/api_packages_alpine_test.go

@@ -91,6 +91,11 @@ Djfa/2q5bH4699v++uMAAAAAAAAAAAAAAAAAAAAAAHbgA/eXQh8AKAAA`
 						AddBasicAuth(user.Name)
 					MakeRequest(t, req, http.StatusBadRequest)
 
+					req = NewRequestWithBody(t, "PUT", uploadURL, bytes.NewReader(content)).
+						AddBasicAuth(user.Name).
+						SetHeader("content-type", "multipart/form-data")
+					MakeRequest(t, req, http.StatusBadRequest)
+
 					req = NewRequestWithBody(t, "PUT", uploadURL, bytes.NewReader(content)).
 						AddBasicAuth(user.Name)
 					MakeRequest(t, req, http.StatusCreated)

tests/integration/api_packages_alt_test.go

@@ -106,6 +106,11 @@ enabled=1`,
 				req := NewRequestWithBody(t, "PUT", url, bytes.NewReader(content))
 				MakeRequest(t, req, http.StatusUnauthorized)
 
+				req = NewRequestWithBody(t, "PUT", url, bytes.NewReader(content)).
+					AddBasicAuth(user.Name).
+					SetHeader("content-type", "multipart/form-data")
+				MakeRequest(t, req, http.StatusBadRequest)
+
 				req = NewRequestWithBody(t, "PUT", url, bytes.NewReader(content)).
 					AddBasicAuth(user.Name)
 				MakeRequest(t, req, http.StatusCreated)

tests/integration/api_packages_arch_test.go

@@ -142,6 +142,10 @@ HMhNSS1IzUsBcpJAPFAwwUXSM0u4BjoaR8EoGAWjgGQAAILFeyQADAAA
 
 			req := NewRequestWithBody(t, "PUT", groupURL, bytes.NewReader(pkgs["any"]))
 			MakeRequest(t, req, http.StatusUnauthorized)
+			req = NewRequestWithBody(t, "PUT", groupURL, bytes.NewReader(pkgs["any"])).
+				AddBasicAuth(user.Name).
+				SetHeader("content-type", "multipart/form-data")
+			MakeRequest(t, req, http.StatusBadRequest)
 
 			req = NewRequestWithBody(t, "PUT", groupURL, bytes.NewReader(pkgs["any"])).
 				AddBasicAuth(user.Name)

tests/integration/api_packages_conan_test.go

@@ -117,6 +117,11 @@ func uploadConanPackageV1(t *testing.T, baseURL, token, name, version, user, cha
 	uploadURL := uploadURLs[conanfileName]
 	assert.NotEmpty(t, uploadURL)
 
+	req = NewRequestWithBody(t, "PUT", uploadURL, strings.NewReader(contentConanfile)).
+		AddTokenAuth(token).
+		SetHeader("content-type", "multipart/form-data")
+	MakeRequest(t, req, http.StatusBadRequest)
+
 	req = NewRequestWithBody(t, "PUT", uploadURL, strings.NewReader(contentConanfile)).
 		AddTokenAuth(token)
 	MakeRequest(t, req, http.StatusCreated)

tests/integration/api_packages_conda_test.go

@@ -67,6 +67,11 @@ func TestPackageConda(t *testing.T) {
 			req := NewRequestWithBody(t, "PUT", root+"/"+filename, bytes.NewReader(buf.Bytes()))
 			MakeRequest(t, req, http.StatusUnauthorized)
 
+			req = NewRequestWithBody(t, "PUT", root+"/"+filename, bytes.NewReader(buf.Bytes())).
+				AddBasicAuth(user.Name).
+				SetHeader("content-type", "multipart/form-data")
+			MakeRequest(t, req, http.StatusBadRequest)
+
 			req = NewRequestWithBody(t, "PUT", root+"/"+filename, bytes.NewReader(buf.Bytes())).
 				AddBasicAuth(user.Name)
 			MakeRequest(t, req, http.StatusCreated)
@@ -108,6 +113,11 @@ func TestPackageConda(t *testing.T) {
 			req := NewRequestWithBody(t, "PUT", root+"/"+channel+"/"+filename, bytes.NewReader(buf.Bytes()))
 			MakeRequest(t, req, http.StatusUnauthorized)
 
+			req = NewRequestWithBody(t, "PUT", root+"/"+channel+"/"+filename, bytes.NewReader(buf.Bytes())).
+				AddBasicAuth(user.Name).
+				SetHeader("content-type", "multipart/form-data")
+			MakeRequest(t, req, http.StatusBadRequest)
+
 			req = NewRequestWithBody(t, "PUT", root+"/"+channel+"/"+filename, bytes.NewReader(buf.Bytes())).
 				AddBasicAuth(user.Name)
 			MakeRequest(t, req, http.StatusCreated)

tests/integration/api_packages_cran_test.go

@@ -78,6 +78,12 @@ func TestPackageCran(t *testing.T) {
 			)).AddBasicAuth(user.Name)
 			MakeRequest(t, req, http.StatusBadRequest)
 
+			req = NewRequestWithBody(t, "PUT", uploadURL, createArchive(
+				"package/DESCRIPTION",
+				createDescription(packageName, packageVersion),
+			)).AddBasicAuth(user.Name).SetHeader("content-type", "multipart/form-data")
+			MakeRequest(t, req, http.StatusBadRequest)
+
 			req = NewRequestWithBody(t, "PUT", uploadURL, createArchive(
 				"package/DESCRIPTION",
 				createDescription(packageName, packageVersion),

tests/integration/api_packages_debian_test.go

@@ -98,6 +98,11 @@ func TestPackageDebian(t *testing.T) {
 								AddBasicAuth(user.Name)
 							MakeRequest(t, req, http.StatusBadRequest)
 
+							req = NewRequestWithBody(t, "PUT", uploadURL, createArchive(packageName, packageVersion, architecture)).
+								AddBasicAuth(user.Name).
+								SetHeader("content-type", "multipart/form-data")
+							MakeRequest(t, req, http.StatusBadRequest)
+
 							req = NewRequestWithBody(t, "PUT", uploadURL, createArchive(packageName, packageVersion, architecture)).
 								AddBasicAuth(user.Name)
 							MakeRequest(t, req, http.StatusCreated)

tests/integration/api_packages_generic_test.go

@@ -37,6 +37,11 @@ func TestPackageGeneric(t *testing.T) {
 		defer tests.PrintCurrentTest(t)()
 
 		req := NewRequestWithBody(t, "PUT", url+"/"+filename, bytes.NewReader(content)).
+			AddBasicAuth(user.Name).
+			SetHeader("content-type", "multipart/form-data")
+		MakeRequest(t, req, http.StatusBadRequest)
+
+		req = NewRequestWithBody(t, "PUT", url+"/"+filename, bytes.NewReader(content)).
 			AddBasicAuth(user.Name)
 		MakeRequest(t, req, http.StatusCreated)
 

tests/integration/api_packages_goproxy_test.go

@@ -61,6 +61,11 @@ func TestPackageGo(t *testing.T) {
 			packageName + "@" + packageVersion + "/go.mod": []byte(goModContent),
 		})
 
+		req = NewRequestWithBody(t, "PUT", url+"/upload", bytes.NewReader(content)).
+			AddBasicAuth(user.Name).
+			SetHeader("content-type", "multipart/form-data")
+		MakeRequest(t, req, http.StatusBadRequest)
+
 		req = NewRequestWithBody(t, "PUT", url+"/upload", bytes.NewReader(content)).
 			AddBasicAuth(user.Name)
 		MakeRequest(t, req, http.StatusCreated)

tests/integration/api_packages_helm_test.go

@@ -94,6 +94,11 @@ dependencies:
 		require.NoError(t, err)
 		assert.Equal(t, int64(len(content)), pb.Size)
 
+		req = NewRequestWithBody(t, "POST", uploadURL, bytes.NewReader(content)).
+			AddBasicAuth(user.Name).
+			SetHeader("content-type", "multipart/form-data")
+		MakeRequest(t, req, http.StatusBadRequest)
+
 		req = NewRequestWithBody(t, "POST", uploadURL, bytes.NewReader(content)).
 			AddBasicAuth(user.Name)
 		MakeRequest(t, req, http.StatusCreated)

tests/integration/api_packages_nuget_test.go

@@ -276,6 +276,11 @@ func TestPackageNuGet(t *testing.T) {
 			defer tests.PrintCurrentTest(t)()
 
 			req := NewRequestWithBody(t, "PUT", url, bytes.NewReader(content)).
+				AddBasicAuth(user.Name).
+				SetHeader("content-type", "multipart/form-data")
+			MakeRequest(t, req, http.StatusBadRequest)
+
+			req = NewRequestWithBody(t, "PUT", url, bytes.NewReader(content)).
 				AddBasicAuth(user.Name)
 			MakeRequest(t, req, http.StatusCreated)
 

tests/integration/api_packages_rpm_test.go

@@ -124,6 +124,11 @@ gpgkey=%sapi/packages/%s/rpm/repository.key`,
 				req := NewRequestWithBody(t, "PUT", url, bytes.NewReader(content))
 				MakeRequest(t, req, http.StatusUnauthorized)
 
+				req = NewRequestWithBody(t, "PUT", url, bytes.NewReader(content)).
+					AddBasicAuth(user.Name).
+					SetHeader("content-type", "multipart/form-data")
+				MakeRequest(t, req, http.StatusBadRequest)
+
 				req = NewRequestWithBody(t, "PUT", url, bytes.NewReader(content)).
 					AddBasicAuth(user.Name)
 				MakeRequest(t, req, http.StatusCreated)

tests/integration/api_packages_rubygems_test.go

@@ -369,6 +369,15 @@ AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA==`)
 		assert.Equal(t, "ruby", pd.Metadata.(*rubygems.Metadata).Platform)
 	})
 
+	t.Run("UploadBadRequest", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+
+		req := NewRequestWithBody(t, "POST", fmt.Sprintf("%s/api/v1/gems", root), bytes.NewReader(gemContent)).
+			AddBasicAuth(user.Name).
+			SetHeader("content-type", "multipart/form-data")
+		MakeRequest(t, req, http.StatusBadRequest)
+	})
+
 	t.Run("UploadExists", func(t *testing.T) {
 		defer tests.PrintCurrentTest(t)()
 

tests/integration/api_packages_vagrant_test.go

@@ -83,6 +83,11 @@ func TestPackageVagrant(t *testing.T) {
 		req = NewRequestWithBody(t, "PUT", uploadURL, bytes.NewReader(content))
 		MakeRequest(t, req, http.StatusUnauthorized)
 
+		req = NewRequestWithBody(t, "PUT", uploadURL, bytes.NewReader(content)).
+			AddBasicAuth(user.Name).
+			SetHeader("content-type", "multipart/form-data")
+		MakeRequest(t, req, http.StatusBadRequest)
+
 		req = NewRequestWithBody(t, "PUT", uploadURL, bytes.NewReader(content)).
 			AddTokenAuth(token)
 		MakeRequest(t, req, http.StatusCreated)