Copilot commented on code in PR #5323: URL: https://github.com/apache/solr/pull/5323#discussion_r4549256268
########## solr/packaging/src/test/org/apache/solr/shell/BasicAuthTest.java: ########## @@ -0,0 +1,148 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.solr.shell; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import org.apache.lucene.util.IOUtils; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +/** + * Port of {@code test_basic_auth.bats}. + * + * <p>Kept close to the original, including the order of the tests and the text each asserts on. + * Where it differs: + * + * <ul> + * <li>The original's {@code setup()} ran before every test; the harness wipes {@code SOLR_HOME} + * once per class, so {@link #startSolrWithBasicAuth} does the rest. Each test gets its own + * node, embedded ZooKeeper data and {@code security.json}, which is what lets two tests share + * the collection name {@code COLL_NAME}. + * <li>The one-command {@code SOLR_INCLUDE=/force/credentials/to/be/supplied} prefix becomes an + * {@code env()} that stays set for the test, the harness environment being per-session rather + * than per-command, and points inside the work directory rather than at an absolute Unix + * path. That also keeps {@code solr auth disable} away from the distribution's own include + * file, which every test class shares. + * <li>{@code curl -I} becomes {@link #http}: its status is the original's {@code "HTTP/1.1 200"}, + * and the header assertion is lower-cased because {@code java.net.http} lower-cases header + * names. + * <li>The second test's credential-less {@code run solr create -c COLL_NAME}, which the original + * leaves unasserted, asserts that it is refused. + * </ul> + */ +public class BasicAuthTest extends SolrShellTestCase { + + @Before + public void startSolrWithBasicAuth() throws IOException { + // BATS' common_clean_setup: every test in this file starts its own Solr from a clean SOLR_HOME. + IOUtils.rm(harnessEnv.solrHome()); + Files.createDirectories(harnessEnv.logsDir()); + + run("solr start -Dsolr.packages.enabled=true"); + + // The auth command exports some system variables that are injected as basic auth username and + // password, however that defeats our test so fake that out by pointing SOLR_INCLUDE at a path + // that does not exist: 'auth enable' then writes neither the include file nor + // SOLR_HOME/basicAuth.conf, and every command below has to supply the credentials itself. + Path bogusInclude = workDir().resolve("force-credentials-to-be-supplied").resolve("solr.in.sh"); + env("SOLR_INCLUDE", bogusInclude.toString()); + run("solr auth enable --type basicAuth --credentials name:password"); + + run( + "solr assert --credentials name:password --cloud http://localhost:${SOLR_PORT}"; + + " --timeout 5000"); + } + + @After + public void disableBasicAuth() { + // BATS' teardown. Stopping the node is automatic; SOLR_INCLUDE still points at the path that + // does not exist, so this only clears security.json out of ZooKeeper. + run("solr auth disable -z localhost:${ZK_PORT}"); + } + + // Remaining commands that should support basic auth: + // package, export + + @Test + public void createConfigApiAndDeleteWithBasicAuth() { + // Test create + run("solr create -u name:password -c COLL_NAME") + .assertContains("Created collection 'COLL_NAME'"); + + // Test config + run("solr config -u name:password -c COLL_NAME --action set-property --property" + + " updateHandler.autoCommit.maxDocs --value 100 --solr-url" + + " http://localhost:${SOLR_PORT}/solr";) + .assertContains("Successfully set-property updateHandler.autoCommit.maxDocs to 100"); + + // Test api + run("solr api -u name:password --solr-url" + + " \"http://localhost:${SOLR_PORT}/solr/COLL_NAME/select?q=*:*\"; --verbose") + .assertContains("\"numFound\":0"); + + // Test delete + run("solr delete --credentials name:password -c COLL_NAME -z localhost:${ZK_PORT}" + + " --delete-config --verbose") + .assertContains("Deleted collection 'COLL_NAME'"); + assertFalse(collectionExists("COLL_NAME")); + assertFalse(configExists("COLL_NAME")); + } + + @Test + public void postPostlogsAndExportWithBasicAuth() { + // Strengthened: the original ignores this line's outcome, but without credentials the create + // has to be refused. + runFail("solr create -c COLL_NAME"); + run("solr create -u name:password -c COLL_NAME") + .assertContains("Created collection 'COLL_NAME'"); + + // Test post + // --name COLL_NAME, not the BATS original's 'monitors': no test ever created that collection, + // so every document 404'd. It passed anyway because the status was ignored and `solr post` + // prints "1 files indexed." even when the collection does not exist (recorded separately). + run("solr post -u name:password -t application/xml --solr-url" + + " http://localhost:${SOLR_PORT} --name COLL_NAME" + + " ${SOLR_TIP}/example/exampledocs/monitor.xml") + .assertContains("1 files indexed."); + + // Test postlogs + run("solr postlogs -u name:password --solr-url http://localhost:${SOLR_PORT}"; + + " --name COLL_NAME --rootdir ${SOLR_LOGS_DIR}/solr.log") Review Comment: The harness substitutes these paths before shell parsing, so spaces in `SOLR_TIP` or `SOLR_LOGS_DIR` split them into multiple arguments and make this authentication test fail for an unrelated path constraint. Quote both the file argument here and the `--rootdir` value at line 128, matching `PostTest` and `PostlogsTest`. ########## solr/packaging/src/test/org/apache/solr/shell/SecurityManagerTest.java: ########## @@ -0,0 +1,85 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.solr.shell; + +import java.io.IOException; +import java.nio.file.Path; +import org.junit.Test; + +/** + * Port of {@code test_security_manager.bats}. + * + * <p>Kept close to the original, including the order of the tests and the text each asserts on. + * Where it differs: + * + * <ul> + * <li>the two directories are created with {@link #mkdirs} and canonicalized with {@code + * toRealPath()}, instead of {@code mkdir -p} followed by {@code cd -P && pwd}; + * <li>the backup {@code location} is built from {@link Path#toUri()} rather than by gluing {@code + * file://} onto the path — the same string as the original produces on Unix, and a valid file + * URL on Windows too, where a bare {@code file://C:\...} would not be; + * <li>{@code solr start} and {@code solr create}, which the original ran without looking at the + * outcome, assert their exit status; + * <li>the rejected backup also asserts a non-zero exit status; the request is answered with HTTP + * 500 either way the rejection happens. + * </ul> + * + * <p>The node's security manager is what this suite is about. On Java 24 and later {@code bin/solr} + * does not install one, and Solr's own {@code allowPaths} check rejects the unpermitted directory + * instead — hence the two alternatives in the final assertion, exactly as in the original. + */ +public class SecurityManagerTest extends SolrShellTestCase { + + @Test + public void allowPathsBackup() throws IOException { + // Make a test tmp dir, as the security policy includes TMP, so that might already contain the + // session work directory. + Path testTmpDir = mkdirs("tmp").toRealPath(); + + Path backupDir = mkdirs("backup-dir").toRealPath(); + + env("SOLR_SECURITY_MANAGER_ENABLED", "true"); + env( + "SOLR_OPTS", + "-Dsolr.security.allow.paths=" + backupDir + " -Djava.io.tmpdir=" + testTmpDir); Review Comment: When either directory contains spaces, `bin/solr` splits these unquoted `SOLR_OPTS` values into extra JVM arguments and Solr fails to start. Pass the properties through an argument-preserving startup path for each shell; on bash, individually quoted command-line `-D` arguments avoid this split, whereas quotes embedded in `SOLR_OPTS` do not. ########## solr/packaging/src/test/org/apache/solr/shell/ZkTest.java: ########## @@ -0,0 +1,210 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.solr.shell; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.time.Duration; +import org.apache.lucene.util.IOUtils; +import org.apache.solr.shell.exec.Shell; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * Port of {@code test_zk.bats}. + * + * <p>Kept deliberately close to the original so the two can be compared line by line, including the + * order of the tests and the text each one asserts on. Four things differ, each for a reason the + * comment at the site explains: files are created through Java rather than {@code touch}, {@code + * curl} becomes {@link #http}, the symlink test works on a copy of the config set instead of + * mutating the shared distribution, and the {@code ZK_HOST} test asserts only the fallback message, + * not the original's refutation of a listing from port 8983. + */ +public class ZkTest extends SolrShellTestCase { + + @BeforeClass + public static void startSolr() { + // BATS' setup_file. The node lives for the whole class and is stopped automatically. + classCli.run("solr start -DminStateByteLenForCompression=0"); + } + + @Test + public void shortHelp() { + run("solr zk ls -h").assertContains("usage: bin/solr zk"); + } + + @Test + public void shortHelpIsInferred() { + runFail("solr zk ls").assertContains("usage: bin/solr zk"); + } + + @Test + public void longHelp() { + run("solr zk -h") + .assertContains("Pass --help or -h after any COMMAND") + .assertContains("updateacls"); + } + + @Test + public void listingOutFiles() { + run("solr zk ls / -z localhost:${ZK_PORT} --recursive").assertContains("aliases.json"); + } + + @Test + public void connectingViaVariousSolrUrlsAndZkHosts() { + run("solr zk ls / --solr-url http://localhost:${SOLR_PORT}";).assertContains("aliases.json"); + run("solr zk ls / --solr-connection http://localhost:${SOLR_PORT}";) + .assertContains("aliases.json"); + run("solr zk ls / -s http://localhost:${SOLR_PORT}";).assertContains("aliases.json"); + run("solr zk ls /").assertContains("aliases.json"); + run("solr zk ls / -z localhost:${ZK_PORT}").assertContains("aliases.json"); + run("solr zk ls / --zk-host localhost:${ZK_PORT}").assertContains("aliases.json"); + } + + @Test + public void copyingFilesAround() { + // touch() rather than the shell's, which cmd.exe does not have. No cd() needed: a session + // starts in its own work directory, so these land there and so does the download below. + touch("myfile.txt"); + touch("myfile2.txt"); + touch("myfile3.txt"); + + run("solr zk cp myfile.txt zk:/myfile.txt -z localhost:${ZK_PORT}") + .assertContains( + "Copying from 'myfile.txt' to 'zk:/myfile.txt'. ZooKeeper at localhost:${ZK_PORT}"); + run("solr zk ls / -z localhost:${ZK_PORT}").assertContains("myfile.txt"); + + run("solr zk cp myfile2.txt zk:myfile2.txt -z localhost:${ZK_PORT}"); + run("solr zk ls / -z localhost:${ZK_PORT}").assertContains("myfile2.txt"); + + run("solr zk cp myfile3.txt zk:/myfile3.txt -z localhost:${ZK_PORT}") + .assertContains( + "Copying from 'myfile3.txt' to 'zk:/myfile3.txt'. ZooKeeper at localhost:${ZK_PORT}"); + run("solr zk ls / -z localhost:${ZK_PORT}").assertContains("myfile3.txt"); + + run( + """ + solr zk cp zk:/ -r "recursive_download/" + """); + Path download = workDir().resolve("recursive_download"); + assertTrue(Files.exists(download.resolve("myfile.txt"))); + assertTrue(Files.exists(download.resolve("myfile2.txt"))); + assertTrue(Files.exists(download.resolve("myfile3.txt"))); + } + + @Test + public void upconfig() { + run(""" + solr zk upconfig -d "${SOLR_TIP}/server/solr/configsets/sample_techproducts_configs" \ + -n techproducts2 -z localhost:${ZK_PORT} + """) + .assertContains("Uploading") + .assertNotContains("ERROR"); + + // BATS polled with curl and grep; the harness' own HTTP helper needs no curl, which is what + // makes this work on Windows. + awaitOrFail( + "config set techproducts2 to appear", + Duration.ofSeconds(10), + Duration.ofSeconds(1), + () -> http("/api/configsets").body().contains("techproducts2")); + assertTrue( + http("/api/configsets").body().contains("\"configSets\":[\"_default\",\"techproducts2\"]")); + } + + @Test + @EnabledOnShell( + Shell.BASH) // Creating a symlink on Windows needs privileges a test cannot assume. + public void solr12429UpconfigFailsWithSymlink() throws IOException { + // The BATS original symlinked inside the distribution's own config set and deleted the links + // afterwards. Here it works on a copy: the distribution is shared by every test class, so + // mutating it would make this test unsafe to run alongside any other. + Path configSet = workDir().resolve("configset"); + copyDirectory( + harnessEnv.solrTip().resolve("server/solr/configsets/sample_techproducts_configs"), + configSet); + + Path conf = configSet.resolve("conf"); + Files.createSymbolicLink( + conf.resolve("symlinked_stopwords.txt"), conf.resolve("stopwords.txt")); + Files.createSymbolicLink(conf.resolve("language"), conf.resolve("lang")); + assertTrue(Files.isSymbolicLink(conf.resolve("symlinked_stopwords.txt"))); + assertTrue(Files.isSymbolicLink(conf.resolve("language"))); + + runFail( + """ + solr zk upconfig -d "configset" -n techproducts_with_symlinks -z localhost:${ZK_PORT} + """) + .assertContains("Uploading") + .assertContains("ERROR: Not uploading symbolic link"); + } + + @Test + public void downconfig() { + run(""" + solr zk downconfig -z localhost:${ZK_PORT} -n _default -d "downconfig" + """) + .assertContains("Downloading") + .assertNotContains("ERROR"); + } + + @Test + public void zkCpGetsSolrHomeFromTheCommandLineOption() { + touch("afile.txt"); + + run("solr zk cp afile.txt zk:/afile.txt -z localhost:${ZK_PORT} --verbose" + + " --solr-home ${SOLR_TIP}/server/solr") Review Comment: The harness substitutes SOLR_TIP before invoking the shell but does not quote its value. With a checkout or distribution path containing spaces, --solr-home receives only the first path segment, so this test no longer exercises the intended home and its assertion fails. Quote the complete path argument, as the upconfig test above does. ########## solr/packaging/src/test/org/apache/solr/shell/ZkTest.java: ########## @@ -0,0 +1,210 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.solr.shell; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.time.Duration; +import org.apache.lucene.util.IOUtils; +import org.apache.solr.shell.exec.Shell; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * Port of {@code test_zk.bats}. + * + * <p>Kept deliberately close to the original so the two can be compared line by line, including the + * order of the tests and the text each one asserts on. Four things differ, each for a reason the + * comment at the site explains: files are created through Java rather than {@code touch}, {@code + * curl} becomes {@link #http}, the symlink test works on a copy of the config set instead of + * mutating the shared distribution, and the {@code ZK_HOST} test asserts only the fallback message, + * not the original's refutation of a listing from port 8983. + */ +public class ZkTest extends SolrShellTestCase { + + @BeforeClass + public static void startSolr() { + // BATS' setup_file. The node lives for the whole class and is stopped automatically. + classCli.run("solr start -DminStateByteLenForCompression=0"); + } + + @Test + public void shortHelp() { + run("solr zk ls -h").assertContains("usage: bin/solr zk"); + } + + @Test + public void shortHelpIsInferred() { + runFail("solr zk ls").assertContains("usage: bin/solr zk"); + } + + @Test + public void longHelp() { + run("solr zk -h") + .assertContains("Pass --help or -h after any COMMAND") + .assertContains("updateacls"); + } + + @Test + public void listingOutFiles() { + run("solr zk ls / -z localhost:${ZK_PORT} --recursive").assertContains("aliases.json"); + } + + @Test + public void connectingViaVariousSolrUrlsAndZkHosts() { + run("solr zk ls / --solr-url http://localhost:${SOLR_PORT}";).assertContains("aliases.json"); + run("solr zk ls / --solr-connection http://localhost:${SOLR_PORT}";) + .assertContains("aliases.json"); + run("solr zk ls / -s http://localhost:${SOLR_PORT}";).assertContains("aliases.json"); + run("solr zk ls /").assertContains("aliases.json"); + run("solr zk ls / -z localhost:${ZK_PORT}").assertContains("aliases.json"); + run("solr zk ls / --zk-host localhost:${ZK_PORT}").assertContains("aliases.json"); + } + + @Test + public void copyingFilesAround() { + // touch() rather than the shell's, which cmd.exe does not have. No cd() needed: a session + // starts in its own work directory, so these land there and so does the download below. + touch("myfile.txt"); + touch("myfile2.txt"); + touch("myfile3.txt"); + + run("solr zk cp myfile.txt zk:/myfile.txt -z localhost:${ZK_PORT}") + .assertContains( + "Copying from 'myfile.txt' to 'zk:/myfile.txt'. ZooKeeper at localhost:${ZK_PORT}"); + run("solr zk ls / -z localhost:${ZK_PORT}").assertContains("myfile.txt"); + + run("solr zk cp myfile2.txt zk:myfile2.txt -z localhost:${ZK_PORT}"); + run("solr zk ls / -z localhost:${ZK_PORT}").assertContains("myfile2.txt"); + + run("solr zk cp myfile3.txt zk:/myfile3.txt -z localhost:${ZK_PORT}") + .assertContains( + "Copying from 'myfile3.txt' to 'zk:/myfile3.txt'. ZooKeeper at localhost:${ZK_PORT}"); + run("solr zk ls / -z localhost:${ZK_PORT}").assertContains("myfile3.txt"); + + run( + """ + solr zk cp zk:/ -r "recursive_download/" + """); + Path download = workDir().resolve("recursive_download"); + assertTrue(Files.exists(download.resolve("myfile.txt"))); + assertTrue(Files.exists(download.resolve("myfile2.txt"))); + assertTrue(Files.exists(download.resolve("myfile3.txt"))); + } + + @Test + public void upconfig() { + run(""" + solr zk upconfig -d "${SOLR_TIP}/server/solr/configsets/sample_techproducts_configs" \ + -n techproducts2 -z localhost:${ZK_PORT} + """) + .assertContains("Uploading") + .assertNotContains("ERROR"); + + // BATS polled with curl and grep; the harness' own HTTP helper needs no curl, which is what + // makes this work on Windows. + awaitOrFail( + "config set techproducts2 to appear", + Duration.ofSeconds(10), + Duration.ofSeconds(1), + () -> http("/api/configsets").body().contains("techproducts2")); + assertTrue( + http("/api/configsets").body().contains("\"configSets\":[\"_default\",\"techproducts2\"]")); + } + + @Test + @EnabledOnShell( + Shell.BASH) // Creating a symlink on Windows needs privileges a test cannot assume. + public void solr12429UpconfigFailsWithSymlink() throws IOException { + // The BATS original symlinked inside the distribution's own config set and deleted the links + // afterwards. Here it works on a copy: the distribution is shared by every test class, so + // mutating it would make this test unsafe to run alongside any other. + Path configSet = workDir().resolve("configset"); + copyDirectory( + harnessEnv.solrTip().resolve("server/solr/configsets/sample_techproducts_configs"), + configSet); + + Path conf = configSet.resolve("conf"); + Files.createSymbolicLink( + conf.resolve("symlinked_stopwords.txt"), conf.resolve("stopwords.txt")); + Files.createSymbolicLink(conf.resolve("language"), conf.resolve("lang")); + assertTrue(Files.isSymbolicLink(conf.resolve("symlinked_stopwords.txt"))); + assertTrue(Files.isSymbolicLink(conf.resolve("language"))); + + runFail( + """ + solr zk upconfig -d "configset" -n techproducts_with_symlinks -z localhost:${ZK_PORT} + """) + .assertContains("Uploading") + .assertContains("ERROR: Not uploading symbolic link"); + } + + @Test + public void downconfig() { + run(""" + solr zk downconfig -z localhost:${ZK_PORT} -n _default -d "downconfig" + """) + .assertContains("Downloading") + .assertNotContains("ERROR"); + } + + @Test + public void zkCpGetsSolrHomeFromTheCommandLineOption() { + touch("afile.txt"); + + run("solr zk cp afile.txt zk:/afile.txt -z localhost:${ZK_PORT} --verbose" + + " --solr-home ${SOLR_TIP}/server/solr") + .assertContains("Using SolrHome: ${SOLR_TIP}/server/solr") + .assertNotContains("Failed to load solr.xml from ZK or SolrHome"); + } + + @Test + public void envVarZkHostIsHonored() { + // SOLR_PORT_LISTEN has to be absent, not empty: the tool looks at whether it is set at all + // before falling back to the default port. + unset("SOLR_PORT_LISTEN"); + + // Only the fallback message is asserted, not that the listing came back empty. The BATS + // original also refuted "aliases.json", which assumes nothing is listening on 8983 — a port no + // test class owns, and the single most likely thing to be running on a Solr developer's own + // machine. That made the test fail for a reason having nothing to do with ZK_HOST. + runFail("solr zk ls / --recursive") Review Comment: This still fails if a developer already has a reachable SolrCloud instance on port 8983. After `SOLR_PORT_LISTEN` is unset, the CLI can discover that instance's ZooKeeper and successfully list it, causing `runFail()` to throw before the warning assertion. Use `tryRun()` here to leave the status unconstrained, as the comment intends; the subsequent command can still assert success against this class's ZooKeeper. ########## solr/test-framework/src/java/org/apache/solr/shell/exec/Shell.java: ########## @@ -0,0 +1,142 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.solr.shell.exec; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.Locale; +import java.util.regex.Pattern; + +/** The shells a command line can be handed to. One process per command; no interactive session. */ +public enum Shell { + + /** POSIX shell. Invoked with a single {@code -c} argument, no profile and no rc files. */ + BASH { + @Override + public List<String> processArgs(String line, Path scriptDir, String scriptName) { + return List.of("bash", "--noprofile", "--norc", "-c", line); + } + + @Override + public String quote(String arg) { + return "'" + arg.replace("'", "'\\''") + "'"; + } + }, + + /** + * Windows {@code cmd.exe}. + * + * <p>The line goes into a temp {@code .cmd} script rather than onto the {@code cmd /c} command + * line, because {@code ProcessBuilder}'s quoting rules for {@code cmd /c} and {@code .cmd} + * arguments are special and fragile. {@code call} is prepended to a single-command line — without + * it a batch file invoking {@code solr.cmd} never gets control back — while a multi-line body is + * written verbatim and is responsible for its own. {@code /d} disables AutoRun registry hooks. + */ + CMD { + @Override + public List<String> processArgs(String line, Path scriptDir, String scriptName) + throws IOException { + String command = line.contains("\n") ? line : "call " + line; + String body = + String.join("\r\n", "@echo off", "chcp 65001 >nul", command, "exit /b %ERRORLEVEL%") + + "\r\n"; + Path script = scriptDir.resolve(scriptName + ".cmd"); + Files.writeString( + script, body.replace("\n", "\r\n").replace("\r\r\n", "\r\n"), StandardCharsets.UTF_8); + return List.of("cmd.exe", "/d", "/c", script.toString()); + } + + /** + * Double quotes protect {@code & | < > ( )} from cmd's parser, and {@code !} is literal because + * these scripts never enable delayed expansion. {@code %} is expanded even inside double + * quotes, so a literal one is written {@code %%} as any batch file must. + * + * <p>Two characters are refused outright, cmd offering no escape that is both correct and + * predictable: a double quote, which has no escape surviving cmd's quote counting and the + * target program's parser at once, and a caret, which the {@code call} used for a single + * command doubles as it re-parses its line, so {@code "a^b"} arrives as {@code "a^^b"}. Pass + * either with {@code script(bashBody, cmdBody)}. + */ + @Override + public String quote(String arg) { + if (arg.indexOf('"') >= 0) { + throw new UnsupportedOperationException( + "cmd.exe has no escape for a double quote that survives both its own quote counting " + + "and the target program's parser. Use script(bashBody, cmdBody) for: " + + arg); + } + if (arg.indexOf('^') >= 0) { + throw new UnsupportedOperationException( + "cmd.exe doubles a caret when 'call' re-parses the line, so it cannot be passed as an " + + "argument. Use script(bashBody, cmdBody) for: " + + arg); + } + String escaped = arg.replace("%", "%%"); + return NEEDS_CMD_QUOTING.matcher(arg).find() ? '"' + escaped + '"' : escaped; Review Comment: An empty argument does not match NEEDS_CMD_QUOTING, so quote("") returns an empty string. Consequently, run(List.of("solr", "…", "")) drops that argument on cmd.exe instead of passing an empty value, while the bash implementation preserves it. Quote empty strings as well so argv-based tests exercise the same arguments on both platforms. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
