Skip to content

Comments

Fix escaping of arguments#47

Closed
mohd-akram wants to merge 2 commits intoallure-framework:mainfrom
mohd-akram:fix-args
Closed

Fix escaping of arguments#47
mohd-akram wants to merge 2 commits intoallure-framework:mainfrom
mohd-akram:fix-args

Conversation

@mohd-akram
Copy link

@mohd-akram mohd-akram commented Feb 23, 2025

Commit a407329 enabled the shell option to fix running on Windows, but that's unsafe and breaks handling of spaces and special characters. Arguments provided to Windows batch files in particular need special escaping that's not provided by Node.js; use batspawn for that purpose.

Fixes #30
Fixes #45

@CLAassistant
Copy link

CLAassistant commented Feb 23, 2025

CLA assistant check
All committers have signed the CLA.

@mohd-akram
Copy link
Author

@baev Thoughts on this?

@Hemavathy12
Copy link

Commit a407329 enabled the shell option to fix running on Windows, but that's unsafe and breaks handling of spaces and special characters. Arguments provided to Windows batch files in particular need special escaping that's not provided by Node.js; use batspawn for that purpose.

Fixes #30 Fixes #45

Hi,

Any updates on this? Will it be included in the upcoming release?

@baev
Copy link
Member

baev commented Dec 9, 2025

this PR introduces a new batspawn dependency that can't be used in our project due to security risks (package is created by @mohd-akram just for this PR and is not used anywhere else, see https://www.npmjs.com/package/batspawn)

@baev baev closed this Dec 9, 2025
@mohd-akram
Copy link
Author

I did not create batspawn for this PR. I do not even use allure personally, it's used internally at my workplace. The ease of accidentally introducing a security vulnerability, as done in a407329 in this project, was my motivation for reporting nodejs/node#57143, which resulted in a deprecation warning for unsafe usage starting from Node.js 24. Since then, there have been many fixes to different packages to address this issue. This PR is yet another, and contains a test for fixing the security vulnerability. batspawn was created because the existing libraries, such as cross-spawn, do not adequately protect against the BatBadBut vulnerability (eg. moxystudio/node-cross-spawn#171). batspawn implements the escaping mechanism mentioned in the linked article, which is also used by Rust and Zig. More details here.

@mohd-akram mohd-akram deleted the fix-args branch December 9, 2025 17:25
@baev
Copy link
Member

baev commented Dec 10, 2025

@mohd-akram thanks for the extra details and sorry if my previous message came across the wrong way. That wasn’t my intention, and I appreciate the work you put into the PR.

To clarify, I’m not against fixing the issue, but I’m hesitant to add a dependency that’s still relatively new and not widely used. If you’re open to it, I’d be happy to review a version of the PR that includes the necessary code directly in this repository instead.

Thanks again for your contribution and understanding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

npm run allure command does not work for current working directory path containing spaces Command does not work with path containing white-space

4 participants