Protected environment settings silently discard failed updates

Problem

In Settings > CI/CD > Protected environments, deleting an approver or a deployer sends the request, discards any error, and leaves the UI looking unchanged. The user gets no indication that anything failed.

updateEnvironment catches every rejection and commits only RECEIVE_UPDATE_PROTECTED_ENVIRONMENT_ERROR, whose mutation sets state.loading = false and nothing else. Every failure class behaves this way: 422, 403, 412, 500, and network errors.

This is reachable today with no unusual configuration. The deploy_access_levels minimum-of-one validation has always rejected removing the last deployer, so that action is already a silent no-op. Confirmed by measurement against a freshly-fetched record:

update result: false
errors: ["Deploy access levels is too short (minimum is 1 character)"]

A second, related defect: updateRule commits RECEIVE_RULE_UPDATED in its .then regardless of outcome, because updateEnvironment swallows the rejection and resolves. That mutation sets editingRules[rule.id] = null, which closes the edit form and discards whatever the user typed.

This is not a security issue

Recording this so it is not re-escalated. state.protectedEnvironments is mutated in only three places, all in ee/app/assets/javascripts/protected_environments/store/edit/mutations.js: line 9 (initial fetch), lines 42-43 (..._SUCCESS), and line 70 (DELETE_PROTECTED_ENVIRONMENT_SUCCESS). REQUEST_UPDATE_PROTECTED_ENVIRONMENT only sets loading = true.

There is no optimistic update, so on failure the list keeps rendering the server's actual state and the entry remains visibly listed. The pending-new- approver picker is likewise only cleared on success. The UI never displays a weaker state than reality, so an operator cannot be misled into believing access was revoked when it was not. The failure mode is a missing explanation, not a misrepresentation.

Steps to reproduce

  1. Protect an environment with exactly one entry under Allowed to deploy.
  2. Go to Settings > CI/CD > Protected environments and expand it.
  3. Delete that sole deployer entry.
  4. The request returns 422. Nothing happens in the UI, and no error appears.

Proposed fix

Three changes in ee/app/assets/javascripts/protected_environments/store/edit/actions.js.

  1. Add a module-private helper that reads the API's message, handling both the array shape (render_api_error! with errors.full_messages) and the plain string shape (destroy_conditionally!), falling back to an existing translated string so locale/gitlab.pot needs no regeneration:

    const apiErrorMessage = (error) =>
  []
    .concat(error?.response?.data?.message ?? __('Something went wrong. Please try again.'))
    .join(', ');

  2. In updateEnvironment, call createAlert({ message: apiErrorMessage(error) }) in the .catch, and return true from the success path and false from the failure path so callers can tell the difference.

  3. In updateRule, early-return when updateEnvironment resolves false, so the edit form stays open. Use an identity check (=== false), not truthiness - spec/frontend/__helpers__/vuex_action_helper.js mocks dispatch to resolve undefined, so !updated would early-return and break the existing updateRule test.

  4. Apply the same createAlert to unprotectEnvironment, which has the identical silent-failure behaviour on a path the docs actively direct users toward.

Tests

  • updateRule keeps the form open on rejection: drive it with dispatch: () => Promise.resolve(false) and assert commit is never called.
  • updateEnvironment alerts the API's message on 422 with { message: [reason] }.
  • updateEnvironment alerts the generic fallback on 500.
  • unprotectEnvironment alerts on failure. Use the string message shape ({ message: '404 Not Found' }) - that endpoint goes through destroy_conditionally! and returns 204, or 412/404 with a string. It never returns a 422 with a model-error array. Note there is no HTTP_STATUS_PRECONDITION_FAILED constant in app/assets/javascripts/lib/utils/http_status.js.

Verification

node_modules/.bin/jest --config jest.config.js ee/spec/frontend/protected_environments

Expect 10 suites and 154 tests passing. Also run eslint and prettier --check on both changed files.

Working implementation

Already written and verified against master at 24d47ccc9ad1: 2 files, +72/-7, jest 154/154, ESLint, Prettier and danger commit-lint clean. Apply with git apply.

Complete diff 
diff --git a/ee/app/assets/javascripts/protected_environments/store/edit/actions.js b/ee/app/assets/javascripts/protected_environments/store/edit/actions.js
index 6b0e5a50c62e..e2da727231e8 100644
--- a/ee/app/assets/javascripts/protected_environments/store/edit/actions.js
+++ b/ee/app/assets/javascripts/protected_environments/store/edit/actions.js
@@ -1,6 +1,8 @@
 import { getUser, getProjectMembers, getGroupMembers } from '~/rest_api';
 import Api from 'ee/api';
+import { createAlert } from '~/alert';
 import { normalizeHeaders, parseIntPagination } from '~/lib/utils/common_utils';
