> Users frequently over-scope their workflow and job permissions, or set broad workflow-level permissions without realizing that all jobs inherit those permissions.
>
> Furthermore, users often don't realize that the _default_ `GITHUB_TOKEN` permissions can be very broad, meaning that workflows that don't configure any permissions at all can _still_ provide excessive credentials to their individual jobs.
>
> **Remediation**
> In general, permissions should be declared as minimally as possible, and as close to their usage site as possible.
>
> In practice, this means that workflows should almost always set `permissions: {}` at the workflow level to disable all permissions by default, and then set specific job-level permissions as needed.
This was already addressed for the other two workflows, just not for the `tests` one.
As far as I can see, the jobs here do not need the `GITHUB_TOKEN` secret and even if they do, only for `content: read`, which for public repos does not need to be set explicitly, though it doesn't do any harm to have that set anyway.
Refs:
* https://docs.zizmor.sh/audits/#excessive-permissions
> By default, using `actions/checkout` causes a credential to be persisted in the checked-out repo's `.git/config`, so that subsequent `git` operations can be authenticated.
>
> Subsequent steps may accidentally publicly persist `.git/config`, e.g. by including it in a publicly accessible artifact via `actions/upload-artifact`.
>
> However, even without this, persisting the credential in the `.git/config` is non-ideal unless actually needed.
>
> **Remediation**
>
> Unless needed for `git` operations, `actions/checkout` should be used with `persist-credentials: false`.
>
> If the persisted credential is needed, it should be made explicit with `persist-credentials: true`.
This has now been addressed in all workflows.
Refs:
* https://unit42.paloaltonetworks.com/github-repo-artifacts-leak-tokens/
* https://docs.zizmor.sh/audits/#artipacked
Recently there has been more and more focus on securing GH Actions workflows - in part due to some incidents.
The problem with "unpinned" action runners is as follows:
* Tags are mutable, which means that a tag could point to a safe commit today, but to a malicious commit tomorrow.
Note that GitHub is currently beta-testing a new "immutable releases" feature (= tags and release artifacts can not be changed anymore once the release is published), but whether that has much effect depends on the ecosystem of the packages using the feature.
Aside from that, it will likely take years before all projects adopt _immutable releases_.
* Action runners often don't even point to a tag, but to a branch, making the used action runner a moving target.
_Note: this type of "floating major" for action runners used to be promoted as good practice when the ecosystem was "young". Insights have since changed._
While it is convenient to use "floating majors" of action runners, as this means you only need to update the workflows on a new major release of the action runner, the price is higher risk of malicious code being executed in workflows.
Dependabot, by now, can automatically submit PRs to update pinned action runners too, as long as the commit-hash pinned runner is followed by a comment listing the released version the commit is pointing to.
So, what with Dependabot being capable of updating workflows with pinned action runners, I believe it is time to update the workflows to the _current_ best practice of using commit-hash pinned action runners.
The downside of this change is that there will be more frequent Dependabot PRs.
If this would become a burden/irritating, the following mitigations can be implemented:
1. Updating the Dependabot config to group updates instead of sending individual PRs per action runner.
2. A workflow to automatically merge Dependabot PRs as long as CI passes.
Includes updating the version for `ossf/scorecard-action` as it was a couple of version behind.
Ref: https://docs.github.com/en/actions/reference/security/secure-use#using-third-party-actions
While workflows are disabled by default in forks, it is quite common for contributors to enable them to verify CI will pass before submitting a pull request.
When enabling workflow runs in forks, it's "all or nothing".
This means that:
* All workflows which are only intended to be run on the canonical repo will also be enabled.
These workflows will also often need access to repo-specific secrets and will typically fail when run from a fork.
* Workflows which contain cron jobs will also be enabled.
Depending on the type of account the contributor has, this can burn through their "CI minutes".
This commit is based on a review of workflows containing cron jobs and disables running the jobs when a cron job is triggered in a fork.
PHP 8.4 removes the IMAP extension (moved to PECL).
With this in mind, I've reviewed how the tests are being run versus the extension requirements and recommendations.
As things are, the tests are currently run in the "ideal" environment, i.e. with all required and optional extensions available.
However, the codebase also contains fall-backs for when certain extensions are **_not_** available and for at least some of those fallbacks, there are dedicated tests available, but in an ideal environment those tests will not run and the fall-backs are not tested, which is the case with the current CI setup.
To improve this situation, I'm proposing to keep running the tests against all PHP versions with the "ideal" extension set, but to also have additional test runs with a far more limited set of PHP extensions.
To determine which extensions should be in each set, I've looked at the following:
* `@requires` tags found in the test suite and the conditions for calls to `markTestSkipped()`.
This brought to light that the `openssl` extension was currently not listed in the "ideal" extension set. This has now been fixed.
* The required extensions of PHPUnit - `dom, json, libxml, mbstring, tokenizer, xml, xmlwriter`.
* The required extensions of PHPMailer itself - `ctype, filter, hash`.
* Not strictly required, but more for convenience/workflow speed: `curl` for Composer.
* And `xdebug` will still be enabled/disabled based on the `coverage` setting.
Note: while some tests would benefit from being run _without_ the `mbstring` extension, that's unfortunately not an option as `mbstring` is a requirement of PHPUnit 🤷
Also note, the tests with the "minimal" extension setup needs to run `composer install` with an `--ignore-platform-req` flag to prevent running into the following issue:
```
Running update with --no-dev does not mean require-dev is ignored, it just means the packages will not be installed. If dev requirements are blocking the update you have to resolve those problems.
```
As this extension "requirement" is for a dependency which is not used in the test run, the extension requirement can be safely ignored.
dependabot[bot]
Marcus Bointon
jrfnl