[bug]Hidden files excluded

[corneliusroemer]

The PR was closed as off-topic, pointing to the issue which itself was then closed as too heated. Come on Github, allow people to let off steam in comments. If you keep closing things they will post elsewhere where you can't stop them.

Original PR:

The issue it points to #602:

[fulldecent]

I believe that SemVer should be a required course in university for CS students. And it is more important than any other course they might take.

[akolodkin-kp]

Please rollback this change. It broke everything.

[andreasg123]

I'm not happy that I had to waste three hours on this but the reactions are over the top. The fix is minimal:

include-hidden-files: true

[andreasg123]

Not that I actually read this blog post but two weeks notice seems to be a little short. https://github.blog/changelog/2024-08-19-notice-of-upcoming-deprecations-and-breaking-changes-in-github-actions-runners/

[jakub7722]

Not that I actually read this blog post but two weeks notice seems to be a little short. https://github.blog/changelog/2024-08-19-notice-of-upcoming-deprecations-and-breaking-changes-in-github-actions-runners/

I have a subscription for the GH blog, but somehow this one I did not see. I had whole folder named .output and was left scratching my head why I'm suddenly getting No files were found with the provided path. No artifacts will be uploaded.

[corneliusroemer]

It's quite something that GitHub didn't at all mention the reason for making the change in the blog post. This is the reason: the "ArtiPACKED" vulnerability https://unit42.paloaltonetworks.com/github-repo-artifacts-leak-tokens

[jakub7722]

I'm not happy that I had to waste three hours on this but the reactions are over the top. The fix is minimal:

include-hidden-files: true

Just multiply this by 100 repositories.. Plus if you simply go and apply the parameter it very much defeats the purpose - this change was meant to be introduced so that credentials/sensitive info don't get accidentally exposed. What good does it do now when people are in a rush to just fix their pipelines?

[corneliusroemer]

@jakub-lesniak-ikea yes you are totally right here. It's a pure blame shifting strategy. Now one can blame the customer: you enabled that inclusion of hidden files so it's your fault not Github's (without telling the user that hidden files might include GitHub tokens?)

[phess101]

This is a terrible change, with an incredibly short release ramp. This version should have been a 5.0 version

[AlexBksiad]

Pushing this change without a major version is an absolute indictment of the practices that are being followed.

In my humble opinion, applying SemVer practices is the bare minimum for utilities and tools that are used as widely as this. Not to mention that there's nothing that requires me to update to the 'latest version' like other tools used in development. Nope, updates to this tooling are automatically applied based on the standard/recommended approach for implementation.

Not only is this guaranteed to break an incredible number of pipelines across GitHub (it sure broke mine), finding out why requires people to come to the issues register for the action. Considering this is an officially maintained action, the last thing that came to mind is a breaking change being pushed to all pipelines globally without so much as even a special message in the log output to make people aware of this in the event things are breaking.

I was lucky, in that I had include-hidden-files: true set, so was immediately made aware of the issue. (My application still broke though because it was left in a partially deployed state, and rollback-deploying the previous version was not possible because surprise - the pipeline was still broken for that). In addition, what happens to other apps that had a blend of hidden and non-hidden files? Applications are likely being deployed without key files included, inevitably causing unknown problems and undefined behaviors.

The justification for this was "security reasons". Understood - security is important, but let's not forget that one of the key tenets of information security is availability - something that has no doubt been impacted for many applications as a result of this.

In addition, who knows what other security implications apps deploying without all the right files may have? Just for one completely random example, let's consider a missing .htaccess file. There goes whatever access restrictions were implemented via that mechanism - potentially exposing sensitive areas of a site or app to the public web.

The way this has been approached is downright dangerous. People rely on these tools to work in a predictable manner, and when they don't, the only result is chaos.

[alonapester]

2 days and it's still a problem, This should of been fixed ASAP...

[cnpatric]

Can't believe that I have to find out this way after trying for hours to figure out what happened since our deployment process was suddenly broken.

