Fix: Resolve Checkov CKV2_GHA_1 error by setting root permissions for workflows (closes #3026) #3032

andrewvaughan · 2023-10-21T16:30:43Z

Proposed Changes

Adds permissions: read-all to resolve Issue Reduce generated permissions to read-all at top level for generated workflow file #3026
Resolves a logical error I found in some of the .mega-linter.yml files

Specifically, this:

VALIDATE_ALL_CODEBASE: >-
  ${{
    github.event_name == 'push' &&
    contains(fromJSON('["refs/heads/main", "refs/heads/master"]'), github.ref)
  }}

This is intended to match both refs/heads/main and refs/heads/master as a shorthand for an or-statement, but it will also match branches like refs/head/main-staging or refs/head/masterfoobar which isn't intended. I've reverted these to a proper, explicit or statement.

Improved documentation and resources within .mega-linter.yml files

I have improved commenting within the configuration files to provide new users better resources
and information on what the files are for.

I've separated item 1 above from the rest into two commits, that way if the other changes are not of interest, we can just commit the permissions changes from issue #3026.

Readiness Checklist

Author/Contributor

Add entry to the CHANGELOG listing the change and linking to the corresponding issue (if appropriate)
If documentation is needed for this change, has that been included in this pull request

Reviewing Maintainer

Label as breaking if this is a large fundamental change
Label as either automation, bug, documentation, enhancement, infrastructure, or performance

README.md

CHANGELOG.md

TEMPLATES/mega-linter.yml

echoix · 2023-10-21T17:36:48Z

Wow! I like that style! It's not rare to have questions from new users that would have needed these kind of hints, either on usage or configuration.

rasa · 2023-10-21T20:21:03Z

    contains(fromJSON('["refs/heads/main", "refs/heads/master"]'), github.ref)
... but it will also match branches like refs/head/main-staging or refs/head/masterfoobar which isn't intended.

With all due respect, not according to
https://docs.github.com/en/actions/learn-github-actions/expressions#contains
which says:
Returns true if search contains item. If search is an array, this function returns true if the item is an element in the array. If search is a string, this function returns true if the item is a substring of search. This function is not case sensitive. Casts values to a string.

Edit: Though

(github.ref == "refs/heads/main" || github.ref == "refs/heads/master")

works just as well, and is perhaps more readable.

andrewvaughan · 2023-10-21T20:42:27Z

Wow! I like that style! It's not rare to have questions from new users that would have needed these kind of hints, either on usage or configuration.

Thanks! If you like my documentation design, you might like my https://github.com/andrewvaughan/template-core project. I've been revamping it the past week or so.

andrewvaughan · 2023-10-21T20:43:58Z

    contains(fromJSON('["refs/heads/main", "refs/heads/master"]'), github.ref)
... but it will also match branches like refs/head/main-staging or refs/head/masterfoobar which isn't intended.
With all due respect, not according to docs.github.com/en/actions/learn-github-actions/expressions#contains which says: Returns true if search contains item. If search is an array, this function returns true if the item is an element in the array. If search is a string, this function returns true if the item is a substring of search. This function is not case sensitive. Casts values to a string.

Edit: Though
(github.ref == "refs/heads/main" || github.ref == "refs/heads/master")
works just as well, and is perhaps more readable.

You're absolutely right and was a misunderstanding of how I understood contains( to work. I agree it's more readable and a better option then - but it'd be good for it to be consistent across the files (which it wasn't before). I will add a commit that updates it in a few.

echoix · 2023-10-21T20:55:45Z

I think there will be missing to sync up the other files and I'll take a new look

CHANGELOG.md

mega-linter-runner/generators/mega-linter/templates/mega-linter.yml

andrewvaughan · 2023-10-22T00:53:52Z

    contains(fromJSON('["refs/heads/main", "refs/heads/master"]'), github.ref)
