From f5e6b12d7be9e72e654258d64d89ddf40e12fa15 Mon Sep 17 00:00:00 2001 From: Karlheinz Friedberger Date: Wed, 23 Sep 2026 10:43:01 +0300 Subject: [PATCH] Read quoted arguments from parameter files, and refuse a file that includes itself CommandLineOptionsParser split an @argfile at every whitespace character and knew no quotes, so a path with a space in it became several arguments. Each was reported as "Skipping non-Java file", and the run still exited 0 (#40, from google/google-java-format#421). A parameter file that included itself recursed until the stack overflowed. This ports google/google-java-format#931 by Karlheinz Friedberger, its three commits squashed into one. An argument in a parameter file may be quoted with double or single quotes and then keeps its whitespace; an unclosed quote runs to the end of the file; and a file that is already being read is reported as "parameter file was included recursively". Escaped quotes are not handled, as upstream. The three CommandLineOptionsParserTest cases come from that PR. --- .../java/CommandLineOptionsParser.java | 64 +++++++++++++++---- .../java/CommandLineOptionsParserTest.java | 56 ++++++++++++++++ 2 files changed, 106 insertions(+), 14 deletions(-) diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/CommandLineOptionsParser.java b/open-java-format/src/main/java/com/palantir/javaformat/java/CommandLineOptionsParser.java index d7c595227..638ed34b1 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/java/CommandLineOptionsParser.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/java/CommandLineOptionsParser.java @@ -14,9 +14,7 @@ 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; @@ -24,10 +22,13 @@ 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}. */ @@ -35,15 +36,29 @@ 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. + * + *

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 options) { CommandLineOptions.Builder optionsBuilder = CommandLineOptions.builder(); List expandedOptions = new ArrayList<>(); - expandParamsFiles(options, expandedOptions); + expandParamsFiles(options, expandedOptions, new ArrayDeque<>()); Iterator it = expandedOptions.iterator(); while (it.hasNext()) { String option = it.next(); @@ -226,7 +241,7 @@ private static Range 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 args, List expanded) { + private static void expandParamsFiles(Iterable args, List expanded, Deque paramFilesStack) { for (String arg : args) { if (arg.isEmpty()) { continue; @@ -236,14 +251,35 @@ private static void expandParamsFiles(Iterable args, List 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 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 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; } } diff --git a/open-java-format/src/test/java/com/palantir/javaformat/java/CommandLineOptionsParserTest.java b/open-java-format/src/test/java/com/palantir/javaformat/java/CommandLineOptionsParserTest.java index 344437e94..ced15a57c 100644 --- a/open-java-format/src/test/java/com/palantir/javaformat/java/CommandLineOptionsParserTest.java +++ b/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"))