From 3158bb5b57cb2c2eb007e99c54a21f070edcafeb Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Sat, 10 Oct 2026 16:42:28 +0200 Subject: [PATCH] fix(16.2): WR-01 check the declared save before a field upload The field upload route ran only protect, so the create form of a controller without the create toolbar button could still write files to the public bucket. It now refuses with 403 where the save route would: create for {id} 0, update otherwise (operationDeclared). --- docs/backend/field-types.md | 2 +- modules/cabana/README.md | 2 +- modules/cabana/field_type_smoke_test.go | 4 ++- modules/cabana/field_type_test.go | 36 ++++++++++++++++++++++++- modules/cabana/field_type_upload.go | 10 +++++++ 5 files changed, 50 insertions(+), 4 deletions(-) diff --git a/docs/backend/field-types.md b/docs/backend/field-types.md index 03b30c0..f83550e 100644 --- a/docs/backend/field-types.md +++ b/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 /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 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 /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. ## The element diff --git a/modules/cabana/README.md b/modules/cabana/README.md index ca94cc5..cc86c87 100644 --- a/modules/cabana/README.md +++ b/modules/cabana/README.md @@ -52,7 +52,7 @@ All paths are relative to `/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). 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`. | | 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. | diff --git a/modules/cabana/field_type_smoke_test.go b/modules/cabana/field_type_smoke_test.go index 60a0b99..6aba497 100644 --- a/modules/cabana/field_type_smoke_test.go +++ b/modules/cabana/field_type_smoke_test.go @@ -401,9 +401,11 @@ func ftNotesMLType() pact.FormFieldType { const ftBodyField = " body:\n type: notesml\n label: Body\n" -// ftMLPlugin has both types, a translatable body field and typed columns. +// ftMLPlugin has both types, a translatable body field, typed columns and +// the create toolbar button (the create form may upload). func ftMLPlugin() ftPlugin { fsys := ftFS(ftNotesField, ftBodyField) + fsys["controllers/notes/config_list.yaml"] = &fstest.MapFile{Data: []byte(ftListConfig + "toolbar:\n buttons: [create]\n")} fsys["models/note/columns.yaml"] = &fstest.MapFile{Data: []byte("columns:\n title:\n label: Title\n notes:\n label: Notes\n type: notes\n body:\n label: Body\n type: notesml\n")} return ftPlugin{id: "acme.demo", fsys: fsys, types: []pact.FormFieldType{ftNotesType(), ftNotesMLType()}} } diff --git a/modules/cabana/field_type_test.go b/modules/cabana/field_type_test.go index a62d204..5ec3fec 100644 --- a/modules/cabana/field_type_test.go +++ b/modules/cabana/field_type_test.go @@ -1225,9 +1225,16 @@ func TestFieldUpload(t *testing.T) { t.Fatal(err) } // upService compiles the notes controller with the notes field given. + // The notes list declares the create toolbar button, so the create form + // may upload (WR-01). + upFS := func(notesField string) fstest.MapFS { + fsys := ftFS(notesField, ftBodyField) + fsys["controllers/notes/config_list.yaml"] = &fstest.MapFile{Data: []byte(ftListConfig + "toolbar:\n buttons: [create]\n")} + return fsys + } upService := func(t *testing.T, notesField string, types ...pact.FormFieldType) *service { t.Helper() - reg, err := ftCompile(ftPlugin{id: "acme.demo", fsys: ftFS(notesField, ftBodyField), types: types}) + reg, err := ftCompile(ftPlugin{id: "acme.demo", fsys: upFS(notesField), types: types}) if err != nil { t.Fatalf("compile: %v", err) } @@ -1256,6 +1263,33 @@ func TestFieldUpload(t *testing.T) { } }) + // WR-01: the upload route refuses where the save route would. + t.Run("403 for the create form of a controller that declares no create", 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) + } + reg, err := ftCompile(ftPlugin{id: "acme.demo", fsys: ftFS(ftNotesField, ftBodyField), types: []pact.FormFieldType{counting, ftNotesMLType()}}) + if err != nil { + t.Fatalf("compile: %v", err) + } + cc, ok := reg.Get("acme.demo.notes") + if !ok || cc.operationDeclared("create") { + t.Fatalf("fixture: the notes controller must not declare create") + } + noCreate := &service{app: app, reg: reg, uploadBytes: 4096} + rec := upCall(t, noCreate, "0", "notes", file, admin) + if rec.Code != http.StatusForbidden || calls != 0 { + t.Fatalf("status=%d calls=%d body=%s", rec.Code, calls, rec.Body.String()) + } + if rec := upCall(t, svc, "0", "notes", file, admin); rec.Code != http.StatusCreated { + t.Fatalf("with the create button: status=%d body=%s", rec.Code, rec.Body.String()) + } + }) + t.Run("404 for a core field, a type without Upload, an unknown field and a context refusal", func(t *testing.T) { for _, field := range []string{"title", "body", "missing"} { if rec := upCall(t, svc, "0", field, file, admin); rec.Code != http.StatusNotFound { diff --git a/modules/cabana/field_type_upload.go b/modules/cabana/field_type_upload.go index 0837ec1..b8c7b51 100644 --- a/modules/cabana/field_type_upload.go +++ b/modules/cabana/field_type_upload.go @@ -16,6 +16,8 @@ import ( // an element of a plugin field type uploads, stored by the type's Upload. // cabana owns the route: the backend guard and the X-Requested-With check // (mounted with requireAjax), the controller's permissions (protect), the +// operation the form would save (create for {id} 0, update otherwise), which +// the controller must declare as the save routes require (403 otherwise), the // 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 @@ -29,6 +31,14 @@ func (s *service) fieldUpload(w http.ResponseWriter, r *http.Request) { writeCRUDError(w, err) return } + // The upload belongs to a save: refuse it where the save route would. + op := "update" + if id == 0 { + op = "create" + } + if !s.operationDeclared(w, r, cc, op) { + return + } field, ok := uploadField(cc, r.PathValue("field"), id) if !ok { writeNotFound(w, r)