... but it will also match branches like refs/head/main-staging or refs/head/masterfoobar which isn't intended.
With all due respect, not according to docs.github.com/en/actions/learn-github-actions/expressions#contains which says: Returns true if search contains item. If search is an array, this function returns true if the item is an element in the array. If search is a string, this function returns true if the item is a substring of search. This function is not case sensitive. Casts values to a string.
Edit: Though
(github.ref == "refs/heads/main" || github.ref == "refs/heads/master")
works just as well, and is perhaps more readable.
You're absolutely right and was a misunderstanding of how I understood contains( to work. I agree it's more readable and a better option then - but it'd be good for it to be consistent across the files (which it wasn't before). I will add a commit that updates it in a few.

I've been thinking on this further...

I think keeping the more readable version:

(github.ref == "refs/heads/main" || github.ref == "refs/heads/master")

Has one major benefit - it's super easy to understand what's going on there. I do quite a bit with GitHub Actions programming and even I missed exactly what was going on with the contains directive. My vote would be to keep it with an explicit || writing so people can extend it easily without having to look into what fromJSON and contains do (or make the mis-assumption like I did that contains is a string-based function and not an array-based function).

Either way, it's your call, but if you could please make a proposed change (if any) for the files as to how you'd like it to look, I'll accept them. Thanks!

andrewvaughan · 2023-10-22T01:00:34Z

QQ for the maintainers - I haven't been super verbose with my commit message when accepting proposals. Do you want me to squash those commits when we've resolved all conversations, or is that something you'll manage during acceptance of the PR?

echoix · 2023-10-22T01:11:41Z

It'll be a squashed commit at the end... so it doesn't really matter. You always could pre-squash them if you'd like, you'd be one of the first to do it, but the committing style hasn't been held to a high standard here yet.

echoix · 2023-10-22T01:15:05Z

contains(fromJSON('["refs/heads/main", "refs/heads/master"]'), github.ref)
... but it will also match branches like refs/head/main-staging or refs/head/masterfoobar which isn't intended.

With all due respect, not according to docs.github.com/en/actions/learn-github-actions/expressions#contains which says: Returns true if search contains item. If search is an array, this function returns true if the item is an element in the array. If search is a string, this function returns true if the item is a substring of search. This function is not case sensitive. Casts values to a string.

Edit: Though
(github.ref == "refs/heads/main" || github.ref == "refs/heads/master")
works just as well, and is perhaps more readable.

You're absolutely right and was a misunderstanding of how I understood contains( to work. I agree it's more readable and a better option then - but it'd be good for it to be consistent across the files (which it wasn't before). I will add a commit that updates it in a few.

I've been thinking on this further...

I think keeping the more readable version:
(github.ref == "refs/heads/main" || github.ref == "refs/heads/master")
Has one major benefit - it's super easy to understand what's going on there. I do quite a bit with GitHub Actions programming and even I missed exactly what was going on with the contains directive. My vote would be to keep it with an explicit || writing so people can extend it easily without having to look into what fromJSON and contains do (or make the mis-assumption like I did that contains is a string-based function and not an array-based function).

Either way, it's your call, but if you could please make a proposed change (if any) for the files as to how you'd like it to look, I'll accept them. Thanks!

I agree with the second though, going with the explicit boolean expression that any programmer should be able to figure out without any docs.

andrewvaughan · 2023-10-22T01:16:15Z

It'll be a squashed commit at the end... so it doesn't really matter. You always could pre-squash them if you'd like, you'd be one of the first to do it, but the committing style hasn't been held to a high standard here yet.

I'll let y'all do it so you can determine what messages are helpful and what aren't. My SDLc actually asks people not to pre-squash for that reason.

andrewvaughan · 2023-10-31T16:59:10Z

Just rebased the branch from main given the latest release.

echoix · 2023-10-31T23:12:18Z

contains(fromJSON('["refs/heads/main", "refs/heads/master"]'), github.ref)
... but it will also match branches like refs/head/main-staging or refs/head/masterfoobar which isn't intended.

With all due respect, not according to docs.github.com/en/actions/learn-github-actions/expressions#contains which says: Returns true if search contains item. If search is an array, this function returns true if the item is an element in the array. If search is a string, this function returns true if the item is a substring of search. This function is not case sensitive. Casts values to a string.

Edit: Though

(github.ref == "refs/heads/main" || github.ref == "refs/heads/master")

works just as well, and is perhaps more readable.

You're absolutely right and was a misunderstanding of how I understood contains( to work. I agree it's more readable and a better option then - but it'd be good for it to be consistent across the files (which it wasn't before). I will add a commit that updates it in a few.

I've been thinking on this further...
I think keeping the more readable version:
(github.ref == "refs/heads/main" || github.ref == "refs/heads/master")
Has one major benefit - it's super easy to understand what's going on there. I do quite a bit with GitHub Actions programming and even I missed exactly what was going on with the contains directive. My vote would be to keep it with an explicit || writing so people can extend it easily without having to look into what fromJSON and contains do (or make the mis-assumption like I did that contains is a string-based function and not an array-based function).
Either way, it's your call, but if you could please make a proposed change (if any) for the files as to how you'd like it to look, I'll accept them. Thanks!
I agree with the second though, going with the explicit boolean expression that any programmer should be able to figure out without any docs.
Cool, let's leave it as is, and that way they can add (or remove) any branches they need easily.

:Québec-Atlanta-High-Five:

I found out that we can use

github.ref == format('refs/heads/{0}', github.event.repository.default_branch)

instead of specifying what is the default branch in a or condition, What do you think?

.github/workflows/mega-linter-for-runner.yml

docs/install-github.md

mega-linter-runner/generators/mega-linter/templates/mega-linter.yml

echoix · 2023-10-31T22:37:39Z

docs/install-github.md

+          # Uncomment to disable copy-paste and spell checks
+          # DISABLE: COPYPASTE,SPELL
+


Suggested change

# Uncomment to disable copy-paste and spell checks

# DISABLE: COPYPASTE,SPELL

To match mega-linter-runner/generators/mega-linter/templates/mega-linter.yml

The other file doesn't have these 3 lines. to discuss

echoix · 2023-10-31T23:28:35Z

.github/workflows/mega-linter-for-runner.yml

+# By default, Megalinter runs whenever a Pull Request is opened with the default
+# branch, or on any push.
+#
+# Later logic enforces a full code-wide test on only the `production` and
+# `staging` Branches. The default Branch only has changed files linted for
+# efficiency.
+#


This is different from the template. The template has:

## # This `env` section can be entirely removed or commented out if you do not wish # for fixes to be applied during the MegaLinter run. # # More info at: # https://docs.github.com/en/actions/learn-github-actions/contexts#env-context

.github/workflows/mega-linter-for-runner.yml

andrewvaughan · 2023-10-31T23:34:33Z

contains(fromJSON('["refs/heads/main", "refs/heads/master"]'), github.ref)
... but it will also match branches like refs/head/main-staging or refs/head/masterfoobar which isn't intended.

With all due respect, not according to docs.github.com/en/actions/learn-github-actions/expressions#contains which says: Returns true if search contains item. If search is an array, this function returns true if the item is an element in the array. If search is a string, this function returns true if the item is a substring of search. This function is not case sensitive. Casts values to a string.

Edit: Though

(github.ref == "refs/heads/main" || github.ref == "refs/heads/master")

works just as well, and is perhaps more readable.

You're absolutely right and was a misunderstanding of how I understood contains( to work. I agree it's more readable and a better option then - but it'd be good for it to be consistent across the files (which it wasn't before). I will add a commit that updates it in a few.

I've been thinking on this further...

I think keeping the more readable version:
(github.ref == "refs/heads/main" || github.ref == "refs/heads/master")
Has one major benefit - it's super easy to understand what's going on there. I do quite a bit with GitHub Actions programming and even I missed exactly what was going on with the contains directive. My vote would be to keep it with an explicit || writing so people can extend it easily without having to look into what fromJSON and contains do (or make the mis-assumption like I did that contains is a string-based function and not an array-based function).

Either way, it's your call, but if you could please make a proposed change (if any) for the files as to how you'd like it to look, I'll accept them. Thanks!

I agree with the second though, going with the explicit boolean expression that any programmer should be able to figure out without any docs.

Cool, let's leave it as is, and that way they can add (or remove) any branches they need easily.

:Québec-Atlanta-High-Five:

