From 2de05b2ccc0358298dbeebbd66862cab59dc8a31 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 28 Sep 2026 07:41:38 +0200 Subject: [PATCH 1/2] sec(frontend): refuse to save a group with zero permissions The group edit form's collectPermissions() skips any row whose action or resource is still the "Select Action"/"Select Resource" placeholder, so removing every real permission row (or leaving only a placeholder row) collected as permissions: []. The update API's documented contract (applyUpdateGroupRequest, internal/auth/service_api.go) treats an empty permissions list as "not sent" and leaves the group's stored permissions untouched -- there is no way for this endpoint to express "clear all permissions". So the form showed "Group updated successfully" and reloaded a group that still carried its old privileges, possibly admin:*. Checked the backend: this "empty means unchanged" behavior is an existing, documented, intentional contract of the update endpoint (not a bug to fix there); the gap is purely that the frontend form let an operator submit a payload that silently meant something other than what the UI told them they were doing. Refuse to submit when collectPermissions() returns an empty list, for both the create and update paths, with an explicit message directing the operator to delete the group instead. Also fixed two pre-existing tests whose assertions encoded the buggy behavior as correct (submitting permissions: [] and expecting it to reach the API). Co-Authored-By: claude-flow --- .../group-edit-empty-permissions.test.ts | 145 ++++++++++++++++++ ...p-edit-unrepresentable-permissions.test.ts | 9 +- frontend/src/__tests__/groups.test.ts | 16 +- frontend/src/groups/groupModals.ts | 16 ++ 4 files changed, 178 insertions(+), 8 deletions(-) create mode 100644 frontend/src/__tests__/group-edit-empty-permissions.test.ts diff --git a/frontend/src/__tests__/group-edit-empty-permissions.test.ts b/frontend/src/__tests__/group-edit-empty-permissions.test.ts new file mode 100644 index 000000000..32ab24953 --- /dev/null +++ b/frontend/src/__tests__/group-edit-empty-permissions.test.ts @@ -0,0 +1,145 @@ +/** + * Regression tests for issue #237: removing every permission row from the + * group edit form and saving reported "Group updated successfully" while + * silently leaving the group's stored permissions untouched. + * + * collectPermissions() (groupModals.ts) skips any row whose action or + * resource is still the empty "Select Action"/"Select Resource" placeholder, + * so clicking Remove on every real row -- or leaving only a placeholder row + * -- collects as permissions: []. The update API's documented contract + * (applyUpdateGroupRequest, internal/auth/service_api.go) treats an empty + * permissions list as "not sent" and leaves the group's permissions + * unchanged -- there is no way for this endpoint to express "clear all + * permissions". So the form would show success and reload a group that + * still carried its old privileges (possibly admin:*). + * + * The fix refuses to submit when collectPermissions() returns [], with an + * explicit error telling the operator to delete the group instead. + */ + +import './setup'; + +jest.mock('../api', () => ({ + getGroup: jest.fn(), + updateGroup: jest.fn(), + createGroup: jest.fn(), +})); + +jest.mock('../users/userActions', () => ({ + loadUsers: jest.fn().mockResolvedValue(undefined), +})); + +jest.mock('../confirmDialog', () => ({ + confirmDialog: jest.fn().mockResolvedValue(true), +})); + +jest.mock('../users/utils', () => ({ + ...jest.requireActual('../users/utils'), + showError: jest.fn(), + showSuccess: jest.fn(), +})); + +import * as api from '../api'; +import * as groupState from '../groups/state'; +import * as groupModals from '../groups/groupModals'; +import { showError, showSuccess } from '../users/utils'; + +function setUpModalDom(): void { + document.body.innerHTML = ` + + `; +} + +async function submitForm(): Promise { + const event = { preventDefault: jest.fn() } as unknown as Event; + await groupModals.saveGroup(event); +} + +const ADMIN_GROUP: api.APIGroup = { + id: 'admin-group-id', + name: 'Admins', + description: 'Old description', + permissions: [{ action: 'admin', resource: '*' }], + created_at: '2024-01-01T00:00:00Z', +}; + +describe('regression: saving a group with every permission row removed (#237)', () => { + beforeEach(() => { + setUpModalDom(); + groupState.setCurrentEditingGroup(null); + jest.clearAllMocks(); + }); + + test('removing every permission row and saving does not call updateGroup and shows an error, not success', async () => { + (api.getGroup as jest.Mock).mockResolvedValue(ADMIN_GROUP); + await groupModals.openEditGroupModal(ADMIN_GROUP.id); + + // Remove the one loaded permission row via its own Remove button -- + // the exact operator action the issue describes. + const removeBtn = document.querySelector('.remove-permission-btn'); + expect(removeBtn).not.toBeNull(); + removeBtn!.click(); + expect(document.querySelectorAll('.permission-item')).toHaveLength(0); + + await submitForm(); + + // The group's real admin:* permissions must never be silently kept by + // submitting an empty list the backend interprets as "unchanged" -- + // submission must not happen at all. + expect(api.updateGroup).not.toHaveBeenCalled(); + expect(showSuccess).not.toHaveBeenCalled(); + expect(showError).toHaveBeenCalledWith(expect.stringContaining('at least one permission')); + }); + + test('a group whose only remaining row is left on the placeholder also refuses to save', async () => { + (api.getGroup as jest.Mock).mockResolvedValue(ADMIN_GROUP); + await groupModals.openEditGroupModal(ADMIN_GROUP.id); + + // Reset the loaded row back to the "Select Action" / "Select Resource" + // placeholder instead of removing it outright -- the issue's second + // reproduction shape. + const actionSelect = document.querySelector('.perm-action')!; + const resourceSelect = document.querySelector('.perm-resource')!; + actionSelect.value = ''; + resourceSelect.value = ''; + + await submitForm(); + + expect(api.updateGroup).not.toHaveBeenCalled(); + expect(showSuccess).not.toHaveBeenCalled(); + expect(showError).toHaveBeenCalledWith(expect.stringContaining('at least one permission')); + }); + + test('creating a new group with no permission rows also refuses to save', async () => { + groupModals.openCreateGroupModal(); + (document.getElementById('group-name') as HTMLInputElement).value = 'New Group'; + + await submitForm(); + + expect(api.createGroup).not.toHaveBeenCalled(); + expect(showSuccess).not.toHaveBeenCalled(); + expect(showError).toHaveBeenCalledWith(expect.stringContaining('at least one permission')); + }); + + test('a group that still has at least one real permission row saves normally', async () => { + (api.getGroup as jest.Mock).mockResolvedValue(ADMIN_GROUP); + await groupModals.openEditGroupModal(ADMIN_GROUP.id); + + await submitForm(); + + expect(api.updateGroup).toHaveBeenCalledWith('admin-group-id', { + name: 'Admins', + description: 'Old description', + permissions: [{ action: 'admin', resource: '*' }], + }); + expect(showError).not.toHaveBeenCalled(); + }); +}); diff --git a/frontend/src/__tests__/group-edit-unrepresentable-permissions.test.ts b/frontend/src/__tests__/group-edit-unrepresentable-permissions.test.ts index 57d6bd996..4f260daf8 100644 --- a/frontend/src/__tests__/group-edit-unrepresentable-permissions.test.ts +++ b/frontend/src/__tests__/group-edit-unrepresentable-permissions.test.ts @@ -579,7 +579,12 @@ describe('regression: group edit preserves constraints.accounts (#1629)', () => test('removing the offending row unblocks the save', async () => { // The verdict lives on the row, so deleting the row clears it. Removing // the permission outright is an explicit edit, not a silent drop. - await openEdit(groupWithAccounts([''])); + // A second, unrelated clean permission is included so removing the + // offending row exercises this refusal in isolation from the separate + // "a group must keep at least one permission" refusal (issue #237). + const group = groupWithAccounts(['']); + group.permissions.push({ action: 'view', resource: 'plans' }); + await openEdit(group); const removeBtn = document.querySelector('.permission-item .remove-permission-btn') as HTMLButtonElement; removeBtn.click(); await clickSave(); @@ -588,7 +593,7 @@ describe('regression: group edit preserves constraints.accounts (#1629)', () => expect(api.updateGroup).toHaveBeenCalledWith('operators-group-id', { name: 'Prod Operators', description: 'Old description', - permissions: [], + permissions: [{ action: 'view', resource: 'plans' }], }); }); diff --git a/frontend/src/__tests__/groups.test.ts b/frontend/src/__tests__/groups.test.ts index 81f729b31..0ebe7f9b2 100644 --- a/frontend/src/__tests__/groups.test.ts +++ b/frontend/src/__tests__/groups.test.ts @@ -718,7 +718,7 @@ describe('groups/groupModals', () => { })); }); - it('should handle missing permissions list', async () => { + it('should handle a missing permissions list without throwing, and refuse to save (issue #237)', async () => { document.body.innerHTML = `
@@ -729,11 +729,15 @@ describe('groups/groupModals', () => { `; const event = { preventDefault: jest.fn() } as unknown as Event; - await groupModals.saveGroup(event); - - expect(api.createGroup).toHaveBeenCalledWith(expect.objectContaining({ - permissions: [], - })); + // Must not throw even though #permissions-list is entirely absent from + // the DOM (collectPermissions() returns [] rather than erroring). + await expect(groupModals.saveGroup(event)).resolves.toBeUndefined(); + + // A group must grant at least one permission -- an empty list is + // refused rather than submitted as a group with zero permissions + // (issue #237: the update path's [] means "leave unchanged", so + // submitting it here would be indistinguishable from that). + expect(api.createGroup).not.toHaveBeenCalled(); }); }); diff --git a/frontend/src/groups/groupModals.ts b/frontend/src/groups/groupModals.ts index b9e8bfae7..192744ab3 100644 --- a/frontend/src/groups/groupModals.ts +++ b/frontend/src/groups/groupModals.ts @@ -106,6 +106,22 @@ export async function saveGroup(e: Event): Promise { const description = (document.getElementById('group-description') as HTMLTextAreaElement).value; const permissions = collectPermissions(); + // Refuse an empty permission list rather than submit it. collectPermissions + // silently drops any row left on the "Select Action"/"Select Resource" + // placeholder, so removing every real row (or leaving only a placeholder + // row) collects as []. The update API treats an empty list as "not sent" + // and leaves the group's stored permissions untouched (issue #237), so + // submitting would show "Group updated successfully" while silently + // keeping the group's old privileges -- there is no way for this form to + // express "clear all permissions". + if (permissions.length === 0) { + showError( + 'A group must grant at least one permission. ' + + 'To remove all privileges, delete the group instead.' + ); + return; + } + try { if (currentEditingGroup) { // Update existing group From 2af296d78ee4c1a62082db64ffc5d10b6e50aab5 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 28 Sep 2026 11:30:46 +0200 Subject: [PATCH 2/2] sec(auth): reject an explicit empty permissions list on group update Independent review of #387 found the frontend guard alone was insufficient: any non-browser client (CLI, scripts, curl) could still PUT permissions: [] directly. UpdateGroupAPI's existing "empty means not sent" contract silently kept the group's old permissions (possibly admin:*) while returning 200, exactly the false-success scenario issue #237 describes for the UI path. encoding/json already distinguishes an omitted permissions field (decodes to nil) from an explicit `"permissions": []` (decodes to a non-nil, zero-length slice), so no API type change is needed. Add a presence check in UpdateGroupAPI: a non-nil-but-empty Permissions is now rejected with the new ErrEmptyPermissions sentinel, mapped to 400 by mapGroupAuthError. An omitted field still leaves permissions unchanged, unaffected. Co-Authored-By: claude-flow --- internal/api/handler_groups.go | 2 +- internal/auth/errors.go | 10 +++++ internal/auth/service_api.go | 20 +++++++++ internal/auth/service_api_test.go | 70 +++++++++++++++++++++++++++++++ 4 files changed, 101 insertions(+), 1 deletion(-) diff --git a/internal/api/handler_groups.go b/internal/api/handler_groups.go index 52ef624f1..13d2c2ca7 100644 --- a/internal/api/handler_groups.go +++ b/internal/api/handler_groups.go @@ -65,7 +65,7 @@ func (h *Handler) createGroup(ctx context.Context, req *events.LambdaFunctionURL // are disjoint. Unrecognized errors pass through for handleRequestError. func mapGroupAuthError(err error) error { switch { - case errors.Is(err, auth.ErrInvalidPermission): + case errors.Is(err, auth.ErrInvalidPermission), errors.Is(err, auth.ErrEmptyPermissions): // Malformed input, not an authorization failure. return NewClientError(400, err.Error()) case errors.Is(err, auth.ErrPermissionCeiling), diff --git a/internal/auth/errors.go b/internal/auth/errors.go index c9732cd3f..ad66392b0 100644 --- a/internal/auth/errors.go +++ b/internal/auth/errors.go @@ -63,6 +63,16 @@ var ( // tenant-wide. Mapped to 403 (issue #1629). ErrSystemManagedGroup = errors.New("system-managed group cannot be modified") + // ErrEmptyPermissions is returned when a group update explicitly sends + // permissions as an empty list. APIUpdateGroupRequest.Permissions is a + // pointer so a nil value (field omitted) can be told apart from a + // pointer to an empty slice (field sent as `[]`): the former means + // "leave unchanged" (this endpoint has no other way to express that), + // the latter must be refused rather than silently treated as a no-op + // that reports success while the group's real permissions -- possibly + // admin:* -- are left untouched (issue #237). Mapped to 400. + ErrEmptyPermissions = errors.New("a group must grant at least one permission; delete the group to remove all privileges") + // ErrCurrentPasswordIncorrect is returned by UpdateUserProfile when the // caller-supplied current password does not match the stored hash. Mapped // to 401 at the API layer (the acting user is verifying their own diff --git a/internal/auth/service_api.go b/internal/auth/service_api.go index 33816deb4..0fb894ea1 100644 --- a/internal/auth/service_api.go +++ b/internal/auth/service_api.go @@ -102,6 +102,16 @@ type APICreateGroupRequest struct { // APIUpdateGroupRequest is the request type for updating groups via API. // AllowedAccounts has no omitempty: clients must be able to send an explicit // empty slice to clear account restrictions. Nil means "not sent". +// +// Permissions has the same "nil means not sent" contract, but encoding/json +// already gives UpdateGroupAPI what it needs without a pointer: decoding an +// omitted field leaves the slice nil, while decoding an explicit `[]` leaves +// it non-nil with length 0 (json.Unmarshal allocates the slice before +// leaving it empty). UpdateGroupAPI checks req.Permissions != nil && +// len(req.Permissions) == 0 to refuse that explicit-empty case -- this +// endpoint has no way to express "clear all permissions" (a group must +// always grant at least one) -- rather than silently keeping the group's +// old permissions while reporting success (issue #237). type APIUpdateGroupRequest struct { Name string `json:"name,omitempty"` Description string `json:"description,omitempty"` @@ -356,6 +366,16 @@ func (s *Service) UpdateGroupAPI(ctx context.Context, actorUserID, groupID strin return nil, fmt.Errorf("%w: %q is seeded and maintained by migrations", ErrSystemManagedGroup, group.Name) } + // req.Permissions == nil means the field was omitted: leave the group's + // permissions unchanged (this endpoint's pre-existing "not sent" + // contract). A non-nil-but-empty slice means the client explicitly sent + // `"permissions": []` (encoding/json allocates the slice before leaving + // it empty, so this is distinguishable from omission after decode), + // which this endpoint has no way to honor as "clear all permissions" -- + // refuse rather than silently no-op it (issue #237). + if req.Permissions != nil && len(req.Permissions) == 0 { + return nil, ErrEmptyPermissions + } perms := apiPermissionsToPermissions(req.Permissions) if err := s.checkGrantCeiling(ctx, actorUserID, perms, group.Permissions); err != nil { return nil, err diff --git a/internal/auth/service_api_test.go b/internal/auth/service_api_test.go index b351547cd..cdb5e42cc 100644 --- a/internal/auth/service_api_test.go +++ b/internal/auth/service_api_test.go @@ -406,6 +406,76 @@ func TestService_CreateGroupAPI(t *testing.T) { func TestService_UpdateGroupAPI(t *testing.T) { ctx := context.Background() + // Issue #237: an explicit `"permissions": []` must be refused (400), not + // silently treated as "leave unchanged". Pre-fix, APIUpdateGroupRequest. + // Permissions was a plain []APIPermission, which cannot distinguish + // "field omitted" from "field sent as []" once decoded -- both left the + // slice empty, and apiPermissionsToPermissions/applyUpdateGroupRequest's + // `len(perms) > 0` check silently kept the group's OLD permissions + // (possibly admin:*) while UpdateGroup still returned success. + t.Run("explicit empty permissions list is refused, not silently kept", func(t *testing.T) { + mockStore := new(MockStore) + mockEmail := new(MockEmailSender) + service := createTestService(mockStore, mockEmail) + + existingGroup := &Group{ + ID: "group-123", + Name: "Admins", + Description: "Old description", + Permissions: []Permission{{Action: ActionAdmin, Resource: ResourceAll}}, + } + mockStore.On("GetGroup", ctx, "group-123").Return(existingGroup, nil).Once() + + req := APIUpdateGroupRequest{ + Name: "Admins", + Description: "New description", + Permissions: []APIPermission{}, // explicit []: every row removed and saved + } + + result, err := service.UpdateGroupAPI(ctx, AdminAPIKeyActorID, "group-123", req) + + require.Error(t, err) + assert.Nil(t, result) + assert.ErrorIs(t, err, ErrEmptyPermissions) + // The group's real admin:* permissions must never be silently kept by + // a write that reports success -- UpdateGroup must not even be called. + mockStore.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything) + }) + + // Negative control: an omitted Permissions field (nil pointer) is the + // pre-existing "leave unchanged" contract and must still work exactly as + // before -- this refusal must not overreach into that case. + t.Run("omitted permissions field leaves the group's permissions unchanged", func(t *testing.T) { + mockStore := new(MockStore) + mockEmail := new(MockEmailSender) + service := createTestService(mockStore, mockEmail) + + existingGroup := &Group{ + ID: "group-123", + Name: "Admins", + Description: "Old description", + Permissions: []Permission{{Action: ActionAdmin, Resource: ResourceAll}}, + } + mockStore.On("GetGroup", ctx, "group-123").Return(existingGroup, nil).Once() + mockStore.On("UpdateGroup", ctx, mock.AnythingOfType("*auth.Group")).Return(nil).Once() + + req := APIUpdateGroupRequest{ + Name: "Admins", + Description: "New description", + // Permissions left as its zero value: nil pointer, field omitted. + } + + result, err := service.UpdateGroupAPI(ctx, AdminAPIKeyActorID, "group-123", req) + + require.NoError(t, err) + apiGroup, ok := result.(*APIGroup) + require.True(t, ok) + assert.Equal(t, "New description", apiGroup.Description) + assert.Equal(t, []Permission{{Action: ActionAdmin, Resource: ResourceAll}}, existingGroup.Permissions, + "omitting the field must leave permissions untouched") + mockStore.AssertExpectations(t) + }) + t.Run("successful group update", func(t *testing.T) { mockStore := new(MockStore) mockEmail := new(MockEmailSender)