Skip to content

Improve build_cts_json.yaml workflow security #105

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Marcono1234
Copy link
Contributor

@Marcono1234 Marcono1234 commented Apr 24, 2025

Limits the increased permissions to a dedicated job which runs only on push events to main.

The previous implementation was already enough to protect against malicious PRs from forks, see for example this run (not malicious), which only has Contents: read as desired.

However, if Dependabot was used on the repository (at some point in the future), it would have increased its default read-only permissions to write permissions, making it easier for compromised dependencies to directly affect the repository.

With the new changes an unprivileged job builds the cts.json now, and more privileged job pushes the changes, but only when running on main. The cts.json file is passed as artifact between the jobs (as described in the documentation).
This should hopefully make it more secure.

Side note: These changes have the side effect that the modified cts.json file now also becomes available in the "Artifacts" section of runs (even for pull requests), see for example https://github.com/jsonpath-standard/jsonpath-compliance-test-suite/actions/runs/14638122807.

Limits the increased permissions to a dedicated job which runs only on push events.

The previous implementation was already enough to protect against malicious PRs from forks.
However, if Dependabot was used on the repository, it would have increased its default
read-only permissions to write permissions, making it easier for compromised dependencies
to directly affect the repository.
@Marcono1234
Copy link
Contributor Author

If you don't think this is worth it or needed, feel free to close this pull request. As mentioned above, the existing workflow should already be safe against malicious PRs from external forks.

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.

1 participant