upload-artifact icon indicating copy to clipboard operation
upload-artifact copied to clipboard

Upcoming breaking change: Hidden Files will be excluded by default

Open joshmgross opened this issue 1 year ago • 60 comments
trafficstars

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/

  • https://github.com/actions/upload-artifact/pull/598
  • https://github.com/actions/upload-artifact/pull/604

joshmgross avatar Aug 19 '24 20:08 joshmgross

Why introducing breaking changes to v3 and v4 instead of making a new v5 version?

dolfinus avatar Sep 02 '24 14:09 dolfinus

      - name: Upload Docker Compose Files Artifact
        id: upload_docker_compose_files
        uses: actions/[email protected]
        include-hidden-files: true
        with:
          name: docker-compose-files
          path: |
            .env
            docker-compose.yaml

I've edited my workflow like above and getting the error:

The workflow is not valid. In .github/workflows/kondukto-package-build-manual.yml (Line: 318, Col: 11): Error from called workflow kondukto-io/kondukto-build/.github/workflows/docker_compose_package.yml@c343cea5db32d5d329cfe25ea5f7f84a88ecb64a (Line: 38, Col: 9): Unexpected value 'include-hidden-files'

I tried both versions v4 and v4.4.0

umrkt avatar Sep 02 '24 15:09 umrkt

I've edited my workflow like above and getting the error:

You should place include-hidden-files: true under with: block

dolfinus avatar Sep 02 '24 15:09 dolfinus

Thanks a lot. My bad.

umrkt avatar Sep 02 '24 15:09 umrkt

image image Funny thing [in](include-hidden-files: true) doesn't work on v3, in one screenshoot I point directly to file and also doing a cat to see if it exists and exists

catYalere avatar Sep 02 '24 16:09 catYalere

This change resulted in deployment failures for us because we no longer included the full set of files in our deployment artifact.

buckett avatar Sep 02 '24 16:09 buckett

This also resulted in a lot of CI failures for us... Would be nice if there was at least a warning before rolling breaking changes 🤷‍♂️

Here's how we were calling this action:

    - uses: actions/upload-artifact@v4
      with:
        name: artifacts-${{ strategy.job-index }}
        path: |-
          coverage/.resultset.json
          tests/junit.xml

Drowze avatar Sep 02 '24 16:09 Drowze

Funny thing [in](include-hidden-files: true) doesn't work on v3, in one screenshoot I point directly to file and also doing a cat to see if it exists and exists

👋 Thanks for the report, we're rolling back the v3 and v3-node20 updates while we work on fixing that issue.

joshmgross avatar Sep 02 '24 16:09 joshmgross

Same here, I suddenly got an issue in my CI while deploying my app that never occurred before: CleanShot 2024-09-02 at 18 30 54@2x

gurvancampion avatar Sep 02 '24 16:09 gurvancampion

We also got burned by this. I see the logic in excluding hidden files recursively nested inside directories. But I struggle to see the value in ignoring an explicit path: .dotfile parameter.

tpope avatar Sep 02 '24 16:09 tpope

Funny thing [in](include-hidden-files: true) doesn't work on v3, in one screenshoot I point directly to file and also doing a cat to see if it exists and exists

👋 Thanks for the report, we're rolling back the v3 and v3-node20 updates while we work on fixing that issue.

This is fixed in #608 & #609

https://github.com/actions/upload-artifact/releases/tag/v3.2.1 https://github.com/actions/upload-artifact/releases/tag/v3.2.1-node20

We're updating the major version tags v3 & v3-node20 to include those fixes.

joshmgross avatar Sep 02 '24 16:09 joshmgross

TY v3 with flag working image

catYalere avatar Sep 02 '24 18:09 catYalere

Breaking changes should bump major version! What kind of semantic versioning is this?

And if old behavior poses security problems, issue a deprecation warning via EMAIL to organization owners that have repos using this very important action.

Breaking developers workflows on such a short notice (if writing a blog post can be considered an actual "notice") is NOT OK

antoniocoratelli avatar Sep 02 '24 19:09 antoniocoratelli

This "minor" non-backward compatible API change also broke my organization's workflows by surprise.

In case it helps someone to troubleshoot, we were using this action to upload a directory created with pip install . --target <directory>, which was later downloaded by other runners for code and behaviour testing.

Apparently, pip installs dependencies in a hidden subdirectory. It stopped being uploaded as part of the artifact, which caused No module named <name> exceptions downstream.

As stated above, it was solved by including include-hidden-files: true as input.

pdmtt avatar Sep 02 '24 20:09 pdmtt

This breaking change also broke the CI workflows for typer and fastapi, because the test suite was writing and then reading .coverage files ... We've also gone ahead and added include-hidden-files: true specifically to all workflows. Still, I would have expected to see this kind of change in v5, not in a minor release.

svlandeg avatar Sep 02 '24 20:09 svlandeg

We also got burned by this. I see the logic in excluding hidden files recursively nested inside directories. But I struggle to see the value in ignoring an explicit path: .dotfile parameter.

Could we at least get explicitly named files back for v3 and v4? There doesn't seem to be a realistic scenario where uploading of explicitly enumerated files poses a security risk. As they have been added explicitly, they should fit the criteria of being vetted.

I can understand if you want a more coherent API. But please just release v5 for that.

mrgrain avatar Sep 02 '24 22:09 mrgrain

I'll add my voice to this comment:

We also got burned by this. I see the logic in excluding hidden files recursively nested inside directories. But I struggle to see the value in ignoring an explicit path: .dotfile parameter.

