Repository navigation
Conversation
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
jiangpengcheng
left a comment
jiangpengcheng
left a comment
There was a problem hiding this comment.
All 12 workflow regression tests passed locally on Python 3.11, and GitHub reports six passing checks on this head. Cloud E2E and publishing remain unverified.
| upload: | ||
| if: startsWith(github.ref, 'refs/tags/v') | ||
| name: Upload Release files | ||
| validate-tag: |
There was a problem hiding this comment.
[P1] Enforce release validation from a protected ref. GitHub loads the release workflow from the tagged commit. Tags that contain the previous workflow still use the old publisher and skip the new ancestry check. Move publishing and its credentials behind a workflow loaded from a protected ref.
| fi | ||
| git fetch --no-tags origin "refs/tags/$TAG:refs/tags/$TAG" | ||
| sha=$(git rev-parse --verify "refs/tags/$TAG^{commit}") | ||
| git merge-base --is-ancestor "$sha" origin/master |
There was a problem hiding this comment.
[P1] Preserve maintenance and release-candidate publishing. The manual bundle publisher requires tags to be ancestors of master. I verified that the published v0.28.1 and v0.30.0-rc-2 tags fail that check. Validate against the matching protected release branch or update the release process so the tagged commit lands on master first.
| name: Precommit - Integration Tests | ||
| on: | ||
| pull_request_target: | ||
| pull_request: |
There was a problem hiding this comment.
[P2] Restore Basic and TLS integration coverage. GitHub marks the existing workflow as disabled_manually. Changing its trigger in place leaves it disabled, and neither job ran on this head. Give the secretless workflow a new filename and verify both jobs on the PR.
|
|
||
| class WorkflowSecurity(unittest.TestCase): | ||
| def test_all_workflows_obey_baseline(self): | ||
| for path in sorted(WORKFLOWS.glob('*.yml')): |
There was a problem hiding this comment.
[P3] Check both workflow file extensions. The trust-boundary and shell syntax tests only enumerate *.yml. GitHub also accepts *.yaml, so include both extensions in both test loops.
Motivation
Audit and harden all 11 existing workflows, including PR-controlled execution, cloud secrets, token permissions, remote debugging, release inputs, action dependencies, caches, and artifacts. The full per-workflow findings, behavior changes, and remaining controls are in
.github/WORKFLOW_SECURITY.md.Changes
pull_requestexecution. Run OAuth2/downloader tests only on official-repositorymasterpushes or manual dispatches, with secrets scoped to the E2E step and GSA limited to its required case.masterbefore publishing jobs start, check out the validated commit SHA, and remove mutable release-helper downloads.Validation
Passed locally:
-shellcheck='' -pyflakes=''across all workflowsgit diff --checkThe new Workflow security Actions run also passed on head
0490c93b679bcce9b7ef68f860a99f9c763989e4, including dependency installation, actionlint and the trust-boundary tests. Other PR checks were still running when this note was updated.Not run locally: Docker/Kubernetes E2E, cloud OAuth/PubSub integration, release publication, or Red Hat submission. No release/cloud workflow was manually dispatched. Check the exact PR-head Actions results before merging.
Review notes
Cloud integration now runs after merge rather than on unmerged PRs. Documentation labeling intentionally drops the old bot's assignment/comment and extra-label behavior. Arbitrary release refs and unmerged release branches are rejected. Existing tag workflows are not retroactively changed. Repository rules, protected environments, credential rotation, cloud IAM, and complete transitive dependency reproducibility remain separate controls; this PR does not modify them.
docdoc-requiredno-need-doc