fix(16.2): WR-02 check the whole upload body before Upload stores it

The route called the type's Upload on the streamed part and only then
checked that no second part followed, so a refused request left a
public, unreferenced object in the bucket. It now reads the multipart
body whole (ReadForm, spooling past 8 MiB to a temporary file), checks
it holds exactly one file part named file, and only then calls Upload.
The remaining window (a failed record transaction after Upload, an
element that stopped waiting) is documented.

docs/backend/field-types.md

@@ -118,7 +118,7 @@ A column in `columns.yaml` may name a plugin alias as its `type`. The list then
 
 ## Uploads
 
-A type that stores files (images in a document, say) sets `Upload`. The element never uploads anything itself: it asks the admin, which posts the file with the administrator's session to `POST <prefix>/api/v1/{vendor}/{plugin}/{controller}/{id}/fields/{field}/upload` as one multipart part named `file`. cabana checks the backend session, the `X-Requested-With` header, the controller's permissions, that the controller declares the save the upload belongs to (create for `{id}` 0, which needs the list's `create` toolbar button, update otherwise; a 403 otherwise, as the save routes answer), that the field has a plugin type with `Upload` and that its `context` allows the operation, then caps the body at `http.body_limits.upload_bytes`. `{id}` 0 is the create form; any other id loads the record through the controller's `pact.FormExtendQuery` scope inside a transaction, and a record outside it is a 404. `Upload` receives a `pact.FormFieldUpload` with the transaction, the record (nil on the create form), the field, its options, the file name and the capped body, and returns the URL the element shows. The route answers 201 `{"data": {"url": "..."}}`; a `pact.FormFieldError` is a 422 on the field, a body over the cap a 413, and any other error the opaque 500. Checking the file (its size, its real type) is the type's job.
+A type that stores files (images in a document, say) sets `Upload`. The element never uploads anything itself: it asks the admin, which posts the file with the administrator's session to `POST <prefix>/api/v1/{vendor}/{plugin}/{controller}/{id}/fields/{field}/upload` as one multipart part named `file`. cabana checks the backend session, the `X-Requested-With` header, the controller's permissions, that the controller declares the save the upload belongs to (create for `{id}` 0, which needs the list's `create` toolbar button, update otherwise; a 403 otherwise, as the save routes answer), that the field has a plugin type with `Upload` and that its `context` allows the operation, then caps the body at `http.body_limits.upload_bytes`. It reads the whole body and checks that it holds exactly that one part before it calls `Upload` (a large file is spooled to a temporary file), so a malformed or oversized request never stores anything. A stored file can still be left unreferenced when the record's transaction fails after `Upload` returned or the element stops waiting for the answer, so `Upload` should store under a name that is safe to leave behind. `{id}` 0 is the create form; any other id loads the record through the controller's `pact.FormExtendQuery` scope inside a transaction, and a record outside it is a 404. `Upload` receives a `pact.FormFieldUpload` with the transaction, the record (nil on the create form), the field, its options, the file name and the capped body, and returns the URL the element shows. The route answers 201 `{"data": {"url": "..."}}`; a `pact.FormFieldError` is a 422 on the field, a body over the cap a 413, and any other error the opaque 500. Checking the file (its size, its real type) is the type's job.
 
 ## The element
 

modules/cabana/README.md

@@ -52,7 +52,7 @@ All paths are relative to `<prefix>/api/v1`. A controller ID `vendor.plugin.cont
 | POST and DELETE `/account/avatar` | Upload the signed-in administrator's avatar (one multipart `file_data` part: a jpg, jpeg, png, gif or webp image of at most 2 MiB; anything else is a 422 on `avatar`, a body over the cap a 413) replacing the previous one, or remove it (also 200 when there is none). Both answer the `cabana.AdminProfile`. Needs any backend session. |
 | GET `/navigation`, GET `/settings` | Navigation and settings entries the administrator may open; a settings entry's `controller` names the admin controller a link entry opens and is empty for a singleton form. |
 | GET `/field-types` | The plugin field types (`pact.HasFormFieldTypes`) sorted by name, each with its `element`, `translatable` flag and the versioned URLs of its `scripts` and `styles` (`cabana.FieldTypesResponse`); an empty list when no plugin registers one. The SPA loads them after sign-in. Needs any backend session. |
-| POST `/{vendor}/{plugin}/{controller}/{id}/fields/{field}/upload` | Store one multipart part named `file` through the `Upload` of the field's plugin field type and answer 201 `{url}` (`cabana.FieldUploadResponse`). `{id}` 0 is the create form and passes no record; any other id loads the record through the form scope (404 outside it). 403 when the controller does not declare the save the upload belongs to (create for `{id}` 0, which needs the list's `create` toolbar button, update otherwise), as the save routes answer. 404 for a field without a plugin type with `Upload` or whose context does not allow the operation, 413 over `http.body_limits.upload_bytes`, 422 on the field for a malformed body or a `pact.FormFieldError`. |
+| POST `/{vendor}/{plugin}/{controller}/{id}/fields/{field}/upload` | Store one multipart part named `file` through the `Upload` of the field's plugin field type and answer 201 `{url}` (`cabana.FieldUploadResponse`). `{id}` 0 is the create form and passes no record; any other id loads the record through the form scope (404 outside it). 403 when the controller does not declare the save the upload belongs to (create for `{id}` 0, which needs the list's `create` toolbar button, update otherwise), as the save routes answer. 404 for a field without a plugin type with `Upload` or whose context does not allow the operation, 413 over `http.body_limits.upload_bytes`, 422 on the field for a malformed body or a `pact.FormFieldError`. The body is read and checked whole before `Upload` runs, so a refused request stores nothing. |
 | GET `/settings/{code}/schema`, GET and PUT `/settings/{code}` | Settings form schema, values and update. |
 | POST `/markdown/preview` | Render `{markdown}` through `cabana.RenderMarkdown` for the form preview and answer `{html}`; a refused output is a 422 `validation_failed` on `markdown`. Needs any backend session. |
 | GET `/{vendor}/{plugin}/{controller}/schema/list`, `.../schema/form`, `.../schema/relation/{name}` | Localized list, form and relation schemas. |

modules/cabana/field_type_test.go

@@ -1345,6 +1345,57 @@ func TestFieldUpload(t *testing.T) {
 		}
 	})
 
+	// WR-02: the body is checked whole before Upload runs, so a refused
+	// request never leaves a stored object behind.
+	t.Run("a refused body never reaches Upload", func(t *testing.T) {
+		calls := 0
+		counting := ftNotesType()
+		upload := counting.Upload
+		counting.Upload = func(ctx context.Context, in pact.FormFieldUpload) (pact.FormFieldUploadResult, error) {
+			calls++
+			return upload(ctx, in)
+		}
+		strict := upService(t, ftNotesField, counting, ftNotesMLType())
+		for name, parts := range map[string][]upPart{
+			"a second file part":         {{"file", "a.txt", []byte("x")}, {"file", "b.txt", []byte("y")}},
+			"a file then another field":  {{"file", "a.txt", []byte("x")}, {"other", "o.txt", []byte("y")}},
+			"a file then a value part":   {{"file", "a.txt", []byte("x")}, {"note", "", []byte("y")}},
+			"a value part before a file": {{"note", "", []byte("y")}, {"file", "a.txt", []byte("x")}},
+		} {
+			rec := upCall(t, strict, "0", "notes", parts, admin)
+			if rec.Code != http.StatusUnprocessableEntity || calls != 0 {
+				t.Fatalf("%s: status=%d calls=%d body=%s", name, rec.Code, calls, rec.Body.String())
+			}
+		}
+		rec := upCall(t, strict, "0", "notes", []upPart{{"file", "a.txt", bytes.Repeat([]byte("x"), 8192)}}, admin)
+		if rec.Code != http.StatusRequestEntityTooLarge || calls != 0 {
+			t.Fatalf("over the cap: status=%d calls=%d", rec.Code, calls)
+		}
+		if rec := upCall(t, strict, "0", "notes", file, admin); rec.Code != http.StatusCreated || calls != 1 {
+			t.Fatalf("one file: status=%d calls=%d body=%s", rec.Code, calls, rec.Body.String())
+		}
+	})
+
+	t.Run("a file past the in-memory part reaches Upload whole", func(t *testing.T) {
+		var got []byte
+		reading := ftNotesType()
+		reading.Upload = func(_ context.Context, in pact.FormFieldUpload) (pact.FormFieldUploadResult, error) {
+			var err error
+			got, err = io.ReadAll(in.Body)
+			return pact.FormFieldUploadResult{URL: "/media/" + in.Filename}, err
+		}
+		reg, err := ftCompile(ftPlugin{id: "acme.demo", fsys: upFS(ftNotesField), types: []pact.FormFieldType{reading, ftNotesMLType()}})
+		if err != nil {
+			t.Fatalf("compile: %v", err)
+		}
+		large := bytes.Repeat([]byte("0123456789abcdef"), (fieldUploadMemory+4096)/16)
+		big := &service{app: app, reg: reg, uploadBytes: int64(len(large)) + 64<<10}
+		rec := upCall(t, big, "0", "notes", []upPart{{"file", "big.bin", large}}, admin)
+		if rec.Code != http.StatusCreated || !bytes.Equal(got, large) {
+			t.Fatalf("status=%d read %d of %d bytes body=%s", rec.Code, len(got), len(large), rec.Body.String())
+		}
+	})
+
 	t.Run("any other Upload error is an opaque 500", func(t *testing.T) {
 		failing := ftNotesType()
 		failing.Upload = func(context.Context, pact.FormFieldUpload) (pact.FormFieldUploadResult, error) {

modules/cabana/field_type_upload.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"io"
+	"mime/multipart"
 	"net/http"
 	"strings"
 
@@ -21,7 +22,11 @@ import (
 // field and its context, the record scope and the body cap. {id} 0 is the
 // create form and passes no record; any other id loads the record through
 // the controller's FormExtendQuery scope inside a transaction, and a record
-// outside it is 404. The body is one multipart part named file. It answers
+// outside it is 404. The body is one multipart part named file; it is read
+// and checked whole before Upload runs, so a malformed or oversized body
+// never reaches Upload. A stored object can still be left unreferenced when
+// the record transaction fails after Upload or the element stops waiting;
+// Upload stores under a name that is safe to leave behind. It answers
 // 201 {data: {url}}; a pact.FormFieldError is a 422 on the field, any other
 // Upload error the opaque 500.
 func (s *service) fieldUpload(w http.ResponseWriter, r *http.Request) {
@@ -56,34 +61,38 @@ func (s *service) fieldUpload(w http.ResponseWriter, r *http.Request) {
 			writeCRUDError(w, uploadInvalid(field.Name))
 			return
 		}
-		part, err := mr.NextPart()
+		// The whole body is read and checked before Upload runs, so a
+		// refused body never leaves a stored object behind (WR-02): a file
+		// past fieldUploadMemory is spooled to a temporary file.
+		form, err := mr.ReadForm(fieldUploadMemory)
 		if err != nil {
 			s.writeFieldUploadError(w, body, field, uploadInvalid(field.Name))
 			return
 		}
-		if part.FormName() != "file" || strings.TrimSpace(part.FileName()) == "" {
+		defer func() { _ = form.RemoveAll() }()
+		header, ok := singleUploadFile(form)
+		if !ok {
 			writeCRUDError(w, uploadInvalid(field.Name))
 			return
 		}
+		file, err := header.Open()
+		if err != nil {
+			WriteError(w, http.StatusInternalServerError, "error", msgServerError)
+			return
+		}
+		defer func() { _ = file.Close() }()
 		run := func(ctx context.Context, tx *gorm.DB, record any) (pact.FormFieldUploadResult, error) {
 			out, err := field.fieldType.def.Upload(ctx, pact.FormFieldUpload{
 				DB:       tx,
 				Record:   record,
 				Field:    field.Name,
 				Options:  copyJSONMap(field.TypeOptions),
-				Filename: part.FileName(),
-				Body:     part,
+				Filename: header.Filename,
+				Body:     file,
 			})
 			if err != nil {
-				if tooLarge(body) {
-					return pact.FormFieldUploadResult{}, body.err
-				}
 				return pact.FormFieldUploadResult{}, fieldTypeFailure(ctx, s.translator(), cc, field, "upload", err)
 			}
-			// Exactly one part: anything after file is refused.
-			if _, err := mr.NextPart(); !errors.Is(err, io.EOF) {
-				return pact.FormFieldUploadResult{}, uploadInvalid(field.Name)
-			}
 			return out, nil
 		}
 		var result pact.FormFieldUploadResult
@@ -138,6 +147,23 @@ func uploadField(cc *CompiledController, name string, id uint) (FormField, bool)
 	return FormField{}, false
 }
 
+// fieldUploadMemory is how much of a field upload's file is kept in memory;
+// the rest is spooled to a temporary file until the request ends.
+const fieldUploadMemory = 8 << 20
+
+// singleUploadFile is the one file of a field upload body: exactly one part,
+// a file part named file with a non-blank file name.
+func singleUploadFile(form *multipart.Form) (*multipart.FileHeader, bool) {
+	if form == nil || len(form.Value) != 0 || len(form.File) != 1 {
+		return nil, false
+	}
+	files := form.File["file"]
+	if len(files) != 1 || files[0] == nil || strings.TrimSpace(files[0].Filename) == "" {
+		return nil, false
+	}
+	return files[0], true
+}
+
 // fieldUploadCap is the body cap of the field upload route:
 // http.body_limits.upload_bytes, or the default upload cap.
 func (s *service) fieldUploadCap() int64 {