GROOVY-12273: Write config keys and values as data, not as source

ConfigObject.writeTo documents a round trip with ConfigSlurper.parse, which
compiles its output as a Groovy script. Keys were written bare unless they
were Groovy keywords, and values were rendered by FormatHelper.inspect,
which quotes a String but not other types. What that produced was source
rather than data, and it was read back as whatever it happened to parse as.

Measured before the change, writing a ConfigObject and parsing it back:

  key 'a b'                        did not parse
  key "a'b"                        did not parse
  key "x = <statement>; y"         *** executed on re-parse ***
  nested block under key 'a b'     did not parse
  GString value holding a dollar   did not parse
  StringBuilder value              did not parse
  GString inside a list            came back altered
  a value of any other type        did not parse

The executing case is the one that matters: a key is data, and an
application which stores an attacker-influenced entry name and later
persists the configuration would run it.

Render every key as an identifier when it is one and as a quoted literal
otherwise, rather than only quoting keywords, and give a quoted leading key
the receiver it needs to open a statement, which is what keyword keys have
always been given. Nested blocks accept a quoted key unchanged, so only the
rendering moved. writeValue now receives a key path whose components have
already been rendered, because it is also called with a composed path and
must not quote the path as a whole.

Carry a value which has no literal form over to its text so that it is
rendered as a quoted String: a CharSequence which is not a String would
otherwise be written double quoted, where a dollar is live, and a value of
any other type would be written as a bare toString(). Collections and maps
are converted through, which covers the same value nested inside them.
Numbers and booleans already write as themselves and are untouched.

Author: paulk-asert

src/main/java/groovy/util/ConfigObject.java

@@ -30,10 +30,13 @@
 import java.io.BufferedWriter;
 import java.io.IOException;
 import java.io.Writer;
+import java.lang.reflect.Array;
 import java.net.URL;
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.LinkedHashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.Properties;
 import java.util.Set;
@@ -251,20 +254,20 @@ private void writeConfig(String prefix, ConfigObject map, BufferedWriter out, in
 
                     if (configSize == 1 || DefaultGroovyMethods.asBoolean(dotsInKeys)) {
                         if (firstSize == 1 && firstValue instanceof ConfigObject) {
-                            key = KEYWORDS.contains(key) ? FormatHelper.inspect(key) : key;
-                            String writePrefix = prefix + key + "." + firstKey + ".";
+                            key = renderKey(key);
+                            String writePrefix = prefix + key + "." + renderKey(String.valueOf(firstKey)) + ".";
                             writeConfig(writePrefix, (ConfigObject) firstValue, out, tab, true);
                         } else if (!DefaultGroovyMethods.asBoolean(dotsInKeys) && firstValue instanceof ConfigObject) {
                             writeNode(key, space, tab, value, out);
                         } else {
                             for (Object j : value.keySet()) {
                                 Object v2 = value.get(j);
-                                Object k2 = ((String) j).indexOf('.') > -1 ? FormatHelper.inspect(j) : j;
+                                Object k2 = renderKey((String) j);
                                 if (v2 instanceof ConfigObject) {
-                                    key = KEYWORDS.contains(key) ? FormatHelper.inspect(key) : key;
+                                    key = renderKey(key);
                                     writeConfig(prefix + key, (ConfigObject) v2, out, tab, false);
                                 } else {
-                                    writeValue(key + "." + k2, space, prefix, v2, out);
+                                    writeValue(renderKey(key) + "." + k2, space, prefix, v2, out);
                                 }
                             }
                         }
@@ -273,30 +276,111 @@ private void writeConfig(String prefix, ConfigObject map, BufferedWriter out, in
                     }
                 }
             } else {
-                writeValue(key, space, prefix, v, out);
+                writeValue(renderKey(key), space, prefix, v, out);
             }
         }
     }
 
-    private static void writeValue(String key, String space, String prefix, Object value, BufferedWriter out) throws IOException {
-//        key = key.indexOf('.') > -1 ? InvokerHelper.inspect(key) : key;
-        boolean isKeyword = KEYWORDS.contains(key);
-        key = isKeyword ? FormatHelper.inspect(key) : key;
-
-        if (!StringGroovyMethods.asBoolean(prefix) && isKeyword) prefix = "this.";
-        out.append(space).append(prefix).append(key).append('=').append(FormatHelper.inspect(value));
+    /**
+     * Writes one entry, given a key path whose components have already been rendered.
+     *
+     * @param keyPath the rendered key path, such as {@code foo} or {@code foo.'a b'}
+     */
+    private static void writeValue(String keyPath, String space, String prefix, Object value, BufferedWriter out) throws IOException {
+        // A quoted key cannot open a statement on its own, so it needs a receiver, exactly as a
+        // keyword key has always done. The statement opens with the prefix when there is one.
+        String statementStart = StringGroovyMethods.asBoolean(prefix) ? prefix : keyPath;
+        if (statementStart.startsWith("'")) prefix = "this." + prefix;
+        out.append(space).append(prefix).append(keyPath).append('=').append(renderValue(value));
         out.newLine();
     }
 
     private void writeNode(String key, String space, int tab, ConfigObject value, BufferedWriter out) throws IOException {
-        key = KEYWORDS.contains(key) ? FormatHelper.inspect(key) : key;
-        out.append(space).append(key).append(" {");
+        out.append(space).append(renderKey(key)).append(" {");
         out.newLine();
         writeConfig("", value, out, tab + 1, true);
         out.append(space).append('}');
         out.newLine();
     }
 
+    /**
+     * Renders a key as it must appear in the written configuration: bare when it is a plain
+     * identifier, and as a quoted literal otherwise. A key which is not an identifier would
+     * otherwise be written as though it were source, and read back as whatever it happened to
+     * parse as.
+     *
+     * @param key the key to render
+     * @return the key as it should be written
+     */
+    private static String renderKey(String key) {
+        return isIdentifier(key) ? key : FormatHelper.inspect(key);
+    }
+
+    private static boolean isIdentifier(String key) {
+        if (key == null || key.isEmpty() || KEYWORDS.contains(key)) return false;
+        if (!Character.isJavaIdentifierStart(key.charAt(0))) return false;
+        for (int i = 1, n = key.length(); i < n; i += 1) {
+            if (!Character.isJavaIdentifierPart(key.charAt(i))) return false;
+        }
+        return true;
+    }
+
+    /**
+     * Renders a value as a literal which reads back as the same data.
+     *
+     * @param value the value to render
+     * @return the value as it should be written
+     */
+    private static String renderValue(Object value) {
+        return FormatHelper.inspect(asWritableData(value));
+    }
+
+    /**
+     * Converts a value into something {@link FormatHelper#inspect} renders as inert data.
+     * <p>
+     * A {@link CharSequence} which is not a {@code String} is rendered as a double quoted
+     * literal, in which a dollar is live, so its text is carried over to a {@code String} and
+     * rendered single quoted instead. An array has no literal form, so it is carried over to a
+     * {@code List}, element by element, and reads back as one. A value of any other type
+     * without a literal form would be written as a bare {@code toString()}, which is not data
+     * at all, so its text is carried over in the same way. Numbers and booleans already write
+     * as themselves.
+     *
+     * @param value the value to convert
+     * @return a value whose rendering is data
+     */
+    private static Object asWritableData(Object value) {
+        if (value == null || value instanceof String || value instanceof Number || value instanceof Boolean) {
+            return value;
+        }
+        if (value instanceof CharSequence) {
+            return value.toString();
+        }
+        if (value instanceof Map<?, ?> map) {
+            Map<Object, Object> converted = new LinkedHashMap<>(map.size());
+            for (Map.Entry<?, ?> entry : map.entrySet()) {
+                converted.put(asWritableData(entry.getKey()), asWritableData(entry.getValue()));
+            }
+            return converted;
+        }
+        if (value instanceof Collection<?> collection) {
+            List<Object> converted = new ArrayList<>(collection.size());
+            for (Object element : collection) {
+                converted.add(asWritableData(element));
+            }
+            return converted;
+        }
+        if (value.getClass().isArray()) {
+            int length = Array.getLength(value);
+            List<Object> converted = new ArrayList<>(length);
+            for (int i = 0; i < length; i += 1) {
+                converted.add(asWritableData(Array.get(value, i)));
+            }
+            return converted;
+        }
+        return value.toString();
+    }
+
     private static Properties convertValuesToString(Map props) {
         Properties newProps = new Properties();
 

src/test/groovy/groovy/util/ConfigObjectTest.groovy

@@ -96,4 +96,121 @@ development {
         def config = new ConfigSlurper().parse(configString)
         assert config == new ConfigSlurper().parse(config.prettyPrint())
     }
+
+    // GROOVY-12273: writeTo documents a round trip with ConfigSlurper.parse, which compiles the
+    // output as a Groovy script. Anything written as source rather than as a literal is read
+    // back as whatever it happens to parse as, up to and including running.
+    private static void assertRoundTrips(String description, ConfigObject original) {
+        def written = new StringWriter()
+        original.writeTo(written)
+        def text = written.toString()
+        def reparsed
+        try {
+            reparsed = new ConfigSlurper().parse(text)
+        } catch (Exception e) {
+            assert false, "$description did not parse back: ${e.message}\n$text"
+        }
+        // The contract is that data survives, not that types do: a value with no literal form,
+        // such as a StringBuilder, necessarily comes back as its text.
+        assert asText(reparsed) == asText(original), "$description changed across the round trip\n$text"
+    }
+
+    private static Object asText(Object value) {
+        if (value instanceof CharSequence) return value.toString()
+        if (value instanceof Map) return value.collectEntries { k, v -> [(asText(k)): asText(v)] }
+        if (value instanceof Collection) return value.collect { asText(it) }
+        if (value != null && value.class.array) return value.collect { asText(it) }
+        value
+    }
+
+    private static ConfigObject configOf(Map entries) {
+        def config = new ConfigObject()
+        entries.each { k, v -> config.put(k, v) }
+        config
+    }
+
+    @Test
+    void testKeysThatAreNotIdentifiersRoundTrip() {
+        [
+            'a keyword'          : 'class',
+            'a space'            : 'a b',
+            'a quote'            : "a'b",
+            'a backslash'        : 'a\\b',
+            'a dot'              : 'a.b',
+            'a leading digit'    : '1abc',
+            'a hyphen'           : 'a-b',
+            'empty'              : '',
+        ].each { description, key ->
+            assertRoundTrips("key with $description", configOf([(key): 1]))
+        }
+    }
+
+    @Test
+    void testKeyCannotSmuggleCodeIntoTheRoundTrip() {
+        def marker = 'groovy.test.configObjectInjection'
+        System.clearProperty(marker)
+        // Concatenated rather than interpolated: a ConfigObject key must be a String.
+        def key = 'x = System.setProperty(\'' + marker + '\', \'yes\'); y'
+
+        assertRoundTrips('key holding an assignment', configOf([(key): 1]))
+        assert System.getProperty(marker) == null, 'a key was executed rather than read as data'
+    }
+
+    @Test
+    void testNestedConfigUnderAKeyThatIsNotAnIdentifierRoundTrips() {
+        def nested = new ConfigObject()
+        nested.p = 1
+        nested.q = 2
+        assertRoundTrips('nested block under an awkward key', configOf(['a b': nested]))
+    }
+
+    @Test
+    void testValuesWhoseTextContainsADollarRoundTrip() {
+        def dollar = '$'
+        [
+            'String'            : 'costs $5',
+            'GString'           : "costs ${dollar}5",
+            'StringBuilder'     : new StringBuilder('costs $5'),
+            'GString in a list' : ["a ${dollar}b"],
+            'GString in a map'  : [k: "a ${dollar}b"],
+        ].each { description, value ->
+            assertRoundTrips("value of type $description", configOf([foo: value]))
+        }
+    }
+
+    @Test
+    void testAwkwardKeysInsideFlattenedKeyChainsRoundTrip() {
+        // single-entry chains are written as dotted prefixes; every component must be rendered,
+        // and a chain opening with a quoted key needs a receiver to parse as navigation
+        def middle = new ConfigObject()
+        middle.x.'a b'.y = 1
+        assertRoundTrips('awkward key in the middle of a chain', middle)
+
+        def leading = new ConfigObject()
+        leading.'a b'.c.d = 1
+        assertRoundTrips('awkward key opening a chain', leading)
+    }
+
+    @Test
+    void testArrayValuesRoundTripAsLists() {
+        // arrays have no literal form; they are written as list literals and read back as lists
+        [
+            'String[]'            : ['one', 'two'] as String[],
+            'int[]'               : [1, 2, 3] as int[],
+            'array inside a list' : [[1, 2] as int[]],
+        ].each { description, value ->
+            assertRoundTrips("value of type $description", configOf([foo: value]))
+        }
+    }
+
+    @Test
+    void testValuesThatWriteAsThemselvesAreUnchanged() {
+        def written = new StringWriter()
+        configOf([i: 42, d: 1.5, b: true, s: 'plain']).writeTo(written)
+        def text = written.toString()
+        assert text.contains('i=42')
+        assert text.contains('d=1.5')
+        assert text.contains('b=true')
+        assert text.contains("s='plain'")
+    }
 }