Skip to content

fix(cli): validate profile names to prevent credentials-file corruption#21

Open
Davidson3556 wants to merge 1 commit into
TestSprite:mainfrom
Davidson3556:fix/validate-profile-name
Open

fix(cli): validate profile names to prevent credentials-file corruption#21
Davidson3556 wants to merge 1 commit into
TestSprite:mainfrom
Davidson3556:fix/validate-profile-name

Conversation

@Davidson3556

Copy link
Copy Markdown

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 the api_key/api_url lines that follow are dropped on read. setup reports success while the credential never persists.
  • a newline in the name splits the header across two lines.
  • leading/trailing whitespace doesn't round-trip (the parser trims section names, so [ prod ] reads back as prod).

Changes:

  • Add assertValidProfileName in credentials.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.
  • 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.
  • Add unit coverage for the guard and the three entry points, plus a subprocess regression.

Demo/Screenshot for feature changes and bug fixes -

Before (main) — the round-trip silently drops the credential (prod] → no readable profile):

$ node -e "import('./dist/lib/credentials.js').then(({serializeCredentials,parseCredentials})=>{
    const f={'prod]':{apiKey:'sk-123',apiUrl:'https://api.testsprite.com'}};
    console.log(serializeCredentials(f));
    console.log('parsed keys:', Object.keys(parseCredentials(serializeCredentials(f))));
  })"
[prod]]
api_key = sk-123
api_url = https://api.testsprite.com

parsed keys: []          # <- key silently lost; `setup` would still say "configured"

After (this branch) — rejected up front; valid names unaffected:

$ testsprite --profile "prod]" project list
Error: Invalid request.
Flag `--profile` is invalid: must contain only letters, digits, dot, underscore, or hyphen (no spaces, brackets, or newlines).
$ echo $?
5

$ testsprite --profile "ci-staging" project list   # valid name -> passes the guard, proceeds
Error: Authentication is required.
Run `testsprite setup` ...

Tests:

npm test
# 1452 passed
npm run typecheck && npm run lint && npm run format:check
# all clean

Code Understanding and AI Usage

Did you use AI assistance (ChatGPT, Claude, Copilot, etc.) to write any part of this code?

  • No, I wrote all the code myself
  • Yes, I used AI assistance (continue below)

If you used AI assistance:

  • I have reviewed every single line of the AI-generated code
  • I can explain the purpose and logic of each function/component I added
  • I have tested edge cases and understand how the code handles them
  • I have modified the AI output to follow this project's coding standards and conventions

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 — and setup reports success regardless, leaving the user with credentials that can't be read back.

Alternatives considered:

  1. Escape/quote profile names in the INI writer and teach the parser to unescape. Rejected: it complicates the file format, and there's no value in supporting exotic profile names — they're short identifiers.
  2. Validate only at write time (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.
  3. (chosen) A small allowlist validator called from all three profile-keyed entry points (readProfile / writeProfile / deleteProfile), throwing the same typed VALIDATION_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 throws localValidationError('profile', …, 'flag') otherwise. Placed in credentials.ts next to the parse/serialize logic that defines the constraint (no import cycle — errors.ts does not import credentials.ts).

Edge cases handled/tested: accepts default, prod, ci-staging, team.qa, a_b, P1; rejects prod], [weird, embedded newline, whitespace-padded, names with spaces or /, and the empty string; writeProfile rejects before creating the file (verified the file is not created).


Checklist before requesting a review

  • I have added proper PR title and linked to the issue
  • I have performed a self-review of my code
  • I can explain the purpose of every function, class, and logic block I added
  • I understand why my changes work and have tested them thoroughly
  • I have considered potential edge cases and how my code handles them
  • If it is a core feature, I have added thorough tests
  • My code follows the project's style guidelines and conventions

Note: Please check Allow edits from maintainers if you would like us to assist in the 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 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A malformed --profile name silently corrupts ~/.testsprite/credentials (key not persisted)

1 participant