Remove support for Aliases/Anchors in Manifests

- move yaml body parsing to the controller action

[#175290617]

Co-authored-by: Weyman Fung <wfung@pivotal.io>
Co-authored-by: Seth Boyles <sboyles@pivotal.io>

Author: weymanf

app/controllers/v3/app_manifests_controller.rb

@@ -79,10 +79,10 @@ def request_content_type_is_yaml?
   end
 
   def parsed_app_manifest_params
-    parsed_application = params[:body]['applications'] && params[:body]['applications'].first
+    parsed_application = parsed_yaml['applications'] && parsed_yaml['applications'].first
 
     raise invalid_request!('Invalid app manifest') unless parsed_application.present?
 
-    parsed_application.to_unsafe_h
+    parsed_application
   end
 end

app/controllers/v3/application_controller.rb

@@ -30,6 +30,10 @@ def bad_request!(message)
     raise CloudController::Errors::ApiError.new_from_details('BadRequest', message)
   end
 
+  def message_parse_error!(message)
+    raise CloudController::Errors::ApiError.new_from_details('MessageParseError', message)
+  end
+
   def service_unavailable!(message)
     raise CloudController::Errors::ApiError.new_from_details('ServiceUnavailable', message)
   end
@@ -80,6 +84,17 @@ def unmunged_body
     JSON.parse(request.body.string)
   end
 
+  def parsed_yaml
+    return @parsed_yaml if @parsed_yaml
+
+    allow_yaml_aliases = false
+    yaml = YAML.safe_load(request.body.string, [], [], allow_yaml_aliases)
+    message_parse_error!('invalid request body') if !yaml.is_a? Hash
+    @parsed_yaml = yaml
+  rescue Psych::BadAlias
+    bad_request!('Manifest does not support Anchors and Aliases')
+  end
+
   def roles
     VCAP::CloudController::SecurityContext.roles
   end

app/controllers/v3/space_manifests_controller.rb

@@ -15,7 +15,7 @@ def apply_manifest
     space_not_found! unless space && permission_queryer.can_read_from_space?(space.guid, space.organization.guid)
     unauthorized! unless permission_queryer.can_write_to_space?(space.guid)
 
-    messages = parsed_app_manifests.map(&:to_unsafe_h).map { |app_manifest| NamedAppManifestMessage.create_from_yml(app_manifest) }
+    messages = parsed_app_manifests.map { |app_manifest| NamedAppManifestMessage.create_from_yml(app_manifest) }
     errors = messages.each_with_index.flat_map { |message, i| errors_for_message(message, i) }
     compound_error!(errors) unless errors.empty?
 
@@ -49,7 +49,7 @@ def diff_manifest
 
     parsed_manifests = parsed_app_manifests.map(&:to_hash)
 
-    messages = parsed_app_manifests.map(&:to_unsafe_h).map { |app_manifest| NamedAppManifestMessage.create_from_yml(app_manifest) }
+    messages = parsed_app_manifests.map { |app_manifest| NamedAppManifestMessage.create_from_yml(app_manifest) }
     errors = messages.each_with_index.flat_map { |message, i| errors_for_message(message, i) }
     compound_error!(errors) unless errors.empty?
 
@@ -92,13 +92,13 @@ def request_content_type_is_yaml?
   end
 
   def check_version_is_supported!
-    version = params[:body]['version']
+    version = parsed_yaml['version']
     raise unprocessable!('Unsupported manifest schema version. Currently supported versions: [1].') unless !version || version == 1
   end
 
   def parsed_app_manifests
     check_version_is_supported!
-    parsed_applications = params[:body].permit!['applications']
+    parsed_applications = parsed_yaml['applications']
     raise unprocessable!("Cannot parse manifest with no 'applications' field.") unless parsed_applications.present?
 
     parsed_applications

config/application.rb

@@ -2,18 +2,6 @@
 
 class Application < ::Rails::Application
   config.exceptions_app = self.routes
-
-  # For Rails 5 / Rack 2 - this is how to add a new parser
-  original_parsers = ActionDispatch::Request.parameter_parsers
-
-  allow_yaml_aliases = true
-  yaml_parser = lambda { |body| YAML.safe_load(body, [], [], allow_yaml_aliases).with_indifferent_access }
-  new_parsers = original_parsers.merge({
-    Mime::Type.lookup('application/x-yaml') => yaml_parser,
-    Mime::Type.lookup('text/yaml') => yaml_parser,
-  })
-  ActionDispatch::Request.parameter_parsers = new_parsers
-
   config.middleware.delete ActionDispatch::Session::CookieStore
   config.middleware.delete ActionDispatch::Cookies
   config.middleware.delete ActionDispatch::Flash

docs/v3/source/includes/resources/manifests/_object.md.erb

@@ -1,5 +1,7 @@
 A manifest is a method for applying bulk configurations to apps and their underlying processes.
 
+Manifests are in the YAML format. However, anchors and aliases are not supported.
+
 ### The manifest schema
 
 ```

spec/request/app_manifests_spec.rb

@@ -244,19 +244,12 @@
         YML
       end
 
-      it 'accepts yaml with anchors' do
+      it 'does NOT accept yaml with anchors' do
         post "/v3/apps/#{app_model.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header)
 
-        expect(last_response.status).to eq(202)
-        job_guid = VCAP::CloudController::PollableJobModel.last.guid
-        expect(last_response.headers['Location']).to match(%r(/v3/jobs/#{job_guid}))
-
-        Delayed::Worker.new.work_off
-        expect(VCAP::CloudController::PollableJobModel.find(guid: job_guid)).to be_complete
-
-        web_process = app_model.web_processes.first
-        expect(web_process.memory).to eq(321)
-        expect(web_process.disk_quota).to eq(321)
+        expect(last_response.status).to eq(400)
+        parsed_response = MultiJson.load(last_response.body)
+        expect(parsed_response['errors'].first['detail']).to eq('Bad request: Manifest does not support Anchors and Aliases')
       end
     end
 

spec/request/space_manifests_spec.rb

@@ -458,6 +458,28 @@
         expect(other_events.map(&:type)).to eq(['audit.app.apply_manifest',])
       end
     end
+
+    context 'yaml anchors' do
+      let(:yml_manifest) do
+        <<~YML
+          ---
+          applications:
+          - name: blah
+            processes:
+            - type: web
+              memory: &default_value 321M
+              disk_quota: *default_value
+        YML
+      end
+
+      it 'does NOT accept yaml with anchors' do
+        post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header)
+
+        expect(last_response.status).to eq(400)
+        parsed_response = MultiJson.load(last_response.body)
+        expect(parsed_response['errors'].first['detail']).to eq('Bad request: Manifest does not support Anchors and Aliases')
+      end
+    end
   end
 
   describe 'POST /v3/spaces/:guid/manifest_diff' do
@@ -713,5 +735,32 @@
         expect(last_response).to have_status_code(404)
       end
     end
+
+    context 'yaml anchors' do
+      let(:yml_manifest) do
+        <<~YML
+          ---
+          applications:
+          - name: blah
+            processes:
+            - type: web
+              memory: &default_value 321M
+              disk_quota: *default_value
+        YML
+      end
+
+      before do
+        space.organization.add_user(user)
+        space.add_developer(user)
+      end
+
+      it 'does NOT accept yaml with anchors' do
+        post "/v3/spaces/#{space.guid}/manifest_diff", yml_manifest, yml_headers(user_header)
+
+        expect(last_response.status).to eq(400)
+        parsed_response = MultiJson.load(last_response.body)
+        expect(parsed_response['errors'].first['detail']).to eq('Bad request: Manifest does not support Anchors and Aliases')
+      end
+    end
   end
 end