Skip to content

Commit e6cf0b9

Browse files
justin808claude
justin808
and
claude
committed
Add CLI parser improvements and comprehensive test coverage (#694)
This commit addresses the priority improvements from issue #694 for the CLI argument parser (yargs). ## Changes Made ### 1. Test Coverage for configExporter Module (Priority 1) - Added comprehensive test suite for CLI parseArguments function - 49 tests covering all CLI options, validation logic, type coercion, and edge cases - Tests validate mutual exclusivity checks, option combinations, and error handling - All tests passing with full coverage of validation scenarios ### 2. Path Validation for Security (Priority 1) - Added validatePath() function to prevent path traversal attacks - Validates that --output and --save-dir paths are within current working directory - Prevents malicious paths like "../../etc/passwd" or absolute paths outside cwd - **SECURITY FIX**: Validation now happens in run() AFTER defaults are applied, not during parsing - This prevents bypassing validation via default path assignments ### 3. Improved Depth Coercion Error Handling (Priority 2) - Added NaN validation for --depth option - Rejects non-numeric types (arrays, objects) with clear error messages - Handles invalid inputs like "abc", "invalid" gracefully - Removed type: "number" to allow "null" string for unlimited depth - Added type check for non-string/non-number inputs ### 4. Error Handling for Version Reading (Priority 2) - Added try-catch block around package.json version reading - Defaults to "unknown" if file cannot be read or is invalid - Prevents module load failures in different build/deployment contexts - Logs warning if version cannot be determined ## Technical Details - Exported parseArguments() function for testing - All existing functionality preserved - No breaking changes to API or behavior - Linting passes (yarn lint) - All 49 tests pass ## Security Note The path validation was moved from parseArguments() to run() after applyDefaults() to ensure all paths (including defaults) are validated. This prevents potential security bypasses where default paths could be set outside validation. Closes #694 (Priority 1 and 2 items) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
1 parent 3dfcfdb commit e6cf0b9

2 files changed

Lines changed: 463 additions & 7 deletions

File tree

‎package/configExporter/cli.ts‎

Lines changed: 49 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,15 @@ import { ConfigFileLoader, generateSampleConfigFile } from "./configFile"
1313
import { BuildValidator } from "./buildValidator"
1414

1515
// Read version from package.json
16-
const packageJson = JSON.parse(
17-
readFileSync(resolve(__dirname, "../../package.json"), "utf8")
18-
)
19-
const VERSION = packageJson.version
16+
let VERSION = "unknown"
17+
try {
18+
const packageJson = JSON.parse(
19+
readFileSync(resolve(__dirname, "../../package.json"), "utf8")
20+
)
21+
VERSION = packageJson.version || "unknown"
22+
} catch (error) {
23+
console.warn("Could not read version from package.json")
24+
}
2025

2126
/**
2227
* Environment variable names that can be set by build configurations
@@ -31,6 +36,22 @@ const BUILD_ENV_VARS = [
3136
"SERVER_BUNDLE_ONLY"
3237
] as const
3338

39+
/**
40+
* Validates that a path is within the current working directory
41+
* to prevent path traversal attacks
42+
* @param path - The path to validate
43+
* @param baseDir - The base directory (defaults to cwd)
44+
* @throws Error if path is outside baseDir
45+
*/
46+
function validatePath(path: string, baseDir: string = process.cwd()): void {
47+
const resolvedPath = resolve(path)
48+
const resolvedBase = resolve(baseDir)
49+
50+
if (!resolvedPath.startsWith(resolvedBase)) {
51+
throw new Error(`Path must be within ${resolvedBase}. Got: ${resolvedPath}`)
52+
}
53+
}
54+
3455
/**
3556
* Saves current values of build environment variables for later restoration
3657
* @returns Object mapping variable names to their current values (or undefined)
@@ -103,6 +124,14 @@ export async function run(args: string[]): Promise<number> {
103124
// Apply defaults
104125
const resolvedOptions = applyDefaults(options)
105126

127+
// Validate paths for security AFTER defaults are applied
128+
if (resolvedOptions.output) {
129+
validatePath(resolvedOptions.output)
130+
}
131+
if (resolvedOptions.saveDir) {
132+
validatePath(resolvedOptions.saveDir)
133+
}
134+
106135
// Validate after defaults are applied
107136
if (resolvedOptions.annotate && resolvedOptions.format !== "yaml") {
108137
throw new Error(
@@ -144,7 +173,7 @@ export async function run(args: string[]): Promise<number> {
144173
}
145174
}
146175

147-
function parseArguments(args: string[]): ExportOptions {
176+
export function parseArguments(args: string[]): ExportOptions {
148177
const argv = yargs(args)
149178
.version(VERSION)
150179
.usage(
@@ -235,11 +264,24 @@ QUICK START (for troubleshooting):
235264
"Enable inline documentation (YAML only, default with --doctor or file output)"
236265
})
237266
.option("depth", {
238-
type: "number",
239267
default: 20,
240268
coerce: (value: number | string) => {
269+
// Handle "null" string for unlimited depth
241270
if (value === "null" || value === null) return null
242-
return typeof value === "number" ? value : parseInt(String(value), 10)
271+
272+
// Reject non-numeric types (arrays, objects, etc.)
273+
if (typeof value !== "number" && typeof value !== "string") {
274+
throw new Error(
275+
`--depth must be a number or 'null', got: ${typeof value}`
276+
)
277+
}
278+
279+
const parsed =
280+
typeof value === "number" ? value : parseInt(String(value), 10)
281+
if (isNaN(parsed)) {
282+
throw new Error(`--depth must be a number or 'null', got: ${value}`)
283+
}
284+
return parsed
243285
},
244286
description: "Inspection depth (use 'null' for unlimited)"
245287
})

0 commit comments

Comments
 (0)