+import { __ } from '~/locale';
 import {
   INHERITED_GROUPS,
   NON_INHERITED_GROUPS,
@@ -9,6 +11,11 @@ import {
 } from '../../constants';
 import * as types from './mutation_types';
 
+const apiErrorMessage = (error) =>
+  []
+    .concat(error?.response?.data?.message ?? __('Something went wrong. Please try again.'))
+    .join(', ');
+
 const fetchUsersForRuleForProject = (
   entityId,
   entityType,
@@ -140,7 +147,13 @@ export const updateRule = ({ dispatch, state, commit }, { environment, ruleKey,
     name: environment.name,
     [ruleKey]: [Object.fromEntries(updatedRuleEntries)],
   };
-  return dispatch('updateEnvironment', updatedEnvironment).then(() => {
+  return dispatch('updateEnvironment', updatedEnvironment).then((updated) => {
+    // Committing here closes the edit form and discards what the user typed,
+    // so leave it open when `updateEnvironment` reports a rejected update.
+    // Only an explicit `false` means a rejection; the Vuex test helper's
+    // `dispatch` resolves `undefined`.
+    if (updated === false) return;
+
     commit(types.RECEIVE_RULE_UPDATED, rule);
   });
 };
@@ -152,9 +165,16 @@ export const updateEnvironment = ({ state, commit, dispatch }, environment) => {
     .then(({ data }) => {
       commit(types.RECEIVE_UPDATE_PROTECTED_ENVIRONMENT_SUCCESS, data);
       dispatch('fetchAllMembersForEnvironment', data);
+
+      return true;
     })
     .catch((error) => {
+      // The API rejects some updates, such as one that removes the last
+      // approval rule. Without the reason, the update looks like a no-op.
+      createAlert({ message: apiErrorMessage(error) });
       commit(types.RECEIVE_UPDATE_PROTECTED_ENVIRONMENT_ERROR, error);
+
+      return false;
     });
 };
 
@@ -186,6 +206,7 @@ export const unprotectEnvironment = ({ state, commit, dispatch }, environment) =
       }
     })
     .catch((error) => {
+      createAlert({ message: apiErrorMessage(error) });
       commit(types.RECEIVE_UPDATE_PROTECTED_ENVIRONMENT_ERROR, error);
     });
 };
diff --git a/ee/spec/frontend/protected_environments/store/edit/actions_spec.js b/ee/spec/frontend/protected_environments/store/edit/actions_spec.js
index a931d1a72f02..13a1f2ee33fb 100644
--- a/ee/spec/frontend/protected_environments/store/edit/actions_spec.js
+++ b/ee/spec/frontend/protected_environments/store/edit/actions_spec.js
@@ -18,7 +18,13 @@ import {
 import * as types from 'ee/protected_environments/store/edit/mutation_types';
 import { state } from 'ee/protected_environments/store/edit/state';
 import axios from '~/lib/utils/axios_utils';
-import { HTTP_STATUS_OK, HTTP_STATUS_INTERNAL_SERVER_ERROR } from '~/lib/utils/http_status';
+import { createAlert } from '~/alert';
+import {
+  HTTP_STATUS_OK,
+  HTTP_STATUS_INTERNAL_SERVER_ERROR,
+  HTTP_STATUS_NOT_FOUND,
+  HTTP_STATUS_UNPROCESSABLE_ENTITY,
+} from '~/lib/utils/http_status';
 import {
   DEPLOYER_RULE_KEY,
   RULE_KEYS,
@@ -27,6 +33,8 @@ import {
 } from 'ee/protected_environments/constants';
 import { MAINTAINER_ACCESS_LEVEL, DEVELOPER_ACCESS_LEVEL } from '../../constants';
 
+jest.mock('~/alert');
+
 describe('ee/protected_environments/store/edit/actions', () => {
   let mockedState;
   let mock;
@@ -301,6 +309,20 @@ describe('ee/protected_environments/store/edit/actions', () => {
         ],
       );
     });
+
+    it('keeps the edit form open when the update was rejected', async () => {
+      const rule = { id: 1, group_id: 1 };
+      const commit = jest.fn();
+
+      mockedState.editingRules = { [rule.id]: rule };
+
+      await updateRule(
+        { commit, state: mockedState, dispatch: () => Promise.resolve(false) },
+        { environment, rule, ruleKey: DEPLOYER_RULE_KEY },
+      );
+
+      expect(commit).not.toHaveBeenCalled();
+    });
   });
 
   describe('updateEnvironment', () => {
@@ -329,10 +351,10 @@ describe('ee/protected_environments/store/edit/actions', () => {
       );
     });
 
-    it('successfully retains the error', () => {
+    it('successfully retains the error', async () => {
       mock.onPut(url, environment).replyOnce(HTTP_STATUS_INTERNAL_SERVER_ERROR);
 
-      return testAction(
+      await testAction(
         updateEnvironment,
         environment,
         mockedState,
@@ -344,6 +366,24 @@ describe('ee/protected_environments/store/edit/actions', () => {
         ],
         [],
       );
+
+      expect(createAlert).toHaveBeenCalledWith({
+        message: 'Something went wrong. Please try again.',
+      });
+    });
+
+    it('alerts the reason the API rejected the update', async () => {
+      const reason = 'Approval rules cannot all be removed.';
+      mock
+        .onPut(url, environment)
+        .replyOnce(HTTP_STATUS_UNPROCESSABLE_ENTITY, { message: [reason] });
+
+      await updateEnvironment(
+        { state: mockedState, commit: jest.fn(), dispatch: jest.fn() },
+        environment,
+      );
+
+      expect(createAlert).toHaveBeenCalledWith({ message: reason });
     });
   });
 
@@ -470,13 +510,15 @@ describe('ee/protected_environments/store/edit/actions', () => {
       );
     });
 
-    it('saves the error on failure', () => {
+    it('saves the error on failure and alerts the reason', async () => {
       const environment = {
         name: 'staging',
       };
-      mock.onDelete(url, environment).replyOnce(HTTP_STATUS_INTERNAL_SERVER_ERROR);
+      // This endpoint answers with a string message, not a model-error array.
+      const reason = '404 Not Found';
+      mock.onDelete(url, environment).replyOnce(HTTP_STATUS_NOT_FOUND, { message: reason });
 
-      return testAction(
+      await testAction(
         unprotectEnvironment,
         environment,
         mockedState,
@@ -486,6 +528,8 @@ describe('ee/protected_environments/store/edit/actions', () => {
         ],
         [],
       );
+
+      expect(createAlert).toHaveBeenCalledWith({ message: reason });
     });
 
     it('redirects to previous page after deleting if current page is empty', () => {

Reference artifacts

These are closed and are context only - do not reopen them:

Edited by 🤖 GitLab Bot 🤖