Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 96 additions & 0 deletions src/commands/project.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
type CliProject,
type CliUpdateProjectResponse,
createProjectCommand,
MAX_PASSWORD_FILE_BYTES,
runCreate,
runGet,
runList,
Expand Down Expand Up @@ -484,6 +485,66 @@ describe('runCreate', () => {
expect(result.type).toBe('backend');
});

it('P6 — reads --password-file from a bounded regular file', async () => {
const { credentialsPath } = makeCreds();
const dir = mkdtempSync(join(tmpdir(), 'project-password-'));
const passwordPath = join(dir, 'password.txt');
writeFileSync(passwordPath, ' secret-from-file \n', 'utf8');
const sentBodies: unknown[] = [];
const fetchImpl = (async (_input: Parameters<typeof fetch>[0], init: RequestInit = {}) => {
if (init.body) sentBodies.push(JSON.parse(init.body as string) as unknown);
return new Response(JSON.stringify({ ...PROJECT_FIXTURE, id: 'proj_pw' }), {
status: 200,
headers: { 'content-type': 'application/json' },
});
}) as typeof fetch;

await runCreate(
{
profile: 'default',
output: 'json',
debug: false,
type: 'backend',
name: 'Password File',
passwordFile: passwordPath,
},
{ credentialsPath, fetchImpl, stdout: () => {}, stderr: () => {} },
);

expect(sentBodies[0]).toMatchObject({ password: 'secret-from-file' });
});

it('P6 — rejects --password-file directories before network', async () => {
const { credentialsPath } = makeCreds();
const dir = mkdtempSync(join(tmpdir(), 'project-password-dir-'));
const fetchImpl = vi.fn(async () => {
throw new Error('should not hit network');
});

await expect(
runCreate(
{
profile: 'default',
output: 'json',
debug: false,
type: 'backend',
name: 'Bad Password File',
passwordFile: dir,
},
{
credentialsPath,
fetchImpl: fetchImpl as unknown as typeof fetch,
stdout: () => {},
stderr: () => {},
},
),
).rejects.toMatchObject({
code: 'VALIDATION_ERROR',
details: expect.objectContaining({ field: 'password-file' }),
});
expect(fetchImpl).not.toHaveBeenCalled();
});

