Skip to content

Conversation

@sylveon
Copy link
Contributor

@sylveon sylveon commented Aug 23, 2025

When using the shell, we need to quote our arguments! Also we only need the shell on Windows, so avoid using it on *nixes.

@pierotofy
Copy link
Member

pierotofy commented Aug 23, 2025

Handling the spaces in the ODM path looks 👍 thanks.

What's the advantage for not using shell on Unix?

@sylveon
Copy link
Contributor Author

sylveon commented Aug 23, 2025

It's just not needed on Unix, and wasn't used before my previous PR. By not using it when not needed, it makes it more efficient and avoids shell injections.

On Windows, newer versions of node require the use of shell: true when launching batch files (due to a fix for CVE-2024-27980). The shell was always used to launch batch files, they have just made it explicit to the user instead of implicit.

@pierotofy
Copy link
Member

pierotofy commented Aug 23, 2025

Ok, so mostly to avoid shell injections, but the config.odm_path parameter should be trusted.

let childProcess = spawn(pythonExe, [path.join(__dirname, "..", "helpers", "odmOptionsToJson.py"),
                    "--project-path", config.odm_path, "bogusname"], { env, stdio: 'inherit', shell: true });

Let's keep abf2e4f and revert c244f07 🙏

@sylveon
Copy link
Contributor Author

sylveon commented Aug 23, 2025

We still need the quotation marks. When using shell: true, the args array is combined with spaces and no attempt at escape is made, this results in failures if ODM is installed in a path with a space, such as Program Files (which is the case when it's packaged with MSIX).

I can update the commit to always quote the project/script path.

@pierotofy
Copy link
Member

Good point; I've made the changes, thanks for the PR! 🙏

@pierotofy pierotofy merged commit 3b72047 into OpenDroneMap:master Aug 23, 2025
1 check failed
@sylveon
Copy link
Contributor Author

sylveon commented Aug 23, 2025

The script path has to be escaped too

@pierotofy
Copy link
Member

3b29fbb

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.

2 participants