I found out that we can use
github.ref == format('refs/heads/{0}', github.event.repository.default_branch)
instead of specifying what is the default branch in a or condition, What do you think?

I had something like this originally, but read that default branch is not available in the if conditional section.

echoix · 2023-10-31T23:42:05Z

I had something like this originally, but read that default branch is not available in the if conditional section.

Oh, let me continue testing it myself in my fork, since some older stackoverflow posts are outdated, since has made some updates over time, ie it wasn't available for scheduled events, but it is now.

echoix · 2023-11-21T01:00:29Z

/build

Command run output
Build command workflow started.
Installing dependencies
Running script ./build.sh
Build command workflow completed updating files.

echoix · 2023-11-21T01:15:46Z

CHANGELOG.md

@@ -118,13 +118,15 @@ Note: Can be used with `oxsecurity/megalinter@beta` in your GitHub Action mega-l
 - Fixes
  - build.py: Remove exclusivity between pip, gem & cargo packages
  - Salesforce linters: Switch sfdx-cli to @salesforce/cli
+  - Set default permissions to all workflows to `read-all` to increase security and prevent Checkov `CKV2_GHA_1` errors, by @andrewvaughan in [#3032](https://github.com/oxsecurity/megalinter/pull/3032)


Should move back to next version

echoix · 2023-11-21T01:16:01Z

CHANGELOG.md

  - Fixed issue with `actionlint` throwing an error on `if` statements in the generated workflow file
  - Added default `.devskim.json` to mitigate errors introduced when no config exists

 - Doc
  - Display list of articles from newest to oldest
  - Fix incorrect environment variable in djlint docs
  - Improve lychee documentation to add an example of `.lycheeignore`
+  - Improved commenting in `.mega-linter.yml` file to help new users configure the GitHub workflow more effectively


Should move to next version

rasa · 2023-11-21T02:08:30Z

.github/workflows/mega-linter-for-runner.yml

@@ -113,61 +258,91 @@ jobs:
            megalinter-reports
            mega-linter.log

-      # Set APPLY_FIXES_IF var for use in future steps


Please freshen this PR from main, so we don't undo #2957 's changes.

rasa · 2023-11-21T02:09:06Z

.github/workflows/mega-linter-for-runner.yml

      - name: Create Pull Request with applied fixes
        uses: peter-evans/create-pull-request@v5
        id: cpr
-        if: env.APPLY_FIXES_IF_PR == 'true'


Undoes #2957

rasa · 2023-11-21T02:09:16Z

.github/workflows/mega-linter-for-runner.yml

        with:
          token: ${{ secrets.PAT || secrets.GITHUB_TOKEN }}
          commit-message: "[MegaLinter] Apply linters automatic fixes"
          title: "[MegaLinter] Apply linters automatic fixes"
          labels: bot

-      - name: Create PR output
-        if: env.APPLY_FIXES_IF_PR == 'true'


Undoes #2957

rasa · 2023-11-21T02:09:27Z

.github/workflows/mega-linter-for-runner.yml

      - name: Prepare commit
-        if: env.APPLY_FIXES_IF_COMMIT == 'true'


Undoes #2957

rasa · 2023-11-21T02:10:33Z

.github/workflows/mega-linter-for-runner.yml

        run: sudo chown -Rc $UID .git/

      - name: Commit and push applied linter fixes
-        uses: stefanzweifel/git-auto-commit-action@v5


Please freshen from main, as the action's version is being downgraded from v5 to v4.

rasa · 2023-11-21T02:13:15Z

.github/workflows/mega-linter.yml

@@ -113,61 +265,91 @@ jobs:
            megalinter-reports
            mega-linter.log

-      # Set APPLY_FIXES_IF var for use in future steps


Undoes #2957

rasa · 2023-11-21T02:14:12Z

.github/workflows/mega-linter.yml

        run: sudo chown -Rc $UID .git/

      - name: Commit and push applied linter fixes
-        uses: stefanzweifel/git-auto-commit-action@v5


Please freshen from main per s/v5/v4/

rasa · 2023-11-21T02:14:48Z

Dockerfile

@@ -34,7 +34,7 @@ FROM yoheimuta/protolint:latest as protolint
 FROM golang:alpine as dustilock
 RUN GOBIN=/usr/bin go install github.com/checkmarx/[email protected]

-FROM zricethezav/gitleaks:v8.18.1 as gitleaks


Please freshen from main per s/1/0/

rasa · 2023-11-21T02:18:42Z

TEMPLATES/mega-linter.yml

@@ -108,61 +264,87 @@ jobs:
            megalinter-reports
            mega-linter.log

-      # Set APPLY_FIXES_IF var for use in future steps


Freshen this PR from main to avoid undoing #2957

echoix · 2023-11-21T02:29:28Z

After #3143, I think we're finally ready to tackle this, as the last release is now done, and the needed fixes to main (and the docs) will be finished.

nvuillam · 2023-12-06T22:38:25Z

@echoix you can merge this one once you are confident of its content :) (looks promising but you investigated it more than i did ^^ )

