Fix race condition, double join, LIKE injection, N+1 queries, and domain API surface in access rules

- Wrap create action in transaction with FOR UPDATE lock to prevent
  concurrent inserts from violating cf:any exclusivity constraints
- Rescue Sequel::UniqueConstraintViolation to return 422 instead of 500
- Join routes table at most once when both route_guids and space_guids
  filters are requested, preventing ambiguous column references
- Escape LIKE metacharacters (% and _) in selector_resource_guids filter
- Replace deprecated routes__column syntax with Sequel[:routes][:column]
- Remove per-row DB existence checks in AccessRulePresenter to eliminate
  N+1 queries; relationship GUIDs are now included directly from selector
- Only include enforce_access_rules and access_rules_scope in domain
  responses when enforce_access_rules is true

Author: rkoster

app/controllers/v3/access_rules_controller.rb

@@ -41,18 +41,28 @@ def create
 
     route = find_and_authorize_route(message.route_guid)
     validate_route_domain(route)
-    validate_selector_exclusivity(route, message.selector)
-
-    access_rule = VCAP::CloudController::RouteAccessRule.new(
-      guid: SecureRandom.uuid,
-      selector: message.selector,
-      route_id: route.id,
-      created_at: Time.now.utc,
-      updated_at: Time.now.utc
-    )
-    access_rule.save
+
+    access_rule = VCAP::CloudController::RouteAccessRule.db.transaction do
+      # Lock existing access rules for this route to prevent concurrent inserts
+      # from violating cf:any exclusivity or uniqueness constraints
+      VCAP::CloudController::RouteAccessRule.where(route_id: route.id).for_update.all
+
+      validate_selector_exclusivity(route, message.selector)
+
+      rule = VCAP::CloudController::RouteAccessRule.new(
+        guid: SecureRandom.uuid,
+        selector: message.selector,
+        route_id: route.id,
+        created_at: Time.now.utc,
+        updated_at: Time.now.utc
+      )
+      rule.save
+      rule
+    end
 
     render status: :created, json: Presenters::V3::AccessRulePresenter.new(access_rule)
+  rescue Sequel::UniqueConstraintViolation
+    unprocessable!("An access rule with selector '#{message.selector}' already exists for this route.")
   end
 
   def update
@@ -126,18 +136,15 @@ def build_dataset(message)
 
     dataset = dataset.where(route_id: readable_route_ids)
 
-    if message.requested?(:route_guids)
+    # Join routes at most once when either route_guids or space_guids is requested
+    if message.requested?(:route_guids) || message.requested?(:space_guids)
       dataset = dataset.
                 join(:routes, id: :route_id).
-                where(routes__guid: message.route_guids).
                 select_all(:route_access_rules)
-    end
 
-    if message.requested?(:space_guids)
-      dataset = dataset.
-                join(:routes, id: :route_id).
-                where(routes__space_id: VCAP::CloudController::Space.where(guid: message.space_guids).select(:id)).
-                select_all(:route_access_rules)
+      dataset = dataset.where(Sequel[:routes][:guid] => message.route_guids) if message.requested?(:route_guids)
+
+      dataset = dataset.where(Sequel[:routes][:space_id] => VCAP::CloudController::Space.where(guid: message.space_guids).select(:id)) if message.requested?(:space_guids)
     end
 
     dataset = dataset.where(guid: message.guids) if message.requested?(:guids)
@@ -146,8 +153,10 @@ def build_dataset(message)
     if message.requested?(:selector_resource_guids)
       # Text-match against selector string for resource GUIDs
       # Handles cf:app:<guid>, cf:space:<guid>, cf:org:<guid>
+      # Escape LIKE metacharacters (% and _) in user-provided values
       conditions = message.selector_resource_guids.map do |guid|
