Skip to content

fix(ci): run release safeguard test in unprivileged workflow - #77

Open
Sayt-0 wants to merge 1 commit into
mainfrom
fix/codeql-untrusted-checkout
Open

fix(ci): run release safeguard test in unprivileged workflow#77
Sayt-0 wants to merge 1 commit into
mainfrom
fix/codeql-untrusted-checkout

Conversation

@Sayt-0

@Sayt-0 Sayt-0 commented Aug 5, 2026

Copy link
Copy Markdown
Member

Summary

Follow-up to #76 that resolves CodeQL alert #204.

Before After
A privileged workflow_run job checked out github.event.workflow_run.head_sha and executed a PR-controlled shell script The shell test runs in the existing unprivileged pull_request/push test workflow
Future permission or runner changes could increase the blast radius The test remains isolated with contents: read and no secrets

Changes

  • remove the release caller-permissions test job from .github/workflows/test-e2e.yml
  • run tests/test-release-caller-permissions.sh after unit tests in .github/workflows/test.yml
  • preserve automatic PR coverage without executing PR-controlled code from workflow_run

Validation

  • actionlint -shellcheck=
  • bash tests/test-release-caller-permissions.sh (7 scenarios passed)
  • git diff --check

Move the PR-controlled shell test out of the workflow_run context and into the pull_request test workflow to resolve the CodeQL untrusted-checkout finding introduced by #76.

@aheritier aheritier left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed at head d725b67. CI is green (8/8 check-runs), but I can't recommend merging as-is: the PR has merge conflicts and its stated purpose has already been fixed on main.

[blocking] The CodeQL finding this PR targets is already fixed on main.

  • gh api repos/docker/docker-agent-action/code-scanning/alerts/204"state": "fixed".
  • git log 5396207..origin/main -- .github/workflows/test-e2e.yml3776045 fix(security): harden fork workflow-run context (#88), which deleted ref: ${{ github.event_name == 'workflow_run' && github.event.workflow_run.head_sha || '' }} from all three shell-test jobs and added a direct pull_request: trigger to test-e2e.yml.
  • On current main, .github/workflows/test-e2e.yml:60-77 runs test-release-caller-permissions with if: push || pull_request || workflow_dispatch, permissions: contents: read, and a default (base-ref) checkout — no PR-controlled ref, no secrets.

So the "Before" column in the PR description no longer describes main, and merging this delivers no security improvement — only a workflow reorganisation. Please either close it or re-scope/rebase it as an explicit consolidation change.

[blocking] Merge conflict.

git merge-tree --write-tree <pr-head> origin/main
CONFLICT (content): Merge conflict in .github/workflows/test-e2e.yml

The PR branch is based on 5396207; #88 rewrote the same lines.

[should-fix] If kept as a consolidation, keep the three shell tests together.
.github/workflows/test.yml:74-75 adds the caller-permissions script to the Unit Tests job while test-output-extraction and test-job-summary stay in test-e2e.yml (:22-58). That splits three sibling shell tests across two workflows, and a "keep both sides" conflict resolution would run this script twice per PR. Move all three, or none.

[optional] The new step lands in the job named Unit Tests (test.yml:57), so a shell-integration failure will be reported as a unit-test failure, and it serialises behind pnpm test. AGENTS.md:237-241 documents tests/*.sh as shell integration tests distinct from pnpm test.

Verified as correct in this change (no action needed):

  • Dropping cd tests is safe — the script resolves its own paths (tests/test-release-caller-permissions.sh:19-20, SCRIPT_DIR/REPO_ROOT).
  • Dropping chmod +x is safe — the file is mode 100755 in the tree.
  • No pnpm build needed — the script stubs git/node/gh, so the Unit Tests job's pnpm install-only setup suffices. Ran locally: all 7 scenarios pass, exit 0.

@aheritier

Copy link
Copy Markdown
Collaborator

PR #77 is superseded by the security remediation already merged in #88 and no longer merges cleanly with main. Recommend closing #77 rather than rebasing it; any remaining shell-test consolidation should be handled in a narrowly scoped follow-up.

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