Fix scan alert #151 by testing user-provided CMake arguments in setup.py

[mhucka]

Code scanning alert #151 reports that the subprocess.run() invocation includes values (from cmake_args) that are user-provided and thus insecure.

Code that passes user input directly to exec, eval, or some other library routine that executes a command, allows the user to execute malicious code.

       additional_cmake_args = os.environ.get("CMAKE_ARGS", "")
       if additional_cmake_args:
           cmake_args += additional_cmake_args.split()

The only way that user-provided values could be snuck in here is via the environment variable CMAKE_ARGS. The types of CMake arguments that make sense in this context are flags, and all of those begin with a dash. As a (admittedly limited) form of protection against this vulnerability, this PR adds a test of the values from CMAKE_ARGS to verify they all begin with a dash character.

This does restrict the form of the flags that can be passed in CMAKE_ARGS. Technically, CMake understands two forms: -DCMAKE_CXX_STANDARD=17 and -D CMAKE_CXX_STANDARD=17. The change in this PR means the flags always have to be of the first form, which seems to be a small inconvenience but not something that prevents any flags from being written.

Included in this PR are some new unit tests.

[gemini-code-assist]

Code Review

This pull request addresses a security vulnerability by validating user-provided CMake arguments from the CMAKE_ARGS environment variable. However, the current validation is still vulnerable to argument injection, as dangerous CMake flags like -P or -E can bypass the check. A more restrictive validation, such as only allowing definition flags (-D), is recommended to fully mitigate the CodeQL #151 alert. The inclusion of unit tests is a great addition, and further improvements to robustness and maintainability are suggested in the specific comments.

[pavoljuhas]

I think it is better to dismiss the original alert and close this PR.
The setup.py is used only at installation and wheel-building time and is not accessible to user in a normal qsim operation.

Trying to sanitize possible CMAKE_ARGS values is probably a hopeless task. The attempts to do so make it harder to follow the code and are likely to introduce new bugs or interfere with legitimate cmake arguments.

[pavoljuhas]

As a general comment, term dummy is problematic and should be avoided in naming things, go/respectful-words.

[mhucka]

Oh yes, quite right. Thank you for pointing that out.

[mhucka]

I think it is better to dismiss the original alert and close this PR. The setup.py is used only at installation and wheel-building time and is not accessible to user in a normal qsim operation.

Trying to sanitize possible CMAKE_ARGS values is probably a hopeless task. The attempts to do so make it harder to follow the code and are likely to introduce new bugs or interfere with legitimate cmake arguments.

Yeah, it's probably true, or at least, the present approach is looking like a poor ROI.