You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@cordova.apache.org by "kant2002 (via GitHub)" <gi...@apache.org> on 2025/02/10 06:35:22 UTC
[PR] Account for Node security patch [cordova-android]
kant2002 opened a new pull request, #1778:
URL: https://github.com/apache/cordova-android/pull/1778
As of https://nodejs.org/en/blog/vulnerability/april-2024-security-releases-2#command-injection-via-args-parameter-of-child_processspawn-without-shell-option-enabled-on-windows-cve-2024-27980---high Cordova produce unrecognized error on Windows.
Fixes: https://github.com/apache/cordova-cli/issues/456
--
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-unsubscribe@cordova.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
Re: [PR] Account for Node security patch [cordova-android]
Posted by "breautek (via GitHub)" <gi...@apache.org>.
breautek commented on code in PR #1778:
URL: https://github.com/apache/cordova-android/pull/1778#discussion_r1949362910
##########
lib/check_reqs.js:
##########
@@ -110,7 +110,7 @@ module.exports.get_gradle_wrapper = function () {
let program_dir;
// OK, This hack only works on Windows, not on Mac OS or Linux. We will be deleting this eventually!
if (module.exports.isWindows()) {
- const result = execa.sync(path.join(__dirname, 'getASPath.bat'));
+ const result = execa.sync(path.join(__dirname, 'getASPath.bat'), { spawn: true });
Review Comment:
```suggestion
// "spawn" option enabled for CVE-2024-27980 (Windows) Mitigation
// See https://nodejs.org/en/blog/vulnerability/april-2024-security-releases-2 for more details
const result = execa.sync(path.join(__dirname, 'getASPath.bat'), { spawn: true });
```
##########
lib/check_reqs.js:
##########
@@ -110,7 +110,7 @@ module.exports.get_gradle_wrapper = function () {
let program_dir;
// OK, This hack only works on Windows, not on Mac OS or Linux. We will be deleting this eventually!
if (module.exports.isWindows()) {
- const result = execa.sync(path.join(__dirname, 'getASPath.bat'));
+ const result = execa.sync(path.join(__dirname, 'getASPath.bat'), { spawn: true });
Review Comment:
Also, I can't find any `spawn` option documented in execa, is this suppose to be `shell` ?
--
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-unsubscribe@cordova.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
Re: [PR] Account for Node security patch [cordova-android]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1778:
URL: https://github.com/apache/cordova-android/pull/1778#issuecomment-2647075986
## [Codecov](https://app.codecov.io/gh/apache/cordova-android/pull/1778?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `0%` with `1 line` in your changes missing coverage. Please review.
> Project coverage is 72.45%. Comparing base [(`eb0f002`)](https://app.codecov.io/gh/apache/cordova-android/commit/eb0f002112538895c2c8fa2f5042eaea4b8cb26b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`f98c708`)](https://app.codecov.io/gh/apache/cordova-android/commit/f98c7085d61778216b146a79df7acdcb200d7817?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
| [Files with missing lines](https://app.codecov.io/gh/apache/cordova-android/pull/1778?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [lib/check\_reqs.js](https://app.codecov.io/gh/apache/cordova-android/pull/1778?src=pr&el=tree&filepath=lib%2Fcheck_reqs.js&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bGliL2NoZWNrX3JlcXMuanM=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/cordova-android/pull/1778?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #1778 +/- ##
==========================================
- Coverage 72.50% 72.45% -0.06%
==========================================
Files 23 23
Lines 1837 1837
==========================================
- Hits 1332 1331 -1
- Misses 505 506 +1
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/cordova-android/pull/1778?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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-unsubscribe@cordova.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
Re: [PR] Account for Node security patch [cordova-android]
Posted by "kant2002 (via GitHub)" <gi...@apache.org>.
kant2002 commented on code in PR #1778:
URL: https://github.com/apache/cordova-android/pull/1778#discussion_r1950373477
##########
lib/check_reqs.js:
##########
@@ -110,7 +110,7 @@ module.exports.get_gradle_wrapper = function () {
let program_dir;
// OK, This hack only works on Windows, not on Mac OS or Linux. We will be deleting this eventually!
if (module.exports.isWindows()) {
- const result = execa.sync(path.join(__dirname, 'getASPath.bat'));
+ const result = execa.sync(path.join(__dirname, 'getASPath.bat'), { spawn: true });
Review Comment:
I indeed manage to made mistake in the one-liner. Thank you. It's `shell`
--
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-unsubscribe@cordova.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
Re: [PR] feat: Account for Node security patch [cordova-android]
Posted by "erisu (via GitHub)" <gi...@apache.org>.
erisu merged PR #1778:
URL: https://github.com/apache/cordova-android/pull/1778
--
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-unsubscribe@cordova.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
Re: [PR] Account for Node security patch [cordova-android]
Posted by "breautek (via GitHub)" <gi...@apache.org>.
breautek commented on code in PR #1778:
URL: https://github.com/apache/cordova-android/pull/1778#discussion_r1951014326
##########
lib/check_reqs.js:
##########
@@ -110,7 +110,9 @@ module.exports.get_gradle_wrapper = function () {
let program_dir;
// OK, This hack only works on Windows, not on Mac OS or Linux. We will be deleting this eventually!
if (module.exports.isWindows()) {
- const result = execa.sync(path.join(__dirname, 'getASPath.bat'));
+ // "spawn" option enabled for CVE-2024-27980 (Windows) Mitigation
Review Comment:
```suggestion
// "shell" option enabled for CVE-2024-27980 (Windows) Mitigation
```
I wrote in "spawn" because that was in the original PR but since we determine the "shell" option is correct, that should be reflected in the comment too.
--
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-unsubscribe@cordova.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
Re: [PR] Account for Node security patch [cordova-android]
Posted by "erisu (via GitHub)" <gi...@apache.org>.
erisu commented on PR #1778:
URL: https://github.com/apache/cordova-android/pull/1778#issuecomment-2647147013
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.
--
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-unsubscribe@cordova.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org
Re: [PR] Account for Node security patch [cordova-android]
Posted by "breautek (via GitHub)" <gi...@apache.org>.
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-unsubscribe@cordova.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org