fix adding enum value & command line exception

[jianyexi]

Fix issues:
#254
Azure/azure-sdk-tools#3748

[mikekistler]

Look good! 👍

[jantache-microsoft]

On inspection (I haven't done any testing), I don't think the escape function fits all cases of escaping. E.g. in Linux bash, one could put $(injected command here) in the path string, and the injected would be evaluated by oad's call to the shell. This will also break if a double-quote is somehow in the path string.

Perhaps handling these cases is out of scope, but if oad is called remotely behind some attacker-controlled input, this could be bad.

Perhaps the shell-invoking command exec and execSync do actually make use of the shell features, in which case the escape function should be improved. But if there's no particular reason a shell needs to be invoked, I strongly recommend using execFile and execFileSync instead, since they pass arguments to the invoked program as-is rather than doing any form of shell evaluation.

[jianyexi]

On inspection (I haven't done any testing), I don't think the escape function fits all cases of escaping. E.g. in Linux bash, one could put $(injected command here) in the path string, and the injected would be evaluated by oad's call to the shell. This will also break if a double-quote is somehow in the path string.

Perhaps handling these cases is out of scope, but if oad is called remotely behind some attacker-controlled input, this could be bad.

Perhaps the shell-invoking command exec and execSync do actually make use of the shell features, in which case the escape function should be improved. But if there's no particular reason a shell needs to be invoked, I strongly recommend using execFile and execFileSync instead, since they pass arguments to the invoked program as-is rather than doing any form of shell evaluation.

I didn't use the execFileSync is because it doesn't work in Windows if I pass the 'node' to it.
And the escape only applies to an internal argument, so there is no security issue, so I think this would be a designated fix for your issue so that we have the minimal impact, if we can still consider using the execFile but need to have a further investigation.

[jantache-microsoft]

Correct me if I am wrong, but isn't the text in this code user-controlled?

    const cmd = `${this.dotNetPath()} ${this.openApiDiffDllPath()} -o ${oldSwagger} -n ${newSwagger}`

    log.debug(`Executing: "${cmd}"`)
    const { stdout } = await exec(cmd, { encoding: "utf8", maxBuffer: 1024 * 1024 * 64 })

And could be attacked by a valid file path like:

$ ls -d $PWD/swagger.json
'/c/Users/jantache/$(echo insert dangerous command here)/swagger.json'

[jianyexi]

Correct me if I am wrong, but isn't the text in this code user-controlled?

    const cmd = `${this.dotNetPath()} ${this.openApiDiffDllPath()} -o ${oldSwagger} -n ${newSwagger}`

    log.debug(`Executing: "${cmd}"`)
    const { stdout } = await exec(cmd, { encoding: "utf8", maxBuffer: 1024 * 1024 * 64 })

And could be attacked by a valid file path like:

$ ls -d $PWD/swagger.json
'/c/Users/jantache/$(echo insert dangerous command here)/swagger.json'

The tool will check if the file exists, if you provide an invalid file path, the tool will fail before executing this command, and your attack works only when your server do contain such dangerous swagger file path which can't be controlled by the attacker

openapi-diff/src/lib/validators/openApiDiff.ts

Line 293 in 51ce753

if (!fs.existsSync(oldSwagger)) {