Allow local actions outside the workspace

[jenseng]

Fixes #2107

Also simplify actionName logic and ensure it returns sensible values for actions outside the workspace (it's only used for logging and docker image name)

[codecov]

Codecov Report

Attention: Patch coverage is 62.50000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 76.36%. Comparing base (5a80a04) to head (94fbf74).
Report is 86 commits behind head on master.

Files	Patch %	Lines
pkg/runner/action.go	62.50%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #2108       +/-   ##
===========================================
+ Coverage   61.56%   76.36%   +14.79%     
===========================================
  Files          53       61        +8     
  Lines        9002     7810     -1192     
===========================================
+ Hits         5542     5964      +422     
+ Misses       3020     1292     -1728     
- Partials      440      554      +114     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ChristopherHX]

In my opinion is Allow local actions outside the workspace more like an exploit (regarding security) for action/runner than an actual feature.

It doesn't look to be far away from Issue 2: Arbitrary file download in artifact server (GHSL-2023-004) of GHSA-pc99-qmg4-rcff.

[jenseng]

In my opinion is Allow local actions outside the workspace more like an exploit (regarding security) for action/runner than an actual feature.

For this change we're talking about reading an existing action.yml file from another directory, not reading or writing arbitrary files. So while there are superficial similarities to GHSL-2023-004, a better comparison would be run: ../../whatever.sh, which is not restricted by act or actions/runner (nor would it be feasible to do so IMO).

In the absence of being able to use contexts within uses, being able to use such a path is useful when working with composite actions in a monorepo, as explained on Rationale section of #2107

[ChristopherHX]

Yes we both have different opinions here... the good thing for you is that my opinion alone doesn't prevent merging this.

Keep in mind doing this in monorepos has an old unfixed bug when referencing local actions in actions/runner (those action which has post steps, like actions/cache, actions/checkout): actions/runner#2009, input and outputs end up with wrong values...

• If GitHub Actions would allow absolute paths in uses: /path/to/action, then it wouldn't be an exploit for me.
  • Get a approval from another maintainer and I might change my mind
• If we could restrict this to paths below rc.Container.GetActPath() and the workdir, I would pass your change (* I have no rights for getting your change merged).

I'm awaiting the opinion of the other maintainers if they have any, either way you need another review

[ChristopherHX]

Side note bug 2009 in actions/runner also technically allows endless recursion via local composite actions. I assume act has the same bug, however for both local and remote composite actions.

[github-actions]

PR is stale and will be closed in 14 days unless there is new activity

[jenseng]

I’d still like to see this reviewed and merged if possible, I have various actions that use this pattern, and I can’t currently run them under act. I’ll update the PR to resolve the conflicts

[mergify]

@jenseng this pull request has failed checks 🛠

[mergify]

@jenseng this pull request has failed checks 🛠

[jenseng]

🤔 lint failure seems like a bug in golangci/golangci-lint-action when using only-new-issues: true. i'll see if i can work around or get a fix made upstream.

i.e. using golangci-lint@1.53, i get the same failure locally if i do:
golangci-lint run --new-from-patch 0001-Allow-local-actions-outside-the-workspace.patch

or:
golangci-lint run --new-from-rev HEAD

but not if i do:
golangci-lint run pkg/runner

[ChristopherHX]

Yes only me again, one maintainer left (at least made the information available) after my last message here
So we are only cplee (owner, mostly reviewer only, not really active), wolfogre (Gitea Maintainer having their own fork, not really active) and me (have my fork as well).
Getting a review by them is out of my control, or it is unfinished and not visible.

My concern option B

If we could restrict this to paths below rc.Container.GetActPath() and the workdir, I would pass your change (* I have no rights for getting your change merged).

has not been resolved, so not my turn (I can technically elevate my permissings to merge PR's, but I would only do it if act is abandoned by the owner for an extended amount of time, but even then I have another program doing similar things like act as my own little project)

The stale bot seems at this state a bit rough

I did downgrade the go syntax in one of my PR's once due to this linter, but your case is different

Updating the linter using the version field would pull new lint errors of newer stricter rules I don't really like deal with.
I generally have my problems with the golangci linter and coverage requirements