Skip to content

Conversation

@radarhere
Copy link
Member

@radarhere radarhere commented Dec 5, 2025

#9318 upgraded zizmor to include https://docs.zizmor.sh/audits/#obfuscation

The CMD shell has no formal grammar, making it impossible to accurately analyze for security issues.

This PR allows for that by

  1. Changing test-windows.yml to avoid the cmd shell.
  2. Moving the cmd shell instructions from wheels.yml into a separate file, where they will not bother zizmor. It is not an ideal solution, but I suspect an ideal solution would involve changing winbuild to no longer generate cmd files, and that seems like a step too far. This will at least allow zizmor's rule to be in place, so that we can enforce it by default.

@radarhere radarhere changed the title Do not use cmd shell in Windows GitHub Actions Do not use cmd shell in GitHub Actions Dec 5, 2025
Comment on lines -189 to -206
setlocal EnableDelayedExpansion
for %%f in (winbuild\build\license\*) do (
set x=%%~nf
rem Skip FriBiDi license, it is not included in the wheel.
set fribidi=!x:~0,7!
if NOT !fribidi!==fribidi (
rem Skip imagequant license, it is not included in the wheel.
set libimagequant=!x:~0,13!
if NOT !libimagequant!==libimagequant (
echo. >> LICENSE
echo ===== %%~nf ===== >> LICENSE
echo. >> LICENSE
type %%f >> LICENSE
)
)
)
call winbuild\\build\\build_env.cmd
%pythonLocation%\python.exe -m cibuildwheel . --output-dir wheelhouse
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can keep this chunk, it doesn't declare shell: cmd so will default to pwsh.

https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#defaultsrunshell

And zizmor doesn't complain about it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the case. The shell: cmd on line 220 is related to this block.

I've separated this change out from the other changes, and you can see that without it, zizmor does complain - https://results.pre-commit.ci/run/github/5171600/1765401638.xj0JlAmBT_6lRfMDGmxoqQ

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants