taiki-e / upload-rust-binary-action

GitHub Action for building and uploading Rust binary to GitHub Releases.
Apache License 2.0
227 stars 19 forks source link

Allow wildcard in include #36

Closed ShayBox closed 1 year ago

ShayBox commented 1 year ago

This allows for syntax such as include: target/${{ matrix.target }}/release/*example.*
Which covers my use-case being libspotify.so and spotify.dll in #35

ShayBox commented 1 year ago

Shell injection is kinda moot when you're the one controlling the workflow file in the first place, you can run any command you want, you can literally run shell code without having to inject it through an option

ShayBox commented 1 year ago

image
GitHub recognizes this risk, and clearly states not to use self-hosted runners on public repositories because that would allow forks to run code on your self-hosted runner (and thus your machine), but again you don't need to shell inject for that, the fork could just add a shell command to the workflow file, thus why they tell you NOT to use self-hosted runners on public repositories.

Using official GitHub runners there's no problem, all the forks can do is modify the outcome of their forks workflow, but it doesn't effect your personal machine or upstream repository, and you're expected to not merge malicious commits to the project (such as shell commands or shell injection).

EDIT: The other concern is pull request workflows leaking or otherwise performing malicious actions on the upstream repository, but you shouldn't use this action or any action that uses GITHUB_TOKEN on pull request workflows for this reason, sanitation doesn't completely protect against that, a pull request could for example execute a build.rs script to inject code and obtain said token or perform malicious actions.

EDIT2: If you're trying to build-test pull requests again you should NOT use action runners that require GITHUB_TOKEN, use an action that doesn't such as https://github.com/taiki-e/setup-cross-toolchain-action.
That should probably be made clear in the README

ShayBox commented 1 year ago

I'm not sure how you'd prefer to handle file grouping {file1,file2} as that's the proper version of file1,file2 but they conflict, I would recommend removing the comma delimitation from the script but that would be a breaking change, but this pull works as-is and that can be handled separately, as the functionality is still kinda there, just not the bash syntax way to do it.

EDIT: Ultimately the bash syntax would allow for shorter lists:
target/${{ matrix.target }}/release/deps/{libspotify.so,spotify.dll}
Instead of:
target/${{ matrix.target }}/release/deps/libspotify.so,target/${{ matrix.target }}/release/deps/spotify.dll, but again that requires a breaking change so I'll leave that up to you.

ShayBox commented 1 year ago

I've tested this as working, it allows for wildcard support in include and allows for non-existent files to not fail the workflow, I would recommend a squash merge because I made a lot of commits using the Web editor.

taiki-e commented 1 year ago

Thanks for the PR! Handling untrusted glob in bash is a pain and I was originally thinking of expanding the glob on the javascript side, but I'm open to accepting this if this approach works well across different platforms and different bash versions.

I'm not sure how you'd prefer to handle file grouping {file1,file2} as that's the proper version of file1,file2 but they conflict, I would recommend removing the comma delimitation from the script but that would be a breaking change, but this pull works as-is and that can be handled separately, as the functionality is still kinda there, just not the bash syntax way to do it.

I think it might work to wrap the entire input with {} and skip the comma delimitation, if the input contains , but not {.