From 63050044a782dcddb074dda88ac7295e87bcd4a1 Mon Sep 17 00:00:00 2001 From: Wei Quan Date: Thu, 24 Sep 2026 10:57:28 +0200 Subject: [PATCH] Require authentication when changing broker URL origin --- app/actions/v3/service_broker_update.rb | 19 ++++- spec/request/service_brokers_spec.rb | 81 +++++++++++++++++++ .../actions/v3/service_broker_update_spec.rb | 54 +++++++++++++ 3 files changed, 152 insertions(+), 2 deletions(-) diff --git a/app/actions/v3/service_broker_update.rb b/app/actions/v3/service_broker_update.rb index 1713a61b267..0e632432d48 100644 --- a/app/actions/v3/service_broker_update.rb +++ b/app/actions/v3/service_broker_update.rb @@ -2,6 +2,7 @@ require 'actions/metadata_update' require 'jobs/enqueuer' require 'jobs/queues' +require 'uri' module VCAP::CloudController module V3 @@ -30,6 +31,12 @@ def update_sync end def enqueue_update + raise InvalidServiceBroker.new('Cannot update a broker when other operation is already in progress') if broker.in_transitional_state? + + if message.requested?(:url) && !message.requested?(:authentication) && origin_changed? + raise InvalidServiceBroker.new('Authentication must be provided when changing the broker URL origin') + end + params = {} params[:broker_url] = message.url if message.requested?(:url) params[:authentication] = message.authentication.to_json if message.requested?(:authentication) @@ -40,8 +47,6 @@ def enqueue_update params[:name] = message.name end - raise InvalidServiceBroker.new('Cannot update a broker when other operation is already in progress') if broker.in_transitional_state? - pollable_job = nil previous_broker_state = broker.state ServiceBrokerUpdateRequest.db.transaction do @@ -71,6 +76,16 @@ def enqueue_update def unique_name! raise InvalidServiceBroker.new('Name must be unique') end + + def origin_changed? + new_uri = URI.parse(message.url) + old_uri = URI.parse(broker.broker_url) + new_uri.scheme != old_uri.scheme || + new_uri.host.to_s.downcase != old_uri.host.to_s.downcase || + new_uri.port != old_uri.port + rescue URI::InvalidURIError + true + end end end end diff --git a/spec/request/service_brokers_spec.rb b/spec/request/service_brokers_spec.rb index 17fd58f0783..929b2aefe09 100644 --- a/spec/request/service_brokers_spec.rb +++ b/spec/request/service_brokers_spec.rb @@ -667,6 +667,87 @@ def expect_empty_list(user_headers) end end + context 'for a space-scoped broker' do + let!(:broker) do + create(:service_broker, + name: 'space-scoped broker', + broker_url: 'http://example.org/old-broker-url', + auth_username: 'old-admin', + auth_password: 'not-welcome', + state: VCAP::CloudController::ServiceBrokerStateEnum::AVAILABLE, + space: space) + end + let(:space_developer_headers) do + org.add_user(user) + space.add_developer(user) + headers_for(user) + end + + context 'when changing the broker URL origin without providing authentication' do + it 'returns 422 and does not enqueue an update job' do + patch "/v3/service_brokers/#{broker.guid}", { url: 'http://attacker.example.com/broker' }.to_json, space_developer_headers + + expect_error( + status: 422, + error: 'UnprocessableEntity', + description: 'Authentication must be provided when changing the broker URL origin' + ) + + expect(VCAP::CloudController::PollableJobModel.count).to eq(0) + expect(broker.reload.broker_url).to eq('http://example.org/old-broker-url') + expect(broker.auth_username).to eq('old-admin') + expect(broker.auth_password).to eq('not-welcome') + expect(broker.state).to eq(VCAP::CloudController::ServiceBrokerStateEnum::AVAILABLE) + end + end + + context 'when changing only the broker URL path on the same origin without providing authentication' do + before do + stub_request(:get, 'http://example.org/different-path/v2/catalog'). + to_return(status: 200, body: catalog(space_broker_id).to_json, headers: {}) + end + + it 'accepts the update' do + patch "/v3/service_brokers/#{broker.guid}", { url: 'http://example.org/different-path' }.to_json, space_developer_headers + + expect(last_response).to have_status_code(202) + end + end + end + + context 'for a global broker' do + context 'when changing the broker URL origin without providing authentication' do + it 'returns 422 and does not enqueue an update job' do + patch "/v3/service_brokers/#{broker.guid}", { url: 'http://attacker.example.com/broker' }.to_json, admin_headers + + expect_error( + status: 422, + error: 'UnprocessableEntity', + description: 'Authentication must be provided when changing the broker URL origin' + ) + + expect(VCAP::CloudController::PollableJobModel.count).to eq(0) + expect(broker.reload.broker_url).to eq('http://example.org/broker-url') + expect(broker.auth_username).to eq('admin') + expect(broker.auth_password).to eq('welcome') + expect(broker.state).to eq(VCAP::CloudController::ServiceBrokerStateEnum::AVAILABLE) + end + end + + context 'when changing only the broker URL path on the same origin without providing authentication' do + before do + stub_request(:get, 'http://example.org/different-path/v2/catalog'). + to_return(status: 200, body: { services: [] }.to_json, headers: {}) + end + + it 'accepts the update' do + patch "/v3/service_brokers/#{broker.guid}", { url: 'http://example.org/different-path' }.to_json, admin_headers + + expect(last_response).to have_status_code(202) + end + end + end + context 'when there is a sql validation error while syncing' do let!(:service_offering) { create(:service, service_broker: broker, unique_id: global_broker_id + '-1') } let!(:service_plan) { create(:service_plan, service: service_offering, name: 'plan_name-1', unique_id: Sham.guid) } diff --git a/spec/unit/actions/v3/service_broker_update_spec.rb b/spec/unit/actions/v3/service_broker_update_spec.rb index 7253ccb11e7..ceda57a33d8 100644 --- a/spec/unit/actions/v3/service_broker_update_spec.rb +++ b/spec/unit/actions/v3/service_broker_update_spec.rb @@ -206,6 +206,15 @@ module CloudController end end + context 'and the update also changes the broker URL origin without providing authentication' do + let(:state) { ServiceBrokerStateEnum::SYNCHRONIZING } + let(:request) { { url: 'http://attacker.example.com/broker' } } + + it 'raises the transitional state error first' do + expect { action.enqueue_update }.to raise_error(V3::ServiceBrokerUpdate::InvalidServiceBroker, expected_error_message) + end + end + context 'If broker is in DELETE_IN_PROGRESS state' do let(:state) { ServiceBrokerStateEnum::DELETE_IN_PROGRESS } @@ -233,6 +242,51 @@ module CloudController end end + context 'when changing the broker URL origin without providing authentication' do + let(:request) { { url: 'http://attacker.example.com/broker' } } + + it 'raises an InvalidServiceBroker error' do + expect { action.enqueue_update }.to raise_error( + V3::ServiceBrokerUpdate::InvalidServiceBroker, + 'Authentication must be provided when changing the broker URL origin' + ) + end + end + + context 'when changing the broker URL origin together with authentication' do + let(:request) do + { + url: 'http://attacker.example.com/broker', + authentication: { + credentials: { + username: 'new-admin', + password: 'welcome' + } + } + } + end + + it 'does not raise an error' do + expect { action.enqueue_update }.not_to raise_error + end + end + + context 'when changing only the broker URL path (same origin)' do + let(:request) { { url: 'http://example.org/different-path' } } + + it 'does not raise an error' do + expect { action.enqueue_update }.not_to raise_error + end + end + + context 'when changing only the broker URL path with different hostname casing (same origin)' do + let(:request) { { url: 'http://EXAMPLE.ORG/different-path' } } + + it 'does not raise an error' do + expect { action.enqueue_update }.not_to raise_error + end + end + context 'legacy service brokers' do let!(:state) { '' } # broker creating from V2 endpoint will not have a state