From 57a03b551b4c1442a17368b0a99a6173fcec387e Mon Sep 17 00:00:00 2001
From: Javiera Tapia <jtapia@gitlab.com>
Date: Mon, 10 Mar 2025 16:35:30 -0300
Subject: [PATCH] approval_rules name is unbounded
Merge branch 'security-519348-approval-rules-name-bounded' into 'master'
See merge request gitlab-org/security/gitlab!4812
Changelog: security
---
doc/api/merge_request_approvals.md | 12 +++++-----
.../approvals/components/rules/rule_form.vue | 13 +++++++++++
.../assets/javascripts/approvals/constants.js | 1 +
ee/app/models/concerns/approval_rule_like.rb | 2 ++
.../components/rules/rule_form_spec.js | 23 +++++++++++++++++++
.../concerns/approval_rule_like_spec.rb | 19 +++++++++++++++
locale/gitlab.pot | 3 +++
7 files changed, 67 insertions(+), 6 deletions(-)
diff --git a/doc/api/merge_request_approvals.md b/doc/api/merge_request_approvals.md
index ed3cb74534ace..a0ae7fbd0352a 100644
--- a/doc/api/merge_request_approvals.md
+++ b/doc/api/merge_request_approvals.md
@@ -505,7 +505,7 @@ Supported attributes:
|-------------------------------------|-------------------|----------|-------------|
| `id` | integer or string | Yes | The ID or [URL-encoded path of a project](rest/_index.md#namespaced-paths). |
| `approvals_required` | integer | Yes | The number of required approvals for this rule. |
-| `name` | string | Yes | The name of the approval rule. |
+| `name` | string | Yes | The name of the approval rule. Limited to 1024 characters. |
| `applies_to_all_protected_branches` | boolean | No | Whether to apply the rule to all protected branches. If set to `true`, ignores the value of `protected_branch_ids`. Default is `false`. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/335316) in GitLab 15.3. |
| `group_ids` | Array | No | The IDs of groups as approvers. |
| `protected_branch_ids` | Array | No | The IDs of protected branches to scope the rule by. To identify the ID, [use the API](protected_branches.md#list-protected-branches). |
@@ -649,7 +649,7 @@ Supported attributes:
| `id` | integer or string | Yes | The ID or [URL-encoded path of a project](rest/_index.md#namespaced-paths). |
| `approvals_required` | integer | Yes | The number of required approvals for this rule. |
| `approval_rule_id` | integer | Yes | The ID of a approval rule. |
-| `name` | string | Yes | The name of the approval rule. |
+| `name` | string | Yes | The name of the approval rule. Limited to 1024 characters. |
| `applies_to_all_protected_branches` | boolean | No | Whether to apply the rule to all protected branches. If set to `true`, it ignores the value of `protected_branch_ids`. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/335316) in GitLab 15.3. |
| `group_ids` | Array | No | The IDs of groups as approvers. |
| `protected_branch_ids` | Array | No | The IDs of protected branches to scope the rule by. To identify the ID, [use the API](protected_branches.md#list-protected-branches). |
@@ -1101,7 +1101,7 @@ Supported attributes:
| `id` | integer or string | Yes | The ID or [URL-encoded path of a project](rest/_index.md#namespaced-paths) |
| `approvals_required` | integer | Yes | The number of required approvals for this rule. |
| `merge_request_iid` | integer | Yes | The IID of the merge request. |
-| `name` | string | Yes | The name of the approval rule. |
+| `name` | string | Yes | The name of the approval rule. Limited to 1024 characters. |
| `approval_project_rule_id` | integer | No | The ID of a project's approval rule. |
| `group_ids` | Array | No | The IDs of groups as approvers. |
| `user_ids` | Array | No | The IDs of users as approvers. If you provide both `user_ids` and `usernames`, it adds both lists of users. |
@@ -1193,7 +1193,7 @@ Supported attributes:
| `merge_request_iid` | integer | Yes | The IID of a merge request. |
| `approvals_required` | integer | No | The number of required approvals for this rule. |
| `group_ids` | Array | No | The IDs of groups as approvers. |
-| `name` | string | No | The name of the approval rule. |
+| `name` | string | No | The name of the approval rule. Limited to 1024 characters. |
| `remove_hidden_groups` | boolean | No | Whether to remove hidden groups. |
| `user_ids` | Array | No | The IDs of users as approvers. If you provide both `user_ids` and `usernames`, it adds both lists of users. |
| `usernames` | string array | No | The usernames of approvers for this rule (same as `user_ids` but requires a list of usernames). If you provide both `user_ids` and `usernames`, it adds both lists of users. |
@@ -1389,7 +1389,7 @@ Supported attributes:
|----------------------|-------------------|----------|-------------|
| `id` | integer or string | Yes | The ID or [URL-encoded path of a group](rest/_index.md#namespaced-paths). |
| `approvals_required` | integer | Yes | The number of required approvals for this rule. |
-| `name` | string | Yes | The name of the approval rule. |
+| `name` | string | Yes | The name of the approval rule. Limited to 1024 characters. |
| `group_ids` | array | No | The IDs of groups as approvers. |
| `rule_type` | string | No | The rule type. `any_approver` is a pre-configured default rule with `approvals_required` at `0`. Other rules are `regular` (used for regular [merge request approval rules](../user/project/merge_requests/approvals/rules.md)) and `report_approver`. Don't use this field to build approval rules from the API. The `report_approver` field is used when GitLab creates an approval rule from configured and enabled [merge request approval policies](../user/application_security/policies/merge_request_approval_policies.md). |
| `user_ids` | array | No | The IDs of users as approvers. |
@@ -1469,7 +1469,7 @@ Supported attributes:
| `id` | integer or string | Yes | The ID or [URL-encoded path of a group](rest/_index.md#namespaced-paths). |
| `approvals_required` | string | No | The number of required approvals for this rule. |
| `group_ids` | integer | No | The IDs of users as approvers. |
-| `name` | string | No | The name of the approval rule. |
+| `name` | string | No | The name of the approval rule. Limited to 1024 characters. |
| `rule_type` | array | No | The rule type. `any_approver` is a pre-configured default rule with `approvals_required` at `0`. Other rules are `regular` (used for regular [merge request approval rules](../user/project/merge_requests/approvals/rules.md)) and `report_approver`. Don't use this field to build approval rules from the API. The `report_approver` field is used when GitLab creates an approval rule from configured and enabled [merge request approval policies](../user/application_security/policies/merge_request_approval_policies.md). |
| `user_ids` | array | No | The IDs of groups as approvers. |
diff --git a/ee/app/assets/javascripts/approvals/components/rules/rule_form.vue b/ee/app/assets/javascripts/approvals/components/rules/rule_form.vue
index 2c4b2de2fdb71..5e199c0a740a4 100644
--- a/ee/app/assets/javascripts/approvals/components/rules/rule_form.vue
+++ b/ee/app/assets/javascripts/approvals/components/rules/rule_form.vue
@@ -109,6 +109,19 @@ export default {
if (!this.name) {
return APPROVAL_DIALOG_I18N.validations.ruleNameMissing;
}
+
+ const lengthError = this.serverValidationErrors.find((error) =>
+ /name is too long \(maximum is \d+ characters\)/.test(error),
+ );
+
+ if (lengthError) {
+ const match = lengthError.match(/name is too long \(maximum is (\d+) characters\)/);
+ const maxLength = match[1];
+
+ return sprintf(APPROVAL_DIALOG_I18N.validations.ruleNameTooLong, {
+ number: maxLength,
+ });
+ }
}
return '';
diff --git a/ee/app/assets/javascripts/approvals/constants.js b/ee/app/assets/javascripts/approvals/constants.js
index 643d3a94e55d3..c41dbc997a399 100644
--- a/ee/app/assets/javascripts/approvals/constants.js
+++ b/ee/app/assets/javascripts/approvals/constants.js
@@ -96,6 +96,7 @@ export const APPROVAL_DIALOG_I18N = {
branchesRequired: __('Please select a valid target branch'),
ruleNameTaken: __('Rule name is already taken.'),
ruleNameMissing: __('Please provide a name'),
+ ruleNameTooLong: __('Please enter a name with less than %{number} characters.'),
},
};
diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb
index 36e7e9942747e..3dd8b77e671fd 100644
--- a/ee/app/models/concerns/approval_rule_like.rb
+++ b/ee/app/models/concerns/approval_rule_like.rb
@@ -13,6 +13,7 @@ module ApprovalRuleLike
NEWLY_DETECTED = 'newly_detected'
NEW_NEEDS_TRIAGE = 'new_needs_triage'
NEW_DISMISSED = 'new_dismissed'
+ NAME_LENGTH_LIMIT = 1024
NEWLY_DETECTED_STATUSES = [NEW_NEEDS_TRIAGE, NEW_DISMISSED].freeze
DEFAULT_VULNERABILITY_STATUSES = [NEW_NEEDS_TRIAGE, NEW_DISMISSED].freeze
@@ -46,6 +47,7 @@ module ApprovalRuleLike
}
validates :name, presence: true
+ validates :name, length: { maximum: NAME_LENGTH_LIMIT }, if: -> { new_record? || name_changed? }
validates :approvals_required, numericality: { less_than_or_equal_to: APPROVALS_REQUIRED_MAX, greater_than_or_equal_to: 0 }
validates :report_type, presence: true, if: :report_approver?
diff --git a/ee/spec/frontend/approvals/components/rules/rule_form_spec.js b/ee/spec/frontend/approvals/components/rules/rule_form_spec.js
index a4eea26c3f5ff..c1c85565b879a 100644
--- a/ee/spec/frontend/approvals/components/rules/rule_form_spec.js
+++ b/ee/spec/frontend/approvals/components/rules/rule_form_spec.js
@@ -41,6 +41,16 @@ const nameTakenError = {
},
};
+const nameTooLongError = {
+ response: {
+ data: {
+ message: {
+ name: ['is too long (maximum is 1024 characters)'],
+ },
+ },
+ },
+};
+
Vue.use(Vuex);
describe('EE Approvals RuleForm', () => {
@@ -389,6 +399,19 @@ describe('EE Approvals RuleForm', () => {
expect(nameGroup.props('state')).toBe(false);
expect(nameGroup.props('invalidFeedback')).toBe('Rule name is already taken.');
});
+
+ it('when submitted with a name too long, shows the "ruleNameTooLong" validation', async () => {
+ actions.postRule.mockRejectedValueOnce(nameTooLongError);
+
+ await findForm().trigger('submit');
+ await waitForPromises();
+
+ const nameGroup = findNameValidation();
+ expect(nameGroup.props('state')).toBe(false);
+ expect(nameGroup.props('invalidFeedback')).toBe(
+ 'Please enter a name with less than 1024 characters.',
+ );
+ });
});
it('adds selected approvers on selection', async () => {
diff --git a/ee/spec/models/concerns/approval_rule_like_spec.rb b/ee/spec/models/concerns/approval_rule_like_spec.rb
index 05c69036e7bca..4ef1f0c74a718 100644
--- a/ee/spec/models/concerns/approval_rule_like_spec.rb
+++ b/ee/spec/models/concerns/approval_rule_like_spec.rb
@@ -371,6 +371,25 @@
end
end
end
+
+ context 'name attribute' do
+ it { is_expected.to validate_length_of(:name).is_at_most(described_class::NAME_LENGTH_LIMIT) }
+
+ context 'when name is above the length limit' do
+ it 'does not cause a validation error when the name is not changed' do
+ # Modify the name in the database directly to bypass validations
+ subject.class.where(id: subject.id).update_all(
+ name: 'x' * (described_class::NAME_LENGTH_LIMIT + 10)
+ )
+
+ subject.reload
+ expect(subject.name.length).to be > described_class::NAME_LENGTH_LIMIT
+
+ subject.update!(approvals_required: described_class::APPROVALS_REQUIRED_MAX)
+ expect(subject).to be_valid
+ end
+ end
+ end
end
end
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 8501347d9b707..18cee3afc1f96 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -43609,6 +43609,9 @@ msgstr ""
msgid "Please enter a name for the custom emoji."
msgstr ""
+msgid "Please enter a name with less than %{number} characters."
+msgstr ""
+
msgid "Please enter a non-negative number"
msgstr ""
--
GitLab