fix(cli): validate profile names to prevent credentials-file corruption#21
Open
Davidson3556 wants to merge 1 commit into
Open
fix(cli): validate profile names to prevent credentials-file corruption#21Davidson3556 wants to merge 1 commit into
Davidson3556 wants to merge 1 commit into
Conversation
A profile name (`--profile` / `TESTSPRITE_PROFILE`) is written verbatim as
an INI section header (`[name]`) in `~/.testsprite/credentials`, but was
never validated. A name containing the characters that break that grammar
silently corrupted the file:
--profile "prod]" -> serialises to `[prod]]`, which the section regex
cannot match, so the api_key/api_url lines that
follow are DROPPED on read. `setup` reports success
while the credential never persists.
--profile $'a\nb' -> the newline splits the header across two lines.
--profile " x " -> does not round-trip (the parser trims section
names, so it reads back as `x`).
Add `assertValidProfileName` in credentials.ts (a conservative allowlist:
letters, digits, dot, underscore, hyphen — covering `default`, `prod`,
`ci-staging`, `team.qa`) and call it from every profile-keyed entry point
(`readProfile`, `writeProfile`, `deleteProfile`). A malformed name now
throws a typed VALIDATION_ERROR (exit 5) before any file write, instead of
silently corrupting or failing to persist credentials.
Adds unit coverage for the guard and the three entry points, plus a
subprocess regression.
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.
Fixes #20
Describe the changes you have made in this PR -
A profile name (
--profile/TESTSPRITE_PROFILE) is written verbatim as an INI section header ([name]) in~/.testsprite/credentials, but was never validated. A name containing the characters that break that grammar silently corrupted the file:--profile "prod]"→ serialises to[prod]], which the section regex cannot match, so theapi_key/api_urllines that follow are dropped on read.setupreports success while the credential never persists.[ prod ]reads back asprod).Changes:
assertValidProfileNameincredentials.ts— a conservative allowlist (letters, digits, dot, underscore, hyphen) that covers conventional names (default,prod,ci-staging,team.qa) and cannot corrupt the INI file.readProfile,writeProfile,deleteProfile. A malformed name now throws a typedVALIDATION_ERROR(exit 5) before any file write.Demo/Screenshot for feature changes and bug fixes -
Before (main) — the round-trip silently drops the credential (
prod]→ no readable profile):After (this branch) — rejected up front; valid names unaffected:
Tests:
Code Understanding and AI Usage
Did you use AI assistance (ChatGPT, Claude, Copilot, etc.) to write any part of this code?
If you used AI assistance:
Explain your implementation approach:
Problem: the profile name is interpolated straight into an INI section header during serialization (
[${section}]), but the parser only accepts^\[([^\]]+)\]$and trims the captured name. So any name containing], a newline, or surrounding whitespace either fails to match (dropping the section and its key/url lines) or doesn't round-trip — andsetupreports success regardless, leaving the user with credentials that can't be read back.Alternatives considered:
writeProfile). Rejected: reading with a malformed name should also fail clearly rather than silently returning "no profile", so the guard belongs on the read path too.readProfile/writeProfile/deleteProfile), throwing the same typedVALIDATION_ERROR(exit 5) used elsewhere. This catches the bad name on every credential flow (setup, status, logout, and any command that resolves config) before a write can corrupt the file.Key function:
assertValidProfileName(profile)tests against^[A-Za-z0-9._-]+$and throwslocalValidationError('profile', …, 'flag')otherwise. Placed incredentials.tsnext to the parse/serialize logic that defines the constraint (no import cycle —errors.tsdoes not importcredentials.ts).Edge cases handled/tested: accepts
default,prod,ci-staging,team.qa,a_b,P1; rejectsprod],[weird, embedded newline, whitespace-padded, names with spaces or/, and the empty string;writeProfilerejects before creating the file (verified the file is not created).Checklist before requesting a review
Note: Please check Allow edits from maintainers if you would like us to assist in the PR.