fix(profile): keep legacy profile names usable and expand values consistently - #25
Merged
Merged
Conversation
…istently Two edges of the 0.2.2 profile work, both found by an oversight pass over what landed since 0.2.1. Profile names that earlier versions accepted became unreachable. The path traversal guard added in #23 was an allow-list over the whole character set ([A-Za-z0-9._-]+), which also rejects names 0.2.1 created happily: a space, a plus, an at sign. Such a profile stayed visible in 'config profile list' while show, --profile and delete all refused it, so it could not even be removed through the CLI. The guard is now a deny-list on what actually traverses ('.', '..', and path separators), and the guarantee is carried by resolving the file and asserting its parent is the profiles directory. A name that is illegal on the running platform but legal elsewhere, such as a colon on Windows, now reports a clean message instead of InvalidPathException. Profile values were expanded on one path and not the others. Interpolation of ${env:HOME} and friends is picocli's, applied when it fills an option default, so 'start --profile p' expanded it while 'restart --profile p' and 'config show --profile p' read the Profile bean directly and got the literal text. Restart therefore asked docker for a directory named '${env:HOME}/...', which is the silent state loss #23 set out to fix. Both now resolve through StartCommand.resolvedFor, the same provider and the same precedence a real start invocation uses, so one profile cannot mean two different directories. That also removes the duplicated field mapping in RestartCommand: ProfileDefaults is the only place a profile field maps to a flag again. Restart now resolves the profile before stopping the container, so a profile that fails to resolve no longer leaves the container stopped.
|
| Filename | Overview |
|---|---|
| src/main/java/io/floci/cli/config/ProfileStore.java | ProfileStore accepts legacy Unix names while keeping each profile file directly inside the profile folder. |
| src/main/java/io/floci/cli/config/ProfileDefaultValueProvider.java | ProfileDefaultValueProvider exposes the profile snapshot it cached during parsing. |
| src/main/java/io/floci/cli/commands/StartCommand.java | StartCommand.resolvedFor applies profiles through the same defaults and value expansion as a normal start. |
| src/main/java/io/floci/cli/commands/config/ConfigShowCommand.java | ConfigShowCommand gets declared keys and expanded values from one profile snapshot. |
| src/main/java/io/floci/cli/commands/RestartCommand.java | RestartCommand resolves start settings before stopping the container and reuses those expanded values. |
Sequence Diagram
sequenceDiagram
actor User
participant CLI
participant Store as ProfileStore
participant Provider as ProfileDefaultValueProvider
participant Start as StartCommand
participant Docker
User->>CLI: restart or config show --profile name
CLI->>Provider: resolve profile defaults
Provider->>Store: read profile once
Store-->>Provider: Profile snapshot
Provider-->>Start: expanded option defaults
alt restart
CLI->>Docker: stop container
CLI->>Start: call cached command
Start->>Docker: start with expanded values
else config show
CLI->>Provider: read cached snapshot
CLI-->>User: show declared expanded values
end
Reviews (2): Last reviewed commit: "fix(profile): stop denying backslash, an..." | Re-trigger Greptile
…nfig show Two P2s from the review on this PR, both correct. Backslash is a separator on Windows and an ordinary file-name character on Unix. Denying it outright orphaned a 'team\alpha' profile that 0.2.1 could create and that list() still returns, which is the same bug class this PR exists to fix, left half fixed. The character check is now forward slash only; resolveInProfilesDir already rejects a real Windows traversal on the parent assertion, so the guarantee is unchanged. config show read the profile twice: key presence from one read, values from a second through resolvedFor. A concurrent edit could print half of one version next to half of another. ProfileDefaultValueProvider already memoizes the profile it resolved, so exposing that memo gives both from one snapshot, and resolvedFor gained a provider overload to pass it through. If the profile has gone since the outer parse, the command now fails loudly instead of printing a mixed row. RestartCommand has the same shape between the outer parse and resolvedFor. Closing it means making the outer provider reachable from commands, which is wider than this fix; documented in place rather than left silent.
Contributor
Author
|
🎉 This PR is included in version 0.2.3 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two edges of the 0.2.2 profile work (#23), both surfaced by an oversight pass over what landed since 0.2.1. One of them was raised by Greptile as a P1 inline on #23 and merged un-adjudicated.
Profile names that earlier versions accepted became unreachable
The traversal guard added in #23 was an allow-list over the whole character set (
[A-Za-z0-9._-]+). That also rejects names 0.2.1 created happily, sinceprofileFile()resolved the raw name with no validation at all. Verified against the released 0.2.2 jar:Blast radius is every name containing a space,
+,@or parens.The guard is now a deny-list on what actually traverses (
.,.., path separators), and the guarantee is carried byresolveInProfilesDir, which resolves the file and asserts its parent is the profiles directory. A name that is illegal on the running platform but legal elsewhere, a colon on Windows, now reports a clean message instead ofInvalidPathException.Profile values expanded on one path and not the others
Interpolation of
${env:HOME}is picocli's, applied when it fills an option default. Sostart --profile pexpanded it, whilerestart --profile pandconfig show --profile pread theProfilebean directly and got the literal text. Restart asked docker for a directory named${env:HOME}/floci-data, which is exactly the silent state loss #23 set out to fix, and the README documents the interpolation as a feature.Both now resolve through
StartCommand.resolvedFor, the same provider and precedence a realstartuses. That also removes the duplicated field mapping inRestartCommand, soProfileDefaultsis once again the only place a profile field maps to a flag.Restart also resolves the profile before stopping the container, so a profile that fails to resolve no longer leaves the container stopped.
Verified end to end, real Docker
Plus: a
team alphaprofile now shows, resolves and deletes;--profile ../escapedstill exits 2.Type of change
fix:)feat:)feat!:orfix!:)Product trees affected
commands/gcp/)commands/az/)commands/oci/)All four through the shared
commands/andconfig/classes; no shim changed.Checklist
mvn testpasses locally, 140 tests, up from 131README.mdupdated, n/a: the documented behaviour is unchanged, this makes it true on every pathnative-check --staticclean; no Jackson type added, no dependency added,reflect-config.jsonuntouchedTests
ProfileStoreTest:keepsAcceptingNamesEarlierVersionsAllowedover the six shapes 0.2.1 created;aLegacyNameRoundTripsAndCanBeDeleted;everyResolvedFileSitsDirectlyInTheProfilesDirectorypins the real guarantee; the traversal rejections stay.RestartCommandTest.interpolatesProfileValuesExactlyAsStartDoesasserts the expanded mount in the docker argv.ConfigCommandsTest.reportsTheInterpolatedPersistDirThatStartWouldActuallyUse.Still open from the same pass
Not in this PR:
CMD-1(start --profile p --helpshows the product default, not the profile's),CFG-4(config profile showprintsProfile: nullwherelistprints the name),CFG-5(error messages name a.yamlpath when the file is.yml), and the dead no-argProfileDefaultValueProvider()constructor. Happy to fold any of them in if you would rather they rode along.