Better escaping of Java options

Christopher Frost · Christopher Frost · commit d95d940907bd · 2015-09-24T18:22:27.000+01:00
This commit makes it possible to use both escaped and unescaped $ and \ characters in the value of a Java option. The $ character is required so users can include environment variables in Java options. The \ character is required so it's possible to include an escaped $ charecter in the value of a Java option.

[#100119572]
diff --git a/docs/framework-java_opts.md b/docs/framework-java_opts.md
@@ -14,7 +14,6 @@ The Java Options Framework contributes arbitrary Java options to the application
 </table>
 Tags are printed to standard output by the buildpack detect script
 
-
 ## Configuration
 For general information on configuring the buildpack, including how to specify configuration values through environment variables, refer to [Configuration and Extension][].
 
@@ -29,6 +28,37 @@ Any `JAVA_OPTS` from either the config file or environment variables that config
 
 Any `JAVA_OPTS` from either the config file or environment variables will be specified in the start command after any Java Opts added by other frameworks.
 
+## Escaping strings
+
+Java options will have special characters escaped when used in the shell command that starts the Java application but the `$` and `\` characters will not be escaped. This is to allow Java options to include environment variables when the application starts.
+
+```bash
+cf set-env my-application JAVA_OPTS '-Dexample.port=$PORT'
+```
+
+If an escaped `$` or `\` character is needed in the Java options they will have to be escaped manually. For example, to obtain this output in the start command.
+
+```bash
+-Dexample.other=something.\$dollar.\\slash
+```
+
+From the command line use;
+```bash
+cf set-env my-application JAVA_OPTS '-Dexample.other=something.\\\\\$dollar.\\\\\\\slash'
+```
+
+From the [`config/java_opts.yml`][] file use;
+```yaml
+from_environment: true
+java_opts: '-Dexample.other=something.\\$dollar.\\\\slash'
+```
+
+Finally, from the applications manifest use;
+```yaml
+  env:
+    JAVA_OPTS: '-Dexample.other=something.\\\\\$dollar.\\\\\\\slash'
+```
+
 ## Example
 ```yaml
 # JAVA_OPTS configuration
diff --git a/lib/java_buildpack/framework/java_opts.rb b/lib/java_buildpack/framework/java_opts.rb
@@ -66,14 +66,22 @@ def parsed_java_opts
         parsed_java_opts.concat ENV[ENVIRONMENT_VARIABLE].shellsplit if supports_environment?
 
         parsed_java_opts.map do |java_opt|
-          if /(?<key>.+)=(?<value>.+)/ =~ java_opt
-            "#{key}=#{value.shellescape}"
+          if /(?<key>.+?)=(?<value>.+)/ =~ java_opt
+            "#{key}=#{parse_shell_string(value)}"
           else
             java_opt
           end
         end
       end
 
+      def parse_shell_string(str)
+        return "''" if str.empty?
+        str = str.dup
+        str.gsub!(%r{([^A-Za-z0-9_\-.,:\/@\n$\\])}, '\\\\\\1')
+        str.gsub!(/\n/, "'\n'")
+        str
+      end
+
       def supports_configuration?
         @configuration.key?(CONFIGURATION_PROPERTY) && !@configuration[CONFIGURATION_PROPERTY].nil?
       end
diff --git a/spec/java_buildpack/framework/java_opts_spec.rb b/spec/java_buildpack/framework/java_opts_spec.rb
@@ -66,35 +66,64 @@
 
     it 'adds split java_opts to context' do
       component.release
-
       expect(java_opts).to include('-Xdebug')
       expect(java_opts).to include('-Xnoagent')
-      expect(java_opts).to include('-Xrunjdwp:transport=dt_socket,server=y,address=8000,suspend=y')
+      expect(java_opts).to include('-Xrunjdwp:transport=dt_socket,server\=y,address\=8000,suspend\=y')
       expect(java_opts).to include('-XX:OnOutOfMemoryError=kill\ -9\ \%p')
     end
   end
 
   context do
     let(:configuration) do
-      { 'java_opts' => '-Dtest=!£$%^&*(){}<>[];~`' }
+      { 'java_opts' => '-Dtest=!£%^&*()<>[]{};~`' }
     end
 
     it 'escapes special characters' do
       component.release
+      expect(java_opts).to include('-Dtest=\!\£\%\^\&\*\(\)\<\>\[\]\{\}\;\~\`')
+    end
+  end
+
+  context do
+    let(:configuration) do
+      { 'java_opts' => '-Dtest=$DOLLAR\\\SLASH' }
+    end
+
+    it 'does not escape the shell variable character from configuration' do
+      component.release
+      expect(java_opts).to include('-Dtest=$DOLLAR\SLASH')
+    end
+  end
 
-      expect(java_opts).to include('-Dtest=\\!\\£\\$\\%\\^\\&\\*\\(\\)\\{\\}\\<\\>\\[\\]\\;\\~\\`')
+  context do
+    let(:configuration) { { 'from_environment' => true } }
+    let(:environment) { { 'JAVA_OPTS' => '-Dtest=$dollar\\\slash' } }
+
+    it 'does not escape the shell variable character from environment' do
+      component.release
+      expect(java_opts).to include('-Dtest=$dollar\slash')
     end
   end
 
   context do
     let(:configuration) do
-      { 'java_opts' => '-javaagent:agent.jar=port="\$PORT",host=localhost' }
+      { 'java_opts' => '-Dtest=something.\\\$dollar.\\\\\\\slash' }
     end
 
-    it 'allows escaped characters' do
+    it 'can escape non-escaped characters ' do
       component.release
+      expect(java_opts).to include('-Dtest=something.\\$dollar.\\\slash')
+    end
+  end
 
-      expect(java_opts).to include('-javaagent:agent.jar=port=$PORT,host=localhost')
+  context do
+    let(:configuration) do
+      { 'java_opts' => '-javaagent:agent.jar=port=$PORT,host=localhost' }
+    end
+
+    it 'escapes equal signs after the first one' do
+      component.release
+      expect(java_opts).to include('-javaagent:agent.jar=port\\=$PORT,host\\=localhost')
     end
   end
 
