fix(cli): vault names join with "--", not "/" — the server rejects slashes - #112
Merged
Conversation
…ashes #109 addressed vaults as <project>/<env> and shipped broken: every push failed with Vault name must be lowercase letters, numbers, and dashes. The vault-create endpoint slugifies through /^[a-z0-9][a-z0-9-]{0,62}$/ (apps/pwa/src/routes/credshare.mjs), so a "/" join is refused outright. Nothing in the CLI ever saw it, because the tests exercised vaultName and splitVaultName in isolation and never made a request — the one assumption that mattered, that the server takes an arbitrary vault name, was the one left unverified. Switches the separator to "--", which is inside the allowed character set and still splits unambiguously since neither half may contain one. A single dash would not: "a-b" + "c" and "a" + "b-c" would collide. Also validates the joined name against the server's own regex before the request, so a bad name fails locally with a useful message rather than a 422 after the .env has been read. The tests now assert the produced name matches that regex, so the separator cannot drift back out of the allowed set without failing. Verified end to end against app.logicsrc.com: push, then pull into a scratch file and diff — keys and values both round-trip losslessly. Then 49 repos pushed under the profullstack team; server reports 49 vaults, 169 secrets, 0 failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
vu1nz Security Review0 finding(s) in PR #? No security issues found. |
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.
The bug
#109 addressed team vaults as
<project>/<env>and shipped broken. Every push failed:The vault-create endpoint slugifies through
/^[a-z0-9][a-z0-9-]{0,62}$/(apps/pwa/src/routes/credshare.mjs:19,172), so a/join is refused outright.teams push,teams pullandteams grantwere all unusable on the new signature.Why the tests missed it
They exercised
vaultName()andsplitVaultName()in isolation and never made a request. The join was self-consistent and round-tripped perfectly — against itself. The one assumption that actually mattered, that the server accepts an arbitrary vault name, was the one nothing checked. #109's own description asserted "the server still stores a single opaque vault name"; that was wrong, and no test could have caught it because no test crossed the boundary.The fix
Separator becomes
--, which is inside the server's allowed character set and still splits unambiguously, since neither half may contain one. A single dash would not work:"a-b" + "c"and"a" + "b-c"both producea-b-c.vaultName()now also validates the joined name against the server's own regex before the request, so an over-long or badly-cased name fails locally with a clear message instead of a 422 after the.envhas already been read.Tests
14 cases, up from 8. The ones that would have caught this:
produces a name the server will accept— asserts the output againstSERVER_SLUG, the server's regex copied verbatimnever uses a separator the server rejects— assertsVAULT_SEPis within[a-z0-9-]keeps a dashed project distinct from a dashed env— pins the reason for--over-rejects characters the server would refuse/rejects a combined name past the server's 63-character limitEnd-to-end verification
Against the real
app.logicsrc.com, which is what #109 lacked:teams push profullstack ai-ollama prod→ vaultai-ollama--prod, 1 key appliedteams pullinto a scratch file, diffed against the source — keys and values both round-trip losslessly*--prod, 169 secrets stored.Note on existing vaults
No migration needed — #109's scheme never successfully created a vault, so there is nothing named
project/envanywhere. Vaults predating #109 keep their single-word names and still don't decompose inteams vaults, exactly as before.🤖 Generated with Claude Code