Accept safe's own escaped paths as input - #15
Merged
Conversation
safe prints paths in its own escaped syntax, but the commands that walk a subtree passed the argument straight to the walk, which talks to Vault in literal paths. A root pasted back from safe's own output therefore 404'd on the backslash: `safe tree 'secret/od\:d'` reported no such secret for a path safe had just printed. Resolve the root first. Secrets.Draw wants the literal path too, since it compares against walked paths and escapes only when printing, so it now gets the same value and stops escaping an already-escaped root a second time. A key or a version cannot scope a recursive walk, so naming one is refused rather than quietly dropped.
export validated the argument through ParsePath but then walked the original string, so exporting a root pasted back from safe's own output failed on the literal backslash. Keep the parsed path, which is already canonicalized, and drop the now-redundant Canonicalize call.
versions parsed its argument to reject a version, then handed the unparsed string to the client, which takes literal Vault paths. An escaped path therefore 404'd. A key was not rejected at all: it stayed part of the path and came back as a misleading "no secret exists". Pass the parsed path and refuse a key the way a version already was.
undelete --all looked the version list up under the parsed path, then issued the undelete against the original argument. On an escaped path that reached Vault with a literal backslash: the command reported success and exit 0 while every version stayed deleted. Issue the undelete against the same path the versions came from.
This was referenced Jul 29, 2026
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
1 participant
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.
safe prints paths in its own escaped syntax, so its output should be
usable as its own input. It is not:
Cause
#10 and #12 established the vocabulary split —
Read,Write, anddeleteEntireSecrettake the escaped mini-language because they callParsePath;Versions,client.*, andConstructSecretstake literalVault paths — and fixed the handoffs inside
pkg/vault. The CLI layerwas never converted. Six call sites still pass raw argv to a consumer
that wants a literal path.
Measured, Vault 1.13.2 KV v2
Fixture is one secret at the literal Vault path
secret/od:d, which themini-language writes as
secret/od\:d, plus asecret/odsibling thatmust not be swept in.
tree 'secret/od\:d'no secret exists, rc=1paths 'secret/od\:d'no secret exists, rc=1export 'secret/od\:d'no secret exists, rc=1values -p 'secret/od\:d' vno secret exists, rc=1versions 'secret/od\:d'no secret exists, rc=1undelete --all 'secret/od\:d'versions 'secret/foo:mykey'no secret existsSpecifying key to versions is not supportedundelete --allis the one to look at. It resolved the version listcorrectly against the parsed path, then issued the undelete against the
original argument, so it reported success and exit 0 while every version
stayed deleted. Verified against a live server by reading the KV metadata
directly, with a colon-free control undeleting correctly in the same run:
Two things worth reviewer attention
Secrets.Drawwants a literal path, not an escaped one. It comparesits root against walked paths and applies
EscapePathSegmentonly whenprinting, so passing the already-escaped argument escaped it twice
(
od\\:d). Today that is masked because the walk errors out first; itwould have become visible the moment the walk was fixed. Passing the
resolved root to both fixes them together.
A key or version in a walk root is now refused, not dropped.
safe tree 'secret/od\:d:leaf'previously would have silently walked the wholesubtree, ignoring the key. It now errors, matching what
DeleteTreedoesfor the same reason.
exportalready refused both;versionsrefused aversion but not a key, and now refuses both.
Verification
make checkgreen. Nine newinternal/clitests drive the real commandhandlers against the fake Vault, which stores paths verbatim. All of
them fail with the fix neutered and pass with it — including a test
that feeds
safe pathsoutput back intosafe paths.unchanged from
develop.control, so the fix is specific and does not disturb ordinary paths.
Not covered by a test
versionsandundelete --allneed a KV v2 mount to reach theirsuccess paths, and the CLI fake serves v1 only. Their guards are unit
tested; the success paths are covered by the live v2 run above. Extending
the fake to v2 is worth doing separately — it would also benefit
tree,paths, andget.