breautek commented on PR #1778: URL: https://github.com/apache/cordova-android/pull/1778#issuecomment-2648431891
> The article is talking about the `shell` option and the `execa` package does have this flag. > > https://www.npmjs.com/package/execa/v/5.1.1#shell > > Bit curious, since the dependency suggests setting it is > > > unsafe, potentially allowing command injection. > > But since we are defining the bat file and not taking in user arg, it should be OK. I assume execa is using `spawn` behind-the-scenes, and it's generally recommended to not enable `shell` because it can be a security concern: This is from the NodeJS docs >If the shell option is enabled, do not pass unsanitized user input to this function. Any input containing shell metacharacters may be used to trigger arbitrary command execution. The security concern arises if we are passing in user supplied values, which we are not: ``` path.join(__dirname, 'getASPath.bat') ``` `path` is a NodeJS API, and `__dirname` is a module-level NodeJS variable, concating hard-coded string `getASPath.bat`. So I think using `shell` option here is safe. I do think we should comment why we are using `shell` though so that someone else doesn't for optimization or to "harden". -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@cordova.apache.org For additional commands, e-mail: issues-h...@cordova.apache.org
