Relax escaping of values to enable limited access to the shell for AppDynamnics config (#911)

[Previous PRs escaped configuration values for AppD](#870). This was done to support things like names with spaces and other characters that, if used, would result in a broken start command.

The PR broke a use case which was documented for the node or tier name. In some cases, you may want to set the node or tier name to a dynamic value that is loaded at runtime so that you can incorporate things like the application instance index. For example `DCX:$(echo $VCAP_APPLICATION | jq -r '.application_name'):$(echo $VCAP_APPLICATION | jq -r '.instance_index')`.

This new PR will use `Shellwords.escape(..)` on the value for all properties unless that property value contains what looks like a subshell `$(..)` or environment variable `${..}` reference. If it looks like a subshell or env variable is being referenced, we will not escape but just wrap the value in escaped quotes. We wrap it in escaped quotes in case the shell variable or subshell returns something which includes spaces. This is not perfect though, and you need to be careful if using subshell/env variables, you should ensure the output is properly escaped.

For example:
- `DCX:$(echo $VCAP_APPLICATION | jq -r '.application_name'):$(echo $VCAP_APPLICATION | jq -r '.instance_index')`
- `$(echo 'Hello world!') and stuff` becomes `\"$(echo 'Hello world!') and stuff\"`
- `--> ${SOME_VAR} <--` becomes `\"--> ${SOME_VAR} <--\"`

Signed-off-by: Daniel Mikusa <[email protected]>

Author: Daniel Mikusa

docs/framework-app_dynamics_agent.md

@@ -26,7 +26,7 @@ When binding AppDynamics using a user-provided service, it must have name or tag
 | `node-name` | (Optional) the application's node name
 | `tier-name` | (Optional) the application's tier name
 
-To provide more complex values such as the `tier-name`, using the interactive mode when creating a user-provided service will manage the character escaping automatically. For example, the default `tier-name` could be set with a value of `Tier-$(expr "$VCAP_APPLICATION" : '.*instance_index[": ]*\([[:digit:]]*\).*')` to calculate a value from the Cloud Foundry instance index.
+To provide more complex values such as the `tier-name`, using the interactive mode when creating a user-provided service will manage the character escaping automatically. For example, the default `tier-name` could be set with a value of `Tier-$(expr "${VCAP_APPLICATION}" : '.*instance_index[": ]*\([[:digit:]]*\).*')` to calculate a value from the Cloud Foundry instance index.
 
 **Note:** Some credentials were previously marked as "(Optional)" as requirements have changed across versions of the AppDynamics agent.  Please see the [AppDynamics Java Agent Configuration Properties][] for the version of the agent used by your application for more details.
 

lib/java_buildpack/framework/app_dynamics_agent.rb

@@ -86,35 +86,35 @@ def supports?
       private_constant :FILTER
 
       def application_name(java_opts, credentials)
-        name = Shellwords.escape(@application.details['application_name'])
+        name = escape(@application.details['application_name'])
         name = @configuration['default_application_name'] if @configuration['default_application_name']
-        name = Shellwords.escape(credentials['application-name']) if credentials['application-name']
+        name = escape(credentials['application-name']) if credentials['application-name']
 
         java_opts.add_system_property('appdynamics.agent.applicationName', name.to_s)
       end
 
       def account_access_key(java_opts, credentials)
         account_access_key = credentials['account-access-key'] || credentials.dig('account-access-secret', 'secret')
-        account_access_key = Shellwords.escape(account_access_key)
+        account_access_key = escape(account_access_key)
 
         java_opts.add_system_property 'appdynamics.agent.accountAccessKey', account_access_key if account_access_key
       end
 
       def account_name(java_opts, credentials)
         account_name = credentials['account-name']
-        java_opts.add_system_property 'appdynamics.agent.accountName', Shellwords.escape(account_name) if account_name
+        java_opts.add_system_property 'appdynamics.agent.accountName', escape(account_name) if account_name
       end
 
       def host_name(java_opts, credentials)
         host_name = credentials['host-name']
         raise "'host-name' credential must be set" unless host_name
 
-        java_opts.add_system_property 'appdynamics.controller.hostName', Shellwords.escape(host_name)
+        java_opts.add_system_property 'appdynamics.controller.hostName', escape(host_name)
       end
 
       def node_name(java_opts, credentials)
         name = @configuration['default_node_name']
-        name = Shellwords.escape(credentials['node-name']) if credentials['node-name']
+        name = escape(credentials['node-name']) if credentials['node-name']
 
         java_opts.add_system_property('appdynamics.agent.nodeName', name.to_s)
       end
@@ -130,15 +130,15 @@ def ssl_enabled(java_opts, credentials)
       end
 
       def tier_name(java_opts, credentials)
-        name = Shellwords.escape(@application.details['application_name'])
+        name = escape(@application.details['application_name'])
         name = @configuration['default_tier_name'] if @configuration['default_tier_name']
-        name = Shellwords.escape(credentials['tier-name']) if credentials['tier-name']
+        name = escape(credentials['tier-name']) if credentials['tier-name']
 
         java_opts.add_system_property('appdynamics.agent.tierName', name.to_s)
       end
 
       def unique_host_name(java_opts)
-        name = Shellwords.escape(@application.details['application_name'])
+        name = escape(@application.details['application_name'])
         name = @configuration['default_unique_host_name'] if @configuration['default_unique_host_name']
 
         java_opts.add_system_property('appdynamics.agent.uniqueHostId', name.to_s)
@@ -180,6 +180,14 @@ def save_cfg_file(file, conf_file)
           FileUtils.cp_r file, target_directory + '/conf/' + conf_file
         end
       end
+
+      def escape(value)
+        if /\$[\(\{][^\)\}]+[\)\}]/ =~ value
+          "\\\"#{value}\\\""
+        else
+          Shellwords.escape(value)
+        end
+      end
     end
   end
 end

spec/java_buildpack/framework/app_dynamics_agent_spec.rb

@@ -92,6 +92,26 @@
         end
       end
 
+      context do
+        let(:credentials) { super().merge 'tier-name' => '--> ${SOME_VAR} <--' }
+
+        it 'adds tier_name from credentials with shell variable in it to JAVA_OPTS if specified' do
+          component.release
+
+          expect(java_opts).to include('-Dappdynamics.agent.tierName=\"--> ${SOME_VAR} <--\"')
+        end
+      end
+
+      context do
+        let(:credentials) { super().merge 'tier-name' => '$(echo \'Hello World!\') and stuff' }
+
+        it 'adds tier_name from credentials with subshell in it to JAVA_OPTS if specified' do
+          component.release
+
+          expect(java_opts).to include('-Dappdynamics.agent.tierName=\"$(echo \'Hello World!\') and stuff\"')
+        end
+      end
+
       context do
         let(:credentials) { super().merge 'application-name' => 'another-test application-name' }
 
@@ -102,6 +122,26 @@
         end
       end
 
+      context do
+        let(:credentials) { super().merge 'application-name' => '$(echo \'Hello World!\') and stuff' }
+
+        it 'adds application_name from credentials with subshell in value to JAVA_OPTS if specified' do
+          component.release
+
+          expect(java_opts).to include('-Dappdynamics.agent.applicationName=\"$(echo \'Hello World!\') and stuff\"')
+        end
+      end
+
+      context do
+        let(:credentials) { super().merge 'application-name' => 'Name ${MY_APP_NAME}' }
+
+        it 'adds application_name from credentials with env variable in value to JAVA_OPTS if specified' do
+          component.release
+
+          expect(java_opts).to include('-Dappdynamics.agent.applicationName=\"Name ${MY_APP_NAME}\"')
+        end
+      end
+
       context do
         let(:configuration) do
           { 'default_tier_name' => nil,