search

actions / upload-artifact

MIT License
3.25k stars 732 forks source link

Exclude hidden files by default #598

Closed joshmgross closed 2 months ago

joshmgross commented 3 months ago

Currently, all files within the search path are uploaded into the artifact. This includes hidden files, which could contain sensitive values that should not be accessible outside of the workflow run. To enhance the default security of this action, this PR excludes hidden files from the path by default. Users who require hidden files have validated that there are no sensitive values in their artifacts can opt-in to uploading hidden files with the include-hidden-files input.

This is part of the following breaking change:

Exclude hidden files by default in Upload Artifact GitHub Actions From September 2nd, 2024, we will no longer include hidden files and folders as part of the default upload of the v3 and v4 upload-artifact actions. This reduces the risk that credentials are accidentally uploaded into artifacts. Customers who need to continue to upload these files can use a new option, ‘include-hidden-files’, to continue to do so. https://github.blog/changelog/2024-08-19-notice-of-upcoming-deprecations-and-breaking-changes-in-github-actions-runners/

bartekpacia commented 2 months ago

This just broke my workflow. I think this warrants a breaking change.

AlexSkrypnyk commented 2 months ago

This has just broke 1000s of workflows that rely on artifacts produced by tools like pcov/phpunit etc. and any other reporting tools.

This should have been released as a major version.

neoye911 commented 2 months ago

This has just broke 1000s of workflows that rely on artifacts produced by tools like pcov/phpunit etc. and any other reporting tools.

This should have been released as a major version.

Failed our release pipeline as well. Agree.

thomas-alias-zaiko commented 2 months ago

Hey guys, even though this is definitely a good change, introducing a breaking change on a minor version isn't great. Like the above comments, this also affected our company's workflows (including our production ones). Please consider breaking changes on major versions next time 🙂

casperdcl commented 2 months ago

It's also likely extremely hard to debug for users. "Breaking" was included in #602's title so it's unforgivable to sneak this in a minor version.

twm commented 2 months ago

This change likely breaks every workflow that uploads the .coverage* files generated by coverage.py. This will have a significant impact in the Python ecosystem.

Did GitHub assess the number of (public) artifact uploads that would be affected by this change? I understand that avoiding upload of .git/config files containing credentials was the goal. Is forbidding the upload of any pathname beginning with . the lowest-impact way to achieve that aim?

Some other possibilities:

  1. Don't upload .git directories by default.
  2. Don't put credentials in .git/config by default.

The first option seems attractive to me, regardless of security implication, since the .git directory is redundant with the content of the Git repository. I struggle to envision a scenario where I would deliberately upload the .git directory.

The second option minimizes harm.

I must express my sympathy with all those who advocated for less impactful options, but did not prevail. Sadly, the OSS ecosystem must deal with the consequences.

casperdcl commented 2 months ago

I'd still suggest reverting in a minor version and re-releasing in a major one regardless of implementation (#598, #599, https://github.com/actions/upload-artifact/pull/598#discussion_r1729047342, etc)

joshmgross commented 2 months ago

Please direct all feedback to the tracking issue https://github.com/actions/upload-artifact/issues/602