Merge pull request #49 from openjavaformat/argfile-quotes

Read quoted arguments from parameter files, and refuse a file that includes itself

Author: abashev

open-java-format/src/main/java/com/palantir/javaformat/java/CommandLineOptionsParser.java

@@ -14,36 +14,51 @@
 
 package com.palantir.javaformat.java;
 
-import static java.nio.charset.StandardCharsets.UTF_8;
-
-import com.google.common.base.CharMatcher;
+import com.google.common.base.Preconditions;
 import com.google.common.base.Splitter;
 import com.google.common.collect.ImmutableRangeSet;
 import com.google.common.collect.Range;
 import java.io.IOException;
 import java.io.UncheckedIOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
-import java.nio.file.Paths;
+import java.util.ArrayDeque;
 import java.util.ArrayList;
+import java.util.Deque;
 import java.util.Iterator;
 import java.util.List;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 import javax.annotation.Nullable;
 
 /** A parser for {@link CommandLineOptions}. */
 final class CommandLineOptionsParser {
 
     private static final Splitter COMMA_SPLITTER = Splitter.on(',');
     private static final Splitter COLON_SPLITTER = Splitter.on(':');
-    private static final Splitter ARG_SPLITTER =
-            Splitter.on(CharMatcher.breakingWhitespace()).omitEmptyStrings().trimResults();
+
+    /**
+     * Splits the arguments of a parameter file on whitespace (including tabs and line breaks), and lets an argument be
+     * quoted so that it keeps the whitespace inside it unchanged.
+     *
+     * <p>The regex matches either a quoted string (single or double quotes are allowed) or a plain unquoted string.
+     * Double quotes may appear inside a single-quoted string and vice versa, and are then kept as they are. For
+     * simplicity, escaped quotes are not handled.
+     */
+    private static final Pattern ARG_MATCHER = Pattern.compile(
+            "\"([^\"]*)(?:\"|$)" // group 1: string in double quotes (or until EOF), with whitespace allowed
+                    + "|" // OR
+                    + "'([^']*)(?:'|$)" // group 2: string in single quotes (or until EOF), with whitespace allowed
+                    + "|" // OR
+                    + "([^\\s\"']+)" // group 3: unquoted string, without whitespace and without any quotes
+            );
 
     /** Parses {@link CommandLineOptions}. */
     @SuppressWarnings("for-rollout:NullAway")
     static CommandLineOptions parse(Iterable<String> options) {
         CommandLineOptions.Builder optionsBuilder = CommandLineOptions.builder();
         List<String> expandedOptions = new ArrayList<>();
-        expandParamsFiles(options, expandedOptions);
+        expandParamsFiles(options, expandedOptions, new ArrayDeque<>());
         Iterator<String> it = expandedOptions.iterator();
         while (it.hasNext()) {
             String option = it.next();
@@ -226,7 +241,7 @@ private static Range<Integer> parseRange(String arg) {
      * Pre-processes an argument list, expanding arguments of the form {@code @filename} by reading the content of the
      * file and appending whitespace-delimited options to {@code arguments}.
      */
-    private static void expandParamsFiles(Iterable<String> args, List<String> expanded) {
+    private static void expandParamsFiles(Iterable<String> args, List<String> expanded, Deque<String> paramFilesStack) {
         for (String arg : args) {
             if (arg.isEmpty()) {
                 continue;
@@ -236,14 +251,35 @@ private static void expandParamsFiles(Iterable<String> args, List<String> expand
             } else if (arg.startsWith("@@")) {
                 expanded.add(arg.substring(1));
             } else {
-                Path path = Paths.get(arg.substring(1));
-                try {
-                    String sequence = new String(Files.readAllBytes(path), UTF_8);
-                    expandParamsFiles(ARG_SPLITTER.split(sequence), expanded);
-                } catch (IOException e) {
-                    throw new UncheckedIOException(path + ": could not read file: " + e.getMessage(), e);
+                String filename = arg.substring(1);
+                if (paramFilesStack.contains(filename)) {
+                    throw new IllegalArgumentException("parameter file was included recursively: " + filename);
+                }
+                paramFilesStack.push(filename);
+                expandParamsFiles(getParamsFromFile(filename), expanded, paramFilesStack);
+                String finishedFilename = paramFilesStack.pop();
+                Preconditions.checkState(filename.equals(finishedFilename));
+            }
+        }
+    }
+
+    /** Reads the parameters from a file, keeping quoted parameters whole. */
+    private static List<String> getParamsFromFile(String filename) {
+        String fileContent;
+        try {
+            fileContent = Files.readString(Path.of(filename));
+        } catch (IOException e) {
+            throw new UncheckedIOException(filename + ": could not read file: " + e.getMessage(), e);
+        }
+        List<String> paramsFromFile = new ArrayList<>();
+        Matcher m = ARG_MATCHER.matcher(fileContent);
+        while (m.find()) {
+            for (int i = 1; i <= m.groupCount(); i++) {
+                if (m.group(i) != null) { // only one group matches: double quotes, single quotes or unquoted string.
+                    paramsFromFile.add(m.group(i));
                 }
             }
         }
+        return paramsFromFile;
     }
 }

open-java-format/src/test/java/com/palantir/javaformat/java/CommandLineOptionsParserTest.java

@@ -16,6 +16,7 @@
 
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.assertj.core.api.Assertions.fail;
 
 import com.google.common.collect.Range;
@@ -176,6 +177,61 @@ public void paramsFile() throws IOException {
         assertThat(options.files()).containsExactly("L", "M", "ℕ", "@O", "P", "Q");
     }
 
+    @Test
+    public void paramsFileWithNesting() throws IOException {
+        Path outer = Files.createFile(testFolder.resolve("outer"));
+        Path exit = Files.createFile(testFolder.resolve("exit"));
+        Path nested1 = Files.createFile(testFolder.resolve("nested1"));
+        Path nested2 = Files.createFile(testFolder.resolve("nested2"));
+        Path nested3 = Files.createFile(testFolder.resolve("nested3"));
+
+        String[] args = {"--dry-run", "@" + exit, "L", "@" + outer, "U"};
+
+        Files.write(exit, "--set-exit-if-changed".getBytes(UTF_8));
+        Files.write(outer, ("M\n@" + nested1.toAbsolutePath() + "\nT").getBytes(UTF_8));
+        Files.write(nested1, ("ℕ\n@" + nested2.toAbsolutePath() + "\nS").getBytes(UTF_8));
+        Files.write(nested2, ("O\n@" + nested3.toAbsolutePath() + "\nR").getBytes(UTF_8));
+        Files.write(nested3, "P\n\n   \n@@Q\n".getBytes(UTF_8));
+
+        CommandLineOptions options = CommandLineOptionsParser.parse(Arrays.asList(args));
+        assertThat(options.files()).containsExactly("L", "M", "ℕ", "O", "P", "@Q", "R", "S", "T", "U");
+    }
+
+    @Test
+    public void paramsFileWithRecursion() throws IOException {
+        Path outer = Files.createFile(testFolder.resolve("outer"));
+        Path exit = Files.createFile(testFolder.resolve("exit"));
+        Path nested1 = Files.createFile(testFolder.resolve("nested1"));
+        Path nested2 = Files.createFile(testFolder.resolve("nested2"));
+
+        String[] args = {"--dry-run", "@" + exit, "L", "@" + outer, "U"};
+
+        Files.write(exit, "--set-exit-if-changed".getBytes(UTF_8));
+        Files.write(outer, ("M\n@" + nested1.toAbsolutePath() + "\nT").getBytes(UTF_8));
+        Files.write(nested1, ("ℕ\n@" + nested2.toAbsolutePath() + "\nS").getBytes(UTF_8));
+        Files.write(nested2, ("O\n@" + nested1.toAbsolutePath() + "\nR").getBytes(UTF_8));
+
+        assertThatThrownBy(() -> CommandLineOptionsParser.parse(Arrays.asList(args)))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessageStartingWith("parameter file was included recursively: ");
+    }
+
+    @Test
+    public void paramsFileWithQuotesAndWhitespaces() throws IOException {
+        Path outer = Files.createFile(testFolder.resolve("outer with whitespace"));
+        Path exit = Files.createFile(testFolder.resolve("exit with whitespace"));
+        Path nested = Files.createFile(testFolder.resolve("nested with whitespace"));
+
+        String[] args = {"--dry-run", "@" + exit, "L +w", "@" + outer, "Q +w"};
+
+        Files.write(exit, "--set-exit-if-changed 'K +w".getBytes(UTF_8));
+        Files.write(outer, ("\"'M' +w\"\n\"@" + nested.toAbsolutePath() + "\"\n'\"P\" +w'").getBytes(UTF_8));
+        Files.write(nested, "\"ℕ +w\"\n\n   \n\"@@O +w".getBytes(UTF_8));
+
+        CommandLineOptions options = CommandLineOptionsParser.parse(Arrays.asList(args));
+        assertThat(options.files()).containsExactly("K +w", "L +w", "'M' +w", "ℕ +w", "@O +w", "\"P\" +w", "Q +w");
+    }
+
     @Test
     public void assumeFilename() {
         assertThat(CommandLineOptionsParser.parse(Arrays.asList("--assume-filename", "Foo.java"))