From 44835e4c6b44a6826e97bb65c47892f58e376e74 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Fri, 23 Oct 2026 16:24:34 +0200 Subject: [PATCH] fix(cabana): safe account action URLs, per-section isolation and the prompted record action contract (B-WR-03, B-WR-05, B-WR-06) - an account action URL is kept only when it is http(s) with a host or a same-origin path; anything else is dropped and logged without the URL - a failing Status marks its section error: true and a failing Applies drops only that action, instead of a 500 for the whole list - status text goes through localizeDetail (B-IN-03); the non-string payload refusal is the phrase key action_prompt_invalid (B-IN-02) - the record action annotations and pact's prompt doc say what happens (B-IN-05 text); admin.json and schema.d.ts regenerated --- admin/openapi/admin.json | 14 ++++--- admin/src/api/schema.d.ts | 17 +++++--- admin/tests/fixtures/lang.json | 3 ++ docs/backend/users-and-permissions.md | 2 +- modules/cabana/README.md | 8 ++-- modules/cabana/account_contributions.go | 40 ++++++++++++++++--- modules/cabana/actions.go | 3 +- modules/cabana/admin_openapi.go | 15 ++++--- .../record_action_prompt_postgres_test.go | 2 +- modules/pact/capabilities.go | 3 +- modules/phrasebook/backend/lang/en/lang.yaml | 1 + modules/phrasebook/backend/lang/pl/lang.yaml | 1 + 12 files changed, 80 insertions(+), 29 deletions(-) diff --git a/admin/openapi/admin.json b/admin/openapi/admin.json index 4e9fe562..771d7634 100644 --- a/admin/openapi/admin.json +++ b/admin/openapi/admin.json @@ -50,6 +50,10 @@ "description": { "type": "string" }, + "error": { + "description": "Error is true when the section's status could not be loaded (the\nplugin's Status failed); the other sections are answered as usual.\nAbsent otherwise.", + "type": "boolean" + }, "label": { "type": "string" }, @@ -57,7 +61,7 @@ "type": "string" }, "status": { - "description": "Status is the section's line for the signed-in admin; \"\" when the\nsection has none.", + "description": "Status is the section's line for the signed-in admin; \"\" when the\nsection has none or its status could not be loaded.", "type": "string" } }, @@ -247,7 +251,7 @@ "cabana.AdminActionRequest": { "properties": { "payload": { - "description": "Payload is the widget's own JSON value (the summer-action event's\ndetail.payload): any JSON, at most 64 KiB, handed to the action as-is.\nToolbar and record routes refuse it." + "description": "Payload is the widget's own JSON value (the summer-action event's\ndetail.payload): any JSON, at most 64 KiB, handed to the action as-is.\nToolbar routes refuse it; record routes accept it only for a prompted\naction, as a JSON string (the prompt's answer)." }, "record_id": { "type": "integer" @@ -5934,7 +5938,7 @@ }, "/{vendor}/{plugin}/{controller}/toolbar/{action}": { "post": { - "description": "Runs a controller-registered action that the list's toolbar.buttons declares. The body must be {}: record_id, values and payload are all refused, and the answer's fill is always empty.", + "description": "Runs a controller-registered action that the list's toolbar.buttons declares. The body must be {}: the route refuses record_id, values and payload, and the answer's fill is always empty.", "parameters": [ { "description": "Vendor", @@ -6480,7 +6484,7 @@ }, "/{vendor}/{plugin}/{controller}/{id}/actions/{action}": { "post": { - "description": "Runs a record action the controller registers and the form's recordActions declares. The record is loaded and row-locked through the controller's form scope in one transaction (404 when missing or out of scope); an action that does not apply to the record's current state answers 409. The body must be {}: record_id, values and payload are all refused, and the answer's fill is always empty.", + "description": "Runs a record action the controller registers and the form's recordActions declares. The record is loaded and row-locked through the controller's form scope in one transaction (404 when missing or out of scope); an action that does not apply to the record's current state answers 409. The body is {} for an action without a prompt; an action whose record response carries prompt takes {\"payload\": \"\u003ctext\u003e\"}, trimmed, non-empty and at most prompt.max_length runes, with a 422 on payload otherwise. record_id and values are always refused, and the answer's fill is always empty.", "parameters": [ { "description": "Vendor", @@ -6536,7 +6540,7 @@ } } }, - "description": "Empty object", + "description": "Empty object, or {payload: text} for a prompted action", "required": true }, "responses": { diff --git a/admin/src/api/schema.d.ts b/admin/src/api/schema.d.ts index 44d8b4fa..801496e8 100644 --- a/admin/src/api/schema.d.ts +++ b/admin/src/api/schema.d.ts @@ -2636,7 +2636,7 @@ export interface paths { put?: never; /** * Run a toolbar action - * @description Runs a controller-registered action that the list's toolbar.buttons declares. The body must be {}: record_id, values and payload are all refused, and the answer's fill is always empty. + * @description Runs a controller-registered action that the list's toolbar.buttons declares. The body must be {}: the route refuses record_id, values and payload, and the answer's fill is always empty. */ post: { parameters: { @@ -3030,7 +3030,7 @@ export interface paths { put?: never; /** * Run a declared record action - * @description Runs a record action the controller registers and the form's recordActions declares. The record is loaded and row-locked through the controller's form scope in one transaction (404 when missing or out of scope); an action that does not apply to the record's current state answers 409. The body must be {}: record_id, values and payload are all refused, and the answer's fill is always empty. + * @description Runs a record action the controller registers and the form's recordActions declares. The record is loaded and row-locked through the controller's form scope in one transaction (404 when missing or out of scope); an action that does not apply to the record's current state answers 409. The body is {} for an action without a prompt; an action whose record response carries prompt takes {"payload": ""}, trimmed, non-empty and at most prompt.max_length runes, with a 422 on payload otherwise. record_id and values are always refused, and the answer's fill is always empty. */ post: { parameters: { @@ -3050,7 +3050,7 @@ export interface paths { }; cookie?: never; }; - /** @description Empty object */ + /** @description Empty object, or {payload: text} for a prompted action */ requestBody: { content: { "application/json": components["schemas"]["cabana.AdminActionRequest"]; @@ -5521,11 +5521,17 @@ export interface components { "cabana.AccountContributionView": { actions: components["schemas"]["cabana.AccountContributionActionView"][]; description: string; + /** + * @description Error is true when the section's status could not be loaded (the + * plugin's Status failed); the other sections are answered as usual. + * Absent otherwise. + */ + error?: boolean; label: string; name: string; /** * @description Status is the section's line for the signed-in admin; "" when the - * section has none. + * section has none or its status could not be loaded. */ status: string; }; @@ -5577,7 +5583,8 @@ export interface components { /** * @description Payload is the widget's own JSON value (the summer-action event's * detail.payload): any JSON, at most 64 KiB, handed to the action as-is. - * Toolbar and record routes refuse it. + * Toolbar routes refuse it; record routes accept it only for a prompted + * action, as a JSON string (the prompt's answer). */ payload?: unknown; record_id?: number; diff --git a/admin/tests/fixtures/lang.json b/admin/tests/fixtures/lang.json index 39cf46ce..e552f393 100644 --- a/admin/tests/fixtures/lang.json +++ b/admin/tests/fixtures/lang.json @@ -228,6 +228,9 @@ "backend::lang.extension.widget_failed": { "other": "Nie udało się wczytać tej kontrolki. Odśwież stronę, a jeśli błąd wróci, sprawdź zasoby wtyczki." }, + "backend::lang.form.action_prompt_invalid": { + "other": "Odpowiedź musi być tekstem." + }, "backend::lang.form.add": { "other": "Dodaj" }, diff --git a/docs/backend/users-and-permissions.md b/docs/backend/users-and-permissions.md index 6c3e0cf6..479bbef9 100644 --- a/docs/backend/users-and-permissions.md +++ b/docs/backend/users-and-permissions.md @@ -147,7 +147,7 @@ Every signed-in administrator can edit their own profile on the My account scree - **Preferences:** the theme (system, light or dark) and the admin language an administrator picks in the SPA are saved through `PUT /api/v1/account/preferences` and read with `GET /api/v1/account/preferences`; `GET .../auth/me` returns them as `preferences`. The body may hold only `theme` and `locale` (`cabana.AdminPreferencesRequest`); any other key, including `id`, answers 422 on that key and nothing is written. Each present key is saved and an absent key keeps its value, so `{}` answers the current preferences. `theme` must be `system`, `light` or `dark`; `locale` must be `""` (follow the request locale) or one of the admin locales `GET /lang` lists in `meta.locales`; another value or a non-string answers 422 on that field. They are stored under `admin_theme` and `admin_locale` in `backend_users.metadata`, next to the subtitle; the defaults (`system`, `""`) are stored as absent keys, every other key survives a save, and metadata that is not a JSON object is never overwritten (422). Like the rest of My account the routes act only on the token's own administrator and refuse personal API tokens. - **API tokens:** a card below the profile form (never inside it — the page Save does not touch tokens) lists, creates and revokes only the signed-in administrator's own tokens. Creating asks for a name, an expiry (`30d`, `90d`, `365d` or `never`) and scopes from `GET /account/tokens/scopes`; the plaintext is shown once in the dialog for copying, then cleared. Revoke asks for confirmation. A "Show revoked tokens" toggle reloads the list with revoked rows. See [API tokens](#api-tokens) for the wire contract and scopes. -- **Plugin sections:** a plugin can add its own card below the API tokens, for something only the signed-in administrator does for themselves, such as linking their site account. The plugin implements `pact.HasAccountContributions` and returns `pact.AccountContribution` entries: a name, a label and description, an optional status line computed for the administrator, and buttons (`pact.AccountAction`) with an optional confirmation (danger variant included), extra permissions and an `Applies` rule. The SPA loads them from `GET /api/v1/account/contributions` and runs a button through `POST /api/v1/account/contributions/{name}/actions/{action}`, which calls the action's `Run` with the signed-in administrator's id inside a transaction (`cabana.TxFromContext`) and shows the returned message; a returned URL is opened in a new tab and shown as a link. Both routes refuse personal API tokens like the rest of My account, and a page without contributing plugins is unchanged. +- **Plugin sections:** a plugin can add its own card below the API tokens, for something only the signed-in administrator does for themselves, such as linking their site account. The plugin implements `pact.HasAccountContributions` and returns `pact.AccountContribution` entries: a name, a label and description, an optional status line computed for the administrator, and buttons (`pact.AccountAction`) with an optional confirmation (danger variant included), extra permissions and an `Applies` rule. The SPA loads them from `GET /api/v1/account/contributions` and runs a button through `POST /api/v1/account/contributions/{name}/actions/{action}`, which calls the action's `Run` with the signed-in administrator's id inside a transaction (`cabana.TxFromContext`) and shows the returned message; a returned URL is opened in a new tab and shown as a link. Only an absolute `http`/`https` URL or a same-origin path is passed on; the admin drops anything else. A section whose status fails to load is still listed, marked `error: true`, and the other sections are unaffected. Both routes refuse personal API tokens like the rest of My account, and a page without contributing plugins is unchanged. Plugins that show an administrator, such as an author box, read the subtitle with `cabana.BackendUser.Subtitle` and the picture with `cabana.BackendUserAvatar`, which returns the avatar file (or nil) for an administrator id. diff --git a/modules/cabana/README.md b/modules/cabana/README.md index 6c29f5e2..49a94b22 100644 --- a/modules/cabana/README.md +++ b/modules/cabana/README.md @@ -58,8 +58,8 @@ Use cases: - Record actions on another plugin's controller: a plugin implementing `pact.HasAdminActionExtensions` adds `pact.AdminRecordAction` entries to any admin controller by its ID (`pact.AdminActionExtension`), its own, another plugin's or the admin's own Administrators screen (`cabana.AdministratorsControllerID`). They are merged at boot after the target's own record actions, in plugin activation order, offered on the target's preview screen and run through the same row-locked record action route; the target's `RequiredPermissions` and the action's own `Permissions` both apply. Contributions targeting `cabana.AdministratorsControllerID` run `guardOutranked` before `Run` (403 `user.outranked`, `Run` not called). An unknown controller ID, a target without a preview block, a name the target already registers (the owner's or an earlier contribution), a reserved or non-identifier name, a missing `Run` or an empty label stops the boot. Extensions are compiled only when the admin is active. - Administrators screen: when some plugin registers admin controllers, `cabana.Activate` also compiles the admin's own screens, an internal plugin with ID `summercms.cabana`. Settings → System → Administrators (controller `cabana.AdministratorsControllerID`, permission `cabana.PermissionManageUsers`) lists backend administrators with their role and last sign-in, opens a preview of each, where record actions (the admin's own and contributed ones) are offered, and creates and edits administrators: login, email, names, a password (hashed at `admin.password.bcrypt_cost`, at least 4 characters and at most 72 bytes), the role and per-administrator permission overrides. The server enforces the privilege rules in the save transaction: only a superuser sees superusers or sends the superuser switch, an administrator can neither grant permissions or roles above their own grants nor change an administrator who holds more than they do, their own role, superuser flag, permissions and password are not changed here, and the last active superuser cannot be demoted. A password set for another administrator ends their sessions (personal API tokens stay valid, as with `admin:reset-password`). Administrators are activated, deactivated and restored by record and bulk actions and deleted softly through `deleted_at` (never hard-deleted); the list shows deleted and deactivated rows with their row states and filters deleted ones, and the preview shows a status hint. Deactivating or deleting an administrator ends their sessions and stops their personal API tokens; the last active superuser cannot be demoted, deactivated or deleted on any of these paths (a transaction-scoped advisory lock serializes the check). The permission codes `cabana.PermissionManageUsers` and `cabana.PermissionManageRoles` are registered with no role grant, so after an upgrade only superusers see the screens until an administrator grants them. An application without admin controllers still activates no admin. - Danger record actions: `pact.AdminRecordAction.Danger` is served as `danger` on the record response's `cabana.RecordAction` (absent when false, so existing payloads are unchanged); the admin's confirm dialog then uses its danger variant. -- Prompted record actions: a `pact.AdminRecordAction` with a `Prompt` (`pact.AdminActionPrompt`) asks the administrator for one text value in its confirm dialog. The record response serves it as `prompt` on the action's `cabana.RecordAction` (absent for every other action, so existing payloads are unchanged), and the action route accepts the body key `payload`, a JSON string, only for such an action: trimmed, not empty and at most `max_length` runes (422 on `payload` with `backend::lang.form.action_prompt_required` or `action_prompt_too_long`, "The payload must be a string." for another JSON type), then handed to `Run` as `pact.AdminRecordActionInput.Value`. Every other record action keeps refusing `record_id`, `values` and `payload`. A prompt without a label fails boot, owned or contributed. -- My account contributions: a plugin implementing `pact.HasAccountContributions` adds sections (`pact.AccountContribution`) to the administrator's own account page, each with a status line and buttons (`pact.AccountAction`). GET `/account/contributions` lists them for the signed-in administrator and POST `/account/contributions/{name}/actions/{action}` runs one for that administrator only, behind the interactive-only guard, the action's permissions and its `Applies` rule, inside a transaction; the answer may carry a URL the admin opens in a new tab. Names are generic: cabana never names the contributing plugin. The existing `/account*` and `/auth/me` payloads are unchanged. A non-identifier or duplicate section name, an empty label, a duplicate or non-identifier action name, a nil `Run` or an empty action label stops the boot. +- Prompted record actions: a `pact.AdminRecordAction` with a `Prompt` (`pact.AdminActionPrompt`) asks the administrator for one text value in its confirm dialog. The record response serves it as `prompt` on the action's `cabana.RecordAction` (absent for every other action, so existing payloads are unchanged), and the action route accepts the body key `payload`, a JSON string, only for such an action: trimmed, not empty and at most `max_length` runes (422 on `payload` with `backend::lang.form.action_prompt_required` or `action_prompt_too_long`, `backend::lang.form.action_prompt_invalid` for another JSON type), then handed to `Run` as `pact.AdminRecordActionInput.Value`. Every other record action keeps refusing `record_id`, `values` and `payload`. A prompt without a label fails boot, owned or contributed. +- My account contributions: a plugin implementing `pact.HasAccountContributions` adds sections (`pact.AccountContribution`) to the administrator's own account page, each with a status line and buttons (`pact.AccountAction`). GET `/account/contributions` lists them for the signed-in administrator and POST `/account/contributions/{name}/actions/{action}` runs one for that administrator only, behind the interactive-only guard, the action's permissions and its `Applies` rule, inside a transaction; the answer may carry a URL the admin opens in a new tab. That URL is kept only when it is an absolute `http`/`https` URL with a host or a same-origin path (one leading `/`); anything else, `javascript:` and `data:` included, is dropped and logged without the URL. A section whose `Status` fails is listed with an empty status and `error: true`, and an action whose `Applies` fails is left out; both are logged and the other sections are answered as usual. A status is translated only when it is a phrase key. Names are generic: cabana never names the contributing plugin. The existing `/account*` and `/auth/me` payloads are unchanged. A non-identifier or duplicate section name, an empty label, a duplicate or non-identifier action name, a nil `Run` or an empty action label stops the boot. - Backend subject events: a save on the Administrators screen that changed an administrator's role, permission overrides or superuser flag, a Roles save that changed a role's grants (one event per administrator holding it), a deactivation or deletion (record and bulk forms) and a revoked personal API token fire `pact.MCPSubjectChanged` on the application's event bus with guard `backend`, so a module acting on behalf of an administrator elsewhere (the MCP endpoint) re-lists what they may do or ends their sessions. A revoked token's event carries its `TokenID`, so only the sessions opened with that token end; the administrator's other sessions stay open. A listener's error is logged and never fails the save. - Singleton settings screens declared with `pact.HasSettings`, read and saved by `cabana.SettingsService`. A `type: password` field on a settings form is a write-only secret on a `lagoon.Encrypted` column (any other column type fails boot naming the field): the GET omits it from `data` and reports `stored`, `env` or `empty` per field in `meta.secrets` (`cabana.SettingsMeta`; `env` when the column is empty and the model's `cabana.SecretFallbacks` config key resolves to a non-empty value); on PUT an absent key or an empty string keeps the stored value, JSON null clears it, a non-string is a 422 on the field, and the answer carries the new states, never a value. A `type: richeditor` on a settings form is sanitized on save exactly like a controller save, with the removals in `meta.notices`; `repeater` and `dependsOn` stay refused. Settings fields keep their `tab`. A `type: dropdown` whose `options` names a method takes its choices from the settings model, which must implement `pact.DropdownOptionsProvider` (boot fails naming the field otherwise); the settings schema serves them localized. A settings entry whose `Controller` field is set links to that admin controller instead: start-up fails when it also declares a model, form or model factory, or names an unknown controller; `GET /settings` lists it, with its `controller`, only to administrators who pass its permissions and may open the controller; and the singleton settings endpoints answer 404 for its code. - Backend navigation (`pact.HasNavigation`) and permissions (`pact.HasPermissions`), filtered per user by `cabana.Registry.Metadata`. An entry whose `pact.NavigationItem.Visible` func answers false for the request is left out like a denied one (a hidden main item with its side menu), and a main item whose controller is a side item hidden this way links to its first visible openable side item. An administrator's own `backend_users.permissions` are merged over the role's as in Winter (a `-1` denies a code the role grants). `cabana.Allows` implements the permission check with Winter's `hasAnyAccess` semantics for JWT sessions: superusers pass, a principal needs any one of the listed codes, and wildcards match on both sides (a grant ending in `.*` covers every code with that prefix, and a required code ending in `.*` is met by any grant under it). When the principal carries a non-full `bouncer.ScopeLimit` (a personal API token), the scope is checked first: one method-allowed scope must match every required code and the owner must hold a required code inside that scope; a superuser with a scoped token is limited to the scope, and an empty requirement list is refused unless the scope is full. @@ -87,8 +87,8 @@ Guarded routes run surf's `locale.from-principal` after the backend guard: an in | GET `/account/tokens`, POST `/account/tokens` | List the caller's personal API tokens (newest first; revoked only with `?revoked=1`) and create one (`cabana.AdminAPITokenRequest`: `name`, `expires`, `scopes`). Create answers 201 with `cabana.AdminAPITokenCreated` (plaintext `token` once). Interactive session only. | | DELETE `/account/tokens/{id}` | Soft-revoke one of the caller's own active tokens (`cabana.AdminAPIToken`); 404 otherwise. Interactive session only. | | GET `/account/tokens/scopes` | Scopes the caller may assign (`cabana.AdminAPITokenScope`), including `*`, `admin:read` and overlapping plugin scopes. Interactive session only. | -| GET `/account/contributions` | The sections plugins contribute to My account (`cabana.AccountContributionView`): translated label, description and status for the signed-in administrator, and the actions (`cabana.AccountContributionActionView`) they hold the permissions for and whose `Applies` reports true. An empty list when no plugin contributes. Interactive session only. | -| POST `/account/contributions/{name}/actions/{action}` | Run one contributed action for the signed-in administrator: 404 for an unknown section or action, 403 without the action's permissions, 409 when it no longer applies; `Run` gets the administrator's id inside a transaction and answers `cabana.AccountActionResultView` (translated `message`, optional `url`). Interactive session only, AJAX header required. | +| GET `/account/contributions` | The sections plugins contribute to My account (`cabana.AccountContributionView`): translated label, description and status for the signed-in administrator, and the actions (`cabana.AccountContributionActionView`) they hold the permissions for and whose `Applies` reports true. A section whose status failed carries `error: true`. An empty list when no plugin contributes. Interactive session only. | +| POST `/account/contributions/{name}/actions/{action}` | Run one contributed action for the signed-in administrator: 404 for an unknown section or action, 403 without the action's permissions, 409 when it no longer applies; `Run` gets the administrator's id inside a transaction and answers `cabana.AccountActionResultView` (translated `message`, optional `url`, only an `http`/`https` or same-origin URL). Interactive session only, AJAX header required. | | GET `/navigation`, GET `/settings` | Navigation and settings entries the administrator may open and, for a navigation entry with a `Visible` func, that it answers true for the request; 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`. The body is read and checked whole before `Upload` runs, so a refused request stores nothing. | diff --git a/modules/cabana/account_contributions.go b/modules/cabana/account_contributions.go index 2441056c..383c199d 100644 --- a/modules/cabana/account_contributions.go +++ b/modules/cabana/account_contributions.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "net/http" + "net/url" "strings" "git.golem15.com/golem15/summercms/modules/lagoon" @@ -89,7 +90,10 @@ func compileAccountContributions(reg *Registry, plugins []party.Plugin) error { // accountContributionsList serves GET /account/contributions: every // contributed section with its status for the signed-in administrator and -// the actions they may run that apply to them, labels translated. +// the actions they may run that apply to them, labels translated. A +// section whose Status fails is sent with Error set and no status, and an +// action whose Applies fails is left out, both logged: one plugin's error +// never answers 500 for the list. func (s *service) accountContributionsList(w http.ResponseWriter, r *http.Request) { principal := accountPrincipal(w, r) if principal == nil { @@ -107,13 +111,15 @@ func (s *service) accountContributionsList(w http.ResponseWriter, r *http.Reques Actions: []AccountContributionActionView{}, } if item.section.Status != nil { + // One plugin's failure never hides the other sections: the + // section is sent with an empty status and the error flag. status, err := item.section.Status(ctx, principal.ID) if err != nil { moduleLog.ErrorContext(ctx, "cabana: account contribution status failed", "section", item.section.Name, "plugin", item.pluginID, "error", err) - WriteError(w, http.StatusInternalServerError, "error", msgServerError) - return + view.Error = true + } else { + view.Status = localizeDetail(ctx, tr, status) } - view.Status = translateKey(ctx, tr, status) } for _, action := range item.section.Actions { if !Allows(principal, action.Permissions) { @@ -122,9 +128,10 @@ func (s *service) accountContributionsList(w http.ResponseWriter, r *http.Reques if action.Applies != nil { applies, err := action.Applies(ctx, principal.ID) if err != nil { + // The action is left out; the section and the + // other sections are still answered. moduleLog.ErrorContext(ctx, "cabana: account contribution applies failed", "section", item.section.Name, "action", action.Name, "plugin", item.pluginID, "error", err) - WriteError(w, http.StatusInternalServerError, "error", msgServerError) - return + continue } if !applies { continue @@ -207,8 +214,29 @@ func (s *service) accountContributionAction(w http.ResponseWriter, r *http.Reque return } moduleLog.InfoContext(ctx, "cabana: account contribution action", "admin_id", principal.ID, "section", item.section.Name, "action", action.Name) + if result.URL != "" && !safeAccountActionURL(result.URL) { + // Never log the URL itself: it may carry a secret or a script. + moduleLog.ErrorContext(ctx, "cabana: account contribution action answered a non-http URL", "section", item.section.Name, "action", action.Name, "plugin", item.pluginID) + result.URL = "" + } WriteData(w, http.StatusOK, AccountActionResultView{ Message: translateKey(ctx, tr, result.Message), URL: result.URL, }, nil) } + +// safeAccountActionURL reports whether an account action's URL may be handed +// to the admin's browser, which renders it as a link and opens it: an +// absolute http or https URL with a host, or a same-origin relative path +// (exactly one leading "/", no scheme). Anything else, javascript: and data: +// included, is refused. +func safeAccountActionURL(raw string) bool { + u, err := url.Parse(raw) + if err != nil { + return false + } + if u.Scheme == "" { + return strings.HasPrefix(raw, "/") && !strings.HasPrefix(raw, "//") && !strings.HasPrefix(raw, "/\\") && u.Host == "" + } + return (u.Scheme == "https" || u.Scheme == "http") && u.Host != "" +} diff --git a/modules/cabana/actions.go b/modules/cabana/actions.go index 6a2306dd..d1a1b7de 100644 --- a/modules/cabana/actions.go +++ b/modules/cabana/actions.go @@ -246,6 +246,7 @@ func (s *service) recordAction(w http.ResponseWriter, r *http.Request) { const ( promptRequiredKey = "backend::lang.form.action_prompt_required" promptTooLongKey = "backend::lang.form.action_prompt_too_long" + promptInvalidKey = "backend::lang.form.action_prompt_invalid" ) // promptValue decodes the prompt answer of a prompted record action from @@ -265,7 +266,7 @@ func promptValue(ctx context.Context, tr *phrasebook.Translator, prompt *pact.Ad } var raw string if err := json.Unmarshal(payload, &raw); err != nil { - return "", refuse("The payload must be a string.") + return "", refuse(translateKey(ctx, tr, promptInvalidKey)) } value := strings.TrimSpace(raw) if value == "" { diff --git a/modules/cabana/admin_openapi.go b/modules/cabana/admin_openapi.go index 49034c60..049b2198 100644 --- a/modules/cabana/admin_openapi.go +++ b/modules/cabana/admin_openapi.go @@ -338,9 +338,13 @@ type AccountContributionView struct { Label string `json:"label"` Description string `json:"description"` // Status is the section's line for the signed-in admin; "" when the - // section has none. + // section has none or its status could not be loaded. Status string `json:"status"` Actions []AccountContributionActionView `json:"actions"` + // Error is true when the section's status could not be loaded (the + // plugin's Status failed); the other sections are answered as usual. + // Absent otherwise. + Error bool `json:"error,omitempty"` } // AccountContributionActionView is one button of a contributed section: the @@ -795,7 +799,7 @@ func AdminBulkAction() {} // AdminRecordAction documents the declared record action route. // // @Summary Run a declared record action -// @Description Runs a record action the controller registers and the form's recordActions declares. The record is loaded and row-locked through the controller's form scope in one transaction (404 when missing or out of scope); an action that does not apply to the record's current state answers 409. The body must be {}: record_id, values and payload are all refused, and the answer's fill is always empty. +// @Description Runs a record action the controller registers and the form's recordActions declares. The record is loaded and row-locked through the controller's form scope in one transaction (404 when missing or out of scope); an action that does not apply to the record's current state answers 409. The body is {} for an action without a prompt; an action whose record response carries prompt takes {"payload": ""}, trimmed, non-empty and at most prompt.max_length runes, with a 422 on payload otherwise. record_id and values are always refused, and the answer's fill is always empty. // @Tags admin // @Accept json // @Produce json @@ -805,7 +809,7 @@ func AdminBulkAction() {} // @Param controller path string true "Controller" // @Param id path integer true "Record id" // @Param action path string true "Record action name" -// @Param body body AdminActionRequest true "Empty object" +// @Param body body AdminActionRequest true "Empty object, or {payload: text} for a prompted action" // @Success 200 {object} Envelope[AdminActionResult] // @Failure 401 {object} ErrorEnvelope // @Failure 403 {object} ErrorEnvelope "also returned when controller code refuses the write; details may name fields" @@ -824,7 +828,8 @@ type AdminActionRequest struct { Values map[string]any `json:"values,omitempty"` // Payload is the widget's own JSON value (the summer-action event's // detail.payload): any JSON, at most 64 KiB, handed to the action as-is. - // Toolbar and record routes refuse it. + // Toolbar routes refuse it; record routes accept it only for a prompted + // action, as a JSON string (the prompt's answer). Payload json.RawMessage `json:"payload,omitempty"` } @@ -863,7 +868,7 @@ func AdminWidgetAction() {} // AdminToolbarAction documents the toolbar action route. // // @Summary Run a toolbar action -// @Description Runs a controller-registered action that the list's toolbar.buttons declares. The body must be {}: record_id, values and payload are all refused, and the answer's fill is always empty. +// @Description Runs a controller-registered action that the list's toolbar.buttons declares. The body must be {}: the route refuses record_id, values and payload, and the answer's fill is always empty. // @Tags admin // @Accept json // @Produce json diff --git a/modules/cabana/record_action_prompt_postgres_test.go b/modules/cabana/record_action_prompt_postgres_test.go index 4cb69c5f..4663c796 100644 --- a/modules/cabana/record_action_prompt_postgres_test.go +++ b/modules/cabana/record_action_prompt_postgres_test.go @@ -74,7 +74,7 @@ func TestRecordActionPromptRun(t *testing.T) { link := userURL(target.ID, "actions", "acme_link") env.expectFieldRefusal(t, http.StatusUnprocessableEntity, http.MethodPost, link, token, map[string]any{"payload": " "}, "payload", "Enter a value to run this action.", false) - env.expectFieldRefusal(t, http.StatusUnprocessableEntity, http.MethodPost, link, token, map[string]any{"payload": 7}, "payload", "The payload must be a string.", false) + env.expectFieldRefusal(t, http.StatusUnprocessableEntity, http.MethodPost, link, token, map[string]any{"payload": 7}, "payload", "The answer must be text.", false) if len(values) != 0 { t.Fatalf("a refused value ran the action: %v", values) } diff --git a/modules/pact/capabilities.go b/modules/pact/capabilities.go index 956e0778..da5ad487 100644 --- a/modules/pact/capabilities.go +++ b/modules/pact/capabilities.go @@ -365,7 +365,8 @@ type AdminRecordAction struct { // shows in its confirm dialog. Label, Placeholder and Comment are phrase keys // (containing "::") or literal text; Label is required. MaxLength caps the // value in runes, 0 meaning 255. The admin refuses an empty or over-long -// value with a validation error on the input before the action runs. +// value before the action runs with a 422 on payload, whose message the +// admin SPA shows as a toast; the prompt dialog then closes. type AdminActionPrompt struct { Label string Placeholder string diff --git a/modules/phrasebook/backend/lang/en/lang.yaml b/modules/phrasebook/backend/lang/en/lang.yaml index 861a5179..3d192289 100644 --- a/modules/phrasebook/backend/lang/en/lang.yaml +++ b/modules/phrasebook/backend/lang/en/lang.yaml @@ -109,6 +109,7 @@ form: action_stale: This action no longer applies to this record. The page has been refreshed. action_prompt_required: Enter a value to run this action. action_prompt_too_long: "The value may not be longer than :max characters." + action_prompt_invalid: The answer must be text. forbidden: You do not have permission to make this change. Nothing was saved. dependent_invalid: Choose one of the listed options. trigger_disabled_on: "Not editable while “:field” is on." diff --git a/modules/phrasebook/backend/lang/pl/lang.yaml b/modules/phrasebook/backend/lang/pl/lang.yaml index fb0a8462..4c8eeb66 100644 --- a/modules/phrasebook/backend/lang/pl/lang.yaml +++ b/modules/phrasebook/backend/lang/pl/lang.yaml @@ -119,6 +119,7 @@ form: action_stale: Ta akcja nie dotyczy już tego rekordu. Strona została odświeżona. action_prompt_required: Wpisz wartość, aby uruchomić tę akcję. action_prompt_too_long: "Wartość nie może być dłuższa niż :max znaków." + action_prompt_invalid: Odpowiedź musi być tekstem. forbidden: Nie masz uprawnień do wprowadzenia tej zmiany. Nic nie zostało zapisane. dependent_invalid: Wybierz jedną z dostępnych opcji. trigger_disabled_on: "Nie można edytować, gdy „:field” jest włączone."