it('P6 — dry-run returns canned shape without hitting the network', async () => {
const { credentialsPath } = makeCreds();
const fetchImpl = vi.fn(async () => {
Expand Down Expand Up @@ -641,6 +702,41 @@ describe('runUpdate', () => {
expect(fetchImpl).not.toHaveBeenCalled();
});

it('P7 — rejects oversized --password-file before network', async () => {
const { credentialsPath } = makeCreds();
const dir = mkdtempSync(join(tmpdir(), 'project-password-big-'));
const passwordPath = join(dir, 'password.txt');
writeFileSync(passwordPath, 'x'.repeat(MAX_PASSWORD_FILE_BYTES + 1), 'utf8');
const fetchImpl = vi.fn(async () => {
throw new Error('should not hit network');
});

await expect(
runUpdate(
{
profile: 'default',
output: 'json',
debug: false,
projectId: 'proj_abc',
passwordFile: passwordPath,
},
{
credentialsPath,
fetchImpl: fetchImpl as unknown as typeof fetch,
stdout: () => {},
stderr: () => {},
},
),
).rejects.toMatchObject({
code: 'PAYLOAD_TOO_LARGE',
details: expect.objectContaining({
field: 'password-file',
maxBytes: MAX_PASSWORD_FILE_BYTES,
}),
});
expect(fetchImpl).not.toHaveBeenCalled();
});

it('P7 — dry-run returns canned shape without network call', async () => {
const { credentialsPath } = makeCreds();
const fetchImpl = vi.fn(async () => {
Expand Down
54 changes: 51 additions & 3 deletions src/commands/project.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { randomUUID } from 'node:crypto';
import { readFileSync } from 'node:fs';
import { readFileSync, statSync } from 'node:fs';
import { resolve } from 'node:path';
import { Command } from 'commander';
import {
makeHttpClient,
Expand All @@ -19,6 +20,8 @@ import {
type PaginationFlags,
} from '../lib/pagination.js';

export const MAX_PASSWORD_FILE_BYTES = 64 * 1024;

export interface CliProject {
id: string;
name: string;
Expand Down Expand Up @@ -185,7 +188,7 @@ export async function runCreate(
// Resolve password: flag > file > none
let password = opts.password;
if (password === undefined && opts.passwordFile !== undefined) {
password = readFileSync(opts.passwordFile, 'utf8').trim();
password = readPasswordFileGuarded(opts.passwordFile);
}

const idempotencyKey = opts.idempotencyKey ?? `cli-proj-create-${randomUUID()}`;
Expand Down Expand Up @@ -257,7 +260,7 @@ export async function runUpdate(
// Resolve password
let password = opts.password;
if (password === undefined && opts.passwordFile !== undefined) {
password = readFileSync(opts.passwordFile, 'utf8').trim();
password = readPasswordFileGuarded(opts.passwordFile);
}
Comment on lines 260 to 264

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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


// P2-7: guard --url against localhost/RFC1918/non-http(s).
Expand Down Expand Up @@ -595,6 +598,51 @@ function renderUpdateText(r: CliUpdateProjectResponse): string {
].join('\n');
}

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();
}
Comment on lines +601 to +632

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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 -S

Repository: 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 || true

Repository: 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.ts

Repository: TestSprite/testsprite-cli

Length of output: 19004


Wrap readFileSync in the same validation pathreadPasswordFileGuarded() 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.

Suggested change
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


function passwordFileError(reason: string, details: Record<string, unknown>): ApiError {
return ApiError.fromEnvelope({
error: {
code: 'VALIDATION_ERROR',
message: 'Invalid request.',
nextAction: `Flag \`--password-file\` is invalid: ${reason}.`,
requestId: 'local',
details: { field: 'password-file', reason, ...details },
},
});
}

function localValidationError(message: string): ApiError {
return ApiError.fromEnvelope({
error: {
Expand Down
37 changes: 36 additions & 1 deletion src/lib/pagination.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
import { describe, expect, it } from 'vitest';
import { ApiError } from './errors.js';
import { paginate, validatePaginationFlags, type FetchPage, type Page } from './pagination.js';
import {
MAX_AUTO_PAGES,
paginate,
validatePaginationFlags,
type FetchPage,
type Page,
} from './pagination.js';

function makePages<T>(pages: Page<T>[]): {
fetchPage: FetchPage<T>;
Expand Down Expand Up @@ -88,6 +94,35 @@ describe('paginate', () => {
expect(calls[0]!.cursor).toBe('resume');
});

it('rejects a repeated nextToken instead of following a cursor cycle forever', async () => {
const { fetchPage, calls } = makePages([
{ items: [1], nextToken: 'cursor-a' },
{ items: [2], nextToken: 'cursor-b' },
{ items: [3], nextToken: 'cursor-a' },
]);

await expect(paginate(fetchPage)).rejects.toMatchObject({
code: 'UNAVAILABLE',
details: expect.objectContaining({ reason: 'repeated_next_token' }),
});
expect(calls).toHaveLength(3);
});

it('rejects when auto-pagination exceeds the hard page budget', async () => {
const { fetchPage, calls } = makePages(
Array.from({ length: MAX_AUTO_PAGES + 1 }, (_, i) => ({
items: [i],
nextToken: `cursor-${i}`,
})),
);

await expect(paginate(fetchPage)).rejects.toMatchObject({
code: 'UNAVAILABLE',
details: expect.objectContaining({ reason: 'max_pages_exceeded' }),
});
expect(calls).toHaveLength(MAX_AUTO_PAGES);
});

it('shrinks the per-call pageSize when remaining < pageSize', async () => {
const { fetchPage, calls } = makePages([
{ items: [1], nextToken: 'cursor-x' },
Expand Down
33 changes: 32 additions & 1 deletion src/lib/pagination.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { HttpClient } from './http.js';
import { localValidationError } from './errors.js';
import { ApiError, localValidationError } from './errors.js';

/**
* Page shape returned by every list endpoint per
Expand All @@ -22,6 +22,7 @@ export interface PaginationFlags {

const HARD_PAGE_SIZE_CAP = 100;
const DEFAULT_PAGE_SIZE = 25;
export const MAX_AUTO_PAGES = 1000;

/**
* Validates and normalizes pagination flags. Per the CLI OpenAPI spec
Expand Down Expand Up @@ -91,14 +92,24 @@ export async function paginate<T>(
const items: T[] = [];
let cursor: string | undefined = flags.startingToken;
let lastNextToken: string | null = null;
let pagesFetched = 0;
const seenNextTokens = new Set<string>();
if (cursor !== undefined) seenNextTokens.add(cursor);

while (true) {
const remaining = maxItems !== undefined ? maxItems - items.length : Infinity;
if (remaining <= 0) break;
if (pagesFetched >= MAX_AUTO_PAGES) {
throw paginationSafetyError('max_pages_exceeded', {
maxPages: MAX_AUTO_PAGES,
cursor,
});
}

const callPageSize = Number.isFinite(remaining) ? Math.min(pageSize, remaining) : pageSize;

const page = await fetchPage({ pageSize: callPageSize, cursor });
pagesFetched += 1;
lastNextToken = page.nextToken;

for (const item of page.items) {
Expand All @@ -107,6 +118,13 @@ export async function paginate<T>(
}

if (page.nextToken === null) break;
if (seenNextTokens.has(page.nextToken)) {
throw paginationSafetyError('repeated_next_token', {
nextToken: page.nextToken,
pagesFetched,
});
}
seenNextTokens.add(page.nextToken);
cursor = page.nextToken;
}

Expand All @@ -129,3 +147,16 @@ export async function fetchSinglePage<T>(
query: { ...extraQuery, pageSize, cursor },
});
}

function paginationSafetyError(reason: string, details: Record<string, unknown>): ApiError {
return ApiError.fromEnvelope({
error: {
code: 'UNAVAILABLE',
message: 'Pagination did not make safe progress.',
nextAction:
'Retry with --page-size and --starting-token, or use --max-items to bound auto-pagination. Report this if the server keeps returning cursors indefinitely.',
requestId: 'local',
details: { reason, ...details },
},
});
}
Loading