dist/ content validation

[GrantBirki]

Validate dist/ contents via CI

This pull request adds a new CI job to the repository that validates the contents of the dist/ directory to ensure that the contents exactly match the outputs of yarn build && yarn package without tampering.

This is directly adopted from the actions/typescript-action template repository which contains many best practices for building secure by default marketplace actions.

I personally use a workflow just like this one in many of my own Actions repositories so that I can be confident that the contents of the dist/ directory haven't had any sneaky changes slide in.

Let me know if you have any questions about this PR, thanks! 🙇

[joshmgross]

If we're pinning actions/upload-artifact, should we pin this as well?

[parkerbxyz]

Additionally, v4.3.0 is the latest.

Suggested change

	uses: actions/setup-node@v4.2.0
	uses: actions/setup-node@v4.3.0

Suggested change

	uses: actions/setup-node@v4.2.0
	uses: actions/setup-node@cdca7365b2dadb8aad0a33bc7601856ffabcc48e # v4.3.0

[GrantBirki]

@joshmgross I was going to but rather chose to keep it the same as all other references in this project for now:

changed-files/.github/workflows/test.yml

Lines 47 to 51 in 0fee5fb

	- name: Use Node.js 20.x
	uses: actions/[email protected]
	with:
	cache: 'yarn'
	node-version: '20.x'

A pretty good follow-up PR to this one would be going through all 3rd party workflows in this project and pinning them to exact commit SHAs IMO.

[cw-alexcroteau]

The maintainer seems to have pinned all other actions, I'd suggest pinning here as well.

[ViacheslavKudinov]

what if something like it ?
yarn install && yarn build && yarn format && yarn package same as in https://github.com/tj-actions/changed-files/blob/main/package.json#L18C42-L18C51 and it is called there https://github.com/tj-actions/changed-files/blob/main/.github/workflows/test.yml#L72

      - run: yarn install && yarn build && yarn format && yarn package
      - run: git status --porcelain
      - run: git diff --exit-code

Anchore uses this approach in their Actions https://github.com/anchore/scan-action/blob/main/.github/workflows/test.yml#L18-L23
https://github.com/anchore/sbom-action/blob/main/.github/workflows/test.yml#L21-L25

[GrantBirki]

Yep that could work as well. I'm pretty much going to leave this PR here "as-is" since it works and this is an implementation I have personally used in the past and it comes right from GitHub's own public template on building Actions. If the maintainers want to adopt it, tweak it, or make modifications that is totally fine and I would expect them to do so.

The extra reasons why I chose this approach was:

• It provides a way to debug unexpected (or even malicious) changes via the actions/upload-artifact step
• Calling a yarn format (or similar step) hasn't ever been something that is required when I used this in the past

[ViacheslavKudinov]

Sounds good!
It will help to catch the similar issue in the future, no matter which implementation to use.
Thank you for creating the PR @GrantBirki

[cw-alexcroteau]

All of this is great, however this repository's settings should be audited (e.g. using OSSF ScoreCard, although the maintainer seemed to have rejected its use in a different issue) to ensure that best practices are followed now, but most importantly, on an ongoing basis. We can't really know if commit signatures are enforced at the repo or even better, organization level!

Without signatures, this PR doesn't bring a lot of value, as an attacker could simply alter the content by modifying previous commits or something similar. Quite simple to hide a sneaky [no ci] in a long commit message in a PR, just as an example 😉 . The fact that historical commits are still unsigned after the compromise makes me think that this did not get resolved, and someone that gains write access to the repository could easily repeat the same pattern as what lead to the compromise.

[GrantBirki]

@cw-alexcroteau I think you are taking a more holistic approach to this which is great but I think you're overlooking how this change helps from a very simple maintenance level. If a user opens a PR that adds a single new core.debug("foo") statement to a source file, then when we re-compile dist/ in CI, we should expect it to match those exact changes. If the changes do not exactly match, it indicates something fishy happened.

Without a CI check like this, it is surprisingly easy to just stuff a exploit directly into any of the dist/* files that are a nightmare to read and currently contain no sort of validation.

[GrantBirki]

For example, have a look at this commit I made as a demo -> GrantBirki/comment@0960470

At a glance it looks totally harmless. Just adding a nice debug log right? Well most people would just assume everything is done correctly and not even bother to peer into these messy dist/ files.

However, if you expand those dist/ files, you can see that one was maliciously modified. Without any sort of validation, you might just miss it!

[ViacheslavKudinov]

@cw-alexcroteau I think you are taking a more holistic approach to this which is great but I think you're overlooking how this change helps from a very simple maintenance level. If a user opens a PR that adds a single new core.debug("foo") statement to a source file, then when we re-compile dist/ in CI, we should expect it to match those exact changes. If the changes do not exactly match, it indicates something fishy happened.

Without a CI check like this, it is surprisingly easy to just stuff a exploit directly into any of the dist/* files that are a nightmare to read and currently contain no sort of validation.

I agree with @GrantBirki these things are not replaceable, but could complement each other.

More we can suggest as a community to improve - more we all benefit from it.

As I know OSSF ScoreCard doesn't currently support Rulesets, if it is used instead of classical branch protection.
So, it will not show it is protected via Rules.

In reality it doesn't mean @jackton1 will accept all the suggestions.

I saw that Anchore uses on their repos https://probot.github.io/apps/dco/ GH app to be sure they get only signed commits.
But, is this app also secure enough to be used ?