diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index f58aa6d21..a55293b50 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -423,7 +423,7 @@ test:browser:integration:api_client_php: ZAMMAD_PHP_API_CLIENT_UNIT_TESTS_PASSWORD: "test" script: - RAILS_ENV=test bundle exec rake db:create - - bundle exec rake zammad:ci:test:start zammad:setup:auto_wizard + - RAILS_ENV=test bundle exec rake zammad:ci:test:start zammad:setup:auto_wizard - git clone https://github.com/zammad/zammad-api-client-php.git - php -r "copy('https://getcomposer.org/installer', 'composer-setup.php');" - php composer-setup.php --install-dir=/usr/local/bin diff --git a/app/controllers/application_controller/handles_errors.rb b/app/controllers/application_controller/handles_errors.rb index 060198964..819af4c88 100644 --- a/app/controllers/application_controller/handles_errors.rb +++ b/app/controllers/application_controller/handles_errors.rb @@ -75,8 +75,18 @@ module ApplicationController::HandlesErrors data[:error_human] = data[:error] end - if Rails.env.production? && data[:error_human].present? - data[:error] = data.delete(:error_human) + if Rails.env.production? + if data[:error_human].present? + data[:error] = data.delete(:error_human) + else + # We want to avoid leaking of internal information but also want the user + # to give the administrator a reference to find the cause of the error. + # Therefore we generate a one time unique error ID that can be used to + # search the logs and find the actual error message. + error_code_prefix = "Error ID #{SecureRandom.urlsafe_base64(6)}:" + Rails.logger.error "#{error_code_prefix} #{data[:error]}" + data[:error] = "#{error_code_prefix} Please contact your administrator." + end end data end diff --git a/app/models/object_manager/attribute.rb b/app/models/object_manager/attribute.rb index 1fe6ce62d..8fa028e60 100644 --- a/app/models/object_manager/attribute.rb +++ b/app/models/object_manager/attribute.rb @@ -910,34 +910,49 @@ is certain attribute used by triggers, overviews or schedulers def check_name return if !name - raise 'Name can\'t get used, *_id and *_ids are not allowed' if name.match?(/.+?_(id|ids)$/i) - raise 'Spaces in name are not allowed' if name.match?(/\s/) - raise 'Only letters from a-z, numbers from 0-9, and _ are allowed' if !name.match?(/^[a-z0-9_]+$/) - raise 'At least one letters is needed' if !name.match?(/[a-z]/) + if name.match?(/.+?_(id|ids)$/i) + errors.add(:name, "can't get used because *_id and *_ids are not allowed") + end + if name.match?(/\s/) + errors.add(:name, 'spaces are not allowed') + end + if !name.match?(/^[a-z0-9_]+$/) + errors.add(:name, 'Only letters from a-z because numbers from 0-9 and _ are allowed') + end + if !name.match?(/[a-z]/) + errors.add(:name, 'At least one letters is needed') + end # do not allow model method names as attributes reserved_words = %w[destroy true false integer select drop create alter index table varchar blob date datetime timestamp url icon initials avatar permission validate subscribe unsubscribe translate search _type _doc _id id] - raise "#{name} is a reserved word, please choose a different one" if name.match?(/^(#{reserved_words.join('|')})$/) + if name.match?(/^(#{reserved_words.join('|')})$/) + errors.add(:name, "#{name} is a reserved word! (1)") + end # fixes issue #2236 - Naming an attribute "attribute" causes ActiveRecord failure begin ObjectLookup.by_id(object_lookup_id).constantize.instance_method_already_implemented? name rescue ActiveRecord::DangerousAttributeError - raise "#{name} is a reserved word, please choose a different one" + errors.add(:name, "#{name} is a reserved word! (2)") end record = object_lookup.name.constantize.new - return true if !record.respond_to?(name.to_sym) - raise "#{name} already exists!" if record.attributes.key?(name) && new_record? - return true if record.attributes.key?(name) + if record.respond_to?(name.to_sym) && record.attributes.key?(name) && new_record? + errors.add(:name, "#{name} already exists!") + end - raise "#{name} is a reserved word, please choose a different one" + if errors.present? + raise ActiveRecord::RecordInvalid, self + end + + true end def check_editable return if editable - raise 'Attribute not editable!' + errors.add(:name, 'Attribute not editable!') + raise ActiveRecord::RecordInvalid, self end private diff --git a/spec/models/object_manager/attribute_spec.rb b/spec/models/object_manager/attribute_spec.rb index 3e973fac0..edbdec6f4 100644 --- a/spec/models/object_manager/attribute_spec.rb +++ b/spec/models/object_manager/attribute_spec.rb @@ -66,14 +66,14 @@ RSpec.describe ObjectManager::Attribute, type: :model do it 'rejects ActiveRecord reserved word "attribute"' do expect do described_class.add attributes_for :object_manager_attribute_text, name: 'attribute' - end.to raise_error 'attribute is a reserved word, please choose a different one' + end.to raise_error(ActiveRecord::RecordInvalid, 'Validation failed: Name attribute is a reserved word! (2)') end %w[destroy true false integer select drop create alter index table varchar blob date datetime timestamp url icon initials avatar permission validate subscribe unsubscribe translate search _type _doc _id id].each do |reserved_word| it "rejects Zammad reserved word '#{reserved_word}'" do expect do described_class.add attributes_for :object_manager_attribute_text, name: reserved_word - end.to raise_error "#{reserved_word} is a reserved word, please choose a different one" + end.to raise_error(ActiveRecord::RecordInvalid, /is a reserved word! \(1\)/) end end @@ -81,7 +81,7 @@ RSpec.describe ObjectManager::Attribute, type: :model do it "rejects word '#{reserved_word}' which is used for database references" do expect do described_class.add attributes_for :object_manager_attribute_text, name: reserved_word - end.to raise_error "Name can't get used, *_id and *_ids are not allowed" + end.to raise_error(ActiveRecord::RecordInvalid, "Validation failed: Name can't get used because *_id and *_ids are not allowed") end end diff --git a/spec/requests/error_spec.rb b/spec/requests/error_spec.rb index e74f78bec..b58f03733 100644 --- a/spec/requests/error_spec.rb +++ b/spec/requests/error_spec.rb @@ -7,7 +7,18 @@ RSpec.describe 'Error handling', type: :request do it { expect(response).to have_http_status(http_status) } it { expect(json_response).to be_a_kind_of(Hash) } - it { expect(json_response['error']).to eq(message) } + + it do + # There is a special case where we mask technical errors and return + # a random error code that can be easily found in the logs by an + # administrator. However, this makes it hard to check for the exact error + # message. Therefore we only check for the substring in this particular case + if message == 'Please contact your administrator' + expect(json_response['error']).to include(message) + else + expect(json_response['error']).to eq(message) + end + end end shared_examples 'HTML response format' do @@ -20,6 +31,39 @@ RSpec.describe 'Error handling', type: :request do it { expect(response.body).to include(message) } end + context 'error with confidential message is raised' do + + let(:admin_user) { create(:admin_user, groups: Group.all) } + let!(:ticket) { create(:ticket) } + let(:invalid_group_id) { 99_999 } + let(:message) { 'Please contact your administrator' } + let(:http_status) { :unprocessable_entity } + + before do + # fake production ENV to enable error hiding + env = double( + production?: true, + test?: false, + development?: false + ) + allow(::Rails).to receive(:env).and_return(env) + + authenticated_as(admin_user) + put "/api/v1/tickets/#{ticket.id}?all=true", params: { group_id: invalid_group_id }, as: as + end + + context 'requesting JSON' do + include_examples 'JSON response format' + end + + context 'requesting HTML' do + let(:title) { '422: Unprocessable Entity' } + let(:headline) { '422: The change you wanted was rejected.' } + + include_examples 'HTML response format' + end + end + context 'URL route does not exist' do before do diff --git a/test/integration/object_manager_test.rb b/test/integration/object_manager_test.rb index ed2166e01..528846cf1 100644 --- a/test/integration/object_manager_test.rb +++ b/test/integration/object_manager_test.rb @@ -73,7 +73,7 @@ class ObjectManagerTest < ActiveSupport::TestCase assert_not(attribute1) # create invalid attributes - assert_raises(RuntimeError) do + assert_raises(ActiveRecord::RecordInvalid) do ObjectManager::Attribute.add( object: 'Ticket', name: 'test2_id', @@ -91,7 +91,7 @@ class ObjectManagerTest < ActiveSupport::TestCase updated_by_id: 1, ) end - assert_raises(RuntimeError) do + assert_raises(ActiveRecord::RecordInvalid) do ObjectManager::Attribute.add( object: 'Ticket', name: 'test3_ids', @@ -286,7 +286,7 @@ class ObjectManagerTest < ActiveSupport::TestCase ) assert_equal(false, ObjectManager::Attribute.pending_migration?) - assert_raises(RuntimeError) do + assert_raises(ActiveRecord::RecordInvalid) do ObjectManager::Attribute.add( object: 'Ticket', name: 'test13|', @@ -307,7 +307,7 @@ class ObjectManagerTest < ActiveSupport::TestCase end assert_equal(false, ObjectManager::Attribute.pending_migration?) - assert_raises(RuntimeError) do + assert_raises(ActiveRecord::RecordInvalid) do ObjectManager::Attribute.add( object: 'Ticket', name: 'test14!', @@ -328,7 +328,7 @@ class ObjectManagerTest < ActiveSupport::TestCase end assert_equal(false, ObjectManager::Attribute.pending_migration?) - assert_raises(RuntimeError) do + assert_raises(ActiveRecord::RecordInvalid) do ObjectManager::Attribute.add( object: 'Ticket', name: 'test15รค', @@ -370,7 +370,7 @@ class ObjectManagerTest < ActiveSupport::TestCase end assert_equal(false, ObjectManager::Attribute.pending_migration?) - assert_raises(RuntimeError) do + assert_raises(ActiveRecord::RecordInvalid) do ObjectManager::Attribute.add( object: 'Ticket', name: 'delete', @@ -391,7 +391,7 @@ class ObjectManagerTest < ActiveSupport::TestCase assert_equal(false, ObjectManager::Attribute.pending_migration?) attribute_count = ObjectManager::Attribute.count - assert_raises(RuntimeError) do + assert_raises(ActiveRecord::RecordInvalid) do ObjectManager::Attribute.add( object: 'Ticket', name: 'updated_at', @@ -413,7 +413,7 @@ class ObjectManagerTest < ActiveSupport::TestCase end assert_equal(attribute_count, ObjectManager::Attribute.count) - assert_raises(RuntimeError) do + assert_raises(ActiveRecord::RecordInvalid) do ObjectManager::Attribute.add( object: 'Ticket', name: 'updated_AT',