Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 15 additions & 28 deletions src/main/java/io/floci/cli/commands/RestartCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,10 @@

import io.floci.cli.GlobalOptions;
import io.floci.cli.ProductProfile;
import io.floci.cli.config.Profile;
import io.floci.cli.config.ProfileStore;
import io.floci.cli.output.Printer;
import picocli.CommandLine.*;

import java.io.IOException;
import java.util.Optional;
import java.util.concurrent.Callable;

@Command(
Expand Down Expand Up @@ -44,6 +41,11 @@ public RestartCommand(ProductProfile profile, ProfileStore store) {
public Integer call() {
Printer printer = global.printer();

// Resolve the profile BEFORE stopping anything. A failure here must not leave the
// container stopped, and the values must not be re-read from a file that could change
// during the stop plus the one second wait below.
StartCommand start = buildStartCommand();

StopCommand stop = new StopCommand(profile);
stop.global = global;
stop.remove = false;
Expand All @@ -54,40 +56,25 @@ public Integer call() {
// Minimal wait to avoid port-already-in-use races
try { Thread.sleep(1000); } catch (InterruptedException e) { Thread.currentThread().interrupt(); }

return buildStartCommand().call();
return start.call();
}

/**
* The {@code start} this restart will run. Picocli never parses this instance, so the profile
* that {@code ProfileDefaultValueProvider} applied to the parsed command line has to be
* carried across by hand — without it, {@code restart --profile x} would silently drop the
* profile's persistence directory and service list.
* has to be applied by hand, and copying fields out of the Profile bean is not good enough:
* values reach a real {@code start} through picocli, which interpolates {@code ${env:HOME}}
* and friends. Going through {@link StartCommand#resolvedFor} keeps the two paths on the same
* provider, the same precedence and the same interpolation, so one profile cannot mean one
* directory on {@code start} and a different one on {@code restart}.
*/
public StartCommand buildStartCommand() {
StartCommand start = new StartCommand(profile);
StartCommand start = StartCommand.resolvedFor(profile, store, global.profile);

// The real invocation's globals win: they already carry the profile plus any flag the
// user passed, resolved once by the outer parse.
start.global = global;
start.pull = "missing";
start.detach = false;

// port and image already hold the product defaults, set by the StartCommand constructor.
if (global.profile != null) {
loadProfile().ifPresent(p -> {
if (p.port != null) start.port = p.port;
if (p.image != null) start.image = p.image;
if (p.persistDir != null) start.persistDir = p.persistDir;
if (p.services != null) start.services = p.services;
});
}
return start;
}

private Optional<Profile> loadProfile() {
try {
return store.get(global.profile);
} catch (IOException | IllegalArgumentException e) {
// Parsing already resolved this name, so a failure here cannot happen in practice;
// keeping the product defaults beats aborting a restart that has already stopped.
return Optional.empty();
}
}
}
30 changes: 30 additions & 0 deletions src/main/java/io/floci/cli/commands/StartCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,13 @@

import io.floci.cli.GlobalOptions;
import io.floci.cli.ProductProfile;
import io.floci.cli.config.ProfileDefaultValueProvider;
import io.floci.cli.config.ProfileStore;
import io.floci.cli.docker.DockerClient;
import io.floci.cli.docker.DockerException;
import io.floci.cli.output.Ansi;
import io.floci.cli.output.Printer;
import picocli.CommandLine;
import picocli.CommandLine.*;

import java.net.URI;
Expand Down Expand Up @@ -54,6 +57,33 @@ protected StartCommand(ProductProfile profile) {
@Option(names = {"--pull"}, description = "Image pull policy: always, missing, never", defaultValue = "missing", paramLabel = "always|missing|never")
String pull;

/**
* A {@code StartCommand} with {@code profileName} applied by exactly the machinery a real
* {@code start} invocation uses: same provider, same precedence, same {@code ${...}}
* interpolation. Anything that needs to know what a profile would start with must go through
* here rather than reading the {@link io.floci.cli.config.Profile} bean, or it silently
* disagrees with {@code start} on any interpolated value.
*/
public static StartCommand resolvedFor(ProductProfile product, ProfileStore store, String profileName) {
StartCommand start = new StartCommand(product);
if (profileName != null) {
new CommandLine(start)
.setCaseInsensitiveEnumValuesAllowed(true)
.setDefaultValueProvider(new ProfileDefaultValueProvider(store))
.parseArgs("--profile", profileName);
}
return start;
}

/** What a profile-resolved instance would start with. Read by {@code config show}. */
public String image() { return image; }

public int port() { return port; }

public String persistDir() { return persistDir; }

public String services() { return services; }

/**
* The {@code docker run} arguments this invocation would use. Extracted from {@link #call()}
* as a test seam so the persistence mount can be pinned without starting a container;
Expand Down
28 changes: 18 additions & 10 deletions src/main/java/io/floci/cli/commands/config/ConfigShowCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import io.floci.cli.GlobalOptions;
import io.floci.cli.ProductProfile;
import io.floci.cli.commands.StartCommand;
import io.floci.cli.config.Profile;
import io.floci.cli.config.ProfileStore;
import io.floci.cli.output.Ansi;
Expand Down Expand Up @@ -58,12 +59,7 @@ public Integer call() {

// image/port/persistDir/services have no global option, so they are read back from the
// profile itself — they only take effect on 'start'.
startSettings().ifPresent(p -> {
if (p.image != null) data.put("image", p.image);
if (p.port != null) data.put("port", p.port);
if (p.persistDir != null) data.put("persistDir", p.persistDir);
if (p.services != null) data.put("services", p.services);
});
addStartSettings(data);

if (printer.format() != OutputFormat.text) {
printer.structured(data);
Expand All @@ -77,14 +73,26 @@ public Integer call() {
return 0;
}

private Optional<Profile> startSettings() {
if (global.profile == null) return Optional.empty();
// Values come from a profile-resolved StartCommand rather than the Profile bean, so an
// interpolated persistDir reads here exactly as 'start' would use it. The bean is still
// consulted for presence, so a product default never shows up looking like a profile value.
private void addStartSettings(Map<String, Object> data) {
if (global.profile == null) return;
Optional<Profile> declared;
try {
return store.get(global.profile);
declared = store.get(global.profile);
} catch (IOException | IllegalArgumentException e) {
// Parsing already resolved this name; nothing useful to add here.
return Optional.empty();
return;
}
if (declared.isEmpty()) return;

Profile p = declared.get();
StartCommand resolved = StartCommand.resolvedFor(profile, store, global.profile);
Comment thread
greptile-apps[bot] marked this conversation as resolved.
Outdated
if (p.image != null) data.put("image", resolved.image());
if (p.port != null) data.put("port", resolved.port());
if (p.persistDir != null) data.put("persistDir", resolved.persistDir());
if (p.services != null) data.put("services", resolved.services());
}

private static String label(String key) {
Expand Down
38 changes: 33 additions & 5 deletions src/main/java/io/floci/cli/config/ProfileStore.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.InvalidPathException;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.List;
Expand All @@ -17,7 +18,13 @@ public class ProfileStore {

// A profile name becomes a file name, so it must not be able to traverse out of the
// profiles directory: 'floci config profile delete ../../foo' resolved the raw name.
private static final Pattern VALID_NAME = Pattern.compile("[A-Za-z0-9._-]+");
//
// This is a deny-list on purpose. An allow-list over the whole character set also rejects
// names earlier versions accepted (a space, '+', '@'), which left those profiles listed by
// 'config profile list' and unreachable by show, --profile and delete, with no way to remove
// them through the CLI. The guarantee is carried by resolveInProfilesDir below, not by the
// character rules.
private static final Pattern PATH_SEPARATOR = Pattern.compile("[/\\\\]");
Comment thread
hectorvent marked this conversation as resolved.
Outdated

private final Path profilesDir;

Expand All @@ -39,9 +46,9 @@ public static String validateName(String name) {
"Profile name must not be empty.\n"
+ "Run 'floci config profile list' to see available profiles.");
}
if (".".equals(name) || "..".equals(name) || !VALID_NAME.matcher(name).matches()) {
if (".".equals(name) || "..".equals(name) || PATH_SEPARATOR.matcher(name).find()) {
throw new IllegalArgumentException(
"Invalid profile name '" + name + "'. Use only letters, digits, '.', '_' and '-'.\n"
"Invalid profile name '" + name + "'. It must not be '.', '..', or contain a path separator.\n"
+ "Run 'floci config profile list' to see available profiles.");
}
return name;
Expand Down Expand Up @@ -85,15 +92,36 @@ public boolean delete(String name) throws IOException {

/** Where {@link #save} writes {@code name}. Always {@code .yaml}. */
public Path profileFile(String name) {
return profilesDir.resolve(validateName(name) + ".yaml");
return resolveInProfilesDir(validateName(name) + ".yaml");
}

// The traversal guarantee: whatever the name looks like, the file it resolves to must sit
// directly in profilesDir. Also the place a name that is illegal on this platform but legal
// on another (a colon on Windows) turns into a clean message instead of InvalidPathException.
private Path resolveInProfilesDir(String fileName) {
Path dir = profilesDir.toAbsolutePath().normalize();
Path resolved;
try {
resolved = dir.resolve(fileName).normalize();
} catch (InvalidPathException e) {
throw new IllegalArgumentException(
"Invalid profile name '" + fileName + "': not a valid file name on this platform.\n"
+ "Run 'floci config profile list' to see available profiles.");
}
if (!dir.equals(resolved.getParent())) {
throw new IllegalArgumentException(
"Invalid profile name '" + fileName + "': it would resolve outside " + profilesDir + ".\n"
+ "Run 'floci config profile list' to see available profiles.");
}
return resolved;
}

// list() has always accepted .yml, so reads must too — otherwise a .yml profile shows up in
// 'config profile list' and then reports "not found" when passed to --profile.
private Path existingProfileFile(String name) {
Path yaml = profileFile(name);
if (Files.exists(yaml)) return yaml;
Path yml = profilesDir.resolve(validateName(name) + ".yml");
Path yml = resolveInProfilesDir(validateName(name) + ".yml");
return Files.exists(yml) ? yml : yaml;
}

Expand Down
10 changes: 10 additions & 0 deletions src/test/java/io/floci/cli/unit/ConfigCommandsTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,16 @@ void anExplicitFlagIsNoLongerDiscardedByTheProfile() throws Exception {
assertTrue(out.contains("\"container\" : \"floci-probe\""), out);
}

@Test
void reportsTheInterpolatedPersistDirThatStartWouldActuallyUse() throws Exception {
writeProfile("interp", "persistDir: ${env:HOME}/floci-data\n");

String out = runShow(ProductProfile.AWS, "--profile", "interp", "-o", "json");

assertTrue(out.contains("\"persistDir\" : \"" + System.getenv("HOME") + "/floci-data\""), out);
assertFalse(out.contains("${env:"), out);
}

@Test
void withoutAProfileItReportsTheProductDefaults() {
String out = runShow(ProductProfile.GCP, "-o", "json");
Expand Down
32 changes: 31 additions & 1 deletion src/test/java/io/floci/cli/unit/ProfileStoreTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,29 @@ void acceptsOrdinaryNames(String name) {
assertEquals(name, ProfileStore.validateName(name));
}

/**
* 0.2.1 resolved the raw name, so every one of these was creatable. An allow-list on the
* character set orphaned them: listed by 'config profile list', rejected by show, --profile
* and delete, with no way to remove them through the CLI.
*/
@ParameterizedTest
@ValueSource(strings = {"", " ", ".", "..", "../x", "../../etc/passwd", "a/b", "a\\b", "/abs", "a:b"})
@ValueSource(strings = {"team alpha", "prod+eu", "dev@local", "staging(1)", "a b", "sam's"})
void keepsAcceptingNamesEarlierVersionsAllowed(String name) {
assertEquals(name, ProfileStore.validateName(name));
}

@Test
void aLegacyNameRoundTripsAndCanBeDeleted() throws Exception {
Files.createDirectories(tempDir);
Files.writeString(tempDir.resolve("team alpha.yaml"), "container: floci-team\n");

assertEquals("floci-team", store().get("team alpha").orElseThrow().container);
assertTrue(store().delete("team alpha"));
assertTrue(store().get("team alpha").isEmpty());
}

@ParameterizedTest
@ValueSource(strings = {"", " ", ".", "..", "../x", "../../etc/passwd", "a/b", "a\\b", "/abs"})
void rejectsNamesThatCouldEscapeTheProfilesDirectory(String name) {
assertThrows(IllegalArgumentException.class, () -> ProfileStore.validateName(name), name);
}
Expand All @@ -109,4 +130,13 @@ void traversalNeverEscapesTheProfilesDirectory() {
assertThrows(IllegalArgumentException.class, () -> store().profileFile("../../escaped"));
assertThrows(IllegalArgumentException.class, () -> store().delete("../../escaped"));
}

/** The guarantee is the resolved parent, not the character rules. */
@Test
void everyResolvedFileSitsDirectlyInTheProfilesDirectory() {
for (String name : new String[]{"ok", "team alpha", "prod+eu", "dev@local", "..leading"}) {
assertEquals(tempDir.toAbsolutePath().normalize(),
store().profileFile(name).getParent(), name);
}
}
}
17 changes: 17 additions & 0 deletions src/test/java/io/floci/cli/unit/RestartCommandTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,23 @@ void carriesTheProfileIntoTheStartItRuns() throws Exception {
"floci/floci:enforced"), args);
}

/**
* Values reach a real 'start' through picocli, which interpolates ${env:...}. Restart applies
* the profile itself, so it has to go through the same machinery or one profile means two
* different host directories depending on which command you ran.
*/
@Test
void interpolatesProfileValuesExactlyAsStartDoes() throws Exception {
writeProfile("interp", "persistDir: ${env:HOME}/floci-data\n");

List<String> args = parse(ProductProfile.AWS, "--profile", "interp")
.buildStartCommand().dockerRunArgs(SOCKET);

assertTrue(args.contains(System.getenv("HOME") + "/floci-data:/app/data"),
"restart must expand the profile the way start does, but got: " + args);
assertFalse(args.stream().anyMatch(a -> a.contains("${env:")), args.toString());
}

@Test
void withoutAProfileItStillUsesTheProductDefaults() {
List<String> args = parse(ProductProfile.GCP).buildStartCommand().dockerRunArgs(SOCKET);
Expand Down
Loading