Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run actionlint static checker pre-merge. #1312

Merged
merged 11 commits into from
May 20, 2024
Merged

Run actionlint static checker pre-merge. #1312

merged 11 commits into from
May 20, 2024

Conversation

sengi
Copy link
Contributor

@sengi sengi commented May 16, 2024

https://github.com/rhysd/actionlint

We badly need this to help prevent more easily-avoidable vulns and bugs creeping into the reusable actions/workflows. Probably should have added this a long time ago, but hindsight etc. etc.

Fix all the pre-existing lints so that we can make this a mandatory check.

Tested: successfully ran the multi-arch build workflow and the Brakeman workflow. The others aren't so easy to test but they're also not that operationally important so we can just fix forward if I've introduced any issues.

@sengi sengi force-pushed the sengi/actionlint branch from 8a54e17 to b05cf7b Compare May 16, 2024 11:16
@sengi
Copy link
Contributor Author

sengi commented May 16, 2024

Heh, this might take a while. Almost wish I hadn't looked underneath this rock 😂

@sengi sengi marked this pull request as draft May 16, 2024 11:26
@sengi sengi force-pushed the sengi/actionlint branch 10 times, most recently from 380cab0 to a5eafc0 Compare May 16, 2024 15:17
@sengi sengi marked this pull request as ready for review May 16, 2024 15:18
@sengi sengi requested a review from dj-maisy May 16, 2024 15:18
"owner": "actionlint",
"pattern": [
{
"regexp": "^(?:\\x1b\\[\\d+m)?(.+?)(?:\\x1b\\[\\d+m)*:(?:\\x1b\\[\\d+m)*(\\d+)(?:\\x1b\\[\\d+m)*:(?:\\x1b\\[\\d+m)*(\\d+)(?:\\x1b\\[\\d+m)*: (?:\\x1b\\[\\d+m)*(.+?)(?:\\x1b\\[\\d+m)* \\[(.+?)\\]$",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that this is matching based on the colours, but meh, it works and I don't want to rewrite it myself :/

@sengi sengi force-pushed the sengi/actionlint branch 5 times, most recently from 45464c3 to fad613e Compare May 17, 2024 15:44
@sengi
Copy link
Contributor Author

sengi commented May 17, 2024

RFAL

sengi added 3 commits May 17, 2024 16:47
- Don't eval arbitrary Ruby code.
- Use shell quotes correctly to avoid unintentional word-splitting etc.
- Quote the version string correctly for sed.
- Simplify the version string parsing.
- Fix a ton of shellcheck errors.
@sengi sengi force-pushed the sengi/actionlint branch 5 times, most recently from e45101d to f3ea433 Compare May 18, 2024 15:03
@sengi sengi force-pushed the sengi/actionlint branch from 3f2dca7 to e9671a0 Compare May 18, 2024 15:24
sengi added 8 commits May 18, 2024 16:27
- Don't substitute values directly into shell scripts. This helps avoid
  command-injection vulns. (Kinda crazy that GitHub even allows this,
  let alone without any kind of built-in lint warnings.)
- Omit superfluous `${{ }}` in `if:` expressions.
- Reduce logspew from apt-get and set it to noninteractive mode.
- Use `apt-get --no-install-recommends` to avoid installing some
  unnecessary packages.
- Avoid some excessive version pinning (update toil) on the AWS actions.
- Fix a bunch of shellcheck issues (mostly with quoting).
Fixes `-ignore` config. Also a bit less third-party stuff to deal with.
- Don't substitute values from GitHub Actions directly into shell
  commands.
- Fix shellcheck issues (mostly with quoting).
- Don't eval arbitrary Ruby code.
- Fix shellcheck issues (mostly with quoting).
- Use all-caps only for actual environment variables.
- Remove the usage comment; we decided a while back not to include
  these as they just get out of date and cause more hassle than they
  save.
- Surface some of the messages as `notice`-level output in GitHub.
- Fix actionlint issues.
- Suppress an actionlint warning about `github.repository_visibility`.
  This appears to be an [undocumented] property of the `github` context.
- Use an up-to-date version of upload-artifact.
- Just output both formats from Brakeman in all cases and upload the the
  SARIF if we're in a public repo. This simplifies the workflow.

[undocumented]: https://docs.github.com/en/actions/learn-github-actions/contexts
@sengi sengi force-pushed the sengi/actionlint branch from e9671a0 to dd614b7 Compare May 18, 2024 15:28
@sengi sengi requested a review from theseanything May 20, 2024 09:48
Copy link
Member

@dj-maisy dj-maisy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥇

@sengi sengi merged commit 7b81761 into main May 20, 2024
1 check passed
@sengi sengi deleted the sengi/actionlint branch May 20, 2024 11:13
@nimalank7
Copy link
Contributor

Closes #1312

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants