Publish wheels from a separate workflow in target repo context#381
Publish wheels from a separate workflow in target repo context#381aristarkhovNV wants to merge 1 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughThe changes reorganize the GitHub Actions CI/CD workflow by extracting three jobs ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
aristarkhovNV
changed the title
…n target repo context
aristarkhovNV
force-pushed
the
aaristarkhov/publish-from-target
branch
from
8bda5c6 to
b0d4345
Compare
There was a problem hiding this comment.
Pull request overview
Adds a dedicated “Release” GitHub Actions workflow that runs in the target repository context (triggered via workflow_run from “Build Ubuntu”) to publish built wheel artifacts to internal Artifactory and optionally submit/monitor a Kitmaker release.
Changes:
- Introduce
.github/workflows/release.ymltriggered by successful completion of the “Build Ubuntu” workflow. - Move wheel publishing + Kitmaker submission/status monitoring out of the build workflow and into the new release workflow.
- Remove the in-workflow publish/release jobs from
.github/workflows/build-ubuntu.yml.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| .github/workflows/release.yml | New workflow to download wheels from the triggering run, publish to Artifactory, and orchestrate Kitmaker release steps. |
| .github/workflows/build-ubuntu.yml | Removes the embedded publish/Kitmaker jobs now handled by the separate release workflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Publish only for PRs wihtin the canonical repository after the full build + test pipeline succeeds. | ||
| if: ${{ github.repository == 'NVIDIA/IsaacTeleop' && github.event.workflow_run.conclusion == 'success' }} |
There was a problem hiding this comment.
The publish-wheel job runs on workflow_run and has access to deployment secrets, but its if: condition only checks github.repository and a successful conclusion. This still allows a successful Build Ubuntu run triggered by a fork PR to publish attacker-controlled wheel artifacts using base-repo secrets. Please add an explicit trust check (e.g., require github.event.workflow_run.event != 'pull_request' for publishing, or if PR publishing is desired then require github.event.workflow_run.head_repository.full_name == github.repository / matching repository id) so fork PRs cannot reach the publishing steps. The docs preview workflow shows a similar workflow_run pattern for fork PR safety in .github/workflows/docs-deploy-preview.yaml:30-33.
| # Publish only for PRs wihtin the canonical repository after the full build + test pipeline succeeds. | |
| if: ${{ github.repository == 'NVIDIA/IsaacTeleop' && github.event.workflow_run.conclusion == 'success' }} | |
| # Publish only for trusted runs in the canonical repository after the full build + test pipeline succeeds. | |
| # Allow non-PR workflow_run events, and allow PRs only when the head repository matches this repository. | |
| if: ${{ github.repository == 'NVIDIA/IsaacTeleop' && github.event.workflow_run.conclusion == 'success' && (github.event.workflow_run.event != 'pull_request' || github.event.workflow_run.head_repository.full_name == github.repository) }} |
| permissions: | ||
| actions: read # needed to download artifacts from the triggering workflow run | ||
|
|
||
| # Publish only for PRs wihtin the canonical repository after the full build + test pipeline succeeds. |
There was a problem hiding this comment.
Typo in comment: "wihtin" → "within".
| # Publish only for PRs wihtin the canonical repository after the full build + test pipeline succeeds. | |
| # Publish only for PRs within the canonical repository after the full build + test pipeline succeeds. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Around line 216-223: The workflow currently echoes the variable response_json
which may contain sensitive wheel URLs or release identifiers; before printing,
ensure any secrets or sensitive fields are masked by calling the GitHub Actions
mask syntax (::add-mask::) for values derived from KITMAKER_TOKEN, payload,
api_url or specific payload_items (e.g., wheel URLs/release IDs), or avoid
printing the raw response entirely; update the step so you extract the sensitive
substrings from response_json (or the original payload variables) and call
::add-mask:: for each sensitive value before the echo, or replace the echo with
a redacted/log-safe summary that does not include those fields.
- Around line 101-105: The workflow currently echoes full JSON payloads
(search_json, metadata_json) which may contain secret-backed Artifactory URIs;
update the error handling around the result_count check (variables result_count,
search_json, wheel_name) to avoid printing raw payloads by instead extracting
and echoing only non-sensitive fields (e.g., artifact name, relative path, or a
redacted download path) or by masking Artifactory host/credential segments
(replace host/credentials or remove "http(s)://...artifactory..." values) before
any echo; apply the same redaction/masking logic to the other failure branches
that echo metadata_json and related variables so no full URIs are written to the
Actions logs.
- Around line 24-25: The current `if:` guard uses `github.repository ==
'NVIDIA/IsaacTeleop'` which still passes when the triggering run was from a
fork; update the publishing guard to also verify the triggering run’s head
repository is the canonical repo and that the run succeeded. Replace the
condition at the existing `if:` check to require both
`github.event.workflow_run.conclusion == 'success'` and that
`github.event.workflow_run.head_repository.full_name == github.repository` (or
explicitly equal to 'NVIDIA/IsaacTeleop'), so the workflow only publishes when
the upstream run originated in the canonical repository rather than a forked
head. Ensure you reference and update the same `if:` expression in the release
workflow so secrets cannot be used for fork-originated `workflow_run`s.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 94f10785-81a0-44fb-a117-0539bf4f7d38
📒 Files selected for processing (2)
.github/workflows/build-ubuntu.yml.github/workflows/release.yml
💤 Files with no reviewable changes (1)
- .github/workflows/build-ubuntu.yml
| # Publish only for PRs wihtin the canonical repository after the full build + test pipeline succeeds. | ||
| if: ${{ github.repository == 'NVIDIA/IsaacTeleop' && github.event.workflow_run.conclusion == 'success' }} |
There was a problem hiding this comment.
Block fork-originated workflow_runs before publishing artifacts.
github.repository == 'NVIDIA/IsaacTeleop' is true for every run in the target repo, including pull_request runs whose head branch lives in a fork. That lets this privileged workflow download attacker-controlled wheel artifacts and publish them with deployment secrets. Gate on the triggering run’s head repository, or skip pull_request runs entirely.
Suggested guard
- if: ${{ github.repository == 'NVIDIA/IsaacTeleop' && github.event.workflow_run.conclusion == 'success' }}
+ if: >-
+ github.repository == 'NVIDIA/IsaacTeleop' &&
+ github.event.workflow_run.conclusion == 'success' &&
+ github.event.workflow_run.head_repository.full_name == github.repositoryAs per coding guidelines: .github/workflows/**: CI/CD workflows. Focus on security (secret handling, permissions) and correctness of build/test steps.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Publish only for PRs wihtin the canonical repository after the full build + test pipeline succeeds. | |
| if: ${{ github.repository == 'NVIDIA/IsaacTeleop' && github.event.workflow_run.conclusion == 'success' }} | |
| # Publish only for PRs wihtin the canonical repository after the full build + test pipeline succeeds. | |
| if: >- | |
| github.repository == 'NVIDIA/IsaacTeleop' && | |
| github.event.workflow_run.conclusion == 'success' && | |
| github.event.workflow_run.head_repository.full_name == github.repository |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release.yml around lines 24 - 25, The current `if:` guard
uses `github.repository == 'NVIDIA/IsaacTeleop'` which still passes when the
triggering run was from a fork; update the publishing guard to also verify the
triggering run’s head repository is the canonical repo and that the run
succeeded. Replace the condition at the existing `if:` check to require both
`github.event.workflow_run.conclusion == 'success'` and that
`github.event.workflow_run.head_repository.full_name == github.repository` (or
explicitly equal to 'NVIDIA/IsaacTeleop'), so the workflow only publishes when
the upstream run originated in the canonical repository rather than a forked
head. Ensure you reference and update the same `if:` expression in the release
workflow so secrets cannot be used for fork-originated `workflow_run`s.
| result_count="$(jq '.results | length' <<< "${search_json}")" | ||
| if [[ "${result_count}" -ne 1 ]]; then | ||
| echo "Expected exactly 1 Artifactory search result for ${wheel_name}, but got ${result_count}" | ||
| echo "${search_json}" | ||
| exit 1 |
There was a problem hiding this comment.
Stop echoing raw Artifactory payloads on failure.
These branches print search_json and metadata_json, which can contain full Artifactory URIs. On a lookup failure, that leaks secret-backed internal URLs into Actions logs. Log only redacted fields, or mask the Artifactory values before any echo.
Based on learnings: "In GitHub Actions workflows, avoid including secrets-containing full URLs in job outputs or other echoed fields. Instead of outputting complete download URLs, have upstream jobs output non-sensitive identifiers/relative Artifactory paths..."
Also applies to: 108-112, 119-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release.yml around lines 101 - 105, The workflow currently
echoes full JSON payloads (search_json, metadata_json) which may contain
secret-backed Artifactory URIs; update the error handling around the
result_count check (variables result_count, search_json, wheel_name) to avoid
printing raw payloads by instead extracting and echoing only non-sensitive
fields (e.g., artifact name, relative path, or a redacted download path) or by
masking Artifactory host/credential segments (replace host/credentials or remove
"http(s)://...artifactory..." values) before any echo; apply the same
redaction/masking logic to the other failure branches that echo metadata_json
and related variables so no full URIs are written to the Actions logs.
| echo "Posting ${#payload_items[@]} wheel(s) to Kitmaker in a single request" | ||
| response_json="$(curl --fail-with-body --show-error --silent --location --connect-timeout 10 --max-time 120 \ | ||
| -X POST "${api_url}" \ | ||
| -H "Authorization: Bearer ${KITMAKER_TOKEN}" \ | ||
| -H "Content-Type: application/json" \ | ||
| -d "${payload}")" | ||
| echo "${response_json}" | ||
|
|
There was a problem hiding this comment.
Mask before logging the Kitmaker response body.
This step echoes response_json without any prior ::add-mask:: calls. If the API reflects submitted wheel URLs or release identifiers, those values are exposed in the workflow log. Either redact the response or mask the sensitive values before printing it.
Based on learnings: "Verify the values are actually used for masking (via ::add-mask::) and that masking happens before any potential logging/printing of response bodies."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release.yml around lines 216 - 223, The workflow currently
echoes the variable response_json which may contain sensitive wheel URLs or
release identifiers; before printing, ensure any secrets or sensitive fields are
masked by calling the GitHub Actions mask syntax (::add-mask::) for values
derived from KITMAKER_TOKEN, payload, api_url or specific payload_items (e.g.,
wheel URLs/release IDs), or avoid printing the raw response entirely; update the
step so you extract the sensitive substrings from response_json (or the original
payload variables) and call ::add-mask:: for each sensitive value before the
echo, or replace the echo with a redacted/log-safe summary that does not include
those fields.
Summary by CodeRabbit