Fix raw path handoffs to re-parsing callers - #12
Merged
Conversation
deleteSpecificKey handed the unescaped path ParsePath returned to Read, Write, and deleteEntireSecret, all of which parse their argument again. A colon inside the secret path was split a second time, so the delete landed on a truncated path and left the key in place. Re-encode the path before each of those calls.
Copy parsed the destination and then handed the unescaped result to Read and Write, which parse again. A colon in the destination path was read as a key separator, so the copy failed outright with "cannot write to paths in /path:key notation". Re-encode the destination for those two calls; the tree walk and SecretEntry.Copy keep the literal path they need.
A deep move finished by destroying the source, but passed the path in the escaped syntax the caller wrote. That call goes straight to Vault, which takes literal paths, so a source path holding a colon was never matched and the secret stayed behind while the move reported success. Unescape the path before destroying it.
revert looked the versions up with the raw argument the user typed, but the version list is fetched by literal path and never unescaped. A path holding an escaped colon was reported as a missing secret. Use the parsed path there and in the message that follows it, and encode it again for the write, which does parse its argument.
gen and uuid split the path:key argument themselves, which unescapes the path, then passed the result to Read and Write, which split it again. A colon in the path made both commands fail with "cannot write to paths in /path:key notation". Encode the path back before those calls, so it stays in the same syntax as the branch that takes the path and key as separate arguments.
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.
Completes the path-vocabulary work started in #10.
ParsePathboth splitsand unescapes, so everything it returns is a raw Vault path. Six call
sites then handed that raw path to something that parses its argument a
second time, splitting it at the wrong colon.
Read,Write,deleteEntireSecretParsePathVersions,client.Destroy*ConstructSecretsMeasured behavior
Fixture is a single secret at the literal Vault path
secret/we:ird,which the mini-language writes as
secret/we\:ird. Vault 1.13.2, KV v2.rm 'secret/we\:ird:alpha'mv --deep 'secret/we\:ird' dstcp 'secret/src:k' 'secret/de\:st:kk'cannot write to paths in /path:key notationrevert 'secret/we\:ird' 1no secret exists at pathgen 'secret/we\:ird:pw'cannot write to paths…uuid 'secret/we\:ird:id'cannot write to paths…The first two failed silently:
rmreported success while the keyremained readable, and
mv --deepreported success while every version ofthe source survived with
destroyed:false. A colon-free control confirmsmv --deepdestroys the source correctly, so the colon is the variable.Verification
make checkgreen; every commit builds and tests green in isolation.from
develop.colon-free control to show the fix is specific and does not disturb the
ordinary path.
Notes for reviewers
cmdReverthad a third handoff on the same line of reasoning —Writealso received the raw path — which is fixed here. It is reachable only on
KV v2, where a revert to an older version actually writes; the KV v1 test
fake short-circuits before it. It is covered by the live v2 run above
rather than by a unit test.
Reverting the fix makes
safe genpanic on a nil dereference insideSecret.Password, becausecmdGentreats a key-not-found from thetruncated path as "secret absent" and continues with a nil secret. These
changes make that branch unreachable from
gen/uuid, but the underlyingnil deref is untouched and still latent for other callers. Not fixed here:
it is a separate defect and outside this change's scope.