How is a breaking change that completely changes behaviour a minor update? Two weeks deprecation warning is insane for a library like this

[sscowden]

Whoever decided to deploy this breaking change in a minor release before a holiday weekend should be fired, with cause.

[sscowden]

I'm not happy that I had to waste three hours on this but the reactions are over the top. The fix is minimal:

include-hidden-files: true

Go tell that to the people who had an outage and had no clue why

[jpboily-axceta]

I agree! It took me half a day to figure it out. In our particular use case, it was not an issue because we are still in the development phase, but if an important bug fix delivery had been blocked because of this, it could have been costly.

[nedbat]

Can we get an update about the plan for moving forward? Dependabot is pushing me to install 4.4.0, but I don't want to add include-hidden-files: true since it defeats the purpose of the security fix. I explicitly request to upload specific hidden files. It would be great to hear that those will be honored without turning off the safety.

[WorldSEnder]

This caught us off-guard too. There's a behaviour problem with this option too. It makes sense that hidden files are not considered when recursively globbing directories. However, an exact match of file paths is also not considered. Before, we had (roughly) the following options set on the action:

      - name: Upload Artifact
        uses: actions/upload-artifact@v4
        with:
          name: benchmark-core
          path: |
            .PR_NUMBER
            artifacts/

.PR_NUMBER is now a file considered "hidden" by the new default and not uploaded. This makes no sense to me from a "security" standpoint. We explicitly mention exactly the filename, which should be clear enough that we do indeed want to consider it part of the artifact without having to opt into the "unsafe" include-hidden-files: true for the whole recursive globbing of the artifacts/ directory.

[joshmgross]

Hidden files are intentionally excluded, that is not a bug.

As for the behavior of explicitly referenced hidden files:

This

- name: Upload Artifact
  uses: actions/upload-artifact@v4
  with:
    name: my-artifact
    path: |
      .my-hidden-file

is the same as

- name: Upload Artifact
  uses: actions/upload-artifact@v4
  with:
    name: my-artifact
    include-hidden-files: false
    path: |
      .my-hidden-file

As that's the default value of include-hidden-files: https://github.com/actions/upload-artifact/blob/b18b1d32f3f31abcdc29dee3f2484801fe7822f4/action.yml#L43-L47

So that is working as intended, but I'm open to changing the behavior if it's confusing for a majority of users. Feel free to file an issue specific to that request and we can gather more feedback.

[phess101]

@joshmgross can we have a better idea of the plan moving forward with this library to avoid potential future issues? The lack of complete SemVer commitment is leaving our organization nervous to continue to use this action.

[nedbat]

So that is working as intended, but I'm open to changing the behavior if it's confusing for a majority of users. Feel free to file an issue specific to that request and we can gather more feedback.

Across the two or three issues about the change in behavior, there are already a number of people asking that explicitly named hidden files be uploaded without having to allow all hidden files. What more feedback do you need?

[nedbat]

If a new issue will help, I wrote #614 to discuss it.

[joshmgross]

@joshmgross can we have a better idea of the plan moving forward with this library to avoid potential future issues? The lack of complete SemVer commitment is leaving our organization nervous to continue to use this action.

See https://github.com/actions/upload-artifact/issues/602#issuecomment-2326695405

Breaking semantic versioning conventions is a rare exception. Breaking changes will always be announced in release notes, and the vast majority of breaking changes will be accompanied with the appropriate major version.

If you do not want to take that risk, I'd recommend pinning your workflows to an exact tag or SHA. You can use Dependabot to keep your actions up to date and evaluate the release notes before opting into any changes - https://docs.github.com/code-security/dependabot/working-with-dependabot/automating-dependabot-with-github-actions.

[jakub7722]

After more than a week I am still amazed by the lack of understanding by the maintainers of this repository that you have failed the community. There's not even a single person that would support what you have done.