echoix · 2023-12-06T22:53:32Z

It will need a little manual work of fixing conflicts, and it seems a bad merge (or outdated starting point) reverted some changes of a previous PR.

As the potential breaking changes would be limited to new users generating a new workflow, I don't think it's really a problem, since there was nothing to break to begin with. If users would want to recreate their workflow by trying again and incorporating their changes back, it's ok, and that's how I expect a template generator to work.

echoix · 2023-12-06T22:54:31Z

And a title that better describes what that PR ended up being :)

Jayllyz · 2024-02-19T21:59:15Z

Any update on this fix ?
Waiting this PR to be merged to fix this one : #3049

andrewvaughan requested review from nvuillam, echoix, bdovaz and Kurt-von-Laven as code owners October 21, 2023 16:30

echoix requested changes Oct 21, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

echoix temporarily deployed to dev October 21, 2023 17:08 — with GitHub Actions Inactive

echoix had a problem deploying to dev October 21, 2023 17:08 — with GitHub Actions Error

echoix reviewed Oct 21, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

echoix added the github_actions Pull requests that update Github_actions code label Oct 21, 2023

andrewvaughan had a problem deploying to dev October 21, 2023 17:14 — with GitHub Actions Error

andrewvaughan had a problem deploying to dev October 21, 2023 17:16 — with GitHub Actions Failure

echoix reviewed Oct 21, 2023

View reviewed changes

andrewvaughan had a problem deploying to dev October 21, 2023 20:44 — with GitHub Actions Error

andrewvaughan had a problem deploying to dev October 21, 2023 20:45 — with GitHub Actions Error

andrewvaughan had a problem deploying to dev October 21, 2023 20:45 — with GitHub Actions Failure

echoix requested changes Oct 21, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

echoix mentioned this pull request Oct 21, 2023

Apply pr2957 to mega-linter-runner/.../mega-linter.yml #3033

Closed

echoix requested changes Oct 21, 2023

View reviewed changes

mega-linter-runner/generators/mega-linter/templates/mega-linter.yml Outdated Show resolved Hide resolved

andrewvaughan had a problem deploying to dev October 22, 2023 00:51 — with GitHub Actions Error

andrewvaughan had a problem deploying to dev October 22, 2023 00:54 — with GitHub Actions Failure

echoix added 3 commits October 31, 2023 12:56

Remove duplicated if in mega-linter-for-runner.yml

9133abd

Run ./build.sh --doc on PR

e3a56cd

Apply suggestions from code review

aa96d18

andrewvaughan force-pushed the bug/3026/checkov-CKV2_GHA_1 branch from febce26 to aa96d18 Compare October 31, 2023 16:57

andrewvaughan temporarily deployed to dev October 31, 2023 16:57 — with GitHub Actions Inactive

echoix reviewed Oct 31, 2023

View reviewed changes

Apply suggestions from code review

c281d8c

echoix temporarily deployed to dev October 31, 2023 23:38 — with GitHub Actions Inactive

echoix mentioned this pull request Nov 4, 2023

Call the mega-linter-runner installer to generate the examples in the docs #3073

Draft

10 tasks

Merge branch 'main' into bug/3026/checkov-CKV2_GHA_1

c941ec6

