Skip to content

ci: declare workflow-level contents: read on chart-test and helm-test#221

Open
arpitjain099 wants to merge 2 commits into
ClickHouse:mainfrom
arpitjain099:chore/declare-workflow-perms
Open

ci: declare workflow-level contents: read on chart-test and helm-test#221
arpitjain099 wants to merge 2 commits into
ClickHouse:mainfrom
arpitjain099:chore/declare-workflow-perms

Conversation

@arpitjain099

Copy link
Copy Markdown

Both workflows just run helm chart tests; no GitHub API writes from the workflows.

Same post-CVE-2025-30066 (tj-actions/changed-files) hardening pattern. YAML validated locally.

Both workflows just run helm chart tests; no GitHub API writes. contents: read at workflow level is the right cap.

Post-CVE-2025-30066 hardening pattern. yaml.safe_load validated.

Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
@arpitjain099 arpitjain099 requested a review from a team as a code owner May 25, 2026 03:15
@changeset-bot

changeset-bot Bot commented May 25, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: d1bb887

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@wrn14897

Copy link
Copy Markdown
Collaborator

Thanks for the contribution! I think the content permission should be read by default, but it's always good to be explicit about it.

@github-actions

Copy link
Copy Markdown
Contributor

Deep Review

The change adds a workflow-level permissions: contents: read block to two CI workflows. helm-test.yaml only checks out code and runs helm template/helm unittest, so contents: read is correct and complete there. chart-test.yml, however, contains an otel-cicd-action job that reads workflow-run/job data via the GitHub Actions REST API — a scope this block silently removes.

🔴 P0/P1 — must fix

  • .github/workflows/chart-test.yml:12 — The workflow-level permissions: contents: read sets every unlisted scope (including actions) to none, but the otel-cicd-action job calls the Actions REST API to export run traces, which needs actions: read, so it will 403 on push-to-main, nightly schedule, and internal-PR runs where OTLP_ENDPOINT is set.
    • Fix: Grant actions: read to the otel-cicd-action job via a job-level permissions: block (preferred, keeps other jobs least-privilege) or add actions: read to the workflow-level block.
    • correctness, reliability, security

🟡 P2 — recommended

  • .github/workflows/chart-test.yml:116 — Third-party actions are pinned to mutable tags (@v3/@v4) rather than commit SHAs, leaving the same supply-chain injection vector the cited CVE class exploits; pre-existing, not introduced here, but in scope of the workflow's stated threat model.
    • Fix: Pin corentinmusard/otel-cicd-action, azure/setup-helm, actions/checkout, and actions/setup-node to full commit SHAs with a trailing version comment, in both workflows.

Reviewers (6): correctness, reliability, security, project-standards, maintainability, testing.

Testing gaps:

  • The new permissions block cannot be validated by PR CI: the otel-cicd-action job is skipped on fork PRs, so a scope regression surfaces only on the first post-merge push to main or the next nightly schedule run — a green PR check does not confirm the otel job still works.

@arpitjain099

Copy link
Copy Markdown
Author

Thanks for the contribution! I think the content permission should be read by default, but it's always good to be explicit about it.

Thank you @wrn14897

@arpitjain099

Copy link
Copy Markdown
Author

Thanks @wrn14897! Right, contents:read is the default here, so this is really just making it explicit. The value is defense-in-depth: if a step ever gets compromised (a bad third-party action version, an injected expression), an explicit read-only token cannot be quietly used to write back to the repo. After the tj-actions/changed-files compromise earlier this year that caught a lot of CI pipelines, being explicit about least privilege on the token felt worth it. Totally your call though, and thanks for taking a look.

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.

2 participants