-        Sequel.like(:selector, "%#{guid}%")
+        escaped_guid = guid.gsub('%', '\\%').gsub('_', '\\_')
+        Sequel.like(:selector, "%#{escaped_guid}%")
       end
       dataset = dataset.where(Sequel.|(*conditions))
     end

app/presenters/v3/access_rule_presenter.rb

@@ -38,29 +38,16 @@ def build_relationships
           }
 
           # Extract resource GUID from selector and populate read-only relationships
-          # Only include the guid in data if the resource actually exists
+          # The guid is included as-is without per-row existence checks to avoid N+1 queries.
+          # Use ?include=selector_resource to get full resource details with batch loading.
           selector_match = access_rule.selector.match(/\Acf:(app|space|org):([0-9a-f-]+)\z/)
           if selector_match
             resource_type = selector_match[1]
             resource_guid = selector_match[2]
 
-            case resource_type
-            when 'app'
-              app_exists = VCAP::CloudController::AppModel.where(guid: resource_guid).any?
-              relationships[:app] = { data: app_exists ? { guid: resource_guid } : nil }
-              relationships[:space] = { data: nil }
-              relationships[:organization] = { data: nil }
-            when 'space'
-              space_exists = VCAP::CloudController::Space.where(guid: resource_guid).any?
-              relationships[:app] = { data: nil }
-              relationships[:space] = { data: space_exists ? { guid: resource_guid } : nil }
-              relationships[:organization] = { data: nil }
-            when 'org'
-              org_exists = VCAP::CloudController::Organization.where(guid: resource_guid).any?
-              relationships[:app] = { data: nil }
-              relationships[:space] = { data: nil }
-              relationships[:organization] = { data: org_exists ? { guid: resource_guid } : nil }
-            end
+            relationships[:app] = { data: resource_type == 'app' ? { guid: resource_guid } : nil }
+            relationships[:space] = { data: resource_type == 'space' ? { guid: resource_guid } : nil }
+            relationships[:organization] = { data: resource_type == 'org' ? { guid: resource_guid } : nil }
           else
             # cf:any or malformed - all relationships are null
             relationships[:app] = { data: nil }

app/presenters/v3/domain_presenter.rb