nvuillam temporarily deployed to dev November 18, 2023 19:27 — with GitHub Actions Inactive

Merge branch 'main' into bug/3026/checkov-CKV2_GHA_1

2d08f28

echoix temporarily deployed to dev November 21, 2023 01:01 — with GitHub Actions Inactive

[build-command] Update generated files

8766889

echoix requested changes Nov 21, 2023

View reviewed changes

rasa suggested changes Nov 21, 2023

View reviewed changes

Merge branch 'main' into bug/3026/checkov-CKV2_GHA_1

7343ffe

nvuillam temporarily deployed to dev November 21, 2023 12:58 — with GitHub Actions Inactive

Merge branch 'main' into bug/3026/checkov-CKV2_GHA_1

a0040e5

nvuillam temporarily deployed to dev December 6, 2023 22:38 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Resolve Checkov CKV2_GHA_1 error by setting root permissions for workflows (closes #3026) #3032

Fix: Resolve Checkov CKV2_GHA_1 error by setting root permissions for workflows (closes #3026) #3032

andrewvaughan commented Oct 21, 2023

echoix commented Oct 21, 2023

rasa commented Oct 21, 2023 •

edited

Loading

andrewvaughan commented Oct 21, 2023 •

edited

Loading

andrewvaughan commented Oct 21, 2023

echoix commented Oct 21, 2023

andrewvaughan commented Oct 22, 2023

andrewvaughan commented Oct 22, 2023

echoix commented Oct 22, 2023

echoix commented Oct 22, 2023

andrewvaughan commented Oct 22, 2023

andrewvaughan commented Oct 31, 2023

echoix commented Oct 31, 2023

echoix Oct 31, 2023

echoix Oct 31, 2023

andrewvaughan commented Oct 31, 2023

echoix commented Oct 31, 2023

echoix commented Nov 21, 2023 •

edited by github-actions bot

Loading

echoix Nov 21, 2023

echoix Nov 21, 2023

rasa Nov 21, 2023

rasa Nov 21, 2023

rasa Nov 21, 2023

rasa Nov 21, 2023

rasa Nov 21, 2023

rasa Nov 21, 2023

rasa Nov 21, 2023

rasa Nov 21, 2023

rasa Nov 21, 2023

echoix commented Nov 21, 2023

nvuillam commented Dec 6, 2023

echoix commented Dec 6, 2023

echoix commented Dec 6, 2023

Jayllyz commented Feb 19, 2024

		# Uncomment to disable copy-paste and spell checks
		# DISABLE: COPYPASTE,SPELL

		- name: Prepare commit
		if: env.APPLY_FIXES_IF_COMMIT == 'true'

Fix: Resolve Checkov CKV2_GHA_1 error by setting root permissions for workflows (closes #3026) #3032

Are you sure you want to change the base?

Fix: Resolve Checkov CKV2_GHA_1 error by setting root permissions for workflows (closes #3026) #3032

Conversation

andrewvaughan commented Oct 21, 2023

Proposed Changes

Readiness Checklist

Author/Contributor

Reviewing Maintainer

echoix commented Oct 21, 2023

rasa commented Oct 21, 2023 • edited Loading

andrewvaughan commented Oct 21, 2023 • edited Loading

andrewvaughan commented Oct 21, 2023

echoix commented Oct 21, 2023

andrewvaughan commented Oct 22, 2023

andrewvaughan commented Oct 22, 2023

echoix commented Oct 22, 2023

echoix commented Oct 22, 2023

andrewvaughan commented Oct 22, 2023

andrewvaughan commented Oct 31, 2023

echoix commented Oct 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewvaughan commented Oct 31, 2023

echoix commented Oct 31, 2023

echoix commented Nov 21, 2023 • edited by github-actions bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

echoix commented Nov 21, 2023

nvuillam commented Dec 6, 2023

echoix commented Dec 6, 2023

echoix commented Dec 6, 2023

Jayllyz commented Feb 19, 2024

rasa commented Oct 21, 2023 •

edited

Loading

andrewvaughan commented Oct 21, 2023 •

edited

Loading

echoix commented Nov 21, 2023 •

edited by github-actions bot

Loading