Recovered: fix: bound pagination and guard password file reads (#61 by @merlinsantiago982-cmd)#248
Conversation
WalkthroughThe CLI now bounds ChangesPassword-file safety
Pagination safety
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: private package registry requires authentication. Disable ESLint in CodeRabbit settings or use public packages. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/project.ts`:
- Around line 260-264: The runUpdate password resolution currently reads
opts.passwordFile before the dry-run path, causing filesystem access during dry
runs. Move the readPasswordFileGuarded call in runUpdate to after the dry-run
early return, matching runCreate while preserving direct opts.password usage.
- Around line 601-632: Update readPasswordFileGuarded so readFileSync failures
are caught and converted through passwordFileError, matching the existing
statSync validation path and preserving the absolute path and underlying error
details. Keep successful reads trimmed and retain the existing regular-file and
size validations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e21e3951-ddbd-4942-855e-5e68d7115bec
📒 Files selected for processing (4)
src/commands/project.test.tssrc/commands/project.tssrc/lib/pagination.test.tssrc/lib/pagination.ts
| // Resolve password | ||
| let password = opts.password; | ||
| if (password === undefined && opts.passwordFile !== undefined) { | ||
| password = readFileSync(opts.passwordFile, 'utf8').trim(); | ||
| password = readPasswordFileGuarded(opts.passwordFile); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file and nearby dry-run logic
git ls-files src/commands/project.ts
wc -l src/commands/project.ts
sed -n '140,340p' src/commands/project.ts
# Find the helper to see whether it touches the filesystem
rg -n "readPasswordFileGuarded|dryRun|passwordFile|presentFields|mutableFields" src/commands/project.ts src -g '!**/dist/**'Repository: TestSprite/testsprite-cli
Length of output: 39461
Defer --password-file reads until after the dry-run return runUpdate still calls readPasswordFileGuarded() before opts.dryRun, so --dry-run --password-file <path> performs statSync/readFileSync and breaks offline parity. Move the file read behind the dry-run early return, as in runCreate.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/project.ts` around lines 260 - 264, The runUpdate password
resolution currently reads opts.passwordFile before the dry-run path, causing
filesystem access during dry runs. Move the readPasswordFileGuarded call in
runUpdate to after the dry-run early return, matching runCreate while preserving
direct opts.password usage.
Source: Path instructions
| function readPasswordFileGuarded(path: string): string { | ||
| const absolute = resolve(path); | ||
| let stat; | ||
| try { | ||
| stat = statSync(absolute); | ||
| } catch (err) { | ||
| const message = err instanceof Error ? err.message : String(err); | ||
| throw passwordFileError('must point to a readable file', { path: absolute, error: message }); | ||
| } | ||
|
|
||
| if (!stat.isFile()) { | ||
| throw passwordFileError('must point to a regular file', { path: absolute }); | ||
| } | ||
|
|
||
| if (stat.size > MAX_PASSWORD_FILE_BYTES) { | ||
| throw ApiError.fromEnvelope({ | ||
| error: { | ||
| code: 'PAYLOAD_TOO_LARGE', | ||
| message: 'Password file is too large.', | ||
| nextAction: `Flag \`--password-file\` is invalid: file must be at most ${MAX_PASSWORD_FILE_BYTES} bytes.`, | ||
| requestId: 'local', | ||
| details: { | ||
| field: 'password-file', | ||
| sizeBytes: stat.size, | ||
| maxBytes: MAX_PASSWORD_FILE_BYTES, | ||
| }, | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| return readFileSync(absolute, 'utf8').trim(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and nearby error handling.
ast-grep outline src/commands/project.ts --view expanded
# Read the target section around the password-file helper.
sed -n '560,670p' src/commands/project.ts
# Find where readPasswordFileGuarded is used and how errors are handled at the command layer.
rg -n "readPasswordFileGuarded|passwordFileError|ApiError\.fromEnvelope|PAYLOAD_TOO_LARGE|exit code|validation error" src/commands src/lib -SRepository: TestSprite/testsprite-cli
Length of output: 15553
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the create/update flows around password-file handling.
sed -n '150,290p' src/commands/project.ts
# Show the command wiring and any local try/catch around execution.
sed -n '322,430p' src/commands/project.ts
# Inspect the top-level error mapping mentioned in the file.
sed -n '330,355p' src/commands/project.ts
# Inspect the CLI exit-code handling in the entrypoint.
fd -a 'index.ts' . | head -n 20
sed -n '1,220p' index.ts 2>/dev/null || trueRepository: TestSprite/testsprite-cli
Length of output: 11058
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the top-level CLI error mapping.
sed -n '1,260p' src/index.ts
# Inspect the error-to-exit-code mapping for plain errors vs ApiError.
sed -n '1,140p' src/lib/errors.ts
sed -n '140,240p' src/lib/errors.tsRepository: TestSprite/testsprite-cli
Length of output: 19004
Wrap readFileSync in the same validation path — readPasswordFileGuarded() still lets a post-statSync read failure escape as a plain Error, so src/index.ts maps it to exit code 1 instead of the documented validation exit code 5.
🛡️ Proposed fix
- return readFileSync(absolute, 'utf8').trim();
+ try {
+ return readFileSync(absolute, 'utf8').trim();
+ } catch (err) {
+ const message = err instanceof Error ? err.message : String(err);
+ throw passwordFileError('must point to a readable file', { path: absolute, error: message });
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function readPasswordFileGuarded(path: string): string { | |
| const absolute = resolve(path); | |
| let stat; | |
| try { | |
| stat = statSync(absolute); | |
| } catch (err) { | |
| const message = err instanceof Error ? err.message : String(err); | |
| throw passwordFileError('must point to a readable file', { path: absolute, error: message }); | |
| } | |
| if (!stat.isFile()) { | |
| throw passwordFileError('must point to a regular file', { path: absolute }); | |
| } | |
| if (stat.size > MAX_PASSWORD_FILE_BYTES) { | |
| throw ApiError.fromEnvelope({ | |
| error: { | |
| code: 'PAYLOAD_TOO_LARGE', | |
| message: 'Password file is too large.', | |
| nextAction: `Flag \`--password-file\` is invalid: file must be at most ${MAX_PASSWORD_FILE_BYTES} bytes.`, | |
| requestId: 'local', | |
| details: { | |
| field: 'password-file', | |
| sizeBytes: stat.size, | |
| maxBytes: MAX_PASSWORD_FILE_BYTES, | |
| }, | |
| }, | |
| }); | |
| } | |
| return readFileSync(absolute, 'utf8').trim(); | |
| } | |
| function readPasswordFileGuarded(path: string): string { | |
| const absolute = resolve(path); | |
| let stat; | |
| try { | |
| stat = statSync(absolute); | |
| } catch (err) { | |
| const message = err instanceof Error ? err.message : String(err); | |
| throw passwordFileError('must point to a readable file', { path: absolute, error: message }); | |
| } | |
| if (!stat.isFile()) { | |
| throw passwordFileError('must point to a regular file', { path: absolute }); | |
| } | |
| if (stat.size > MAX_PASSWORD_FILE_BYTES) { | |
| throw ApiError.fromEnvelope({ | |
| error: { | |
| code: 'PAYLOAD_TOO_LARGE', | |
| message: 'Password file is too large.', | |
| nextAction: `Flag \`--password-file\` is invalid: file must be at most ${MAX_PASSWORD_FILE_BYTES} bytes.`, | |
| requestId: 'local', | |
| details: { | |
| field: 'password-file', | |
| sizeBytes: stat.size, | |
| maxBytes: MAX_PASSWORD_FILE_BYTES, | |
| }, | |
| }, | |
| }); | |
| } | |
| try { | |
| return readFileSync(absolute, 'utf8').trim(); | |
| } catch (err) { | |
| const message = err instanceof Error ? err.message : String(err); | |
| throw passwordFileError('must point to a readable file', { path: absolute, error: message }); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/project.ts` around lines 601 - 632, Update
readPasswordFileGuarded so readFileSync failures are caught and converted
through passwordFileError, matching the existing statSync validation path and
preserving the absolute path and underlying error details. Keep successful reads
trimmed and retain the existing regular-file and size validations.
Source: Path instructions
|
Hi @merlinsantiago982-cmd 👋 — this is the recovery of your PR #61 , which was auto-closed by an error in our 2026-07-09 release process. Your commits are intact — thank you for the contribution! It currently shows merge conflicts because Option A — merge (no force-push): git checkout BRANCH
git fetch upstream # your remote for TestSprite/testsprite-cli (may be "origin")
git merge upstream/main # resolve conflicts, commit
git pushOption B — rebase (cleaner history, needs force): git checkout BRANCH
git fetch upstream
git rebase upstream/main # resolve conflicts
git push --force-with-leaseOnce you push, the conflicts here resolve automatically and we'll take it into review. Sorry again for the extra step! |
2 participants
Summary
--password-filereads with stat-first validation, regular-file checks, and a 64 KiB size capWhy
A malicious or broken server could keep returning non-null cursors indefinitely, causing the CLI to loop and grow memory.
--password-filealso read arbitrary paths without checking the target type or size before sending the content as the project password.Testing
Fixes #58
Summary by CodeRabbit
--password-filevalidation for project creation and updates.