From 068adaabf9dd14195b895023ff1814d32eaef609 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Thu, 22 Oct 2026 13:29:38 +0200 Subject: [PATCH] fix(19.6.1-04): refuse code-declared role capture and wrap Administrators Runs - Roles create and update refuse a submitted code that code-declared grants attach to - compileActionExtensions wraps contributed Administrators Runs with guardOutranked - Prove both holes on Postgres; keep the boot smoke callable with a superuser principal Co-authored-by: Cursor --- modules/cabana/action_extensions.go | 23 +++++++ .../cabana/action_extensions_postgres_test.go | 28 ++++++++ modules/cabana/action_extensions_test.go | 4 +- modules/cabana/backend_roles_admin.go | 25 ++++++- modules/cabana/roles_postgres_test.go | 66 +++++++++++++++++++ 5 files changed, 143 insertions(+), 3 deletions(-) diff --git a/modules/cabana/action_extensions.go b/modules/cabana/action_extensions.go index dd8e39e1..b9760abb 100644 --- a/modules/cabana/action_extensions.go +++ b/modules/cabana/action_extensions.go @@ -1,9 +1,11 @@ package cabana import ( + "context" "fmt" "strings" + "git.golem15.com/golem15/summercms/modules/bouncer" "git.golem15.com/golem15/summercms/modules/pact" "git.golem15.com/golem15/summercms/modules/party" ) @@ -48,6 +50,9 @@ func compileActionExtensions(reg *Registry, plugins []party.Plugin) error { if err := checkContributedAction(cc, pluginID, ext.Controller, action); err != nil { return err } + if ext.Controller == AdministratorsControllerID { + action.Run = wrapAdministratorsContributedRun(reg, action.Run) + } // A new slice and map: the compiled backing array may be // shared with another schema value. names := make([]string, 0, len(cc.Form.recordActions)+1) @@ -95,3 +100,21 @@ func checkContributedAction(cc *CompiledController, pluginID, controller string, } return nil } + +// wrapAdministratorsContributedRun runs guardOutranked on the locked +// Administrators record before the contributor's Run (D-19). Contributions +// to any other controller are stored unchanged. The wrap is not exported: +// cabana owns the privilege check. +func wrapAdministratorsContributedRun(reg *Registry, next func(context.Context, pact.AdminRecordActionInput) (pact.AdminRecordActionResult, error)) func(context.Context, pact.AdminRecordActionInput) (pact.AdminRecordActionResult, error) { + return func(ctx context.Context, in pact.AdminRecordActionInput) (pact.AdminRecordActionResult, error) { + principal, _ := bouncer.User(ctx) + user, ok := in.Record.(*BackendUser) + if !ok || user == nil { + return pact.AdminRecordActionResult{}, fmt.Errorf("cabana: contributed Administrators action: record is not a backend user") + } + if err := guardOutranked(ctx, reg, principal, user); err != nil { + return pact.AdminRecordActionResult{}, err + } + return next(ctx, in) + } +} diff --git a/modules/cabana/action_extensions_postgres_test.go b/modules/cabana/action_extensions_postgres_test.go index b95a6f60..c02ad0fa 100644 --- a/modules/cabana/action_extensions_postgres_test.go +++ b/modules/cabana/action_extensions_postgres_test.go @@ -61,3 +61,31 @@ func TestActionExtensionRun(t *testing.T) { hidden := env.seedAdmin(t, adminSeed{Name: "ext-hidden", Superuser: true}) env.expectRefusal(t, http.StatusNotFound, http.MethodPost, userURL(hidden.ID, "actions", "acme_ping"), bothToken, map[string]any{}, "") } + +// TestActionExtensionOutranked (WR-03, D-19): a contributed record action +// on an outranking Administrators target is 403 user.outranked and Run is +// not called. Built-in deactivate stays 403 on that target. A covered +// target still runs. +func TestActionExtensionOutranked(t *testing.T) { + env := newScreensEnv(t) + delegate := env.seedAdmin(t, adminSeed{Name: "extout-delegate", RolePermissions: map[string]int{cabana.PermissionManageUsers: 1, pingPermission: 1}}) + delegateToken := env.login(t, delegate.Login) + target := env.seedAdmin(t, adminSeed{Name: "extout-target", RolePermissions: map[string]int{"acme.demo.access": 1}}) + ping := userURL(target.ID, "actions", "acme_ping") + + env.expectRefusal(t, http.StatusForbidden, http.MethodPost, ping, delegateToken, map[string]any{}, enOutranked) + if runs, _ := env.spy.runs(); len(runs) != 0 { + t.Fatalf("outranked acme_ping ran: %v", runs) + } + env.expectRefusal(t, http.StatusForbidden, http.MethodPost, userURL(target.ID, "actions", "deactivate"), delegateToken, map[string]any{}, enOutranked) + + covered := env.seedAdmin(t, adminSeed{Name: "extout-covered"}) + result := dataOf(env.expect(t, http.StatusOK, http.MethodPost, userURL(covered.ID, "actions", "acme_ping"), delegateToken, map[string]any{})) + if result["message"] != "Pinged" { + t.Fatalf("covered result = %v", result) + } + runs, withTx := env.spy.runs() + if len(runs) != 1 || runs[0] != uint64(covered.ID) || !withTx[0] { + t.Fatalf("runs = %v tx = %v, want one run on %d inside the transaction", runs, withTx, covered.ID) + } +} diff --git a/modules/cabana/action_extensions_test.go b/modules/cabana/action_extensions_test.go index ed6e5271..819eee7b 100644 --- a/modules/cabana/action_extensions_test.go +++ b/modules/cabana/action_extensions_test.go @@ -7,6 +7,7 @@ import ( "testing" "git.golem15.com/golem15/summercms/modules/backpack" + "git.golem15.com/golem15/summercms/modules/bouncer" "git.golem15.com/golem15/summercms/modules/pact" "git.golem15.com/golem15/summercms/modules/party" ) @@ -148,7 +149,8 @@ func TestActionExtensionBoot(t *testing.T) { if !ok { t.Fatal("acme_spy not resolvable") } - if _, err := resolved.Run(context.Background(), pact.AdminRecordActionInput{RecordID: 7, Record: &BackendUser{ID: 7}}); err != nil || got.RecordID != 7 { + ctx := bouncer.WithUser(context.Background(), &bouncer.Principal{ID: 1, Backend: true, IsSuperuser: true}) + if _, err := resolved.Run(ctx, pact.AdminRecordActionInput{RecordID: 7, Record: &BackendUser{ID: 7}}); err != nil || got.RecordID != 7 { t.Fatalf("run = %+v, %v", got, err) } }) diff --git a/modules/cabana/backend_roles_admin.go b/modules/cabana/backend_roles_admin.go index 97acc741..fb3553e1 100644 --- a/modules/cabana/backend_roles_admin.go +++ b/modules/cabana/backend_roles_admin.go @@ -155,13 +155,31 @@ func (rolesAdmin) AdminSetPermissionValues(_ context.Context, field string, reco return nil } -// FormBeforeCreate refuses a code another role uses. A role created here is -// never a system role: is_system is not fillable. +// refuseCodeDeclaredCode refuses a submitted code that code-declared grants +// (pact.Permission.Roles) attach to when it is not the stored code (empty +// on create). A role never becomes a code-declared system role through this +// screen, including when the editor is a superuser (D-16). +func (c rolesAdmin) refuseCodeDeclaredCode(submitted, stored string) error { + if submitted == stored { + return nil + } + if len(c.registry().rolePermissions(submitted)) > 0 { + return &ValidationError{Details: map[string]any{"code": []string{rolesLang + "code_locked"}}} + } + return nil +} + +// FormBeforeCreate refuses a code another role uses and a code that +// code-declared grants attach to. A role created here is never a system +// role: is_system is not fillable, and a code-declared code is refused. func (c rolesAdmin) FormBeforeCreate(ctx context.Context, model any) error { role, err := asRole(model) if err != nil { return err } + if err := c.refuseCodeDeclaredCode(role.Code, ""); err != nil { + return err + } return c.checkCode(ctx, role) } @@ -184,6 +202,9 @@ func (c rolesAdmin) FormBeforeUpdate(ctx context.Context, model any) error { if c.systemRole(stored.IsSystem, stored.Code) && role.Code != stored.Code { return &ValidationError{Details: map[string]any{"code": []string{rolesLang + "code_locked"}}} } + if err := c.refuseCodeDeclaredCode(role.Code, stored.Code); err != nil { + return err + } return c.checkCode(ctx, role) } diff --git a/modules/cabana/roles_postgres_test.go b/modules/cabana/roles_postgres_test.go index d5bd96a0..e0ff8e0a 100644 --- a/modules/cabana/roles_postgres_test.go +++ b/modules/cabana/roles_postgres_test.go @@ -261,6 +261,72 @@ func TestRolesCodeDeclaredLocks(t *testing.T) { } } +// TestRolesCodeDeclaredCodeRefused (CR-01, SC-2, D-07, D-16): a role never +// becomes a code-declared system role through the Roles screen. Create and +// rename onto a code that pact.Permission.Roles attaches grants to answer +// 422 code_locked, including when the editor is a superuser. A unique +// non-declared code still creates and a same-code save of a system role +// still succeeds. +func TestRolesCodeDeclaredCodeRefused(t *testing.T) { + declared := fmt.Sprintf("plugcap-%d", time.Now().UnixNano()) + env := newScreensEnv(t, grantPlugin{id: "acme.capturer", grants: []pact.Permission{{Code: "acme.capturer.secret", Roles: []string{declared}}}}) + editor := env.seedAdmin(t, adminSeed{Name: "codecap-editor", RolePermissions: map[string]int{cabana.PermissionManageRoles: 1}}) + editorToken := env.login(t, editor.Login) + mutable := env.seedRole(t, roleSeed{Name: "codecap-mutable"}) + before := env.roleRow(t, mutable.ID) + + env.expectFieldRefusal(t, http.StatusUnprocessableEntity, http.MethodPut, roleURL(mutable.ID), editorToken, + roleBody(mutable, map[string]any{"code": declared}), "code", enCodeLocked, false) + if env.roleRow(t, mutable.ID) != before { + t.Fatal("a refused rename onto a code-declared code touched the role") + } + + env.expectFieldRefusal(t, http.StatusUnprocessableEntity, http.MethodPost, rolesPath, editorToken, + map[string]any{"name": "Capture " + env.stamp, "code": declared}, "code", enCodeLocked, false) + if env.roleCountByCode(t, declared) != 0 { + t.Fatal("a refused create inserted a role with the code-declared code") + } + + super := env.seedAdmin(t, adminSeed{Name: "codecap-super", Superuser: true}) + superToken := env.login(t, super.Login) + env.expectFieldRefusal(t, http.StatusUnprocessableEntity, http.MethodPut, roleURL(mutable.ID), superToken, + roleBody(mutable, map[string]any{"code": declared}), "code", enCodeLocked, false) + if env.roleRow(t, mutable.ID) != before { + t.Fatal("a superuser rename onto a code-declared code touched the role") + } + env.expectFieldRefusal(t, http.StatusUnprocessableEntity, http.MethodPost, rolesPath, superToken, + map[string]any{"name": "Super capture " + env.stamp, "code": declared}, "code", enCodeLocked, false) + if env.roleCountByCode(t, declared) != 0 { + t.Fatal("a superuser create inserted a role with the code-declared code") + } + + plain := "plain-" + env.stamp + created := dataOf(env.expect(t, http.StatusCreated, http.MethodPost, rolesPath, editorToken, + map[string]any{"name": "Plain " + env.stamp, "code": plain, "description": "Not declared"})) + createdID := uint(created["id"].(float64)) + t.Cleanup(func() { env.gdb.Exec(`DELETE FROM backend_user_roles WHERE id = ?`, createdID) }) + + var developer cabana.BackendUserRole + if err := env.gdb.Where("code = ?", "developer").First(&developer).Error; err != nil { + t.Fatal(err) + } + env.expect(t, http.StatusOK, http.MethodPut, roleURL(developer.ID), superToken, + roleBody(developer, map[string]any{"description": "Same code " + env.stamp})) + t.Cleanup(func() { + env.gdb.Exec(`UPDATE backend_user_roles SET description = ? WHERE id = ?`, developer.Description, developer.ID) + }) +} + +// roleCountByCode counts backend_user_roles rows with code. +func (e *screensEnv) roleCountByCode(t *testing.T, code string) int64 { + t.Helper() + var n int64 + if err := e.gdb.Raw(`SELECT count(*) FROM backend_user_roles WHERE code = ?`, code).Scan(&n).Error; err != nil { + t.Fatal(err) + } + return n +} + // reloadRoleByCode reads the role with code. func (e *screensEnv) reloadRoleByCode(t *testing.T, code string) cabana.BackendUserRole { t.Helper()