For the continued lack of commitment to sem ver - you are endangering everyone. You had one responsibility on your part - to upload the files that you were asked to upload. It is not your call to decide what is dangerous, what you are not going to upload - this is simply other people responsibility.

Someone gave a great example with .htaccess - by not uploading this file you potentially endangered unknown number of systems out there. This call wasn't yours to make.

If you do not want to take that risk, I'd recommend pinning your workflows to an exact tag or SHA

How can we trust your tags if you have broken the commitment already? What's the guarantee that next time you don't decide to introduce breaking changes in patch releases? Semver is well documented and understood. Breaking semver means that you are simply unpredictable - there's no documented alternative.

Everyone can understand that people make mistakes. That is expected and does happen. However what happens here is:

• someone made a bad decision
• is still insisting that it was good even though the community was unanimous on the feedback (that is rare!)
• still does not commit to adhere to semver which is a cornerstone of software engineering
• does not offer any alternative or predictability in how the releases are going to be done and what rules will be followed
• breaks trust of the community

[corneliusroemer]

@jakub7722 I agree almost entirely with your comments, just two slight qualifications:

• It's possible that some people think the change was worth making - they just haven't commented/complained here. Though I tend to agree that there probably aren't that many given the facts mentioned in the discussion (best example being the .htaccess.
• I would slightly tone down those two wordings:
  • There's not even a single person that would support what you have done. -> Few if any have come out in support of the breaking change.
  • is still insisting that it was good even though the community was unanimous on the feedback (that is rare!) -> is still insisting that it was good even though the community was unanimous (to the best of my knowledge) on the feedback (that is rare!)

I might be mistaken here, but I thought that (at least until recently) dependabot didn't work with Github actions pinned to anything but major version. If that's been changed or my memory is wrong that'd be great news!

[corneliusroemer]

As a followup, Github docs say the following (emphasis mine):

Pin actions to a tag only if you trust the creator

Although pinning to a commit SHA is the most secure option, specifying a tag is more convenient and is widely used. If you’d like to specify a tag, then be sure that you trust the action's creators. The ‘Verified creator’ badge on GitHub Marketplace is a useful signal, as it indicates that the action was written by a team whose identity has been verified by GitHub. Note that there is risk to this approach even if you trust the author, because a tag can be moved or deleted if a bad actor gains access to the repository storing the action. from https://docs.github.com/en/actions/security-for-github-actions/security-guides/security-hardening-for-github-actions#using-third-party-actions

If you want everyone to pin to commit SHAs and discourage using tags, that's fine, but it's not something that Github org as a whole seems to recommend, so you might want to check internally to align your messaging.

Also note that dependabot support for pinned SHAs is not great. In this case, it bumped the SHA but didn't mention it in the PR summary. There's no indication of what changed (changelog) for the sha bump in the PR description. Before you recommend users do that and suggesting that dependabot supports that use case, please double check with the dependabot team.

I made a test repo to check your claims of dependabot support: https://github.com/corneliusroemer/dependabot-ghactions-test/pull/2

Further evidence of dependabot struggling:

I downgraded and pinned two more actions, to test whether dependabot would make PRs. It didn't.

Downgrade commit:

Dependabot run on that commit that didn't result in any PRs (it should have, if GHA were fully supported at sub-major-tag-level and sha-level, which is necessary for your suggestion to make sense): https://github.com/corneliusroemer/dependabot-ghactions-test/actions/runs/10830167324

So to me it seems that the following statement made here by @joshmgross is not correct in its generality and should be struck/qualified:

If you do not want to take that risk, I'd recommend pinning your workflows to an exact tag or SHA. You can use Dependabot to keep your actions up to date and evaluate the release notes before opting into any changes - https://docs.github.com/code-security/dependabot/working-with-dependabot/automating-dependabot-with-github-actions.

In particular, when pinning at sub-major-tag, one doesn't seem to get tag updates, unless one is lucky (?). Also, release notes ignore the sha updates.

[alan-czajkowski]

this is how you lose trust in your customers, and lose users to other products