@@ -20,16 +20,14 @@ def initialize(
     end
 
     def to_hash
-      {
+      hash = {
         guid: domain.guid,
         created_at: domain.created_at,
         updated_at: domain.updated_at,
         name: domain.name,
         internal: domain.internal,
         router_group: hashified_router_group(domain.router_group_guid),
         supported_protocols: domain.protocols,
-        enforce_access_rules: domain.enforce_access_rules || false,
-        access_rules_scope: domain.access_rules_scope,
         relationships: {
           organization: {
             data: owning_org_guid
@@ -44,6 +42,13 @@ def to_hash
         },
         links: build_links
       }
+
+      if domain.enforce_access_rules
+        hash[:enforce_access_rules] = true
+        hash[:access_rules_scope] = domain.access_rules_scope
+      end
+
+      hash
     end
 
     private

spec/request/access_rules_spec.rb

@@ -206,6 +206,24 @@ def expected_rule_json(rule)
         expect(last_response.body).to include('Selector')
       end
     end
+
+    context 'when a concurrent request creates the same selector (UniqueConstraintViolation)' do
+      it 'returns 422 instead of 500' do
+        # Simulate a race condition where the DB unique constraint catches the duplicate
+        # after validation passes but before the insert commits
+        allow_any_instance_of(VCAP::CloudController::RouteAccessRule).to receive(:save).and_raise(
+          Sequel::UniqueConstraintViolation.new('UNIQUE constraint failed: route_access_rules.route_id, route_access_rules.selector')
+        )
+
+        post '/v3/access_rules', {
+          selector: "cf:app:#{valid_uuid}",
+          relationships: { route: { data: { guid: mtls_route.guid } } }
+        }.to_json, admin_header
+
+        expect(last_response.status).to eq(422)
+        expect(last_response.body).to include('already exists')
+      end
+    end
   end
 
   describe 'GET /v3/access_rules/:guid' do
@@ -345,6 +363,52 @@ def expected_rule_json(rule)
       end
     end
 
+    describe 'filtering by both route_guids and space_guids' do
+      let(:other_org) { VCAP::CloudController::Organization.make }
+      let(:other_space) { VCAP::CloudController::Space.make(organization: other_org) }
+      let(:other_mtls_domain) do
+        VCAP::CloudController::PrivateDomain.make(
+          owning_organization: other_org,
+          enforce_access_rules: true,
+          access_rules_scope: 'space'
+        )
+      end
+      let(:other_route) { VCAP::CloudController::Route.make(space: other_space, domain: other_mtls_domain) }
+      let!(:rule_in_other_space) do
+        VCAP::CloudController::RouteAccessRule.create(
+          guid: SecureRandom.uuid,
+          selector: 'cf:any',
+          route_id: other_route.id
+        )
+      end
+
+      before do
+        other_org.add_user(user)
+        other_space.add_developer(user)
+      end
+
+      it 'returns results matching both route_guids and space_guids without ambiguous column errors' do
+        get "/v3/access_rules?route_guids=#{mtls_route.guid}&space_guids=#{space.guid}", nil, admin_header
+
+        expect(last_response.status).to eq(200)
+        parsed = Oj.load(last_response.body)
+        guids = parsed['resources'].pluck('guid')
+        expect(guids).to include(rule1.guid)
+        expect(guids).not_to include(rule_in_other_space.guid)
+      end
+    end
+
+    describe 'filtering by selector_resource_guids' do
+      it 'does not match unintended rows when guid contains LIKE wildcards' do
+        get '/v3/access_rules?selector_resource_guids=%25', nil, admin_header
+
+        expect(last_response.status).to eq(200)
+        parsed = Oj.load(last_response.body)
+        # Should not match all rows via SQL wildcard; % is escaped
+        expect(parsed['resources'].length).to eq(0)
+      end
+    end
+
     context 'with include=selector_resource' do
       let!(:frontend_app) { VCAP::CloudController::AppModel.make(space: space, name: 'frontend-app') }
       let!(:other_space) { VCAP::CloudController::Space.make(organization: org, name: 'other-space') }

spec/unit/presenters/v3/domain_presenter_spec.rb

@@ -238,6 +238,43 @@ module VCAP::CloudController::Presenters::V3
           end
         end
 
+        context 'when the domain has enforce_access_rules enabled' do
+          let(:org) { VCAP::CloudController::Organization.make }
+          let(:domain) do
+            VCAP::CloudController::PrivateDomain.make(
+              name: 'mtls.domain.com',
+              owning_organization: org,
+              enforce_access_rules: true,
+              access_rules_scope: 'space'
+            )
+          end
+
+          it 'includes enforce_access_rules and access_rules_scope in the output' do
+            expect(subject[:enforce_access_rules]).to be(true)
+            expect(subject[:access_rules_scope]).to eq('space')
+          end
+        end
+
+        context 'when the domain does not have enforce_access_rules enabled' do
+          let(:domain) do
+            VCAP::CloudController::SharedDomain.make(
+              name: 'regular.domain.com'
+            )
+          end
+
+          let(:routing_api_client) { instance_double(VCAP::CloudController::RoutingApi::Client) }
+
+          before do
+            allow_any_instance_of(CloudController::DependencyLocator).to receive(:routing_api_client).and_return(routing_api_client)
+            allow(routing_api_client).to receive_messages(enabled?: true, router_group: nil)
+          end
+
+          it 'does not include enforce_access_rules or access_rules_scope in the output' do
+            expect(subject).not_to have_key(:enforce_access_rules)
+            expect(subject).not_to have_key(:access_rules_scope)
+          end
+        end
+
         context 'and the routing API is disabled' do
           before do
             allow(routing_api_client).to receive(:enabled?).and_return false