It's less secure to ignore my explicit instruction and force me to add another option that could possibly include hidden files I didn't intend. Please change this behavior.

To make an analogy: ls ignores hidden files. ls -a shows them. ls .gitignore shows me the hidden .gitignore file even without the -a flag.

nedbat avatar Sep 02 '24 23:09 nedbat

It got worse: now actionlint does needs to be updated as well https://github.com/rhysd/actionlint/issues/447

AlexSkrypnyk avatar Sep 03 '24 00:09 AlexSkrypnyk

Can we get something in the README about this project not following semantic versioning guidelines?

Releasing breaking changes in minor version bumps is nothing short of asinine.

andrewdcato avatar Sep 03 '24 01:09 andrewdcato

Wasted a lot of time trying to diagnose a broken system all of a sudden after deployment. Even tried rolling back the changes but no use. Luckily this wasn't a critical system. After spending a lot of time, Figured out this was the actual issue. As most of the people mentioned, Broken changes should not be introduced in minor versions.

I understand open source packages come with their own risk and maintenance challenges. We appreciate everyone who contribute to this and other open source packages. But official package from github actions which is used by thousands of repositories should follow basic semantic versioning guidelines. Especially when the package versioning itself is in the format of semantic versioning. Could we release a quick version which includes a link to this issue as a warning message so devs can quickly identify the exact issue and update their workflows?

I hope others who are experiencing this issue can quickly find this thread so they can resolve their issues quickly.

srinathreddydudi avatar Sep 03 '24 03:09 srinathreddydudi

I understand the context of include-hidden-files: true: https://unit42.paloaltonetworks.com/github-repo-artifacts-leak-tokens/

The directory contains the hidden .git folder that stores the persisted GITHUB_TOKEN, leading the publicly accessible artifacts to contain the GITHUB_TOKEN.

Using the path: . option causes the token stored in the .git folder to leak. In this case, a breaking change might be reasonable to prevent this risk.

      - uses: actions/upload-artifact@v4
        with:
          name: artifact
          path: . 
          # or
          # path: "*" 

However, it also affects explicit options like path: .coverage, which has caused some confusion.

MtkN1 avatar Sep 03 '24 04:09 MtkN1

If you have to make a breaking change this way at least add a logged warning to the workflow so people who get errors right now will have a potential error source right in their log

Spent too much time looking through my long workflow before stumbling on this thread... Semantic versioning is there for a reason and people who break it...

jacklul avatar Sep 03 '24 07:09 jacklul

Using the path: . option causes the token stored in the .git folder to leak. In this case, a breaking change might be reasonable to prevent this risk.

Instead of a breaking change to silently ignore all dotfiles, it would be just easier to ignore .git explicitly or via an added optional parameter that has a default of .git.

rantoniuk avatar Sep 03 '24 08:09 rantoniuk

I don't understand why this wasn't possible to be added with a default of true and then switch to false in a major version bump. The amount of effort needed for GitHub users to resolve issues caused by this rash rollout does not seem in relation to the security gains as it erodes trust in GitHub's engineering planning. Do better GitHub.

jezdez avatar Sep 03 '24 08:09 jezdez

Also here because CI workflows suddenly stopped working. Thankfully we didn't deploy anything broken 😅

I'd like to echo the other suggestions that an explicit path to a hidden file/directory should be allowed.

Also, it would be cool if this repo had a pinned issue where breaking changes are announced. I'm not subscribed to issues in this repo or the GitHub blog, but I would definitely subscribe to a pinned issue that contains only breaking change announcements.

grncdr avatar Sep 03 '24 08:09 grncdr

Putting this here, so people googling why their firebase deployment pipeline on github actions suddenly fails will hopefully find it.

Error: Deploy target admin not configured for project xxx. Configure with:
  firebase target:apply hosting yyy <resources...>

Even if you put your .firebaserc explicitly in the path option of the actions/upload-artifact@v4 action, the .firebaserc file will not be uploaded. You need to explicitly add include-hidden-files: true, like this:

      - uses: actions/upload-artifact@v4
        with:
          name: your-artifact
          include-hidden-files: true
          path: |
            dist/
            .firebaserc

patricsteiner avatar Sep 03 '24 08:09 patricsteiner

I also would like to raise that how this was rolled out is far from optimal.... Lost hours just because of this. While I agree this might be a good idea in general there should have been some notice at least before switching things in a non major version bump

normanmaurer avatar Sep 03 '24 09:09 normanmaurer

I also lost hours due to this issue. Azure functions necessitate the presence of the hidden folder .azurefunctions, which inexplicably disappeared from the artifact. This change should have been implemented as a major version update, or as suggested with a warning message.

fleed avatar Sep 03 '24 09:09 fleed

Adding up here for people aggregating their simplecov test coverage using the upload artifact function getting No files were found with the provided path: coverage/.resultset.json. No artifacts will be uploaded.

Even with the path specifically mentioning the actual file, it will fail after this update. include-hidden-files needs to be specified.

        uses: actions/upload-artifact@v4
        with:
          name: coverage-${{ github.sha }}-${{ matrix.check_name }}
          path: coverage/.resultset.json
          retention-days: 1
          include-hidden-files: true

bryan-brilivan avatar Sep 03 '24 09:09 bryan-brilivan

Is it possible that the team will manage to roll it back and re-release it as v5, or is that not even an option when the die has been cast? I feel like keeping from v4.4 in favor of v4.3.6 at least until this issue gets resolved, but am not sure when everything seems so full of confusion 😢

sato11 avatar Sep 03 '24 09:09 sato11